Descendant scripts don't have a valid Referrer |
|||||||||
Issue description
ModuleScriptLoader::Fetch() sets the Referrer in ModuleScriptFetchRequest after creating a FetchParameters instance. This doesn't take effect.
void ModuleScriptLoader::Fetch(const ModuleScriptFetchRequest& module_request,
ModuleGraphLevel level) {
// snip...
// Note: |options| should not be modified after here.
FetchParameters fetch_params(resource_request, options);
// snip...
// Step 5. "... referrer is referrer, ..." [spec text]
if (!module_request.GetReferrer().IsNull()) {
resource_request.SetHTTPReferrer(SecurityPolicy::GenerateReferrer(
module_request.GetReferrerPolicy(), module_request.Url(),
module_request.GetReferrer()));
}
// snip...
}
When Referrer is not set in ModuleScriptLoader::Fetch(), it's set in BaseFetchContext::AddAdditionalRequestHeaders() and this seems to work for top-level modules, but not for descendant modules.
,
Nov 20 2017
,
Nov 21 2017
CL is under review: https://chromium-review.googlesource.com/c/chromium/src/+/780479
,
Nov 22 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/f2b7c8b0ac80747359c999d8862739440acfcaa5 commit f2b7c8b0ac80747359c999d8862739440acfcaa5 Author: Hiroki Nakagawa <nhiroki@chromium.org> Date: Wed Nov 22 10:02:41 2017 ES6 Modules: Fix referrer handling for module scripts Before this CL, ModuleScriptLoader::Fetch() sets the Referrer in ModuleScriptFetchRequest after creating a FetchParameters instance. This doesn't take effect. This CL fixes it by updating the Referrer via FetchParameters::MutableResourceRequest(). Also, this adds WPT tests for the Referrer on module scripts. Bug: 786862 Change-Id: Ib0031407796e1a7e701565094396e5442aa75702 Reviewed-on: https://chromium-review.googlesource.com/780479 Commit-Queue: Hiroki Nakagawa <nhiroki@chromium.org> Reviewed-by: Kouhei Ueno <kouhei@chromium.org> Reviewed-by: Takeshi Yoshino <tyoshino@chromium.org> Cr-Commit-Position: refs/heads/master@{#518591} [add] https://crrev.com/f2b7c8b0ac80747359c999d8862739440acfcaa5/third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/referrer-no-referrer.sub.html [add] https://crrev.com/f2b7c8b0ac80747359c999d8862739440acfcaa5/third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/referrer-origin-when-cross-origin.sub-expected.txt [add] https://crrev.com/f2b7c8b0ac80747359c999d8862739440acfcaa5/third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/referrer-origin-when-cross-origin.sub.html [add] https://crrev.com/f2b7c8b0ac80747359c999d8862739440acfcaa5/third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/referrer-origin.sub.html [add] https://crrev.com/f2b7c8b0ac80747359c999d8862739440acfcaa5/third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/referrer-same-origin.sub-expected.txt [add] https://crrev.com/f2b7c8b0ac80747359c999d8862739440acfcaa5/third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/referrer-same-origin.sub.html [add] https://crrev.com/f2b7c8b0ac80747359c999d8862739440acfcaa5/third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/referrer-unsafe-url.sub.html [add] https://crrev.com/f2b7c8b0ac80747359c999d8862739440acfcaa5/third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/resources/import-referrer-checker.sub.js [add] https://crrev.com/f2b7c8b0ac80747359c999d8862739440acfcaa5/third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/resources/import-referrer-checker.sub.js.headers [add] https://crrev.com/f2b7c8b0ac80747359c999d8862739440acfcaa5/third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/resources/import-remote-origin-referrer-checker.sub.js [add] https://crrev.com/f2b7c8b0ac80747359c999d8862739440acfcaa5/third_party/WebKit/LayoutTests/external/wpt/html/semantics/scripting-1/the-script-element/module/resources/referrer-checker.py [modify] https://crrev.com/f2b7c8b0ac80747359c999d8862739440acfcaa5/third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp
,
Nov 30 2017
A remaining work is to fix a failed test case. It's failing because of a bug on referrer handling with "same-origin" policy. When a document is loaded with the "same-origin" policy and imports a remote-origin module script (A) that also imports a module script (B) in the same remote-origin, the referrer should not be sent to the module scripts. However, in the current implementation, "same-origin" policy is calculated for the module script (B) based on the module script (A), so the referrer is unexpectedly sent to the module script (B).
,
Nov 30 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/c04fad33c933b8db2ab61ea54d37cb2c5ee53535 commit c04fad33c933b8db2ab61ea54d37cb2c5ee53535 Author: Hiroki Nakagawa <nhiroki@chromium.org> Date: Thu Nov 30 03:30:26 2017 Worklet: Set ReferrerPolicy in WorkletGlobalScope This is a follow-up CL for the CL: https://chromium-review.googlesource.com/c/chromium/src/+/737311 WorkletGlobalScope should inherit the owner Document's ReferrerPolicy and the previous CL meant to implement it. But, the CL actually forgot to set WorkletGlobalScope's ReferrerPolicy to the owner Document's ReferrerPolicy. This CL does it. One test case fails because of a bug in referrer handling on ES6 Modules. See issue 786862 for details. Bug: 773921 , 786862 Change-Id: I0eb95bd742c393ce6b39926ae41a9ef0ec3fc526 Reviewed-on: https://chromium-review.googlesource.com/754150 Commit-Queue: Hiroki Nakagawa <nhiroki@chromium.org> Reviewed-by: Takeshi Yoshino <tyoshino@chromium.org> Reviewed-by: Kinuko Yasuda <kinuko@chromium.org> Reviewed-by: Kouhei Ueno <kouhei@chromium.org> Cr-Commit-Position: refs/heads/master@{#520415} [add] https://crrev.com/c04fad33c933b8db2ab61ea54d37cb2c5ee53535/third_party/WebKit/LayoutTests/external/wpt/worklets/animation-worklet-referrer.https-expected.txt [add] https://crrev.com/c04fad33c933b8db2ab61ea54d37cb2c5ee53535/third_party/WebKit/LayoutTests/external/wpt/worklets/paint-worklet-referrer.https-expected.txt [add] https://crrev.com/c04fad33c933b8db2ab61ea54d37cb2c5ee53535/third_party/WebKit/LayoutTests/external/wpt/worklets/resources/import-referrer-checker-worklet-script.sub.js [add] https://crrev.com/c04fad33c933b8db2ab61ea54d37cb2c5ee53535/third_party/WebKit/LayoutTests/external/wpt/worklets/resources/import-referrer-checker-worklet-script.sub.js.headers [add] https://crrev.com/c04fad33c933b8db2ab61ea54d37cb2c5ee53535/third_party/WebKit/LayoutTests/external/wpt/worklets/resources/import-remote-origin-referrer-checker-worklet-script.sub.js [add] https://crrev.com/c04fad33c933b8db2ab61ea54d37cb2c5ee53535/third_party/WebKit/LayoutTests/external/wpt/worklets/resources/import-remote-origin-referrer-checker-worklet-script.sub.js.headers [add] https://crrev.com/c04fad33c933b8db2ab61ea54d37cb2c5ee53535/third_party/WebKit/LayoutTests/external/wpt/worklets/resources/referrer-checker.py [modify] https://crrev.com/c04fad33c933b8db2ab61ea54d37cb2c5ee53535/third_party/WebKit/LayoutTests/external/wpt/worklets/resources/referrer-tests.js [modify] https://crrev.com/c04fad33c933b8db2ab61ea54d37cb2c5ee53535/third_party/WebKit/LayoutTests/external/wpt/worklets/resources/referrer-window.html [delete] https://crrev.com/49f1b6c8563eaeef13628490d5f233d5f4c18094/third_party/WebKit/LayoutTests/external/wpt/worklets/resources/referrer.py [modify] https://crrev.com/c04fad33c933b8db2ab61ea54d37cb2c5ee53535/third_party/WebKit/Source/core/workers/WorkletGlobalScope.cpp
,
Mar 7 2018
,
May 10 2018
,
May 27 2018
The reason we're failing this final test is because the WebAppSec Referrer Policy spec's "determine a request's referrer" [1] algorithm, under the "same-origin" (referrer policy) section, determines whether a request is a "same-origin request" [2] by comparing a request's origin against its URL, whereas the current Chromium implementation [3] actually compares the origin generated by the _referrer_, against the request's URL. This is actually fine most of the time, since we rarely explicitly set the referrer to anything other than document's `OutgoingReferrer()`, whose origin should be the same as request's origin (set in step 1 > substep 2 of https://fetch.spec.whatwg.org/#concept-fetch). In the module-script-descendant-fetching case though, we explicitly set the referrer, which feeds "bad" data into SecurityPolicy::GenerateReferrer. If we wanted to fix this short-term, we can add some special-case logic somewhere near [4] to detect a same-origin referrer policy and maybe do some acrobatics to make sure we're generating the referrer correctly. The larger problem here is that we're performing the referrer generation (SecurityPolicy::GenerateReferrer) out of order compared to other fetch stuff. We should be able to explicitly set a request's concept referrer (not generate though), and then generate a referrer given the request's origin and URL. The issue in this case is that we don't have request's origin at the time we call SecurityPolicy::GenerateReferrer, because request's origin is dependent on the request's client, which is set much later on [5]. I intend to take some of this on in https://docs.google.com/document/d/1s2bSTktwijQVVBqXi7LIP5zuerR-p3ZRusa6VKLYqMY/edit, but would like some initial feedback before moving forward. We should be performing SecurityPolicy::GenerateReferrer later than we should, after we set request's referrer, referrer policy, and client (for the origin). Then SecurityPolicy::GenerateReferrer should maybe take in an entire request so it has access to these members. [1]: https://w3c.github.io/webappsec-referrer-policy/#determine-requests-referrer [2]: https://w3c.github.io/webappsec-referrer-policy/#same-origin-request [3]: https://cs.chromium.org/chromium/src/third_party/blink/renderer/platform/weborigin/security_policy.cc?sq=package:chromium&g=0&l=205 [4]: https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/loader/modulescript/module_script_loader.cc?sq=package:chromium&g=0&l=137 [5]: https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/loader/modulescript/module_script_loader.cc?q=module_script_loader.cc&sq=package:chromium&g=0&l=144
,
May 27 2018
,
Jun 8 2018
Should probably wait for 850813 so we don't keep adding to fragile `Referer`-setting conditionals.
,
Jul 21
|
|||||||||
►
Sign in to add a comment |
|||||||||
Comment 1 by nhiroki@chromium.org
, Nov 20 2017