XFO tests for comma-separated header values

[mozfreddyb]

These adds test for headers like X-Frame-Options: DENY,SAMEORIGIN (with various combinations of DENY, SAMEORIGIN and INVALID).

The existing cases send two headers, this does adds explicit tests for comma-separated header values.

There are existing tests that timeout in browsers other than Firefox.
Due to the fact that I am adding test cases, I am also adding more timeouts 😕

Tested to pass in Firefox (and 8 pass, 4 timeout) in Chrome as expected.

CC @annevk

[mozfreddyb]

I should add that I rewrote the file to use loops for the test cases instead of copy-pasta code, I tried splitting things up in individual commits but failed mid-way. I hope this ends up somewhat readable.

[mikewest]

Rewriting the tests as a for loop is fine. From that perspective, LGTM.

At a deeper level, I don't think 308fdea#diff-0e5a26ef8bf3673359740ddfdf04f79c was correct. That is, it's clear that we have disagreement between browsers about the expected behavior. Chrome and Safari both fail closed when multiple headers are present with differing values. The linked bug https://bugzilla.mozilla.org/show_bug.cgi?id=1492059 doesn't explain why this behavior was chosen. Can you help me understand the rationale?

[mozfreddyb]

@mikewest Looking at it, it seems that Safari and Chrome don't fail close all the time.
There are tests for (X-Frame-Options: INVALID that seem to fail open](https://wpt.fyi/results/x-frame-options/invalid.sub.html?label=experimental&label=master&aligned) whereas you want to fail close on X-Frame-Options: INVALID, SAMEORIGIN. That seems ambiguous to me. Can you clarify why you want to fail open on "INVALID" and fail closed on "INVALID, SAMEORIGIN"?

I was unaware of the discussion you've had in May 2019. Apologies, if this adds confusion.
I have just now read the comments in https://bugs.chromium.org/p/chromium/issues/detail?id=958128 and https://bugzilla.mozilla.org/show_bug.cgi?id=1551494

[mikewest]

Looking at it, it seems that Safari and Chrome don't fail close all the time.

AFAIR, we fail closed iff the server asserts two policies that aren't the same. "I am confused, please block me." the server says. But I haven't looked at this in a while...

[mozfreddyb]

One could also read the X-FRAME-OPTIONS: INVALID as "I am confused, please block me", hence my question..

[mikewest]

Unfortunately, Chrome and Safari both include ALLOWFROM as invalid. Blocking that would make developers sad, I think.

[annevk]

But why should we split on commas even? A basic processing model for this header would be to combine, then get the value, and then do a case-insensitive match for "sameorigin" or "deny". If you specify something else, we'd fail open I guess as that's what we already do. That would also be consistent with a large number of existing headers.

Doing a special thing for multiple values is rather odd. E.g., would invalid,invalid2 block but invalid not?

[mozfreddyb]

Ah. Chrome and Safari not implementing ALLOWFROM is a bummer :) Maybe that needs to go away?

Doing a special thing for multiple values is rather odd. E.g., would invalid,invalid2 block but invalid not?

FWIW, I've tested Chrome for these cases. All go through as allow:

INVALID, INVALID
INVALID, INVALID2
SAMEORIGIN, sameorigin

So, the pseudo-code is along those lines..

• discard all invalid tokens
• empty list? fail open
• if policy is a single token, use that token
• if policy is a list, fail close unless every list item is the same

[mikewest]

So, the pseudo-code is along those lines..

That sounds familiar to me. I don't think Chrome's current behavior is crazy, but I can understand if y'all would like it to be different.

Is there an interop problem that this would resolve? As-is, XFO is nowhere on my priority list. Should that change?

[annevk]

The lack of a clear processing model makes refactoring it harder than needed and we'd also like Firefox to align with other browsers in terms of what values to support.

So what is the Chrome/Safari model, in detail?

1. Let values be the result of https://fetch.spec.whatwg.org/#concept-header-list-get-decode-split X-Frame-Options from response's header list.
2. If values is null, then return null.
3. Let policy be null.
4. If values contains an item that is byte-case-insensitive match for sameorigin or deny, then set policy to that item.
5. If policy is null, then return null.
6. If values contains an item that is not a byte-case-insensitive match for policy, then return deny.
7. Return policy.

[mikewest]

The lack of a clear processing model makes refactoring it harder than needed and we'd also like Firefox to align with other browsers in terms of what values to support.

I agree, and I'm totally willing to make changes to Chromium if that's helpful to you. I'm just trying to understand the priority from y'all's side. I'm not objecting to making changes. :)

Chromium parses XFO in https://cs.chromium.org/chromium/src/content/browser/frame_host/ancestor_throttle.cc?rcl=9941923164bfa5fc210ea810eb2547e55e4888b4&l=354 and enforces in https://cs.chromium.org/chromium/src/content/browser/frame_host/ancestor_throttle.cc?rcl=9941923164bfa5fc210ea810eb2547e55e4888b4&l=200 (tests in https://cs.chromium.org/chromium/src/content/browser/frame_host/ancestor_throttle_browsertest.cc?g=0). The model is something more like:

"""
Assuming some sort of disposition enum of { NONE, DENY, SAMEORIGIN, INVALID, ALLOWALL, CONFLICT } and a result enum of { ALLOW, DENY }.

1. Let values be the result of https://fetch.spec.whatwg.org/#concept-header-list-get-decode-split X-Frame-Options from response's header list.

2. If values is null, then return ALLOW.

3. Let result be NONE.

4. For each value in values:

   1. Let current be INVALID.

   2. If value is byte-case-insensitive match for sameorigin, set current to SAMEORIGIN.

   3. Ditto for deny.

   4. Ditto for allowall (which broke something at some point when we considered it invalid).

   5. If result is NONE:

      1. Set result to current.

      Else:

      1. Set result to CONFLICT.

5. Return the result of switching on result:

   CONFLICT

   DENY

   DENY

   NONE

   ALLOWALL

   ALLOW

   INVALID

   ALLOW (_with a TODO(mkwst) to evaluate blocking, see below_)

   SAMEORIGIN

   Do some origin-based logic here.

"""

Looking at the last 7 days of framed navigations on Windows stable, the histogram looks as follows (details on each bucket at https://cs.chromium.org/chromium/src/content/browser/frame_host/ancestor_throttle.cc?g=0&l=35):

XFO	%
none	93.158%
deny	0.401%
sameorigin (allowed)	1.795%
sameorigin (blocked)	0.931%
sameorigin (allowed, but with an ancestor mismatch)	0.001%
allowall	0.078%
invalid	0.267%
conflict	0.002%
bypass (because frame-ancestors is present)	2.481%
redirect (would be blocked if we did https://crbug.com/835465)	0.886%

It would be nice if we had more detail on that invalid bucket; I'm reluctant to just start breaking it.

[domenic]

I'm working on the spec for this in whatwg/html#5737, and have some similar tests in #24618. I'm happy to figure out the overlap and either get this merged or incorporate it into my tests.

However, we do need to make a decision on what happens in the conflict case. Let's discuss that in whatwg/html#1230.

[domenic]

I've fully merged these into #24618 for now, so let's close this.