Skip to content

downgrade a public-suffix Domain cookie to host-only - #9691

Open
basavaraj-sm05 wants to merge 2 commits into
lysine-dev:mainfrom
basavaraj-sm05:cookie-public-suffix-host-only
Open

downgrade a public-suffix Domain cookie to host-only#9691
basavaraj-sm05 wants to merge 2 commits into
lysine-dev:mainfrom
basavaraj-sm05:cookie-public-suffix-host-only

Conversation

@basavaraj-sm05

Copy link
Copy Markdown
Contributor

When a Set-Cookie header carries a Domain attribute, Cookie.parse checks that the domain isn't a public suffix, so a server can't scope a cookie to something like github.io or a *.elb.amazonaws.com label and have it ride along to every host underneath. The problem is that the check is gated on urlHost.length != domain.length, so it only runs when the Domain is a proper suffix of the request host and is skipped whenever the Domain equals the host exactly. A response from a host that is itself a public suffix slips right through: https://github.io/ returning Set-Cookie: a=b; Domain=github.io never reaches the public-suffix lookup, hostOnly stays false, and the cookie then matches every *.github.io host in Cookie.matches. RFC 6265 section 5.3 covers this exact case by turning the cookie into a host-only cookie rather than a shared domain cookie. I ran into it reading the length comparison next to the existing domainIsPublicSuffix test, which only walks the proper-suffix path and never the equal-length one. The fix runs the public-suffix lookup whenever a Domain is present, still refuses a proper-suffix public suffix as before, and marks the cookie host-only when the domain equals the host; registrable domains like example.com are untouched because their eTLD+1 is non-null.

@swankjesse swankjesse left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Great analysis. One hazard with this code change is it causes us to evaluate PublicSuffixDatabase.get() more eagerly, and that’s a potentially expensive operation.

Will investigate

@basavaraj-sm05

Copy link
Copy Markdown
Contributor Author

You're right, and it was actually worse than the diff suggests: since domain defaults to urlHost when no Domain attribute is present, the patch as written ran the lookup for every cookie, Domain or not. I've pushed a change that nests the public-suffix check inside the Domain branch, so parses without a Domain attribute never touch PublicSuffixDatabase, same as today. The remaining delta from main is the Domain-equals-host case, and I don't see a way around that one: deciding whether to downgrade to host-only means asking whether the host is a public suffix, so a server that echoes its own host in Domain now pays the one-time list load it previously skipped. After that it's just a few binary searches per parse.

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

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants