De-deplicate is_cors_safelisted_request_header helper functions #25236

pshaughn · 2019-12-10T20:46:25Z

Separate is_cors_safelisted_request_header implementations in script::dom::request and net::fetch::methods have been merged to a single implementation in net_traits::request, with additional logic for spec requirements that weren't previously there. This doesn't pass all the failing tests, but it doesn't fail any passing ones either and it reduces confusion about what's supposed to happen where.

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #25235 and some but not all subcases in #25175

There are tests for these changes, in that the WPT CORS tests that did already pass still do

highfive · 2019-12-10T20:46:33Z

Heads up! This PR modifies the following files:

@asajeffrey: components/script/dom/request.rs, components/script/dom/headers.rs
@KiChjang: components/script/dom/request.rs, components/net/fetch/methods.rs, components/script/dom/headers.rs, components/net_traits/request.rs, components/net/http_loader.rs

highfive · 2019-12-10T20:46:35Z

Warning

These commits modify net and script code, but no tests are modified. Please consider adding a test!

jdm · 2019-12-13T20:38:19Z

@bors-servo r+

bors-servo · 2019-12-13T20:38:21Z

📌 Commit 3272cfb has been approved by jdm

bors-servo · 2019-12-13T21:21:05Z

⌛ Testing commit 3272cfb with merge 3b9131a...


        Auto merge of #25236 - pshaughn:safelistct, r=jdm

De-deplicate is_cors_safelisted_request_header helper functions  Separate is_cors_safelisted_request_header implementations in script::dom::request and net::fetch::methods have been merged to a single implementation in net_traits::request, with additional logic for spec requirements that weren't previously there. This doesn't seem to pass any failing tests, but it doesn't fail any passing ones either and it reduces confusion about what's supposed to happen where. ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #25235  - [X] There are tests for these changes, in that the WPT CORS tests that did already pass still do

bors-servo · 2019-12-13T21:58:07Z

💔 Test failed - status-taskcluster

pshaughn · 2019-12-14T01:03:26Z

It turns out this did pass a few failing tests I hadn't tried, and after seeing that I looked back at the spec, saw a rule I missed, and made it pass more of them. I think everything else that went wrong in that test run was intermittents of some sort, but I stand to be proven wrong.

Don't retry yet! There's one more rule I overlooked.

jdm · 2019-12-14T01:58:26Z

Yes, there were a bunch of known intermittent failures in the non-cors test results.

CYBAI · 2019-12-16T01:31:53Z

