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 empty domain attribute #1332

Closed
essen opened this issue Nov 25, 2020 · 11 comments
Closed

rfc6265bis empty domain attribute #1332

essen opened this issue Nov 25, 2020 · 11 comments
Assignees
Labels

Comments

@essen
Copy link
Contributor

essen commented Nov 25, 2020

The behavior for when the domain attribute is empty is confusing when there are multiple domain attributes.

The spec says:

   If the attribute-name case-insensitively matches the string "Domain",
   the user agent MUST process the cookie-av as follows.

   1.  If the attribute-value is empty, the behavior is undefined.
       However, the user agent SHOULD ignore the cookie-av entirely.

This means that when there's a single empty domain attribute, and the implementation follows this SHOULD, then this attribute is removed. No problem there. But when there's two attributes including an empty domain attribute, then only the empty attribute is removed. This means that the three following set-cookie headers are equivalent:

Set-Cookie: foo=bar; domain=foo.example.org; domain=
Set-Cookie: foo=bar; domain=; domain=foo.example.org
Set-Cookie: foo=bar; domain=foo.example.org

When the domain is foo.example.org all 3 cookies end up stored. When the domain is home.example.org all 3 are dropped.

The first case is confusing because it is in stark contrast with the path attribute where an empty path equals default-path and the final path wins, so an empty final path has the effect of "resetting" the path. I would also suggest the first case is not intuitive.

An implementation that does not follow the SHOULD would have undefined behavior. This is the only place in the spec where this is the case I believe. It would be good to get rid of it.

I would suggest first changing the SHOULD into a MUST so as to avoid differing behaviors. I would also suggest either having behavior similar to path, or invalidating the cookie entirely.

(This is a followup to #1136 and the relevant http-state test was as follow:)

Set-Cookie: foo=bar; domain=foo.example.org; domain=
Location: http://home.example.org:8888/cookie-parser-result?optional-domain0042
@miketaylr
Copy link
Collaborator

This test now lives at https://wpt.fyi/results/cookies/attributes/domain/domain.sub.html?label=master&label=experimental&aligned

https://github.com/web-platform-tests/wpt/blob/489679e4fccedcb37ae4d3e028b5d1a56adbc769/cookies/attributes/resources/domain-child.sub.html#L311-L316

        {
          cookie: `test=42; domain=${www1Host}; domain=`,
          expected: "",
          name: "No cookie returned for domain mismatch (with domain mismatch as first domain attribute and second a bare attribute)",
          location: `http://${wwwHost}:${port}/cookies/attributes/resources/path.html`,
        },

The test works like so:

  1. open the test page to http://home.example.org/path/to/file (using example.org here instead of the variables so it's less confusing)
  2. (attempt to) set the following cookie: test=42; domain=sibling.example.org; domain=; path=/cookies/attributes/resources
  3. The server that sets the cookie will 302 to http://home.example.org/cookies/attributes/resources/path.html
  4. The test page sees if any cookies are sent

There's disagreement between Chrome (which doesn't set a cookie) and Firefox & Safari (which do) here.

I would suggest first changing the SHOULD into a MUST so as to avoid differing behaviors. I would also suggest either having behavior similar to path, or invalidating the cookie entirely.

It does seem a little strange that some browsers would send a cookie, I'm not sure if that could be abused though -- if you can modify a domain attribute to make it empty, you can modify it to whatever you want.

Do we keep it as SHOULD? Change to MUST? Thoughts @chlily1 @englehardt?

@chlily1
Copy link
Contributor

chlily1 commented Feb 3, 2021

I believe the correct behavior (according to the SHOULD) should be to not store a cookie, since the empty domain= attribute should be ignored, leaving the last valid one (domain=sibling.example.org) which doesn't match the current host. I wouldn't mind changing this to a MUST; it seems in line with the definition of "MUST" in RFC 2119.

Agreed that it's weird the Domain attribute behaves like this. In Chromium's parsing code, there's a special case for empty Domain attributes, different from all the other attributes.

