From 23d2a8a23c72a42d597a59722c5421f161c0f577 Mon Sep 17 00:00:00 2001
From: Krzysztof Kotowicz <koto@google.com>
Date: Tue, 11 Aug 2020 12:49:47 +0200
Subject: [PATCH] Gate attribute stringification on bug detection logic (IE<=9
 does not 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 #19587.
---
 fixtures/dom/src/components/Header.js         |  3 +
 .../AttributeStringificationTestCase.js       | 36 +++++++++
 .../attribute-stringification/index.js        | 28 +++++++
 fixtures/dom/src/polyfills.js                 |  1 +
 .../src/client/DOMPropertyOperations.js       | 39 +++++++---
 .../__tests__/trustedTypes-test.internal.js   | 73 ++++++++++++++++++-
 packages/react-dom/src/shared/sanitizeURL.js  | 16 +++-
 7 files changed, 182 insertions(+), 14 deletions(-)
 create mode 100644 fixtures/dom/src/components/fixtures/attribute-stringification/AttributeStringificationTestCase.js
 create mode 100644 fixtures/dom/src/components/fixtures/attribute-stringification/index.js

diff --git a/fixtures/dom/src/components/Header.js b/fixtures/dom/src/components/Header.js
index fe9709af00318..9e0c24e41e170 100644
--- a/fixtures/dom/src/components/Header.js
+++ b/fixtures/dom/src/components/Header.js
@@ -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">
diff --git a/fixtures/dom/src/components/fixtures/attribute-stringification/AttributeStringificationTestCase.js b/fixtures/dom/src/components/fixtures/attribute-stringification/AttributeStringificationTestCase.js
new file mode 100644
index 0000000000000..a0bffe9864bca
--- /dev/null
+++ b/fixtures/dom/src/components/fixtures/attribute-stringification/AttributeStringificationTestCase.js
@@ -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;
diff --git a/fixtures/dom/src/components/fixtures/attribute-stringification/index.js b/fixtures/dom/src/components/fixtures/attribute-stringification/index.js
new file mode 100644
index 0000000000000..2a5e5de97834e
--- /dev/null
+++ b/fixtures/dom/src/components/fixtures/attribute-stringification/index.js
@@ -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;
diff --git a/fixtures/dom/src/polyfills.js b/fixtures/dom/src/polyfills.js
index ed6e08f59beb5..52146913f9244 100644
--- a/fixtures/dom/src/polyfills.js
+++ b/fixtures/dom/src/polyfills.js
@@ -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
diff --git a/packages/react-dom/src/client/DOMPropertyOperations.js b/packages/react-dom/src/client/DOMPropertyOperations.js
index 2dcb1ace8ab9f..cafd72e0c3b8d 100644
--- a/packages/react-dom/src/client/DOMPropertyOperations.js
+++ b/packages/react-dom/src/client/DOMPropertyOperations.js
@@ -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.
@@ -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)) {
@@ -155,7 +176,7 @@ export function setValueForProperty(
       } else {
         node.setAttribute(
           attributeName,
-          enableTrustedTypesIntegration ? (value: any) : '' + (value: any),
+          setAttributeCanStringify ? (value: any) : '' + (value: any),
         );
       }
     }
@@ -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) {
diff --git a/packages/react-dom/src/client/__tests__/trustedTypes-test.internal.js b/packages/react-dom/src/client/__tests__/trustedTypes-test.internal.js
index f74186f87dc60..9569db7b5c34e 100644
--- a/packages/react-dom/src/client/__tests__/trustedTypes-test.internal.js
+++ b/packages/react-dom/src/client/__tests__/trustedTypes-test.internal.js
@@ -16,11 +16,24 @@ 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) {
@@ -28,11 +41,12 @@ describe('when Trusted Types are available in global object', () => {
         }
         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 = {
@@ -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(() => {
@@ -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 {
diff --git a/packages/react-dom/src/shared/sanitizeURL.js b/packages/react-dom/src/shared/sanitizeURL.js
index 81d54478a9d76..faa9d9e55e0ed 100644
--- a/packages/react-dom/src/shared/sanitizeURL.js
+++ b/packages/react-dom/src/shared/sanitizeURL.js
@@ -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.
@@ -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),
@@ -41,6 +52,7 @@ function sanitizeURL(url: string) {
       );
     }
   }
+  return url;
 }
 
 export default sanitizeURL;