Skip to content
This repository has been archived by the owner on May 3, 2024. It is now read-only.

fix(reactHtml): renderScripts adding unknown integrity #1166

Merged
merged 7 commits into from
Nov 13, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
71 changes: 48 additions & 23 deletions __tests__/server/plugins/reactHtml/index.spec.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -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',
},
},
Expand All @@ -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>"'
);
});

Expand All @@ -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',
},
},
Expand All @@ -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>"'
);
});

Expand All @@ -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({
Expand All @@ -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>"'
);
});
});
Expand All @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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`
);
});
});

Expand Down
8 changes: 4 additions & 4 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,7 @@
"fastify-plugin": "^4.5.1",
"flat": "^5.0.2",
"helmet": "^7.0.0",
"holocron": "^1.8.2",
JAdshead marked this conversation as resolved.
Show resolved Hide resolved
"holocron": "^1.9.1",
Copy link
Member

Choose a reason for hiding this comment

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

I this required? If I remember correctly the 1.9.0 release was done unintentionally?

Copy link
Contributor Author

@JAdshead JAdshead Nov 13, 2023

Choose a reason for hiding this comment

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

yes, it includes the fixes for integrity loading in holocron - americanexpress/holocron#162

"holocron-module-route": "^1.7.0",
"immutable": "^4.1.0",
"ip": "^1.1.8",
Expand Down
10 changes: 6 additions & 4 deletions src/server/plugins/reactHtml/index.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -110,7 +114,6 @@ export function renderExternalFallbacks({
clientInitialState,
moduleMap,
isDevelopmentEnv,
isLegacy,
}) {
const loadedModules = clientInitialState.getIn(['holocron', 'loaded'], iSet()).toArray();
const requiredFallbacks = loadedModules
Expand All @@ -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,
});
Expand Down
Loading