-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Use module referrer policy for descendant fetches #9210
Conversation
I think I like this approach. If we go this route we should probably add a note in the script fetch options definition about how it mutates over time. However I have a question about it which is more of a design discussion, so let me head over to w3c/webappsec-referrer-policy#111. Also, #3744 is related, but I guess separate. |
I'll go ahead and mark this as ready for review, assuming no major direction changes emerges from the further design discussion. I don't think per that thread, we have browser signals or tests yet, but I suppose the spec change itself can be reviewed anyways. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Please restore the PR template so we can track when this is ready to merge.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(I'm reviewing this PR because I changed many of the algorithms for module loading)
While reading the updated spec, I initially couldn't figure out how it works with a module a.js that imports b.js and c.js, because I had the impression that b.js and c.js would share the same script fetch options struct: they would both use the descendant script fetch options of a.js' script fetch options (HostLoadImportedModule step 6.3).
It took me a while to realize that "the descendant script fetch options" is a new struct every time, so mutating it is safe. Maybe for clarity you could change the step linked above to
Set fetchOptions to
thea new descendant script fetch options for referencingScript's fetch options.
or add a note making it clear that even if we use "the" it's not unique but it's a new one every time.
Thanks. Since descendant options are more of an "algorithm" than a new type, I went with a compromise and wrote "the new descendant script fetch options". Hopefully that's clear enough. |
@annevk What do you think about this from a WebKit perspective? I'm fine with this change in Blink. |
What is the proposed commit message? I have two thoughts here:
|
Per 1b90cd1 this PR no longer privileges top-level modules. I just forgot to update the title. |
OK I've adjusted the PR to hoist the R-P parsing and assignment logic outside of the "If JavaScript" block. It will only kick in for other modules of we ever decide to use script fetch options for them, of course. Proposed commit message:(for once we get a WebKit signal and tests written)
|
Just to be clear, if you have A -> B -> C. And A -> D. And A and B have a referrer policy (different from each other), B and D will be fetched with A's policy and C with B's? Regardless of the point-in-time A -> D is fetched? Assuming that's correct we'd be okay matching that I think (within our own overarching referrer policies of course). Tests would be great. |
Yes.
Can you clarify this? |
@domfarolino I meant that browser policies around referrer ultimately win. |
Ah you mean ones that may override the standard right, in the user's interest right? |
I've started implementing this in Chromium, and our bots fully passing tells me there are indeed no existing tests that we can simply update the expectations of. I'm happy to see the Chromium implementation through on this as well as #9407 but I'm wondering if someone might be willing to volunteer to write some WPTs for this? Let me go out on a limb and kindly see if I can recruit @nicolo-ribaudo, as he's already driving another PR & tests in an area related to this :) |
As for mutability of script fetch options: tl;dr I'm mostly neutral (very slightly preferring the non-mutable way though), with random thoughts below. One one hand, I like the non-mutability of script fetch options because:
On the other hand, I also agree with #9210 (comment), i.e. non-mutable version might be not super better (e.g. we need to add a new parameter to descendant script fetch options and a new member to concept-script which might complicate the spec than needed), so I feel the current PR and Chromium impl CL are also OK. (But this might be because I'm not really strictly careful about referrer policy -- if I should be really careful e.g. like nonce and CSP, I'd take the non-mutable path, distinguishing "script's fetch options's referrer policy" used for the script itself and "script's response referrer policy" to be used for descendants -- I'm not sure it's worth distinguishing these two with extra care) |
Not sure how much additional work it would be, but it seems worthwhile to be careful and match what we do for other fields. |
By "match what we do for other fields" do mean just be immutable @annevk? IMO the big downside to making script fetch options immutable is in the OP:
It seems a confusing to have a script referrer policy member and a script fetch options referrer policy member. If the top-level script is sent down with an R-P header and we update script's referrer policy accordingly, what do we do with its fetch options's referrer policy member? I guess we just have to know to ignore it all the time when creating descendant fetch options? That member effectively becomes dead-weight I guess, which seems weird? I think that's more confusing than the mutability. |
I think the confusion anyway exists: in the current PR, it's just script fetch options referrer policy after updated vs. script fetch options referrer policy before updated rather than separate members. But I anyway don't want to block this due to this issue (just weak preference), so probably we can just proceed with the current PR, if others don't have strong objections. |
This CL implements whatwg/html#9210, which allows for the `Referer-Policy` header delivered on module scripts to be used in the request for their descendant scripts. This CL accomplishes this by: - Marking `ScriptFetchOptions::referrer_policy_` as mutable (to avoid having to carry it around as a non-const everywhere it is currently const). This might feel a little weird but there seems to be ample precedent in `ModuleScriptCreationParams` - Adding a new `response_referrer_policy` member to `ModuleScriptCreationParams` - Use `ModuleScriptCreationParams::response_referrer_policy_` to influence `ScriptFetchOptions::referrer_policy_` in `ModuleScriptLoader::NotifyFetchFinishedSuccess`, corresponding to the spec PR changes in #fetch-a-single-module-script There are four entry points into `ModuleScriptLoader::NotifyFetchFinishedSuccess`: 1. DocumentModuleScriptFetcher::NotifyFinished 2. InstalledServiceWorkerModuleScriptFetcher::Fetch 3. ModuleScriptFetcher::Client::OnFetched 4. WorkerModuleScriptFetcher::NotifyClient Each one of these (though, a bit further up the pipeline for (4)) is modified to parse the `Referer-Policy` header from the resource and pass it into the `ModuleScriptCreationparams`. [1]: https://whatpr.org/html/9210/9bf6568...fffb2ba/webappapis.html#fetch-a-single-module-script Bug: 1462366 Change-Id: Ib7e21e0fb315ef3823fdcf98ec07dce1fb5261c9
This CL implements whatwg/html#9210, which allows for the `Referer-Policy` header delivered on module scripts to be used in the request for their descendant scripts. This CL accomplishes this by: - Marking `ScriptFetchOptions::referrer_policy_` as mutable (to avoid having to carry it around as a non-const everywhere it is currently const). This might feel a little weird but there seems to be ample precedent in `ModuleScriptCreationParams` - Adding a new `response_referrer_policy` member to `ModuleScriptCreationParams` - Use `ModuleScriptCreationParams::response_referrer_policy_` to influence `ScriptFetchOptions::referrer_policy_` in `ModuleScriptLoader::NotifyFetchFinishedSuccess`, corresponding to the spec PR changes in #fetch-a-single-module-script There are four entry points into `ModuleScriptLoader::NotifyFetchFinishedSuccess`: 1. DocumentModuleScriptFetcher::NotifyFinished 2. InstalledServiceWorkerModuleScriptFetcher::Fetch 3. ModuleScriptFetcher::Client::OnFetched 4. WorkerModuleScriptFetcher::NotifyClient Each one of these (though, a bit further up the pipeline for (4)) is modified to parse the `Referer-Policy` header from the resource and pass it into the `ModuleScriptCreationparams`. [1]: https://whatpr.org/html/9210/9bf6568...fffb2ba/webappapis.html#fetch-a-single-module-script Bug: 1462366 Change-Id: Ib7e21e0fb315ef3823fdcf98ec07dce1fb5261c9
Removing |
This CL implements whatwg/html#9210, which allows for the `Referer-Policy` header delivered on module scripts to be used in the request for their descendant scripts. This CL accomplishes this by: - Marking `ScriptFetchOptions::referrer_policy_` as mutable (to avoid having to carry it around as a non-const everywhere it is currently const). This might feel a little weird but there seems to be ample precedent in `ModuleScriptCreationParams` - Adding a new `response_referrer_policy` member to `ModuleScriptCreationParams` - Use `ModuleScriptCreationParams::response_referrer_policy_` to influence `ScriptFetchOptions::referrer_policy_` in `ModuleScriptLoader::NotifyFetchFinishedSuccess`, corresponding to the spec PR changes in #fetch-a-single-module-script There are four entry points into `ModuleScriptLoader::NotifyFetchFinishedSuccess`: 1. DocumentModuleScriptFetcher::NotifyFinished 2. InstalledServiceWorkerModuleScriptFetcher::Fetch 3. ModuleScriptFetcher::Client::OnFetched 4. WorkerModuleScriptFetcher::NotifyClient Each one of these (though, a bit further up the pipeline for (4)) is modified to parse the `Referer-Policy` header from the resource and pass it into the `ModuleScriptCreationparams`. [1]: https://whatpr.org/html/9210/9bf6568...fffb2ba/webappapis.html#fetch-a-single-module-script Bug: 1462366 Change-Id: Ib7e21e0fb315ef3823fdcf98ec07dce1fb5261c9 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4636712 Reviewed-by: Hiroshige Hayashizaki <hiroshige@chromium.org> Commit-Queue: Dominic Farolino <dom@chromium.org> Cr-Commit-Position: refs/heads/main@{#1166154}
This CL implements whatwg/html#9210, which allows for the `Referer-Policy` header delivered on module scripts to be used in the request for their descendant scripts. This CL accomplishes this by: - Marking `ScriptFetchOptions::referrer_policy_` as mutable (to avoid having to carry it around as a non-const everywhere it is currently const). This might feel a little weird but there seems to be ample precedent in `ModuleScriptCreationParams` - Adding a new `response_referrer_policy` member to `ModuleScriptCreationParams` - Use `ModuleScriptCreationParams::response_referrer_policy_` to influence `ScriptFetchOptions::referrer_policy_` in `ModuleScriptLoader::NotifyFetchFinishedSuccess`, corresponding to the spec PR changes in #fetch-a-single-module-script There are four entry points into `ModuleScriptLoader::NotifyFetchFinishedSuccess`: 1. DocumentModuleScriptFetcher::NotifyFinished 2. InstalledServiceWorkerModuleScriptFetcher::Fetch 3. ModuleScriptFetcher::Client::OnFetched 4. WorkerModuleScriptFetcher::NotifyClient Each one of these (though, a bit further up the pipeline for (4)) is modified to parse the `Referer-Policy` header from the resource and pass it into the `ModuleScriptCreationparams`. [1]: https://whatpr.org/html/9210/9bf6568...fffb2ba/webappapis.html#fetch-a-single-module-script Bug: 1462366 Change-Id: Ib7e21e0fb315ef3823fdcf98ec07dce1fb5261c9 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4636712 Reviewed-by: Hiroshige Hayashizaki <hiroshige@chromium.org> Commit-Queue: Dominic Farolino <dom@chromium.org> Cr-Commit-Position: refs/heads/main@{#1166154}
This CL implements whatwg/html#9210, which allows for the `Referer-Policy` header delivered on module scripts to be used in the request for their descendant scripts. This CL accomplishes this by: - Marking `ScriptFetchOptions::referrer_policy_` as mutable (to avoid having to carry it around as a non-const everywhere it is currently const). This might feel a little weird but there seems to be ample precedent in `ModuleScriptCreationParams` - Adding a new `response_referrer_policy` member to `ModuleScriptCreationParams` - Use `ModuleScriptCreationParams::response_referrer_policy_` to influence `ScriptFetchOptions::referrer_policy_` in `ModuleScriptLoader::NotifyFetchFinishedSuccess`, corresponding to the spec PR changes in #fetch-a-single-module-script There are four entry points into `ModuleScriptLoader::NotifyFetchFinishedSuccess`: 1. DocumentModuleScriptFetcher::NotifyFinished 2. InstalledServiceWorkerModuleScriptFetcher::Fetch 3. ModuleScriptFetcher::Client::OnFetched 4. WorkerModuleScriptFetcher::NotifyClient Each one of these (though, a bit further up the pipeline for (4)) is modified to parse the `Referer-Policy` header from the resource and pass it into the `ModuleScriptCreationparams`. [1]: https://whatpr.org/html/9210/9bf6568...fffb2ba/webappapis.html#fetch-a-single-module-script Bug: 1462366 Change-Id: Ib7e21e0fb315ef3823fdcf98ec07dce1fb5261c9 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4636712 Reviewed-by: Hiroshige Hayashizaki <hiroshige@chromium.org> Commit-Queue: Dominic Farolino <dom@chromium.org> Cr-Commit-Position: refs/heads/main@{#1166154}
…icy for descendant fetches, a=testonly Automatic update from web-platform-tests Script: Use module response referrer policy for descendant fetches This CL implements whatwg/html#9210, which allows for the `Referer-Policy` header delivered on module scripts to be used in the request for their descendant scripts. This CL accomplishes this by: - Marking `ScriptFetchOptions::referrer_policy_` as mutable (to avoid having to carry it around as a non-const everywhere it is currently const). This might feel a little weird but there seems to be ample precedent in `ModuleScriptCreationParams` - Adding a new `response_referrer_policy` member to `ModuleScriptCreationParams` - Use `ModuleScriptCreationParams::response_referrer_policy_` to influence `ScriptFetchOptions::referrer_policy_` in `ModuleScriptLoader::NotifyFetchFinishedSuccess`, corresponding to the spec PR changes in #fetch-a-single-module-script There are four entry points into `ModuleScriptLoader::NotifyFetchFinishedSuccess`: 1. DocumentModuleScriptFetcher::NotifyFinished 2. InstalledServiceWorkerModuleScriptFetcher::Fetch 3. ModuleScriptFetcher::Client::OnFetched 4. WorkerModuleScriptFetcher::NotifyClient Each one of these (though, a bit further up the pipeline for (4)) is modified to parse the `Referer-Policy` header from the resource and pass it into the `ModuleScriptCreationparams`. [1]: https://whatpr.org/html/9210/9bf6568...fffb2ba/webappapis.html#fetch-a-single-module-script Bug: 1462366 Change-Id: Ib7e21e0fb315ef3823fdcf98ec07dce1fb5261c9 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4636712 Reviewed-by: Hiroshige Hayashizaki <hiroshige@chromium.org> Commit-Queue: Dominic Farolino <dom@chromium.org> Cr-Commit-Position: refs/heads/main@{#1166154} -- wpt-commits: 3030c6986a70d861e6b25c8ce1b7ee371aab226b wpt-pr: 40889
…icy for descendant fetches, a=testonly Automatic update from web-platform-tests Script: Use module response referrer policy for descendant fetches This CL implements whatwg/html#9210, which allows for the `Referer-Policy` header delivered on module scripts to be used in the request for their descendant scripts. This CL accomplishes this by: - Marking `ScriptFetchOptions::referrer_policy_` as mutable (to avoid having to carry it around as a non-const everywhere it is currently const). This might feel a little weird but there seems to be ample precedent in `ModuleScriptCreationParams` - Adding a new `response_referrer_policy` member to `ModuleScriptCreationParams` - Use `ModuleScriptCreationParams::response_referrer_policy_` to influence `ScriptFetchOptions::referrer_policy_` in `ModuleScriptLoader::NotifyFetchFinishedSuccess`, corresponding to the spec PR changes in #fetch-a-single-module-script There are four entry points into `ModuleScriptLoader::NotifyFetchFinishedSuccess`: 1. DocumentModuleScriptFetcher::NotifyFinished 2. InstalledServiceWorkerModuleScriptFetcher::Fetch 3. ModuleScriptFetcher::Client::OnFetched 4. WorkerModuleScriptFetcher::NotifyClient Each one of these (though, a bit further up the pipeline for (4)) is modified to parse the `Referer-Policy` header from the resource and pass it into the `ModuleScriptCreationparams`. [1]: https://whatpr.org/html/9210/9bf6568...fffb2ba/webappapis.html#fetch-a-single-module-script Bug: 1462366 Change-Id: Ib7e21e0fb315ef3823fdcf98ec07dce1fb5261c9 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4636712 Reviewed-by: Hiroshige Hayashizaki <hiroshigechromium.org> Commit-Queue: Dominic Farolino <domchromium.org> Cr-Commit-Position: refs/heads/main{#1166154} -- wpt-commits: 3030c6986a70d861e6b25c8ce1b7ee371aab226b wpt-pr: 40889 UltraBlame original commit: 06a59b38e048560a04d97da9f96d8afdd151a35b
…icy for descendant fetches, a=testonly Automatic update from web-platform-tests Script: Use module response referrer policy for descendant fetches This CL implements whatwg/html#9210, which allows for the `Referer-Policy` header delivered on module scripts to be used in the request for their descendant scripts. This CL accomplishes this by: - Marking `ScriptFetchOptions::referrer_policy_` as mutable (to avoid having to carry it around as a non-const everywhere it is currently const). This might feel a little weird but there seems to be ample precedent in `ModuleScriptCreationParams` - Adding a new `response_referrer_policy` member to `ModuleScriptCreationParams` - Use `ModuleScriptCreationParams::response_referrer_policy_` to influence `ScriptFetchOptions::referrer_policy_` in `ModuleScriptLoader::NotifyFetchFinishedSuccess`, corresponding to the spec PR changes in #fetch-a-single-module-script There are four entry points into `ModuleScriptLoader::NotifyFetchFinishedSuccess`: 1. DocumentModuleScriptFetcher::NotifyFinished 2. InstalledServiceWorkerModuleScriptFetcher::Fetch 3. ModuleScriptFetcher::Client::OnFetched 4. WorkerModuleScriptFetcher::NotifyClient Each one of these (though, a bit further up the pipeline for (4)) is modified to parse the `Referer-Policy` header from the resource and pass it into the `ModuleScriptCreationparams`. [1]: https://whatpr.org/html/9210/9bf6568...fffb2ba/webappapis.html#fetch-a-single-module-script Bug: 1462366 Change-Id: Ib7e21e0fb315ef3823fdcf98ec07dce1fb5261c9 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4636712 Reviewed-by: Hiroshige Hayashizaki <hiroshigechromium.org> Commit-Queue: Dominic Farolino <domchromium.org> Cr-Commit-Position: refs/heads/main{#1166154} -- wpt-commits: 3030c6986a70d861e6b25c8ce1b7ee371aab226b wpt-pr: 40889 UltraBlame original commit: 06a59b38e048560a04d97da9f96d8afdd151a35b
…icy for descendant fetches, a=testonly Automatic update from web-platform-tests Script: Use module response referrer policy for descendant fetches This CL implements whatwg/html#9210, which allows for the `Referer-Policy` header delivered on module scripts to be used in the request for their descendant scripts. This CL accomplishes this by: - Marking `ScriptFetchOptions::referrer_policy_` as mutable (to avoid having to carry it around as a non-const everywhere it is currently const). This might feel a little weird but there seems to be ample precedent in `ModuleScriptCreationParams` - Adding a new `response_referrer_policy` member to `ModuleScriptCreationParams` - Use `ModuleScriptCreationParams::response_referrer_policy_` to influence `ScriptFetchOptions::referrer_policy_` in `ModuleScriptLoader::NotifyFetchFinishedSuccess`, corresponding to the spec PR changes in #fetch-a-single-module-script There are four entry points into `ModuleScriptLoader::NotifyFetchFinishedSuccess`: 1. DocumentModuleScriptFetcher::NotifyFinished 2. InstalledServiceWorkerModuleScriptFetcher::Fetch 3. ModuleScriptFetcher::Client::OnFetched 4. WorkerModuleScriptFetcher::NotifyClient Each one of these (though, a bit further up the pipeline for (4)) is modified to parse the `Referer-Policy` header from the resource and pass it into the `ModuleScriptCreationparams`. [1]: https://whatpr.org/html/9210/9bf6568...fffb2ba/webappapis.html#fetch-a-single-module-script Bug: 1462366 Change-Id: Ib7e21e0fb315ef3823fdcf98ec07dce1fb5261c9 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4636712 Reviewed-by: Hiroshige Hayashizaki <hiroshigechromium.org> Commit-Queue: Dominic Farolino <domchromium.org> Cr-Commit-Position: refs/heads/main{#1166154} -- wpt-commits: 3030c6986a70d861e6b25c8ce1b7ee371aab226b wpt-pr: 40889 UltraBlame original commit: 06a59b38e048560a04d97da9f96d8afdd151a35b
…icy for descendant fetches, a=testonly Automatic update from web-platform-tests Script: Use module response referrer policy for descendant fetches This CL implements whatwg/html#9210, which allows for the `Referer-Policy` header delivered on module scripts to be used in the request for their descendant scripts. This CL accomplishes this by: - Marking `ScriptFetchOptions::referrer_policy_` as mutable (to avoid having to carry it around as a non-const everywhere it is currently const). This might feel a little weird but there seems to be ample precedent in `ModuleScriptCreationParams` - Adding a new `response_referrer_policy` member to `ModuleScriptCreationParams` - Use `ModuleScriptCreationParams::response_referrer_policy_` to influence `ScriptFetchOptions::referrer_policy_` in `ModuleScriptLoader::NotifyFetchFinishedSuccess`, corresponding to the spec PR changes in #fetch-a-single-module-script There are four entry points into `ModuleScriptLoader::NotifyFetchFinishedSuccess`: 1. DocumentModuleScriptFetcher::NotifyFinished 2. InstalledServiceWorkerModuleScriptFetcher::Fetch 3. ModuleScriptFetcher::Client::OnFetched 4. WorkerModuleScriptFetcher::NotifyClient Each one of these (though, a bit further up the pipeline for (4)) is modified to parse the `Referer-Policy` header from the resource and pass it into the `ModuleScriptCreationparams`. [1]: https://whatpr.org/html/9210/9bf6568...fffb2ba/webappapis.html#fetch-a-single-module-script Bug: 1462366 Change-Id: Ib7e21e0fb315ef3823fdcf98ec07dce1fb5261c9 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4636712 Reviewed-by: Hiroshige Hayashizaki <hiroshige@chromium.org> Commit-Queue: Dominic Farolino <dom@chromium.org> Cr-Commit-Position: refs/heads/main@{#1166154} -- wpt-commits: 3030c6986a70d861e6b25c8ce1b7ee371aab226b wpt-pr: 40889
This is meant to address w3c/webappsec-referrer-policy#111.
Approach:
At first I thought I'd introduce a new a member to the script struct, and reference it from https://html.spec.whatwg.org/#hostloadimportedmodule when fetching descendant modules, but upon remembering that a script's script fetch options are stored in the script struct and used for later descendant module fetches, I figured we could just mutate a script's fetch options once the top-level response comes down from the network, to be the associated
Referrer-Policy
included in the script response (and unchanged otherwise).On one hand this seems elegant and minimally-intrusive, but on the other hand I don't know if there are any negative consequences of changing a script's script fetch options part-way through its lifetime. For example, if there is any way the top-level script gets "unloaded" somehow, then when we re-fetch it, we'll be doing so with script fetch options that are mutated solely for the purposes of the descendant modules, but I don't think this case can ever happen. On the other hand if we really we wanted to stay away from mutating a script's script fetch options after the top-level script response comes down, we could add a new member to the script struct called "referrer policy" that holds the top-level response's referrer policy, if any was sent down, and simply override the descendant script fetch options's referrer policy with this member if it is non-null/default/something. I'm not sure that's much better though, since that introduces two referrer policies associated with a single script, and one of which is only necessary/used for top-level script requests anyways.
(See WHATWG Working Mode: Changes for more details.)
/webappapis.html ( diff )