Block scripts with text/csv, audio/, video/ and image/* mime types #16126

ferjm · 2017-03-24T19:03:13Z

This patch implements step 12 of the Main Fetch section of the Fetch API standard. It blocks the load of scripts with text/csv, audio/*, video/* and image/* mime types.

Credit for the logic of should_block_mime_type function should go to the author of #14770.

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix Block scripts with media/CSV mime types #14520
There are tests for these changes

This change is

highfive · 2017-03-24T19:03:16Z

Heads up! This PR modifies the following files:

@KiChjang: components/net/fetch/methods.rs

highfive · 2017-03-24T19:03:17Z

Warning

These commits include an empty title element (<title></title>). Consider adding appropriate metadata.

ferjm · 2017-03-24T19:04:50Z

r? @asajeffrey

nox

Some improvements can be made.

nox · 2017-03-25T09:57:22Z

components/net/fetch/methods.rs

@@ -616,6 +621,18 @@ fn should_block_nosniff(request: &Request, response: &Response) -> bool {
    };
 }

+/// https://fetch.spec.whatwg.org/#should-response-to-request-be-blocked-due-to-mime-type?
+fn should_block_mime_type(request: &Request, response: &Response) -> bool {


Nit: should_be_blocked_due_to_mime_type, given I recently named the other one should_be_blocked_due_to_bad_port.

nox · 2017-03-25T10:01:24Z

components/net/fetch/methods.rs

+fn should_block_mime_type(request: &Request, response: &Response) -> bool {
+    let mime_type = response.headers.get::<ContentType>();
+    let csv: Mime = "text/csv".parse().unwrap();
+    request.type_ == Type::Script && mime_type.is_some() && match *mime_type.unwrap() {


You can first unwrap mime_type and return early if it is None.

let mime_type = match response.headers.get::<ContentType>() { Some(header) => header, None => return false, };

I'd also improve this by not pre-allocating "text/csv".parse() at all and pattern-match directly against ContentType(Mime(TopLevel::Text, SubLevel::Csv, _)).

SubLevel::Csv doesn't exist, and I already suggested a way to not parse the CSV mimetype below. :)

nox · 2017-03-25T10:05:00Z

components/net/fetch/methods.rs

+        ContentType(Mime(TopLevel::Audio, _, _)) |
+        ContentType(Mime(TopLevel::Video, _, _)) |
+        ContentType(Mime(TopLevel::Image, _, _)) => true,
+        ContentType(ref m_type) => *m_type == csv


No need to allocate a Mime value for csv.

match *mime_type { ... ContentType(Mime(TopLevel::Text, SubLevel::Ext(ref ext), _)) => ext == "csv", _ => false, }

ferjm · 2017-03-27T11:07:50Z

Thanks for the feedback @nox. Could you take another look, please?

nox · 2017-03-28T07:18:02Z

Squash the two commits together and this is ready to be merged.

ferjm · 2017-03-28T08:13:06Z

Thank you @nox. Rebased and squashed.

nox · 2017-03-28T08:14:28Z

@jdm Shouldn't tests/wpt/mozilla/tests/mozilla/block-mime-as-script.html be in tests/wpt/web-platform-tests instead?

bors-servo · 2017-03-29T09:15:59Z

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

ferjm · 2017-03-29T12:23:36Z

Rebased and moved tests to tests/wpt/web-platform-tests

bors-servo · 2017-04-03T13:29:09Z

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

jdm · 2017-04-03T14:46:07Z

Seems good to me.

nox · 2017-04-03T15:43:12Z

components/net/fetch/methods.rs

                // Defer rebinding result
-                blocked_error_response = Response::network_error(NetworkError::Internal("Blocked by nosniff".into()));
+                blocked_error_response = Response::network_error(
+                    NetworkError::Internal("Blocked by nosniff or mime type".into()));


Please instead do a else if block and separate the two blocking errors.

ferjm · 2017-04-03T17:12:06Z

Done. r? @nox

nox · 2017-04-05T07:15:49Z

@bors-servo r+

bors-servo · 2017-04-05T07:15:49Z

📌 Commit 29a56c4 has been approved by nox

bors-servo · 2017-04-05T07:34:29Z

⌛ Testing commit 29a56c4 with merge 1071c33...

Block scripts with text/csv, audio/*, video/* and image/* mime types This patch implements step 12 of the Main Fetch section of the Fetch API standard. It blocks the load of scripts with `text/csv`, `audio/*`, `video/*` and `image/*` mime types. Credit for the logic of `should_block_mime_type` function should go to the author of #14770. - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #14520 - [X] There are tests for these changes  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/16126)

bors-servo · 2017-04-05T08:14:32Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-msvc-dev
Approved by: nox
Pushing 1071c33 to master...

Upstreamed from servo/servo#16126 [ci skip]

highfive assigned Manishearth Mar 24, 2017

highfive added the S-awaiting-review There is new code that needs to be reviewed. label Mar 24, 2017

highfive assigned asajeffrey and unassigned Manishearth Mar 24, 2017

ferjm force-pushed the issue-14520-block-media-csv branch from a63e21c to 2c3b985 Compare March 24, 2017 21:11

nox suggested changes Mar 25, 2017

View reviewed changes

nox assigned nox and unassigned asajeffrey Mar 25, 2017

nox added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. and removed S-awaiting-review There is new code that needs to be reviewed. labels Mar 25, 2017

ferjm force-pushed the issue-14520-block-media-csv branch from 2c3b985 to e9915f3 Compare March 27, 2017 10:21

highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels Mar 27, 2017

nox approved these changes Mar 28, 2017

View reviewed changes

nox added S-needs-squash Some (or all) of the commits in the PR should be combined. and removed S-awaiting-review There is new code that needs to be reviewed. labels Mar 28, 2017

ferjm force-pushed the issue-14520-block-media-csv branch from e9915f3 to e91c177 Compare March 28, 2017 08:10

highfive added the S-awaiting-review There is new code that needs to be reviewed. label Mar 28, 2017

nox added S-awaiting-answer Someone asked a question that requires an answer. and removed S-awaiting-review There is new code that needs to be reviewed. S-needs-squash Some (or all) of the commits in the PR should be combined. labels Mar 28, 2017

highfive added the S-needs-rebase There are merge conflict errors. label Mar 29, 2017

ferjm force-pushed the issue-14520-block-media-csv branch from e91c177 to d6dab7b Compare March 29, 2017 11:48

highfive added the S-awaiting-review There is new code that needs to be reviewed. label Mar 29, 2017

ferjm removed the S-needs-rebase There are merge conflict errors. label Mar 29, 2017

highfive added the S-needs-rebase There are merge conflict errors. label Apr 3, 2017

ferjm force-pushed the issue-14520-block-media-csv branch from d6dab7b to cf44fab Compare April 3, 2017 14:39

ferjm removed the S-needs-rebase There are merge conflict errors. label Apr 3, 2017

jdm removed the S-awaiting-answer Someone asked a question that requires an answer. label Apr 3, 2017

nox reviewed Apr 3, 2017

View reviewed changes

nox added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. and removed S-awaiting-review There is new code that needs to be reviewed. labels Apr 3, 2017

Block scripts with text/csv, audio/*, video/* and image/* mime types

29a56c4

ferjm force-pushed the issue-14520-block-media-csv branch from cf44fab to 29a56c4 Compare April 3, 2017 16:25

highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels Apr 3, 2017

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels Apr 5, 2017

bors-servo merged commit 29a56c4 into servo:master Apr 5, 2017

highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Apr 5, 2017

ferjm deleted the issue-14520-block-media-csv branch April 5, 2017 08:52

jdm pushed a commit to web-platform-tests/wpt that referenced this pull request Apr 17, 2017

Block scripts with text/csv, audio/*, video/* and image/* mime types

599246f

Upstreamed from servo/servo#16126 [ci skip]

jdm mentioned this pull request Apr 17, 2017

Block scripts with text/csv, audio/*, video/* and image/* mime types web-platform-tests/wpt#5584

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Block scripts with text/csv, audio/, video/ and image/* mime types #16126

Block scripts with text/csv, audio/, video/ and image/* mime types #16126

ferjm commented Mar 24, 2017 •

edited by larsbergstrom

Loading

highfive commented Mar 24, 2017

highfive commented Mar 24, 2017

ferjm commented Mar 24, 2017

nox left a comment

nox Mar 25, 2017

nox Mar 25, 2017

KiChjang Mar 25, 2017

nox Mar 26, 2017

nox Mar 25, 2017

ferjm commented Mar 27, 2017

nox commented Mar 28, 2017

ferjm commented Mar 28, 2017

nox commented Mar 28, 2017

bors-servo commented Mar 29, 2017

ferjm commented Mar 29, 2017

bors-servo commented Apr 3, 2017

jdm commented Apr 3, 2017

nox Apr 3, 2017

ferjm commented Apr 3, 2017

nox commented Apr 5, 2017

bors-servo commented Apr 5, 2017

bors-servo commented Apr 5, 2017

bors-servo commented Apr 5, 2017

Block scripts with text/csv, audio/*, video/* and image/* mime types #16126

Block scripts with text/csv, audio/*, video/* and image/* mime types #16126

Conversation

ferjm commented Mar 24, 2017 • edited by larsbergstrom Loading

highfive commented Mar 24, 2017

highfive commented Mar 24, 2017

ferjm commented Mar 24, 2017

nox left a comment

Choose a reason for hiding this comment

nox Mar 25, 2017

Choose a reason for hiding this comment

nox Mar 25, 2017

Choose a reason for hiding this comment

KiChjang Mar 25, 2017

Choose a reason for hiding this comment

nox Mar 26, 2017

Choose a reason for hiding this comment

nox Mar 25, 2017

Choose a reason for hiding this comment

ferjm commented Mar 27, 2017

nox commented Mar 28, 2017

ferjm commented Mar 28, 2017

nox commented Mar 28, 2017

bors-servo commented Mar 29, 2017

ferjm commented Mar 29, 2017

bors-servo commented Apr 3, 2017

jdm commented Apr 3, 2017

nox Apr 3, 2017

Choose a reason for hiding this comment

ferjm commented Apr 3, 2017

nox commented Apr 5, 2017

bors-servo commented Apr 5, 2017

bors-servo commented Apr 5, 2017

bors-servo commented Apr 5, 2017

Block scripts with text/csv, audio/, video/ and image/* mime types #16126

Block scripts with text/csv, audio/, video/ and image/* mime types #16126

ferjm commented Mar 24, 2017 •

edited by larsbergstrom

Loading