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.

Sign up for GitHub

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

Jump to bottom

Add Trusted Types policy #13509

Closed
wants to merge 1 commit into from
Closed

Add Trusted Types policy #13509

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 16 additions & 1 deletion packages/next/client/page-loader.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,19 @@ import { getRouteMatcher } from './../next-server/lib/router/utils/route-matcher
import { getRouteRegex } from './../next-server/lib/router/utils/route-regex'
import { delBasePath } from './../next-server/lib/router/router'

let trustedTypesPolicy = undefined
if (window?.trustedTypes?.createPolicy) {
trustedTypesPolicy = window.trustedTypes.createPolicy('next-trusted-types', {
// Needs security review regarding DOM XSS
createHTML(dirty) {
Copy link
Contributor

Choose a reason for hiding this comment

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

is this even needed for this particular case

Copy link

Choose a reason for hiding this comment

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

Looks like it isn't used, so the createHTML method here can be dropped.

return dirty.replace(/</g, '&lt;') // Escapes `<` characters to prevent the creation of new HTML elements
},
createScriptURL(dirty) {
Copy link

Choose a reason for hiding this comment

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

So, the attack you're guarding against with createScriptUrl is an untrusted/malicious URL, which can execute arbitrary code (e.g. by loading a script from a hostile origin that exfiltrates user data, mines bitcoins, etc)

It looks like you construct the URLs in two places, at line 263 and line 103. If you are confident that the data used to construct those URLs can't come from untrusted sources, then you could have a createScriptURL function that just returns its argument, and call policy.createScriptUrl there (with comments about your assumptions).

Then, because policy isn't exported, you and your users can reduce the amount of code that needs to be subject to careful security review to just those where the policy is used in this file.

return new URL(dirty.replace(/</g, '&lt;'), document.baseURI)
},
})
}

function hasRel(rel, link) {
try {
link = document.createElement('link')
Expand Down Expand Up @@ -262,7 +275,9 @@ export default class PageLoader {
if (isPage) url = url.replace(/\.js$/, '.module.js')
}
script.crossOrigin = process.crossOrigin
script.src = url
script.src = trustedTypesPolicy
? trustedTypesPolicy.createScriptURL(url)
: url
script.onerror = () => {
const error = new Error(`Error loading script ${url}`)
error.code = 'PAGE_LOAD_ERROR'
Expand Down