-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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
feat: support running with TrustedTypes enforced #4447
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -313,6 +313,24 @@ function error(msg) { | |
document.body.appendChild(fragment('<div id="mocha-error">%s</div>', msg)); | ||
} | ||
|
||
let policy = { | ||
createHTML: function(html) { | ||
/** | ||
* Note that this policy lets html through unchanged. This is potentially | ||
* a security vulnerability if untrusted data is set to innerHTML, as it | ||
* allows arbitrary code execution. | ||
* | ||
* Ideally this code would be refactored to not use .innerHTML, and this | ||
* policy deleted, or this policy could return the html after it has been | ||
* processed by a secure sanitization system like dompurify | ||
*/ | ||
return html; | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [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. |
||
}; | ||
if (typeof window !== 'undefined' && window.trustedTypes != null) { | ||
policy = window.trustedTypes.createPolicy('mocha-html-reporter', policy); | ||
} | ||
|
||
/** | ||
* Return a DOM fragment from `html`. | ||
* | ||
|
@@ -323,15 +341,17 @@ function fragment(html) { | |
var div = document.createElement('div'); | ||
var i = 1; | ||
|
||
div.innerHTML = html.replace(/%([se])/g, function(_, type) { | ||
switch (type) { | ||
case 's': | ||
return String(args[i++]); | ||
case 'e': | ||
return escape(args[i++]); | ||
// no default | ||
} | ||
}); | ||
div.innerHTML = policy.createHTML( | ||
html.replace(/%([se])/g, function(_, type) { | ||
switch (type) { | ||
case 's': | ||
return String(args[i++]); | ||
case 'e': | ||
return escape(args[i++]); | ||
// no default | ||
} | ||
}) | ||
); | ||
|
||
return div.firstChild; | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,6 +3,7 @@ import nodeResolve from '@rollup/plugin-node-resolve'; | |
import json from '@rollup/plugin-json'; | ||
import builtins from 'rollup-plugin-node-builtins'; | ||
import globals from 'rollup-plugin-node-globals'; | ||
import * as fs from 'fs'; | ||
|
||
import {babel} from '@rollup/plugin-babel'; | ||
|
||
|
@@ -11,6 +12,36 @@ import visualizer from 'rollup-plugin-visualizer'; | |
|
||
import pickFromPackageJson from './scripts/pick-from-package-json'; | ||
|
||
/** | ||
* A temporary plugin workaround for a globalThis polyfill. | ||
* | ||
* Older versions of regenerator-runtime use Function("return this")() to get | ||
* the global `this` value when running in strict mode. This is not compatible | ||
* with some content security policies, including trusted-types, which we | ||
* test with in browsers that support it. | ||
* | ||
* Fortunately, all browsers that support trusted-types also support the global | ||
* variable named `globalThis` for accessing the global `this` value. So | ||
* whenever we would run `Function("return this")()` we can instead first look | ||
* whether `globalThis` is defined, and if so, just use that. | ||
* | ||
* The latest version of regenerator-runtime does rely on calling Function | ||
* to get globalThis, so we only need this plugin until the updated version | ||
* has percolated through our dependency tree. We can try to remove it on | ||
* 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 = { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Seems like others would have this problem. Is there not an "official" fix anywhere? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. 😄 |
||
writeBundle(options) { | ||
let contents = fs.readFileSync(options.file, {encoding: 'utf8'}); | ||
contents = contents.replace( | ||
/Function\("return this"\)\(\)/g, | ||
`(typeof globalThis !== 'undefined' ? globalThis : Function("return this")())` | ||
); | ||
fs.writeFileSync(options.file, contents, {encoding: 'utf8'}); | ||
} | ||
}; | ||
|
||
const config = { | ||
input: './browser-entry.js', | ||
output: { | ||
|
@@ -47,7 +78,8 @@ const config = { | |
] | ||
], | ||
babelHelpers: 'bundled' | ||
}) | ||
}), | ||
applyTemporaryCspPatchPlugin | ||
], | ||
onwarn: (warning, warn) => { | ||
if (warning.code === 'CIRCULAR_DEPENDENCY') return; | ||
|
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.
[Style] Nit: no need for this wrapper, I think?