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

Rewrite Trusted types tests for CSP violations #50124

Merged
merged 11 commits into from
Jan 22, 2025

Conversation

fred-wang
Copy link
Contributor

Currently the listener to "securitypolicyviolation" is added before actually running the statement that triggers violations, so it could be possible that some violations are not caught. This bad pattern is duplicated in several trusted-types*reporting* tests.

This patch adds a new helper file to properly wrap the listener registration and statement execution in a promise, and reuses it in existing tests.

w3c/trusted-types#576

Currently the listener to "securitypolicyviolation" is added before
actually running the statement that triggers violations, so it could
be possible that some violations are not caught. This bad pattern is
duplicated in several `trusted-types*reporting*` tests.

This patch adds a new helper file to properly wrap the
listener registration and statement execution in a promise, and
reuses it in existing tests.

w3c/trusted-types#576
@fred-wang
Copy link
Contributor Author

cc @lukewarlow

This gives much better results for me in Firefox. Still need to convert the other reporting files and though.

@fred-wang fred-wang requested a review from lukewarlow January 16, 2025 17:43
@fred-wang fred-wang marked this pull request as ready for review January 16, 2025 17:43
@fred-wang
Copy link
Contributor Author

fred-wang commented Jan 16, 2025

It seems there are more tests for violation but they use a slightly different approach, so probably will handle them in separate PRs.

(also, some of these tests will probably need review and updates too, but for now let's keep the current expectations)

@fred-wang fred-wang marked this pull request as draft January 17, 2025 06:20
@fred-wang fred-wang marked this pull request as ready for review January 17, 2025 07:34
@fred-wang
Copy link
Contributor Author

@lukewarlow I fixed more tests this morning. The remaining ones using securitypolicyviolation are:

find -type f | xargs grep securitypolicyviolation | grep addEventListener | grep -v csp-violations.js
./empty-default-policy.html:    document.addEventListener("securitypolicyviolation", e => {
./default-policy-report-only.html:    document.addEventListener("securitypolicyviolation", e => {
./support/navigation-support.js:    document.addEventListener("securitypolicyviolation", bounceEventToOpener);
./resources/block-text-node-insertion.js:      document.addEventListener("securitypolicyviolation", e => {
./empty-default-policy-report-only.html:    document.addEventListener("securitypolicyviolation", e => {
./default-policy.html:    document.addEventListener("securitypolicyviolation", e => {
  • empty-default-policy.html, default-policy-report-only.html, empty-default-policy-report-only.html, default-policy.html: looks ok, assuming the promise_test is executed immediately so that it calls addEventListener before anything that can trigger an a security violation.
  • support/navigation-support.js: look ok too, addEventListener is called before the final navigationElement.click() that triggers the navigation.
  • block-text-node-insertion.js: usages of checkSecurityPolicyViolationEvent in block-text-node-insertion-into-script-element.html and block-text-node-insertion-into-svg-script-element.html look wrong, the tests execute statements causing violation reports, but checkSecurityPolicyViolationEvent (and so addEventListener) is only done at the end of the tests, so that can lead to flaky failures.

@@ -50,9 +44,9 @@
let script_src = script_origin +
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this isn't directly related to this PR but the way this URL is constructed doesn't work if you access WPT through https

let script_src = new URL("/trusted-types/support/set-inner-html.js", location.href).href; This would be better I think

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm but that might be wrong for the below tests? Maybe leave it as is for now but with a note explaining this mgiht not be ideal?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I guess we should try to fix that, but this PR is already big and we are not going to fix all existing issues in these tests. I'll try to fix that in a follow-up PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

checking https://github.com/web-platform-tests/wpt/blob/master/common/get-host-info.sub.js it seems we want ORIGIN and REMOTE_ORIGIN instead, but I'm not really sure so let's handle this separately

Copy link
Member

@lukewarlow lukewarlow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assuming the assertions inside the helper function are enough for violation object (though idk that they are see comment below), this LGTM

@fred-wang fred-wang merged commit bb5f835 into master Jan 22, 2025
13 of 16 checks passed
@fred-wang fred-wang deleted the tt-refactor-reporting-tests branch January 22, 2025 22:08
fred-wang added a commit that referenced this pull request Mar 19, 2025
…ting script's src

Double slashes were added in #50124
to work around testharness errors due to a HTML page being used as a script but
the actual fix is to use a JS file instead.
fred-wang added a commit that referenced this pull request Mar 19, 2025
…ting script's src (#51446)

Double slashes were added in #50124
to work around testharness errors due to a HTML page being used as a script but
the actual fix is to use a JS file instead.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants