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 parsing for X-Content-Type-Options in detail #818

Merged
merged 4 commits into from Nov 1, 2018

Conversation

6 participants
@annevk
Copy link
Member

commented Oct 17, 2018

And add some of the infrastructure needed to define parsing better for all headers going forward (needed for #814). Fixes #752.

This also fixes an issue with CORB as it simply assumed an X-Content-Type-Options was present.

Tests: web-platform-tests/wpt#13559.


Preview | Diff

Define parsing for X-Content-Type-Options in detail
And add some of the infrastructure needed to define parsing better for all headers going forward (needed for #814). Fixes #752.

This also fixes an issue with CORB as it simply assumed an X-Content-Type-Options was present.

Tests: web-platform-tests/wpt#13559.

@annevk annevk requested review from mikewest and domenic Oct 17, 2018

@annevk

This comment has been minimized.

Copy link
Member Author

commented Oct 17, 2018

The only somewhat icky bit here in my opinion is using "isomorphic encode" to avoid the lack of byte sequence manipulation infrastructure. It seems fine for now, but worth keeping an eye on.

I also filed a follow-up, unrelated to that: #817.

cc @MattMenke2

@domenic
Copy link
Member

left a comment

Don't feel super competent to review the actual processing model but overall seems good. Editorial review done.

fetch.bs Outdated

<li>
<p>Let <var>tokens</var> be the result of
<a lt="split on commas">Spliting <var>stringValue</var> on commas</a>.

This comment has been minimized.

Copy link
@domenic

domenic Oct 17, 2018

Member

lowercase "s"

Show resolved Hide resolved fetch.bs
Show resolved Hide resolved fetch.bs
@mikewest
Copy link
Member

left a comment

One style nit, but the substance looks correct to me. Thank you!

fetch.bs Outdated

<li><p>If <var>value</var> is null, then return false.

<li><p>Let <var>stringValue</var> be the <a>isomorphic encode</a> of <var>value</var>.

This comment has been minimized.

Copy link
@mikewest

mikewest Oct 18, 2018

Member

I wonder if this should be part of the generic get algorithm?

This comment has been minimized.

Copy link
@annevk

annevk Oct 18, 2018

Author Member

It should be isomorphic decode. I think for some header values we want to always decode using UTF-8?

This comment has been minimized.

Copy link
@annevk

annevk Oct 18, 2018

Author Member

I'd support adding a "get as string|getting as string" for header lists though to make this easier for simple cases.

Show resolved Hide resolved fetch.bs
Show resolved Hide resolved fetch.bs
@MattMenke2
Copy link

left a comment

This looks good to me.

Show resolved Hide resolved fetch.bs Outdated
Show resolved Hide resolved fetch.bs Outdated
@annevk

This comment has been minimized.

Copy link
Member Author

commented Oct 30, 2018

@MattMenke2 so I updated the tests and ran them again and Chrome and Safari both treat nosniff immediately followed by 0x0B or 0x0C as a "valid value". Firefox does so for 0x0C only. I think that's why I went with treating 0x0C as valid and 0x0B as invalid. I'm happy for them to both be restricted, but are you equally sure about wanting this after knowing these results?

@MattMenke2

This comment has been minimized.

Copy link

commented Oct 30, 2018

@MattMenke2 so I updated the tests and ran them again and Chrome and Safari both treat nosniff immediately followed by 0x0B or 0x0C as a "valid value". Firefox does so for 0x0C only. I think that's why I went with treating 0x0C as valid and 0x0B as invalid. I'm happy for them to both be restricted, but are you equally sure about wanting this after knowing these results?

Running just Chrome's MIME sniffer through a test case (With a real HTTP server), "X-Content-Type-Options: nosniff\x0C" doesn't result in MIME sniffing, and there is no code anywhere in Chrome's network stack that could account for that.

However, it looks to me like Blink's code uses "nosniff" in a couple places, and that code does use a more permissive definition of space:

inline bool IsASCIISpace(CharType c) {
return c <= ' ' && (c == ' ' || (c <= 0xD && c >= 0x9));
}

So it looks to me like what's happening is the network stack sniffs the type, and gives "text/plain" to the renderer. Blink then applies its own header parsing logic, sees the header as being a nosniff header, and decides that "text/plain" just isn't going to cut it in a script tag.

For more well-behaved tags that don't have their own nosniff logic in Blink, like images (I assume), the network stack's interpretation will be respected, but in at least the particular case of Javascript, no so much... So that gets us back where we started.

@annevk

This comment has been minimized.

Copy link
Member Author

commented Oct 30, 2018

That seems somewhat bad… I guess fingers crossed that we can align on not treating 0x0B and 0x0C as whitespace across all code bases?

@MattMenke2

This comment has been minimized.

Copy link

commented Oct 30, 2018

I agree. Actually, it seems very bad to me.

I still suspect we won't see much breakage here (I mean, really, who uses that character?), but it does mean this has the potential to break pages. I'm much less gung ho about removing support for it now, but I still think doing so is best for the long term health of the platform, if we can manage it. I've made enough changes to Chrome's header parsing (One more is in the pipeline) in this version that I'm not comfortable making more until the current set has made it to stable or at least been on beta for a while, but we are going to have to change Chrome's behavior here, regardless, at some point.

@MattMenke2

This comment has been minimized.

Copy link

commented Oct 30, 2018

This bifurcated space handling logic also applies to a number of other response headers in blink. Cache headers, Accept headers, and quite possibly others. Blink doesn't have an HTTP-specific whitespace checking method, so it will be difficult to locate them all.

@annevk

This comment has been minimized.

Copy link
Member Author

commented Oct 31, 2018

As a heads up, I plan to merge this tomorrow or Friday, as it sets out new infrastructure for header parsing it's kind of nice to have it landed so the other bits can start to be converted.

@asankah
Copy link

left a comment

Whitespace restrictions look good to me!

@MattMenke2
Copy link

left a comment

Likewise happy with this.

pull bot pushed a commit to hoojaoh/web-platform-tests that referenced this pull request Nov 1, 2018

@annevk annevk merged commit 32c7b1c into master Nov 1, 2018

2 checks passed

Participation annevk participates on behalf of Mozilla Corporation
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@annevk annevk deleted the annevk/x-content-type-options branch Nov 1, 2018

kisg pushed a commit to paul99/webkit-mips that referenced this pull request Nov 6, 2018

commit-queue@webkit.org
Some minor X-Content-Type-Options parsing issues
https://bugs.webkit.org/show_bug.cgi?id=191107

Patch by Rob Buis <rbuis@igalia.com> on 2018-11-06
Reviewed by Darin Adler.

LayoutTests/imported/w3c:

Update improved result.

* web-platform-tests/fetch/nosniff/parsing-nosniff-expected.txt:

Source/WebCore:

Implement new parsing rules for X-Content-Type-Options [1]:
whatwg/fetch#818

[1] https://fetch.spec.whatwg.org/#x-content-type-options-header

Test: web-platform-tests/fetch/nosniff/parsing-nosniff.html

* platform/network/HTTPParsers.cpp:
(WebCore::isHTTPTabOrSpace):
(WebCore::parseContentTypeOptionsHeader):

git-svn-id: http://svn.webkit.org/repository/webkit/trunk@237850 268f45cc-cd09-0410-ab3c-d52691b4dbfc

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Nov 10, 2018

Bug 1499659 [wpt PR 13559] - Fetch: test nosniff parsing better, a=te…
…stonly

Automatic update from web-platform-testsFetch: X-Content-Type-Options: nosniff parsing

For whatwg/fetch#818.
--

wpt-commits: 10f708e6f13141fc92ca9318c7606590f659fcf2
wpt-pr: 13559

jyc pushed a commit to jyc/gecko that referenced this pull request Nov 11, 2018

Bug 1499659 [wpt PR 13559] - Fetch: test nosniff parsing better, a=te…
…stonly

Automatic update from web-platform-testsFetch: X-Content-Type-Options: nosniff parsing

For whatwg/fetch#818.
--

wpt-commits: 10f708e6f13141fc92ca9318c7606590f659fcf2
wpt-pr: 13559
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.