Add trusted types to react on client side #16157
Conversation
Details of bundled changes.Comparing: fc80772...eb1a123 react-dom
Generated by 🚫 dangerJS |
|
Thanks for the PR!
I think that behavior was always there. It's not new since Oct 12. This is because in the past, we always used
I'm not following.. Does this mean IE has support for real Trusted Types? Is there any version of IE where this would matter? |
|
@gaearon I have completed the changes, can you have a look? |
|
I will check on Monday, thank you! |
|
Should we put it behind a feature flag? See |
|
How can clients use feature flags outside of FB? I assume a rebuild of React is needed to have one's own flag set (based on this comment? We can put it behind a flag, but so far the changes look quite inobtrusive (it's only removing stringification for sinks, warning about SVG interpolation in IE and |
You'd need to do a build yourself, yes.
A flag is a safer option because if something is wrong, we can disable it instead of reverting the whole PR to unblock other work. We use flags for all non-trivial changes, and this one is a bit non-trivial. I don't mean a long-lived flag, but just for a few weeks so we can start testing this safely without worrying about it affecting release timing. |
There was a problem hiding this comment.
- Let's add deduplication to the warning. It shouldn't warn more than once.
- Let's remove the
typeof TrustedTypescheck (PascalCase) we accidentally left in. - Let's create a feature flag and put all new logic behind it. When it's false, we should behave identically to how we used to. See any flag in
ReactFeatureFlagsand its usage as an example. You'll need to rename test totrustedTypes-test.internal.jsand dorequire('ReactFeatureFlags')there like in other tests to set the flag.
Thank you!
Details of bundled changes.Comparing: 8f03109...941ec84 react-dom
|
| trustedTypes.isHTML(value) || | ||
| trustedTypes.isScript(value) || | ||
| trustedTypes.isScriptURL(value) || | ||
| // TrustedURLs are deprecated and will be removed soon: https://github.com/WICG/trusted-types/pull/204 |
There was a problem hiding this comment.
Do we even want to support them then?
There was a problem hiding this comment.
I think at least for now we shouldn't. Both chrome and the polyfill currently support them and if we don't, trying out the integration will for the users will be harder (because the values will be stringified). They would need to create a default policy that would bless all URLs.
|
This is getting close. Exciting! One more thing I’d like to ask you is to compare Also, do we have a server rendering story? |
There was a problem hiding this comment.
I compared the build using diff utility using and it looks pretty similar. Here is the result of the comparison.
There is also a server rendering draft PR. However, the reason of it being a draft was that getProprtyType function defintion might have changed at that time. This has been resolved and the function definition is going to remain the same. However, currently the polyfill doesn't work in nodejs. But I have already fired a PR will make it work.
So basically, the implementation of the PR is ready and can be reviewed.
packages/react-dom/src/client/__tests__/trustedTypes-test.internal.js
Outdated
Show resolved
Hide resolved
|
Resolved the minors :) looking forward to 🚢 |
|
Looks good, thanks. |
…26016) ## Summary The flag was first added in #16157 and was rolled out to employees in D17430095. #25997 removed this flag because it wasn't dynamically set to a value in www. The www side was mistakenly removed in D41851685 due to deprecation of a TypedJSModule but we still want to keep this flag, so let's add it back in + add a GK on the www side to match the previous rollout. See D42574435 for the dynamic value change in www ## How did you test this change? ``` yarn test yarn test --prod ```
…26016) ## Summary The flag was first added in #16157 and was rolled out to employees in D17430095. #25997 removed this flag because it wasn't dynamically set to a value in www. The www side was mistakenly removed in D41851685 due to deprecation of a TypedJSModule but we still want to keep this flag, so let's add it back in + add a GK on the www side to match the previous rollout. See D42574435 for the dynamic value change in www ## How did you test this change? ``` yarn test yarn test --prod ``` DiffTrain build for [48b687f](48b687f) [View git log for this commit](https://github.com/facebook/react/commits/48b687fc95a172cec8f305312a27d105e5719581)
Trusted Types
Trusted Types (spec, introductory article) is a new experimental DOM API implemented within the WICG , with a working Chrome implementation.
The API creates a few new objects available on the global object in the browser, like most other web APIs (impl in TS and in Closure compiler).
Under certain conditions, controlled by a HTTP header (analogous to Content-Security-Policy behavior), the API can enable the enforcement - then it changes the signature of several DOM API functions and property setters, such that they accept specific object types, and reject strings. Colloquially, DOM API becomes strongly typed.
For example, with Trusted Types Element.innerHTML property setter accepts a TrustedHTML object.
Trusted Type objects stringify to their inner value. This API shape is a deliberate choice that enables existing web applications and libraries to gradually migrate from strings to Trusted Types without breaking functionality. In our example, it makes it possible to write the following:
The above code works regardless if the Trusted Types enforcement is enabled or not.
Reading from the DOM is unaffected, so Element.innerHTML getter returns a string. That's for practical reasons -- web applications read from DOM more often than they write to it, and only writing exposes the application to DOM XSS risks. Typing only the setters allows us to secure web applications with minimal code changes.
Adding Trusted Types to React for client side
React applications rarely manipulate DOM directly as this is what React is for. That means that, if users want to opt in to Trusted Types, values assigned to execution sinks must be trusted. React on its own doesn't produce these execution sinks, except one hack for IE. The only problem in React is the stringifying of values before assigning them to element properties or attributes.
This PR enables applications to use Trusted Types on client side. The implementation removes stringifying of trusted values in reactDOM/client. It does so only when Trusted Types are available in global object. If they are available, there are special is* functions which are used to check if the value is trusted type or not. If the value is a trusted type, we return the value itself, otherwise we preserve the original logic and stringify the value.
Problems
We cannot use Trusted Types in Internet Explorer (IE) when using dangerouslySetInnerHTML on svg elements - The cause is that in IE svg elements doesn't have innerHTML property, which means that to support innerHTML, react needs to use a hack, which wraps the innerHTML of the svg and wraps them in an svg element and assigns this to innerHTML of some HTML element. This is a problem for Trusted Types because the original svg innerHTML value is stringified. There are a few workarounds for this:
a) Use default Trusted Type policy allowing creating svg nodes
b) Do not use svg with dangerouslySetInnerHTML but wrap the svg with div element and use dangerouslySetInnerHTML there
Reference
Update