DOM attribute stringification fixes

koto commented 4 years ago

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 as DOMString). 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 enableTrustedTypesIntegration feature 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 https://github.com/facebook/react/commit/b0455f46709fca94da0b6126b719d6dd07605e65, 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' toString function.

Jsdom does not have the issue anymore. Since at least 4.0.0 its setAttribute function does stringify the values via its IDL layer (runkit demo).
React doesn't support IE 8 anymore.
The issue still exists for IE9 (contrary to https://github.com/facebook/react/issues/11735, my tests confirm that the bug still exists, but one needs to try a standard attribute, like p.title, and not one with a custom name).
All other browsers, even in their old versions (I tested IEs, Firefox, Chrome, Safari, Opera and a few mobile browsers ) correctly stringify.

I propose to remove the stringification (similar to https://github.com/facebook/react/pull/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.

gaearon commented 4 years ago

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.

koto commented 4 years ago

No worries. In the meantime I found some interactions of this change with the javascript: URL sanitization that has to be gated on the TT feature flag to be secure. I'll send a PR once ready anyhow, such that we can identify the issues early on.

stale[bot] commented 3 years ago

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!

koto commented 3 years ago

bump, looking for an answer to https://github.com/facebook/react/pull/19588#issuecomment-737201199.

koto commented 3 years ago

Friendly ping, cc @gaearon. This would probably need rebasing, but the open question is whether we should go with:

the changes here - These practically unblock using react-DOM with Trusted Types interpolated into attributes without requiring enableTrustedTypesIntegration feature flag. I feel the change is also bw-compatible, as reasoned in the PR description, OR
enableTrustedTypesIntegration by default maybe? :)

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.

gaearon commented 3 years ago

From https://github.com/facebook/react/pull/19588: