1454605 - Investigate and evaluate why tests on http://rfc6265.biz/tests/ are failing

Status: RESOLVED FIXED

(Core :: DOM: Security, defect, P3)

Description

[Christoph Kerschbaumer [:ckerschb}]

Ideally we have a clear sense of why certain tests of our same-site cookie implementation are failing on http://rfc6265.biz/tests/.

We should mark those down within this bug.

As a reference, at some point we used to pass [1]:
fetch:       10/12 PASS (2 timeouts)
form [GET]:  12 timeouts
form [POST]: 12 timeouts
iframe:      10/12 PASS (2 timeouts)
img:         12/12 PASS (1 timout)
window.open:  5/12 PASS (7 timeouts)

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1286861#c16

Comment 1

[Mark Goodwin [:mgoodwin]]

I've looked into this; re-applying the "drop cookies if same-site is bad" from here https://bugzilla.mozilla.org/attachment.cgi?id=8965727&action=diff fixes the issue

Comment 2

[Christoph Kerschbaumer [:ckerschb}]

Attachment: discard_bogus_values.patch.patch

It seems that most the tests are failing because Firefox implementation discards a bogus attribute value, whereas the tests rely on the fact that the entire cookie is discarded in case a same-site cookie uses a different value than "lax" or "strict".

The attached patch lets Firefox ignore the entire cookie in case the same-site cookie attribute value is bogus.

Comment 3

[Mark Goodwin [:mgoodwin]]

Francois was going to see about getting a clarification on the spec here - I can see the benefit in the tested behaviour but I might be alone in this here ;) - regardless of which, maybe we can fix Mike's tests to only show failures for the subsections of each test that fail (rather than failing everything).

We can also stick the above behind a pref so we can run these tests and see how we stack up without that difference.

Comment 4

[Christoph Kerschbaumer [:ckerschb}]

Mike, we just fixed a redirect problem for iframes within Bug 1454027. When running the iframe tests on [0] Firefox passes 10, but fails 2 tests.

In particular, we are failing:
* Cross-site redirecting to same-host fetches are strictly same-site
* Cross-site redirecting to subdomain fetches are strictly same-site

When looking at the source of the test [1] I see:
create_test(ORIGIN, redirectTo(CROSS_SITE_ORIGIN, ORIGIN), SameSiteStatus.STRICT, "Cross-site redirecting to same-host fetches are strictly same-site");

which indicates the test performs a redirect from same origin to cross origin and back, right? In my opinion the status should *not* be SameSiteStatus.STRICT, but rather SameSiteStatus.CROSS_SITE or am I missing something here?

[0] http://rfc6265.biz/tests/samesite/iframe.html
[1] view-source:http://rfc6265.biz/tests/samesite/iframe.html

Comment 5

[Christoph Kerschbaumer [:ckerschb}]

Mike, please see previous comment - thanks!

Comment 6

[Christoph Kerschbaumer [:ckerschb}]

I did a full comparison running all the tests on Chrome(65) and our implementation of same-site cookies. Please note that I applied the patch from this bug on top of our implementation so cookies with a bogus same-site attribute are ignored.

Here are the results:

             Chrome(65)   mozilla-central         Reason
fetch:       12/12        8/12                    cross-site redirect tests fail (see comment 4)  
form [GET]:  12 Timeouts  12 Timeouts             probably issue with test-harness
form [POST]: 12 Timeouts  12 Timeouts             probably issue with test-harness
iframe:      12/12        10/12                   cross-site redirect tests fail (see comment 4)                       
img:         12/12        10/12                   cross-site redirect tests fail (see comment 4)
window.open: 12 Fail      4 Pass/1Fail/7Timeouts  all tests not passing seem related to redirects as well

Comment 7

[François Marier [:francois]]

(In reply to Mark Goodwin [:mgoodwin] from comment #3)
> Francois was going to see about getting a clarification on the spec here

Yes, the spec has been updated: https://github.com/httpwg/http-extensions/commit/ebe8f8ea69c02d83e5dceb91fdae0e787845c7ba

So I guess we should submit a PR to fix the tests now.

Comment 8

[Christoph Kerschbaumer [:ckerschb}]

Ok, given our results, I guess it's fair to put those in the backlog for now.

Comment 9

[Andrea Marchesini [:baku]]

Those tests are now part of WPTs. We pass most of them and we have separate bugs to treat the failures.
This bug can be closed. To be more precise

http://rfc6265.biz/tests/samesite/fetch.html -> /cookies/samesite/fetch.https.html
http://rfc6265.biz/tests/samesite/form-get-blank.html-> /cookies/samesite/form-get-blank.https.html
http://rfc6265.biz/tests/samesite/form-post-blank.html-> /cookies/samesite/form-post-blank.https.html
http://rfc6265.biz/tests/samesite/iframe.html-> /cookies/samesite/iframe.https.html
http://rfc6265.biz/tests/samesite/img.html-> /cookies/samesite/img.https.html
http://rfc6265.biz/tests/samesite/window-open.html-> /cookies/samesite/window-open.https.html
http://rfc6265.biz/tests/samesite/form-get-blank-reload.html-> /cookies/samesite/form-get-blank-reload.https.html
http://rfc6265.biz/tests/samesite/form-post-blank-reload.html-> /cookies/samesite/form-post-blank-reload.https.html
http://rfc6265.biz/tests/samesite/window-open-reload.html-> /cookies/samesite/window-open-reload.https.html

The remaining tests have been fixed by bug 976073.

Comment 10

[Christoph Kerschbaumer [:ckerschb}]

Clearing out my ni? queue with super old ni? requests which rendered unnecessary in the meantime.