Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Last set-cookie headers on a page overwrites the ones before #43

Closed
erikthorselius opened this issue Mar 14, 2022 · 11 comments
Closed

Last set-cookie headers on a page overwrites the ones before #43

erikthorselius opened this issue Mar 14, 2022 · 11 comments

Comments

@erikthorselius
Copy link

erikthorselius commented Mar 14, 2022

If a homepage has multiple set-cookes headers it is only the last set-cookie is in the response :header map.

@borkdude
Copy link
Collaborator

borkdude commented Mar 14, 2022

@erikthorselius Can you give a more concrete example which allows us to repro/test and fix the problem?

@borkdude
Copy link
Collaborator

I guess the problem happens here:

[status (assoc parsed-headers (str/lower-case k) (str/trim v))]

Should we instead of creating a map, create a seq of map-entries/vectors? It will be a breaking change, but maybe for the good? Let's investigate how other clients do this (e.g. clj-http).

/cc @lispyclouds

@borkdude
Copy link
Collaborator

borkdude commented Mar 14, 2022

clj-http-lite:

https://github.com/clj-commons/clj-http-lite/blob/6b53000df55ac05c4ff8e5047a5323fc08a52e8b/src/clj_http/lite/core.clj#L10-L15

If a name appears more than once (like set-cookie) then the value
will be a vector containing the values in the order they appeared
in the headers.

@borkdude
Copy link
Collaborator

@borkdude
Copy link
Collaborator

clj-http, same as clj-http-lite:

https://github.com/dakrone/clj-http/blob/5f0f842f330201cdfbeedcde781bb62cbdd018b8/src/clj_http/core.clj#L33-L35

I guess we would be safe doing the same as people should already be relatively used to this behavior and we would not break anything.

@borkdude
Copy link
Collaborator

@borkdude
Copy link
Collaborator

Let's go ahead with the clj-http/lite approach.

@erikthorselius
Copy link
Author

erikthorselius commented Mar 15, 2022

Wow you are fast!

Sorry for my short description and it looks like you figured it out. I tried to get a session cookie from the headers but it was only one set-cookie header in the headers map. But if I curl the page directly it was two set-cookie headers and the session was the first so it was overwritten by the second.

@lispyclouds
Copy link
Member

Sounds good to me too! Would be consistent with the query params impl now 😄

@borkdude
Copy link
Collaborator

Would be consistent with the query params impl now

How's that?

@lispyclouds
Copy link
Member

lispyclouds commented Mar 15, 2022

Ah yeah I misread the clj-http impl as the way we handle the same named query params by passing a vector of vectors. But now that I read it again its different. Also this is about the response headers not the request ones.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants