Skip to content

Commit

Permalink
[Trusted Types] Don't stringify DOM attributes (facebook#19588 redo)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
onionymous committed Dec 29, 2023
1 parent c5b9375 commit 468cea7
Show file tree
Hide file tree
Showing 3 changed files with 60 additions and 34 deletions.
21 changes: 4 additions & 17 deletions packages/react-dom-bindings/src/client/DOMPropertyOperations.js
Expand Up @@ -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';

Expand Down Expand Up @@ -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));
}
}

Expand All @@ -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(
Expand All @@ -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(
Expand Down
20 changes: 6 additions & 14 deletions packages/react-dom-bindings/src/client/ReactDOMComponent.js
Expand Up @@ -70,7 +70,6 @@ import {
enableClientRenderFallbackOnTextMismatch,
enableFormActions,
disableIEWorkarounds,
enableTrustedTypesIntegration,
enableFilterEmptyStringAttributesDOM,
} from 'shared/ReactFeatureFlags';
import {
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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;
}
Expand All @@ -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);
}
Expand Down
Expand Up @@ -16,23 +16,25 @@ 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) {
throw new Error(this);
}
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 = {
Expand Down Expand Up @@ -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(<a href={trustedScriptUrlHttps} />, 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(<a href={trustedScriptUrlJavascript} />, container);
expect(container.innerHTML).toBe(
'<a href="javascript:throw new Error(\'React has blocked a javascript: URL as a security precaution.\')"></a>',
);
} finally {
Element.prototype.setAttribute = setAttribute;
}
});

it('should not stringify trusted values for setAttributeNS', () => {
const setAttributeNS = Element.prototype.setAttributeNS;
try {
Expand Down

0 comments on commit 468cea7

Please sign in to comment.