Implement external <script type="speculationrules"> [chromium/src : main]
Viktoria Zlatinova (Gerrit)
Message from Viktoria Zlatinova![]( "Open in Gerrit")
Set Ready For Review
Related details
- Viktoria Zlatinova
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Viktoria Zlatinova (Gerrit)
Viktoria Zlatinova added 1 comment![]( "Open in Gerrit")
PTAL when you have a chance, thanks!
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yoav Weiss (@Shopify) (Gerrit)
Yoav Weiss (@Shopify) added 1 comment![]( "Open in Gerrit")
I think you'd need to start an HTML spec discussion on this feature, ideally backed by use-cases it would solve.
You probably also should put the feature behind a runtime-enabled flag [1], and start the relevant intent process [2].
[1] https://chromium.googlesource.com/chromium/src/+/main/third_party/blink/renderer/platform/RuntimeEnabledFeatures.md
[2] https://www.chromium.org/blink/launching-features/
Related details
- Viktoria Zlatinova
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Viktoria Zlatinova (Gerrit)
Viktoria Zlatinova added 1 comment![]( "Open in Gerrit")
I think you'd need to start an HTML spec discussion on this feature, ideally backed by use-cases it would solve.
You probably also should put the feature behind a runtime-enabled flag [1], and start the relevant intent process [2].
[1] https://chromium.googlesource.com/chromium/src/+/main/third_party/blink/renderer/platform/RuntimeEnabledFeatures.md
[2] https://www.chromium.org/blink/launching-features/
Thanks! Will add a flag and start the intent process.
I created a PR for the HTML spec here: https://github.com/whatwg/html/pull/11697
Some extra discussion here: https://github.com/whatwg/html/issues/11123#issuecomment-3336819191
Original issue in WICG: https://github.com/WICG/nav-speculation/issues/348
I will collect the use cases and continue the discussion in the HTML spec to ensure there is cross-browser support
Related details
- Yoav Weiss (@Shopify)
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Viktoria Zlatinova (Gerrit)
Viktoria Zlatinova added 1 comment![]( "Open in Gerrit")
PTAL when you have a chance, thank you!
Related details
- Hiroshige Hayashizaki
- Takashi Toyoshima
- Yoav Weiss (@Shopify)
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Takashi Toyoshima (Gerrit)
Takashi Toyoshima added 8 comments![]( "Open in Gerrit")
https://chromestatus.com/feature/4868908869877760Any link to the CSP spec's algorithm change?
Also nice to have a direct link to the speculation rules side GitHub issue that discusses this.
// Speculation rules from HTTP headers are exempt from CSP, unlike speculationSorry, I could not remember details on what I changed a few years ago, but it's great if you note how this affect inline speculation rules cases.
enum class OptOutRuleSetType { kInline, kExternalScript, kHttpHeader };`kInlineScript` for consistency
Let's keep this protected and make the caller class a friend class.
element_document.AddConsoleMessage(MakeGarbageCollected<ConsoleMessage>(
mojom::blink::ConsoleMessageSource::kJavaScript,
mojom::blink::ConsoleMessageLevel::kError,
"External speculation rules are not yet supported."));Maybe it's good to keep this in the disabled code path and say 'not yet enabled'?
// Copyright 2024 The Chromium Authors2026
// Copyright 2024 The Chromium Authorsditto
bool IsFromScript() const;Can we have a clear comment to say that this includes both inline and external cases?
Related details
- Hiroshige Hayashizaki
- Viktoria Zlatinova
- Yoav Weiss (@Shopify)
Viktoria Zlatinova (Gerrit)
Viktoria Zlatinova added 7 comments![]( "Open in Gerrit")
// Speculation rules from HTTP headers are exempt from CSP, unlike speculationSorry, I could not remember details on what I changed a few years ago, but it's great if you note how this affect inline speculation rules cases.
Done
enum class OptOutRuleSetType { kInline, kExternalScript, kHttpHeader };Viktoria Zlatinova`kInlineScript` for consistency
Done
Let's keep this protected and make the caller class a friend class.
Done
element_document.AddConsoleMessage(MakeGarbageCollected<ConsoleMessage>(
mojom::blink::ConsoleMessageSource::kJavaScript,
mojom::blink::ConsoleMessageLevel::kError,
"External speculation rules are not yet supported."));Maybe it's good to keep this in the disabled code path and say 'not yet enabled'?
Done
// Copyright 2024 The Chromium AuthorsViktoria Zlatinova2026
Done
// Copyright 2024 The Chromium AuthorsViktoria Zlatinovaditto
Done
Can we have a clear comment to say that this includes both inline and external cases?
- Hiroshige Hayashizaki
- Takashi Toyoshima
- Yoav Weiss (@Shopify)
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Takashi Toyoshima (Gerrit)
Takashi Toyoshima added 3 comments![]( "Open in Gerrit")
This change for the content_security_policy.cc is related to the CSP spec's algorithm and requires Mike's another review to check if we need another spec change.
Any link to the CSP spec's algorithm change?
Also nice to have a direct link to the speculation rules side GitHub issue that discusses this.
thank you!
- CSP spec change: https://github.com/w3c/webappsec-csp/pull/776This seems to be an upstreaming patch that ported my original CSP spec change for the inline script, and wasn't for an external script?
Related details
- Hiroshige Hayashizaki
- Viktoria Zlatinova
- Yoav Weiss (@Shopify)
Viktoria Zlatinova (Gerrit)
Viktoria Zlatinova added 1 comment![]( "Open in Gerrit")
- CSP spec change: https://github.com/w3c/webappsec-csp/pull/776This seems to be an upstreaming patch that ported my original CSP spec change for the inline script, and wasn't for an external script?
I created a branch based off of this active PR to include the external script speculation rules case: https://github.com/domenic/webappsec-csp/pull/1. I'll follow up with folks on GitHub to get that reviewed. Can implementation continue in parallel?
Related details
- Hiroshige Hayashizaki
- Takashi Toyoshima
- Yoav Weiss (@Shopify)
Takashi Toyoshima (Gerrit)
Takashi Toyoshima added 2 comments![]( "Open in Gerrit")
+mkwst for a sign-off around the CSP changes
- CSP spec change: https://github.com/w3c/webappsec-csp/pull/776Viktoria ZlatinovaThis seems to be an upstreaming patch that ported my original CSP spec change for the inline script, and wasn't for an external script?
I created a branch based off of this active PR to include the external script speculation rules case: https://github.com/domenic/webappsec-csp/pull/1. I'll follow up with folks on GitHub to get that reviewed. Can implementation continue in parallel?
So, can you at least add TODO comments against changes in content_security_policy.cc?
Related details
- Hiroshige Hayashizaki
- Mike West
- Viktoria Zlatinova
- Yoav Weiss (@Shopify)
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mike West (Gerrit)
Mike West added 5 comments![]( "Open in Gerrit")
Thanks! I'll defer to loading folks for most of this, but I do have some questions about the CSP integration below:
check_header_type, options.initiator_info.name)) {This parameter's purpose isn't clear to me. Can you help me understand the scenarios in which checking the request's context/destination wouldn't be enough?
bool is_from_external_script);Nit: Here and below, prefer an enum or `base::StrongAlias` (see https://chromium.googlesource.com/chromium/src/+/main/styleguide/c++/blink-c++.md#Prefer-enums-or-StrongAliases-to-bare-bools-for-function-parameters for context).
network::mojom::blink::RequestDestination::kSpeculationRules);Dropping this will change the `Sec-Fetch-Destination` header sent with the request, as well as the request's `destination` which is web-accessible via the `Request` object. Can you help me understand why you're removing the annotation?
</script>It would be ideal to add web platform tests verifying the CSP integration.
Related details
- Hiroshige Hayashizaki
Viktoria Zlatinova (Gerrit)
Viktoria Zlatinova added 6 comments![]( "Open in Gerrit")
Viktoria ZlatinovaI think you'd need to start an HTML spec discussion on this feature, ideally backed by use-cases it would solve.
You probably also should put the feature behind a runtime-enabled flag [1], and start the relevant intent process [2].
[1] https://chromium.googlesource.com/chromium/src/+/main/third_party/blink/renderer/platform/RuntimeEnabledFeatures.md
[2] https://www.chromium.org/blink/launching-features/
Thanks! Will add a flag and start the intent process.
I created a PR for the HTML spec here: https://github.com/whatwg/html/pull/11697
Some extra discussion here: https://github.com/whatwg/html/issues/11123#issuecomment-3336819191Original issue in WICG: https://github.com/WICG/nav-speculation/issues/348
I will collect the use cases and continue the discussion in the HTML spec to ensure there is cross-browser support
Done
+mkwst for a sign-off around the CSP changes
Done
- CSP spec change: https://github.com/w3c/webappsec-csp/pull/776Viktoria ZlatinovaThis seems to be an upstreaming patch that ported my original CSP spec change for the inline script, and wasn't for an external script?
Takashi ToyoshimaI created a branch based off of this active PR to include the external script speculation rules case: https://github.com/domenic/webappsec-csp/pull/1. I'll follow up with folks on GitHub to get that reviewed. Can implementation continue in parallel?
So, can you at least add TODO comments against changes in content_security_policy.cc?
Done
check_header_type, options.initiator_info.name)) {This parameter's purpose isn't clear to me. Can you help me understand the scenarios in which checking the request's context/destination wouldn't be enough?
Currently the context/destination are the same for external script speculation rules and HTTP header speculations rules, but CSP doesn't apply to the HTTP header case. Using the initiator here to continue exempting the header case - https://chromium.googlesource.com/chromium/src/+/1d7cf351315ad93d53b369011414990a8fde7dbd/third_party/blink/renderer/core/frame/csp/content_security_policy.cc#:~:text=//%20Speculation%20rules%20from,%7D
Nit: Here and below, prefer an enum or `base::StrongAlias` (see https://chromium.googlesource.com/chromium/src/+/main/styleguide/c++/blink-c++.md#Prefer-enums-or-StrongAliases-to-bare-bools-for-function-parameters for context).
Done
network::mojom::blink::RequestDestination::kSpeculationRules);Dropping this will change the `Sec-Fetch-Destination` header sent with the request, as well as the request's `destination` which is web-accessible via the `Request` object. Can you help me understand why you're removing the annotation?
I moved this part to SpeculationRulesResource, as it is shared by the HTTP header and external script cases: https://chromium.googlesource.com/chromium/src/+/1d7cf351315ad93d53b369011414990a8fde7dbd/third_party/blink/renderer/core/loader/resource/speculation_rules_resource.cc#:~:text=resource_request.SetRequestContext,%3A%3AkSpeculationRules)%3B
Related details
- Hiroshige Hayashizaki
- Mike West
- Takashi Toyoshima
- Yoav Weiss (@Shopify)
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mike West (Gerrit)
Mike West added 1 comment![]( "Open in Gerrit")
check_header_type, options.initiator_info.name)) {Viktoria ZlatinovaThis parameter's purpose isn't clear to me. Can you help me understand the scenarios in which checking the request's context/destination wouldn't be enough?
Currently the context/destination are the same for external script speculation rules and HTTP header speculations rules, but CSP doesn't apply to the HTTP header case. Using the initiator here to continue exempting the header case - https://chromium.googlesource.com/chromium/src/+/1d7cf351315ad93d53b369011414990a8fde7dbd/third_party/blink/renderer/core/frame/csp/content_security_policy.cc#:~:text=//%20Speculation%20rules%20from,%7D
Ok. I guess my next question is why it's a good idea to distinguish between those scenarios. :) It looks like Antonio asked about that on the CSP PR as well, and didn't get an answer (https://github.com/w3c/webappsec-csp/pull/776#issuecomment-3327621673). Perhaps you could provide one here?
Related details
- Viktoria Zlatinova
- Yoav Weiss (@Shopify)
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hiroshige Hayashizaki (Gerrit)
Hiroshige Hayashizaki added 1 comment![]( "Open in Gerrit")
network::mojom::blink::RequestDestination::kSpeculationRules);Viktoria ZlatinovaDropping this will change the `Sec-Fetch-Destination` header sent with the request, as well as the request's `destination` which is web-accessible via the `Request` object. Can you help me understand why you're removing the annotation?
I moved this part to SpeculationRulesResource, as it is shared by the HTTP header and external script cases: https://chromium.googlesource.com/chromium/src/+/1d7cf351315ad93d53b369011414990a8fde7dbd/third_party/blink/renderer/core/loader/resource/speculation_rules_resource.cc#:~:text=resource_request.SetRequestContext,%3A%3AkSpeculationRules)%3B
Basically setting the `resource_request` should be the responsibility of the callers of `*Resource::Fetch()`, so I feel it's better to place here.
Also the current patchset uses `SpeculationRulesResource` from `SpeculationRulesHeader::StartFetches()` and `ExternalSpeculationRulesClient` separately, but these two paths should be further merged (e.g. Lines 141-146 etc.), to ensure external speculation rules are loaded in the same way by Link header and by <script>.
So, How about further deduplicating the code here and at ScriptLoader and `ExternalSpeculationRulesClient` with `SpeculationRulesHeader::StartFetches()`, `SpeculationRuleLoader`?
e.g. introduce a common helper like
```
LoadExternalSpeculationRules(bunch of params...) {
... Line 133-165 here, plus necessary modification to accept parameters that differs between Link header and <script> ...
... ResourceRequest creation and modification are all done here ...
}
```
and call `LoadExternalSpeculationRules()` here and from ScriptLoader.
Hiroshige Hayashizaki (Gerrit)
Hiroshige Hayashizaki added 1 comment![]( "Open in Gerrit")
check_header_type, options.initiator_info.name)) {Viktoria ZlatinovaThis parameter's purpose isn't clear to me. Can you help me understand the scenarios in which checking the request's context/destination wouldn't be enough?
Mike WestCurrently the context/destination are the same for external script speculation rules and HTTP header speculations rules, but CSP doesn't apply to the HTTP header case. Using the initiator here to continue exempting the header case - https://chromium.googlesource.com/chromium/src/+/1d7cf351315ad93d53b369011414990a8fde7dbd/third_party/blink/renderer/core/frame/csp/content_security_policy.cc#:~:text=//%20Speculation%20rules%20from,%7D
Ok. I guess my next question is why it's a good idea to distinguish between those scenarios. :) It looks like Antonio asked about that on the CSP PR as well, and didn't get an answer (https://github.com/w3c/webappsec-csp/pull/776#issuecomment-3327621673). Perhaps you could provide one here?
As for implementation, if we'd set `mojom::blink::RequestContextType::SCRIPT` for external spec rules from <script>, then the issue is resolved without modification to csp stack?
+1 to Mike about the spec -- I'll also ask around.
Viktoria Zlatinova (Gerrit)
Viktoria Zlatinova voted and added 3 comments![]( "Open in Gerrit")
Votes added by Viktoria Zlatinova
| Commit-Queue | +1 |
3 comments
check_header_type, options.initiator_info.name)) {Viktoria ZlatinovaThis parameter's purpose isn't clear to me. Can you help me understand the scenarios in which checking the request's context/destination wouldn't be enough?
Mike WestCurrently the context/destination are the same for external script speculation rules and HTTP header speculations rules, but CSP doesn't apply to the HTTP header case. Using the initiator here to continue exempting the header case - https://chromium.googlesource.com/chromium/src/+/1d7cf351315ad93d53b369011414990a8fde7dbd/third_party/blink/renderer/core/frame/csp/content_security_policy.cc#:~:text=//%20Speculation%20rules%20from,%7D
Hiroshige HayashizakiOk. I guess my next question is why it's a good idea to distinguish between those scenarios. :) It looks like Antonio asked about that on the CSP PR as well, and didn't get an answer (https://github.com/w3c/webappsec-csp/pull/776#issuecomment-3327621673). Perhaps you could provide one here?
As for implementation, if we'd set `mojom::blink::RequestContextType::SCRIPT` for external spec rules from <script>, then the issue is resolved without modification to csp stack?
+1 to Mike about the spec -- I'll also ask around.
Yes, using `mojom::blink::RequestContextType::SCRIPT` resolves the issue without modification to the CSP stack. Updated to this, let me know what you think!
I can update the CSP/Fetch spec PRs separately to add some reasoning as to why the header case is exempt (with further discussion if needed)
network::mojom::blink::RequestDestination::kSpeculationRules);Viktoria ZlatinovaDropping this will change the `Sec-Fetch-Destination` header sent with the request, as well as the request's `destination` which is web-accessible via the `Request` object. Can you help me understand why you're removing the annotation?
Hiroshige HayashizakiI moved this part to SpeculationRulesResource, as it is shared by the HTTP header and external script cases: https://chromium.googlesource.com/chromium/src/+/1d7cf351315ad93d53b369011414990a8fde7dbd/third_party/blink/renderer/core/loader/resource/speculation_rules_resource.cc#:~:text=resource_request.SetRequestContext,%3A%3AkSpeculationRules)%3B
Basically setting the `resource_request` should be the responsibility of the callers of `*Resource::Fetch()`, so I feel it's better to place here.
Also the current patchset uses `SpeculationRulesResource` from `SpeculationRulesHeader::StartFetches()` and `ExternalSpeculationRulesClient` separately, but these two paths should be further merged (e.g. Lines 141-146 etc.), to ensure external speculation rules are loaded in the same way by Link header and by <script>.
So, How about further deduplicating the code here and at ScriptLoader and `ExternalSpeculationRulesClient` with `SpeculationRulesHeader::StartFetches()`, `SpeculationRuleLoader`?
e.g. introduce a common helper like
```
LoadExternalSpeculationRules(bunch of params...) {
... Line 133-165 here, plus necessary modification to accept parameters that differs between Link header and <script> ...
... ResourceRequest creation and modification are all done here ...
}
```and call `LoadExternalSpeculationRules()` here and from ScriptLoader.
Further deduped this code in `SpeculationRuleLoader`. Some of the response handling is also different between the header and script case so I kept those differences - for example, raising DOM events and adding the rules to script loader for script case, and keeping the console messages for header case.
It would be ideal to add web platform tests verifying the CSP integration.
- Hiroshige Hayashizaki
- Mike West
- Yoav Weiss (@Shopify)
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mike West (Gerrit)
Mike West added 1 comment![]( "Open in Gerrit")
check_header_type, options.initiator_info.name)) {Viktoria ZlatinovaThis parameter's purpose isn't clear to me. Can you help me understand the scenarios in which checking the request's context/destination wouldn't be enough?
Mike WestCurrently the context/destination are the same for external script speculation rules and HTTP header speculations rules, but CSP doesn't apply to the HTTP header case. Using the initiator here to continue exempting the header case - https://chromium.googlesource.com/chromium/src/+/1d7cf351315ad93d53b369011414990a8fde7dbd/third_party/blink/renderer/core/frame/csp/content_security_policy.cc#:~:text=//%20Speculation%20rules%20from,%7D
Hiroshige HayashizakiOk. I guess my next question is why it's a good idea to distinguish between those scenarios. :) It looks like Antonio asked about that on the CSP PR as well, and didn't get an answer (https://github.com/w3c/webappsec-csp/pull/776#issuecomment-3327621673). Perhaps you could provide one here?
Viktoria ZlatinovaAs for implementation, if we'd set `mojom::blink::RequestContextType::SCRIPT` for external spec rules from <script>, then the issue is resolved without modification to csp stack?
+1 to Mike about the spec -- I'll also ask around.
Yes, using `mojom::blink::RequestContextType::SCRIPT` resolves the issue without modification to the CSP stack. Updated to this, let me know what you think!
I can update the CSP/Fetch spec PRs separately to add some reasoning as to why the header case is exempt (with further discussion if needed)
Please do come back to the CSP spec discussion in https://github.com/w3c/webappsec-csp/pull/776. I'd like to get that worked out before landing an implementation. cc Antonio
Related details
- Hiroshige Hayashizaki
- Viktoria Zlatinova
Hiroshige Hayashizaki (Gerrit)
Hiroshige Hayashizaki added 1 comment![]( "Open in Gerrit")
check_header_type, options.initiator_info.name)) {Viktoria ZlatinovaThis parameter's purpose isn't clear to me. Can you help me understand the scenarios in which checking the request's context/destination wouldn't be enough?
Mike WestCurrently the context/destination are the same for external script speculation rules and HTTP header speculations rules, but CSP doesn't apply to the HTTP header case. Using the initiator here to continue exempting the header case - https://chromium.googlesource.com/chromium/src/+/1d7cf351315ad93d53b369011414990a8fde7dbd/third_party/blink/renderer/core/frame/csp/content_security_policy.cc#:~:text=//%20Speculation%20rules%20from,%7D
Hiroshige HayashizakiOk. I guess my next question is why it's a good idea to distinguish between those scenarios. :) It looks like Antonio asked about that on the CSP PR as well, and didn't get an answer (https://github.com/w3c/webappsec-csp/pull/776#issuecomment-3327621673). Perhaps you could provide one here?
Viktoria ZlatinovaAs for implementation, if we'd set `mojom::blink::RequestContextType::SCRIPT` for external spec rules from <script>, then the issue is resolved without modification to csp stack?
+1 to Mike about the spec -- I'll also ask around.
Mike WestYes, using `mojom::blink::RequestContextType::SCRIPT` resolves the issue without modification to the CSP stack. Updated to this, let me know what you think!
I can update the CSP/Fetch spec PRs separately to add some reasoning as to why the header case is exempt (with further discussion if needed)
Please do come back to the CSP spec discussion in https://github.com/w3c/webappsec-csp/pull/776. I'd like to get that worked out before landing an implementation. cc Antonio
I posted https://github.com/whatwg/html/pull/11697#issuecomment-3994806128 for clarifying the scope to be resolved before landing (or shipping) this feature.
Anyway the implementation will be anyway separate for separate issues:
This CL (current patch set) is for [1] in https://github.com/whatwg/html/pull/11697#issuecomment-3994806128, applying `script-src`, while [2] and [3] will anyway need separate CLs.
Related details
- Viktoria Zlatinova
Viktoria Zlatinova (Gerrit)
Viktoria Zlatinova added 2 comments![]( "Open in Gerrit")
check_header_type, options.initiator_info.name)) {Viktoria ZlatinovaThis parameter's purpose isn't clear to me. Can you help me understand the scenarios in which checking the request's context/destination wouldn't be enough?
Mike WestCurrently the context/destination are the same for external script speculation rules and HTTP header speculations rules, but CSP doesn't apply to the HTTP header case. Using the initiator here to continue exempting the header case - https://chromium.googlesource.com/chromium/src/+/1d7cf351315ad93d53b369011414990a8fde7dbd/third_party/blink/renderer/core/frame/csp/content_security_policy.cc#:~:text=//%20Speculation%20rules%20from,%7D
Hiroshige HayashizakiOk. I guess my next question is why it's a good idea to distinguish between those scenarios. :) It looks like Antonio asked about that on the CSP PR as well, and didn't get an answer (https://github.com/w3c/webappsec-csp/pull/776#issuecomment-3327621673). Perhaps you could provide one here?
Viktoria ZlatinovaAs for implementation, if we'd set `mojom::blink::RequestContextType::SCRIPT` for external spec rules from <script>, then the issue is resolved without modification to csp stack?
+1 to Mike about the spec -- I'll also ask around.
Mike WestYes, using `mojom::blink::RequestContextType::SCRIPT` resolves the issue without modification to the CSP stack. Updated to this, let me know what you think!
I can update the CSP/Fetch spec PRs separately to add some reasoning as to why the header case is exempt (with further discussion if needed)
Hiroshige HayashizakiPlease do come back to the CSP spec discussion in https://github.com/w3c/webappsec-csp/pull/776. I'd like to get that worked out before landing an implementation. cc Antonio
I posted https://github.com/whatwg/html/pull/11697#issuecomment-3994806128 for clarifying the scope to be resolved before landing (or shipping) this feature.
Anyway the implementation will be anyway separate for separate issues:
This CL (current patch set) is for [1] in https://github.com/whatwg/html/pull/11697#issuecomment-3994806128, applying `script-src`, while [2] and [3] will anyway need separate CLs.
Thanks Hiroshige and Mike! Per the latest plan discussed in https://github.com/whatwg/html/pull/11697, can we proceed with this CL?
network::mojom::blink::RequestDestination::kSpeculationRules);Viktoria ZlatinovaDropping this will change the `Sec-Fetch-Destination` header sent with the request, as well as the request's `destination` which is web-accessible via the `Request` object. Can you help me understand why you're removing the annotation?
Hiroshige HayashizakiI moved this part to SpeculationRulesResource, as it is shared by the HTTP header and external script cases: https://chromium.googlesource.com/chromium/src/+/1d7cf351315ad93d53b369011414990a8fde7dbd/third_party/blink/renderer/core/loader/resource/speculation_rules_resource.cc#:~:text=resource_request.SetRequestContext,%3A%3AkSpeculationRules)%3B
Viktoria ZlatinovaBasically setting the `resource_request` should be the responsibility of the callers of `*Resource::Fetch()`, so I feel it's better to place here.
Also the current patchset uses `SpeculationRulesResource` from `SpeculationRulesHeader::StartFetches()` and `ExternalSpeculationRulesClient` separately, but these two paths should be further merged (e.g. Lines 141-146 etc.), to ensure external speculation rules are loaded in the same way by Link header and by <script>.
So, How about further deduplicating the code here and at ScriptLoader and `ExternalSpeculationRulesClient` with `SpeculationRulesHeader::StartFetches()`, `SpeculationRuleLoader`?
e.g. introduce a common helper like
```
LoadExternalSpeculationRules(bunch of params...) {
... Line 133-165 here, plus necessary modification to accept parameters that differs between Link header and <script> ...
... ResourceRequest creation and modification are all done here ...
}
```and call `LoadExternalSpeculationRules()` here and from ScriptLoader.
Further deduped this code in `SpeculationRuleLoader`. Some of the response handling is also different between the header and script case so I kept those differences - for example, raising DOM events and adding the rules to script loader for script case, and keeping the console messages for header case.
- Hiroshige Hayashizaki
- Mike West
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mike West (Gerrit)
Mike West added 1 comment![]( "Open in Gerrit")
check_header_type, options.initiator_info.name)) {Viktoria ZlatinovaThis parameter's purpose isn't clear to me. Can you help me understand the scenarios in which checking the request's context/destination wouldn't be enough?
Mike WestCurrently the context/destination are the same for external script speculation rules and HTTP header speculations rules, but CSP doesn't apply to the HTTP header case. Using the initiator here to continue exempting the header case - https://chromium.googlesource.com/chromium/src/+/1d7cf351315ad93d53b369011414990a8fde7dbd/third_party/blink/renderer/core/frame/csp/content_security_policy.cc#:~:text=//%20Speculation%20rules%20from,%7D
Hiroshige HayashizakiOk. I guess my next question is why it's a good idea to distinguish between those scenarios. :) It looks like Antonio asked about that on the CSP PR as well, and didn't get an answer (https://github.com/w3c/webappsec-csp/pull/776#issuecomment-3327621673). Perhaps you could provide one here?
Viktoria ZlatinovaAs for implementation, if we'd set `mojom::blink::RequestContextType::SCRIPT` for external spec rules from <script>, then the issue is resolved without modification to csp stack?
+1 to Mike about the spec -- I'll also ask around.
Mike WestYes, using `mojom::blink::RequestContextType::SCRIPT` resolves the issue without modification to the CSP stack. Updated to this, let me know what you think!
I can update the CSP/Fetch spec PRs separately to add some reasoning as to why the header case is exempt (with further discussion if needed)
Hiroshige HayashizakiPlease do come back to the CSP spec discussion in https://github.com/w3c/webappsec-csp/pull/776. I'd like to get that worked out before landing an implementation. cc Antonio
Viktoria ZlatinovaI posted https://github.com/whatwg/html/pull/11697#issuecomment-3994806128 for clarifying the scope to be resolved before landing (or shipping) this feature.
Anyway the implementation will be anyway separate for separate issues:
This CL (current patch set) is for [1] in https://github.com/whatwg/html/pull/11697#issuecomment-3994806128, applying `script-src`, while [2] and [3] will anyway need separate CLs.
Thanks Hiroshige and Mike! Per the latest plan discussed in https://github.com/whatwg/html/pull/11697, can we proceed with this CL?
+Antonio, as it doesn't look to me as though that conversation on the spec has resolved yet.
Related details
- Antonio Sartori
- Hiroshige Hayashizaki
- Viktoria Zlatinova
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Antonio Sartori (Gerrit)
Antonio Sartori added 1 comment![]( "Open in Gerrit")
check_header_type, options.initiator_info.name)) {Viktoria ZlatinovaThis parameter's purpose isn't clear to me. Can you help me understand the scenarios in which checking the request's context/destination wouldn't be enough?
Mike WestCurrently the context/destination are the same for external script speculation rules and HTTP header speculations rules, but CSP doesn't apply to the HTTP header case. Using the initiator here to continue exempting the header case - https://chromium.googlesource.com/chromium/src/+/1d7cf351315ad93d53b369011414990a8fde7dbd/third_party/blink/renderer/core/frame/csp/content_security_policy.cc#:~:text=//%20Speculation%20rules%20from,%7D
Hiroshige HayashizakiOk. I guess my next question is why it's a good idea to distinguish between those scenarios. :) It looks like Antonio asked about that on the CSP PR as well, and didn't get an answer (https://github.com/w3c/webappsec-csp/pull/776#issuecomment-3327621673). Perhaps you could provide one here?
Viktoria ZlatinovaAs for implementation, if we'd set `mojom::blink::RequestContextType::SCRIPT` for external spec rules from <script>, then the issue is resolved without modification to csp stack?
+1 to Mike about the spec -- I'll also ask around.
Mike WestYes, using `mojom::blink::RequestContextType::SCRIPT` resolves the issue without modification to the CSP stack. Updated to this, let me know what you think!
I can update the CSP/Fetch spec PRs separately to add some reasoning as to why the header case is exempt (with further discussion if needed)
Hiroshige HayashizakiPlease do come back to the CSP spec discussion in https://github.com/w3c/webappsec-csp/pull/776. I'd like to get that worked out before landing an implementation. cc Antonio
Viktoria ZlatinovaI posted https://github.com/whatwg/html/pull/11697#issuecomment-3994806128 for clarifying the scope to be resolved before landing (or shipping) this feature.
Anyway the implementation will be anyway separate for separate issues:
This CL (current patch set) is for [1] in https://github.com/whatwg/html/pull/11697#issuecomment-3994806128, applying `script-src`, while [2] and [3] will anyway need separate CLs.
Mike WestThanks Hiroshige and Mike! Per the latest plan discussed in https://github.com/whatwg/html/pull/11697, can we proceed with this CL?
+Antonio, as it doesn't look to me as though that conversation on the spec has resolved yet.
My understanding is that we were trying to agree on https://github.com/whatwg/html/pull/11697#issuecomment-4070809909 (adding a new directive speculation-rules-src which falls back to script-src-elem). That makes sense to me (but do others agree with that?)
If that's the plan, I think it would be good to update the linked spec change (https://github.com/w3c/webappsec-csp/pull/776) which doesn't reflect that, possibly before moving forward with this CL.
Related details
- Hiroshige Hayashizaki
- Viktoria Zlatinova
Viktoria Zlatinova (Gerrit)
Viktoria Zlatinova added 1 comment![]( "Open in Gerrit")
@Hiroshige @Mike what do you think? I created a PR to w3c here capturing
these changes based off of Domenic's initial PR:
https://github.com/w3c/webappsec-csp/pull/808
Related details
- Antonio Sartori
- Hiroshige Hayashizaki
- Mike West
Hiroshige Hayashizaki (Gerrit)
Hiroshige Hayashizaki added 1 comment![]( "Open in Gerrit")
My understanding is that we were trying to agree on https://github.com/whatwg/html/pull/11697#issuecomment-4070809909 (adding a new directive speculation-rules-src which falls back to script-src-elem). That makes sense to me (but do others agree with that?)
I'm checking with the prefetch/preloading folks about CSP changes (e.g. is it OK to apply the new `speculationrules` CSP directive to speculation rules headers that are currently exempted from CSP), which seems to take a couple of weeks.
Also for clarifying the roadmap:
It seems we are heading to the non-incremental way (resolving all of [1][2][3] of https://github.com/whatwg/html/pull/11697#issuecomment-3994806128 before proceeding this CL), which will take more time. i.e.
- [A] landing spec PRs for [2][3] (
- https://github.com/w3c/webappsec-csp/pull/808 and other related PRs)
- [B] implementing [1] (the current patch set of this CL)
- [C] implementing [2][3], i.e. new `speculationrules` directives (none right now?)
Do we also need to land [C] (which might probably need its separate shipping/experiment process) before shipping external speculation rules?
Related details
- Antonio Sartori
- Mike West
- Viktoria Zlatinova
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Antonio Sartori (Gerrit)
Antonio Sartori added 1 comment![]( "Open in Gerrit")
I don't think we need to wait to land everything as long as we have an agreed plan and we go in one direction.
If the agreed direction is https://github.com/whatwg/html/pull/11697#issuecomment-4070809909 though, then we should put all speculation rules (also headers) behind script-src-elem (which I don't think this CL does). Having a draft PR on the spec is helpful to understand what we are agreeing upon (so updating https://github.com/w3c/webappsec-csp/pull/808 is helpful I believe).
I think we can ship external speculation rules before landing `speculation-rules-src` - the drawback will be that developers will not be able to use speculation rules headers with strict CSP.
Related details
- Mike West
- Viktoria Zlatinova