Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upUpdate referrer computation #27104
Merged
Update referrer computation #27104
+152
−2,065
Conversation
highfive
commented
Jun 28, 2020
|
Heads up! This PR modifies the following files:
|
|
@bors-servo try=wpt |
bors-servo
added a commit
that referenced
this pull request
Jun 28, 2020
Update referrer computation The PR updates the request's referrer computation in consideration with the recent changes in [w3c/webappsec-referrer-policy#135](w3c/webappsec-referrer-policy#135). --- - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #27030 - [x] There are tests for these changes
|
|
|
Looks great, just one question and some docs that can be added. |
| @@ -242,56 +239,67 @@ fn is_schemelessy_same_site(site_a: &ImmutableOrigin, site_b: &ImmutableOrigin) | |||
| } | |||
|
|
|||
| /// <https://w3c.github.io/webappsec-referrer-policy/#strip-url> | |||
| fn strip_url(mut referrer_url: ServoUrl, origin_only: bool) -> Option<ServoUrl> { | |||
| fn strip_url_for_use_as_referrer(mut url: ServoUrl, origin_only: bool) -> Option<ServoUrl> { | |||
| const MAX_REFERRER_URL_LENGTH: usize = 4096; | |||
This comment has been minimized.
This comment has been minimized.
gterzian
Jun 29, 2020
Member
I'm wondering why url always seems to be non-null, where are we dealing with Step 1?
This comment has been minimized.
This comment has been minimized.
utsavoza
Jun 29, 2020
Author
Contributor
hmm, it doesn't seem clear to me from the spec when a referrerSource can be null
This comment has been minimized.
This comment has been minimized.
gterzian
Jun 30, 2020
•
Member
Ok, thanks for checking, I have filed w3c/webappsec-referrer-policy#139
This comment has been minimized.
This comment has been minimized.
| url.set_username("").unwrap(); | ||
| url.set_password(None).unwrap(); | ||
| url.set_fragment(None); | ||
| if origin_only || url.as_str().len() > MAX_REFERRER_URL_LENGTH { |
This comment has been minimized.
This comment has been minimized.
gterzian
Jun 29, 2020
Member
Let's add a note that the length check is Step 6 of https://w3c.github.io/webappsec-referrer-policy/#determine-requests-referrer.
| @@ -97,6 +97,11 @@ impl ServoUrl { | |||
| scheme == "https" || scheme == "wss" | |||
| } | |||
|
|
|||
| pub fn is_local_scheme(&self) -> bool { | |||
This comment has been minimized.
This comment has been minimized.
gterzian
Jun 29, 2020
Member
Please add the spec link \\\ <https://fetch.spec.whatwg.org/#local-scheme>
components/url/lib.rs
Outdated
| @@ -169,6 +174,20 @@ impl ServoUrl { | |||
| Ok(Self::from_url(Url::from_file_path(path)?)) | |||
| } | |||
|
|
|||
| // https://w3c.github.io/webappsec-secure-contexts/#potentially-trustworthy-url | |||
This comment has been minimized.
This comment has been minimized.
gterzian
Jun 29, 2020
Member
nit: I see this hasn't been done consistently elsewhere in this file, and I do think it's better to document this as \\\ <https://w3c.github.io/webappsec-secure-contexts/#potentially-trustworthy-url>
|
Ok, just wondering about one extra change in the new commit which might require a spec number. Also please squash into one commit. Should be good to go after! |
| @@ -240,6 +240,7 @@ pub fn main_fetch( | |||
| let referrer_url = match mem::replace(&mut request.referrer, Referrer::NoReferrer) { | |||
| Referrer::NoReferrer => None, | |||
| Referrer::ReferrerUrl(referrer_source) | Referrer::Client(referrer_source) => { | |||
| request.headers.remove(header::REFERER); | |||
This comment has been minimized.
This comment has been minimized.
gterzian
Jun 30, 2020
Member
Was this change intended? Could you please add a spec number if possible?
This comment has been minimized.
This comment has been minimized.
utsavoza
Jun 30, 2020
Author
Contributor
Oh, that was an existing step I'd earlier omitted by mistake. The step doesn't correspond to the spec, but I believe it removes Referer headers that were set in past redirects or preflights.
This comment has been minimized.
This comment has been minimized.
gterzian
added
S-needs-code-changes
S-needs-squash
and removed
S-awaiting-review
labels
utsavoza
force-pushed the
utsavoza:ugo/issue-27030/28-06-2020
branch
from
358f4d2
to
64464c6
|
Squashed into one commit |
|
Thanks! Looking at the test expectations update, besides a lot of progress, it also seems to include some new FAIL. Do you know what is causing those? They seem to relate to the "no-refferer" being too aggressive? Is there perhaps a follow-up issue that could be filed for them? It could also be that the tests are outdated. |
I didn't get a chance to take a look at this earlier, but afaict the referrer policy isn't getting propagated properly during servo/components/script/script_thread.rs Lines 3876 to 3877 in 19b36bd Maybe I should update the PR to include the change and see if it fixes the current failing tests. Edit: I have updated the PR with the change, but we'll need to see if it breaks any other tests unexpectedly. |
|
I think this PR should also close #14505 |
Awesome! |
|
Thanks! @bors-servo r+ |
bors-servo
added a commit
that referenced
this pull request
Jul 2, 2020
Update referrer computation The PR updates the request's referrer computation in consideration with the recent changes in [w3c/webappsec-referrer-policy#135](w3c/webappsec-referrer-policy#135). --- - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #27030 - [x] There are tests for these changes
|
|
|
Looks like same failures again. Or maybe we can say this PR makes the test failure non-intermittent now |
|
Based on the panic trace I observed while running the test locally: it looks like url.set_username("").unwrap();
url.set_password(None).unwrap();because the underlying implementation (for Earlier we skipped the algorithm for all urls that didn't have I am not sure about the correct behavior in this case (that aligns with the spec), however returning url.set_username("").ok()?;
url.set_password(None).ok()?;or ignoring the returned result might resolve the test failure. let _ = url.set_username("");
let _ = url.set_password(None); |
I just checked the spec of Strip url for use as a referrer and it says "If url’s scheme is a local scheme, then return no referrer." in step 2. I'm not pretty familiar with referrer policy though, (btw, might be an off-topic question though, why doesn't |
|
@utsavoza Thanks for looking into this. Since the purpose of " Strip url for use as a referrer " is to remove some information from the url, I'd say if the information cannot be present in the first place, for example password/username with file urls, then we should not unwrap on those errors, since the "stripping" is actually unnecessary and the information not present in the first place. And the Url implementation in Rust in such a case "does nothing and returns an error" https://docs.rs/url/2.0.0/url/struct.Url.html#method.set_password So instead of unwrapping, we should just assume success(since there is actually nothing to strip in the first place) and continue with the algorithm I think. That is also what the spec does, for example at https://url.spec.whatwg.org/#dom-url-username Where the setter returns early in case of https://url.spec.whatwg.org/#cannot-have-a-username-password-port
Local scheme is defined at https://fetch.spec.whatwg.org/#local-scheme |
Thanks for pointing to this spec link! Didn't find this one. Does this mean, the step 2 should be "If url’s scheme is a
Sorry for being off-topic to this PR >_< I did find the spec link of local scheme but what I'm not clear is, what does the |
I don't think it means we should always return no referrer in the file case. Instead, we should simply let step 3 and 4 basically do nothing, and continue with the other steps.
I think it just means "a scheme that is "about", "blob", or "data"". |
…_referrer
utsavoza
force-pushed the
utsavoza:ugo/issue-27030/28-06-2020
branch
from
b9adf52
to
1b9e84b
|
@bors-servo try=wpt |
bors-servo
added a commit
that referenced
this pull request
Jul 3, 2020
Update referrer computation The PR updates the request's referrer computation in consideration with the recent changes in [w3c/webappsec-referrer-policy#135](w3c/webappsec-referrer-policy#135). --- - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #27030 and fix #14505 - [x] There are tests for these changes
|
|
|
Thanks, awesome work! |
|
@bors-servo r+ |
|
|
|
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
utsavoza commentedJun 28, 2020
•
edited
The PR updates the request's referrer computation in consideration with the recent changes in w3c/webappsec-referrer-policy#135.
./mach build -ddoes not report any errors./mach test-tidydoes not report any errors