Simple test with `require-trusted-types-for 'script';` CSP throws about wpt HOT 27 OPEN

I wonder how intrusive it would be to make the WPT helpers trusted type compliant? The alternative is a custom harness for trusted type tests that does what it needs to in a trusted types compliant way?

from wpt.

I wonder how intrusive it would be to make the WPT helpers trusted type compliant?

Don't know. Presumably the only way to find out is to do it. @otherdaniel has this been attempted before?

The alternative is a custom harness for trusted type tests that does what it needs to in a trusted types compliant way?

It seems that would need more effort than making the WPT helpers TT compliant.

from wpt.

Yeah that's the trade off, benefit it's less intrusive to the rest of the suite, it wouldn't need to be the whole harness like just the helpers we use but just cloning rather than changing the ones used everywhere else.

from wpt.

Yeah that's the trade off, benefit it's less intrusive to the rest of the suite, it wouldn't need to be the whole harness like just the helpers we use but just cloning rather than changing the ones used everywhere else.

@smaug---- suggested potentially running the tests in a new window, reporting their result via postMessage. Don't know if that would allow calling assert_*s (e.g. assert_equals, https://searchfox.org/mozilla-central/source/testing/web-platform/tests/resources/testharness.js#1531) though.

from wpt.

You should be able to use fetch_tests_from_window, compare for example https://github.com/web-platform-tests/wpt/tree/master/FileAPI/url/resources/fetch-tests.js

from wpt.

I wonder how intrusive it would be to make the WPT helpers trusted type compliant?

Don't know. Presumably the only way to find out is to do it. @otherdaniel has this been attempted before?

I don't recall having done anything in particular to make Trusted Types work with WPT. wpt.fyi seems to run the tests just fine.

from wpt.

Yeah, as explained in #44348, the issue comes up when running a test manually, because it goes wrong when rendering the results table and "rerun" button

from wpt.

You should be able to use fetch_tests_from_window, compare for example https://github.com/web-platform-tests/wpt/tree/master/FileAPI/url/resources/fetch-tests.js

Okay, that's https://searchfox.org/mozilla-central/rev/9bb5d5f55da6cc7163dd93825c25609b769822f9/testing/web-platform/tests/resources/testharness.js#3886-3897 and should work.

from wpt.

You should be able to use fetch_tests_from_window, compare for example https://github.com/web-platform-tests/wpt/tree/master/FileAPI/url/resources/fetch-tests.js

Okay, that's https://searchfox.org/mozilla-central/rev/9bb5d5f55da6cc7163dd93825c25609b769822f9/testing/web-platform/tests/resources/testharness.js#3886-3897 and should work.

