Include "content-type" in cors safelisted request headers. #12915

jeenalee · 2016-08-17T22:16:43Z

The changes in headers.rs will allow headers with "content-type" name to be classified as cors safelisted request headers, depending on its value according to the Fetch spec. As a result of this change, more request web platform tests pass, whose expected test results are updated with this commit.

There is possibly one TODO related to this PR:

Figure out what name/'invalid' is in step 4 of the Headers Delete method, and how to implement that.

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #__ (github issue number if applicable).

There are tests for these changes OR
These changes do not require tests because web platform tests for the changes already exist.

This commit allows headers with "content-type" name to be classified as valid header name, depending on its value according to the Fetch spec. As a result of this change, more request web platform tests pass, whose expected test results are updated as well.

This change is

KiChjang · 2016-08-17T22:23:25Z

components/script/dom/headers.rs

+        match value_without_parameter {
+            "application/x-www-form-urlencoded" |
+            "multipart/form-data" |
+            "text/plain" => Ok(true),


I'm quite sad that all of this infrastructure isn't using the mime crate.

I didn't know about mime! Thanks for letting me know. Looks like it will be really helpful. I'll look into it.

Yes. We should parse the value first, and operate on the parsed Header instance, matching on its type.

This is important because some of the other headers (e.g. Downlink) are only safe if they are parsed correctly.

Actually, hyper doesn't expose clean APIs for dealing with this, so for now it's fine if you just parse as mime type and ignore the others.

jeenalee · 2016-08-22T17:12:37Z

Hi @Manishearth, I updated the script to use the mime crate. When you have a moment, can you review it please? Thanks!

Manishearth · 2016-08-22T17:35:24Z

components/script/dom/headers.rs

-        if self.guard.get() == Guard::RequestNoCors && !is_cors_safelisted_request_header(&valid_name) {
-            return Ok(());
-        }
+        // TODO: Requires clarification from the spec.


link to spec bug here please

Manishearth · 2016-08-22T17:37:04Z

components/script/dom/headers.rs

-// "DPR", "Downlink", "Save-Data", "Viewport-Width", "Width":
-// once parsed, the value should not be failure.
+fn is_cors_safelisted_request_content_type(value: &[u8]) -> Result<bool, Error> {
+    let value_string = try!(str::from_utf8(value).map_err(


The error doesn't get used in the end. What about

let value_string = if let Ok(s) = str::from_utf8(value) { s } else { return false; }

and then just return a bool

Manishearth · 2016-08-22T17:37:51Z

r=me with those two issues fixed


        Include "content-type" in cors safelisted request headers.

This commit allows headers with "content-type" name to be classified as valid header name, depending on its value according to [the Fetch spec](https://fetch.spec.whatwg.org/#cors-safelisted-request-header). As a result of this change, more request web platform tests pass, whose expected test results are updated with this commit.

jeenalee · 2016-08-22T18:13:28Z

@Manishearth The two issues have been fixed. Thank you!

Manishearth · 2016-08-22T18:18:12Z

@bors-servo r+

bors-servo · 2016-08-22T18:18:13Z

📌 Commit cc7b1c5 has been approved by Manishearth

bors-servo · 2016-08-22T18:28:42Z

⌛ Testing commit cc7b1c5 with merge 148647d...


        Auto merge of #12915 - jeenalee:parse-headers, r=Manishearth

Include "content-type" in cors safelisted request headers.  The changes in headers.rs will allow headers with "content-type" name to be classified as cors safelisted request headers, depending on its value according to [the Fetch spec](https://fetch.spec.whatwg.org/#cors-safelisted-request-header). As a result of this change, more request web platform tests pass, whose expected test results are updated with this commit. There is possibly one TODO related to this PR: - Figure out what `name/'invalid'` is in step 4 of the [Headers Delete method](https://fetch.spec.whatwg.org/#dom-headers-delete), and how to implement that. ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [ ] These changes fix #__ (github issue number if applicable).  - [ ] There are tests for these changes OR - [X] These changes do not require tests because web platform tests for the changes already exist.  This commit allows headers with "content-type" name to be classified as valid header name, depending on its value according to [the Fetch spec](https://fetch.spec.whatwg.org/#cors-safelisted-request-header). As a result of this change, more request web platform tests pass, whose expected test results are updated as well.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12915)

bors-servo · 2016-08-22T19:15:22Z

💔 Test failed - linux-rel

highfive · 2016-08-22T19:15:24Z

  ▶ FAIL [expected PASS] /css-transforms-1_dev/html/transform-table-007.htm
  └   → /css-transforms-1_dev/html/transform-table-007.htm a5c014b20ef1363bea6f24eda28c7efb7c45698a
/css-transforms-1_dev/html/reference/transform-blank-ref.htm fa6407b1acbbfea27e27061e7d1bdeca98e4a728
Testing a5c014b20ef1363bea6f24eda28c7efb7c45698a == fa6407b1acbbfea27e27061e7d1bdeca98e4a728

jeenalee · 2016-08-22T19:28:10Z

Hmm, the /css-transforms-1_dev/html/transform-table-007.htm test passes locally...

Manishearth · 2016-08-22T19:28:26Z

@bors-servo retry

#11574

bors-servo · 2016-08-22T20:12:39Z

⚡ Previous build results for arm32, arm64, linux-dev, mac-dev-unit, mac-rel-css, mac-rel-wpt, windows-dev are reusable. Rebuilding only linux-rel...

bors-servo · 2016-08-22T21:00:30Z

💔 Test failed - linux-rel

highfive · 2016-08-22T21:00:32Z

  ▶ FAIL [expected PASS] /css-transforms-1_dev/html/transform-table-007.htm
  └   → /css-transforms-1_dev/html/transform-table-007.htm a5c014b20ef1363bea6f24eda28c7efb7c45698a
/css-transforms-1_dev/html/reference/transform-blank-ref.htm fa6407b1acbbfea27e27061e7d1bdeca98e4a728
Testing a5c014b20ef1363bea6f24eda28c7efb7c45698a == fa6407b1acbbfea27e27061e7d1bdeca98e4a728

jdm · 2016-08-22T22:06:43Z

@bors-servo: retry

##11574


        Update headers.rs to version that includes "content-type"

The version that includes "content-type" in valid header name is from [PR 12915](servo#12915).

bors-servo · 2016-08-22T23:25:46Z

⌛ Testing commit cc7b1c5 with merge 117a39f...


        Auto merge of #12915 - jeenalee:parse-headers, r=Manishearth

Include "content-type" in cors safelisted request headers.  The changes in headers.rs will allow headers with "content-type" name to be classified as cors safelisted request headers, depending on its value according to [the Fetch spec](https://fetch.spec.whatwg.org/#cors-safelisted-request-header). As a result of this change, more request web platform tests pass, whose expected test results are updated with this commit. There is possibly one TODO related to this PR: - Figure out what `name/'invalid'` is in step 4 of the [Headers Delete method](https://fetch.spec.whatwg.org/#dom-headers-delete), and how to implement that. ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [ ] These changes fix #__ (github issue number if applicable).  - [ ] There are tests for these changes OR - [X] These changes do not require tests because web platform tests for the changes already exist.  This commit allows headers with "content-type" name to be classified as valid header name, depending on its value according to [the Fetch spec](https://fetch.spec.whatwg.org/#cors-safelisted-request-header). As a result of this change, more request web platform tests pass, whose expected test results are updated as well.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12915)

bors-servo · 2016-08-23T00:20:17Z

💔 Test failed - linux-rel

jdm · 2016-08-23T00:42:27Z

@bors-servo: retry

#12592

bors-servo · 2016-08-23T01:17:16Z

⌛ Testing commit cc7b1c5 with merge 1e10f8b...


        Auto merge of #12915 - jeenalee:parse-headers, r=Manishearth

Include "content-type" in cors safelisted request headers.  The changes in headers.rs will allow headers with "content-type" name to be classified as cors safelisted request headers, depending on its value according to [the Fetch spec](https://fetch.spec.whatwg.org/#cors-safelisted-request-header). As a result of this change, more request web platform tests pass, whose expected test results are updated with this commit. There is possibly one TODO related to this PR: - Figure out what `name/'invalid'` is in step 4 of the [Headers Delete method](https://fetch.spec.whatwg.org/#dom-headers-delete), and how to implement that. ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [ ] These changes fix #__ (github issue number if applicable).  - [ ] There are tests for these changes OR - [X] These changes do not require tests because web platform tests for the changes already exist.  This commit allows headers with "content-type" name to be classified as valid header name, depending on its value according to [the Fetch spec](https://fetch.spec.whatwg.org/#cors-safelisted-request-header). As a result of this change, more request web platform tests pass, whose expected test results are updated as well.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12915)

bors-servo · 2016-08-23T02:12:21Z

☀️ Test successful - arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt, windows-dev

highfive assigned Manishearth Aug 17, 2016

highfive added the S-awaiting-review label Aug 17, 2016

KiChjang reviewed Aug 17, 2016
View changes

jeenalee force-pushed the jeenalee:parse-headers branch from d2ae750 to 7b4f654 Aug 18, 2016

Manishearth reviewed Aug 22, 2016
View changes

jeenalee force-pushed the jeenalee:parse-headers branch from 7b4f654 to cc7b1c5 Aug 22, 2016

highfive added S-awaiting-merge and removed S-awaiting-review labels Aug 22, 2016

highfive added S-tests-failed and removed S-awaiting-merge labels Aug 22, 2016

highfive added the S-tests-failed label Aug 22, 2016

jeenalee added a commit to jeenalee/servo that referenced this pull request Aug 22, 2016

Update headers.rs to version that includes "content-type"

a62888c

The version that includes "content-type" in valid header name is from [PR 12915](servo#12915).

highfive added S-awaiting-merge and removed S-tests-failed labels Aug 22, 2016

highfive added S-tests-failed and removed S-awaiting-merge labels Aug 23, 2016

highfive added S-awaiting-merge and removed S-tests-failed labels Aug 23, 2016

bors-servo merged commit cc7b1c5 into servo:master Aug 23, 2016
3 checks passed

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

highfive removed the S-awaiting-merge label Aug 23, 2016

bors-servo mentioned this pull request Aug 23, 2016

Wrap hyper::header::Headers in Rc #12884

Closed

3 of 5 tasks complete

jeenalee deleted the jeenalee:parse-headers branch Aug 24, 2016

servo / servo

Include "content-type" in cors safelisted request headers. #12915

Include "content-type" in cors safelisted request headers. #12915

jeenalee commented Aug 17, 2016 •

edited

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

jeenalee commented Aug 22, 2016

This comment has been minimized.

This comment has been minimized.

Manishearth commented Aug 22, 2016

jeenalee commented Aug 22, 2016

Manishearth commented Aug 22, 2016

bors-servo commented Aug 22, 2016

bors-servo commented Aug 22, 2016

bors-servo commented Aug 22, 2016

highfive commented Aug 22, 2016

jeenalee commented Aug 22, 2016

Manishearth commented Aug 22, 2016

bors-servo commented Aug 22, 2016

bors-servo commented Aug 22, 2016

highfive commented Aug 22, 2016

jdm commented Aug 22, 2016

bors-servo commented Aug 22, 2016

bors-servo commented Aug 23, 2016

jdm commented Aug 23, 2016

bors-servo commented Aug 23, 2016

bors-servo commented Aug 23, 2016

servo / servo

Join GitHub today

Include "content-type" in cors safelisted request headers. #12915

Include "content-type" in cors safelisted request headers. #12915

Conversation

jeenalee commented Aug 17, 2016 • edited

This comment has been minimized.

KiChjang Aug 17, 2016

This comment has been minimized.

jeenalee Aug 17, 2016 • edited

This comment has been minimized.

Manishearth Aug 18, 2016 • edited

This comment has been minimized.

Manishearth Aug 18, 2016

jeenalee commented Aug 22, 2016

This comment has been minimized.

Manishearth Aug 22, 2016

This comment has been minimized.

Manishearth Aug 22, 2016 • edited

Manishearth commented Aug 22, 2016

jeenalee commented Aug 22, 2016

Manishearth commented Aug 22, 2016

bors-servo commented Aug 22, 2016

bors-servo commented Aug 22, 2016

bors-servo commented Aug 22, 2016

highfive commented Aug 22, 2016

jeenalee commented Aug 22, 2016

Manishearth commented Aug 22, 2016

bors-servo commented Aug 22, 2016

bors-servo commented Aug 22, 2016

highfive commented Aug 22, 2016

jdm commented Aug 22, 2016

bors-servo commented Aug 22, 2016

bors-servo commented Aug 23, 2016

jdm commented Aug 23, 2016

bors-servo commented Aug 23, 2016

bors-servo commented Aug 23, 2016

jeenalee commented Aug 17, 2016 •

edited

jeenalee Aug 17, 2016 •

edited

Manishearth Aug 18, 2016 •

edited

Manishearth Aug 22, 2016 •

edited