The value of ignoring an empty domain attribute might have been to avoid possible ambiguity in the correct behavior of an empty domain attribute? I don't like either of the alternatives suggested above. Having behavior similar to path (where an empty attribute means filling in a default, in this case the current host) might be dangerous, as it could lead to unintentionally sending the cookie to subdomains which may be less-secure origins. Invalidating the cookie entirely is inconsistent with what happens when other attributes end up with nonsensical values (see issue #933).

I think the most sensible thing is to either continue to ignore empty domain attributes (and make the SHOULD a MUST), or to say that cookies with an empty domain attribute as the last domain attribute behave as host-only cookies (effectively treating domain= as an explicit marker of a host-only cookie). These two are different, e.g. in the case of domain=foo.example;domain=, which currently ends up as a domain cookie for foo.example, but under the second proposal would end up as a host-only cookie.

@chlily1
Copy link
Contributor

chlily1 commented Feb 3, 2021

There's disagreement between Chrome (which doesn't set a cookie) and Firefox & Safari (which do) here.

(Do we know if Firefox/Safari treat it as a host cookie, or do they fill in some default value for the domain attribute?)

@chlily1 chlily1 self-assigned this Feb 23, 2021
@sbingler sbingler assigned miketaylr and unassigned chlily1 Sep 14, 2021
@miketaylr
Copy link
Collaborator

miketaylr commented Oct 1, 2021

(Do we know if Firefox/Safari treat it as a host cookie, or do they fill in some default value for the domain attribute?)

Gah, it took me a minute to figure out what was happening, and why Firefox & Safari would send a cookie, but Chrome wouldn't. It turns out it was a typo:

{
  cookie: `test=43 domain=${www1Host}; domain=`,
  expected: "",
  name: "No cookie returned for domain mismatch (first attribute is a different subdomain and second is bare)",
  location: `http://${www2wwwHost}:${port}/cookies/attributes/resources/path.html`,
},

There should be a semi-colon between 43 and domain. 🙈

