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.

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

Route Loader Trusted Types Violation Fix #34730

Merged
merged 2 commits into from May 3, 2022

Conversation

jgoping
Copy link
Contributor

@jgoping jgoping commented Feb 23, 2022

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.
  • 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 TrustedScriptURLs. 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.

@jgoping jgoping force-pushed the tt-violation-fix-route-loader branch from b31e0a5 to b8fc109 Compare February 25, 2022 15:49
@ijjk
Copy link
Member

ijjk commented Feb 25, 2022

Stats from current PR

Default Build (Increase detected ⚠️)
General Overall increase ⚠️
vercel/next.js canary jgoping/next.js tt-violation-fix-route-loader Change
buildDuration 19.4s 19.8s ⚠️ +460ms
buildDurationCached 7.8s 7.7s -52ms
nodeModulesSize 367 MB 368 MB ⚠️ +3.08 kB
Page Load Tests Overall decrease ⚠️
vercel/next.js canary jgoping/next.js tt-violation-fix-route-loader Change
/ failed reqs 0 0
/ total time (seconds) 3.686 3.713 ⚠️ +0.03
/ avg req/sec 678.28 673.26 ⚠️ -5.02
/error-in-render failed reqs 0 0
/error-in-render total time (seconds) 1.787 1.839 ⚠️ +0.05
/error-in-render avg req/sec 1399.25 1359.59 ⚠️ -39.66
Client Bundles (main, webpack) Overall increase ⚠️
vercel/next.js canary jgoping/next.js tt-violation-fix-route-loader Change
450.HASH.js gzip 179 B 179 B
framework-HASH.js gzip 42 kB 42 kB
main-HASH.js gzip 27.8 kB 28 kB ⚠️ +210 B
webpack-HASH.js gzip 1.44 kB 1.44 kB
Overall change 71.5 kB 71.7 kB ⚠️ +210 B
Legacy Client Bundles (polyfills)
vercel/next.js canary jgoping/next.js tt-violation-fix-route-loader Change
polyfills-HASH.js gzip 31 kB 31 kB
Overall change 31 kB 31 kB
Client Pages
vercel/next.js canary jgoping/next.js tt-violation-fix-route-loader Change
_app-HASH.js gzip 1.36 kB 1.36 kB
_error-HASH.js gzip 194 B 194 B
amp-HASH.js gzip 312 B 312 B
css-HASH.js gzip 326 B 326 B
dynamic-HASH.js gzip 2.57 kB 2.57 kB
head-HASH.js gzip 350 B 350 B
hooks-HASH.js gzip 919 B 919 B
image-HASH.js gzip 5.05 kB 5.05 kB
index-HASH.js gzip 263 B 263 B
link-HASH.js gzip 2.26 kB 2.26 kB
routerDirect..HASH.js gzip 321 B 321 B
script-HASH.js gzip 383 B 383 B
withRouter-HASH.js gzip 318 B 318 B
85e02e95b279..7e3.css gzip 107 B 107 B
Overall change 14.7 kB 14.7 kB
Client Build Manifests
vercel/next.js canary jgoping/next.js tt-violation-fix-route-loader Change
_buildManifest.js gzip 460 B 460 B
Overall change 460 B 460 B
Rendered Page Sizes Overall increase ⚠️
vercel/next.js canary jgoping/next.js tt-violation-fix-route-loader Change
index.html gzip 530 B 531 B ⚠️ +1 B
link.html gzip 544 B 544 B
withRouter.html gzip 526 B 526 B
Overall change 1.6 kB 1.6 kB ⚠️ +1 B

Diffs

Diff for main-HASH.js
@@ -2081,6 +2081,7 @@
       var _getAssetPathFromRoute = _interopRequireDefault(
         __webpack_require__(3891)
       );
