Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Re-land: Fix `Referer` for descendant module scripts and worklets #18887

Merged
merged 1 commit into from Sep 11, 2019

Conversation

@chromium-wpt-export-bot
Copy link
Collaborator

chromium-wpt-export-bot commented Sep 6, 2019

This CL addresses a problem with SecurityPolicy::GenerateReferrer
when it comes to checking the same-origin-ness of a request. The
WebAppSec Referrer Policy Standard defines a same-origin request 1 as
one where the request's origin and current URL are same-origin with
each other. This comparison is done in "determine a request's referrer"
algorithm.

The analogous place in our implementation is
SecurityPolicy::GenerateReferrer. Before this CL, GenerateReferrer would
determine a request's same-origin-ness by comparing the origin of the
request's referrer string and the origin of the request's current URL.
Most of the time this was sufficient, as the request's referrer string
is almost always same-origin with the request's origin (initiator
in Blink). With descendant module scripts and worklets however, the
origin of the request's referrer string and request's origin (initiator)
could be different, which breaks the correctness of our GenerateReferrer
method.

This CL introduces a blink::SecurityOrigin parameter to the
GenerateReferrer method, so that correct same-origin comparisons can be
carried out. In all GenerateReferrer call-sites, an appropriate origin
is passed in.

The original CL 2 was reverted because the semantics of
SecurityPolicy::GenerateReferrer were not kept in sync with the similar
logic in net::URLRequestJob::ComputeReferrerForPolicy, which caused a
DumpWithoutCrashing bug seen in https://crbug.com/1000614, and request
cancellations. This reland updates the ComputeReferrerForPolicy logic
to match the corresponding Blink logic, and includes documentation
mentioning that changes to one section should be reflected in the other.
This CL also includes web platform tests for the scenario in the
aforementioned bug, which pass with this CL, as well as net unit tests
for RedirectInfo and URLRequestJob.

Bug: 786862
Change-Id: I1deeaae8191b07856c593ddb2486297344e0b846
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1786260
Commit-Queue: Dominic Farolino <dom@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Benoit L <lizeb@chromium.org>
Reviewed-by: Matt Menke <mmenke@chromium.org>
Reviewed-by: Andrey Kosyakov <caseq@chromium.org>
Reviewed-by: Yutaka Hirano <yhirano@chromium.org>
Reviewed-by: Kouhei Ueno <kouhei@chromium.org>
Reviewed-by: Tarun Bansal <tbansal@chromium.org>
Reviewed-by: Hiroki Nakagawa <nhiroki@chromium.org>
Cr-Commit-Position: refs/heads/master@{#695523}

Copy link
Collaborator

wpt-pr-bot left a comment

Already reviewed downstream.

@chromium-wpt-export-bot chromium-wpt-export-bot force-pushed the chromium-export-cl-1786260 branch 9 times, most recently from 73f6fb3 to 26a8d23 Sep 6, 2019
This CL addresses a problem with SecurityPolicy::GenerateReferrer
when it comes to checking the same-origin-ness of a request. The
WebAppSec Referrer Policy Standard defines a same-origin request [1] as
one where the request's origin and current URL are same-origin with
each other. This comparison is done in "determine a request's referrer"
algorithm.

The analogous place in our implementation is
SecurityPolicy::GenerateReferrer. Before this CL, GenerateReferrer would
determine a request's same-origin-ness by comparing the origin of the
request's referrer string and the origin of the request's current URL.
Most of the time this was sufficient, as the request's referrer string
is almost always same-origin with the request's origin (initiator
in Blink). With descendant module scripts and worklets however, the
origin of the request's referrer string and request's origin (initiator)
could be different, which breaks the correctness of our GenerateReferrer
method.

This CL introduces a blink::SecurityOrigin parameter to the
GenerateReferrer method, so that correct same-origin comparisons can be
carried out. In all GenerateReferrer call-sites, an appropriate origin
is passed in.

The original CL [2] was reverted because the semantics of
SecurityPolicy::GenerateReferrer were not kept in sync with the similar
logic in net::URLRequestJob::ComputeReferrerForPolicy, which caused a
DumpWithoutCrashing bug seen in https://crbug.com/1000614, and request
cancellations. This reland updates the ComputeReferrerForPolicy logic
to match the corresponding Blink logic, and includes documentation
mentioning that changes to one section should be reflected in the other.
This CL also includes web platform tests for the scenario in the
aforementioned bug, which pass with this CL, as well as net unit tests
for RedirectInfo and URLRequestJob.

[1]: https://www.w3.org/TR/referrer-policy/#same-origin-request

[2]: https://crrev.com/c/1768501

Bug: 786862
Change-Id: I1deeaae8191b07856c593ddb2486297344e0b846
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1786260
Commit-Queue: Dominic Farolino <dom@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Benoit L <lizeb@chromium.org>
Reviewed-by: Matt Menke <mmenke@chromium.org>
Reviewed-by: Andrey Kosyakov <caseq@chromium.org>
Reviewed-by: Yutaka Hirano <yhirano@chromium.org>
Reviewed-by: Kouhei Ueno <kouhei@chromium.org>
Reviewed-by: Tarun Bansal <tbansal@chromium.org>
Reviewed-by: Hiroki Nakagawa <nhiroki@chromium.org>
Cr-Commit-Position: refs/heads/master@{#695523}
@chromium-wpt-export-bot chromium-wpt-export-bot force-pushed the chromium-export-cl-1786260 branch from 26a8d23 to eae99dd Sep 11, 2019
@chromium-wpt-export-bot chromium-wpt-export-bot merged commit 266cd16 into master Sep 11, 2019
12 of 14 checks passed
12 of 14 checks passed
update-pr-preview
Details
staging.wpt.fyi - chrome[experimental] Chrome results
Details
wpt.fyi - chrome[experimental] Chrome results
Details
Azure Pipelines Build #20190911.45 succeeded
Details
Azure Pipelines (./wpt test-jobs) ./wpt test-jobs succeeded
Details
Azure Pipelines (affected tests without changes: Safari Technology Preview) affected tests without changes: Safari Technology Preview succeeded
Details
Azure Pipelines (affected tests: Safari Technology Preview) affected tests: Safari Technology Preview succeeded
Details
Azure Pipelines (wpt.fyi hook: safari-preview-affected-tests) wpt.fyi hook: safari-preview-affected-tests succeeded
Details
Azure Pipelines (wpt.fyi hook: safari-preview-affected-tests-without-changes) wpt.fyi hook: safari-preview-affected-tests-without-changes succeeded
Details
Taskcluster (pull_request) TaskGroup: success
Details
staging.wpt.fyi - firefox[experimental] Firefox results
Details
staging.wpt.fyi - safari[experimental] Safari results
Details
wpt.fyi - firefox[experimental] Firefox results
Details
wpt.fyi - safari[experimental] Safari results
Details
@chromium-wpt-export-bot chromium-wpt-export-bot deleted the chromium-export-cl-1786260 branch Sep 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.