Skip to content

Cookie layering - Http prefix #3110

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

Open
wants to merge 17 commits into
base: main
Choose a base branch
from

Conversation

yoavweiss
Copy link
Contributor

@yoavweiss yoavweiss commented Jun 16, 2025

@yoavweiss yoavweiss requested review from johannhof and annevk June 16, 2025 15:59
@yoavweiss
Copy link
Contributor Author

Given https://lists.w3.org/Archives/Public/ietf-http-wg/2025AprJun/0188.html, maybe we want to land __HttpOnly first while we figure out what the compounding mechanism here should be?

@reschke reschke added the cookies Issues with the layered-cookies draft. label Jun 17, 2025
@annevk
Copy link

annevk commented Jun 18, 2025

It might be good to open an issue to discuss further, but my inclination would be that we turn this into some kind of table of prefix strings and their corresponding restrictions down the line. I don't think the combinatorial explosion is all that bad and allowing them in arbitrary order would in fact turn a simple prefix match into a parser question with all the resulting issues that come with that.

@yoavweiss
Copy link
Contributor Author

Yeah, there's a discussion happening on that front in https://lists.w3.org/Archives/Public/ietf-http-wg/2025AprJun/0192.html

I think we can split this into two separate things:

  1. Add an HttpOnly (or Http) prefix
  2. Figure out a way to combine multiple prefixes

If that makes sense, I'm happy to turn this into (1) and open an issue to further discuss (2)

Copy link

@annevk annevk left a comment

Choose a reason for hiding this comment

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

Unfortunately this will also need a corresponding PR to the Cookie Store API. Perhaps if we eventually abstract this convention somehow we can improve that.

@annevk
Copy link

annevk commented Jun 18, 2025

I like the idea of the prefix being just Http for brevity's sake. I also think we should have an issue for the multiple discussion as it'll come back.

@yoavweiss yoavweiss changed the title Cookie layering - HttpOnly and HostHttpOnly prefixes Cookie layering - HttpOnly prefix Jun 18, 2025
@yoavweiss yoavweiss changed the title Cookie layering - HttpOnly prefix Cookie layering - Http prefix Jun 18, 2025
@yoavweiss
Copy link
Contributor Author

I like the idea of the prefix being just Http for brevity's sake. I also think we should have an issue for the multiple discussion as it'll come back.

Revamped this PR to just handle the Http prefix, and opened #3111 for further discussion on prefix combinatorics.

@annevk
Copy link

annevk commented Jun 18, 2025

I added a checklist to OP for what remains to be done. I suppose we also need to make some assessment as to whether this has broad enough support, but I think it has from the list discussion.

@yoavweiss
Copy link
Contributor Author

@annevk - I tried to split out the conditions to separate algorithms, to make it easier/clearer when combining them for HostHttp. Let me know what you think.

@yoavweiss yoavweiss requested a review from annevk June 20, 2025 06:58
Copy link

@annevk annevk left a comment

Choose a reason for hiding this comment

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

I like it. I'm half-tempted to also suggest introducing a list of known prefixes so we can deduplicate some of that across specifications, but I think this is sufficient for now. At some point the abstractions might also impede clarity.

yoavweiss and others added 6 commits June 20, 2025 10:38
Co-authored-by: Anne van Kesteren <annevk@annevk.nl>
Co-authored-by: Anne van Kesteren <annevk@annevk.nl>
Co-authored-by: Anne van Kesteren <annevk@annevk.nl>
Co-authored-by: Anne van Kesteren <annevk@annevk.nl>
Co-authored-by: Anne van Kesteren <annevk@annevk.nl>
yoavweiss and others added 2 commits June 20, 2025 10:43
Co-authored-by: Anne van Kesteren <annevk@annevk.nl>
Co-authored-by: Anne van Kesteren <annevk@annevk.nl>
@yoavweiss yoavweiss requested a review from annevk June 20, 2025 08:44

1. _cookie_'s host-only is true; and

1. _cookie_'s path's size is 1 and _cookie_'s path[0] is the empty string,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I realize you're inheriting this from the cookie layering draft, but is the empty string correct?

Above (in prose) this is described as:

Path attribute with a value of /

(that's also what 6265bis has in 5.7.21.3)

Copy link
Contributor Author

@yoavweiss yoavweiss Jun 20, 2025

Choose a reason for hiding this comment

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

In case there's a Path attribute, I see the following algorithm:

1. If _attributeName_ is a byte-case-insensitive match for `Path`:
    1. If _attributeValue_ is not empty and if the first byte of  _attributeValue_ is 0x2F (/), then set _cookie_'s path to _attributeValue_  split on 0x2F (/).

So I think a list of size 1 with an empty string is correct here.

In case there's no Path attribute, we fall back to the Cookie Default Path algorithm which I think intends to do the same thing and set the path as a list of size 1 with an empty string if the path contains only a single /.

At the same time, that algorithm operates on a list (and currently receives a URL). I'm also not sure if we should accept a __Host prefix if the cookie doesn't include an explicit Path=/ attribute.

Copy link

@annevk annevk Jun 20, 2025

Choose a reason for hiding this comment

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

We should not, according to bis: https://httpwg.org/http-extensions/draft-ietf-httpbis-rfc6265bis.html#section-5.7

However, that means we have to keep additional data around that we're currently not. We need to store some information about the attributes on the parsed cookie in some manner. That's unfortunate. #3112

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm also not sure if we should accept a __Host prefix if the cookie doesn't include an explicit Path=/ attribute.

RFC 6265bis suggests we shouldn't, but I'm not sure there's a way to make that distinction ATM. I can add a default_path flag that would enable it

Copy link

Choose a reason for hiding this comment

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

I think it would be more natural to add a field to cookie along the lines of "had explicit path" that we then check here in addition to checking that it had the correct value.

Alternatively we could maybe compute ahead of time whether it's a host-prefix cookie and store that as a bit during parsing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a path-attribute boolean. Let me know if this makes sense (and if this should be split to a separate PR, as it impacts __Host)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(I'm on train wifi, so I made the above comments oblivious to your comments. Apologies for any incoherence)

yoavweiss and others added 3 commits June 20, 2025 17:03
Co-authored-by: Mike Taylor <miketaylr@google.com>
Co-authored-by: Mike Taylor <miketaylr@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cookies Issues with the layered-cookies draft.
Development

Successfully merging this pull request may close these issues.

4 participants