Referer header #10696

rebstar6 · 2016-04-19T00:42:47Z

This puts the code and data structures in place to set the Referer header based on the Referrer Policy for a given document. Note that document:: get_referrer_policy() always returns the 'No Referrer' option, so for now, this should have no impact on production code, and that policy requires that the Referer header is not added.

Later PRs will determine the policy and edit that get_referrer_policy() accordingly.

This change is

highfive · 2016-04-19T00:42:51Z

Heads up! This PR modifies the following files:

@KiChjang: components/net/file_loader.rs, components/script/dom/bindings/trace.rs, components/net/http_loader.rs, components/script/dom/xmlhttprequest.rs, components/net_traits/lib.rs, components/net_traits/lib.rs, components/script/document_loader.rs, components/script/script_thread.rs, components/script/dom/window.rs, components/script/dom/document.rs, components/net/image_cache_thread.rs

rebstar6 · 2016-04-19T00:44:03Z

@jdm fyi on this PR. Also, merge conflicts, lame. I'll look into addressing.

jdm · 2016-04-19T03:37:59Z

r? @jdm

bors-servo · 2016-04-19T06:59:53Z

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

rebstar6 · 2016-04-19T18:31:47Z

Want to point out - LoadData creation with referrer in components/msg/constellation_msg.rs is handled with a new_with_referrer() method. In components/net_traits/lib.rs, I edited the new() method to just add these 2 new fields in.

I assume I should make them consistent, but wanted your opinion on which is better 1st. new_with_referrer means less changes to track down throughout the code, and you don't just have "None, None" thrown on the end most of the time you are creating. Editing new is maybe cleaner though, considering there may be future adds.

Either way.

jdm · 2016-04-19T21:42:33Z

Yeah, I'd prefer to stay with editing the new method for now just to make sure we catch all the right places.

As for these changes, great work! I'm a bit leery of the way we pass around URLs and strings in the main referrer determination algorithm, so I'd like to see what that looks like if we write it more like the specification algorithm and only deal in URLs instead.

-S-awaiting-review +S-needs-code-changes

Reviewed 3 of 15 files at r1, 12 of 12 files at r2, 6 of 6 files at r4.
Review status: all files reviewed at latest revision, 21 unresolved discussions.

components/msg/constellation_msg.rs, line 420 [r4] (raw file):
Let's go with [Policies](https://w3c.github.io/webappsec-referrer-policy/#referrer-policy-states) for providing a referrer header for a request

components/net/http_loader.rs, line 369 [r2] (raw file):
This can accept &Url values. Let's just call serialize instead of accepting the serialized form as an argument as well.

components/net/http_loader.rs, line 370 [r2] (raw file):
Yes.

components/net/http_loader.rs, line 380 [r2] (raw file):
That is correct.

components/net/http_loader.rs, line 381 [r2] (raw file):
We should be relying on the Url::origin method here instead.

components/net/http_loader.rs, line 388 [r2] (raw file):
mut referrer_url instead of creating ref_mutable below.

components/net/http_loader.rs, line 391 [r2] (raw file):
relative.path.truncate()

components/net/http_loader.rs, line 398 [r2] (raw file):
mut referrer_url: Url
Also, nonorigin in the name is unclear. I assume this is referring to the origin-only flag specified in the original algorithm? I'd prefer to implement the algorithm from the specification as written (ie. as single method that accepts a boolean argument), since it's much easier to reason about the results in the caller than comparing the implementation of generate_nonorigin_referer_url to generate_origin_referer_url and the specification.

components/net/http_loader.rs, line 401 [r2] (raw file):
No need for this.

components/net/http_loader.rs, line 413 [r2] (raw file):
Let's have this return an Option<Url> instead of modifying the header directly. Then we could call this determine_request_referrer and have it more clearly match the specification algorithm.

components/net/http_loader.rs, line 416 [r2] (raw file):
We should be able to assert that this is false instead.

components/net/http_loader.rs, line 427 [r2] (raw file):
Let's use explicit patterns here instead of the catch-all _.

components/net/http_loader.rs, line 412 [r4] (raw file):
The proper way to support step 2 and related bits about redirections isn't totally clear to me right now. Let's add some TODO comments to call that out and deal with it later.

components/net_traits/lib.rs, line 90 [r2] (raw file):
I'd go with /// The policy and referring URL for the originator of this request

components/script/script_thread.rs, line 1886 [r2] (raw file):
Is this still relevant?

components/script/dom/document.rs, line 229 [r2] (raw file):
Let's make this link https://w3c.github.io/webappsec-referrer-policy/#referrer-policy-states

components/script/dom/document.rs, line 1689 [r2] (raw file):
Let's initialize this to NoReferrer.

