[DO NOT LAND] Allow Previously Seen Client References to be Passed Back to a Reply #28609
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This concept is kind of like a temporary reference but instead of referring to in memory slots it passes client reference metadata back to the server if we got a function from the server in the places as a client reference. This mode only kicks in if temporary references are not available such as in progressive enhancement. However, it allows for passing all values back to the server if they came from there in the first place.
facebook-github-bot
added
CLA Signed
React Core Team
This was referenced Mar 21, 2024
sebmarkbage
added a commit
that referenced
this pull request
Mar 21, 2024
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.
sebmarkbage
added a commit
that referenced
this pull request
Mar 21, 2024
…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.
github-actions bot
pushed a commit
that referenced
this pull request
Mar 21, 2024
…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)
EdisonVan
pushed a commit
to EdisonVan/react
that referenced
this pull request
Apr 15, 2024
…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.
EdisonVan
pushed a commit
to EdisonVan/react
that referenced
this pull request
Apr 15, 2024
…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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This was a follow up #28564 but I now feel uncomfortable with this solution due to security implications. I'm only opening it for reference. Instead I propose locking it down further in #28610 and instead to allow existing code to work we land #28611.
This PR tracks Client References that came from the server and then allows them to be passed back as part of a reply - even without a temporary reference set. That way they work with
$$FORM_ACTION.Since now any kind of element could be serialize this also re-enables React Element serialization.
Security Implications
The realization is that this opens an uncomfortable security implication. When implementing Server Actions we recommend that you block cross-origin form POST by checking the Host (or even by doing your own CSRF token). At the very least you shouldn't let cookies be passed in those cases which is the default. If you don't implement the Host protection you can invoke a server action from another page by POSTing a regular form to it but at least you can't spoof a cookie.
Allowing elements to be passed into the action this way opens up a potentially scary implication because it allows a spoofed form hosted on a different origin to POST with an element that could contain XSS information and then potentially get the receiving side to render it. E.g.
This wouldn't happen in a proper framework if you block the host/origin like Next.js does. However, it does seem concerning enough since it opens up a new category of risk in the case you mess this up so it's valuable to block this at a layer. It's also not really a legit use case other than for progressive enhancement case as noted in #28564.
So I lean towards just more intentionally blocking this instead.