-
Notifications
You must be signed in to change notification settings - Fork 46.3k
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
Escape bootstrapScriptContent for javascript embedding into HTML #24385
Conversation
11ea57a
to
be5bc4d
Compare
Comparing: 726ba80...b8e4253 Critical size changesIncludes critical production bundles, as well as any change greater than 2%:
Significant size changesIncludes any change greater than 0.2%: Expand to show
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some nits but the essence looks good.
|
||
const scriptRegex = /(<\/|<)(s)(cript)/gi; | ||
const scriptReplacer = (match, prefix, s, suffix) => | ||
`${prefix}${substitutions[s]}${suffix}`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's make this an inline condition instead of going through a whole hidden class check and fake-map look up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant s === 's' ? '\\u0073' : '\\u0053'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
packages/react-dom/src/__tests__/escapeScriptForBrowser-test.js
Outdated
Show resolved
Hide resolved
packages/react-dom/src/__tests__/escapeScriptForBrowser-test.js
Outdated
Show resolved
Hide resolved
5a3e000
to
7dd5cc7
Compare
The previous escape was for Text into HTML and breaks script contents. The new escaping ensures that the script contents cannot prematurely close the host script tag by escaping script open and close string sequences using a unicode escape substitution.
7dd5cc7
to
11c06bf
Compare
the escaping of this function does is tailored to the specific use case of how bootstrapScriptContent is currently set up and having it be a module suggests it is meant for a more general than it has been considered for. Additionally the tests were redone to focus on practical implications for what is and is not escaped
11c06bf
to
52786bf
Compare
const scriptReplacer = (match, prefix, s, suffix) => | ||
`${prefix}${s === 's' ? '\\u0073' : '\\u0053'}${suffix}`; | ||
|
||
function escapeBootstrapScriptContent(scriptText) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe a leave a comment with the rationale and clarifying that this only works if the whole content is encoded and if it's an actual JS script tag, and doesn't work for attributes or CSS.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
The previous escape was for Text into HTML and breaks script contents. The new escaping ensures that the script contents cannot prematurely close the host script tag by escaping script open and close string sequences using a unicode escape substitution.
closes: #23063