Since existing tests suffer from the same problem (e.g. #44348), the proper fix would be to either rewrite all of those tests or fix the testharness. The existing tests are quite numerous (https://searchfox.org/mozilla-central/source/testing/web-platform/tests/trusted-types).

from wpt.

When commenting out https://searchfox.org/mozilla-central/rev/9bb5d5f55da6cc7163dd93825c25609b769822f9/testing/web-platform/tests/resources/testharness.js#4367,4507 the harness throws no exceptions.

from wpt.

For https://searchfox.org/mozilla-central/rev/9bb5d5f55da6cc7163dd93825c25609b769822f9/testing/web-platform/tests/resources/testharness.js#4367 there's a performance-relevant comment at https://searchfox.org/mozilla-central/rev/9bb5d5f55da6cc7163dd93825c25609b769822f9/testing/web-platform/tests/resources/testharness.js#4257-4261.

CC @smaug----

from wpt.

For https://searchfox.org/mozilla-central/rev/9bb5d5f55da6cc7163dd93825c25609b769822f9/testing/web-platform/tests/resources/testharness.js#4367 there's a performance-relevant comment at https://searchfox.org/mozilla-central/rev/9bb5d5f55da6cc7163dd93825c25609b769822f9/testing/web-platform/tests/resources/testharness.js#4257-4261.

CC @smaug----

According to @smaug---- the performance-comment is presumably still relevant, because using innerHTML saves JS wrappers for the <td> elements.

from wpt.

I wonder how intrusive it would be to make the WPT helpers trusted type compliant? The alternative is a custom harness for trusted type tests that does what it needs to in a trusted types compliant way?

Another alternative is to set a default policy which propagates scripts and HTML at the end of the last test. I.e.:

t.add_cleanup(() => {
      // The testharness passes strings to injection sinks, see <TODO-file-bug>.
      // The following default policy allows that.
      const p = trustedTypes.createPolicy("default", { createHTML: s => s, createScript: s => s});
})

Works for synchronous tests which aren't already using a default policy.

from wpt.

I wonder how intrusive it would be to make the WPT helpers trusted type compliant? The alternative is a custom harness for trusted type tests that does what it needs to in a trusted types compliant way?

Another alternative is to set a default policy which propagates scripts and HTML at the end of the last test. I.e.:

t.add_cleanup(() => {
      // The testharness passes strings to injection sinks, see <TODO-file-bug>.
      // The following default policy allows that.
      const p = trustedTypes.createPolicy("default", { createHTML: s => s, createScript: s => s});
})

Works for synchronous tests which aren't already using a default policy.

This might be less intrusive than the suggested alternatives.

Once trusted types are supported by all browser engines, the testharness could be refactored to use non-default policies at the relevant sinks, allowing to remove the default policies in the tests' cleanup functions.

from wpt.

Since existing tests suffer from the same problem (e.g. #44348), the proper fix would be to either rewrite all of those tests or fix the testharness. The existing tests are quite numerous (https://searchfox.org/mozilla-central/source/testing/web-platform/tests/trusted-types).

Maybe adapting the testharness wouldn't be that much work. I tried doing this in a draft-PR, here: #44561 This basically de-string-ifies building of the result table.

For https://searchfox.org/mozilla-central/rev/9bb5d5f55da6cc7163dd93825c25609b769822f9/testing/web-platform/tests/resources/testharness.js#4367 there's a performance-relevant comment at https://searchfox.org/mozilla-central/rev/9bb5d5f55da6cc7163dd93825c25609b769822f9/testing/web-platform/tests/resources/testharness.js#4257-4261.
CC @smaug----

According to @smaug---- the performance-comment is presumably still relevant, because using innerHTML saves JS wrappers for the <td> elements.

Ah... that's a very good point I hadn't thought of. Provided the PR above looks generally good to people, what exactly should I measure to figure out whether the performance cost is still here?

from wpt.

Since existing tests suffer from the same problem (e.g. #44348), the proper fix would be to either rewrite all of those tests or fix the testharness. The existing tests are quite numerous (https://searchfox.org/mozilla-central/source/testing/web-platform/tests/trusted-types).

Maybe adapting the testharness wouldn't be that much work. I tried doing this in a draft-PR, here: #44561 This basically de-string-ifies building of the result table.

For https://searchfox.org/mozilla-central/rev/9bb5d5f55da6cc7163dd93825c25609b769822f9/testing/web-platform/tests/resources/testharness.js#4367 there's a performance-relevant comment at https://searchfox.org/mozilla-central/rev/9bb5d5f55da6cc7163dd93825c25609b769822f9/testing/web-platform/tests/resources/testharness.js#4257-4261.
CC @smaug----

According to @smaug---- the performance-comment is presumably still relevant, because using innerHTML saves JS wrappers for the <td> elements.

Ah... that's a very good point I hadn't thought of. Provided the PR above looks generally good to people, what exactly should I measure to figure out whether the performance cost is still here?

Trying to figure that out. It might be, that such measurements nowadays are unnecessary. The performance comment at https://searchfox.org/mozilla-central/rev/9bb5d5f55da6cc7163dd93825c25609b769822f9/testing/web-platform/tests/resources/testharness.js#4257-4261 was added 2014, so things might've changed. One argument for that is that the runs at <wpt.fyi> potentially don't need that information. See e.g. https://wpt.fyi/results/trusted-types/block-string-assignment-to-attribute-via-attribute-node.html?label=experimental&label=master&aligned.

Moreover, when running a folder of tests locally, e.g.

./wpt run --no-headless chrome trusted-types/

the output table is never shown.

It's only shown when executing a single test file, e.g.:

./wpt run --no-headless chrome trusted-types/

Single tests have a timeout of at most 60s, see https://web-platform-tests.org/writing-tests/testharness-api.html#harness-timeout.

The variant feature (https://web-platform-tests.org/writing-tests/testharness.html#variants) allows exceeding that timeout, but no output table is shown in that case.

Is anyone aware of present-day scenarios when the output table is generated for multiple, e.g. hundreds or thousands, of tests?

from wpt.

http://wpt.live/html/dom/reflection-text.html has over 10000

from wpt.

http://wpt.live/html/dom/reflection-text.html has over 10000

It executes in a few seconds though.

from wpt.

cc @otherdaniel

from wpt.

The time to build the results table (with this patch) for html/dom/reflection-text.text (with indeed >10k entries) took ~800ms (my machine; current version of Chrome). That was the total build time; not the cost difference between parsing and .textContent. I'd say the warning of "Using textContent on each individual adds tens of seconds of execution time for large test suites (tens of thousands of tests)." is no longer accurate.

Unless anyone has additional concerns, I'll then clean up the patch; handle Mirko's feedback; and submit.

from wpt.

The time to build the results table (with this patch) for html/dom/reflection-text.text (with indeed >10k entries) took ~800ms (my machine; current version of Chrome). That was the total build time; not the cost difference between parsing and .textContent. I'd say the warning of "Using textContent on each individual adds tens of seconds of execution time for large test suites (tens of thousands of tests)." is no longer accurate.

Unless anyone has additional concerns, I'll then clean up the patch; handle Mirko's feedback; and submit.

Agree. Moreover the 60s limit mentioned at #44352 (comment) applies to all tests.

@jgraham PTAL at the last question mentioned in #44352 (comment).

from wpt.

@jgraham could you also PTAL at the corresponding testharness patch. Especially at https://chromium-review.googlesource.com/c/chromium/src/+/5290877/comment/b3ec4e27_c7bdddba/.

from wpt.

Could we move this patch into github, please? Although we generally support syncing with vendor repos, for infra changes it's quite hard to be sure that they work correctly if they are only run through vendor CI. In this specific case I'd like to fetch the branch and test the perf difference locally.

from wpt.

Could we move this patch into github, please? Although we generally support syncing with vendor repos, for infra changes it's quite hard to be sure that they work correctly if they are only run through vendor CI. In this specific case I'd like to fetch the branch and test the perf difference locally.

CC @otherdaniel

from wpt.

Could we move this patch into github, please? Although we generally support syncing with vendor repos, for infra changes it's quite hard to be sure that they work correctly if they are only run through vendor CI. In this specific case I'd like to fetch the branch and test the perf difference locally.

#44561

from wpt.

I meant a real PR on wpt, not one that's owned by the Chromium CI bot and where the status is controlled by what happens in Chromium's code review.

from wpt.

I meant a real PR on wpt, not one that's owned by the Chromium CI bot and where the status is controlled by what happens in Chromium's code review.

Ah, I didn't realize this would block perf testing. Please look at it here: #45002

Please also look at #45003 That seems to do extra work after each test run, which might be relevant if we're performance limited. It's code that was originally added (see reference in PR description) to support an experiment. A cursory search doesn't turn up any usage of it. Even if it's used, there might be better ways to do this, e.g. adding the string to something that isn't a <script>.

from wpt.