Resolves #4183 - Implemementing context-based MIME type sniffing #7842

Yoric · 2015-10-03T19:34:23Z

As mentioned in #4183, I'm not sure how to test this.

highfive · 2015-10-03T19:34:28Z

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @larsbergstrom (or someone else) soon.

eefriedman · 2015-10-03T20:12:45Z

components/net/mime_classifier.rs

+                // 1. If the supplied MIME type is undefined, [unspecified at the time of this implementation, so fallback to Browsing]
+                // 2. The computed MIME type is the supplied MIME type.
+                match *supplied_type {
+                    None => self.classify(LoadContext::Browsing, no_sniff_flag, apache_bug_flag, supplied_type, data),


The actual algorithm should be something like this:

If the supplied MIME type is undefined, the computed MIME type is "text/css".

The computed MIME type is the supplied MIME type.

Fine with me. I just wanted to make sure that I didn't accidentally add something incompatible.

eefriedman · 2015-10-03T20:20:37Z

components/net/mime_classifier.rs

+                // 1. If the supplied MIME type is undefined, [unspecified at the time of this implementation, so fallback to Browsing]
+                // 2. The computed MIME type is the supplied MIME type.
+                match *supplied_type {
+                    None => self.classify(LoadContext::Browsing, no_sniff_flag, apache_bug_flag, supplied_type, data),


Script should default to "application/javascript".

What, no vbscript :) ?
I'll do that.

eefriedman · 2015-10-03T21:00:59Z

In terms of testing this... we don't actually examine the mime type in non-browser contexts, so I'm pretty sure it's impossible to test without other changes. That said, it would probably be pretty easy to add some sensitivity. For example, if we only tried to render images which have a known mimetype, you could check that in an <img> tag an image served as ~~text/html~~ application/mathml+xml doesn't get rendered, but an image served as application/octet-stream does.

Yoric · 2015-10-04T08:11:25Z

That said, it would probably be pretty easy to add some sensitivity. For example, if we only tried to render images which have a known mimetype, you could check that in an tag an image served as text/html doesn't get rendered, but an image served as application/octet-stream does.

Do you think we want this in Servo? Perhaps if sniffing is deactivated?

eefriedman · 2015-10-04T18:09:05Z

components/net/mime_classifier.rs

+                //
+                // 4. The computed MIME type is the supplied MIME type.
+                match MIMEClassifier::maybe_get_media_type(supplied_type) {
+                    Some(MediaType::Xml) | Some(MediaType::Html) => supplied_type.clone(),


"1. If the supplied MIME type is an XML type," doesn't mention HTML.

My bad, misread the definition of XML type.

eefriedman · 2015-10-04T18:25:05Z

Do you think we want this in Servo? Perhaps if sniffing is deactivated?

For testing purposes, perhaps only if sniffing is activated (which will be the default eventually).

Yoric · 2015-10-05T09:56:15Z

I have applied the change you suggested to the image cache, but I haven't been able to see any different behavior. I suspect that the actual culprit is actually my lack of mastery of Apache, though, as Rust and Firefox seem to have the exact same behavior.

What's the next step?

bors-servo · 2015-10-06T11:45:01Z

☔ The latest upstream changes (presumably #7882) made this pull request unmergeable. Please resolve the merge conflicts.

Yoric · 2015-10-07T10:24:50Z

Rebased. @jdm, I'm waiting for your instructions.

bors-servo · 2015-10-14T16:04:25Z

☔ The latest upstream changes (presumably #7967) made this pull request unmergeable. Please resolve the merge conflicts.

jdm · 2015-10-18T05:09:09Z

For testing, you could add a MIME type check to the font-loading code (with a server that returns no Content-Type), and look at the Content-Type response header for an XHR to the same font.
-S-awaiting-review +S-needs-code-changes

Reviewed 12 of 12 files at r1, 3 of 3 files at r2, 5 of 5 files at r3.
Review status: all files reviewed at latest revision, 10 unresolved discussions, some commit checks failed.

^{components/net/http_loader.rs, line 141 [r1] (raw file):}
The should be the same context as the original load.

^{components/net/mime_classifier.rs, line 76 [r1] (raw file):}
It's generally preferred to have comments like // Step 1 rather than quoting the full spec text.

^{components/net/mime_classifier.rs, line 79 [r1] (raw file):}
nit: . should be on the next line

^{components/net/mime_classifier.rs, line 90 [r1] (raw file):}
Remove the HTML here.

^{components/net/mime_classifier.rs, line 109 [r1] (raw file):}
@elifriedman Do you have sources to back this up? MIME sniffing is really hairy; I'd rather stick to implementing the precise spec text to start (i.e. don't take any action here)

^{components/net/mime_classifier.rs, line 118 [r1] (raw file):}
This does not sound like a good idea to me; script sniffing-related errors are some of the most easily exploited. I'd rather take no action at all until the spec is clarified.

^{components/net/mime_classifier.rs, line 228 [r1] (raw file):}
supplied_type.map(|(ref media_type, ref media_subtype)| {
MIMEClassifier::get_media_type(media_type, media_subtype)
})

^{components/net_traits/lib.rs, line 137 [r2] (raw file):}
Let's use /// so it shows up in the docs.

^{components/script/document_loader.rs, line 41 [r1] (raw file):}
nit: | on the previous line, please.

Comments from the review on Reviewable.io

eefriedman · 2015-10-18T06:25:24Z

Review status: all files reviewed at latest revision, 10 unresolved discussions, some commit checks failed.

^{components/net/mime_classifier.rs, line 109 [r1] (raw file):}
Firefox also picks text/css for an unspecified MIME type; see http://mxr.mozilla.org/mozilla-central/source/layout/style/Loader.cpp#925 .

Granted, this is known to be a tricky area; sniffing CSS can lead to cross-origin attacks. See http://blogs.msdn.com/b/ie/archive/2010/10/26/mime-handling-changes-in-internet-explorer.aspx etc.

If we wanted to be extremely conservative, we could sniff to text/plain if the MIME type is unspecified.

^{components/net/mime_classifier.rs, line 118 [r1] (raw file):}
Err, I'm not sure exactly what you think the risk is here? Firefox doesn't check the MIME type of JavaScript at all.

Comments from the review on Reviewable.io


        Resolves #4183 - Implemementing context-based MIME type sniffing

The version of the standard is not finalized at the time of this writing. Specifications may be found here: https://mimesniff.spec.whatwg.org/#context-specific-sniffing .

jdm · 2015-10-25T16:01:03Z

Rebased in #8190.


        Auto merge of #8190 - jdm:4138, r=eefriedman

@Yoric

Implemementing context-based MIME type sniffing This is a rebase of #7842 that also adds a test. @Yoric, how's this look to you?  [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/8190)


        Auto merge of #8190 - jdm:4138, r=<try>

@Yoric

Implemementing context-based MIME type sniffing This is a rebase of #7842 that also adds a test. @Yoric, how's this look to you?  [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/8190)


        Auto merge of #8190 - jdm:4138, r=eefriedman

@Yoric

Implemementing context-based MIME type sniffing This is a rebase of #7842 that also adds a test. Fixes #4183. @Yoric, how's this look to you?  [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/8190)


        Auto merge of #8190 - jdm:4138, r=<try>

@Yoric

Implemementing context-based MIME type sniffing This is a rebase of #7842 that also adds a test. Fixes #4183. @Yoric, how's this look to you?  [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/8190)


        Auto merge of #8190 - jdm:4138, r=eefriedman

@Yoric

Implemementing context-based MIME type sniffing This is a rebase of #7842 that also adds a test. Fixes #4183. @Yoric, how's this look to you?  [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/8190)

highfive added the S-awaiting-review label Oct 3, 2015

jdm added the S-fails-tidy label Oct 3, 2015

eefriedman reviewed Oct 3, 2015
View changes

Yoric force-pushed the Yoric:4183 branch from de8f675 to 9c775ad Oct 4, 2015

Yoric force-pushed the Yoric:4183 branch from 9c775ad to 3b26540 Oct 4, 2015

eefriedman reviewed Oct 4, 2015
View changes

Yoric force-pushed the Yoric:4183 branch from 3b26540 to 9910617 Oct 4, 2015

jdm self-assigned this Oct 5, 2015

highfive added S-needs-rebase and removed S-awaiting-merge labels Oct 6, 2015

Yoric force-pushed the Yoric:4183 branch from 9910617 to ace3c71 Oct 7, 2015

jdm removed S-needs-rebase S-fails-tidy labels Oct 7, 2015

highfive added S-needs-rebase and removed S-awaiting-merge labels Oct 14, 2015

jdm added S-needs-code-changes and removed S-awaiting-review labels Oct 18, 2015

Resolves #4183 - Implemementing context-based MIME type sniffing

a178e64

The version of the standard is not finalized at the time of this writing. Specifications may be found here: https://mimesniff.spec.whatwg.org/#context-specific-sniffing .

Yoric force-pushed the Yoric:4183 branch from ace3c71 to a178e64 Oct 20, 2015

highfive added S-awaiting-review and removed S-needs-code-changes labels Oct 20, 2015

jdm mentioned this pull request Oct 25, 2015

Implemementing context-based MIME type sniffing #8190

Merged

jdm closed this Oct 25, 2015

servo / servo

Resolves #4183 - Implemementing context-based MIME type sniffing #7842

Resolves #4183 - Implemementing context-based MIME type sniffing #7842

Yoric commented Oct 3, 2015

highfive commented Oct 3, 2015

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

eefriedman commented Oct 3, 2015

Yoric commented Oct 4, 2015

This comment has been minimized.

This comment has been minimized.

eefriedman commented Oct 4, 2015

Yoric commented Oct 5, 2015

bors-servo commented Oct 6, 2015

Yoric commented Oct 7, 2015

bors-servo commented Oct 14, 2015

jdm commented Oct 18, 2015

eefriedman commented Oct 18, 2015

jdm commented Oct 25, 2015

servo / servo

Join GitHub today

Resolves #4183 - Implemementing context-based MIME type sniffing #7842

Resolves #4183 - Implemementing context-based MIME type sniffing #7842

Conversation

Yoric commented Oct 3, 2015

highfive commented Oct 3, 2015

This comment has been minimized.

eefriedman Oct 3, 2015

This comment has been minimized.

Yoric Oct 4, 2015

This comment has been minimized.

eefriedman Oct 3, 2015

This comment has been minimized.

Yoric Oct 4, 2015

eefriedman commented Oct 3, 2015

Yoric commented Oct 4, 2015

This comment has been minimized.

eefriedman Oct 4, 2015

This comment has been minimized.

Yoric Oct 4, 2015

eefriedman commented Oct 4, 2015

Yoric commented Oct 5, 2015

bors-servo commented Oct 6, 2015

Yoric commented Oct 7, 2015

bors-servo commented Oct 14, 2015

jdm commented Oct 18, 2015

eefriedman commented Oct 18, 2015

jdm commented Oct 25, 2015