fix(reactHtml): use browser integrity for external fallbacks (#1166)

americanexpress · Nov 13, 2023 · 6ae1e65 · 6ae1e65
1 parent 1131e9c
commit 6ae1e65
Show file tree

Hide file tree

Showing 4 changed files with 59 additions and 32 deletions.
diff --git a/__tests__/server/plugins/reactHtml/index.spec.jsx b/__tests__/server/plugins/reactHtml/index.spec.jsx
@@ -920,7 +920,8 @@ describe('reactHtml', () => {
           'this-dep': {
             filename: 'this-dep.js',
             semanticRange: '^2.2.0',
-            integrity: '12345hash',
+            browserIntegrity: '12345hash-browser',
+            nodeIntegrity: '12345hash-node',
             version: '2.3.1',
           },
         },
@@ -934,7 +935,7 @@ describe('reactHtml', () => {
           moduleMap,
         })
       ).toMatchInlineSnapshot(
-        '"<script src="https://example.com/cdn/child-module-a/1.0.0/this-dep.browser.js?clientCacheRevision=123" crossorigin="anonymous" integrity="12345hash"></script>"'
+        '"<script src="https://example.com/cdn/child-module-a/1.0.0/this-dep.browser.js?clientCacheRevision=123" crossorigin="anonymous" integrity="12345hash-browser"></script>"'
       );
     });
 
@@ -944,15 +945,17 @@ describe('reactHtml', () => {
           'this-dep': {
             filename: 'this-dep.js',
             semanticRange: '^2.2.0',
-            integrity: '12345hash',
+            browserIntegrity: '12345hash-browser',
+            nodeIntegrity: '12345hash-node',
             version: '2.3.1',
           },
         },
         'child-module-b': {
           'this-dep': {
             filename: 'this-dep.js',
             semanticRange: '^2.2.0',
-            integrity: '12345hash',
+            browserIntegrity: '12345hash-browser',
+            nodeIntegrity: '12345hash-node',
             version: '2.3.1',
           },
         },
@@ -968,7 +971,7 @@ describe('reactHtml', () => {
           moduleMap,
         })
       ).toMatchInlineSnapshot(
-        '"<script src="https://example.com/cdn/child-module-b/1.1.0/this-dep.browser.js?clientCacheRevision=123" crossorigin="anonymous" integrity="12345hash"></script><script src="https://example.com/cdn/child-module-a/1.0.0/this-dep.browser.js?clientCacheRevision=123" crossorigin="anonymous" integrity="12345hash"></script>"'
+        '"<script src="https://example.com/cdn/child-module-b/1.1.0/this-dep.browser.js?clientCacheRevision=123" crossorigin="anonymous" integrity="12345hash-browser"></script><script src="https://example.com/cdn/child-module-a/1.0.0/this-dep.browser.js?clientCacheRevision=123" crossorigin="anonymous" integrity="12345hash-browser"></script>"'
       );
     });
 
@@ -984,6 +987,30 @@ describe('reactHtml', () => {
       );
     });
 
+    it('includes integrity as "undefined" when not present', () => {
+      setRequiredExternalsRegistry({
+        'child-module-a': {
+          'this-dep': {
+            filename: 'this-dep.js',
+            semanticRange: '^2.2.0',
+            version: '2.3.1',
+          },
+        },
+      });
+
+      const fallbackScripts = renderExternalFallbacks({
+        clientInitialState,
+        moduleMap,
+      });
+      expect(fallbackScripts).toMatchInlineSnapshot(
+        '"<script src="https://example.com/cdn/child-module-a/1.0.0/this-dep.browser.js?clientCacheRevision=123" crossorigin="anonymous" integrity="undefined"></script>"'
+      );
+
+      expect(console.warn).toHaveBeenCalledWith(
+        'No SRI integrity hash found for script https://example.com/cdn/child-module-a/1.0.0/this-dep.browser.js. This is a security risk.'
+      );
+    });
+
     it('handles trailing "/" in module baseUrl', () => {
       expect(
         renderExternalFallbacks({
@@ -998,20 +1025,7 @@ describe('reactHtml', () => {
           },
         })
       ).toMatchInlineSnapshot(
-        '"<script src="https://example.com/cdn/child-module-a/1.0.0/this-dep.browser.js?clientCacheRevision=123" crossorigin="anonymous" integrity="12345hash"></script>"'
-      );
-    });
-
-    it('supports legacy browsers', () => {
-      expect(
-        renderExternalFallbacks({
-          clientInitialState,
-          moduleMap,
-          isDevelopmentEnv: true,
-          isLegacy: true,
-        })
-      ).toMatchInlineSnapshot(
-        '"<script src="https://example.com/cdn/child-module-a/1.0.0/this-dep.legacy.browser.js" crossorigin="anonymous" ></script>"'
+        '"<script src="https://example.com/cdn/child-module-a/1.0.0/this-dep.browser.js?clientCacheRevision=123" crossorigin="anonymous" integrity="12345hash-browser"></script>"'
       );
     });
   });
