Skip to content

Switch to a more lenient cookie parsing method#900

Merged
lovelydinosaur merged 3 commits intoKludex:masterfrom
erewok:lenient_cookie_parsing
Apr 23, 2020
Merged

Switch to a more lenient cookie parsing method#900
lovelydinosaur merged 3 commits intoKludex:masterfrom
erewok:lenient_cookie_parsing

Conversation

@erewok
Copy link
Copy Markdown
Contributor

@erewok erewok commented Apr 15, 2020

This PR is for issue #898, requesting a more lenient cookie-parsing technique for Starlette. @tomchristie requested a failing test to start, so that's what has been included here. I reused the example from my issue. Happy to modify or update as desired by the maintainers.

@erewok
Copy link
Copy Markdown
Contributor Author

erewok commented Apr 15, 2020

Also, for reference, here's the latest spec I have found, RFC 6265, which obsoletes the prior spec. This document offers the following parsing syntax:

   cookie-header = "Cookie:" OWS cookie-string OWS
   cookie-string = cookie-pair *( ";" SP cookie-pair )
   cookie-pair       = cookie-name "=" cookie-value
   cookie-name       = token
   cookie-value      = *cookie-octet / ( DQUOTE *cookie-octet DQUOTE )
   cookie-octet      = %x21 / %x23-2B / %x2D-3A / %x3C-5B / %x5D-7E
                       ; US-ASCII characters excluding CTLs,
                       ; whitespace DQUOTE, comma, semicolon,
                       ; and backslash
 token             = <token, defined in [RFC2616], Section 2.2>

According to this spec, the Starlette cookie parser (and Python's http.cookies.SimpleCookie.load) are not properly handling cookies.

@erewok
Copy link
Copy Markdown
Contributor Author

erewok commented Apr 15, 2020

I also found a related discussion in Tornado, where they also copied what Django is doing after finding that SimpleCookie would not work for the project.

@erewok erewok force-pushed the lenient_cookie_parsing branch from 29d0a28 to 6b971d5 Compare April 15, 2020 15:53
@erewok
Copy link
Copy Markdown
Contributor Author

erewok commented Apr 15, 2020

I can add all of the tests they used in Tornado as well, which should give pretty broad coverage of various scenarios.

@erewok
Copy link
Copy Markdown
Contributor Author

erewok commented Apr 15, 2020

I have a working version of cookie parsing which will pass all of these tests. I am stuck on mypy telling me the following:

starlette/requests.py:50: error: Module has no attribute "_unquote"
Found 1 error in 1 file (checked 32 source files)

We have two options here, I believe:

  • Ignore this mypy error, or
  • Pull the entirety of this _unquote function from the Python standard library into this codebase (this is what Tornado did, but not for mypy: they write that they "did not want to have to depend on non-public interfaces", which seems like a reasonable opinion to me, honestly).

I will wait to commit and push what I have until feedback from the maintainers on proceeding with this PR. I believe we have a pretty comprehensive perspective on what other major Python projects are doing in this area and what the typical path is and so I think this is the right approach for Starlette.

Feedback welcome.

@erewok
Copy link
Copy Markdown
Contributor Author

erewok commented Apr 21, 2020

I decided to push my changes so they can be compared with other solutions. I would appreciate resolving this in Starlette, so we can pull our own custom cookie-parsing out of our projects. Let me know what you think.

@erewok
Copy link
Copy Markdown
Contributor Author

erewok commented Apr 21, 2020

I can't reproduce this failing test locally and it's unrelated to any modules that I modified. Any suggestions?

@lovelydinosaur
Copy link
Copy Markdown

Seems great yup!

I'll have to look into the failing database test separately.
Ended up not being noticed previously as travis wasn't reporting correctly - will look into switching to GitHub actions for CI too.

@lovelydinosaur
Copy link
Copy Markdown

Let me know what you think.

Seems good to merge, right? (Or did I misunderstand something?)

@erewok
Copy link
Copy Markdown
Contributor Author

erewok commented Apr 22, 2020

Yes, I thought it was ready but that failing test really threw me off! If you're sure it's unrelated, then I think it's ready to merge. (This PR closes #898)

@erewok erewok deleted the lenient_cookie_parsing branch April 23, 2020 15:17
@Orenoid Orenoid mentioned this pull request Sep 2, 2024
3 tasks
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

Successfully merging this pull request may close these issues.

2 participants