-
Notifications
You must be signed in to change notification settings - Fork 35
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
Allow tab characters in cookies #204
Conversation
Hmm, after reading the latest version of the spec again, the changes in this PR don't exactly line up (likely due to an oversight)... I'll update this PR so that it aligns with what's in the spec today |
Are there WPT changes that correspond to this? |
I just submitted the corresponding WPT changes for review - https://chromium-review.googlesource.com/c/chromium/src/+/3084521 |
@@ -1103,7 +1103,7 @@ optional |expires|, | |||
|sameSite|, | |||
run the following steps: | |||
|
|||
1. If |name|, |value|, |domain|, or |path| contain U+003B (`;`), any [=C0 control=] character, or U+007F, then return failure. | |||
1. If |name| or |value| contain U+003B (`;`), any [=C0 control=] character except U+0009 (the horizontal tab character), or U+007F, then return failure. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens to validation for domain and path?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a comment here about this - httpwg/http-extensions#1593 (comment)
In the RFC, the character restrictions apply to attribute values derived from Set-Cookie headers, but in the Storage Model
section it only says that these character checks should happen for name
and value
... I think this may be more of an oversight than intentional, but regardless, my thinking with the change above is that this spec should match what's in the RFC today and then when/if the character restrictions are made to apply to attribute values as well, we can update it here also. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SGTM, but I'd add a TODO: ...
or Issue: ...
block to the spec referencing the issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Since the expires and sameSite attributes aren't passed through from script as strings I don't think those are a concern.
SHA: 0c572ae Reason: push, by @inexorabletash Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Fixes #203 - Allow tab characters in cookies
Also, applies the invalid character checks to all other attribute values as well for added clarity (even if they are less likely to contain invalid characters due to other restrictions on the types for those)Preview | Diff