Skip to content

Commit 740d977

Browse files
committed
[Trusted Types] Don't stringify DOM attributes (facebook#19588 redo)
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.
1 parent c5b9375 commit 740d977

File tree

3 files changed

+70
-34
lines changed

3 files changed

+70
-34
lines changed

packages/react-dom-bindings/src/client/DOMPropertyOperations.js

Lines changed: 4 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -8,10 +8,7 @@
88
*/
99

1010
import isAttributeNameSafe from '../shared/isAttributeNameSafe';
11-
import {
12-
enableTrustedTypesIntegration,
13-
enableCustomElementPropertySupport,
14-
} from 'shared/ReactFeatureFlags';
11+
import {enableCustomElementPropertySupport} from 'shared/ReactFeatureFlags';
1512
import {checkAttributeStringCoercion} from 'shared/CheckStringCoercion';
1613
import {getFiberCurrentPropsFromNode} from './ReactDOMComponentTree';
1714

@@ -133,10 +130,7 @@ export function setValueForAttribute(
133130
if (__DEV__) {
134131
checkAttributeStringCoercion(value, name);
135132
}
136-
node.setAttribute(
137-
name,
138-
enableTrustedTypesIntegration ? (value: any) : '' + (value: any),
139-
);
133+
node.setAttribute(name, (value: any));
140134
}
141135
}
142136

@@ -161,10 +155,7 @@ export function setValueForKnownAttribute(
161155
if (__DEV__) {
162156
checkAttributeStringCoercion(value, name);
163157
}
164-
node.setAttribute(
165-
name,
166-
enableTrustedTypesIntegration ? (value: any) : '' + (value: any),
167-
);
158+
node.setAttribute(name, (value: any));
168159
}
169160

170161
export function setValueForNamespacedAttribute(
@@ -189,11 +180,7 @@ export function setValueForNamespacedAttribute(
189180
if (__DEV__) {
190181
checkAttributeStringCoercion(value, name);
191182
}
192-
node.setAttributeNS(
193-
namespace,
194-
name,
195-
enableTrustedTypesIntegration ? (value: any) : '' + (value: any),
196-
);
183+
node.setAttributeNS(namespace, name, (value: any));
197184
}
198185

199186
export function setValueForPropertyOnCustomComponent(

packages/react-dom-bindings/src/client/ReactDOMComponent.js

Lines changed: 6 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -70,7 +70,6 @@ import {
7070
enableClientRenderFallbackOnTextMismatch,
7171
enableFormActions,
7272
disableIEWorkarounds,
73-
enableTrustedTypesIntegration,
7473
enableFilterEmptyStringAttributesDOM,
7574
} from 'shared/ReactFeatureFlags';
7675
import {
@@ -473,9 +472,8 @@ function setProp(
473472
if (__DEV__) {
474473
checkAttributeStringCoercion(value, key);
475474
}
476-
const sanitizedValue = (sanitizeURL(
477-
enableTrustedTypesIntegration ? value : '' + (value: any),
478-
): any);
475+
const attributeValue = (value: any);
476+
const sanitizedValue = (sanitizeURL(attributeValue): any);
479477
domElement.setAttribute(key, sanitizedValue);
480478
break;
481479
}
@@ -561,9 +559,8 @@ function setProp(
561559
if (__DEV__) {
562560
checkAttributeStringCoercion(value, key);
563561
}
564-
const sanitizedValue = (sanitizeURL(
565-
enableTrustedTypesIntegration ? value : '' + (value: any),
566-
): any);
562+
const attributeValue = (value: any);
563+
const sanitizedValue = (sanitizeURL(attributeValue): any);
567564
domElement.setAttribute(key, sanitizedValue);
568565
break;
569566
}
@@ -662,9 +659,7 @@ function setProp(
662659
if (__DEV__) {
663660
checkAttributeStringCoercion(value, key);
664661
}
665-
const sanitizedValue = (sanitizeURL(
666-
enableTrustedTypesIntegration ? value : '' + (value: any),
667-
): any);
662+
const sanitizedValue = (sanitizeURL((value: any)): any);
668663
domElement.setAttributeNS(xlinkNamespace, 'xlink:href', sanitizedValue);
669664
break;
670665
}
@@ -690,10 +685,7 @@ function setProp(
690685
if (__DEV__) {
691686
checkAttributeStringCoercion(value, key);
692687
}
693-
domElement.setAttribute(
694-
key,
695-
enableTrustedTypesIntegration ? (value: any) : '' + (value: any),
696-
);
688+
domElement.setAttribute(key, (value: any));
697689
} else {
698690
domElement.removeAttribute(key);
699691
}

packages/react-dom/src/client/__tests__/trustedTypes-test.internal.js

Lines changed: 60 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -16,23 +16,37 @@ describe('when Trusted Types are available in global object', () => {
1616
let container;
1717
let ttObject1;
1818
let ttObject2;
19+
let fakeTTObjects;
20+
21+
const expectToReject = fn => {
22+
let msg;
23+
try {
24+
fn();
25+
} catch (x) {
26+
msg = x.message;
27+
}
28+
expect(msg).toContain(
29+
'React has blocked a javascript: URL as a security precaution.',
30+
);
31+
};
1932

2033
beforeEach(() => {
2134
jest.resetModules();
2235
container = document.createElement('div');
23-
const fakeTTObjects = new Set();
36+
fakeTTObjects = new Set();
2437
window.trustedTypes = {
2538
isHTML: function (value) {
2639
if (this !== window.trustedTypes) {
2740
throw new Error(this);
2841
}
2942
return fakeTTObjects.has(value);
3043
},
31-
isScript: () => false,
32-
isScriptURL: () => false,
44+
isScript: value => fakeTTObjects.has(value),
45+
isScriptURL: value => fakeTTObjects.has(value),
3346
};
3447
ReactFeatureFlags = require('shared/ReactFeatureFlags');
3548
ReactFeatureFlags.enableTrustedTypesIntegration = true;
49+
ReactFeatureFlags.disableJavaScriptURLs = true;
3650
React = require('react');
3751
ReactDOM = require('react-dom');
3852
ttObject1 = {
@@ -152,6 +166,49 @@ describe('when Trusted Types are available in global object', () => {
152166
}
153167
});
154168

169+
it('should not stringify attributes that go through sanitizeURL', () => {
170+
const setAttribute = Element.prototype.setAttribute;
171+
try {
172+
const setAttributeCalls = [];
173+
Element.prototype.setAttribute = function (name, value) {
174+
setAttributeCalls.push([this, name.toLowerCase(), value]);
175+
return setAttribute.apply(this, arguments);
176+
};
177+
const trustedScriptUrlHttps = {
178+
toString: () => 'https://ok.example',
179+
};
180+
fakeTTObjects.add(trustedScriptUrlHttps);
181+
// It's not a matching type (under Trusted Types a.href a string will do),
182+
// but a.href undergoes URL and we're only testing if the value was
183+
// passed unmodified to setAttribute.
184+
ReactDOM.render(<a href={trustedScriptUrlHttps} />, container);
185+
expect(setAttributeCalls.length).toBe(1);
186+
expect(setAttributeCalls[0][0]).toBe(container.firstChild);
187+
expect(setAttributeCalls[0][1]).toBe('href');
188+
// Ensure it didn't get stringified when passed to a DOM sink:
189+
expect(setAttributeCalls[0][2]).toBe(trustedScriptUrlHttps);
190+
} finally {
191+
Element.prototype.setAttribute = setAttribute;
192+
}
193+
});
194+
195+
it('should sanitize attributes even if they are Trusted Types', () => {
196+
const setAttribute = Element.prototype.setAttribute;
197+
try {
198+
const trustedScriptUrlJavascript = {
199+
// eslint-disable-next-line no-script-url
200+
toString: () => 'javascript:notfine',
201+
};
202+
fakeTTObjects.add(trustedScriptUrlJavascript);
203+
// Assert that the URL sanitization will correctly unwrap and verify the
204+
// value.
205+
ReactDOM.render(<a href={trustedScriptUrlJavascript} />, container);
206+
expect(container.innerHTML).toBe('<a href="javascript:throw new Error(\'React has blocked a javascript: URL as a security precaution.\')"></a>');
207+
} finally {
208+
Element.prototype.setAttribute = setAttribute;
209+
}
210+
});
211+
155212
it('should not stringify trusted values for setAttributeNS', () => {
156213
const setAttributeNS = Element.prototype.setAttributeNS;
157214
try {

0 commit comments

Comments
 (0)