From e34e1df2f9da7f1c466710e06a9fe43e409339d0 Mon Sep 17 00:00:00 2001 From: Josh Story Date: Thu, 12 May 2022 15:57:20 -0700 Subject: [PATCH] support encoding errors in html stream and escape user input This commit adds another way to get errors to the suspense instance by encoding them as dataset properties of a template element at the head of the boundary. Previously if there was an error before the boundary flushed there was no way to stream the error to the client because there would never be a client render instruction. Additionally the error is sent in 3 parts 1) error hash - this is always sent (dev or prod) if one is provided 2) error message - Dev only 3) error component stack - Dev only, this now captures the stack at the point of error Another item addressed in this commit is the escaping of potentially unsafe data. all error components are escaped as test for browers when written into the html and as javascript strings when written into a client render instruction. --- .../src/__tests__/ReactDOMFizzServer-test.js | 440 +++++++++++++++--- .../ReactDOMFizzServerBrowser-test.js | 22 +- .../__tests__/ReactDOMFizzServerNode-test.js | 30 +- .../src/client/ReactDOMHostConfig.js | 25 +- .../src/server/ReactDOMFizzServerNode.js | 2 +- .../src/server/ReactDOMServerFormatConfig.js | 119 ++++- .../ReactDOMServerLegacyFormatConfig.js | 4 + .../server/ReactNativeServerFormatConfig.js | 8 +- .../src/ReactFiberBeginWork.new.js | 26 +- .../src/ReactFiberBeginWork.old.js | 26 +- .../ReactFiberHostConfigWithNoHydration.js | 2 +- .../src/forks/ReactFiberHostConfig.custom.js | 4 +- .../ReactDOMServerFB-test.internal.js | 7 +- packages/react-server/src/ReactFizzServer.js | 124 ++++- 14 files changed, 705 insertions(+), 134 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js b/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js index e519d92691ead..1710161227302 100644 --- a/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js @@ -90,32 +90,46 @@ describe('ReactDOMFizzServer', () => { }); function expectErrors(errorsArr, toBeDevArr, toBeProdArr) { + const mappedErrows = errorsArr.map(error => { + if (error.componentStack) { + return [ + error.message, + error.hash, + normalizeCodeLocInfo(error.componentStack), + ]; + } else if (error.hash) { + return [error.message, error.hash]; + } + return error.message; + }); if (__DEV__) { - expect(errorsArr.map(error => normalizeCodeLocInfo(error))).toEqual( - toBeDevArr.map(error => { - if (typeof error === 'string' || error instanceof String) { - return error; - } - let str = JSON.stringify(error).replace(/\\n/g, '\n'); - // this gets stripped away by normalizeCodeLocInfo... - // Kind of hacky but lets strip it away here too just so they match... - // easier than fixing the regex to account for this edge case - if (str.endsWith('at **)"}')) { - str = str.replace(/at \*\*\)\"}$/, 'at **)'); - } - return str; - }), + expect(mappedErrows).toEqual( + toBeDevArr, + // .map(([errorMessage, errorHash, errorComponentStack]) => { + // if (typeof error === 'string' || error instanceof String) { + // return error; + // } + // let str = JSON.stringify(error).replace(/\\n/g, '\n'); + // // this gets stripped away by normalizeCodeLocInfo... + // // Kind of hacky but lets strip it away here too just so they match... + // // easier than fixing the regex to account for this edge case + // if (str.endsWith('at **)"}')) { + // str = str.replace(/at \*\*\)\"}$/, 'at **)'); + // } + // return str; + // }), ); } else { - expect(errorsArr).toEqual(toBeProdArr); + expect(mappedErrows).toEqual(toBeProdArr); } } - function componentStack(components) { - return components - .map(component => `\n in ${component} (at **)`) - .join(''); - } + // @TODO we will use this in a followup change once we start exposing componentStacks from server errors + // function componentStack(components) { + // return components + // .map(component => `\n in ${component} (at **)`) + // .join(''); + // } async function act(callback) { await callback(); @@ -436,8 +450,6 @@ describe('ReactDOMFizzServer', () => { }); }); - const loggedErrors = []; - function App({isClient}) { return (
@@ -455,20 +467,26 @@ describe('ReactDOMFizzServer', () => { // Attempt to hydrate the content. ReactDOMClient.hydrateRoot(container, , { onRecoverableError(error) { - errors.push(error.message); + errors.push(error); }, }); }; + const theError = new Error('Test'); + const loggedErrors = []; + function onError(x) { + loggedErrors.push(x); + return 'Hash of (' + x.message + ')'; + } + // const expectedHash = onError(theError); + // loggedErrors.length = 0; + await act(async () => { const {pipe} = ReactDOMFizzServer.renderToPipeableStream( , { bootstrapScriptContent: '__INIT__();', - onError(x) { - loggedErrors.push(x); - return 'Hash'; - }, + onError, }, ); pipe(writable); @@ -483,7 +501,6 @@ describe('ReactDOMFizzServer', () => { expect(loggedErrors).toEqual([]); - const theError = new Error('Test'); await act(async () => { rejectComponent(theError); }); @@ -497,13 +514,10 @@ describe('ReactDOMFizzServer', () => { expect(Scheduler).toFlushAndYield([]); expectErrors( errors, + [theError.message], [ - { - error: theError.message, - componentStack: componentStack(['Lazy', 'Suspense', 'div', 'App']), - }, + 'The server could not finish this Suspense boundary, likely due to an error during server rendering. Switched to client rendering.', ], - ['Hash'], ); // The client rendered HTML is now in place. @@ -552,7 +566,14 @@ describe('ReactDOMFizzServer', () => { }); }); + const theError = new Error('Test'); const loggedErrors = []; + function onError(x) { + loggedErrors.push(x); + return 'hash of (' + x.message + ')'; + } + // const expectedHash = onError(theError); + // loggedErrors.length = 0; function App({isClient}) { return ( @@ -569,10 +590,7 @@ describe('ReactDOMFizzServer', () => { , { - onError(x) { - loggedErrors.push(x); - return 'hash'; - }, + onError, }, ); pipe(writable); @@ -583,7 +601,7 @@ describe('ReactDOMFizzServer', () => { // Attempt to hydrate the content. ReactDOMClient.hydrateRoot(container, , { onRecoverableError(error) { - errors.push(error.message); + errors.push(error); }, }); Scheduler.unstable_flushAll(); @@ -593,7 +611,6 @@ describe('ReactDOMFizzServer', () => { expect(loggedErrors).toEqual([]); - const theError = new Error('Test'); await act(async () => { rejectElement(theError); }); @@ -608,18 +625,157 @@ describe('ReactDOMFizzServer', () => { expectErrors( errors, + [theError.message], + [ + 'The server could not finish this Suspense boundary, likely due to an error during server rendering. Switched to client rendering.', + ], + ); + + // The client rendered HTML is now in place. + // expect(getVisibleChildren(container)).toEqual(
Hello
); + + expect(loggedErrors).toEqual([theError]); + }); + + // @gate experimental + it('Errors in boundaries should be sent to the client and reported on client render - Error before flushing', async () => { + function Indirection({level, children}) { + if (level > 0) { + return {children}; + } + return children; + } + + const theError = new Error('uh oh'); + + function Erroring({isClient}) { + if (isClient) { + return 'Hello World'; + } + throw theError; + } + + function App({isClient}) { + return ( +
+ loading...}> + + +
+ ); + } + + const loggedErrors = []; + function onError(x) { + loggedErrors.push(x); + return 'hash(' + x.message + ')'; + } + // const expectedHash = onError(theError); + // loggedErrors.length = 0; + + await act(async () => { + const {pipe} = ReactDOMFizzServer.renderToPipeableStream( + , + + { + onError, + }, + ); + pipe(writable); + }); + expect(loggedErrors).toEqual([theError]); + + const errors = []; + // Attempt to hydrate the content. + ReactDOMClient.hydrateRoot(container, , { + onRecoverableError(error) { + errors.push(error); + }, + }); + Scheduler.unstable_flushAll(); + + expect(getVisibleChildren(container)).toEqual(
Hello World
); + + expectErrors( + errors, + [theError.message], [ + 'The server could not finish this Suspense boundary, likely due to an error during server rendering. Switched to client rendering.', + ], + ); + }); + + // @gate experimental + it('Errors in boundaries should be sent to the client and reported on client render - Error after flushing', async () => { + let rejectComponent; + const LazyComponent = React.lazy(() => { + return new Promise((resolve, reject) => { + rejectComponent = reject; + }); + }); + + function App({isClient}) { + return ( +
+ }> + {isClient ? : } + +
+ ); + } + + const loggedErrors = []; + const theError = new Error('uh oh'); + function onError(x) { + loggedErrors.push(x); + return 'hash(' + x.message + ')'; + } + // const expectedHash = onError(theError); + // loggedErrors.length = 0; + + await act(async () => { + const {pipe} = ReactDOMFizzServer.renderToPipeableStream( + , + { - error: theError.message, - componentStack: componentStack(['div', 'App']), + onError, }, + ); + pipe(writable); + }); + expect(loggedErrors).toEqual([]); + + const errors = []; + // Attempt to hydrate the content. + ReactDOMClient.hydrateRoot(container, , { + onRecoverableError(error) { + errors.push(error); + }, + }); + Scheduler.unstable_flushAll(); + + expect(getVisibleChildren(container)).toEqual(
Loading...
); + + await act(async () => { + rejectComponent(theError); + }); + + expect(loggedErrors).toEqual([theError]); + expect(getVisibleChildren(container)).toEqual(
Loading...
); + + // Now we can client render it instead. + expect(Scheduler).toFlushAndYield([]); + + expectErrors( + errors, + [theError.message], + [ + 'The server could not finish this Suspense boundary, likely due to an error during server rendering. Switched to client rendering.', ], - ['hash'], ); // The client rendered HTML is now in place. expect(getVisibleChildren(container)).toEqual(
Hello
); - expect(loggedErrors).toEqual([theError]); }); @@ -891,9 +1047,15 @@ describe('ReactDOMFizzServer', () => { ); } + const loggedErrors = []; + function onError(error) { + loggedErrors.push(error); + return `Hash of (${error.message})`; + } + let controls; await act(async () => { - controls = ReactDOMFizzServer.renderToPipeableStream(); + controls = ReactDOMFizzServer.renderToPipeableStream(, {onError}); controls.pipe(writable); }); @@ -903,7 +1065,7 @@ describe('ReactDOMFizzServer', () => { // Attempt to hydrate the content. ReactDOMClient.hydrateRoot(container, , { onRecoverableError(error) { - errors.push(error.message); + errors.push(error); }, }); Scheduler.unstable_flushAll(); @@ -921,7 +1083,9 @@ describe('ReactDOMFizzServer', () => { expectErrors( errors, ['This Suspense boundary was aborted by the server'], - ['This Suspense boundary was aborted by the server'], + [ + 'The server could not finish this Suspense boundary, likely due to an error during server rendering. Switched to client rendering.', + ], ); expect(getVisibleChildren(container)).toEqual(
Loading...
); @@ -1580,17 +1744,22 @@ describe('ReactDOMFizzServer', () => { ); } + const theError = new Error('Test'); const loggedErrors = []; + function onError(x) { + loggedErrors.push(x); + return `hash of (${x.message})`; + } + // const expectedHash = onError(theError); + // loggedErrors.length = 0; + let controls; await act(async () => { controls = ReactDOMFizzServer.renderToPipeableStream( , { - onError(x) { - loggedErrors.push(x); - return 'error hash'; - }, + onError, }, ); controls.pipe(writable); @@ -1602,7 +1771,7 @@ describe('ReactDOMFizzServer', () => { // Attempt to hydrate the content. ReactDOMClient.hydrateRoot(container, , { onRecoverableError(error) { - errors.push(error.message); + errors.push(error); }, }); Scheduler.unstable_flushAll(); @@ -1612,7 +1781,6 @@ describe('ReactDOMFizzServer', () => { expect(loggedErrors).toEqual([]); - const theError = new Error('Test'); // Error the content, but we don't have a fallback yet. await act(async () => { rejectText('Hello', theError); @@ -1636,20 +1804,10 @@ describe('ReactDOMFizzServer', () => { expect(Scheduler).toFlushAndYield([]); expectErrors( errors, + [theError.message], [ - { - error: theError.message, - componentStack: componentStack([ - 'AsyncText', - 'h1', - 'Suspense', - 'div', - 'Suspense', - 'App', - ]), - }, + 'The server could not finish this Suspense boundary, likely due to an error during server rendering. Switched to client rendering.', ], - ['error hash'], ); // The client rendered HTML is now in place. @@ -2856,6 +3014,160 @@ describe('ReactDOMFizzServer', () => { ); }); + describe('error escaping', () => { + //@gate experimental + it('escapes error hash, message, and component stack values in directly flushed errors (html escaping)', async () => { + window.__outlet = {}; + + const dangerousErrorString = + '">