correctly assess node equality when nonce attribute is present #27573

ericbiewener · 2021-07-29T00:08:51Z

When a nonce is present on an element, browsers such as Chrome and Firefox strip it out of the actual HTML attributes for security reasons when the element is added to the document. Thus, given two equivalent elements that have nonces, Element,isEqualNode() will return false if one of those elements gets added to the document. Although the element.nonce property will be the same for both elements, the one that was added to the document will return an empty string for its nonce HTML attribute value. This custom isEqualNode() function therefore removes the nonce value from the newTag before comparing it to oldTag, restoring it afterwards.

For more information, see: https://bugs.chromium.org/p/chromium/issues/detail?id=1211471#c12

Bug

Related issues linked using fixes #number
Integration tests added
Errors have helpful link attached, see contributing.md

Documentation / Examples

Make sure the linting passes

ericbiewener · 2021-07-29T19:29:57Z

I need some help getting an integration test to work. Since I need the ability to set a CSP header, that means I need to use a custom server in the test. I've got two approaches in this PR, but neither is working. In both cases, I get error output that looks like this:

test/integration/head-manager/server.js

test/integration/head-manager/start-server.js

test/integration/image-component/basic/test/index.test.js

test/unit/is-equal-node.unit.test.js

Co-authored-by: Steven <[email protected]>

This reverts commit d67b997.

ijjk · 2021-11-10T19:07:23Z

Failing test suites

Commit: 0109113

test/integration/head-manager/test/nonce.test.js

nonce > Re-rendering should not re-execute the script

Expand output

● nonce › Re-rendering should not re-execute the script

expect(received).toEqual(expected) // deep equality

