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

Define X-Frame-Options processing #5737

Merged
merged 8 commits into from
Aug 18, 2020
Merged

Define X-Frame-Options processing #5737

merged 8 commits into from
Aug 18, 2020

Conversation

domenic
Copy link
Member

@domenic domenic commented Jul 15, 2020

Closes #1230.

@annevk can you help me define the ABNF for this, and relatedly, the header handling? I used "get, decode, and split" but maybe I should be using one of https://fetch.spec.whatwg.org/#extract-header-values ?

Interop issues/deviations from this spec:

  • Firefox does not treat quoted values the same as unquoted values. Chrome does. This spec does (by using "get, decode, and split").
  • Chrome and Safari treat XFO: INVALID; XFO: SAMEORIGIN (or the reverse) as XFO: DENY. Firefox and this spec treats them as XFO: SAMEORIGIN.

(See WHATWG Working Mode: Changes for more details.)


/browsing-the-web.html ( diff )
/iana.html ( diff )
/index.html ( diff )
/infrastructure.html ( diff )
/references.html ( diff )

domenic added a commit to web-platform-tests/wpt that referenced this pull request Jul 15, 2020
domenic added a commit to web-platform-tests/wpt that referenced this pull request Jul 15, 2020
Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

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

Thanks for tackling this. I've asked @mozfreddyb to take a look as well.

source Show resolved Hide resolved
source Show resolved Hide resolved
source Outdated Show resolved Hide resolved
@dveditz
Copy link
Member

dveditz commented Jul 23, 2020

Why do we need to parse quoted values? They weren't in the original IE implementation nor RFC 7034. In Scott Helme's "top 1 million Alexa site" security header scans there are very few instances of XFO headers that contain quotes at all, and most of those are otherwise invalid with or without the quotes, like this gem: ALLOW-FROM="^https?:\/\/([^\/]+\.)?(tv-karelia\.ru|webvisor\.com)\/". There were 8 instances of "SAMEORIGIN" that allowing quotes would make valid. That doesn't seem like enough to change this syntax.

https://crawler.ninja/files/xfo-values.txt

@domenic
Copy link
Member Author

domenic commented Jul 23, 2020

Because that's consistent with all other comma-delimited headers that use https://fetch.spec.whatwg.org/#concept-header-list-get-decode-split .

@domenic
Copy link
Member Author

domenic commented Jul 23, 2020

Hmm, adding more tests for quoted values explicitly:

  • Firefox treats "sameOriGin" and "denY" as SAMEORIGIN
  • Chrome treats "sameOriGin" and "denY" as no-header (allows all)

Given the lack of interop here, I think the proposed spec (treating the quoted values the same as unquoted ones) is better, as it is stricter and matches other headers' processing models.

@annevk
Copy link
Member

annevk commented Aug 5, 2020

Daniel's comment did make me wonder if X-Content-Type-Options does in fact support "nosniff" as value. And it seems that fetch/nosniff/resources/x-content-type-options.json has no coverage for that and browsers do not support it. I filed whatwg/fetch#1068 on Fetch for this. Would appreciate your thoughts.

@domenic
Copy link
Member Author

domenic commented Aug 5, 2020

Hmm, OK. So let's write this spec assuming whatwg/fetch#1068 is fixed, and thus quoted values are treated as invalid values.

With the current PR draft that would give Chrome's behavior, treating "sameOriGin" and "denY" as no-header, and thus allowing all values through. I'll update the tests PR in that direction, tentatively.

@domenic
Copy link
Member Author

domenic commented Aug 5, 2020

Something must have been wrong with my tests previously. (There is a lot of copying and pasting in the current infrastructure; probably we should straighten that out in the future.) With the latest version Firefox passes everything.

@domenic
Copy link
Member Author

domenic commented Aug 5, 2020

Let's continue the discussion in the issue: #1230 (comment)

@domenic domenic added addition/proposal New features or enhancements security/privacy There are security or privacy implications topic: navigation labels Aug 7, 2020
@domenic domenic requested a review from annevk August 7, 2020 19:12
Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

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

Thanks @domenic. I think this looks good now that we resolved the confusion around quotes. Should we address the quotes in an example to be sure or is that no longer needed now that Fetch covers it?

@mozfreddyb you might want to have a final look at this.

source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
@domenic
Copy link
Member Author

domenic commented Aug 18, 2020

Fixed up; going to merge! Indeed, I think the quotes are not necessary to address in the examples table, as it's handled at a lower layer and doesn't exemplify any of the specific processing model in this algorithm. In general the examples table treads a fine line between duplicating web platform tests and being a helpful aid to the reader, so my instinct is to keep it more minimal...

@domenic domenic merged commit bd32618 into master Aug 18, 2020
@domenic domenic deleted the x-frame-options branch August 18, 2020 16:34
domenic added a commit to web-platform-tests/wpt that referenced this pull request Aug 18, 2020
Follows whatwg/html#5737. Closes #21730 by incorporating all of those tests.
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Aug 25, 2020
…tions, a=testonly

Automatic update from web-platform-tests
Expand X-Frame-Options tests

Follows whatwg/html#5737. Closes web-platform-tests/wpt#21730 by incorporating all of those tests.
--

wpt-commits: 29c58c00b7729431a79e333b2143d1289775fa76
wpt-pr: 24618
ambroff pushed a commit to ambroff/gecko that referenced this pull request Nov 4, 2020
…tions, a=testonly

Automatic update from web-platform-tests
Expand X-Frame-Options tests

Follows whatwg/html#5737. Closes web-platform-tests/wpt#21730 by incorporating all of those tests.
--

wpt-commits: 29c58c00b7729431a79e333b2143d1289775fa76
wpt-pr: 24618
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addition/proposal New features or enhancements security/privacy There are security or privacy implications topic: navigation
Development

Successfully merging this pull request may close these issues.

HTML's "navigate" algorithm should handle 'X-Frame-Options' and CSP's 'frame-ancestors'
3 participants