From 44c89e50c874c7f2b889df15d1679f85670fd9f9 Mon Sep 17 00:00:00 2001 From: Justin Goping <32006038+jgoping@users.noreply.github.com> Date: Tue, 3 May 2022 19:22:08 -0400 Subject: [PATCH] Route Loader Trusted Types Violation Fix (#34730) Linked to issue #32209. ## Feature - [ ] Implements an existing feature request or RFC. Make sure the feature request has been accepted for implementation before opening a PR. - [x] Related issues linked using `fixes #number` - [ ] Integration tests added - [ ] Documentation added - [ ] Telemetry added. In case of a feature if it's used or not. - [ ] Errors have helpful link attached, see `contributing.md` ## Documentation There is one tsec violation that is fixed in this PR: ### 1. ban-script-src-assignment: route-loader.ts XSS can occur with the line script.src = src in appendScript(src, script) if src can be controlled by a malicious user. From tracing through the code, it was determined that src comes from the function `getFilesForRoute(route)`. The behaviour of this function differs depending on the environment (development vs. production), but in both cases the function will construct strings that lead to valid file paths. These strings depend on two variables: `assetPrefix` and `route`, but due to the nature of the constructed strings it was determined that the scripts here are safe to use. Thus, the solution was to promote these strings to `TrustedScriptURL`s. This is the Trusted Types way of declaring that the script URL passed to the DOM sink is safe from DOM XSS attacks. To create a `TrustedScriptURL`, a policy needs to be created. This policy was put in its own file: `client/trusted-types.ts`. This policy has the name `nextjs`. If this name should be changed to something else, feel free to change it now. However, once it is released to the public and application developers begin using it, it may be harder to change the value since any application developers with a custom policy name allowlist would now need to update their `next.config.js` headers to allow this new name. The code was tested in a sample application to ensure it behaved as expected. --- package.json | 1 + packages/next/client/route-loader.ts | 33 ++++++++++++++---------- packages/next/client/trusted-types.ts | 37 +++++++++++++++++++++++++++ tsec-exemptions.json | 6 ++--- yarn.lock | 5 ++++ 5 files changed, 65 insertions(+), 17 deletions(-) create mode 100644 packages/next/client/trusted-types.ts diff --git a/package.json b/package.json index a364f9fa38b3..af8fd7315dd9 100644 --- a/package.json +++ b/package.json @@ -64,6 +64,7 @@ "@types/selenium-webdriver": "4.0.15", "@types/sharp": "0.29.3", "@types/string-hash": "1.1.1", + "@types/trusted-types": "2.0.2", "@typescript-eslint/eslint-plugin": "4.29.1", "@typescript-eslint/parser": "4.29.1", "@vercel/fetch": "6.1.1", diff --git a/packages/next/client/route-loader.ts b/packages/next/client/route-loader.ts index d1198fc2a024..56d0a0200ba8 100644 --- a/packages/next/client/route-loader.ts +++ b/packages/next/client/route-loader.ts @@ -1,5 +1,6 @@ import type { ComponentType } from 'react' import getAssetPathFromRoute from '../shared/lib/router/utils/get-asset-path-from-route' +import { __unsafeCreateTrustedScriptURL } from './trusted-types' import { requestIdleCallback } from './request-idle-callback' // 3.8s was arbitrarily chosen as it's what https://web.dev/interactive @@ -135,7 +136,7 @@ export function isAssetError(err?: Error): boolean | undefined { } function appendScript( - src: string, + src: TrustedScriptURL | string, script?: HTMLScriptElement ): Promise { return new Promise((resolve, reject) => { @@ -154,7 +155,7 @@ function appendScript( // 3. Finally, set the source and inject into the DOM in case the child // must be appended for fetching to start. - script.src = src + script.src = src as string document.body.appendChild(script) }) } @@ -254,7 +255,7 @@ export function getMiddlewareManifest() { } interface RouteFiles { - scripts: string[] + scripts: (TrustedScriptURL | string)[] css: string[] } function getFilesForRoute( @@ -262,12 +263,12 @@ function getFilesForRoute( route: string ): Promise { if (process.env.NODE_ENV === 'development') { + const scriptUrl = + assetPrefix + + '/_next/static/chunks/pages' + + encodeURI(getAssetPathFromRoute(route, '.js')) return Promise.resolve({ - scripts: [ - assetPrefix + - '/_next/static/chunks/pages' + - encodeURI(getAssetPathFromRoute(route, '.js')), - ], + scripts: [__unsafeCreateTrustedScriptURL(scriptUrl)], // Styles are handled by `style-loader` in development: css: [], }) @@ -280,7 +281,9 @@ function getFilesForRoute( (entry) => assetPrefix + '/_next/' + encodeURI(entry) ) return { - scripts: allFiles.filter((v) => v.endsWith('.js')), + scripts: allFiles + .filter((v) => v.endsWith('.js')) + .map((v) => __unsafeCreateTrustedScriptURL(v)), css: allFiles.filter((v) => v.endsWith('.css')), } }) @@ -294,12 +297,14 @@ export function createRouteLoader(assetPrefix: string): RouteLoader { const routes: Map | RouteLoaderEntry> = new Map() - function maybeExecuteScript(src: string): Promise { + function maybeExecuteScript( + src: TrustedScriptURL | string + ): Promise { // With HMR we might need to "reload" scripts when they are // disposed and readded. Executing scripts twice has no functional // differences if (process.env.NODE_ENV !== 'development') { - let prom: Promise | undefined = loadedScripts.get(src) + let prom: Promise | undefined = loadedScripts.get(src.toString()) if (prom) { return prom } @@ -309,7 +314,7 @@ export function createRouteLoader(assetPrefix: string): RouteLoader { return Promise.resolve() } - loadedScripts.set(src, (prom = appendScript(src))) + loadedScripts.set(src.toString(), (prom = appendScript(src))) return prom } else { return appendScript(src) @@ -432,7 +437,9 @@ export function createRouteLoader(assetPrefix: string): RouteLoader { .then((output) => Promise.all( canPrefetch - ? output.scripts.map((script) => prefetchViaDom(script, 'script')) + ? output.scripts.map((script) => + prefetchViaDom(script.toString(), 'script') + ) : [] ) ) diff --git a/packages/next/client/trusted-types.ts b/packages/next/client/trusted-types.ts new file mode 100644 index 000000000000..9682635eb945 --- /dev/null +++ b/packages/next/client/trusted-types.ts @@ -0,0 +1,37 @@ +/** + * Stores the Trusted Types Policy. Starts as undefined and can be set to null + * if Trusted Types is not supported in the browser. + */ +let policy: TrustedTypePolicy | null | undefined + +/** + * Getter for the Trusted Types Policy. If it is undefined, it is instantiated + * here or set to null if Trusted Types is not supported in the browser. + */ +function getPolicy() { + if (typeof policy === 'undefined' && typeof window !== 'undefined') { + policy = + window.trustedTypes?.createPolicy('nextjs', { + createHTML: (input) => input, + createScript: (input) => input, + createScriptURL: (input) => input, + }) || null + } + + return policy +} + +/** + * Unsafely promote a string to a TrustedScriptURL, falling back to strings + * when Trusted Types are not available. + * This is a security-sensitive function; any use of this function + * must go through security review. In particular, it must be assured that the + * provided string will never cause an XSS vulnerability if used in a context + * that will cause a browser to load and execute a resource, e.g. when + * assigning to script.src. + */ +export function __unsafeCreateTrustedScriptURL( + url: string +): TrustedScriptURL | string { + return getPolicy()?.createScriptURL(url) || url +} diff --git a/tsec-exemptions.json b/tsec-exemptions.json index ec33c3b673ea..acbcecdaa91c 100644 --- a/tsec-exemptions.json +++ b/tsec-exemptions.json @@ -9,10 +9,8 @@ "packages/next/client/script.tsx" ], "ban-script-content-assignments": ["packages/next/client/script.tsx"], - "ban-script-src-assignments": [ - "packages/next/client/route-loader.ts", - "packages/next/client/script.tsx" - ], + "ban-script-src-assignments": ["packages/next/client/script.tsx"], + "ban-trustedtypes-createpolicy": ["packages/next/client/trusted-types.ts"], "ban-window-stringfunctiondef": [ "packages/next/lib/recursive-delete.ts", "packages/next/client/dev/fouc.ts" diff --git a/yarn.lock b/yarn.lock index f3c7da9ae708..3cae0cb367a2 100644 --- a/yarn.lock +++ b/yarn.lock @@ -5407,6 +5407,11 @@ dependencies: "@types/node" "*" +"@types/trusted-types@2.0.2": + version "2.0.2" + resolved "https://registry.yarnpkg.com/@types/trusted-types/-/trusted-types-2.0.2.tgz#fc25ad9943bcac11cceb8168db4f275e0e72e756" + integrity sha512-F5DIZ36YVLE+PN+Zwws4kJogq47hNgX3Nx6WyDJ3kcplxyke3XIzB8uK5n/Lpm1HBsbGzd6nmGehL8cPekP+Tg== + "@types/ua-parser-js@0.7.36": version "0.7.36" resolved "https://registry.yarnpkg.com/@types/ua-parser-js/-/ua-parser-js-0.7.36.tgz#9bd0b47f26b5a3151be21ba4ce9f5fa457c5f190"