components/script/dom/document.rs, line 1821 [r2] (raw file):
Let's make this return self.referrer_policy

components/script/dom/xmlhttprequest.rs, line 585 [r2] (raw file):
For future reference, we'll want to add an API to GlobalRef that returns a ReferrerPolicy value; we can use global.get_url() for the other I believe.

tests/unit/net/http_loader.rs, line 1461 [r2] (raw file):
The explicit type after load isn't necessary any more.

tests/unit/net/http_loader.rs, line 1470 [r2] (raw file):
Since these tests have so much code in common, let's merge them like so:

fn assert_referer_header_matches(request_url: &str, referrer_url: &str, referrer_policy: Option<ReferrerPolicy>, expected_referrer: &str) {
    // set up the URLs, create the LoadData, set the header value, call load
}

Then we can either have a bunch of different #[test] functions that all delegate to that one, or just one that contains a bunch of tests.

Comments from Reviewable

bors-servo · 2016-04-20T19:15:57Z

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

rebstar6 · 2016-04-21T01:40:04Z

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

components/msg/constellation_msg.rs, line 420 [r4] (raw file):
Done.

components/net/http_loader.rs, line 369 [r2] (raw file):
Done.

components/net/http_loader.rs, line 381 [r2] (raw file):
Done.

components/net/http_loader.rs, line 391 [r2] (raw file):
Done.

components/net/http_loader.rs, line 398 [r2] (raw file):
Done.

components/net/http_loader.rs, line 416 [r2] (raw file):
No explicit method for it - this is how the other headers are handled fyi.

components/net/http_loader.rs, line 427 [r2] (raw file):
Done.

components/net/http_loader.rs, line 412 [r4] (raw file):
Done.

components/script/script_thread.rs, line 1886 [r2] (raw file):
nope.

components/script/dom/document.rs, line 229 [r2] (raw file):
Done.

components/script/dom/document.rs, line 1689 [r2] (raw file):
Done.

Comments from Reviewable

rebstar6 · 2016-04-21T01:40:31Z

FYI - code comments addressed. Test changes are still a todo.

rebstar6 · 2016-04-25T01:06:07Z

Please re-review the strip_url method after the latest commit - looks like rust-url was updated, so that method is changed.

Latest commit has that change and all comments to this point addressed.

jdm · 2016-04-25T14:44:32Z

Looking great!
-S-awaiting-review +S-needs-code-changes

Reviewed 11 of 15 files at r9, 4 of 4 files at r10.
Review status: all files reviewed at latest revision, 4 unresolved discussions.

components/net/http_loader.rs, line 416 [r2] (raw file):
Any headers object we pass in here should not already have a Referer header; APIs like XMLHttpRequest/Fetch forbid setting it, and other browser code shouldn't be setting it either. We don't carry over headers that were set by the browser prior to a redirect, either.

components/net/http_loader.rs, line 369 [r10] (raw file):
Add .unwrap() to each of the lines that complains about unused results. These operations shouldn't fail, and we'll want to know if they do, so unwrap is the right thing here..

tests/unit/net/http_loader.rs, line 1565 [r10] (raw file):
Shouldn't this be https?

tests/unit/net/http_loader.rs, line 1575 [r10] (raw file):
The test is called http_to_http...

Comments from Reviewable

rebstar6 · 2016-04-25T18:19:29Z

Review status: all files reviewed at latest revision, 4 unresolved discussions.

components/net/http_loader.rs, line 416 [r2] (raw file):
Done.

components/net/http_loader.rs, line 369 [r10] (raw file):
Done.

tests/unit/net/http_loader.rs, line 1565 [r10] (raw file):
Oops, flipped these two.

tests/unit/net/http_loader.rs, line 1575 [r10] (raw file):
Done.

Comments from Reviewable

jdm · 2016-04-25T18:23:42Z

A+ would review again. Go ahead and squash the commits into one!
-S-awaiting-review +S-needs-squash

Reviewed 2 of 2 files at r11.
Review status: all files reviewed at latest revision, all discussions resolved.

Comments from Reviewable


        Add referrer policy pass-through and referrer header logic

add pass-through from doc to http-loader for referrer_policy, ref_URL add logic for setting referer header add script pass-through for referrer add unit tests for setting referer header

jdm · 2016-04-25T20:51:59Z

@bors-servo: r+

bors-servo · 2016-04-25T20:51:59Z

📌 Commit 526525b has been approved by jdm

bors-servo · 2016-04-25T20:52:02Z

⌛ Testing commit 526525b with merge 3490081...


        Auto merge of #10696 - rebstar6:referrerPolicy, r=jdm

Referer header PR1 for #10311 This puts the code and data structures in place to set the Referer header based on the Referrer Policy for a given document. Note that document:: get_referrer_policy() always returns the 'No Referrer' option, so for now, this should have no impact on production code, and that policy requires that the Referer header is not added. Later PRs will determine the policy and edit that get_referrer_policy() accordingly.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/10696)

bors-servo · 2016-04-25T21:37:43Z

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

highfive assigned asajeffrey Apr 19, 2016

highfive added the S-awaiting-review label Apr 19, 2016

rebstar6 force-pushed the rebstar6:referrerPolicy branch from b1d5b51 to 1460ccf Apr 19, 2016

highfive assigned jdm and unassigned asajeffrey Apr 19, 2016

highfive added the S-needs-rebase label Apr 19, 2016

rebstar6 force-pushed the rebstar6:referrerPolicy branch from 1460ccf to 7e64c9b Apr 19, 2016

jdm removed the S-needs-rebase label Apr 19, 2016

jdm added S-needs-code-changes and removed S-awaiting-review labels Apr 19, 2016

highfive added S-needs-rebase S-awaiting-review and removed S-needs-code-changes labels Apr 20, 2016

rebstar6 force-pushed the rebstar6:referrerPolicy branch 3 times, most recently from 6c9f53c to b361838 Apr 21, 2016

KiChjang added S-needs-tests and removed S-needs-rebase labels Apr 21, 2016

jdm added S-needs-code-changes and removed S-awaiting-review S-needs-tests labels Apr 21, 2016

highfive added the S-awaiting-review label Apr 24, 2016

rebstar6 force-pushed the rebstar6:referrerPolicy branch from 9a55533 to 50a921d Apr 25, 2016

jdm removed the S-needs-rebase label Apr 25, 2016

jdm added S-needs-code-changes and removed S-awaiting-review labels Apr 25, 2016

highfive added S-awaiting-review and removed S-needs-code-changes labels Apr 25, 2016

jdm added S-needs-squash and removed S-awaiting-review labels Apr 25, 2016

Add referrer policy pass-through and referrer header logic

Loading status checks…

526525b

add pass-through from doc to http-loader for referrer_policy, ref_URL add logic for setting referer header add script pass-through for referrer add unit tests for setting referer header

rebstar6 force-pushed the rebstar6:referrerPolicy branch from f478e8d to 526525b Apr 25, 2016

highfive added the S-awaiting-review label Apr 25, 2016

highfive removed S-awaiting-review S-needs-squash labels Apr 25, 2016

highfive added the S-awaiting-merge label Apr 25, 2016

highfive removed the S-awaiting-merge label Apr 25, 2016

This was referenced Apr 25, 2016

Communicate a backtrace to the constellation when panicking. #10824

Merged

Add webdriver commands for (Get|Add)Cookie #10826

Merged

CSSRule, CSSRuleList, CSSStyleSheet partial implementation #10765

Closed

Private browsing student project #10690

Closed

servo / servo

Referer header #10696

Referer header #10696

rebstar6 commented Apr 19, 2016

highfive commented Apr 19, 2016

rebstar6 commented Apr 19, 2016

jdm commented Apr 19, 2016

bors-servo commented Apr 19, 2016

rebstar6 commented Apr 19, 2016

jdm commented Apr 19, 2016

bors-servo commented Apr 20, 2016

rebstar6 commented Apr 21, 2016

rebstar6 commented Apr 21, 2016 •

edited

rebstar6 commented Apr 25, 2016

jdm commented Apr 25, 2016

rebstar6 commented Apr 25, 2016

jdm commented Apr 25, 2016

jdm commented Apr 25, 2016

bors-servo commented Apr 25, 2016

bors-servo commented Apr 25, 2016

bors-servo commented Apr 25, 2016

servo / servo

Join GitHub today

Referer header #10696

Referer header #10696

Conversation

rebstar6 commented Apr 19, 2016

highfive commented Apr 19, 2016

rebstar6 commented Apr 19, 2016

jdm commented Apr 19, 2016

bors-servo commented Apr 19, 2016

rebstar6 commented Apr 19, 2016

jdm commented Apr 19, 2016

bors-servo commented Apr 20, 2016

rebstar6 commented Apr 21, 2016

rebstar6 commented Apr 21, 2016 • edited

rebstar6 commented Apr 25, 2016

jdm commented Apr 25, 2016

rebstar6 commented Apr 25, 2016

jdm commented Apr 25, 2016

jdm commented Apr 25, 2016

bors-servo commented Apr 25, 2016

bors-servo commented Apr 25, 2016

bors-servo commented Apr 25, 2016

rebstar6 commented Apr 21, 2016 •

edited