Diff in /repo/components/net_traits/request.rs at line 554:
 ) -> bool {
     let name: &str = name.as_ref();
     let value: &[u8] = value.as_ref();
-    if value.len()>128 { return false; }
�(B+    if value.len() > 128 {
�(B+        return false;
�(B+    }
�(B     match name {
         "accept" => is_cors_safelisted_request_accept(value),
         "accept-language" | "content-language" => is_cors_safelisted_language(value),
clang-format not installed. Skipping CPP formatting.
Run `./mach fmt` to fix the formatting

pshaughn · 2019-12-16T13:56:26Z

Ready to retry; there are still some failing cases but I think I've gotten as far as I can on this without needing changes to how we use the hyper HeaderMap itself.

jdm · 2019-12-16T14:01:42Z

@bors-servo try=wpt

bors-servo · 2019-12-16T14:01:47Z

⌛ Trying commit 3028799 with merge 4b636d6...


        Auto merge of #25236 - pshaughn:safelistct, r=<try>

De-deplicate is_cors_safelisted_request_header helper functions  Separate is_cors_safelisted_request_header implementations in script::dom::request and net::fetch::methods have been merged to a single implementation in net_traits::request, with additional logic for spec requirements that weren't previously there. This doesn't pass all the failing tests, but it doesn't fail any passing ones either and it reduces confusion about what's supposed to happen where. ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #25235 and some but not all subcases in #25175  - [X] There are tests for these changes, in that the WPT CORS tests that did already pass still do


        Now just one is_cors_safelisted_request_header, with closer spec alig…

…nment

jdm · 2019-12-16T14:54:32Z

@bors-servo r+

bors-servo · 2019-12-16T14:54:34Z

📌 Commit 67827de has been approved by jdm

bors-servo · 2019-12-16T20:48:28Z

⌛ Testing commit 67827de with merge e7697b3...

bors-servo · 2019-12-16T21:19:28Z

💔 Test failed - status-taskcluster

jdm · 2019-12-16T21:35:45Z

@bors-servo retry

#24726

bors-servo · 2019-12-16T22:32:07Z

⌛ Testing commit 67827de with merge 3586a7f...


        Auto merge of #25236 - pshaughn:safelistct, r=jdm

De-deplicate is_cors_safelisted_request_header helper functions  Separate is_cors_safelisted_request_header implementations in script::dom::request and net::fetch::methods have been merged to a single implementation in net_traits::request, with additional logic for spec requirements that weren't previously there. This doesn't pass all the failing tests, but it doesn't fail any passing ones either and it reduces confusion about what's supposed to happen where. ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #25235 and some but not all subcases in #25175  - [X] There are tests for these changes, in that the WPT CORS tests that did already pass still do

bors-servo · 2019-12-16T23:10:09Z

💔 Test failed - status-taskcluster

jdm · 2019-12-16T23:21:33Z

@bors-servo retry

#24726

bors-servo · 2019-12-17T00:05:19Z

⌛ Testing commit 67827de with merge 64e90b0...


        Auto merge of #25236 - pshaughn:safelistct, r=jdm

De-deplicate is_cors_safelisted_request_header helper functions  Separate is_cors_safelisted_request_header implementations in script::dom::request and net::fetch::methods have been merged to a single implementation in net_traits::request, with additional logic for spec requirements that weren't previously there. This doesn't pass all the failing tests, but it doesn't fail any passing ones either and it reduces confusion about what's supposed to happen where. ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #25235 and some but not all subcases in #25175  - [X] There are tests for these changes, in that the WPT CORS tests that did already pass still do

bors-servo · 2019-12-17T01:09:40Z

💔 Test failed - status-taskcluster

jdm · 2019-12-17T01:18:13Z

@bors-servo retry

bors-servo · 2019-12-17T01:51:11Z

⌛ Testing commit 67827de with merge b274d59...


        Auto merge of #25236 - pshaughn:safelistct, r=jdm

De-deplicate is_cors_safelisted_request_header helper functions  Separate is_cors_safelisted_request_header implementations in script::dom::request and net::fetch::methods have been merged to a single implementation in net_traits::request, with additional logic for spec requirements that weren't previously there. This doesn't pass all the failing tests, but it doesn't fail any passing ones either and it reduces confusion about what's supposed to happen where. ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #25235 and some but not all subcases in #25175  - [X] There are tests for these changes, in that the WPT CORS tests that did already pass still do

bors-servo · 2019-12-17T02:08:57Z

☀️ Test successful - status-taskcluster
Approved by: jdm
Pushing b274d59 to master...

highfive assigned jdm Dec 10, 2019

highfive added the S-awaiting-review label Dec 10, 2019

jdm changed the title ~~Now just one fn is_cors_safelisted_request_header~~ De-deplicate is_cors_safelisted_request_header helper functions Dec 13, 2019

highfive added S-awaiting-merge and removed S-awaiting-review labels Dec 13, 2019

highfive added S-tests-failed and removed S-awaiting-merge labels Dec 13, 2019

pshaughn force-pushed the pshaughn:safelistct branch from 3272cfb to 1ac3909 Dec 14, 2019

highfive added S-awaiting-review and removed S-tests-failed labels Dec 14, 2019

pshaughn mentioned this pull request Dec 14, 2019

Appending multiple request headers with the same name isn't behaving as WPT expects #25290

Open

pshaughn force-pushed the pshaughn:safelistct branch from 1ac3909 to 3028799 Dec 16, 2019

Now just one is_cors_safelisted_request_header, with closer spec alig…

Loading status checks…

67827de

…nment

pshaughn force-pushed the pshaughn:safelistct branch from 3028799 to 67827de Dec 16, 2019

highfive added S-awaiting-merge and removed S-tests-failed labels Dec 16, 2019

highfive added S-tests-failed and removed S-awaiting-merge labels Dec 16, 2019

highfive added S-awaiting-merge and removed S-tests-failed labels Dec 16, 2019

highfive added S-tests-failed and removed S-awaiting-merge labels Dec 16, 2019

highfive added S-awaiting-merge and removed S-tests-failed labels Dec 17, 2019

highfive added S-tests-failed and removed S-awaiting-merge labels Dec 17, 2019

highfive added S-awaiting-merge and removed S-tests-failed labels Dec 17, 2019

bors-servo merged commit 67827de into servo:master Dec 17, 2019
2 checks passed

2 checks passed

Community-TC (pull_request) TaskGroup: success
Details

homu Test successful
Details

highfive removed the S-awaiting-merge label Dec 17, 2019

servo / servo

De-deplicate is_cors_safelisted_request_header helper functions #25236

De-deplicate is_cors_safelisted_request_header helper functions #25236

pshaughn commented Dec 10, 2019 •

edited

highfive commented Dec 10, 2019

highfive commented Dec 10, 2019

jdm commented Dec 13, 2019

bors-servo commented Dec 13, 2019

bors-servo commented Dec 13, 2019

bors-servo commented Dec 13, 2019

pshaughn commented Dec 14, 2019 •

edited

jdm commented Dec 14, 2019

CYBAI commented Dec 16, 2019

pshaughn commented Dec 16, 2019

jdm commented Dec 16, 2019

bors-servo commented Dec 16, 2019

jdm commented Dec 16, 2019

bors-servo commented Dec 16, 2019

bors-servo commented Dec 16, 2019

bors-servo commented Dec 16, 2019

jdm commented Dec 16, 2019

bors-servo commented Dec 16, 2019

bors-servo commented Dec 16, 2019

jdm commented Dec 16, 2019

bors-servo commented Dec 17, 2019

bors-servo commented Dec 17, 2019

jdm commented Dec 17, 2019

bors-servo commented Dec 17, 2019

bors-servo commented Dec 17, 2019

servo / servo

Join GitHub today

De-deplicate is_cors_safelisted_request_header helper functions #25236

De-deplicate is_cors_safelisted_request_header helper functions #25236

Conversation

pshaughn commented Dec 10, 2019 • edited

highfive commented Dec 10, 2019

highfive commented Dec 10, 2019

jdm commented Dec 13, 2019

bors-servo commented Dec 13, 2019

bors-servo commented Dec 13, 2019

bors-servo commented Dec 13, 2019

pshaughn commented Dec 14, 2019 • edited

jdm commented Dec 14, 2019

CYBAI commented Dec 16, 2019

pshaughn commented Dec 16, 2019

jdm commented Dec 16, 2019

bors-servo commented Dec 16, 2019

jdm commented Dec 16, 2019

bors-servo commented Dec 16, 2019

bors-servo commented Dec 16, 2019

bors-servo commented Dec 16, 2019

jdm commented Dec 16, 2019

bors-servo commented Dec 16, 2019

bors-servo commented Dec 16, 2019

jdm commented Dec 16, 2019

bors-servo commented Dec 17, 2019

bors-servo commented Dec 17, 2019

jdm commented Dec 17, 2019

bors-servo commented Dec 17, 2019

bors-servo commented Dec 17, 2019

pshaughn commented Dec 10, 2019 •

edited

pshaughn commented Dec 14, 2019 •

edited