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.

Sign up for GitHub

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

Jump to bottom

Resource hint blocking / "least restrictive" as specified does nothing? #633

Open
evilpie opened this issue Dec 27, 2023 · 5 comments · May be fixed by #637
Open

Resource hint blocking / "least restrictive" as specified does nothing? #633

evilpie opened this issue Dec 27, 2023 · 5 comments · May be fixed by #637

Comments

@evilpie
Copy link
Contributor

evilpie commented Dec 27, 2023 •
edited
Loading

"6.7.2.2 Does resource hint request violate policy?" executes the pre-request check on every directive.

The pre-request check of almost all directives looks like this (e.g. img-src Pre-request check):

  1. Let name be the result of executing § 6.8.1 Get the effective directive for request on request.
  2. If the result of executing § 6.8.4 Should fetch directive execute on name, img-src and policy is "No", return "Allowed".

6.8.1. Get the effective directive for request

  1. If request’s initiator is "prefetch" or "prerender", return default-src.

6.8.4. Should fetch directive execute

  1. Let directive fallback list be the result of executing § 6.8.3 Get fetch directive fallback list on effective directive name.

6.8.3 Get fetch directive fallback list

  1. Switch on directive name: (This does not include default-src)
  2. Return << >>.

So we probably need to extend 6.8.3. However that still means we would only match the explicit default-src. In the end it seems like using the pre-request check, for what is a totally unrelated request, is just bound to cause issues.
Maybe it would be better to have an explicit list of supported directives e.g. img/video/script etc. and just call § 6.7.2.4 Does request match source list? directly from 6.7.2.2.

#582
/cc @noamr
@noamr
Copy link
Contributor

noamr commented Dec 28, 2023

Thanks, yes I can see the bug in the spec. Since this is implemented in Chromium I'll try to find where they diverge and amend the spec.

@noamr
Copy link
Contributor

noamr commented Jan 15, 2024

I'm seeing that here we call "Get the effective directive for request" but that doesn't exist in chromium, istead there is something called the "operative directive".
@antosart or others, do you have any background for this?

@antosart
Copy link
Member

I think "Get the effective directive for request" corresponds in chromium to GetDirectiveTypeFromRequestContextType.

I don't know the background, but I suspect that the implementation diverges from the spec for optimization reasons (for example for an image, the spec mandates to do a loop over all directives and perform the pre-request check of every directive, but that pre-request check will be useless unless the directive is img-src or (if there is no img-src) default-src, so what chromium does instead is: check if there is img-src or else fallback to default-src and just perform that one pre-request check).

Maybe it would be better to have an explicit list of supported directives e.g. img/video/script etc. and just call § 6.7.2.4 Does request match source list? directly from 6.7.2.2.

I think this is the best way to go.

noamr added a commit to noamr/webappsec-csp that referenced this issue Jan 16, 2024
@noamr
Resource hint: check directives explicitly
a0be36b 
Apparently the previous wording was a no-op.
Instead of calling the pre-request check, checking
the resource list for the directives that have that as a value.

Closes w3c#633
@noamr noamr linked a pull request Jan 16, 2024 that will close this issue
Open
@noamr
Copy link
Contributor

noamr commented Jan 16, 2024

OK then! #637

@jonathanKingston
Copy link
Contributor

@noamr can I check the wording of the default directive also:

If |defaultDirective| does not exist, return "Does Not Violate"

In the absence of a default-src, doesn't that skip all validation? I'm assuming that's not the intended behaviour at least.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Resource hint: check directives explicitly noamr/webappsec-csp
4 participants
@noamr @evilpie @jonathanKingston @antosart