Skip to content

Commit

Permalink
Gate attribute stringification on bug detection logic (IE<=8 does not…
Browse files Browse the repository at this point in the history
… stringify attributes), instead of Trusted Types feature flag.

Added fixture tests for the logic.

For attributes undergoing sanitizeURL, the value is stringified in sanitizeURL function, unless enableTrustedTypesIntegration is true and the value is and 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.
  • Loading branch information
koto committed Aug 11, 2020
1 parent ce37bfa commit 40ef48a
Show file tree
Hide file tree
Showing 7 changed files with 182 additions and 14 deletions.
3 changes: 3 additions & 0 deletions fixtures/dom/src/components/Header.js
Expand Up @@ -89,6 +89,9 @@ class Header extends React.Component {
<option value="/selection-events">Selection Events</option>
<option value="/suspense">Suspense</option>
<option value="/form-state">Form State</option>
<option value="/attribute-stringification">
Attribute Stringification
</option>
</select>
</label>
<label htmlFor="global_version">
Expand Down
@@ -0,0 +1,36 @@
import Fixture from '../../Fixture';

const React = window.React;

class AttributeStringificationTestCase extends React.Component {
state = {
title: {
prop: 'if you see this, the test failed',
toString: () => 'stringified',
},
};
constructor(props) {
super(props);
this.input = React.createRef();
}
componentDidMount() {
this.setState(
Object.assign(this.state, {
titleRead: this.input.current.getAttribute('title'),
})
);
}

render() {
return (
<Fixture>
<div>
<input ref={this.input} title={this.state.title} />
<p>Attribute Value: {JSON.stringify(this.state.titleRead)}</p>
</div>
</Fixture>
);
}
}

export default AttributeStringificationTestCase;
@@ -0,0 +1,28 @@
import FixtureSet from '../../FixtureSet';
import TestCase from '../../TestCase';
import AttributeStringificationTestCase from './AttributeStringificationTestCase';

const React = window.React;

function AttributeStringification() {
return (
<FixtureSet title="Attribute stringification">
<TestCase
title="Stringification in attribute setters"
description={`
Some browsers fail to stringify objects passed to Element.setAttribute(NS)
function. This test verifies that React correctly detects this and stringifies the value itself.
`}
affectedBrowsers="IE9">
<TestCase.ExpectedResult>
The Attribute value displayed below the input field is "stringified".
The value is not "[object]".
</TestCase.ExpectedResult>

<AttributeStringificationTestCase />
</TestCase>
</FixtureSet>
);
}

export default AttributeStringification;
1 change: 1 addition & 0 deletions fixtures/dom/src/polyfills.js
Expand Up @@ -2,6 +2,7 @@ import 'core-js/es6/symbol';
import 'core-js/es6/promise';
import 'core-js/es6/set';
import 'core-js/es6/map';
import 'core-js/features/array/fill';

// http://paulirish.com/2011/requestanimationframe-for-smart-animating/
// http://my.opera.com/emoller/blog/2011/12/20/requestanimationframe-for-smart-er-animating
Expand Down
39 changes: 30 additions & 9 deletions packages/react-dom/src/client/DOMPropertyOperations.js
Expand Up @@ -16,14 +16,32 @@ import {
OVERLOADED_BOOLEAN,
} from '../shared/DOMProperty';
import sanitizeURL from '../shared/sanitizeURL';
import {
disableJavaScriptURLs,
enableTrustedTypesIntegration,
} from 'shared/ReactFeatureFlags';
import {disableJavaScriptURLs} from 'shared/ReactFeatureFlags';
import {isOpaqueHydratingObject} from './ReactDOMHostConfig';

import type {PropertyInfo} from '../shared/DOMProperty';

/**
* Cached result of detectStringification() function.
* Should become true in all environments but IE<=9.
*/
let setAttributeCanStringify = undefined;

/**
* Detect if Element.setAttribute stringifies attribute values.
* Should return true for all environments but IE <= 9.
* @param {DOMElement} node
*/
function detectStringification(node: Element) {
const obj: any = {
toString: () => 'foo',
};
const attrName = 'reacttest';
const el = node.ownerDocument.createElement('p');
el.setAttribute(attrName, obj);
return el.getAttribute(attrName) === 'foo';
}

/**
* Get the value for a property on a node. Only used in DEV for SSR validation.
* The "expected" argument is used as a hint of what the expected value is.
Expand Down Expand Up @@ -146,6 +164,9 @@ export function setValueForProperty(
if (shouldRemoveAttribute(name, value, propertyInfo, isCustomComponentTag)) {
value = null;
}
if (setAttributeCanStringify === undefined) {
setAttributeCanStringify = detectStringification(node);
}
// If the prop isn't in the special list, treat it as a simple attribute.
if (isCustomComponentTag || propertyInfo === null) {
if (isAttributeNameSafe(name)) {
Expand All @@ -155,7 +176,7 @@ export function setValueForProperty(
} else {
node.setAttribute(
attributeName,
enableTrustedTypesIntegration ? (value: any) : '' + (value: any),
setAttributeCanStringify ? (value: any) : '' + (value: any),
);
}
}
Expand Down Expand Up @@ -186,15 +207,15 @@ export function setValueForProperty(
// and we won't require Trusted Type here.
attributeValue = '';
} else {
// `setAttribute` with objects becomes only `[object]` in IE8/9,
// ('' + value) makes it output the correct toString()-value.
if (enableTrustedTypesIntegration) {
if (setAttributeCanStringify) {
attributeValue = (value: any);
} else {
// As `setAttribute` does not stringify the value itself.
// ('' + value) makes it output the correct toString()-value.
attributeValue = '' + (value: any);
}
if (propertyInfo.sanitizeURL) {
sanitizeURL(attributeValue.toString());
attributeValue = sanitizeURL(attributeValue);
}
}
if (attributeNamespace) {
Expand Down
Expand Up @@ -16,23 +16,37 @@ describe('when Trusted Types are available in global object', () => {
let container;
let ttObject1;
let ttObject2;
let fakeTTObjects;

const expectToReject = fn => {
let msg;
try {
fn();
} catch (x) {
msg = x.message;
}
expect(msg).toContain(
'React has blocked a javascript: URL as a security precaution.',
);
};

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 All @@ -47,6 +61,9 @@ describe('when Trusted Types are available in global object', () => {
};
fakeTTObjects.add(ttObject1);
fakeTTObjects.add(ttObject2);
// Run setAttributeCanStringify detection first to simplify counting
// setAttribute calls later.
ReactDOM.render(<div foo="bar" />, container);
});

afterEach(() => {
Expand Down Expand Up @@ -152,6 +169,56 @@ 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 setAttributeCalls = [];
Element.prototype.setAttribute = function(name, value) {
setAttributeCalls.push([this, name.toLowerCase(), value]);
return setAttribute.apply(this, arguments);
};
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.
expectToReject(() => {
ReactDOM.render(<a href={trustedScriptUrlJavascript} />, container)
});
expect(setAttributeCalls.length).toBe(0);
} finally {
Element.prototype.setAttribute = setAttribute;
}
});

it('should not stringify trusted values for setAttributeNS', () => {
const setAttributeNS = Element.prototype.setAttributeNS;
try {
Expand Down
16 changes: 14 additions & 2 deletions packages/react-dom/src/shared/sanitizeURL.js
Expand Up @@ -8,7 +8,10 @@
*/

import invariant from 'shared/invariant';
import {disableJavaScriptURLs} from 'shared/ReactFeatureFlags';
import {
disableJavaScriptURLs,
enableTrustedTypesIntegration,
} from 'shared/ReactFeatureFlags';

// A javascript: URL can contain leading C0 control or \u0020 SPACE,
// and any newline or tab are filtered out as if they're not part of the URL.
Expand All @@ -24,7 +27,15 @@ const isJavaScriptProtocol = /^[\u0000-\u001F ]*j[\r\n\t]*a[\r\n\t]*v[\r\n\t]*a[

let didWarn = false;

function sanitizeURL(url: string) {
function sanitizeURL(url: any): any {
if (
!enableTrustedTypesIntegration ||
typeof trustedTypes === 'undefined' ||
!trustedTypes.isScriptURL(url)
) {
// Coerce to a string, unless we know it's an immutable TrustedScriptURL object.
url = '' + url;
}
if (disableJavaScriptURLs) {
invariant(
!isJavaScriptProtocol.test(url),
Expand All @@ -41,6 +52,7 @@ function sanitizeURL(url: string) {
);
}
}
return url;
}

export default sanitizeURL;

0 comments on commit 40ef48a

Please sign in to comment.