-
Notifications
You must be signed in to change notification settings - Fork 79
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
Upstream Trusted Types enforcement in EnsureCSPDoesNotBlockStringCompilation #659
Conversation
…ilation - Also update the violation object resource definition.
@annevk just so I'm not putting all this spec stuff on your plate, do you know who else might be able to review this? |
https://bugs.webkit.org/show_bug.cgi?id=275392 Reviewed by NOBODY (OOPS!). This patch accurately follows the spec for sample clipping in trusted types. Spec: w3c/webappsec-csp#659 * LayoutTests/imported/w3c/web-platform-tests/content-security-policy/reporting/report-clips-sample.https-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/content-security-policy/reporting/report-clips-sample.https.html: * Source/WebCore/page/csp/ContentSecurityPolicy.cpp: (WebCore::ContentSecurityPolicy::didReceiveHeader): (WebCore::ContentSecurityPolicy::allowMissingTrustedTypesForSinkGroup const): (WebCore::ContentSecurityPolicy::reportViolation const): (WebCore::ContentSecurityPolicy::setUpgradeInsecureRequests): * Source/WebCore/page/csp/ContentSecurityPolicy.h:
I'll take a look tomorrow. 👍 |
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.
The algorithm LGTM. I left a few nits that seem relevant to me, but nothing substantive.
index.bs
Outdated
|
||
1. For each |arg| in |parameterArgs|: | ||
|
||
1. Let |index| be the index of |arg| in |parameterArgs|. |
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'd suggest writing this loop differently, as I don't think Infra provides a way to get the index of a given element in a list. Something like the following:
1. Assert: |parameterArgs|' [list/length=] is equal to [parameterStrings]' [=list/length=].
1. [=list/iterate|For each=] |index| of [=the range=] 0 to |parameterArgs|' [=list/length=]:
1. Let |arg| be |parameterArgs|[|index|].
Alternatively, we could add something to Infra to either create a For each
variant that provides both an item and its index, or some mechanism to get the index of a given item? @annevk might have thoughts about which path might be preferable.
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've gone with what you suggested for now.
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. Happy to merge this after you either accept or reject my annoying nits. :)
This should be ready to merge @mikewest (I have permissions but don't know if there's any specific commit message changes you'd like to make) |
Nope. In that case, still still LGTM. Feel free to merge it. :) |
Updates EnsureCSPDoesNotBlockStringCompilation to upstream changes from the Trusted Types spec. For non timers this now goes through the motions of checking CSP for trusted types and doing neccessary enforcement.
unsafe-eval is left as is.
Preview | Diff