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

Use "navigation request's policy container's CSP list" instead of "navigation request's client's global object's CSP list" #692

Merged

Conversation

mbrodesser-Igalia
Copy link
Contributor

@mbrodesser-Igalia mbrodesser-Igalia commented Nov 25, 2024

Complements #494 in order to make the spec consistent.

Preparation for fixing whatwg/html#4651.


Preview | Diff

…vigation request's client's global object's CSP list"

Complements #494 in order make
the spec consistent.

Preparation to fix whatwg/html#4651.
@antosart
Copy link
Member

I think the problem with this is that navigation request's policy container is initially "client" and is only updated to the source document's policy container thing during fetch (https://fetch.spec.whatwg.org/#concept-request-policy-container), while javascript: url navigations do not go through fetch. I think if you want this to work you need to set navigation request's policy container before this check is called from html.

Or am I missing anything?

@mbrodesser-Igalia
Copy link
Contributor Author

I think the problem with this is that navigation request's policy container is initially "client" and is only updated to the source document's policy container thing during fetch (https://fetch.spec.whatwg.org/#concept-request-policy-container), while javascript: url navigations do not go through fetch.

Correct.

There seems to be another issue, which should be addressed outside of this PR:

The request's policy container is set only in 1, step 12. "should navigation request of type be blocked by Content Security Policy?" is called from 2, step 19.3. 1 is called from step 19.5 of 2. So during the first iteration of the while-loop at step 19 of 2, the request's policy container will be its default value, "client". 3 doesn't handle "client", though.

I think if you want this to work you need to set navigation request's policy container before this check is called from html.

Yes.

The call from html, 4, step 5 is already broken since the request's client isn't set.

A corrective way forward here would be to merge this PR only together with setting the navigation request's policy container in the HTML spec. WDYT?

Or am I missing anything?

Footnotes

  1. https://fetch.spec.whatwg.org/#concept-fetch 2

  2. https://html.spec.whatwg.org/multipage/browsing-the-web.html#create-navigation-params-by-fetching 2 3

  3. https://www.w3.org/TR/CSP3/#should-block-navigation-request

  4. https://html.spec.whatwg.org/multipage/browsing-the-web.html#the-javascript:-url-special-case

@antosart
Copy link
Member

Right! Thanks for the deep investigation, this makes sense to me.

I agree the best thing is to set navigation request's policy container in html before calling the CSP check.

@ciaramcmullin ciaramcmullin added agenda+ To be discussed at a triage meeting blocked Unable to proceed due to pending work or discussion. and removed agenda+ To be discussed at a triage meeting labels Dec 16, 2024
@ciaramcmullin
Copy link
Collaborator

Blocked on whatwg/html#10796

domenic added a commit to domenic/webappsec-csp that referenced this pull request Jan 28, 2025
This allows HTML to pass in the correct CSP list, to help fix whatwg/html#10796.

See also: whatwg/html#10949 and w3c#494.

Closes w3c#692 by superseding it.
domenic added a commit to domenic/webappsec-csp that referenced this pull request Jan 28, 2025
This allows HTML to pass in the correct CSP list, to help fix whatwg/html#10796.

See also: whatwg/html#10949 and w3c#494.

Closes w3c#692 by superseding it.
@domenic
Copy link
Contributor

domenic commented Jan 28, 2025

I have created an updated version of this at #705 which integrates with whatwg/html#10949. Review and merging would be lovely.

After some more discussion in whatwg/html#10949, it turns out this PR works well as-is, and we only needed to fix HTML. Please merge it.

domenic added a commit to whatwg/html that referenced this pull request Feb 6, 2025
Closes #10796, by passing along the intended snapshotted source CSP instead of attempting to look up the policy container from the request (which will not work when it's left as "client").

w3c/webappsec-csp#692 is also necessary to fully get the intended behavior.
@domenic
Copy link
Contributor

domenic commented Feb 6, 2025

I've merged the HTML PR, so please merge this whenever it is convenient!

@antosart antosart removed the blocked Unable to proceed due to pending work or discussion. label Feb 6, 2025
@antosart
Copy link
Member

antosart commented Feb 6, 2025

Thanks @domenic!

@antosart antosart merged commit 535ad80 into main Feb 6, 2025
2 checks passed
@antosart antosart deleted the NoBug_use_policy_containers_CSP_instead_of_globals_CSP branch February 6, 2025 07:16
github-actions bot added a commit that referenced this pull request Feb 6, 2025
…vigation request's client's global object's CSP list" (#692)

SHA: 535ad80
Reason: push, by antosart

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
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 this pull request may close these issues.

4 participants