Fixes #14787: Set Origin header in http_network_or_cache_fetch #14811

rabisg · 2017-01-01T12:44:31Z

Sets Origin header on request with CORS flag set or on requests other
than those with GET/HEAD methods

There are 2 TODOs marked in the current implementation:

Set Origin to be Client origin when client object is implemented in Request
Set Origin to null (OpaqueOrigin) when hyper supports it (This seems to be missing from the current implementation in hyper)

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix Set origin header in http_network_or_cache_fetch #14787

There are tests for these changes

This change is

highfive · 2017-01-01T12:44:37Z

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

highfive · 2017-01-01T12:44:37Z

Heads up! This PR modifies the following files:

@KiChjang: components/net/http_loader.rs

bors-servo · 2017-01-02T22:18:41Z

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

KiChjang · 2017-01-03T08:19:06Z

Sorry, but could you remove your merge commit and rebase against the master branch instead? We don't accept PRs that contain merge commits.

rabisg · 2017-01-03T08:42:55Z

Sorry was trying Github resolve conflict for the first time. Didn't realise it created a merge commit. Fixed that.

Sets Origin header on request with CORS flag set or on requests other than those with GET/HEAD methods

jdm · 2017-01-12T04:23:55Z

@wafflespeanut Review ping!

wafflespeanut · 2017-01-12T04:25:08Z

I don't even recall seeing this. Sorry, will review it in a bit :)

wafflespeanut

Sorry for the delay in review. Just a few comments 😄

wafflespeanut · 2017-01-12T13:33:50Z

components/net/http_loader.rs

-        // TODO update this when https://github.com/hyperium/hyper/pull/691 is finished
-        // http_request.headers.borrow_mut().set_raw("origin", origin);
+    // Step 7
+    if http_request.omit_origin_header.get() == false {


Nit: This should be,

if !http_request.omit_origin_header.get() {

wafflespeanut · 2017-01-12T13:35:01Z

components/net/http_loader.rs

-      (*http_request.method.borrow() != Method::Get && *http_request.method.borrow() != Method::Head) {
-        // TODO update this when https://github.com/hyperium/hyper/pull/691 is finished
-        // http_request.headers.borrow_mut().set_raw("origin", origin);
+    // Step 7


This is step 9.

wafflespeanut · 2017-01-12T13:43:07Z

components/net/http_loader.rs

+        if cors_flag || (*method != Method::Get && *method != Method::Head) {
+            match *http_request.origin.borrow() {
+                Origin::Origin(ref url_origin) =>
+                    match try_url_origin_to_hyper_origin(url_origin.clone()) {


We don't have to clone if we don't move anything from it. We can just pass the reference, deref it, and clone stuff only when needed. Also, this could be rewritten with if let as,

if let Some(hyper_origin) = try_url_origin_to_hyper_origin(url_origin) { // }

wafflespeanut · 2017-01-12T14:06:06Z

components/net/http_loader.rs

+    // Step 7
+    if http_request.omit_origin_header.get() == false {
+        let method = http_request.method.borrow();
+        if cors_flag || (*method != Method::Get && *method != Method::Head) {


According to the spec, the request's origin will be changed to "client" during fetching. We should have a debug_assert!(*http_request.origin.borrow() != Origin::Client) and go with if let here.

wafflespeanut · 2017-01-22T15:35:57Z

@rabisg Any plans to finish this off?

jdm · 2017-02-03T22:12:13Z

Closing due to lack of activity.

Fixes #14787 building on #14811: Set Origin header in http_network_or_cache_fetch Incorporates review feedback from #14811 I noticed that the "steps" in the comments don't necessarily correspond to the steps in the current fetch standard. This adds step 11 in the spec, but the steps commented in the implementation look wrong. --- - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #14787. - [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/15547)

Allow for redirects after a CORS-preflight ~Continue the work done in #14811 and #15547~. And applies the Fetch [spec change](whatwg/fetch@0d9a4db) about [allowing redirects after CORS preflights](whatwg/fetch#204). - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix ~#14787 and~ #14519 - [X] There are tests for these changes. Several WPTs go from FAIL to SUCCESS. The new expected FAILs for the cors/redirect-preflight.htm tests are caused by the spec change done [here](whatwg/fetch@0d9a4db), as suggested in the associated [issue](#14519 (comment)).  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/15889)

highfive assigned wafflespeanut Jan 1, 2017

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

rabisg force-pushed the set-origin-header branch 2 times, most recently from 75f4077 to 234fd2d Compare January 2, 2017 18:34

rabisg mentioned this pull request Jan 2, 2017

Set origin header in http_network_or_cache_fetch #14787

Closed

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

rabisg force-pushed the set-origin-header branch from 148ed45 to 2ff8d00 Compare January 3, 2017 08:41

Fixes servo#14787: Set Origin header in http_network_or_cache_fetch

010b937

Sets Origin header on request with CORS flag set or on requests other than those with GET/HEAD methods

rabisg force-pushed the set-origin-header branch from 2ff8d00 to 010b937 Compare January 3, 2017 11:49

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

wafflespeanut reviewed Jan 12, 2017

View reviewed changes

jdm 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 Jan 19, 2017

jdm closed this Feb 3, 2017

samgiles mentioned this pull request Feb 14, 2017

Fixes #14787 building on #14811: Set Origin header in http_network_or_cache_fetch #15547

Closed

4 tasks

ferjm mentioned this pull request Mar 9, 2017

Allow for redirects after a CORS-preflight #15889

Merged

4 tasks

wafflespeanut mentioned this pull request Mar 10, 2017

Set Origin header in http_network_or_cache_fetch #15903

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #14787: Set Origin header in http_network_or_cache_fetch #14811

Fixes #14787: Set Origin header in http_network_or_cache_fetch #14811

rabisg commented Jan 1, 2017 •

edited by larsbergstrom

Loading

highfive commented Jan 1, 2017

highfive commented Jan 1, 2017

bors-servo commented Jan 2, 2017

KiChjang commented Jan 3, 2017

rabisg commented Jan 3, 2017

jdm commented Jan 12, 2017

wafflespeanut commented Jan 12, 2017

wafflespeanut left a comment

wafflespeanut Jan 12, 2017

wafflespeanut Jan 12, 2017

wafflespeanut Jan 12, 2017

wafflespeanut Jan 12, 2017

wafflespeanut commented Jan 22, 2017

jdm commented Feb 3, 2017

Fixes #14787: Set Origin header in http_network_or_cache_fetch #14811

Fixes #14787: Set Origin header in http_network_or_cache_fetch #14811

Conversation

rabisg commented Jan 1, 2017 • edited by larsbergstrom Loading

highfive commented Jan 1, 2017

highfive commented Jan 1, 2017

bors-servo commented Jan 2, 2017

KiChjang commented Jan 3, 2017

rabisg commented Jan 3, 2017

jdm commented Jan 12, 2017

wafflespeanut commented Jan 12, 2017

wafflespeanut left a comment

Choose a reason for hiding this comment

wafflespeanut Jan 12, 2017

Choose a reason for hiding this comment

wafflespeanut Jan 12, 2017

Choose a reason for hiding this comment

wafflespeanut Jan 12, 2017

Choose a reason for hiding this comment

wafflespeanut Jan 12, 2017

Choose a reason for hiding this comment

wafflespeanut commented Jan 22, 2017

jdm commented Feb 3, 2017

rabisg commented Jan 1, 2017 •

edited by larsbergstrom

Loading