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

Clarify Speculation Rules header interaction with CSP #327

Closed
SulemanAhmadd opened this issue Aug 13, 2024 · 10 comments · Fixed by #340
Closed

Clarify Speculation Rules header interaction with CSP #327

SulemanAhmadd opened this issue Aug 13, 2024 · 10 comments · Fixed by #340
Assignees

Comments

@SulemanAhmadd
Copy link

We have seen that pages that have strict CSP headers (strict-dynamic) set on a page, refuse to load speculation rules from the link in the header.

The specification talks about applying CSP for speculation rules that are added by an inline script. Its unclear from the spec whether the same CSP directive apply to rulesets retrieved using the header?

The inline-speculation-rules CSP value allows inclusion of speculation rules from inline <script> tags, does the same apply for rules dynamically loaded using the Speculation Rules header?

@domenic
Copy link
Collaborator

domenic commented Aug 14, 2024

/cc @jeremyroman to check my reading

We have seen that pages that have strict CSP headers (strict-dynamic) set on a page, refuse to load speculation rules from the link in the header.

Can you provide a demo page?

Its unclear from the spec whether the same CSP directive apply to rulesets retrieved using the header?

It should not apply, by my reading of the spec, since such fetches use the "speculationrules" request destination.

The inline-speculation-rules CSP value allows inclusion of speculation rules from inline <script> tags, does the same apply for rules dynamically loaded using the Speculation Rules header?

Per spec, it only affects inline speculation rules scripts.

@SulemanAhmadd
Copy link
Author

SulemanAhmadd commented Aug 14, 2024

We are working with a partner. Unfortunately, they mentioned that they dont have access to the specific error producing page anymore (they reverted the speculation rules change) so we cannot reproduce at the moment, but they did share the following screenshot:

image

The Speculation-Rules header for that page was: Speculation-Rules: "/cdn-cgi/speculation-rules?url=www.wthubspot.com/"

So its obvious that www.wthubspot.com is not part of their CSP policy. But, should we clarify in the specification that CSP directives are respected for ruleset fetches? Is this intended behaviour?

@jeremyroman
Copy link
Collaborator

Its unclear from the spec whether the same CSP directive apply to rulesets retrieved using the header?

It should not apply, by my reading of the spec, since such fetches use the "speculationrules" request destination.

But that request destination has script-src as its effective directive: https://wicg.github.io/nav-speculation/speculation-rules.html#content-security-policy-patches-effective-directive

I would have expected the 'self' part of the directive to allow this, though. My first guess here is that 'strict-dynamic' is causing trouble, since it causes a nonce or hash to be required for any fetch respecting the directive. At the moment that's simply incompatible, but maybe we should fix it.

The two basic options would seem to be:

  • ignore 'strict-dynamic' for speculation rules (it doesn't apply to other resource types)
  • expand Speculation-Rules to provide a place for supplying a nonce or hash, and then expect authors to use that (e.g., Speculation-Rules: "/cdn-cgi/speculation-rules?url=www.wthubspot.com/"; nonce="nFShhc..."

Honestly I suspect the former is correct, assuming this is indeed the problem, but would want a CSP expert's take, too.

@SulemanAhmadd
Copy link
Author

expand Speculation-Rules to provide a place for supplying a nonce or hash, and then expect authors to use that

How would website owners specify that nonce? The rules retrieved from the header do not involve specific a script tag. Also, this option would make CDN based deployments using the header painful.

ignore 'strict-dynamic' for speculation rules (it doesn't apply to other resource types)

Have to see if there are no security implications. Looks okay to me on the surface.

Other than that, the wording on the draft at https://wicg.github.io/nav-speculation/speculation-rules.html#content-security-policy-patches-effective-directive, is very confusing. Seems incomplete?

@jeremyroman
Copy link
Collaborator

expand Speculation-Rules to provide a place for supplying a nonce or hash, and then expect authors to use that

How would website owners specify that nonce? The rules retrieved from the header do not involve specific a script tag. Also, this option would make CDN based deployments using the header painful.

It would look something like the example I gave later on (appearing as a parameter in the header); this would be a new addition.

I certainly agree that this would make the CDN case harder. The CDN would have to parse and potentially modify the CSP header, if one exists.

It already potentially has to, because script-src might not include 'self', but in practice I'm pretty sure authors usually do include it. If we did make CDNs have to do this, it wouldn't be entirely new (if you use a CDN to inject a telemetry script or similar into the response body today, it's also necessary to make sure it's compliant with the CSP).

ignore 'strict-dynamic' for speculation rules (it doesn't apply to other resource types)

Have to see if there are no security implications. Looks okay to me on the surface.

There are some security implications in contrived cases, at least, but I think they're fairly slight in practice.

Other than that, the wording on the draft at https://wicg.github.io/nav-speculation/speculation-rules.html#content-security-policy-patches-effective-directive, is very confusing. Seems incomplete?

All of section 2 is just patches against [CSP]. This particular section should just adds one switch case to this algorithm.

In theory making the Speculation-Rules header CSP exempt altogether would also work, but I don't think that's likely to be a good path forward.

@SulemanAhmadd
Copy link
Author

If we did make CDNs have to do this, it wouldn't be entirely new (if you use a CDN to inject a telemetry script or similar into the response body today, it's also necessary to make sure it's compliant with the CSP).

Fair point. I believe then it would be another extra check for CDN deployments to make sure the nonce matches the strict-dynamic directive nonce value for all requests.

The least friction solution in my opinion would be to ignore strict-dynamic for Speculation Rules injected through the header.

@jeremyroman
Copy link
Collaborator

jeremyroman commented Aug 16, 2024

I have been able to reproduce what I guessed in #327 (comment), with a minimal header of Content-Security-Policy: script-src 'self' 'strict-dynamic' (and removing 'strict-dynamic' causes the load to succeed).

@jeremyroman
Copy link
Collaborator

Quick update here -- still need to discuss what is feasible here from our security team's perspective, but summer vacations are affecting availability.

@SulemanAhmadd
Copy link
Author

Thanks @jeremyroman, looking forward to it. Some site owners have responded back that unless they modify their CSP header to remove strict-dynamic, it has been affecting their Google PageSpeed rating as well due to the console errors.

@kjmcnee
Copy link
Collaborator

kjmcnee commented Oct 4, 2024

Hello. Based on said discussions, the proposed resolution is to simply not apply CSP to fetches triggered by the Speculation Rules header, as it is outside of CSP's threat model.

I'll be updating the spec, so I'll assign this issue to myself. Liviu is handling the implementation.

@kjmcnee kjmcnee self-assigned this Oct 4, 2024
@kjmcnee kjmcnee linked a pull request Oct 4, 2024 that will close this issue
domenic pushed a commit that referenced this issue Oct 7, 2024
Per discussions, this is outside of CSP's threat model. Closes #327.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants