Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

React 18: bootstrapScriptContent escapes HTML so quotes can’t be used #23063

Closed
calebmer opened this issue Jan 3, 2022 · 12 comments · Fixed by #24385
Closed

React 18: bootstrapScriptContent escapes HTML so quotes can’t be used #23063

calebmer opened this issue Jan 3, 2022 · 12 comments · Fixed by #24385
Labels
React 18 Bug reports, questions, and general feedback about React 18 Type: Discussion

Comments

@calebmer
Copy link
Contributor

calebmer commented Jan 3, 2022

If you use the bootstrapScriptContent option of renderToPipeableStream() to provide hydration data (as recommended in the <script> upgrade guide) with JSON.stringify() it doesn’t work because React escapes HTML characters in bootstrapScriptContent like quotes. I’ve worked around this by using backticks to deliniate strings.

What’s the correct thing to do here?

if (bootstrapScriptContent !== undefined) {
bootstrapChunks.push(
inlineScriptWithNonce,
stringToChunk(escapeTextForBrowser(bootstrapScriptContent)),
endInlineScript,
);
}

@calebmer calebmer added React 18 Bug reports, questions, and general feedback about React 18 Type: Discussion labels Jan 3, 2022
@salazarm
Copy link
Contributor

@sebmarkbage @acdlite Is the escapeTextForBrowser usage here intentional? I think it might make sense not to escape here since it's not HTML and its supposed to be javascript here.

@sebmarkbage
Copy link
Collaborator

Yea it's probably not the correct escape. We should escape somehow though because </script> and such content shouldn't be allowed. Not escaping random JSON that might contain user generated strings would be a bad and common security issue.

It's a common security issue in many non-meta frameworks.

cc @sophiebits knows the exact pattern needed.

If we can't make it generic we should at least rename it "dangerous" and describe how to escape any JSON content someone might add there.

@sophiebits
Copy link
Collaborator

sophiebits commented Jan 10, 2022

See the update at the end of my post here: https://sophiebits.com/2012/08/03/preventing-xss-json.html. Basically replacing the s in </script with \u0073 (or S with \u0053). Make sure to match the pattern case-insensitively.

It's security-safe (to my belief) but note that this technically can change the meaning of a script with a tagged template literal since the raw text is available, but I doubt that would come up in practice so it's probably fine.

The only other ideas I have are having the script encoded in some fully out-of-band way like a base64 data URI or putting the whole script in a string and then eval-ing it (both of which are weird for CSP).

@sebmarkbage
Copy link
Collaborator

@devknoll @timneutkens What does Next.js do to escape JSON? Presumably that has received enough exposure.

@sophiebits
Copy link
Collaborator

sophiebits commented Jan 10, 2022

For JSON you only need to escape / as \/ (many JSON encoders do this automatically).

@tranvansang
Copy link

tranvansang commented Apr 3, 2022

This is blocking me from the upgrade to react 18.

How about optionally accepting an object with __htlm key and leave the responsibility to the user?

{
  bootstrapScriptContent: {
    __dangerouslySetInnerHTML: 'window.__BOOTSTRAP_REACT_APP__("complex inline ssr data")`
  }
}

With react 17, I used serialize-javascript to serialize the ssr data without any problem.

@sebmarkbage
Copy link
Collaborator

sebmarkbage commented Apr 14, 2022

@sophiebits I found this little tidbit while reading the spec. w3c/html#1617

I'm not sure you can use that to inject new scripts but you can certainly cover up future scripts or content and break the page.

I think it's probably ok if you also encode the s in <script.

@sebmarkbage
Copy link
Collaborator

sebmarkbage commented Apr 15, 2022

This API only really promises to encode valid JS into HTML. It doesn't promise to turn JSON into JS. In practice anyone using this to inject JSON will need to use another tool to escape JSON into valid JS since we can't parse out the JSON parts from arbitrary JS anyway.

So it's not super critical that we cover all cases because they'll be covered by that encoding anyway. Our escaping is more of a convenience and extra layer of protection.

Edit: Actually because of https://github.com/tc39/proposal-json-superset you probably don't need to escape it in modern browsers. Although many probably will keep doing it as a precaution. You also have to consider "__proto__" properties in JSON anyway.

Escaping the s in </script> and <script> to \u0073 as well as the upper-case version is probably sufficient.

@sebmarkbage
Copy link
Collaborator

sebmarkbage commented Apr 15, 2022

I think we're just going to deprecate XHTML support therefore I don't think ]]> is a sequence of concern. Correct me if I'm wrong.

Additionally, some escape <!-- and --> in JSON because they can switch the parsing modes. However, since we escape all <script> and </script> in the entire script and not just the JSON part of it, I don't think that concern applies here.

The reason we wouldn't escape them is because the clever hack doesn't work on these while it remains valid JS.

@gnoff
Copy link
Collaborator

gnoff commented Apr 16, 2022

@calebmer should be available in the next minor

@gaearon
Copy link
Collaborator

gaearon commented Apr 27, 2022

Fixed in 18.1.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
React 18 Bug reports, questions, and general feedback about React 18 Type: Discussion
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants