feat: support running with TrustedTypes enforced #4447

rictic · 2020-09-15T00:48:00Z

Description of the Change

Trusted Types is a new Content Security Policy specification,
currently implemented in browsers based on Chromium 83 or higher, which
requires that data passed to APIs which may result in arbitrary code
execution must go through an explicit policy. This helps to catch
unintended use of dangerous APIs, and reduces the surface area for
some security reviews.

I'm not sure if test infrastructure like mocha is a likely target
for attack – seems like in most cases an attacker could only access test
data, and it is rare for tests to handle untrusted data. However,
there's value for infrastructure to be compatible with running with
Trusted Types enabled, as it will allow users to write tests to ensure
that the code under test can run with Trusted Types.

This change creates and applies policies for the two places in mocha
that call innerHTML, and adds a temporary patch to the rollup build.
With those changes in place, we can run mocha's karma tests with
Trusted Types enabled (save for the one test that runs with requirejs,
which relies on eval).

Alternate Designs

It would be possible to integrate dompurify into the HTML reporter, which would make it more secure. Alternatively, it may be possible to do a larger refactoring to construct the HTML output in a more structured way, rather than using string concatenation of HTML.

Why should this be in core?

This must be in core in order for these parts of core to be trusted types compatible.

Benefits

Users will be able to test their code with mocha in a browser with native Trusted Types enforcement enabled.

Possible Drawbacks

This change adds additional code, which adds weight to the mocha package for users, and maintenance burden on the mocha core devs.

Our own dependencies may inadvertently break us by doing otherwise safe operations (e.g. eval(str), Function(str), setting innerHTML, etc).

This is an enhancement (minor release).

More info and related work

Spec: https://w3c.github.io/webappsec-trusted-types/dist/spec/#introduction
Related PR adding support to karma: feat(client): Add trusted types support karma-runner/karma#3360

Trusted Types is a new Content Security Policy specification, currently implemented in browsers based on Chromium 83 or higher, which requires that data passed to APIs which may result in arbitrary code execution must go through an explicit policy. This helps to catch unintended use of dangerous APIs, and reduces the surface area for some security reviews. I'm not sure if test infrastructure like mocha is a likely target for attack – seems like in most cases an attacker could only access test data, and it is rare for tests to handle untrusted data. However, there's value for infrastructure to be compatible with running with Trusted Types enabled, as it will allow users to write tests to ensure that the code under test can run with Trusted Types. This change creates and applies policies for the two places in mocha that call innerHTML, and adds a temporary patch to the rollup build. With those changes in place, we can run mocha's karma tests with Trusted Types enabled (save for the one test that runs with requirejs, which relies on eval). More info: * Spec: https://w3c.github.io/webappsec-trusted-types/dist/spec/#introduction * Related PR adding support to karma: karma-runner/karma#3360

jsf-clabot · 2020-09-15T00:48:03Z

All committers have signed the CLA.

coveralls · 2020-09-15T01:01:42Z

Coverage increased (+0.007%) to 93.966% when pulling 095bc91 on rictic:trustedTypes into 738a575 on mochajs:master.

boneskull

Thanks! Seems like a good addition. Unfortunately, I don't know anything about Trusted Types, so I think I need to educate myself or defer the decision to another maintainer who does (e.g., @Munter?).

My main question is: if a test is written using eval(), and we ship this change, will the test now fail when run in a Trusted-Types-supporting browser? If so, this behavior must be opt-in. And if it needs to be opt-in, does this defeat the purpose?

boneskull · 2020-09-25T20:34:59Z

rollup.config.js

+ * 2021-01-01. This behavior is tested, so we can just remove the plugin
+ * from our array and try `npm test`. If the tests pass, this can be removed.
+ */
+const applyTemporaryCspPatchPlugin = {


Seems like others would have this problem. Is there not an "official" fix anywhere?

The latest version of regenerator-runtime is compatible with trusted types. It looks like some dependency of a dependency is shipping with an older regenerator-runtime version compiled in. That's why I'm expecting that in a few months they'll have shipped an update with the new regenerator-runtime and we can drop applyTemporaryCspPatchPlugin

Speaking from the perspective of a reviewer in September 2020: I'd want a comment in the code pointing to the relevant tracking issue. That way it's easier to get context on it and check whether it's been resolved.

Speaking from the perspective of a reviewer now, in 2024: I'm betting this has since been resolved. 😄

lib/reporters/html.js

boneskull · 2020-09-25T20:45:43Z

Further, I think it'd be good to see a test which asserts the behavior works as intended.

rictic · 2020-09-26T22:22:26Z

My main question is: if a test is written using eval(), and we ship this change, will the test now fail when run in a Trusted-Types-supporting browser? If so, this behavior must be opt-in. And if it needs to be opt-in, does this defeat the purpose?

Trusted type enforcement is opt in. Like other CSP features, you enable it with either an HTTP header or with a <meta> tag in the page's <head>.

The change should have no effect on existing tests, which won't have opted into enforcing Trusted Types.

Shipping this change will make mocha compatible with trusted types, so it will enable testing with mocha with TT enforced. See e.g. lit/lit#1323.

So tests that use potential unsafe APIs like eval() will not be broken by this change. They'd need to take the additional step of configuring their test runtime to enforce trusted types, in which case breaking a naive use of eval() would be the point.

Further, I think it'd be good to see a test which asserts the behavior works as intended.

See the changes to karma.conf.js, which adds headers to enable Trusted Type enforcement for mocha's existing browser tests (in supporting browsers, which is currently browsers based on a recent Chromium like Brave, Chrome, and Edge).

boneskull · 2020-09-29T23:41:35Z

Further, I think it'd be good to see a test which asserts the behavior works as intended.
See the changes to karma.conf.js, which adds headers to enable Trusted Type enforcement for mocha's existing browser tests (in supporting browsers, which is currently browsers based on a recent Chromium like Brave, Chrome, and Edge).

sorry, what I meant was that the existing tests assert compatibility, but we do not have:

a test asserting some sort of exception is thrown when unsafe code is run
a test asserting no such exception is thrown when unsafe code is run

boneskull · 2020-09-29T23:42:20Z

(AppVeyor is no longer building our PRs; I've removed it from the list of checks)

rictic · 2020-09-30T05:30:52Z

I'm not 100% sure I follow. You'd like to see a test of native Trusted Types functionality? A test that only runs in Chromium browsers and would fail if the trusted types headers in karma.conf.js didn't result in TT being enforced?

boneskull · 2020-10-02T21:50:36Z

I'm not 100% sure I follow. You'd like to see a test of native Trusted Types functionality? A test that only runs in Chromium browsers and would fail if the trusted types headers in karma.conf.js didn't result in TT being enforced?

Yes, essentially:

a test that runs in Chromium that asserts "unsafe" code will break
a test that runs either in Chromium w/ the headers disabled, or somewhere else that lacks support, and asserts "unsafe" code does not break

The point is not to test the browser feature--I'm sure Chromium has plenty of tests--but rather to establish a baseline of behavior which Mocha will exhibit when encountering these configurations. We need to make sure that future changes in Mocha do not violate the assumptions of its behavior established here.

outsideris · 2020-10-03T09:26:59Z

I need to learn about TrustedTypes.

boneskull · 2020-11-04T20:10:34Z

@rictic are you able to continue on this PR?

JoshuaKGoldberg

Looks like a good start & most of the way there! Requesting changes on the HTML reporter, with the note that I'm not confident. 🙂

Also, +1 to boneskull's requests for more testing.

JoshuaKGoldberg · 2024-03-01T21:28:00Z

rollup.config.js

+ * 2021-01-01. This behavior is tested, so we can just remove the plugin
+ * from our array and try `npm test`. If the tests pass, this can be removed.
+ */
+const applyTemporaryCspPatchPlugin = {


Speaking from the perspective of a reviewer in September 2020: I'd want a comment in the code pointing to the relevant tracking issue. That way it's easier to get context on it and check whether it's been resolved.

Speaking from the perspective of a reviewer now, in 2024: I'm betting this has since been resolved. 😄

JoshuaKGoldberg · 2024-03-01T21:30:15Z

lib/browser/highlight-tags.js

+  createHTML: function(html) {
+    // The highlight function escapes its input.
+    return highlight(html);
+  }


[Style] Nit: no need for this wrapper, I think?

Suggested change

createHTML: function(html) {

// The highlight function escapes its input.

return highlight(html);

}

// The highlight function escapes its input.

createHTML: highlight,

JoshuaKGoldberg · 2024-03-01T21:33:15Z

lib/reporters/html.js

+     * processed by a secure sanitization system like dompurify
+     */
+    return html;
+  }


[Bug] Indeed, this looks like it partially (not completely) bypasses the point of Trusted Types. Which worries me. If Mocha were to ship with this change as-is and claim to supported Trusted Types, that claim would be a little misleading.

I'd think making this truly "safe" should be a blocker to the PR. But I'm very open to being convinced otherwise if I'm off.

JoshuaKGoldberg · 2024-03-02T16:39:19Z

👋 coming back to this @rictic, are you still interested in working on this PR? As of #5027 there are again maintainers who can review it now. No worries if you no longer have time - but if you do that'd be great!

rictic mentioned this pull request Sep 15, 2020

[infra] Add support for trusted types to test system lit/lit#1270

Open

boneskull reviewed Sep 25, 2020

View reviewed changes

boneskull added area: browser browser-specific type: feature enhancement proposal semver-minor implementation requires increase of "minor" version number; "features" labels Sep 25, 2020

boneskull requested a review from Munter September 25, 2020 20:45

rictic mentioned this pull request Sep 26, 2020

[lit-html] WIP on supporting trusted types lit/lit#1323

Closed

rictic added 2 commits September 26, 2020 15:22

Use let over var, drop unnecessary check for window in html reporter.

35511d0

Restore check for window, tests fail without it.

095bc91

JoshuaKGoldberg requested changes Mar 1, 2024

View reviewed changes

JoshuaKGoldberg added the status: waiting for author waiting on response from OP - more information needed label Mar 1, 2024

JoshuaKGoldberg changed the title ~~Support running with TrustedTypes enforced.~~ feat: support running with TrustedTypes enforced Mar 4, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: support running with TrustedTypes enforced #4447

feat: support running with TrustedTypes enforced #4447

rictic commented Sep 15, 2020

jsf-clabot commented Sep 15, 2020 •

edited

coveralls commented Sep 15, 2020 •

edited

boneskull left a comment

boneskull Sep 25, 2020

rictic Sep 26, 2020

JoshuaKGoldberg Mar 1, 2024

boneskull commented Sep 25, 2020

rictic commented Sep 26, 2020 •

edited

boneskull commented Sep 29, 2020

boneskull commented Sep 29, 2020

rictic commented Sep 30, 2020

boneskull commented Oct 2, 2020

outsideris commented Oct 3, 2020 •

edited

boneskull commented Nov 4, 2020

JoshuaKGoldberg left a comment •

edited

JoshuaKGoldberg Mar 1, 2024

JoshuaKGoldberg Mar 1, 2024

JoshuaKGoldberg Mar 1, 2024

JoshuaKGoldberg commented Mar 2, 2024

feat: support running with TrustedTypes enforced #4447

Are you sure you want to change the base?

feat: support running with TrustedTypes enforced #4447

Conversation

rictic commented Sep 15, 2020

Description of the Change

Alternate Designs

Why should this be in core?

Benefits

Possible Drawbacks

More info and related work

jsf-clabot commented Sep 15, 2020 • edited

coveralls commented Sep 15, 2020 • edited

boneskull left a comment

Choose a reason for hiding this comment

boneskull Sep 25, 2020

Choose a reason for hiding this comment

rictic Sep 26, 2020

Choose a reason for hiding this comment

JoshuaKGoldberg Mar 1, 2024

Choose a reason for hiding this comment

boneskull commented Sep 25, 2020

rictic commented Sep 26, 2020 • edited

boneskull commented Sep 29, 2020

boneskull commented Sep 29, 2020

rictic commented Sep 30, 2020

boneskull commented Oct 2, 2020

outsideris commented Oct 3, 2020 • edited

boneskull commented Nov 4, 2020

JoshuaKGoldberg left a comment • edited

Choose a reason for hiding this comment

JoshuaKGoldberg Mar 1, 2024

Choose a reason for hiding this comment

JoshuaKGoldberg Mar 1, 2024

Choose a reason for hiding this comment

JoshuaKGoldberg Mar 1, 2024

Choose a reason for hiding this comment

JoshuaKGoldberg commented Mar 2, 2024

jsf-clabot commented Sep 15, 2020 •

edited

coveralls commented Sep 15, 2020 •

edited

rictic commented Sep 26, 2020 •

edited

outsideris commented Oct 3, 2020 •

edited

JoshuaKGoldberg left a comment •

edited