+      var _trustedTypes = __webpack_require__(4991);
       var _requestIdleCallback = __webpack_require__(9311);
       function _interopRequireDefault(obj) {
         return obj && obj.__esModule
@@ -2250,6 +2251,7 @@
       }
       function getFilesForRoute(assetPrefix, route) {
         if (false) {
+          var scriptUrl;
         }
         return getClientBuildManifest().then(function(manifest) {
           if (!(route in manifest)) {
@@ -2261,9 +2263,13 @@
             return assetPrefix + "/_next/" + encodeURI(entry);
           });
           return {
-            scripts: allFiles.filter(function(v) {
-              return v.endsWith(".js");
-            }),
+            scripts: allFiles
+              .filter(function(v) {
+                return v.endsWith(".js");
+              })
+              .map(function(v) {
+                return (0, _trustedTypes).__unsafeCreateTrustedScriptURL(v);
+              }),
             css: allFiles.filter(function(v) {
               return v.endsWith(".css");
             })
@@ -2276,7 +2282,7 @@
           // disposed and readded. Executing scripts twice has no functional
           // differences
           if (true) {
-            var prom = loadedScripts.get(src);
+            var prom = loadedScripts.get(src.toString());
             if (prom) {
               return prom;
             }
@@ -2284,7 +2290,7 @@
             if (document.querySelector('script[src^="'.concat(src, '"]'))) {
               return Promise.resolve();
             }
-            loadedScripts.set(src, (prom = appendScript(src)));
+            loadedScripts.set(src.toString(), (prom = appendScript(src)));
             return prom;
           } else {
           }
@@ -2439,7 +2445,7 @@
                 return Promise.all(
                   canPrefetch
                     ? output.scripts.map(function(script) {
-                        return prefetchViaDom(script, "script");
+                        return prefetchViaDom(script.toString(), "script");
                       })
                     : []
                 );
@@ -3215,6 +3221,52 @@
       /***/
     },
 
+    /***/ 4991: /***/ function(__unused_webpack_module, exports) {
+      "use strict";
+
+      Object.defineProperty(exports, "__esModule", {
+        value: true
+      });
+      exports.__unsafeCreateTrustedScriptURL = __unsafeCreateTrustedScriptURL;
+      /**
+       * Stores the Trusted Types Policy. Starts as undefined and can be set to null
+       * if Trusted Types is not supported in the browser.
+       */ var policy;
+      /**
+       * 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" && "object" !== "undefined") {
+          var ref;
+          policy =
+            ((ref = window.trustedTypes) === null || ref === void 0
+              ? void 0
+              : ref.createPolicy("nextjs", {
+                  createHTML: function(input) {
+                    return input;
+                  },
+                  createScript: function(input) {
+                    return input;
+                  },
+                  createScriptURL: function(input) {
+                    return input;
+                  }
+                })) || null;
+        }
+        return policy;
+      }
+      function __unsafeCreateTrustedScriptURL(url) {
+        var ref;
+        return (
+          ((ref = getPolicy()) === null || ref === void 0
+            ? void 0
+            : ref.createScriptURL(url)) || url
+        );
+      } //# sourceMappingURL=trusted-types.js.map
+
+      /***/
+    },
+
     /***/ 8981: /***/ function(
       __unused_webpack_module,
       exports,
Diff for index.html
@@ -19,7 +19,7 @@
       defer=""
     ></script>
     <script
-      src="/_next/static/chunks/main-cc79f475267d4370.js"
+      src="/_next/static/chunks/main-8eb30633721bfb57.js"
       defer=""
     ></script>
     <script
Diff for link.html
@@ -19,7 +19,7 @@
       defer=""
     ></script>
     <script
-      src="/_next/static/chunks/main-cc79f475267d4370.js"
+      src="/_next/static/chunks/main-8eb30633721bfb57.js"
       defer=""
     ></script>
     <script
Diff for withRouter.html
@@ -19,7 +19,7 @@
       defer=""
     ></script>
     <script
-      src="/_next/static/chunks/main-cc79f475267d4370.js"
+      src="/_next/static/chunks/main-8eb30633721bfb57.js"
       defer=""
     ></script>
     <script

Default Build with SWC (Increase detected ⚠️)
General Overall increase ⚠️
vercel/next.js canary jgoping/next.js tt-violation-fix-route-loader Change
buildDuration 24.3s 24s -251ms
buildDurationCached 7.8s 7.4s -422ms
nodeModulesSize 367 MB 368 MB ⚠️ +3.08 kB
Page Load Tests Overall increase ✓
vercel/next.js canary jgoping/next.js tt-violation-fix-route-loader Change
/ failed reqs 0 0
/ total time (seconds) 3.878 3.795 -0.08
/ avg req/sec 644.59 658.69 +14.1
/error-in-render failed reqs 0 0
/error-in-render total time (seconds) 1.884 1.78 -0.1
/error-in-render avg req/sec 1326.95 1404.27 +77.32
Client Bundles (main, webpack) Overall increase ⚠️
vercel/next.js canary jgoping/next.js tt-violation-fix-route-loader Change
450.HASH.js gzip 179 B 179 B
framework-HASH.js gzip 42.1 kB 42.1 kB
main-HASH.js gzip 27.8 kB 28 kB ⚠️ +181 B
webpack-HASH.js gzip 1.44 kB 1.44 kB
Overall change 71.6 kB 71.8 kB ⚠️ +181 B
Legacy Client Bundles (polyfills)
vercel/next.js canary jgoping/next.js tt-violation-fix-route-loader Change
polyfills-HASH.js gzip 31 kB 31 kB
Overall change 31 kB 31 kB
Client Pages
vercel/next.js canary jgoping/next.js tt-violation-fix-route-loader Change
_app-HASH.js gzip 1.35 kB 1.35 kB
_error-HASH.js gzip 180 B 180 B
amp-HASH.js gzip 305 B 305 B
css-HASH.js gzip 321 B 321 B
dynamic-HASH.js gzip 2.56 kB 2.56 kB
head-HASH.js gzip 342 B 342 B
hooks-HASH.js gzip 911 B 911 B
image-HASH.js gzip 5.08 kB 5.08 kB
index-HASH.js gzip 256 B 256 B
link-HASH.js gzip 2.28 kB 2.28 kB
routerDirect..HASH.js gzip 314 B 314 B
script-HASH.js gzip 375 B 375 B
withRouter-HASH.js gzip 309 B 309 B
85e02e95b279..7e3.css gzip 107 B 107 B
Overall change 14.7 kB 14.7 kB
Client Build Manifests
vercel/next.js canary jgoping/next.js tt-violation-fix-route-loader Change
_buildManifest.js gzip 459 B 459 B
Overall change 459 B 459 B
Rendered Page Sizes Overall increase ⚠️
vercel/next.js canary jgoping/next.js tt-violation-fix-route-loader Change
index.html gzip 533 B 534 B ⚠️ +1 B
link.html gzip 547 B 548 B ⚠️ +1 B
withRouter.html gzip 527 B 528 B ⚠️ +1 B
Overall change 1.61 kB 1.61 kB ⚠️ +3 B

Diffs

Diff for main-HASH.js
@@ -2081,6 +2081,7 @@
       var _getAssetPathFromRoute = _interopRequireDefault(
         __webpack_require__(3891)
       );
+      var _trustedTypes = __webpack_require__(4991);
       var _requestIdleCallback = __webpack_require__(9311);
       function _interopRequireDefault(obj) {
         return obj && obj.__esModule
@@ -2250,6 +2251,7 @@
       }
       function getFilesForRoute(assetPrefix, route) {
         if (false) {
+          var scriptUrl;
         }
         return getClientBuildManifest().then(function(manifest) {
           if (!(route in manifest)) {
@@ -2261,9 +2263,13 @@
             return assetPrefix + "/_next/" + encodeURI(entry);
           });
           return {
-            scripts: allFiles.filter(function(v) {
-              return v.endsWith(".js");
-            }),
+            scripts: allFiles
+              .filter(function(v) {
+                return v.endsWith(".js");
+              })
+              .map(function(v) {
+                return (0, _trustedTypes).__unsafeCreateTrustedScriptURL(v);
+              }),
             css: allFiles.filter(function(v) {
               return v.endsWith(".css");
             })
@@ -2276,7 +2282,7 @@
           // disposed and readded. Executing scripts twice has no functional
           // differences
           if (true) {
-            var prom = loadedScripts.get(src);
+            var prom = loadedScripts.get(src.toString());
             if (prom) {
               return prom;
             }
@@ -2284,7 +2290,7 @@
             if (document.querySelector('script[src^="'.concat(src, '"]'))) {
               return Promise.resolve();
             }
-            loadedScripts.set(src, (prom = appendScript(src)));
+            loadedScripts.set(src.toString(), (prom = appendScript(src)));
             return prom;
           } else {
           }
@@ -2439,7 +2445,7 @@
                 return Promise.all(
                   canPrefetch
                     ? output.scripts.map(function(script) {
-                        return prefetchViaDom(script, "script");
+                        return prefetchViaDom(script.toString(), "script");
                       })
                     : []
                 );
@@ -3215,6 +3221,52 @@
       /***/
     },
 
+    /***/ 4991: /***/ function(__unused_webpack_module, exports) {
+      "use strict";
+
+      Object.defineProperty(exports, "__esModule", {
+        value: true
+      });
+      exports.__unsafeCreateTrustedScriptURL = __unsafeCreateTrustedScriptURL;
+      /**
+       * Stores the Trusted Types Policy. Starts as undefined and can be set to null
+       * if Trusted Types is not supported in the browser.
+       */ var policy;
+      /**
+       * 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" && "object" !== "undefined") {
+          var ref;
+          policy =
+            ((ref = window.trustedTypes) === null || ref === void 0
+              ? void 0
+              : ref.createPolicy("nextjs", {
+                  createHTML: function(input) {
+                    return input;
+                  },
+                  createScript: function(input) {
+                    return input;
+                  },
+                  createScriptURL: function(input) {
+                    return input;
+                  }
+                })) || null;
+        }
+        return policy;
+      }
+      function __unsafeCreateTrustedScriptURL(url) {
+        var ref;
+        return (
+          ((ref = getPolicy()) === null || ref === void 0
+            ? void 0
+            : ref.createScriptURL(url)) || url
+        );
+      } //# sourceMappingURL=trusted-types.js.map
+
+      /***/
+    },
+
     /***/ 8981: /***/ function(
       __unused_webpack_module,
       exports,
Diff for index.html
@@ -19,7 +19,7 @@
       defer=""
     ></script>
     <script
-      src="/_next/static/chunks/main-cc79f475267d4370.js"
+      src="/_next/static/chunks/main-8eb30633721bfb57.js"
       defer=""
     ></script>
     <script
Diff for link.html
@@ -19,7 +19,7 @@
       defer=""
     ></script>
     <script
-      src="/_next/static/chunks/main-cc79f475267d4370.js"
+      src="/_next/static/chunks/main-8eb30633721bfb57.js"
       defer=""
     ></script>
     <script
Diff for withRouter.html
@@ -19,7 +19,7 @@
       defer=""
     ></script>
     <script
-      src="/_next/static/chunks/main-cc79f475267d4370.js"
+      src="/_next/static/chunks/main-8eb30633721bfb57.js"
       defer=""
     ></script>
     <script
Commit: b8fc109

@jgoping
Copy link
Contributor Author

jgoping commented Mar 8, 2022

@ijjk Hi there! I noticed that you reviewed my previous PR related to Trusted Types. Do you think you could give this PR a review as well? If you have any questions feel free to reach out to me, thank you!

@kodiakhq kodiakhq bot merged commit 44c89e5 into vercel:canary May 3, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 3, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants