[Fizz] Recover from errors thrown by progressive enhancement form generation #28611

sebmarkbage · 2024-03-21T20:43:59Z

This a follow up to #28564. It's alternative to #28609 which takes #28610 into account.

It used to be possible to return JSX from an action with useActionState.

async function action(errors, payload) {
  "use server";
  try {
    ...
  } catch (x) {
    return <div>Error message</div>;
  }
}

const [errors, formAction] = useActionState(action);
return <div>{errors}</div>;

Returning JSX from an action is itself not anything problematic. It's that it also has to return the previous state to the action reducer again that's the problem. When this happens we accidentally could serialize an Element back to the server.

I fixed this in #28564 so it's now blocked if you don't have a temporary reference set.

However, you can't have that for the progressive enhancement case. The reply is eagerly encoded as part of the SSR render. Typically you wouldn't have these in the initial state so the common case is that they show up after the first POST back that yields an error and it's only in the no-JS case where this happens so it's hard to discover.

As noted in #28609 there's a security implication with allowing elements to be sent across this kind of payload, so we can't just make it work.

When an error happens during SSR our general policy is to try to recover on the client instead. After all, SSR is mainly a perf optimization in React terms and it's not primarily intended for a no JS solution.

This PR takes the approach that if we fail to generate the progressive enhancement payload. I.e. if the serialization of previous state / closures throw. Then we fallback to the replaying semantics just client actions instead which will succeed.

The effect of this is that this pattern mostly just works:

First render in the typical case doesn't have any JSX in it so it just renders a progressive enhanced form.
If JS fails to hydrate or you click early we do a form POST. If that hits an error and it tries to render it using JSX, then the new page will render successfully - however this time with a Replaying form instead.
If you try to submit the form again it'll have to be using JS.

Meaning if you use JSX as the error return value of form state and you make a first attempt that fails, then no JS won't work because either the first or second attempt has to hydrate.

We have ideas for potentially optimizing away serializing unused arguments like if you don't actually use previous state which would also solve it but it wouldn't cover all cases such as if it was deeply nested in complex state.

Another approach that I considered was to poison the prev state if you passed an element back but let it through to the action but if you try to render the poisoned value, it wouldn't work. The downside of this is when to error. Because in the progressive enhancement case it wouldn't error early but when you actually try to invoke it at which point it would be too late to fallback to client replaying. It would probably have to always error even on the client which is unfortunate since this mostly just works as long as it hydrates.

react-sizebot · 2024-03-21T20:48:07Z

Comparing: a493901...5ba7fda

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name	+/-	Base	Current	+/- gzip	Base gzip	Current gzip
oss-stable/react-dom/cjs/react-dom.production.min.js	=	176.84 kB	176.84 kB	=	54.92 kB	54.92 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js	=	173.27 kB	173.27 kB	=	54.04 kB	54.04 kB
facebook-www/ReactDOM-prod.classic.js	=	594.30 kB	594.30 kB	=	104.45 kB	104.45 kB
facebook-www/ReactDOM-prod.modern.js	=	577.56 kB	577.56 kB	=	101.48 kB	101.48 kB
test_utils/ReactAllWarnings.js	Deleted	66.48 kB	0.00 kB	Deleted	16.26 kB	0.00 kB

Significant size changes

Includes any change greater than 0.2%:

Expand to show

Name	+/-	Base	Current	+/- gzip	Base gzip	Current gzip
test_utils/ReactAllWarnings.js	Deleted	66.48 kB	0.00 kB	Deleted	16.26 kB	0.00 kB

Generated by 🚫 dangerJS against 5ba7fda

As mentioned in #28609 there's a potential security risk if you allow a passed value to the server to spoof Elements because it allows a hacker to POST cross origin. This is only an issue if your framework allows this which it shouldn't but it seems like we should provide an extra layer of security here. ```js function action(errors, payload) { try { ... } catch (x) { return [newError].concat(errors); } } ``` ```js const [errors, formAction] = useActionState(action); return <div>{errors}</div>; ``` This would allow you to construct a payload where the previous "errors" set includes something like `<script src="danger.js" />`. We could block only elements from being received but it could potentially be a risk with creating other React types like Context too. We use symbols as a way to securely brand these. Most JS don't use this kind of branding with symbols like we do. They're generally properties which we don't support anyway. However in theory someone else could be using them like we do. So in an abundance of carefulness I just ban all symbols from being passed (except by temporary reference) - not just ours. This means that the format isn't fully symmetric even beyond just React Nodes. #28611 allows code that includes symbols/elements to continue working but may have to bail out to replaying instead of no JS sometimes. However, you still can't access the symbols inside the server - they're by reference only.

