Set blink::ResourceRequest referrer string and referrer policy separately |
||||
Issue descriptionCurrently we store a blink::ResourceRequest's referrer string as a Referer header, and referrer policy as a separate member. As a result, every time we want to set a ResourceRequest's referrer policy, we have to regenerate the string stored as the Referer header (via SecurityPolicy::GenerateReferrer). Ideally we would be able to set the referrer string, corresponding to request concept referrer [1], and the referrer policy, corresponding to request concept referrer policy [2], independently. Then, in a few "sink" locations, we would actually invoke the "determine request's referrer", as we do in Main Fetch (step 2 > 7) [3]. Actually ideally we'd only have a single location where we'd need to invoke this algorithm (something similar to BaseFetchContext::AddAdditionalRequestHeaders), but it seems Blink's loading stack isn't currently organized in a way such that all requests eventually go through a common place before hitting lower-level things. For example, a ton of requests ultimately go through BaseFetchContext::AddAdditionalRequestHeaders, but some others don't, and therefore a few pieces of referrer-setting are necessarily duplicated. That seems like a separate conversation. This issue is separate from 850813, which is more centered around how we set and retrieve the final referrer value from a blink::ResourceRequest. This issue is more about minimizing the number of entries into SecurityPolicy::GenerateReferrer() (~analogous to "determine a request's referrer"). [1]: https://fetch.spec.whatwg.org/#concept-request-referrer [2]: https://fetch.spec.whatwg.org/#concept-request-referrer-policy [3]: https://fetch.spec.whatwg.org/#main-fetch
,
Jul 19
,
Jul 20
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/7019eb2337024154bd30d70caa46d8006027dbd8 commit 7019eb2337024154bd30d70caa46d8006027dbd8 Author: Dominic Farolino <domfarolino@gmail.com> Date: Fri Jul 20 04:35:46 2018 Set ResourceRequest's referrer and referrer policy independently This aims to tackle the issue of setting a ResourceRequest's referrer and referrer policy (corresponding to Fetch's concept request referrer and concept request referrer policy) independently. There is no observable effect from this CL; it serves only as a refactoring of when we generate a ResourceRequest's Referer header. Before this CL, in order to set a ResourceRequest's referrer policy, the Referer header had to be generated on the spot. After this CL, a ResourceRequest's referrer and referrer policy are stored and able to be set independently. Prior to the ResourceRequest being sent out, we generate the final Referer header (analogous to Main Fetch invoking "determine request's referrer" algorithm). This CL also adds TODOs for later work revolving around storing ResourceRequest's referrer in the form of a separate member, as opposed to a Referer header. Bug: 863769 Change-Id: I269c95a956b347c131115ce2cec34965d1baf090 Reviewed-on: https://chromium-review.googlesource.com/1137928 Commit-Queue: Dominic Farolino <domfarolino@gmail.com> Reviewed-by: Yutaka Hirano <yhirano@chromium.org> Reviewed-by: Kouhei Ueno <kouhei@chromium.org> Reviewed-by: Kinuko Yasuda <kinuko@chromium.org> Cr-Commit-Position: refs/heads/master@{#576780} [modify] https://crrev.com/7019eb2337024154bd30d70caa46d8006027dbd8/content/renderer/loader/web_url_loader_impl.cc [modify] https://crrev.com/7019eb2337024154bd30d70caa46d8006027dbd8/content/renderer/render_frame_proxy.cc [modify] https://crrev.com/7019eb2337024154bd30d70caa46d8006027dbd8/third_party/blink/renderer/core/fetch/fetch_manager.cc [modify] https://crrev.com/7019eb2337024154bd30d70caa46d8006027dbd8/third_party/blink/renderer/core/fetch/fetch_request_data.cc [modify] https://crrev.com/7019eb2337024154bd30d70caa46d8006027dbd8/third_party/blink/renderer/core/fetch/fetch_request_data.h [modify] https://crrev.com/7019eb2337024154bd30d70caa46d8006027dbd8/third_party/blink/renderer/core/fetch/request.cc [modify] https://crrev.com/7019eb2337024154bd30d70caa46d8006027dbd8/third_party/blink/renderer/core/html/html_anchor_element.cc [modify] https://crrev.com/7019eb2337024154bd30d70caa46d8006027dbd8/third_party/blink/renderer/core/html/html_frame_owner_element.cc [modify] https://crrev.com/7019eb2337024154bd30d70caa46d8006027dbd8/third_party/blink/renderer/core/html/imports/link_import.cc [modify] https://crrev.com/7019eb2337024154bd30d70caa46d8006027dbd8/third_party/blink/renderer/core/html/parser/preload_request.cc [modify] https://crrev.com/7019eb2337024154bd30d70caa46d8006027dbd8/third_party/blink/renderer/core/inspector/inspector_network_agent.cc [modify] https://crrev.com/7019eb2337024154bd30d70caa46d8006027dbd8/third_party/blink/renderer/core/loader/base_fetch_context.cc [modify] https://crrev.com/7019eb2337024154bd30d70caa46d8006027dbd8/third_party/blink/renderer/core/loader/document_threadable_loader.cc [modify] https://crrev.com/7019eb2337024154bd30d70caa46d8006027dbd8/third_party/blink/renderer/core/loader/frame_loader.cc [modify] https://crrev.com/7019eb2337024154bd30d70caa46d8006027dbd8/third_party/blink/renderer/core/loader/history_item.cc [modify] https://crrev.com/7019eb2337024154bd30d70caa46d8006027dbd8/third_party/blink/renderer/core/loader/image_loader.cc [modify] https://crrev.com/7019eb2337024154bd30d70caa46d8006027dbd8/third_party/blink/renderer/core/loader/link_loader.cc [modify] https://crrev.com/7019eb2337024154bd30d70caa46d8006027dbd8/third_party/blink/renderer/core/loader/ping_loader.cc [modify] https://crrev.com/7019eb2337024154bd30d70caa46d8006027dbd8/third_party/blink/renderer/core/page/create_window.cc [modify] https://crrev.com/7019eb2337024154bd30d70caa46d8006027dbd8/third_party/blink/renderer/platform/exported/web_url_request.cc [modify] https://crrev.com/7019eb2337024154bd30d70caa46d8006027dbd8/third_party/blink/renderer/platform/loader/fetch/resource_request.cc [modify] https://crrev.com/7019eb2337024154bd30d70caa46d8006027dbd8/third_party/blink/renderer/platform/loader/fetch/resource_request.h [modify] https://crrev.com/7019eb2337024154bd30d70caa46d8006027dbd8/third_party/blink/renderer/platform/loader/fetch/resource_request_test.cc [modify] https://crrev.com/7019eb2337024154bd30d70caa46d8006027dbd8/third_party/blink/renderer/platform/network/http_names.json5 [modify] https://crrev.com/7019eb2337024154bd30d70caa46d8006027dbd8/third_party/blink/renderer/platform/weborigin/referrer.h
,
Jul 20
,
Jul 23
Re-opening this as it well-describes the TODOs added in https://crrev.com/c/1146065 that revolve around setting the referrer string and referrer policy separately for module script fetching.
,
Aug 6
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/19723c0db2c2065f31cb91a75dd50ea4101d95b8 commit 19723c0db2c2065f31cb91a75dd50ea4101d95b8 Author: Dominic Farolino <domfarolino@gmail.com> Date: Mon Aug 06 05:26:19 2018 Set referrer string and referrer policy separately for module script fetching There is no observable result of this change. Before this CL, ModuleScriptFetchRequest stored a blink::Referrer and a blink::ScriptFetchOptions. This effectively stored two blink::ReferrerPolicy members under ModuleScriptFetchRequest. We also generated ModuleScriptFetchRequest's Referrer in two different places in ModuleTreeLinker. Finally ModuleScriptFetchRequest's Referrer member was used in ModuleScriptLoader::FetchInternal to set ResourceRequest's referrer header. After this CL, we store only a referrer string in ModuleScriptFetchRequest, as the spec indicates, and do not generate a blink::Referrer in ModuleTreeLinker. Then in ModuleScriptLoader::FetchInternal, we generate and set ResourceRequest's final referrer. This also leaves a TODO, to stop storing ResourceRequest's referrer as a header value. R=kinuko@chromium.org, kouhei@chromium.org, nhiroki@chromium.org, yhirano@chromium.org Bug: 863769 Change-Id: Id22467f5c93e59b33ca01571addeceb1a505a1af Reviewed-on: https://chromium-review.googlesource.com/1157237 Commit-Queue: Dominic Farolino <domfarolino@gmail.com> Reviewed-by: Kouhei Ueno <kouhei@chromium.org> Reviewed-by: Kinuko Yasuda <kinuko@chromium.org> Reviewed-by: Yutaka Hirano <yhirano@chromium.org> Reviewed-by: Hiroki Nakagawa <nhiroki@chromium.org> Cr-Commit-Position: refs/heads/master@{#580804} [modify] https://crrev.com/19723c0db2c2065f31cb91a75dd50ea4101d95b8/third_party/blink/renderer/core/loader/link_loader.cc [modify] https://crrev.com/19723c0db2c2065f31cb91a75dd50ea4101d95b8/third_party/blink/renderer/core/loader/link_loader_test.cc [modify] https://crrev.com/19723c0db2c2065f31cb91a75dd50ea4101d95b8/third_party/blink/renderer/core/loader/modulescript/module_script_fetch_request.h [modify] https://crrev.com/19723c0db2c2065f31cb91a75dd50ea4101d95b8/third_party/blink/renderer/core/loader/modulescript/module_script_loader.cc [modify] https://crrev.com/19723c0db2c2065f31cb91a75dd50ea4101d95b8/third_party/blink/renderer/core/loader/modulescript/module_tree_linker.cc
,
Aug 10
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/48552ef979a63b9651cbf08c87f437091af1649a commit 48552ef979a63b9651cbf08c87f437091af1649a Author: Dominic Farolino <domfarolino@gmail.com> Date: Fri Aug 10 05:14:42 2018 Use correct referrer policy for module workers and worklets Some changes in http://crrev.com/c/1111743 had made the default referrer policy the only referrer policy used for fetching module workers and worklet scripts, when in fact the spec indicates the referrer policy of the fetch client settings object ("outer settings" in the spec") should be used in the case where a ScriptFetchOptions' referrer policy was not explicitly set. This CL ensures that we fallback to the fetch client settings object's referrer policy in the appropriate cases. It is a follow-up to the WPT fixes made in both: - https://github.com/web-platform-tests/wpt/pull/12321 - https://github.com/web-platform-tests/wpt/pull/12330 Remaining failures are due to, and tracked by https://crbug.com/786862, which revolves around SecurityPolicy::GenerateReferrer not taking into account a request's origin when determining whether a request is same-origin or not. R=kinuko@chromium.org, kouhei@chromium.org, nhiroki@chromium.org, yhirano@chromium.org Bug: 863769 Change-Id: If493b728df5410a332b064265bbea30527ec9e0f Reviewed-on: https://chromium-review.googlesource.com/1164648 Reviewed-by: Kouhei Ueno <kouhei@chromium.org> Commit-Queue: Dominic Farolino <domfarolino@gmail.com> Cr-Commit-Position: refs/heads/master@{#582065} [delete] https://crrev.com/95847dbf41670132bf7139606244b5005d6af5f3/third_party/WebKit/LayoutTests/external/wpt/workers/modules/dedicated-worker-import-referrer-expected.txt [modify] https://crrev.com/48552ef979a63b9651cbf08c87f437091af1649a/third_party/WebKit/LayoutTests/external/wpt/worklets/animation-worklet-referrer.https-expected.txt [modify] https://crrev.com/48552ef979a63b9651cbf08c87f437091af1649a/third_party/WebKit/LayoutTests/external/wpt/worklets/layout-worklet-referrer.https-expected.txt [modify] https://crrev.com/48552ef979a63b9651cbf08c87f437091af1649a/third_party/WebKit/LayoutTests/external/wpt/worklets/paint-worklet-referrer.https-expected.txt [modify] https://crrev.com/48552ef979a63b9651cbf08c87f437091af1649a/third_party/blink/renderer/core/loader/modulescript/module_script_loader.cc [modify] https://crrev.com/48552ef979a63b9651cbf08c87f437091af1649a/third_party/blink/renderer/core/script/script_loader.cc
,
Sep 6
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/bd27fde7c9069fccef4c571838ea25d9a1fa7acb commit bd27fde7c9069fccef4c571838ea25d9a1fa7acb Author: Yutaka Hirano <yhirano@chromium.org> Date: Thu Sep 06 14:36:19 2018 FetchManager should not set "referer" HTTP header Instead it should use ResourceRequest::SetReferrerString and ResourceRequest::SetReferrerPolicy. Bug: 863769 Change-Id: I4de47bae9aa259ace5ffb5bbdd17d6cf423931d7 Reviewed-on: https://chromium-review.googlesource.com/1203474 Commit-Queue: Yutaka Hirano <yhirano@chromium.org> Reviewed-by: Dominic Farolino <domfarolino@gmail.com> Cr-Commit-Position: refs/heads/master@{#589159} [modify] https://crrev.com/bd27fde7c9069fccef4c571838ea25d9a1fa7acb/third_party/blink/renderer/core/fetch/fetch_manager.cc [modify] https://crrev.com/bd27fde7c9069fccef4c571838ea25d9a1fa7acb/third_party/blink/renderer/core/loader/threadable_loader.cc
,
Sep 6
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/fb043b0b6a82e2e7ea325bd8a5b28c4a3fdbe3a9 commit fb043b0b6a82e2e7ea325bd8a5b28c4a3fdbe3a9 Author: Justin Donnelly <jdonnelly@chromium.org> Date: Thu Sep 06 21:55:50 2018 Revert "FetchManager should not set "referer" HTTP header" This reverts commit bd27fde7c9069fccef4c571838ea25d9a1fa7acb. Reason for revert: Suspected of causing check failures across a variety of tests on the Android CFI bot. See https://crbug.com/881538 for more details. Original change's description: > FetchManager should not set "referer" HTTP header > > Instead it should use ResourceRequest::SetReferrerString and > ResourceRequest::SetReferrerPolicy. > > Bug: 863769 > Change-Id: I4de47bae9aa259ace5ffb5bbdd17d6cf423931d7 > Reviewed-on: https://chromium-review.googlesource.com/1203474 > Commit-Queue: Yutaka Hirano <yhirano@chromium.org> > Reviewed-by: Dominic Farolino <domfarolino@gmail.com> > Cr-Commit-Position: refs/heads/master@{#589159} TBR=yhirano@chromium.org,domfarolino@gmail.com Change-Id: Ie5e58f0e1b794acf2e9ca2f7d1ae24f043d9fe8e No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: 863769, 881538 Reviewed-on: https://chromium-review.googlesource.com/1211957 Reviewed-by: Justin Donnelly <jdonnelly@chromium.org> Commit-Queue: Justin Donnelly <jdonnelly@chromium.org> Cr-Commit-Position: refs/heads/master@{#589318} [modify] https://crrev.com/fb043b0b6a82e2e7ea325bd8a5b28c4a3fdbe3a9/third_party/blink/renderer/core/fetch/fetch_manager.cc [modify] https://crrev.com/fb043b0b6a82e2e7ea325bd8a5b28c4a3fdbe3a9/third_party/blink/renderer/core/loader/threadable_loader.cc
,
Sep 7
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/9fb4b416c74095492b09183316e1f315e33604ad commit 9fb4b416c74095492b09183316e1f315e33604ad Author: Yutaka Hirano <yhirano@chromium.org> Date: Fri Sep 07 06:22:03 2018 Reland "FetchManager should not set "referer" HTTP header" This is a reland of bd27fde7c9069fccef4c571838ea25d9a1fa7acb Original change's description: > FetchManager should not set "referer" HTTP header > > Instead it should use ResourceRequest::SetReferrerString and > ResourceRequest::SetReferrerPolicy. > > Bug: 863769 > Change-Id: I4de47bae9aa259ace5ffb5bbdd17d6cf423931d7 > Reviewed-on: https://chromium-review.googlesource.com/1203474 > Commit-Queue: Yutaka Hirano <yhirano@chromium.org> > Reviewed-by: Dominic Farolino <domfarolino@gmail.com> > Cr-Commit-Position: refs/heads/master@{#589159} Bug: 863769 Change-Id: Ie01801ef1ab7adec166832511539b19b707ab220 Tbr: domfarolino@gmail.com Reviewed-on: https://chromium-review.googlesource.com/1212208 Reviewed-by: Yutaka Hirano <yhirano@chromium.org> Commit-Queue: Yutaka Hirano <yhirano@chromium.org> Cr-Commit-Position: refs/heads/master@{#589446} [modify] https://crrev.com/9fb4b416c74095492b09183316e1f315e33604ad/third_party/blink/renderer/core/fetch/fetch_manager.cc [modify] https://crrev.com/9fb4b416c74095492b09183316e1f315e33604ad/third_party/blink/renderer/core/loader/threadable_loader.cc |
||||
►
Sign in to add a comment |
||||
Comment 1 by domfarolino@gmail.com
, Jul 15