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

RFC6265bis: Update 'Storage Model' to include more character set restrictions / processing #1593

Open
recvfrom opened this issue Aug 6, 2021 · 4 comments
Assignees

Comments

@recvfrom
Copy link
Contributor

recvfrom commented Aug 6, 2021

Per @chlily1's comment in #1210 (comment), it'd be worth updating the 'Storage Model' section to add restrictions on cookie-name, cookie-value, and the attribute values to account for characters that would break serialization... Specifically, we should add that:

  • cookie-name should not contain = or ;.
  • cookie-value should not contain ;
  • the attribute values should not contain ;

Although it's not possible for those characters to appear in cookies constructed by parsing Set-Cookie headers, these values could be introduced via cookies from non-HTTP APIs.

Also, should there be a step in the Storage Model that removes leading and trailing whitespace from cookie-name, cookie-value, and the attribute values (to further close the gap between what can be created via Set-Cookie header parsing and via non-HTTP APIs)?

(also per @chlily1's comment, a better way to mitigate this would be to extract out the logic common to the Set-Cookie header parsing section and the Storage Model section and reference that in both places)

@recvfrom
Copy link
Contributor Author

recvfrom commented Aug 9, 2021

Hmm, another difference is that in the Storage Model section, the control character checks only apply to cookie-name and cookie-value, but since the corresponding checks in the Set-Cookie Header Field section apply to the entire cookie-line they implicitly cover the attribute values as well. Should the Storage Model section be updated such that the control character checks also apply to the attribute values?

@recvfrom
Copy link
Contributor Author

One thing that complicates the Storage Model section, especially when considering cookies set via non-HTTP APIs like Cookie Store API, is that attribute values aren't direct inputs to the Storage Model section and are instead expected to be extracted from entries in the cookie-attribute-list. The Storage Model section, then, needs steps to skip invalid entries present in the list, and many of those same steps are already present in the Set-Cookie Header section to prevent invalid entries from being added to the cookie-attribute-list in the first place. Also, for non-HTTP APIs that allow each attribute value to be specified directly, it's unclear how or if the steps to parse the cookie-attribute-list should apply (for example, the attribute value length check).

It might make more sense to replace cookie-attribute-list with something like cookie-attribute-dict, where each key present maps to only one cookie attribute value. The existing logic in the Storage Model section for choosing the right entry would need to get moved into the Set-Cookie Header section, but then it'd be simpler to apply the other Storage Model rules to attribute values from both HTTP and non-HTTP cookie APIs.

@miketaylr
Copy link
Collaborator

It might make more sense to replace cookie-attribute-list with something like cookie-attribute-dict, where each key present maps to only one cookie attribute value. The existing logic in the Storage Model section for choosing the right entry would need to get moved into the Set-Cookie Header section, but then it'd be simpler to apply the other Storage Model rules to attribute values from both HTTP and non-HTTP cookie APIs.

I think this particular change would be a nice improvement, but probably out of scope for 6265bis, especially if we're trying to focus on interoperability (given that Cookie Store is only implemented in one browser for now).

@miketaylr
Copy link
Collaborator

Let's punt on this for now.

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

No branches or pull requests

3 participants