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

Add trusted types to react on client side #16157

Merged
merged 13 commits into from
Sep 16, 2019
Prev Previous commit
Next Next commit
Apply PR suggesstions
Emanuel Tesar committed Sep 3, 2019

Verified

This commit was signed with the committer’s verified signature. The key has expired.
addaleax Anna Henningsen
commit 9b1ec8c8f9fa552e7b1707cb16081aa29dd5de4b
1 change: 0 additions & 1 deletion .eslintrc.js
Original file line number Diff line number Diff line change
@@ -149,7 +149,6 @@ module.exports = {
spyOnProd: true,
__PROFILE__: true,
__UMD__: true,
TrustedTypes: true,
trustedTypes: true,
},
};
26 changes: 14 additions & 12 deletions packages/react-dom/src/__tests__/ReactEmptyComponent-test.js
Original file line number Diff line number Diff line change
@@ -136,8 +136,6 @@ describe('ReactEmptyComponent', () => {
});

it('should distinguish between a script placeholder and an actual script tag', () => {
const consoleSpy = spyOnDev(console, 'error');

const instance1 = (
<TogglingComponent firstComponent={null} secondComponent={'script'} />
);
@@ -147,10 +145,22 @@ describe('ReactEmptyComponent', () => {

expect(function() {
ReactTestUtils.renderIntoDocument(instance1);
}).not.toThrow();
}).toWarnDev(
'Warning: Encountered script tag while rendering React component. ' +
'Scripts inside React components are parser inserted into document ' +
'and they are never executed. Furthemore rendering script nodes ' +
'inside components breaks when using Trusted Types.',
{withoutStack: true},
);
expect(function() {
ReactTestUtils.renderIntoDocument(instance2);
}).not.toThrow();
}).toWarnDev(
'Warning: Encountered script tag while rendering React component. ' +
'Scripts inside React components are parser inserted into document ' +
'and they are never executed. Furthemore rendering script nodes ' +
'inside components breaks when using Trusted Types.',
{withoutStack: true},
);

expect(log).toHaveBeenCalledTimes(4);
expect(log).toHaveBeenNthCalledWith(1, null);
@@ -163,14 +173,6 @@ describe('ReactEmptyComponent', () => {
expect.objectContaining({tagName: 'SCRIPT'}),
);
expect(log).toHaveBeenNthCalledWith(4, null);
if (__DEV__) {
expect(consoleSpy).toHaveBeenCalledWith(
'Warning: Encountered script tag while rendering React component. ' +
'Scripts inside React components are parser inserted into document ' +
'and they are never executed. Furthemore rendering script nodes ' +
'inside components breaks when using Trusted Types.',
);
}
});

