DOM attribute stringification fixes #19587
Labels
Comments
|
We briefly chatted about this with @sebmarkbage and he suggested it's still a bit too risky to put in 17.0 (for which we have an RC) but might be okay to put in 17.1 if we're confident it's not a breaking change and if there is exhaustive browser testing. |
|
No worries. In the meantime I found some interactions of this change with the |
koto
added a commit
to koto/react
that referenced
this issue
Aug 11, 2020
… stringify attributes), instead of Trusted Types feature flag. Added fixture tests for the logic. For attributes undergoing sanitizeURL, the value is stringified in sanitizeURL function, unless enableTrustedTypesIntegration is true and the value is and immutable TrustedScriptURL value. This ascertains that objects with custom toString() function cannot be used to bypass the sanitization (now that DOMPropertyOperations don't stringify on their own). Fixes facebook#19587.
koto
added a commit
to koto/react
that referenced
this issue
Aug 11, 2020
… stringify attributes), instead of Trusted Types feature flag. Added fixture tests for the logic. For attributes undergoing sanitizeURL, the value is stringified in sanitizeURL function, unless enableTrustedTypesIntegration is true and the value is and immutable TrustedScriptURL value. This ascertains that objects with custom toString() function cannot be used to bypass the sanitization (now that DOMPropertyOperations don't stringify on their own). Fixes facebook#19587.
koto
added a commit
to koto/react
that referenced
this issue
Aug 11, 2020
… stringify attributes), instead of Trusted Types feature flag. Added fixture tests for the logic. For attributes undergoing sanitizeURL, the value is stringified in sanitizeURL function, unless enableTrustedTypesIntegration is true and the value is and immutable TrustedScriptURL value. This ascertains that objects with custom toString() function cannot be used to bypass the sanitization (now that DOMPropertyOperations don't stringify on their own). Fixes facebook#19587.
koto
added a commit
to koto/react
that referenced
this issue
Aug 11, 2020
… stringify attributes), instead of Trusted Types feature flag. Added fixture tests for the logic. For attributes undergoing sanitizeURL, the value is stringified in sanitizeURL function, unless enableTrustedTypesIntegration is true and the value is and immutable TrustedScriptURL value. This ascertains that objects with custom toString() function cannot be used to bypass the sanitization (now that DOMPropertyOperations don't stringify on their own). Fixes facebook#19587.
|
This issue has been automatically marked as stale. If this issue is still affecting you, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize it yet. If you have any new additional information, please include it with your comment! |
|
bump, looking for an answer to #19588 (comment). |
|
Friendly ping, cc @gaearon. This would probably need rebasing, but the open question is whether we should go with:
I'm happy to help with whatever's needed for either of the options, but the call on which option to take is yours :) FWIW, we just merged webpack 5 TT support which leaves only a tiny blocker and this issue for create-react-app apps to be TT compliant by default. |
|
From #19588:
It's ok to drop IE9 by this point. If you have a fix that only breaks something in IE9 without adding extra logic let's merge it. |
koto
added a commit
to koto/react
that referenced
this issue
Jun 15, 2021
… stringify attributes), instead of Trusted Types feature flag. Added fixture tests for the logic. For attributes undergoing sanitizeURL, the value is stringified in sanitizeURL function, unless enableTrustedTypesIntegration is true and the value is and immutable TrustedScriptURL value. This ascertains that objects with custom toString() function cannot be used to bypass the sanitization (now that DOMPropertyOperations don't stringify on their own). Fixes facebook#19587.
Just done in #19588, it's ready for review. |
|
@koto great work. This will be a huge addition to securing React. |
|
The PR was closed unexpected. It there any plan to implement it? |
onionymous
added a commit
to onionymous/react
that referenced
this issue
Dec 28, 2023
Summary: This is a resubmit of facebook#19588. Fixes facebook#19587.
onionymous
added a commit
to onionymous/react
that referenced
this issue
Dec 28, 2023
Summary: This is a resubmit of facebook#19588. Fixes facebook#19587.
onionymous
added a commit
to onionymous/react
that referenced
this issue
Dec 28, 2023
Summary: This is a resubmit of facebook#19588 since it was never merged and closed in error. However, the issue it fixes is still relevant and will unblock rollout of the TT feature flag internally. Copying the original PR message here: ----- Instead of using Trusted Types feature flag, assume that the browser would stringify the attributes, so that React-DOM doesn't have to (making interpolation into node attributes "just work" regardless of the Trusted Types enforcement and availability) . Currently only IE<=9 does not stringify attributes. This PR implicitly drops the support for IE 9. For attributes undergoing sanitizeURL, the value is stringified in sanitizeURL function, unless enableTrustedTypesIntegration is true and the value is an immutable TrustedScriptURL value. This ascertains that objects with custom toString() function cannot be used to bypass the sanitization (now that DOMPropertyOperations don't stringify on their own). Fixes facebook#19587. ## Summary The PR decouples the attribute stringification from the Trusted Types logic, dropping the former completely. It was only used as a workaround for a IE <=9 browser bug. A small improvement for Trusted Types would be that it moves the most important functionality from behind the flag - i.e. allows most React apps to pass trusted types into DOM sinks without enabling the feature flag. Some rare functionality and dev warnings are still gated on the flag, but those are unrelated to the stringification issue. ## Test Plan Appropriate tests are added.
onionymous
added a commit
to onionymous/react
that referenced
this issue
Dec 28, 2023
Summary: This is a resubmit of facebook#19588 since it was never merged and closed in error. However, the issue it fixes is still relevant and will unblock rollout of the TT feature flag internally. Copying the original PR message here: ----- Instead of using Trusted Types feature flag, assume that the browser would stringify the attributes, so that React-DOM doesn't have to (making interpolation into node attributes "just work" regardless of the Trusted Types enforcement and availability) . Currently only IE<=9 does not stringify attributes. This PR implicitly drops the support for IE 9. For attributes undergoing sanitizeURL, the value is stringified in sanitizeURL function, unless enableTrustedTypesIntegration is true and the value is an immutable TrustedScriptURL value. This ascertains that objects with custom toString() function cannot be used to bypass the sanitization (now that DOMPropertyOperations don't stringify on their own). Fixes facebook#19587. ## Summary The PR decouples the attribute stringification from the Trusted Types logic, dropping the former completely. It was only used as a workaround for a IE <=9 browser bug. A small improvement for Trusted Types would be that it moves the most important functionality from behind the flag - i.e. allows most React apps to pass trusted types into DOM sinks without enabling the feature flag. Some rare functionality and dev warnings are still gated on the flag, but those are unrelated to the stringification issue. ## Test Plan Appropriate tests are added.
onionymous
added a commit
to onionymous/react
that referenced
this issue
Dec 28, 2023
Summary: This is a resubmit of facebook#19588 since it was never merged and closed in error. However, the issue it fixes is still relevant and will unblock rollout of the TT feature flag internally. Copying the original PR message here: ----- Instead of using Trusted Types feature flag, assume that the browser would stringify the attributes, so that React-DOM doesn't have to (making interpolation into node attributes "just work" regardless of the Trusted Types enforcement and availability) . Currently only IE<=9 does not stringify attributes. This PR implicitly drops the support for IE 9. For attributes undergoing sanitizeURL, the value is stringified in sanitizeURL function, unless enableTrustedTypesIntegration is true and the value is an immutable TrustedScriptURL value. This ascertains that objects with custom toString() function cannot be used to bypass the sanitization (now that DOMPropertyOperations don't stringify on their own). Fixes facebook#19587. ## Summary The PR decouples the attribute stringification from the Trusted Types logic, dropping the former completely. It was only used as a workaround for a IE <=9 browser bug. A small improvement for Trusted Types would be that it moves the most important functionality from behind the flag - i.e. allows most React apps to pass trusted types into DOM sinks without enabling the feature flag. Some rare functionality and dev warnings are still gated on the flag, but those are unrelated to the stringification issue. ## Test Plan Appropriate tests are added.
onionymous
added a commit
to onionymous/react
that referenced
this issue
Dec 29, 2023
Summary: This is a resubmit of facebook#19588 since it was never merged and closed in error. However, the issue it fixes is still relevant and will unblock rollout of the TT feature flag internally. Copying the original PR message here: ----- Instead of using Trusted Types feature flag, assume that the browser would stringify the attributes, so that React-DOM doesn't have to (making interpolation into node attributes "just work" regardless of the Trusted Types enforcement and availability) . Currently only IE<=9 does not stringify attributes. This PR implicitly drops the support for IE 9. For attributes undergoing sanitizeURL, the value is stringified in sanitizeURL function, unless enableTrustedTypesIntegration is true and the value is an immutable TrustedScriptURL value. This ascertains that objects with custom toString() function cannot be used to bypass the sanitization (now that DOMPropertyOperations don't stringify on their own). Fixes facebook#19587. ## Summary The PR decouples the attribute stringification from the Trusted Types logic, dropping the former completely. It was only used as a workaround for a IE <=9 browser bug. A small improvement for Trusted Types would be that it moves the most important functionality from behind the flag - i.e. allows most React apps to pass trusted types into DOM sinks without enabling the feature flag. Some rare functionality and dev warnings are still gated on the flag, but those are unrelated to the stringification issue. ## Test Plan Appropriate tests are added.
onionymous
added a commit
to onionymous/react
that referenced
this issue
Dec 29, 2023
Summary: This is a resubmit of facebook#19588 since it was never merged and closed in error. However, the issue it fixes is still relevant and will unblock rollout of the TT feature flag internally. Copying the original PR message here: ----- Instead of using Trusted Types feature flag, assume that the browser would stringify the attributes, so that React-DOM doesn't have to (making interpolation into node attributes "just work" regardless of the Trusted Types enforcement and availability) . Currently only IE<=9 does not stringify attributes. This PR implicitly drops the support for IE 9. For attributes undergoing sanitizeURL, the value is stringified in sanitizeURL function, unless enableTrustedTypesIntegration is true and the value is an immutable TrustedScriptURL value. This ascertains that objects with custom toString() function cannot be used to bypass the sanitization (now that DOMPropertyOperations don't stringify on their own). Fixes facebook#19587. ## Summary The PR decouples the attribute stringification from the Trusted Types logic, dropping the former completely. It was only used as a workaround for a IE <=9 browser bug. A small improvement for Trusted Types would be that it moves the most important functionality from behind the flag - i.e. allows most React apps to pass trusted types into DOM sinks without enabling the feature flag. Some rare functionality and dev warnings are still gated on the flag, but those are unrelated to the stringification issue. ## Test Plan Appropriate tests are added.
onionymous
added a commit
to onionymous/react
that referenced
this issue
Dec 29, 2023
Summary: This is a resubmit of facebook#19588 since it was never merged and closed in error. However, the issue it fixes is still relevant and will unblock rollout of the TT feature flag internally. Copying the original PR message here: ----- Instead of using Trusted Types feature flag, assume that the browser would stringify the attributes, so that React-DOM doesn't have to (making interpolation into node attributes "just work" regardless of the Trusted Types enforcement and availability) . Currently only IE<=9 does not stringify attributes. This PR implicitly drops the support for IE 9. For attributes undergoing sanitizeURL, the value is stringified in sanitizeURL function, unless enableTrustedTypesIntegration is true and the value is an immutable TrustedScriptURL value. This ascertains that objects with custom toString() function cannot be used to bypass the sanitization (now that DOMPropertyOperations don't stringify on their own). Fixes facebook#19587. ## Summary The PR decouples the attribute stringification from the Trusted Types logic, dropping the former completely. It was only used as a workaround for a IE <=9 browser bug. A small improvement for Trusted Types would be that it moves the most important functionality from behind the flag - i.e. allows most React apps to pass trusted types into DOM sinks without enabling the feature flag. Some rare functionality and dev warnings are still gated on the flag, but those are unrelated to the stringification issue. ## Test Plan Appropriate tests are added.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
This is regarding the discussion in #17773.
React-DOM currently stringifies DOM attribute values before passing them to
Element.setAttribute(NS)functions. This might be unnecessary, as these functions implicitly stringify attribute values on their own (WebIDL attributes typed asDOMString). It also makes it difficult to enforce Trusted Types in React applications, as the trusted type objects would be stringified before values reach the DOM sinks.Currently there is a
enableTrustedTypesIntegrationfeature flag to disable stringification, but it seems like this behavior can be safely removed for modern browsers with no backwards-compatibility problems. Let me explain:Attribute stringification was introduced in b0455f4, at that time to workaround a jsdom limitation (jsdom's DOM emulation didn't stringify on its own). IE 8/9 have a similar issue. If an object is passed to a DOM attribute, its value becomes
[object], ignoring any stringification rules defined in objects'toStringfunction.setAttributefunction does stringify the values via its IDL layer (runkit demo).p.title, and not one with a custom name).I propose to remove the stringification (similar to #17774) unless a browser bug is detected.
That way there is no spurious stringification, and the code branches with the workaround can be removed once buggy browsers stop being supported. My testing shows that only IE9 is affected. The change would be backwards-compatible. I'll send a PR with the proposed change.
The text was updated successfully, but these errors were encountered: