From 468cea793fa41be2928adff6c317d8e52f6f5cde Mon Sep 17 00:00:00 2001 From: Stephanie Ding Date: Wed, 27 Dec 2023 23:55:27 -0800 Subject: [PATCH] [Trusted Types] Don't stringify DOM attributes (#19588 redo) Summary: This is a resubmit of #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/react#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. --- .../src/client/DOMPropertyOperations.js | 21 ++------ .../src/client/ReactDOMComponent.js | 20 +++---- .../__tests__/trustedTypes-test.internal.js | 53 +++++++++++++++++-- 3 files changed, 60 insertions(+), 34 deletions(-) diff --git a/packages/react-dom-bindings/src/client/DOMPropertyOperations.js b/packages/react-dom-bindings/src/client/DOMPropertyOperations.js index ad14b6498c835..42ae4b0775fef 100644 --- a/packages/react-dom-bindings/src/client/DOMPropertyOperations.js +++ b/packages/react-dom-bindings/src/client/DOMPropertyOperations.js @@ -8,10 +8,7 @@ */ import isAttributeNameSafe from '../shared/isAttributeNameSafe'; -import { - enableTrustedTypesIntegration, - enableCustomElementPropertySupport, -} from 'shared/ReactFeatureFlags'; +import {enableCustomElementPropertySupport} from 'shared/ReactFeatureFlags'; import {checkAttributeStringCoercion} from 'shared/CheckStringCoercion'; import {getFiberCurrentPropsFromNode} from './ReactDOMComponentTree'; @@ -133,10 +130,7 @@ export function setValueForAttribute( if (__DEV__) { checkAttributeStringCoercion(value, name); } - node.setAttribute( - name, - enableTrustedTypesIntegration ? (value: any) : '' + (value: any), - ); + node.setAttribute(name, (value: any)); } } @@ -161,10 +155,7 @@ export function setValueForKnownAttribute( if (__DEV__) { checkAttributeStringCoercion(value, name); } - node.setAttribute( - name, - enableTrustedTypesIntegration ? (value: any) : '' + (value: any), - ); + node.setAttribute(name, (value: any)); } export function setValueForNamespacedAttribute( @@ -189,11 +180,7 @@ export function setValueForNamespacedAttribute( if (__DEV__) { checkAttributeStringCoercion(value, name); } - node.setAttributeNS( - namespace, - name, - enableTrustedTypesIntegration ? (value: any) : '' + (value: any), - ); + node.setAttributeNS(namespace, name, (value: any)); } export function setValueForPropertyOnCustomComponent( diff --git a/packages/react-dom-bindings/src/client/ReactDOMComponent.js b/packages/react-dom-bindings/src/client/ReactDOMComponent.js index a94c990733915..27f07139d846a 100644 --- a/packages/react-dom-bindings/src/client/ReactDOMComponent.js +++ b/packages/react-dom-bindings/src/client/ReactDOMComponent.js @@ -70,7 +70,6 @@ import { enableClientRenderFallbackOnTextMismatch, enableFormActions, disableIEWorkarounds, - enableTrustedTypesIntegration, enableFilterEmptyStringAttributesDOM, } from 'shared/ReactFeatureFlags'; import { @@ -473,9 +472,8 @@ function setProp( if (__DEV__) { checkAttributeStringCoercion(value, key); } - const sanitizedValue = (sanitizeURL( - enableTrustedTypesIntegration ? value : '' + (value: any), - ): any); + const attributeValue = (value: any); + const sanitizedValue = (sanitizeURL(attributeValue): any); domElement.setAttribute(key, sanitizedValue); break; } @@ -561,9 +559,8 @@ function setProp( if (__DEV__) { checkAttributeStringCoercion(value, key); } - const sanitizedValue = (sanitizeURL( - enableTrustedTypesIntegration ? value : '' + (value: any), - ): any); + const attributeValue = (value: any); + const sanitizedValue = (sanitizeURL(attributeValue): any); domElement.setAttribute(key, sanitizedValue); break; } @@ -662,9 +659,7 @@ function setProp( if (__DEV__) { checkAttributeStringCoercion(value, key); } - const sanitizedValue = (sanitizeURL( - enableTrustedTypesIntegration ? value : '' + (value: any), - ): any); + const sanitizedValue = (sanitizeURL((value: any)): any); domElement.setAttributeNS(xlinkNamespace, 'xlink:href', sanitizedValue); break; } @@ -690,10 +685,7 @@ function setProp( if (__DEV__) { checkAttributeStringCoercion(value, key); } - domElement.setAttribute( - key, - enableTrustedTypesIntegration ? (value: any) : '' + (value: any), - ); + domElement.setAttribute(key, (value: any)); } else { domElement.removeAttribute(key); } diff --git a/packages/react-dom/src/client/__tests__/trustedTypes-test.internal.js b/packages/react-dom/src/client/__tests__/trustedTypes-test.internal.js index 777ef41756ce4..c0c018105f15b 100644 --- a/packages/react-dom/src/client/__tests__/trustedTypes-test.internal.js +++ b/packages/react-dom/src/client/__tests__/trustedTypes-test.internal.js @@ -16,11 +16,12 @@ describe('when Trusted Types are available in global object', () => { let container; let ttObject1; let ttObject2; + let fakeTTObjects; beforeEach(() => { jest.resetModules(); container = document.createElement('div'); - const fakeTTObjects = new Set(); + fakeTTObjects = new Set(); window.trustedTypes = { isHTML: function (value) { if (this !== window.trustedTypes) { @@ -28,11 +29,12 @@ describe('when Trusted Types are available in global object', () => { } return fakeTTObjects.has(value); }, - isScript: () => false, - isScriptURL: () => false, + isScript: value => fakeTTObjects.has(value), + isScriptURL: value => fakeTTObjects.has(value), }; ReactFeatureFlags = require('shared/ReactFeatureFlags'); ReactFeatureFlags.enableTrustedTypesIntegration = true; + ReactFeatureFlags.disableJavaScriptURLs = true; React = require('react'); ReactDOM = require('react-dom'); ttObject1 = { @@ -152,6 +154,51 @@ describe('when Trusted Types are available in global object', () => { } }); + it('should not stringify attributes that go through sanitizeURL', () => { + const setAttribute = Element.prototype.setAttribute; + try { + const setAttributeCalls = []; + Element.prototype.setAttribute = function (name, value) { + setAttributeCalls.push([this, name.toLowerCase(), value]); + return setAttribute.apply(this, arguments); + }; + const trustedScriptUrlHttps = { + toString: () => 'https://ok.example', + }; + fakeTTObjects.add(trustedScriptUrlHttps); + // It's not a matching type (under Trusted Types a.href a string will do), + // but a.href undergoes URL and we're only testing if the value was + // passed unmodified to setAttribute. + ReactDOM.render(, container); + expect(setAttributeCalls.length).toBe(1); + expect(setAttributeCalls[0][0]).toBe(container.firstChild); + expect(setAttributeCalls[0][1]).toBe('href'); + // Ensure it didn't get stringified when passed to a DOM sink: + expect(setAttributeCalls[0][2]).toBe(trustedScriptUrlHttps); + } finally { + Element.prototype.setAttribute = setAttribute; + } + }); + + it('should sanitize attributes even if they are Trusted Types', () => { + const setAttribute = Element.prototype.setAttribute; + try { + const trustedScriptUrlJavascript = { + // eslint-disable-next-line no-script-url + toString: () => 'javascript:notfine', + }; + fakeTTObjects.add(trustedScriptUrlJavascript); + // Assert that the URL sanitization will correctly unwrap and verify the + // value. + ReactDOM.render(, container); + expect(container.innerHTML).toBe( + '', + ); + } finally { + Element.prototype.setAttribute = setAttribute; + } + }); + it('should not stringify trusted values for setAttributeNS', () => { const setAttributeNS = Element.prototype.setAttributeNS; try {