(edit: the typo shouldn't really account for the difference here, I think?)

@miketaylr
Copy link
Collaborator

miketaylr commented Oct 1, 2021

I should have just looked at the source... https://source.chromium.org/chromium/chromium/src/+/main:net/cookies/parsed_cookie.cc;l=770?q=parsedcookie

Screen Shot 2021-10-01 at 6 57 11 PM

That check was added in 2016, to be more compatible with other UAs, in theory.

From the bug:

Set cookie: 
"foo=bar; domain=foo.example.org; domain="

Results URL: 
http://home.example.org:8888/cookie-parser-result?optional-domain0042

GOOD (Spec, Safari, IE, Edge):  ""
BAD (Chrome, Firefox): "foo=bar"

Those results don't map to what I currently see (but IE and Edge are dead and Chromium now, respectively).

For a test like, which is opened on wwwHost:

{
          cookie: `test=43; domain=${www1Host}; domain=`,
          expected: "test=43",
          name: "No cookie returned for domain mismatch (first attribute is a different subdomain and second is bare)",
          location: `http://${wwwHost}:${port}/cookies/attributes/resources/path.html`,
},

Safari and Firefox pass, Chrome fails. Unsure what to make of the comments from 2016, but it seems like we want an empty domain attribute to be equivalent to no domain attribute at all, i.e., a host cookie (rather than falling back to the last known parsed domain attribute value).

miketaylr added a commit to miketaylr/http-extensions that referenced this issue Oct 4, 2021
@sbingler
Copy link
Collaborator

sbingler commented Oct 5, 2021

Safari and Firefox pass, Chrome fails.

Where do you see this? Both a local run and wpt.fyi show Chrome passing this test

@miketaylr
Copy link
Collaborator

Where do you see this? Both a local run and wpt.fyi show Chrome passing this test

Sorry, I could have been more clear. test=42 ("No cookie returned for domain mismatch (with domain mismatch as first domain attribute and second a bare attribute)") passes in Chrome today, and fails in Safari and Firefox - but this PR changes the behavior to match Firefox and Safari. I have a local patch that flips the assertion.

@sbingler
Copy link
Collaborator

sbingler commented Oct 5, 2021

(Do we know if Firefox/Safari treat it as a host cookie, or do they fill in some default value for the domain attribute?)

I don't have access to a Mac so I don't know what Safari does. But I did test Chrome and Firefox

Test Cases and Results

(All to be run on example.com)

Test case 1: If the cookie-av is ignored then we should have only the foo=hostonly and foo=nonhostonly cookies.

Clear all cookies on example.com then run

document.cookie = "foo=hostonly"
document.cookie = "foo=nonhostonly; domain=example.com"
document.cookie = "foo=domaintest; domain=other.com; domain="
  • Chrome 94
    • foo=hostonly; foo=nonhostonly
  • Firefox 92
    • foo=nonhostonly; foo=domaintest

Test case 2: If the cookie-av is ignored then we should have only the foo=hostonly and foo=domaintest cookies.

Clear all cookies on example.com then run

document.cookie = "foo=hostonly"
document.cookie = "foo=nonhostonly; domain=example.com"
document.cookie = "foo=domaintest; domain=example.com; domain="
  • Chrome 94
    • foo=hostonly; foo=domaintest
  • Firefox 92
    • foo=nonhostonly; foo=domaintest

Test case 3: If the cookie-av is ignored then we should have only the foo=domaintest and foo=nonhostonly cookies.

Clear all cookies on example.com then run

document.cookie = "foo=hostonly"
document.cookie = "foo=nonhostonly; domain=example.com"
document.cookie = "foo=domaintest; domain="
  • Chrome 94
    • foo=nonhostonly; foo=domaintest
  • Firefox 92
    • foo=nonhostonly; foo=domaintest

Summary

So it appears that Chrome is ignoring the cookie-av while Firefox always make the cookie host-only when it encounters domain=.
Additionally Firefox rejects cookies when it sees domain=. as the last domain attribute, Chrome doesn't seem to mind the ..

@miketaylr
Copy link
Collaborator

miketaylr commented Oct 5, 2021

Here's a WIP CL to update WPTs, I'll add some domain=. nonsense (and update the spec text) tomorrow. Thanks for the feedback so far!

edit: actual CL https://chromium-review.googlesource.com/c/chromium/src/+/3203692

@miketaylr
Copy link
Collaborator

Results from Safari for the above 3 tests:

TC1: "foo=nonhostonly; foo=domaintest" (matches Firefox)
TC2: "foo=nonhostonly; foo=domaintest" (matches Firefox)
TC3: "foo=nonhostonly; foo=domaintest" (matches Firefox)

miketaylr added a commit to miketaylr/http-extensions that referenced this issue Oct 5, 2021
miketaylr added a commit to miketaylr/http-extensions that referenced this issue Oct 6, 2021
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Oct 8, 2021
Also, add a few more tests inspired by bingler@'s GitHub comments.

Previously, the cookie spec left an empty domain attribute as
undefined behavior with SHOULD-level guidance.
httpwg/http-extensions#1709 changes the
spec to require handling of a cookie as a host cookie when its
last domain attribute is empty. This matches the current behavior
of Firefox and Safari.

See httpwg/http-extensions#1332 for
more discussion.

Change-Id: Ibf4f243c929b11768ff406e940d6988a37434754
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Oct 8, 2021
Also, add a few more tests inspired by bingler@'s GitHub comments.

Previously, the cookie spec left an empty domain attribute as
undefined behavior with SHOULD-level guidance.
httpwg/http-extensions#1709 changes the
spec to require handling of a cookie as a host cookie when its
last domain attribute is empty. This matches the current behavior
of Firefox and Safari.

See httpwg/http-extensions#1332 for
more discussion.

Change-Id: Ibf4f243c929b11768ff406e940d6988a37434754
Bug: 1255273
@chlily1 chlily1 closed this as completed in bc3fc24 Oct 8, 2021
@essen
Copy link
Contributor Author

essen commented Oct 8, 2021

Thanks for removing the undefined behavior from the spec! Much appreciated.

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Oct 12, 2021
Also, add a few more tests inspired by bingler@'s GitHub comments.

Previously, the cookie spec left an empty domain attribute as
undefined behavior with SHOULD-level guidance.
httpwg/http-extensions#1709 changes the
spec to require handling of a cookie as a host cookie when its
last domain attribute is empty. This matches the current behavior
of Firefox and Safari.

See httpwg/http-extensions#1332 for
more discussion.

Change-Id: Ibf4f243c929b11768ff406e940d6988a37434754
Bug: 1255273
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Oct 12, 2021
Also, add a few more tests inspired by bingler@'s GitHub comments.

Previously, the cookie spec left an empty domain attribute as
undefined behavior with SHOULD-level guidance.
httpwg/http-extensions#1709 changes the
spec to require handling of a cookie as a host cookie when its
last domain attribute is empty. This matches the current behavior
of Firefox and Safari.

See httpwg/http-extensions#1332 for
more discussion.

Change-Id: Ibf4f243c929b11768ff406e940d6988a37434754
Bug: 1255273
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Oct 12, 2021
Also, add a few more tests inspired by bingler@'s GitHub comments.

Previously, the cookie spec left an empty domain attribute as
undefined behavior with SHOULD-level guidance.
httpwg/http-extensions#1709 changes the
spec to require handling of a cookie as a host cookie when its
last domain attribute is empty. This matches the current behavior
of Firefox and Safari.

See httpwg/http-extensions#1332 for
more discussion.

Change-Id: Ibf4f243c929b11768ff406e940d6988a37434754
Bug: 1255273
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Oct 13, 2021
Also, add a few more tests inspired by bingler@'s GitHub comments.

Previously, the cookie spec left an empty domain attribute as
undefined behavior with SHOULD-level guidance.
httpwg/http-extensions#1709 changes the
spec to require handling of a cookie as a host cookie when its
last domain attribute is empty. This matches the current behavior
of Firefox and Safari.

See httpwg/http-extensions#1332 for
more discussion.

Change-Id: Ibf4f243c929b11768ff406e940d6988a37434754
Bug: 1255273
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Oct 14, 2021
Also, add a few more tests inspired by bingler@'s GitHub comments.

Previously, the cookie spec left an empty domain attribute as
undefined behavior with SHOULD-level guidance.
httpwg/http-extensions#1709 changes the
spec to require handling of a cookie as a host cookie when its
last domain attribute is empty. This matches the current behavior
of Firefox and Safari.

See httpwg/http-extensions#1332 for
more discussion.

Change-Id: Ibf4f243c929b11768ff406e940d6988a37434754
Bug: 1255273
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3203692
Reviewed-by: Steven Bingler <[email protected]>
Commit-Queue: Mike Taylor <[email protected]>
Cr-Commit-Position: refs/heads/main@{#931471}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Oct 14, 2021
Also, add a few more tests inspired by bingler@'s GitHub comments.

Previously, the cookie spec left an empty domain attribute as
undefined behavior with SHOULD-level guidance.
httpwg/http-extensions#1709 changes the
spec to require handling of a cookie as a host cookie when its
last domain attribute is empty. This matches the current behavior
of Firefox and Safari.

See httpwg/http-extensions#1332 for
more discussion.

Change-Id: Ibf4f243c929b11768ff406e940d6988a37434754
Bug: 1255273
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3203692
Reviewed-by: Steven Bingler <[email protected]>
Commit-Queue: Mike Taylor <[email protected]>
Cr-Commit-Position: refs/heads/main@{#931471}
blueboxd pushed a commit to blueboxd/chromium-legacy that referenced this issue Oct 14, 2021
Also, add a few more tests inspired by bingler@'s GitHub comments.

Previously, the cookie spec left an empty domain attribute as
undefined behavior with SHOULD-level guidance.
httpwg/http-extensions#1709 changes the
spec to require handling of a cookie as a host cookie when its
last domain attribute is empty. This matches the current behavior
of Firefox and Safari.

See httpwg/http-extensions#1332 for
more discussion.

Change-Id: Ibf4f243c929b11768ff406e940d6988a37434754
Bug: 1255273
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3203692
Reviewed-by: Steven Bingler <[email protected]>
Commit-Queue: Mike Taylor <[email protected]>
Cr-Commit-Position: refs/heads/main@{#931471}
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Oct 28, 2021
…t 6265bis update., a=testonly

Automatic update from web-platform-tests
Update some domain tests to follow latest 6265bis update.

Also, add a few more tests inspired by bingler@'s GitHub comments.

Previously, the cookie spec left an empty domain attribute as
undefined behavior with SHOULD-level guidance.
httpwg/http-extensions#1709 changes the
spec to require handling of a cookie as a host cookie when its
last domain attribute is empty. This matches the current behavior
of Firefox and Safari.

See httpwg/http-extensions#1332 for
more discussion.

Change-Id: Ibf4f243c929b11768ff406e940d6988a37434754
Bug: 1255273
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3203692
Reviewed-by: Steven Bingler <[email protected]>
Commit-Queue: Mike Taylor <[email protected]>
Cr-Commit-Position: refs/heads/main@{#931471}

--

wpt-commits: 58dba963a5607ef657943e21b16a416109e422b0
wpt-pr: 31162
jamienicol pushed a commit to jamienicol/gecko that referenced this issue Oct 28, 2021
…t 6265bis update., a=testonly

Automatic update from web-platform-tests
Update some domain tests to follow latest 6265bis update.

Also, add a few more tests inspired by bingler@'s GitHub comments.

Previously, the cookie spec left an empty domain attribute as
undefined behavior with SHOULD-level guidance.
httpwg/http-extensions#1709 changes the
spec to require handling of a cookie as a host cookie when its
last domain attribute is empty. This matches the current behavior
of Firefox and Safari.

See httpwg/http-extensions#1332 for
more discussion.

Change-Id: Ibf4f243c929b11768ff406e940d6988a37434754
Bug: 1255273
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3203692
Reviewed-by: Steven Bingler <[email protected]>
Commit-Queue: Mike Taylor <[email protected]>
Cr-Commit-Position: refs/heads/main@{#931471}

--

wpt-commits: 58dba963a5607ef657943e21b16a416109e422b0
wpt-pr: 31162
Gabisampaio pushed a commit to Gabisampaio/wpt that referenced this issue Nov 18, 2021
Also, add a few more tests inspired by bingler@'s GitHub comments.

Previously, the cookie spec left an empty domain attribute as
undefined behavior with SHOULD-level guidance.
httpwg/http-extensions#1709 changes the
spec to require handling of a cookie as a host cookie when its
last domain attribute is empty. This matches the current behavior
of Firefox and Safari.

See httpwg/http-extensions#1332 for
more discussion.

Change-Id: Ibf4f243c929b11768ff406e940d6988a37434754
Bug: 1255273
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3203692
Reviewed-by: Steven Bingler <[email protected]>
Commit-Queue: Mike Taylor <[email protected]>
Cr-Commit-Position: refs/heads/main@{#931471}
mjfroman pushed a commit to mjfroman/moz-libwebrtc-third-party that referenced this issue Oct 14, 2022
Also, add a few more tests inspired by bingler@'s GitHub comments.

Previously, the cookie spec left an empty domain attribute as
undefined behavior with SHOULD-level guidance.
httpwg/http-extensions#1709 changes the
spec to require handling of a cookie as a host cookie when its
last domain attribute is empty. This matches the current behavior
of Firefox and Safari.

See httpwg/http-extensions#1332 for
more discussion.

Change-Id: Ibf4f243c929b11768ff406e940d6988a37434754
Bug: 1255273
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3203692
Reviewed-by: Steven Bingler <[email protected]>
Commit-Queue: Mike Taylor <[email protected]>
Cr-Commit-Position: refs/heads/main@{#931471}
NOKEYCHECK=True
GitOrigin-RevId: 3813959c16bf46f428c7691a8e1a916538793e4a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

No branches or pull requests

5 participants