packages/react-dom-bindings/src/server/ReactFizzConfigDOM.js

This would be from encoding previous state or bound/closures. In this case we fallback to event replaying instead of erroring the page.

gnoff · 2024-03-21T21:15:00Z

packages/react-dom-bindings/src/server/ReactFizzConfigDOM.js

+      }
+    }
+  }
+  return null;


What was the old behavior if processReply errored? These changes make sense for the stated use case but could processReply error in a way where we wouldn't want the client action wait for hydration style treatement on SSR?

It used to just error the SSR render - which itself is just a client-render to recover so not really that different.

The nice thing about that is that if your previous state or closure contains something non-serializable it would give you an early hint that IF you submitted the form it would error. Now that happens after submitting.

In principle we could in DEV or something eagerly serialize these on the client to see if it would've failed on the client too early.

But in general it's just going to error when you try to invoke the form instead.

However, since most things with temporary references is serializable anyway we don't really error for anything other than if like a promise we're trying to serialize rejects (which should arguably error on the server instead as discussed before - if we wanted to support streaming replies). More likely the server will error when you try to inspect a Reference.

Should we brand the temporaryReferenceSet errors so we can treat them passively like this but let the other errors error the SSR render?

Other than the promise thing - which should probably be serialized as an error instead. Any other error is really a bug in the serialization and it seems better to fallback in that case anyway.

Regardless it seems like it's better in production to fallback to client handling just for the form than client handling for the whole page.

gnoff

Maybe we only passively ignore the temporaryReferenceSet errors. but we can decide independent of landing since the existing semantics are preserved just we move where the error is encountered

…eration (#28611) This a follow up to #28564. It's alternative to #28609 which takes #28610 into account. It used to be possible to return JSX from an action with `useActionState`. ```js async function action(errors, payload) { "use server"; try { ... } catch (x) { return <div>Error message</div>; } } ``` ```js const [errors, formAction] = useActionState(action); return <div>{errors}</div>; ``` Returning JSX from an action is itself not anything problematic. It's that it also has to return the previous state to the action reducer again that's the problem. When this happens we accidentally could serialize an Element back to the server. I fixed this in #28564 so it's now blocked if you don't have a temporary reference set. However, you can't have that for the progressive enhancement case. The reply is eagerly encoded as part of the SSR render. Typically you wouldn't have these in the initial state so the common case is that they show up after the first POST back that yields an error and it's only in the no-JS case where this happens so it's hard to discover. As noted in #28609 there's a security implication with allowing elements to be sent across this kind of payload, so we can't just make it work. When an error happens during SSR our general policy is to try to recover on the client instead. After all, SSR is mainly a perf optimization in React terms and it's not primarily intended for a no JS solution. This PR takes the approach that if we fail to generate the progressive enhancement payload. I.e. if the serialization of previous state / closures throw. Then we fallback to the replaying semantics just client actions instead which will succeed. The effect of this is that this pattern mostly just works: - First render in the typical case doesn't have any JSX in it so it just renders a progressive enhanced form. - If JS fails to hydrate or you click early we do a form POST. If that hits an error and it tries to render it using JSX, then the new page will render successfully - however this time with a Replaying form instead. - If you try to submit the form again it'll have to be using JS. Meaning if you use JSX as the error return value of form state and you make a first attempt that fails, then no JS won't work because either the first or second attempt has to hydrate. We have ideas for potentially optimizing away serializing unused arguments like if you don't actually use previous state which would also solve it but it wouldn't cover all cases such as if it was deeply nested in complex state. Another approach that I considered was to poison the prev state if you passed an element back but let it through to the action but if you try to render the poisoned value, it wouldn't work. The downside of this is when to error. Because in the progressive enhancement case it wouldn't error early but when you actually try to invoke it at which point it would be too late to fallback to client replaying. It would probably have to always error even on the client which is unfortunate since this mostly just works as long as it hydrates. DiffTrain build for [fee786a](fee786a)

…28610) As mentioned in facebook#28609 there's a potential security risk if you allow a passed value to the server to spoof Elements because it allows a hacker to POST cross origin. This is only an issue if your framework allows this which it shouldn't but it seems like we should provide an extra layer of security here. ```js function action(errors, payload) { try { ... } catch (x) { return [newError].concat(errors); } } ``` ```js const [errors, formAction] = useActionState(action); return <div>{errors}</div>; ``` This would allow you to construct a payload where the previous "errors" set includes something like `<script src="danger.js" />`. We could block only elements from being received but it could potentially be a risk with creating other React types like Context too. We use symbols as a way to securely brand these. Most JS don't use this kind of branding with symbols like we do. They're generally properties which we don't support anyway. However in theory someone else could be using them like we do. So in an abundance of carefulness I just ban all symbols from being passed (except by temporary reference) - not just ours. This means that the format isn't fully symmetric even beyond just React Nodes. facebook#28611 allows code that includes symbols/elements to continue working but may have to bail out to replaying instead of no JS sometimes. However, you still can't access the symbols inside the server - they're by reference only.