Expected: ["src-1.js"]
Received: undefined

  31 | describe('nonce', () => {
  32 |   afterAll(async () => {
> 33 |     if (ctx.browser) await ctx.browser.close()
     |                                               ^
  34 |     killApp(ctx.server)
  35 |   })
  36 |

  at Object.<anonymous> (integration/head-manager/test/nonce.test.js:33:69)

● Test suite failed to run

TypeError: Cannot read property 'pid' of undefined

  398 |   return new Promise((resolve, reject) => {
  399 |     const newArgs = [
> 400 |       ...args,
      |               ^
  401 |       function (err, res) {
  402 |         if (err) return reject(err)
  403 |         resolve(res)

  at Object.killApp (lib/next-test-utils.js:400:32)
  at integration/head-manager/test/nonce.test.js:27:29

test/integration/image-component/basic/test/index.test.js

Image Component Tests > Client-side Image Component Tests > should render an image tag
Image Component Tests > SSR Image Component Tests > should render an image tag

Expand output

● Image Component Tests › SSR Image Component Tests › should render an image tag

thrown: "Exceeded timeout of 90000 ms for a test.
Use jest.setTimeout(newTimeout) to increase the timeout value, if this is a long-running test."

  14 | const appDir = join(__dirname, '../')
  15 | let appPort
> 16 | let app
     |     ^
  17 | let browser
  18 | const emptyImage =
  19 |   'data:image/gif;base64,R0lGODlhAQABAIAAAAAAAP///yH5BAEAAAAALAAAAAABAAEAAAIBRAA7'

  at runTests (integration/image-component/basic/test/index.test.js:16:5)
  at integration/image-component/basic/test/index.test.js:167:9
  at integration/image-component/basic/test/index.test.js:160:5
  at Object.<anonymous> (integration/image-component/basic/test/index.test.js:152:1)

● Image Component Tests › Client-side Image Component Tests › should render an image tag

thrown: "Exceeded timeout of 90000 ms for a test.
Use jest.setTimeout(newTimeout) to increase the timeout value, if this is a long-running test."

  14 | const appDir = join(__dirname, '../')
  15 | let appPort
> 16 | let app
     |     ^
  17 | let browser
  18 | const emptyImage =
  19 |   'data:image/gif;base64,R0lGODlhAQABAIAAAAAAAP///yH5BAEAAAAALAAAAAABAAEAAAIBRAA7'

  at runTests (integration/image-component/basic/test/index.test.js:16:5)
  at integration/image-component/basic/test/index.test.js:196:9
  at integration/image-component/basic/test/index.test.js:188:5
  at Object.<anonymous> (integration/image-component/basic/test/index.test.js:152:1)

styfle

Thanks for this PR! 🎉

I updated the tests and I think it captures the original intent 👍

ericbiewener · 2021-11-10T22:51:02Z

@styfle My team patched this in our version of Next and have since updated the isEqualNode function to be:

function isEqualNode(oldTag, newTag) {
    if (oldTag instanceof HTMLElement && newTag instanceof HTMLElement) {
      const nonce = newTag.getAttribute('nonce')
      // Only strip the nonce if `oldTag` has had it stripped. An element's nonce attribute will not
      // be stripped if there is no content security policy response header that includes a nonce.
      if (nonce && !oldTag.getAttribute('nonce')) {
        const newTagSansNonce = newTag.cloneNode(true);
        newTagSansNonce.setAttribute('nonce', '');
        newTagSansNonce.nonce = nonce;
        return nonce === oldTag.nonce && oldTag.isEqualNode(newTagSansNonce);
      }
    }
  
    return oldTag.isEqualNode(newTag)
}

The main difference here is just the cloning of newTag before mutating it in order to perform the final comparison. Perhaps this doesn't matter, but it feels a bit safer. Happy to push the commit (or feel free to do so yourself) if you want this change.

styfle · 2021-11-10T23:09:39Z

That will be a bit slower but should be much safer. I updated the PR, thanks 👍

ijjk · 2021-11-10T23:25:04Z

Stats from current PR

Default Build (Increase detected ⚠️)

General Overall increase ⚠️

	vercel/next.js canary	ericbiewener/next.js head-manager-is-equal-node-nonce	Change
buildDuration	16.9s	17.2s	⚠️ +255ms
buildDurationCached	3.4s	3.4s	-3ms
nodeModulesSize	334 MB	334 MB	⚠️ +2.24 kB

Page Load Tests Overall increase ✓

	vercel/next.js canary	ericbiewener/next.js head-manager-is-equal-node-nonce	Change
/ failed reqs	0	0	✓
/ total time (seconds)	2.743	2.695	-0.05
/ avg req/sec	911.42	927.7	+16.28
/error-in-render failed reqs	0	0	✓
/error-in-render total time (seconds)	1.346	1.279	-0.07
/error-in-render avg req/sec	1857.51	1954.92	+97.41

Client Bundles (main, webpack, commons) Overall increase ⚠️

	vercel/next.js canary	ericbiewener/next.js head-manager-is-equal-node-nonce	Change
450.HASH.js gzip	179 B	179 B	✓
framework-HASH.js gzip	42.2 kB	42.2 kB	✓
main-HASH.js gzip	28.2 kB	28.3 kB	⚠️ +103 B
webpack-HASH.js gzip	1.45 kB	1.45 kB	✓
Overall change	72.1 kB	72.2 kB	⚠️ +103 B

Legacy Client Bundles (polyfills)

	vercel/next.js canary	ericbiewener/next.js head-manager-is-equal-node-nonce	Change
polyfills-HASH.js gzip	31 kB	31 kB	✓
Overall change	31 kB	31 kB	✓

Client Pages

	vercel/next.js canary	ericbiewener/next.js head-manager-is-equal-node-nonce	Change
_app-HASH.js gzip	1.37 kB	1.37 kB	✓
_error-HASH.js gzip	194 B	194 B	✓
amp-HASH.js gzip	312 B	312 B	✓
css-HASH.js gzip	327 B	327 B	✓
dynamic-HASH.js gzip	2.38 kB	2.38 kB	✓
head-HASH.js gzip	350 B	350 B	✓
hooks-HASH.js gzip	635 B	635 B	✓
image-HASH.js gzip	4.44 kB	4.44 kB	✓
index-HASH.js gzip	263 B	263 B	✓
link-HASH.js gzip	1.87 kB	1.87 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	13.3 kB	13.3 kB	✓

Client Build Manifests

	vercel/next.js canary	ericbiewener/next.js head-manager-is-equal-node-nonce	Change
_buildManifest.js gzip	460 B	460 B	✓
Overall change	460 B	460 B	✓

Rendered Page Sizes Overall increase ⚠️

	vercel/next.js canary	ericbiewener/next.js head-manager-is-equal-node-nonce	Change
index.html gzip	522 B	523 B	⚠️ +1 B
link.html gzip	535 B	535 B	✓
withRouter.html gzip	516 B	517 B	⚠️ +1 B
Overall change	1.57 kB	1.57 kB	⚠️ +2 B

Diffs

Diff for main-HASH.js

@@ -60,9 +60,21 @@
     /***/ 6007: /***/ function(__unused_webpack_module, exports) {
       "use strict";
 
+      function _instanceof(left, right) {
+        if (
+          right != null &&
+          typeof Symbol !== "undefined" &&
+          right[Symbol.hasInstance]
+        ) {
+          return right[Symbol.hasInstance](left);
+        } else {
+          return left instanceof right;
+        }
+      }
       Object.defineProperty(exports, "__esModule", {
         value: true
       });
+      exports.isEqualNode = isEqualNode;
       exports["default"] = initHeadManager;
       exports.DOMAttributeNames = void 0;
       var DOMAttributeNames = {
@@ -106,6 +118,23 @@
         }
         return el;
       }
+      function isEqualNode(oldTag, newTag) {
+        if (
+          _instanceof(oldTag, HTMLElement) &&
+          _instanceof(newTag, HTMLElement)
+        ) {
+          var nonce = newTag.getAttribute("nonce");
+          // Only strip the nonce if `oldTag` has had it stripped. An element's nonce attribute will not
+          // be stripped if there is no content security policy response header that includes a nonce.
+          if (nonce && !oldTag.getAttribute("nonce")) {
+            var cloneTag = newTag.cloneNode(true);
+            cloneTag.setAttribute("nonce", "");
+            cloneTag.nonce = nonce;
+            return nonce === oldTag.nonce && oldTag.isEqualNode(cloneTag);
+          }
+        }
+        return oldTag.isEqualNode(newTag);
+      }
       function updateElements(type, components) {
         var headEl = document.getElementsByTagName("head")[0];
         var headCountEl = headEl.querySelector("meta[name=next-head-count]");
@@ -138,7 +167,7 @@
           .filter(function(newTag) {
             for (var k = 0, len = oldTags.length; k < len; k++) {
               var oldTag = oldTags[k];
-              if (oldTag.isEqualNode(newTag)) {
+              if (isEqualNode(oldTag, newTag)) {
                 oldTags.splice(k, 1);
                 return false;
               }

Diff for index.html

@@ -19,7 +19,7 @@
       defer=""
     ></script>
     <script
-      src="/_next/static/chunks/main-6b0f14463d84c5dc.js"
+      src="/_next/static/chunks/main-0e133286d9b91502.js"
       defer=""
     ></script>
     <script

Diff for link.html

@@ -19,7 +19,7 @@
       defer=""
     ></script>
     <script
-      src="/_next/static/chunks/main-6b0f14463d84c5dc.js"
+      src="/_next/static/chunks/main-0e133286d9b91502.js"
       defer=""
     ></script>
     <script

Diff for withRouter.html

@@ -19,7 +19,7 @@
       defer=""
     ></script>
     <script
-      src="/_next/static/chunks/main-6b0f14463d84c5dc.js"
+      src="/_next/static/chunks/main-0e133286d9b91502.js"
       defer=""
     ></script>
     <script

Default Build with SWC (Increase detected ⚠️)

General Overall increase ⚠️

	vercel/next.js canary	ericbiewener/next.js head-manager-is-equal-node-nonce	Change
buildDuration	17.8s	18.2s	⚠️ +443ms
buildDurationCached	3.5s	3.5s	-24ms
nodeModulesSize	334 MB	334 MB	⚠️ +2.24 kB

Page Load Tests Overall decrease ⚠️

	vercel/next.js canary	ericbiewener/next.js head-manager-is-equal-node-nonce	Change
/ failed reqs	0	0	✓
/ total time (seconds)	2.684	2.728	⚠️ +0.04
/ avg req/sec	931.35	916.4	⚠️ -14.95
/error-in-render failed reqs	0	0	✓
/error-in-render total time (seconds)	1.294	1.308	⚠️ +0.01
/error-in-render avg req/sec	1931.27	1911.59	⚠️ -19.68

Client Bundles (main, webpack, commons) Overall increase ⚠️

	vercel/next.js canary	ericbiewener/next.js head-manager-is-equal-node-nonce	Change
450.HASH.js gzip	179 B	179 B	✓
framework-HASH.js gzip	42.3 kB	42.3 kB	✓
main-HASH.js gzip	28.4 kB	28.5 kB	⚠️ +107 B
webpack-HASH.js gzip	1.43 kB	1.43 kB	✓
Overall change	72.4 kB	72.5 kB	⚠️ +107 B

Legacy Client Bundles (polyfills)

	vercel/next.js canary	ericbiewener/next.js head-manager-is-equal-node-nonce	Change
polyfills-HASH.js gzip	31 kB	31 kB	✓
Overall change	31 kB	31 kB	✓

Client Pages

	vercel/next.js canary	ericbiewener/next.js head-manager-is-equal-node-nonce	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.38 kB	2.38 kB	✓
head-HASH.js gzip	342 B	342 B	✓
hooks-HASH.js gzip	622 B	622 B	✓
image-HASH.js gzip	4.46 kB	4.46 kB	✓
index-HASH.js gzip	256 B	256 B	✓
link-HASH.js gzip	1.91 kB	1.91 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	13.2 kB	13.2 kB	✓

Client Build Manifests

	vercel/next.js canary	ericbiewener/next.js head-manager-is-equal-node-nonce	Change
_buildManifest.js gzip	459 B	459 B	✓
Overall change	459 B	459 B	✓

Rendered Page Sizes Overall increase ⚠️

	vercel/next.js canary	ericbiewener/next.js head-manager-is-equal-node-nonce	Change
index.html gzip	522 B	522 B	✓
link.html gzip	534 B	534 B	✓
withRouter.html gzip	515 B	516 B	⚠️ +1 B
Overall change	1.57 kB	1.57 kB	⚠️ +1 B

Diffs

Diff for main-HASH.js

@@ -60,9 +60,21 @@
     /***/ 6007: /***/ function(__unused_webpack_module, exports) {
       "use strict";
 
+      function _instanceof(left, right) {
+        if (
+          right != null &&
+          typeof Symbol !== "undefined" &&
+          right[Symbol.hasInstance]
+        ) {
+          return right[Symbol.hasInstance](left);
+        } else {
+          return left instanceof right;
+        }
+      }
       Object.defineProperty(exports, "__esModule", {
         value: true
       });
+      exports.isEqualNode = isEqualNode;
       exports["default"] = initHeadManager;
       exports.DOMAttributeNames = void 0;
       var DOMAttributeNames = {
@@ -106,6 +118,23 @@
         }
         return el;
       }
+      function isEqualNode(oldTag, newTag) {
+        if (
+          _instanceof(oldTag, HTMLElement) &&
+          _instanceof(newTag, HTMLElement)
+        ) {
+          var nonce = newTag.getAttribute("nonce");
+          // Only strip the nonce if `oldTag` has had it stripped. An element's nonce attribute will not
+          // be stripped if there is no content security policy response header that includes a nonce.
+          if (nonce && !oldTag.getAttribute("nonce")) {
+            var cloneTag = newTag.cloneNode(true);
+            cloneTag.setAttribute("nonce", "");
+            cloneTag.nonce = nonce;
+            return nonce === oldTag.nonce && oldTag.isEqualNode(cloneTag);
+          }
+        }
+        return oldTag.isEqualNode(newTag);
+      }
       function updateElements(type, components) {
         var headEl = document.getElementsByTagName("head")[0];
         var headCountEl = headEl.querySelector("meta[name=next-head-count]");
@@ -138,7 +167,7 @@
           .filter(function(newTag) {
             for (var k = 0, len = oldTags.length; k < len; k++) {
               var oldTag = oldTags[k];
-              if (oldTag.isEqualNode(newTag)) {
+              if (isEqualNode(oldTag, newTag)) {
                 oldTags.splice(k, 1);
                 return false;
               }

Diff for index.html

@@ -19,7 +19,7 @@
       defer=""
     ></script>
     <script
-      src="/_next/static/chunks/main-6b0f14463d84c5dc.js"
+      src="/_next/static/chunks/main-0e133286d9b91502.js"
       defer=""
     ></script>
     <script

Diff for link.html

@@ -19,7 +19,7 @@
       defer=""
     ></script>
     <script
-      src="/_next/static/chunks/main-6b0f14463d84c5dc.js"
+      src="/_next/static/chunks/main-0e133286d9b91502.js"
       defer=""
     ></script>
     <script

Diff for withRouter.html

@@ -19,7 +19,7 @@
       defer=""
     ></script>
     <script
-      src="/_next/static/chunks/main-6b0f14463d84c5dc.js"
+      src="/_next/static/chunks/main-0e133286d9b91502.js"
       defer=""
     ></script>
     <script

Commit: 5c9bfc4

Eric Biewener added 2 commits July 28, 2021 15:20

add isEqualNode function

c7e90b5

add test

4c6d11c

ijjk added the type: next label Jul 29, 2021

ericbiewener and others added 2 commits July 28, 2021 17:10

Merge branch 'canary' into head-manager-is-equal-node-nonce

797026f

trying to make integration test work

8ee497b

ericbiewener marked this pull request as ready for review July 29, 2021 19:28

ericbiewener requested review from huozhi, ijjk, padmaia, shuding, styfle and timneutkens as code owners July 29, 2021 19:28

ericbiewener commented Jul 29, 2021

View reviewed changes

test/integration/head-manager/server.js Outdated Show resolved Hide resolved

ericbiewener commented Jul 29, 2021

View reviewed changes

test/integration/head-manager/start-server.js Outdated Show resolved Hide resolved

styfle reviewed Aug 27, 2021

View reviewed changes

test/integration/image-component/basic/test/index.test.js Outdated Show resolved Hide resolved

styfle reviewed Aug 27, 2021

View reviewed changes

test/unit/is-equal-node.unit.test.js Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

Eric Biewener and others added 4 commits August 27, 2021 16:00

revert

d67b997

Update test/unit/is-equal-node.unit.test.js

2cde708

Co-authored-by: Steven <[email protected]>

Revert "revert"

190a652

This reverts commit d67b997.

Merge branch 'canary' into head-manager-is-equal-node-nonce

0109113

This comment has been minimized.

Sign in to view

styfle added 4 commits November 10, 2021 16:48

Fix tests

d8826b6

Use TS for unit test

33adad0

Revert waitfor

6448044

Start tests with "should"

fdfb921

styfle previously approved these changes Nov 10, 2021

View reviewed changes

ijjk previously approved these changes Nov 10, 2021

View reviewed changes

Merge branch 'canary' into head-manager-is-equal-node-nonce

3effe08

This comment has been minimized.

Sign in to view

Fix lint

c64f39f

styfle dismissed stale reviews from ijjk and themself via c64f39f November 10, 2021 22:29

styfle previously approved these changes Nov 10, 2021

View reviewed changes

This comment has been minimized.

Sign in to view

Use cloneNode()

5c9bfc4

styfle dismissed their stale review via 5c9bfc4 November 10, 2021 23:07

ijjk approved these changes Nov 11, 2021

View reviewed changes

Merge branch 'canary' into head-manager-is-equal-node-nonce

57dae8c

ijjk merged commit c791da0 into vercel:canary Nov 11, 2021

vercel locked as resolved and limited conversation to collaborators Jan 27, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

correctly assess node equality when nonce attribute is present #27573

correctly assess node equality when nonce attribute is present #27573

ericbiewener commented Jul 29, 2021

ericbiewener commented Jul 29, 2021

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

ijjk commented Nov 10, 2021 •

edited

Loading

styfle left a comment

This comment has been minimized.

This comment has been minimized.

ericbiewener commented Nov 10, 2021

styfle commented Nov 10, 2021

ijjk commented Nov 10, 2021

Diffs

Diffs

correctly assess node equality when nonce attribute is present #27573

correctly assess node equality when nonce attribute is present #27573

Conversation

ericbiewener commented Jul 29, 2021

Bug

Documentation / Examples

ericbiewener commented Jul 29, 2021

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

ijjk commented Nov 10, 2021 • edited Loading

Failing test suites

styfle left a comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

ericbiewener commented Nov 10, 2021

styfle commented Nov 10, 2021

ijjk commented Nov 10, 2021

Stats from current PR

Diffs

Diffs

ijjk commented Nov 10, 2021 •

edited

Loading