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

[Fizz] escape <style> textContent as css #28870

Merged
merged 1 commit into from
Apr 19, 2024
Merged

Conversation

gnoff
Copy link
Collaborator

@gnoff gnoff commented Apr 18, 2024

style text content has historically been escaped as HTML which is non-sensical and often leads users to using dangerouslySetInnerHTML as a matter of course. While rendering untrusted style rules is a security risk React doesn't really provide any special protection here and forcing users to use a completely unescaped API is if anything worse. So this PR updates the style escaping rules for Fizz to only escape the text content to ensure the tag scope cannot be closed early. This is accomplished by encoding "s" and "S" as hexadecimal unicode representation "\73 " and "\53 " respectively when found within a sequence like </style>. We have to be careful to support casing here just like with the script closing tag regex for bootstrap scripts.

style text content has historically been escaped as HTML which is non-sensical and often leads users to using dangerouslySetInnerHTML as a matter of course. While rendering untrusted style rules is a security risk React doesn't really provide any special protection here and forcing users to use a completely unescaped API is if anything worse. So this PR updates the style escaping rules for Fizz to only escape the text content to ensure the tag scope cannot be closed early. This is accomplished by encoding "s" and "S" as hexadecimal unicode representation "\73 " and "\53 " respectively when found within a sequence like </style>. We have to be careful to support casing here just like with the script closing tag regex for bootstrap scripts.
@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Apr 18, 2024
@react-sizebot
Copy link

Comparing: 4c34a7f...a61a35d

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 = 168.91 kB 168.91 kB = 52.93 kB 52.94 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js = 170.56 kB 170.56 kB = 53.43 kB 53.43 kB
facebook-www/ReactDOM-prod.classic.js = 590.81 kB 590.81 kB = 103.90 kB 103.90 kB
facebook-www/ReactDOM-prod.modern.js = 566.62 kB 566.62 kB = 100.09 kB 100.09 kB
test_utils/ReactAllWarnings.js Deleted 64.44 kB 0.00 kB Deleted 16.10 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 64.44 kB 0.00 kB Deleted 16.10 kB 0.00 kB

Generated by 🚫 dangerJS against a61a35d

prefix: string,
s: string,
suffix: string,
) => `${prefix}${s === 's' ? '\\73 ' : '\\53 '}${suffix}`;
Copy link
Member

Choose a reason for hiding this comment

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

should there be the spaces here? Not exactly sure how that escaping works. '\\73 '

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes. That’s how the end of the escape sequence is encoded in CSS. It should be exactly one.

@gnoff gnoff merged commit aead514 into facebook:main Apr 19, 2024
38 checks passed
@gnoff gnoff deleted the escape-style branch April 19, 2024 00:24
github-actions bot pushed a commit that referenced this pull request Apr 19, 2024
style text content has historically been escaped as HTML which is
non-sensical and often leads users to using dangerouslySetInnerHTML as a
matter of course. While rendering untrusted style rules is a security
risk React doesn't really provide any special protection here and
forcing users to use a completely unescaped API is if anything worse. So
this PR updates the style escaping rules for Fizz to only escape the
text content to ensure the tag scope cannot be closed early. This is
accomplished by encoding "s" and "S" as hexadecimal unicode
representation "\73 " and "\53 " respectively when found within a
sequence like </style>. We have to be careful to support casing here
just like with the script closing tag regex for bootstrap scripts.

DiffTrain build for [aead514](aead514)
gnoff added a commit that referenced this pull request Apr 19, 2024
stacked on #28870 

inline script children have been encoded as HTML for a while now but
this can easily break script parsing so practically if you were
rendering inline scripts you were using dangerouslySetInnerHTML. This is
not great because now there is no escaping at all so you have to be even
more careful. While care should always be taken when rendering untrusted
script content driving users to use dangerous APIs is not the right
approach and in this PR the escaping functionality used for
bootstrapScripts and importMaps is being extended to any inline script.

the approach is to escape 's' or 'S" with the appropriate unicode code
point if it is inside a <script or </script sequence. This has the nice
benefit of minimally escaping the text for readability while still
preserving full js parsing capabilities. As articulated when we
introduced this escaping for prior use cases this is only safe because
we are escaping the entire script content. It would be unsafe if we were
not escaping the entirety of the script because we would no longer be
able to ensure there are no earlier or later <script sequences that put
the parser in unexpected states.
github-actions bot pushed a commit that referenced this pull request Apr 19, 2024
stacked on #28870

inline script children have been encoded as HTML for a while now but
this can easily break script parsing so practically if you were
rendering inline scripts you were using dangerouslySetInnerHTML. This is
not great because now there is no escaping at all so you have to be even
more careful. While care should always be taken when rendering untrusted
script content driving users to use dangerous APIs is not the right
approach and in this PR the escaping functionality used for
bootstrapScripts and importMaps is being extended to any inline script.

the approach is to escape 's' or 'S" with the appropriate unicode code
point if it is inside a <script or </script sequence. This has the nice
benefit of minimally escaping the text for readability while still
preserving full js parsing capabilities. As articulated when we
introduced this escaping for prior use cases this is only safe because
we are escaping the entire script content. It would be unsafe if we were
not escaping the entirety of the script because we would no longer be
able to ensure there are no earlier or later <script sequences that put
the parser in unexpected states.

DiffTrain build for [561c023](561c023)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed React Core Team Opened by a member of the React Core Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants