Skip to content
This repository has been archived by the owner on Nov 6, 2019. It is now read-only.

cookies/prefix/__secure.header.https.html results seem off. #599

Closed
mikewest opened this issue Aug 31, 2018 · 5 comments
Closed

cookies/prefix/__secure.header.https.html results seem off. #599

mikewest opened this issue Aug 31, 2018 · 5 comments
Assignees

Comments

@mikewest
Copy link
Member

https://wpt.fyi/results/cookies/prefix/__secure.header.https.html shows as failing on Firefox and Chrome. It seems to pass on both Chromium's bots and in Chrome when run directly from https://w3c-test.org/cookies/prefix/__secure.header.https.html. I'm happy to dig in a bit to try to figure out why it's failing in the harness, but it's not at all clear to me where I could look for logs or anything else.

Halp? :)

@jugglinmike jugglinmike self-assigned this Sep 3, 2018
@jugglinmike
Copy link
Collaborator

I'm on the case!

@jugglinmike
Copy link
Collaborator

This looks like a problem with test interaction.

To reproduce with the WPT CLI, run the following commands:

$ ./wpt run firefox cookies/prefix/__secure.document-cookie.https.html
$ ./wpt run firefox cookies/prefix/__secure.header.https.html
$ ./wpt run firefox cookies/prefix/__secure.document-cookie.https.html cookies/prefix/__secure.header.https.html

The first two will pass, but the third will fail, ostensibly due to __secure.header.https.html. This is also reproducible using chrome in place of firefox.

To reproduce with w3c-test.org Visit pages in the following sequence:

  1. https://w3c-test.org/cookies/prefix/__secure.header.https.html (all sub-tests pass)
  2. https://w3c-test.org/cookies/prefix/__secure.document-cookie.https.html (all sub-tests pass)
  3. https://w3c-test.org/cookies/prefix/__secure.header.https.html (all sub-tests fail)

Both tests have been written to defensively remove cookies prior to testing via the erase_cookie_from_js utility function. That function attempts to remove cookies by overwriting with an expired entry that has no domain attribute, but the final sub-test in __secure.document-cookie.https.html sets a cookie with an explicit domain attribute.

This is where I get a little lost. In HTML, document.cookie delegates to rfc6265, where the Storage Model describes how the domain is interpreted. According to steps 4 through 6, these strings should describe the same cookie in the store.

However, Chrome, Edge, Firefox, IE, and Safari all store these cookies independently:

<script>
document.cookie = 'foo=1';
document.cookie = 'foo=2';
document.cookie = 'foo=3; domain=' + document.location.hostname;
document.cookie = 'foo=4; domain=' + document.location.hostname;
console.log(document.cookie); // 'foo=2; foo=4'
</script>

That would explain the failure we're seeing: __secure.document-cookie.https.html finishes by setting a cookie which __secure.header.https.html fails to delete, causing an error in its "sanity check." It would even account for the fact that Safari Technology Preview passes the reported test. STP is the only browser that fails __secure.document-cookie.https.html, so it never sets the domain-specifying cookie.

This is a strong indication that I've misinterpreted the spec. Given that we work on different time zones, verifying this could delay a fix for another day at least. To avoid that, I've filed a patch based on the assumption that the mistake is in my reading (and not all the major browsers): web-platform-tests/wpt#12817

@jugglinmike
Copy link
Collaborator

Separately, here's my take on how an unstable test like this could make its way into WPT's master branch. The project's continuous integration system rejects tests that are unstable, so the patch that introduced it should have been blocked in the review process. Instead, CI labeled the patch as stable.

This seems to be a case of unfortunate scheduling. The following command executes each test 10 times:

./wpt check-stability chrome:dev cookies/prefix/__secure.{document-cookie,header}.https.html

It reports __secure.document-cookie.https.html as "stable" (consistently passing), and it reports __secure.header.https.html as "stable" (consistently failing). It exits with a code of 0 because it does not identify inconsistent behavior. That's because for each iteration, wpt check-stability executes the two tests in series and then restarts the browser.

WPT offers a second command for verifying test stability:

./wpt run --verify chrome submissions/cookies/prefix/__secure.{document-cookie,header}.https.html

That command reports __secure.document-cookie.https.html as unstable (failing 50% of the time), and it reports __secure.header.https.html as "stable" (consistently passing). It exits with a non-zero exit code. It does this because it runs the first test 10 times in series and then runs the second test 10 times in series. (It reports 5 passes and 5 failures because it restarts the browser after each failure.)

We've recently been discussing relevant changes to the CI setup, so I'll raise this issue there.

@mikewest
Copy link
Member Author

mikewest commented Sep 4, 2018

This is where I get a little lost. In HTML, document.cookie delegates to rfc6265, where the Storage Model describes how the domain is interpreted. According to steps 4 through 6, these strings should describe the same cookie in the store.

However, Chrome, Edge, Firefox, IE, and Safari all store these cookies independently:

Amusingly, we just fixed this in the cookie spec: httpwg/http-extensions@b415d03

Thank you very much for digging in, and apologies that it was my own fault. :(

@jugglinmike
Copy link
Collaborator

Thanks for the link (it's good to know my reading was accurate), and no worries about the mistake

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants