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

RFC Compliant Cookie Parsing #2540

Merged
merged 7 commits into from Mar 11, 2021
Merged

RFC Compliant Cookie Parsing #2540

merged 7 commits into from Mar 11, 2021

Conversation

t-ae
Copy link
Contributor

@t-ae t-ae commented Dec 14, 2020

⚠️ Vapor now has stricter cookie parsing. If you're relying on non-standard cookie parsing (like non-ascii characters) you'lll need to migrate off before updating ⚠️

Changes cookie name parsing to conform to RFC6265 and RFC2616.

  • Some characters (!"#$%&'*+^`) are newly allowed
  • Non-ASCII characters are banned

#2540

@t-ae
Copy link
Contributor Author

t-ae commented Dec 14, 2020

This PR contains the improvements I suggested in #2511.

I also renamed isDirectiveKey to isTokenCharacter to clarify the definition is from RFC.

I afraid that banning non-ASCII characters can cause problem.
isTokenCharacter is used in parseDirectives, and parseDirectives is also used for other headers.
As far as I can see, none of them allows non-ASCII character as directive key. Can anyone verify it?

Here's the list of headers which use parseDirectives:

Copy link
Member

@0xTim 0xTim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@0xTim 0xTim added the semver-patch Internal changes only label Mar 5, 2021
@0xTim 0xTim changed the title Changes cookie name parsing to strictly conform to RFC RFC Compliant Cookie Parsing Mar 5, 2021
@0xTim 0xTim merged commit 062a408 into vapor:master Mar 11, 2021
@tanner0101
Copy link
Member

These changes are now available in 4.41.4

@t-ae t-ae deleted the cookie-rfc branch March 18, 2021 00:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-patch Internal changes only
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants