Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Trusted Types] Don't stringify DOM attributes (#19588 redo) #27859

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 4 additions & 17 deletions packages/react-dom-bindings/src/client/DOMPropertyOperations.js
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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