it(
6 changes: 3 additions & 3 deletions packages/react-dom/src/client/DOMPropertyOperations.js
Original file line number Diff line number Diff line change
@@ -16,7 +16,7 @@ import {
OVERLOADED_BOOLEAN,
} from '../shared/DOMProperty';
import sanitizeURL from '../shared/sanitizeURL';
import {trustedTypesAwareToString} from './ToStringValue';
import {toStringOrTrustedType} from './ToStringValue';
import {disableJavaScriptURLs} from 'shared/ReactFeatureFlags';
import {setAttribute, setAttributeNS} from './setAttribute';

@@ -144,7 +144,7 @@ export function setValueForProperty(
if (value === null) {
node.removeAttribute(attributeName);
} else {
setAttribute(node, attributeName, trustedTypesAwareToString(value));
setAttribute(node, attributeName, toStringOrTrustedType(value));
}
}
return;
@@ -176,7 +176,7 @@ export function setValueForProperty(
} else {
// `setAttribute` with objects becomes only `[object]` in IE8/9,
// ('' + value) makes it output the correct toString()-value.
attributeValue = trustedTypesAwareToString(value);
attributeValue = toStringOrTrustedType(value);
if (propertyInfo.sanitizeURL) {
sanitizeURL('' + attributeValue.toString());
}
4 changes: 2 additions & 2 deletions packages/react-dom/src/client/ReactDOMComponent.js
Original file line number Diff line number Diff line change
@@ -85,7 +85,7 @@ import possibleStandardNames from '../shared/possibleStandardNames';
import {validateProperties as validateARIAProperties} from '../shared/ReactDOMInvalidARIAHook';
import {validateProperties as validateInputProperties} from '../shared/ReactDOMNullInputValuePropHook';
import {validateProperties as validateUnknownProperties} from '../shared/ReactDOMUnknownPropertyHook';
import {trustedTypesAwareToString} from './ToStringValue';
import {toStringOrTrustedType} from './ToStringValue';

import {enableFlareAPI} from 'shared/ReactFeatureFlags';

@@ -787,7 +787,7 @@ export function diffProperties(
if (lastHtml !== nextHtml) {
(updatePayload = updatePayload || []).push(
propKey,
trustedTypesAwareToString(nextHtml),
toStringOrTrustedType(nextHtml),
);
}
} else {
38 changes: 12 additions & 26 deletions packages/react-dom/src/client/ToStringValue.js
Original file line number Diff line number Diff line change
@@ -38,32 +38,18 @@ export function getToStringValue(value: mixed): ToStringValue {

/**
* Returns true only if Trusted Types are available in global object and the value is a trusted type.
*
* Trusted Types undergo a change where window.TrustedTypes was renamed to window.trustedTypes
* (https://github.com/WICG/trusted-types/pull/205).
*/
function isTrustedTypesValue(value: any): boolean {
// $FlowExpectedError - TrustedTypes are defined only in some browsers or with polyfill
if (typeof TrustedTypes !== 'undefined') {
return (
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
(TrustedTypes.isURL && TrustedTypes.isURL(value))
);
// $FlowExpectedError - trustedTypes are defined only in some browsers or with polyfill
} else if (typeof trustedTypes !== 'undefined') {
return (
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
(trustedTypes.isURL && trustedTypes.isURL(value))
);
} else {
return false;
}
let isTrustedTypesValue: (value: any) => boolean;
// $FlowExpectedError - TrustedTypes are defined only in some browsers or with polyfill
if (typeof trustedTypes !== 'undefined') {
isTrustedTypesValue = (value: any) =>
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
Copy link
Collaborator

@gaearon gaearon Sep 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we even want to support them then?

Copy link
Contributor Author

@Siegrift Siegrift Sep 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

(trustedTypes.isURL && trustedTypes.isURL(value));
} else {
isTrustedTypesValue = () => false;
}

/** Trusted value is a wrapper for "safe" values which can be assigned to DOM execution sinks. */
@@ -76,7 +62,7 @@ export opaque type TrustedValue: {toString(): string} = {toString(): string};
*
* If application uses Trusted Types we don't stringify trusted values, but preserve them as objects.
*/
export function trustedTypesAwareToString(value: any): string | TrustedValue {
export function toStringOrTrustedType(value: any): string | TrustedValue {
// fast-path string values as it's most frequent usage of the function
if (typeof value !== 'string' && isTrustedTypesValue(value)) {
return value;
4 changes: 2 additions & 2 deletions packages/react-dom/src/client/__tests__/trustedTypes-test.js
Original file line number Diff line number Diff line change
@@ -3,13 +3,13 @@ describe('when Trusted Types are available in global object', () => {
let ReactDOM;

beforeEach(() => {
React = require('react');
ReactDOM = require('react-dom');
window.trustedTypes = {
isHTML: () => true,
isScript: () => false,
isScriptURL: () => false,
};
React = require('react');
ReactDOM = require('react-dom');
});

afterEach(() => {
1 change: 0 additions & 1 deletion scripts/rollup/validate/eslintrc.cjs.js
Original file line number Diff line number Diff line change
@@ -22,7 +22,6 @@ module.exports = {
setImmediate: true,
Buffer: true,
// Trusted Types
TrustedTypes: true,
trustedTypes: true,

// Scheduler profiling
1 change: 0 additions & 1 deletion scripts/rollup/validate/eslintrc.fb.js
Original file line number Diff line number Diff line change
@@ -23,7 +23,6 @@ module.exports = {
setImmediate: true,
Buffer: true,
// Trusted Types
TrustedTypes: true,
trustedTypes: true,

// Scheduler profiling
1 change: 0 additions & 1 deletion scripts/rollup/validate/eslintrc.rn.js
Original file line number Diff line number Diff line change
@@ -22,7 +22,6 @@ module.exports = {
// for more information
nativeFabricUIManager: true,
// Trusted Types
TrustedTypes: true,
trustedTypes: true,

// Scheduler profiling
1 change: 0 additions & 1 deletion scripts/rollup/validate/eslintrc.umd.js
Original file line number Diff line number Diff line change
@@ -25,7 +25,6 @@ module.exports = {
require: true,
global: true,
// Trusted Types
TrustedTypes: true,
trustedTypes: true,

// Scheduler profiling