Skip to content

Fix escaping in ReactDOMInput code #26630

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

Merged
merged 3 commits into from
Apr 24, 2023
Merged

Conversation

sophiebits
Copy link
Collaborator

JSON.stringify isn't the right thing here. Luckily this doesn't look to have any security impact.

JSON.stringify isn't the right thing here. Luckily this doesn't look to have any security impact.
@sophiebits sophiebits requested review from sebmarkbage and gnoff April 15, 2023 00:42
@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Apr 15, 2023
@react-sizebot
Copy link

react-sizebot commented Apr 15, 2023

Comparing: d121c67...a4b9f9c

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 = 164.42 kB 164.42 kB = 51.69 kB 51.69 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js = 166.86 kB 166.86 kB +0.02% 52.34 kB 52.35 kB
facebook-www/ReactDOM-prod.classic.js +0.05% 564.45 kB 564.75 kB +0.05% 99.40 kB 99.45 kB
facebook-www/ReactDOM-prod.modern.js +0.05% 548.24 kB 548.53 kB +0.05% 96.71 kB 96.75 kB

Significant size changes

Includes any change greater than 0.2%:

(No significant changes)

Generated by 🚫 dangerJS against a4b9f9c

jsdom's implementation of ~= is buggy (just filed dperini/nwsapi#84) -- but = is a better fit here regardless since href is not a space-separated list of values.
Copy link
Collaborator

@gnoff gnoff left a comment

Choose a reason for hiding this comment

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

Hmm I thought the space would be consumed by the escape sequence. The selector can’t change, we rely on an invariant that hrefs for float style tags and style sheet link tags do not have spaces in them. This is because we coalesce multiple style tags into a single flushed tag and use this space separated selector to look for a match.

I wouldn’t have thought the escape change would affect this, shouldn’t the new space be consumed?

@sophiebits
Copy link
Collaborator Author

sophiebits commented Apr 15, 2023

(Thanks for looking – was going to tag you on Monday before merging.)

Yes. It's a bug in the selector library used by jsdom that filed an issue for this morning: dperini/nwsapi#84. I confirmed that ~= with an escape works correctly in Blink, WebKit, and Gecko. So arguably we shouldn't need to change the actual code to make the test work, but I figured href= seemed equally or more correct ¯\_(ツ)_/¯. I think this new code is correct since I'm still using data-href~= but lmk if you don't think so.

@sophiebits
Copy link
Collaborator Author

@gnoff thoughts?

Copy link
Collaborator

@gnoff gnoff left a comment

Choose a reason for hiding this comment

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

I'm sorry I lost the thread. You're right (of course :))

I would like to refactor against the notion of key <> selector now that they aren't really one to one. But agree this does preserver the style selector semantics

@sophiebits
Copy link
Collaborator Author

no worries! thanks for looking

@sophiebits sophiebits merged commit 9ee7964 into facebook:main Apr 24, 2023
EdisonVan pushed a commit to EdisonVan/react that referenced this pull request Apr 15, 2024
JSON.stringify isn't the right thing here. Luckily this doesn't look to have any security impact.
bigfootjon pushed a commit that referenced this pull request Apr 18, 2024
JSON.stringify isn't the right thing here. Luckily this doesn't look to have any security impact.

DiffTrain build for commit 9ee7964.
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