Clashes due to multiple Set-Cookie headers

[MarkTsengTW]

Hi, first time submitting an issue on Github so I hope this makes sense!

The function _parse_headers appears to handle multiple headers with the same name by successively overwriting them, leaving only the final header with that name.

I'm trying to read the headers from a requests.post() response where the response included two 'Set-Cookie' headers.

The headers are:

Set-Cookie: JSESSIONID=...
Set-Cookie: SERVERID=...

This works on standard Python requests when accessing response.headers, and I've verified using Wireshark that the correct headers are being sent to the CircuitPython board too.

However, on CircuitPython, response.headers only yields:

Set-Cookie: SERVERID=...

I'm assuming that because _headers is a dictionary and duplicate keys aren't allowed, _parse_headers doesn't support multiple cookies. I wish I could suggest a solution to this but as a beginner it's beyond me!

I'm not sure if this issue affects requests.session. I came across it working with the Adafruit Matrix Portal, where requests.session isn't supported.

[MarkTsengTW]

After tea and meditation I have arrived at an answer.

The problematic area is:

            if title and content:
                # enforce that all headers are lowercase
                title = str(title, "utf-8").lower()
                content = str(content, "utf-8")
                if title == "content-length":
                    self._remaining = int(content)
                if title == "transfer-encoding":
                    self._chunked = content.strip().lower() == "chunked"
                self._headers[title] = content

The following works for my project:

            if title and content:
                # enforce that all headers are lowercase
                title = str(title, "utf-8").lower()
                content = str(content, "utf-8")
                if title == "content-length":
                    self._remaining = int(content)
                if title == "transfer-encoding":
                    self._chunked = content.strip().lower() == "chunked"
                if title == "set-cookie":
                    if "set-cookie" in self._headers:
                        self._headers["set-cookie"] = self._headers["set-cookie"] + ", " + content
                    else:
                        self._headers[title] = content

It checks for an existing set-cookie header and, if one exists, appends the next cookie to the previous one, separated by a comma.

I do not know if this breaks other projects or other parts of the library. Another issue is that it does not behave the same way as requests, preserving the headers as originally sent (which the user would reasonably expect). Finally, I am not sure if the code is the most efficient or correct way to get the result.

[askpatrickw]

@MarkTsengTW you seem to be doing a great job narrowing it down, do you have a suggested fix?

[MarkTsengTW]

Hi @askpatrickw Thanks for the reply. I have a fix that works for my project - could you look it over?

My code appends cookies separated by a commas as follows:

                if title == "set-cookie":
                    if "set-cookie" in self._headers:
                        self._headers["set-cookie"] = self._headers["set-cookie"] + ", " + content
                    else:
                        self._headers[title] = content

This is a kludge but it mimics the behaviour of Python requests when it comes to response.headers['set-cookie'] and response.headers.items(), so it's enough to get my project working. I am not confident enough to say whether it would improve or break other uses of adafruit_requests though, especially sessions. I would like to do some testing but don't have hardware that's compatible with sessions.

For what it's worth, I'm now firmly of the view this is a genuine issue. I've done some research and discovered that multiple set-cookie headers are correct under RFC 6265 and the relevant Mozilla documentation. As long as adafruit_requests stores headers in a simple python dictionary then it's impossible to handle multiple cookies correctly.

I suppose that the 'correct' way to do it would be to store headers as multiple dictionaries in a list, but that would require extensive work and it would add significantly to the size of the module. I can see why it is the way it is currently.

[askpatrickw]

Best way to get feedback is to submit a PR so we can clearly see the diffs, make suggestions and give it a test run.

[tekktrik]

I was looking into this a little bit ago, and yeah, I agree comma+space separated is probably a good solution (and what CPython does as well, like you mentioned).

The people that typically work on the core may have more information or thoughts, but since this library gets frozen into the firmware for a few boards, than my money is that a change to add cookies as a comma+space separated string is going to use less memory than trying to implement something like the cookie property or CookieJar that CPython implements.

I agree with @askpatrickw - a PR for people to review is going to be the best path forward!

[MarkTsengTW]

Thanks for the encouragement @askpatrickw and @tekktrik. I've made a pull request here with an improved version of my code.

[tekktrik]

Hey @MarkTsengTW, I followed up on your PR about submitting it the this repository, let us know if you have any questions!

[FoamyGuy]

resolved by #108