@@ -1029,7 +1043,10 @@ describe('reactHtml', () => {
 
       sendHtml(request, reply);
       expect(request.log.error).toHaveBeenCalledTimes(1);
-      expect(request.log.error).toHaveBeenCalledWith('encountered an error serializing full client initial state', fullStateError);
+      expect(request.log.error).toHaveBeenCalledWith(
+        'encountered an error serializing full client initial state',
+        fullStateError
+      );
       expect(transit.toJSON).toHaveBeenCalledTimes(3);
 
       expect(reply.send).toHaveBeenCalledTimes(1);
@@ -1061,8 +1078,14 @@ describe('reactHtml', () => {
 
       sendHtml(request, reply);
       expect(request.log.error).toHaveBeenCalledTimes(3);
-      expect(request.log.error).toHaveBeenCalledWith('encountered an error serializing full client initial state', fullStateError);
-      expect(request.log.error).toHaveBeenCalledWith('unable to build the most basic initial state for a client to startup', minimalStateError);
+      expect(request.log.error).toHaveBeenCalledWith(
+        'encountered an error serializing full client initial state',
+        fullStateError
+      );
+      expect(request.log.error).toHaveBeenCalledWith(
+        'unable to build the most basic initial state for a client to startup',
+        minimalStateError
+      );
       expect(transit.toJSON).toHaveBeenCalledTimes(4);
 
       expect(reply.send).toHaveBeenCalledTimes(1);
@@ -1115,7 +1138,9 @@ describe('reactHtml', () => {
       jest.spyOn(console, 'error');
       isRedirectUrlAllowed.mockImplementationOnce(() => false);
       checkStateForRedirectAndStatusCode(req, reply);
-      expect(util.format(...console.error.mock.calls[0])).toBe(`'${destination}' is not an allowed redirect URL`);
+      expect(util.format(...console.error.mock.calls[0])).toBe(
+        `'${destination}' is not an allowed redirect URL`
+      );
     });
   });
 

diff --git a/package-lock.json b/package-lock.json
diff --git a/package.json b/package.json
@@ -112,7 +112,7 @@
     "fastify-plugin": "^4.5.1",
     "flat": "^5.0.2",
     "helmet": "^7.0.0",
-    "holocron": "^1.8.2",
+    "holocron": "^1.9.1",
     "holocron-module-route": "^1.7.0",
     "immutable": "^4.1.0",
     "ip": "^1.1.8",

diff --git a/src/server/plugins/reactHtml/index.jsx b/src/server/plugins/reactHtml/index.jsx
@@ -76,6 +76,10 @@ function renderI18nScript(clientInitialState, appBundlesURLPrefix) {
 function renderScript({
   src, integrity, isDevelopmentEnv, clientCacheRevision,
 }) {
+  if (!integrity && !isDevelopmentEnv) console.warn(`No SRI integrity hash found for script ${src}. This is a security risk.`);
+  // TODO: consider throwing an error in next major version. This is a breaking change.
+  // currently we rely on "undefined" to throw integrity error in the browser, this is
+  // results in poor DX, hard to find bugs.
   const additionalAttributes = isDevelopmentEnv ? '' : `integrity="${integrity}"`;
   const scriptSource = isDevelopmentEnv || !clientCacheRevision
     ? src
@@ -110,7 +114,6 @@ export function renderExternalFallbacks({
   clientInitialState,
   moduleMap,
   isDevelopmentEnv,
-  isLegacy,
 }) {
   const loadedModules = clientInitialState.getIn(['holocron', 'loaded'], iSet()).toArray();
   const requiredFallbacks = loadedModules
@@ -132,22 +135,21 @@ export function renderExternalFallbacks({
   const { clientCacheRevision, modules } = moduleMap;
 
   return requiredFallbacks
-    .map(({ name, integrity, moduleName }) => {
+    .map(({ name, browserIntegrity, moduleName }) => {
       const { baseUrl } = modules[moduleName];
       const endsWithSlash = baseUrl.endsWith('/');
       const src = [
         baseUrl,
         [
           name,
-          isLegacy ? 'legacy' : '',
           'browser',
           'js',
         ].filter(Boolean).join('.'),
       ].join(endsWithSlash ? '' : '/');
 
       return renderScript({
         src,
-        integrity,
+        integrity: browserIntegrity,
         isDevelopmentEnv,
         clientCacheRevision,
       });