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

Follow-up to initial Trusted Types support #16795

Merged
merged 3 commits into from
Sep 17, 2019
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
50 changes: 23 additions & 27 deletions packages/react-dom/src/client/ToStringValue.js
Original file line number Diff line number Diff line change
Expand Up @@ -38,22 +38,6 @@ export function getToStringValue(value: mixed): ToStringValue {
}
}

/**
* Returns true only if Trusted Types are available in global object and the value is a trusted type.
*/
let isTrustedTypesValue: (value: any) => boolean;
// $FlowExpectedError - TrustedTypes are defined only in some browsers or with polyfill
if (enableTrustedTypesIntegration && 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
(trustedTypes.isURL && trustedTypes.isURL(value));
} else {
isTrustedTypesValue = () => false;
}

/** Trusted value is a wrapper for "safe" values which can be assigned to DOM execution sinks. */
export opaque type TrustedValue: {toString(): string, valueOf(): string} = {
toString(): string,
Expand All @@ -67,15 +51,27 @@ export opaque type TrustedValue: {toString(): string, valueOf(): string} = {
*
* If application uses Trusted Types we don't stringify trusted values, but preserve them as objects.
*/
export function toStringOrTrustedType(value: any): string | TrustedValue {
if (
enableTrustedTypesIntegration &&
// fast-path string values as it's most frequent usage of the function
typeof value !== 'string' &&
isTrustedTypesValue(value)
) {
return value;
} else {
return '' + value;
}
export let toStringOrTrustedType: any => string | TrustedValue = toString;
if (enableTrustedTypesIntegration && typeof trustedTypes !== 'undefined') {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This seems like it would make this function not inlinable if the flag is on or dynamic which isn’t great because it used to be a simple ’’ + that gets inlined.

Copy link
Collaborator Author

@gaearon gaearon Sep 17, 2019

Choose a reason for hiding this comment

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

That’s actually what got me looking at it in the first place — it wasn’t getting inlined with a dynamic flag in FB bundle. Lemme think about this more.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If the flag is on, it's already not being inlined, even before this PR. Probably because the body is bigger. Since we're doing a function call either way, I think a smaller function (as in this PR) would be better than a larger one. It's also a single call (in this PR) rather than two calls (as in before).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm going to get this in because it's not being inlined either way with the flag on. If we want to fix this, we'll have to inline it manually at concrete callsites.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we toString in user space instead of letting the browser do it again?

Seems like we only need it in some IE legacy thing and/or for the javascript: URL check so we should only ever do this at all in those cases.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's for IE9 I think? We could feature-test it I guess.

const isHTML = trustedTypes.isHTML;
const isScript = trustedTypes.isScript;
const isScriptURL = trustedTypes.isScriptURL;
// TrustedURLs are deprecated and will be removed soon: https://github.com/WICG/trusted-types/pull/204
const isURL = trustedTypes.isURL ? trustedTypes.isURL : value => false;
toStringOrTrustedType = value => {
if (typeof value === 'string') {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I guess numbers should also go here.

// Fast-path string values as it's most frequent usage of the function.
return value;
}
if (
isHTML(value) ||
isScript(value) ||
isScriptURL(value) ||
isURL(value)
) {
// Pass Trusted Types through.
return value;
}
return toString(value);
};
}
208 changes: 193 additions & 15 deletions packages/react-dom/src/client/__tests__/trustedTypes-test.internal.js
Original file line number Diff line number Diff line change
@@ -1,42 +1,219 @@
/**
* Copyright (c) Facebook, Inc. and its affiliates.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*
* @emails react-core
*/

'use strict';

describe('when Trusted Types are available in global object', () => {
let React;
let ReactDOM;
let ReactFeatureFlags;
let container;
let fakeTTObjects;

beforeEach(() => {
jest.resetModules();
container = document.createElement('div');
window.trustedTypes = {
isHTML: () => true,
isHTML: value => fakeTTObjects.has(value),
isScript: () => false,
isScriptURL: () => false,
};
ReactFeatureFlags = require('shared/ReactFeatureFlags');
ReactFeatureFlags.enableTrustedTypesIntegration = true;
React = require('react');
ReactDOM = require('react-dom');
fakeTTObjects = new Set();
});

afterEach(() => {
delete window.trustedTypes;
ReactFeatureFlags.enableTrustedTypesIntegration = false;
});

it('should not stringify trusted values', () => {
const trustedObject = {toString: () => 'I look like a trusted object'};
class Component extends React.Component {
state = {inner: undefined};
render() {
return <div dangerouslySetInnerHTML={{__html: this.state.inner}} />;
}
it('should not stringify trusted values for dangerouslySetInnerHTML', () => {
const ttObject1 = {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would move this to beforeEach as it's being used in a few places.

toString() {
return '<b>Hi</b>';
},
};
const ttObject2 = {
toString() {
return '<b>Bye</b>';
},
};
fakeTTObjects.add(ttObject1);
fakeTTObjects.add(ttObject2);

let innerHTMLDescriptor = Object.getOwnPropertyDescriptor(
Element.prototype,
'innerHTML',
);
try {
const innerHTMLCalls = [];
Object.defineProperty(Element.prototype, 'innerHTML', {
get() {
return innerHTMLDescriptor.get.apply(this, arguments);
},
set(value) {
innerHTMLCalls.push(value);
return innerHTMLDescriptor.set.apply(this, arguments);
},
});
ReactDOM.render(
<div dangerouslySetInnerHTML={{__html: ttObject1}} />,
container,
);
expect(container.innerHTML).toBe('<div><b>Hi</b></div>');
expect(innerHTMLCalls.length).toBe(1);
// Ensure it didn't get stringified when passed to a DOM sink:
expect(innerHTMLCalls[0]).toBe(ttObject1);

innerHTMLCalls.length = 0;
ReactDOM.render(
<div dangerouslySetInnerHTML={{__html: ttObject2}} />,
container,
);
expect(container.innerHTML).toBe('<div><b>Bye</b></div>');
expect(innerHTMLCalls.length).toBe(1);
// Ensure it didn't get stringified when passed to a DOM sink:
expect(innerHTMLCalls[0]).toBe(ttObject2);
} finally {
Object.defineProperty(
Element.prototype,
'innerHTML',
innerHTMLDescriptor,
);
}
});

it('should not stringify trusted values for setAttribute (unknown attribute)', () => {
const ttObject1 = {
toString() {
return '<b>Hi</b>';
},
};
const ttObject2 = {
toString() {
return '<b>Bye</b>';
},
};
fakeTTObjects.add(ttObject1);
fakeTTObjects.add(ttObject2);

let setAttribute = Element.prototype.setAttribute;
try {
const setAttributeCalls = [];
Element.prototype.setAttribute = function(name, value) {
setAttributeCalls.push([this, name.toLowerCase(), value]);
return setAttribute.apply(this, arguments);
};
ReactDOM.render(<div data-foo={ttObject1} />, container);
expect(container.innerHTML).toBe('<div data-foo="<b>Hi</b>"></div>');
expect(setAttributeCalls.length).toBe(1);
expect(setAttributeCalls[0][0]).toBe(container.firstChild);
expect(setAttributeCalls[0][1]).toBe('data-foo');
// Ensure it didn't get stringified when passed to a DOM sink:
expect(setAttributeCalls[0][2]).toBe(ttObject1);

setAttributeCalls.length = 0;
ReactDOM.render(<div data-foo={ttObject2} />, container);
expect(setAttributeCalls.length).toBe(1);
expect(setAttributeCalls[0][0]).toBe(container.firstChild);
expect(setAttributeCalls[0][1]).toBe('data-foo');
// Ensure it didn't get stringified when passed to a DOM sink:
expect(setAttributeCalls[0][2]).toBe(ttObject2);
} finally {
Element.prototype.setAttribute = setAttribute;
}
});

it('should not stringify trusted values for setAttribute (known attribute)', () => {
const ttObject1 = {
toString() {
return '<b>Hi</b>';
},
};
const ttObject2 = {
toString() {
return '<b>Bye</b>';
},
};
fakeTTObjects.add(ttObject1);
fakeTTObjects.add(ttObject2);

const isHTMLSpy = jest.spyOn(window.trustedTypes, ['isHTML']);
const instance = ReactDOM.render(<Component />, container);
instance.setState({inner: trustedObject});
let setAttribute = Element.prototype.setAttribute;
try {
const setAttributeCalls = [];
Element.prototype.setAttribute = function(name, value) {
setAttributeCalls.push([this, name.toLowerCase(), value]);
return setAttribute.apply(this, arguments);
};
ReactDOM.render(<div className={ttObject1} />, container);
expect(container.innerHTML).toBe('<div class="<b>Hi</b>"></div>');
expect(setAttributeCalls.length).toBe(1);
expect(setAttributeCalls[0][0]).toBe(container.firstChild);
expect(setAttributeCalls[0][1]).toBe('class');
// Ensure it didn't get stringified when passed to a DOM sink:
expect(setAttributeCalls[0][2]).toBe(ttObject1);

expect(container.firstChild.innerHTML).toBe(trustedObject.toString());
expect(isHTMLSpy).toHaveBeenCalledWith(trustedObject);
setAttributeCalls.length = 0;
ReactDOM.render(<div className={ttObject2} />, container);
expect(setAttributeCalls.length).toBe(1);
expect(setAttributeCalls[0][0]).toBe(container.firstChild);
expect(setAttributeCalls[0][1]).toBe('class');
// Ensure it didn't get stringified when passed to a DOM sink:
expect(setAttributeCalls[0][2]).toBe(ttObject2);
} finally {
Element.prototype.setAttribute = setAttribute;
}
});

it('should not stringify trusted values for setAttributeNS', () => {
const ttObject1 = {
toString() {
return '<b>Hi</b>';
},
};
const ttObject2 = {
toString() {
return '<b>Bye</b>';
},
};
fakeTTObjects.add(ttObject1);
fakeTTObjects.add(ttObject2);

let setAttributeNS = Element.prototype.setAttributeNS;
try {
const setAttributeNSCalls = [];
Element.prototype.setAttributeNS = function(ns, name, value) {
setAttributeNSCalls.push([this, ns, name, value]);
return setAttributeNS.apply(this, arguments);
};
ReactDOM.render(<svg xlinkHref={ttObject1} />, container);
expect(container.innerHTML).toBe('<svg xlink:href="<b>Hi</b>"></svg>');
expect(setAttributeNSCalls.length).toBe(1);
expect(setAttributeNSCalls[0][0]).toBe(container.firstChild);
expect(setAttributeNSCalls[0][1]).toBe('http://www.w3.org/1999/xlink');
expect(setAttributeNSCalls[0][2]).toBe('xlink:href');
// Ensure it didn't get stringified when passed to a DOM sink:
expect(setAttributeNSCalls[0][3]).toBe(ttObject1);

setAttributeNSCalls.length = 0;
ReactDOM.render(<svg xlinkHref={ttObject2} />, container);
expect(setAttributeNSCalls.length).toBe(1);
expect(setAttributeNSCalls[0][0]).toBe(container.firstChild);
expect(setAttributeNSCalls[0][1]).toBe('http://www.w3.org/1999/xlink');
expect(setAttributeNSCalls[0][2]).toBe('xlink:href');
// Ensure it didn't get stringified when passed to a DOM sink:
expect(setAttributeNSCalls[0][3]).toBe(ttObject2);
} finally {
Element.prototype.setAttributeNS = setAttributeNS;
}
});

describe('dangerouslySetInnerHTML in svg elements in Internet Explorer', () => {
Expand Down Expand Up @@ -81,6 +258,7 @@ describe('when Trusted Types are available in global object', () => {
"You can try to wrap your svg element inside a div and use 'dangerouslySetInnerHTML' " +
'on the enclosing div instead.',
);
expect(container.innerHTML).toBe('<svg>unsafe html</svg>');
});
});

Expand All @@ -95,7 +273,7 @@ describe('when Trusted Types are available in global object', () => {
' in script (at **)',
);

// check that the warning is print only once
// check that the warning is printed only once
ReactDOM.render(<script>alert("I am not executed")</script>, container);
});
});
37 changes: 19 additions & 18 deletions packages/react-dom/src/client/setInnerHTML.js
Original file line number Diff line number Diff line change
Expand Up @@ -27,23 +27,26 @@ const setInnerHTML = createMicrosoftUnsafeLocalFunction(function(
node: Element,
html: string | TrustedValue,
): void {
// IE does not have innerHTML for SVG nodes, so instead we inject the
// new markup in a temp node and then move the child nodes across into
// the target node
if (node.namespaceURI === Namespaces.svg) {
if (enableTrustedTypesIntegration && __DEV__) {
warning(
// $FlowExpectedError - trustedTypes are defined only in some browsers or with polyfill
typeof trustedTypes === 'undefined',
"Using 'dangerouslySetInnerHTML' in an svg element with " +
'Trusted Types enabled in an Internet Explorer will cause ' +
'the trusted value to be converted to string. Assigning string ' +
"to 'innerHTML' will throw an error if Trusted Types are enforced. " +
"You can try to wrap your svg element inside a div and use 'dangerouslySetInnerHTML' " +
'on the enclosing div instead.',
);
if (__DEV__) {
if (enableTrustedTypesIntegration) {
// TODO: reconsider the text of this warning and when it should show
// before enabling the feature flag.
warning(
typeof trustedTypes === 'undefined',
"Using 'dangerouslySetInnerHTML' in an svg element with " +
'Trusted Types enabled in an Internet Explorer will cause ' +
'the trusted value to be converted to string. Assigning string ' +
"to 'innerHTML' will throw an error if Trusted Types are enforced. " +
"You can try to wrap your svg element inside a div and use 'dangerouslySetInnerHTML' " +
'on the enclosing div instead.',
);
}
}
if (!('innerHTML' in node)) {
// IE does not have innerHTML for SVG nodes, so instead we inject the
// new markup in a temp node and then move the child nodes across into
// the target node
reusableSVGContainer =
reusableSVGContainer || document.createElement('div');
reusableSVGContainer.innerHTML =
Expand All @@ -55,12 +58,10 @@ const setInnerHTML = createMicrosoftUnsafeLocalFunction(function(
while (svgNode.firstChild) {
node.appendChild(svgNode.firstChild);
}
} else {
node.innerHTML = (html: any);
return;
}
} else {
node.innerHTML = (html: any);
}
node.innerHTML = (html: any);
});

export default setInnerHTML;
8 changes: 8 additions & 0 deletions scripts/flow/environment.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,14 @@ declare var __REACT_DEVTOOLS_GLOBAL_HOOK__: any; /*?{
inject: ?((stuff: Object) => void)
};*/

declare var trustedTypes: {|
isHTML: (value: any) => boolean,
isScript: (value: any) => boolean,
isScriptURL: (value: any) => boolean,
// TrustedURLs are deprecated and will be removed soon: https://github.com/WICG/trusted-types/pull/204
isURL?: (value: any) => boolean,
|};

// ReactFeatureFlags www fork
declare module 'ReactFeatureFlags' {
declare module.exports: any;
Expand Down