…eration (facebook#28611) This a follow up to facebook#28564. It's alternative to facebook#28609 which takes facebook#28610 into account. It used to be possible to return JSX from an action with `useActionState`. ```js async function action(errors, payload) { "use server"; try { ... } catch (x) { return <div>Error message</div>; } } ``` ```js const [errors, formAction] = useActionState(action); return <div>{errors}</div>; ``` Returning JSX from an action is itself not anything problematic. It's that it also has to return the previous state to the action reducer again that's the problem. When this happens we accidentally could serialize an Element back to the server. I fixed this in facebook#28564 so it's now blocked if you don't have a temporary reference set. However, you can't have that for the progressive enhancement case. The reply is eagerly encoded as part of the SSR render. Typically you wouldn't have these in the initial state so the common case is that they show up after the first POST back that yields an error and it's only in the no-JS case where this happens so it's hard to discover. As noted in facebook#28609 there's a security implication with allowing elements to be sent across this kind of payload, so we can't just make it work. When an error happens during SSR our general policy is to try to recover on the client instead. After all, SSR is mainly a perf optimization in React terms and it's not primarily intended for a no JS solution. This PR takes the approach that if we fail to generate the progressive enhancement payload. I.e. if the serialization of previous state / closures throw. Then we fallback to the replaying semantics just client actions instead which will succeed. The effect of this is that this pattern mostly just works: - First render in the typical case doesn't have any JSX in it so it just renders a progressive enhanced form. - If JS fails to hydrate or you click early we do a form POST. If that hits an error and it tries to render it using JSX, then the new page will render successfully - however this time with a Replaying form instead. - If you try to submit the form again it'll have to be using JS. Meaning if you use JSX as the error return value of form state and you make a first attempt that fails, then no JS won't work because either the first or second attempt has to hydrate. We have ideas for potentially optimizing away serializing unused arguments like if you don't actually use previous state which would also solve it but it wouldn't cover all cases such as if it was deeply nested in complex state. Another approach that I considered was to poison the prev state if you passed an element back but let it through to the action but if you try to render the poisoned value, it wouldn't work. The downside of this is when to error. Because in the progressive enhancement case it wouldn't error early but when you actually try to invoke it at which point it would be too late to fallback to client replaying. It would probably have to always error even on the client which is unfortunate since this mostly just works as long as it hydrates.

…ize a Blob (#28987) This follows the same principle as in #28611. We cannot serialize Blobs of a form data into HTML because you can't initialize a file input to some value. However the serialization of state in an Action can contain blobs. In this case we do error but outside the try/catch that recovers to error to client replaying instead of MPA mode. This errors earlier to ensure that this works. Testing this is a bit annoying because JSDOM doesn't have any of the Blob methods but the Blob needs to be compatible with FormData and the FormData needs to be compatible with `<form>` nodes in these tests. So I polyfilled those in JSDOM with some hacks. A possible future enhancement would be to encode these blobs in a base64 mode instead and have some way to receive them on the server. It's just a matter of layering this. I think the RSC layer's `FORM_DATA` implementation can pass some flag to encode as base64 and then have decodeAction include some way to parse them. That way this case would work in MPA mode too.

…ize a Blob (#28987) This follows the same principle as in #28611. We cannot serialize Blobs of a form data into HTML because you can't initialize a file input to some value. However the serialization of state in an Action can contain blobs. In this case we do error but outside the try/catch that recovers to error to client replaying instead of MPA mode. This errors earlier to ensure that this works. Testing this is a bit annoying because JSDOM doesn't have any of the Blob methods but the Blob needs to be compatible with FormData and the FormData needs to be compatible with `<form>` nodes in these tests. So I polyfilled those in JSDOM with some hacks. A possible future enhancement would be to encode these blobs in a base64 mode instead and have some way to receive them on the server. It's just a matter of layering this. I think the RSC layer's `FORM_DATA` implementation can pass some flag to encode as base64 and then have decodeAction include some way to parse them. That way this case would work in MPA mode too. DiffTrain build for [6bac4f2](6bac4f2)

sebmarkbage requested review from gnoff and acdlite March 21, 2024 20:43

facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Mar 21, 2024

sebmarkbage mentioned this pull request Mar 21, 2024

[DO NOT LAND] Allow Previously Seen Client References to be Passed Back to a Reply #28609

Closed

sebmarkbage mentioned this pull request Mar 21, 2024

[Flight Reply] Don't allow Symbols to be passed to a reply #28610

Merged

sebmarkbage force-pushed the jsxformstate branch from 20065e4 to e8eed72 Compare March 21, 2024 20:51

sebmarkbage force-pushed the jsxformstate branch from e8eed72 to 0a37151 Compare March 21, 2024 21:08

sebmarkbage commented Mar 21, 2024

View reviewed changes

packages/react-dom-bindings/src/server/ReactFizzConfigDOM.js Outdated Show resolved Hide resolved

Recover from errors thrown by progressive enhancement form generation

5ba7fda

This would be from encoding previous state or bound/closures. In this case we fallback to event replaying instead of erroring the page.

sebmarkbage force-pushed the jsxformstate branch from 0a37151 to 5ba7fda Compare March 21, 2024 21:13

gnoff reviewed Mar 21, 2024

View reviewed changes

gnoff approved these changes Mar 21, 2024

View reviewed changes

sebmarkbage merged commit fee786a into facebook:main Mar 21, 2024
38 checks passed

gnoff mentioned this pull request Mar 25, 2024

Update React from 14898b6a9 to e373190fa vercel/next.js#63685

Closed

eps1lon mentioned this pull request Apr 19, 2024

Update React from 14898b6a9 to c3048aab4 vercel/next.js#64798

Merged

sebmarkbage mentioned this pull request May 3, 2024

[Fizz] Fallback to client replaying actions if we're trying to serialize a Blob #28987

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Fizz] Recover from errors thrown by progressive enhancement form generation #28611

[Fizz] Recover from errors thrown by progressive enhancement form generation #28611

sebmarkbage commented Mar 21, 2024

react-sizebot commented Mar 21, 2024 •

edited

Loading

gnoff Mar 21, 2024

sebmarkbage Mar 21, 2024 •

edited

Loading

gnoff Mar 21, 2024

sebmarkbage Mar 21, 2024

gnoff left a comment

[Fizz] Recover from errors thrown by progressive enhancement form generation #28611

[Fizz] Recover from errors thrown by progressive enhancement form generation #28611

Conversation

sebmarkbage commented Mar 21, 2024

react-sizebot commented Mar 21, 2024 • edited Loading

Critical size changes

Significant size changes

gnoff Mar 21, 2024

Choose a reason for hiding this comment

sebmarkbage Mar 21, 2024 • edited Loading

Choose a reason for hiding this comment

gnoff Mar 21, 2024

Choose a reason for hiding this comment

sebmarkbage Mar 21, 2024

Choose a reason for hiding this comment

gnoff left a comment

Choose a reason for hiding this comment

react-sizebot commented Mar 21, 2024 •

edited

Loading

sebmarkbage Mar 21, 2024 •

edited

Loading