From 26589468ad7f1203e8e5d8424aae01bf9494fa21 Mon Sep 17 00:00:00 2001
From: Sebastian Markbage <sebastian@calyptus.eu>
Date: Wed, 29 Mar 2023 14:35:20 -0400
Subject: [PATCH] Generate safe javascript url instead of throwing

---
 .../src/client/DOMPropertyOperations.js       |  25 +-
 .../src/shared/sanitizeURL.js                 |   7 +-
 ...ctDOMServerIntegrationUntrustedURL-test.js | 272 ++++++++++++------
 3 files changed, 196 insertions(+), 108 deletions(-)

diff --git a/packages/react-dom-bindings/src/client/DOMPropertyOperations.js b/packages/react-dom-bindings/src/client/DOMPropertyOperations.js
index 7b5178dda8303..c27066b0cc31d 100644
--- a/packages/react-dom-bindings/src/client/DOMPropertyOperations.js
+++ b/packages/react-dom-bindings/src/client/DOMPropertyOperations.js
@@ -17,7 +17,6 @@ import {
 } from '../shared/DOMProperty';
 import sanitizeURL from '../shared/sanitizeURL';
 import {
-  disableJavaScriptURLs,
   enableTrustedTypesIntegration,
   enableCustomElementPropertySupport,
   enableFilterEmptyStringAttributesDOM,
@@ -43,15 +42,6 @@ export function getValueForProperty(
       const {propertyName} = propertyInfo;
       return (node: any)[propertyName];
     }
-    if (!disableJavaScriptURLs && propertyInfo.sanitizeURL) {
-      // If we haven't fully disabled javascript: URLs, and if
-      // the hydration is successful of a javascript: URL, we
-      // still want to warn on the client.
-      if (__DEV__) {
-        checkAttributeStringCoercion(expected, name);
-      }
-      sanitizeURL(expected);
-    }
 
     const attributeName = propertyInfo.attributeName;
 
@@ -134,6 +124,11 @@ export function getValueForProperty(
     }
 
     // shouldRemoveAttribute
+    switch (typeof expected) {
+      case 'function':
+      case 'symbol': // eslint-disable-line
+        return value;
+    }
     switch (propertyInfo.type) {
       case BOOLEAN: {
         if (expected) {
@@ -175,6 +170,16 @@ export function getValueForProperty(
     if (__DEV__) {
       checkAttributeStringCoercion(expected, name);
     }
+    if (propertyInfo.sanitizeURL) {
+      // We have already verified this above.
+      // eslint-disable-next-line react-internal/safe-string-coercion
+      if (value === '' + (sanitizeURL(expected): any)) {
+        return expected;
+      }
+      return value;
+    }
+    // We have already verified this above.
+    // eslint-disable-next-line react-internal/safe-string-coercion
     if (value === '' + (expected: any)) {
       return expected;
     }
diff --git a/packages/react-dom-bindings/src/shared/sanitizeURL.js b/packages/react-dom-bindings/src/shared/sanitizeURL.js
index 8cde90e7dfc0c..b3de4657e9158 100644
--- a/packages/react-dom-bindings/src/shared/sanitizeURL.js
+++ b/packages/react-dom-bindings/src/shared/sanitizeURL.js
@@ -30,9 +30,10 @@ function sanitizeURL<T>(url: T): T | string {
   const stringifiedURL = '' + (url: any);
   if (disableJavaScriptURLs) {
     if (isJavaScriptProtocol.test(stringifiedURL)) {
-      throw new Error(
-        'React has blocked a javascript: URL as a security precaution.',
-      );
+      // Return a different javascript: url that doesn't cause any side-effects and just
+      // throws if ever visited.
+      // eslint-disable-next-line no-script-url
+      return "javascript:throw new Error('React has blocked a javascript: URL as a security precaution.')";
     }
   } else if (__DEV__) {
     if (!didWarn && isJavaScriptProtocol.test(stringifiedURL)) {
diff --git a/packages/react-dom/src/__tests__/ReactDOMServerIntegrationUntrustedURL-test.js b/packages/react-dom/src/__tests__/ReactDOMServerIntegrationUntrustedURL-test.js
index 6aadcb75749fb..c7da08897ae37 100644
--- a/packages/react-dom/src/__tests__/ReactDOMServerIntegrationUntrustedURL-test.js
+++ b/packages/react-dom/src/__tests__/ReactDOMServerIntegrationUntrustedURL-test.js
@@ -19,7 +19,38 @@ let ReactDOM;
 let ReactDOMServer;
 let ReactTestUtils;
 
-function runTests(itRenders, itRejectsRendering, expectToReject) {
+const EXPECTED_SAFE_URL =
+  "javascript:throw new Error('React has blocked a javascript: URL as a security precaution.')";
+
+describe('ReactDOMServerIntegration - Untrusted URLs', () => {
+  // The `itRenders` helpers don't work with the gate pragma, so we have to do
+  // this instead.
+  if (gate(flags => flags.disableJavaScriptURLs)) {
+    it("empty test so Jest doesn't complain", () => {});
+    return;
+  }
+
+  function initModules() {
+    jest.resetModules();
+    React = require('react');
+    ReactDOM = require('react-dom');
+    ReactDOMServer = require('react-dom/server');
+    ReactTestUtils = require('react-dom/test-utils');
+
+    // Make them available to the helpers.
+    return {
+      ReactDOM,
+      ReactDOMServer,
+      ReactTestUtils,
+    };
+  }
+
+  const {resetModules, itRenders} = ReactDOMServerIntegrationUtils(initModules);
+
+  beforeEach(() => {
+    resetModules();
+  });
+
   itRenders('a http link with the word javascript in it', async render => {
     const e = await render(
       <a href="http://javascript:0/thisisfine">Click me</a>,
@@ -28,7 +59,7 @@ function runTests(itRenders, itRejectsRendering, expectToReject) {
     expect(e.href).toBe('http://javascript:0/thisisfine');
   });
 
-  itRejectsRendering('a javascript protocol href', async render => {
+  itRenders('a javascript protocol href', async render => {
     // Only the first one warns. The second warning is deduped.
     const e = await render(
       <div>
@@ -41,20 +72,17 @@ function runTests(itRenders, itRejectsRendering, expectToReject) {
     expect(e.lastChild.href).toBe('javascript:notfineagain');
   });
 
-  itRejectsRendering(
-    'a javascript protocol with leading spaces',
-    async render => {
-      const e = await render(
-        <a href={'  \t \u0000\u001F\u0003javascript\n: notfine'}>p0wned</a>,
-        1,
-      );
-      // We use an approximate comparison here because JSDOM might not parse
-      // \u0000 in HTML properly.
-      expect(e.href).toContain('notfine');
-    },
-  );
+  itRenders('a javascript protocol with leading spaces', async render => {
+    const e = await render(
+      <a href={'  \t \u0000\u001F\u0003javascript\n: notfine'}>p0wned</a>,
+      1,
+    );
+    // We use an approximate comparison here because JSDOM might not parse
+    // \u0000 in HTML properly.
+    expect(e.href).toContain('notfine');
+  });
 
-  itRejectsRendering(
+  itRenders(
     'a javascript protocol with intermediate new lines and mixed casing',
     async render => {
       const e = await render(
@@ -65,7 +93,7 @@ function runTests(itRenders, itRejectsRendering, expectToReject) {
     },
   );
 
-  itRejectsRendering('a javascript protocol area href', async render => {
+  itRenders('a javascript protocol area href', async render => {
     const e = await render(
       <map>
         <area href="javascript:notfine" />
@@ -75,20 +103,17 @@ function runTests(itRenders, itRejectsRendering, expectToReject) {
     expect(e.firstChild.href).toBe('javascript:notfine');
   });
 
-  itRejectsRendering('a javascript protocol form action', async render => {
+  itRenders('a javascript protocol form action', async render => {
     const e = await render(<form action="javascript:notfine">p0wned</form>, 1);
     expect(e.action).toBe('javascript:notfine');
   });
 
-  itRejectsRendering(
-    'a javascript protocol button formAction',
-    async render => {
-      const e = await render(<input formAction="javascript:notfine" />, 1);
-      expect(e.getAttribute('formAction')).toBe('javascript:notfine');
-    },
-  );
+  itRenders('a javascript protocol button formAction', async render => {
+    const e = await render(<input formAction="javascript:notfine" />, 1);
+    expect(e.getAttribute('formAction')).toBe('javascript:notfine');
+  });
 
-  itRejectsRendering('a javascript protocol input formAction', async render => {
+  itRenders('a javascript protocol input formAction', async render => {
     const e = await render(
       <button formAction="javascript:notfine">p0wned</button>,
       1,
@@ -96,12 +121,12 @@ function runTests(itRenders, itRejectsRendering, expectToReject) {
     expect(e.getAttribute('formAction')).toBe('javascript:notfine');
   });
 
-  itRejectsRendering('a javascript protocol iframe src', async render => {
+  itRenders('a javascript protocol iframe src', async render => {
     const e = await render(<iframe src="javascript:notfine" />, 1);
     expect(e.src).toBe('javascript:notfine');
   });
 
-  itRejectsRendering('a javascript protocol frame src', async render => {
+  itRenders('a javascript protocol frame src', async render => {
     const e = await render(
       <html>
         <head />
@@ -114,7 +139,7 @@ function runTests(itRenders, itRejectsRendering, expectToReject) {
     expect(e.lastChild.firstChild.src).toBe('javascript:notfine');
   });
 
-  itRejectsRendering('a javascript protocol in an SVG link', async render => {
+  itRenders('a javascript protocol in an SVG link', async render => {
     const e = await render(
       <svg>
         <a href="javascript:notfine" />
@@ -124,7 +149,7 @@ function runTests(itRenders, itRejectsRendering, expectToReject) {
     expect(e.firstChild.getAttribute('href')).toBe('javascript:notfine');
   });
 
-  itRejectsRendering(
+  itRenders(
     'a javascript protocol in an SVG link with a namespace',
     async render => {
       const e = await render(
@@ -142,49 +167,15 @@ function runTests(itRenders, itRejectsRendering, expectToReject) {
   it('rejects a javascript protocol href if it is added during an update', () => {
     const container = document.createElement('div');
     ReactDOM.render(<a href="thisisfine">click me</a>, container);
-    expectToReject(() => {
+    expect(() => {
       ReactDOM.render(<a href="javascript:notfine">click me</a>, container);
-    });
-  });
-}
-
-describe('ReactDOMServerIntegration - Untrusted URLs', () => {
-  // The `itRenders` helpers don't work with the gate pragma, so we have to do
-  // this instead.
-  if (gate(flags => flags.disableJavaScriptURLs)) {
-    it("empty test so Jest doesn't complain", () => {});
-    return;
-  }
-
-  function initModules() {
-    jest.resetModules();
-    React = require('react');
-    ReactDOM = require('react-dom');
-    ReactDOMServer = require('react-dom/server');
-    ReactTestUtils = require('react-dom/test-utils');
-
-    // Make them available to the helpers.
-    return {
-      ReactDOM,
-      ReactDOMServer,
-      ReactTestUtils,
-    };
-  }
-
-  const {resetModules, itRenders} = ReactDOMServerIntegrationUtils(initModules);
-
-  beforeEach(() => {
-    resetModules();
-  });
-
-  runTests(itRenders, itRenders, fn =>
-    expect(fn).toErrorDev(
+    }).toErrorDev(
       'Warning: A future version of React will block javascript: URLs as a security precaution. ' +
         'Use event handlers instead if you can. If you need to generate unsafe HTML try using ' +
         'dangerouslySetInnerHTML instead. React was passed "javascript:notfine".\n' +
         '    in a (at **)',
-    ),
-  );
+    );
+  });
 });
 
 describe('ReactDOMServerIntegration - Untrusted URLs - disableJavaScriptURLs', () => {
@@ -216,34 +207,127 @@ describe('ReactDOMServerIntegration - Untrusted URLs - disableJavaScriptURLs', (
   const {
     resetModules,
     itRenders,
-    itThrowsWhenRendering,
     clientRenderOnBadMarkup,
     clientRenderOnServerString,
   } = ReactDOMServerIntegrationUtils(initModules);
 
-  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(() => {
     resetModules();
   });
 
-  runTests(
-    itRenders,
-    (message, test) =>
-      itThrowsWhenRendering(message, test, 'blocked a javascript: URL'),
-    expectToReject,
+  itRenders('a http link with the word javascript in it', async render => {
+    const e = await render(
+      <a href="http://javascript:0/thisisfine">Click me</a>,
+    );
+    expect(e.tagName).toBe('A');
+    expect(e.href).toBe('http://javascript:0/thisisfine');
+  });
+
+  itRenders('a javascript protocol href', async render => {
+    // Only the first one warns. The second warning is deduped.
+    const e = await render(
+      <div>
+        <a href="javascript:notfine">p0wned</a>
+        <a href="javascript:notfineagain">p0wned again</a>
+      </div>,
+    );
+    expect(e.firstChild.href).toBe(EXPECTED_SAFE_URL);
+    expect(e.lastChild.href).toBe(EXPECTED_SAFE_URL);
+  });
+
+  itRenders('a javascript protocol with leading spaces', async render => {
+    const e = await render(
+      <a href={'  \t \u0000\u001F\u0003javascript\n: notfine'}>p0wned</a>,
+    );
+    // We use an approximate comparison here because JSDOM might not parse
+    // \u0000 in HTML properly.
+    expect(e.href).toBe(EXPECTED_SAFE_URL);
+  });
+
+  itRenders(
+    'a javascript protocol with intermediate new lines and mixed casing',
+    async render => {
+      const e = await render(
+        <a href={'\t\r\n Jav\rasCr\r\niP\t\n\rt\n:notfine'}>p0wned</a>,
+      );
+      expect(e.href).toBe(EXPECTED_SAFE_URL);
+    },
   );
 
+  itRenders('a javascript protocol area href', async render => {
+    const e = await render(
+      <map>
+        <area href="javascript:notfine" />
+      </map>,
+    );
+    expect(e.firstChild.href).toBe(EXPECTED_SAFE_URL);
+  });
+
+  itRenders('a javascript protocol form action', async render => {
+    const e = await render(<form action="javascript:notfine">p0wned</form>);
+    expect(e.action).toBe(EXPECTED_SAFE_URL);
+  });
+
+  itRenders('a javascript protocol button formAction', async render => {
+    const e = await render(<input formAction="javascript:notfine" />);
+    expect(e.getAttribute('formAction')).toBe(EXPECTED_SAFE_URL);
+  });
+
+  itRenders('a javascript protocol input formAction', async render => {
+    const e = await render(
+      <button formAction="javascript:notfine">p0wned</button>,
+    );
+    expect(e.getAttribute('formAction')).toBe(EXPECTED_SAFE_URL);
+  });
+
+  itRenders('a javascript protocol iframe src', async render => {
+    const e = await render(<iframe src="javascript:notfine" />);
+    expect(e.src).toBe(EXPECTED_SAFE_URL);
+  });
+
+  itRenders('a javascript protocol frame src', async render => {
+    const e = await render(
+      <html>
+        <head />
+        <frameset>
+          <frame src="javascript:notfine" />
+        </frameset>
+      </html>,
+    );
+    expect(e.lastChild.firstChild.src).toBe(EXPECTED_SAFE_URL);
+  });
+
+  itRenders('a javascript protocol in an SVG link', async render => {
+    const e = await render(
+      <svg>
+        <a href="javascript:notfine" />
+      </svg>,
+    );
+    expect(e.firstChild.getAttribute('href')).toBe(EXPECTED_SAFE_URL);
+  });
+
+  itRenders(
+    'a javascript protocol in an SVG link with a namespace',
+    async render => {
+      const e = await render(
+        <svg>
+          <a xlinkHref="javascript:notfine" />
+        </svg>,
+      );
+      expect(
+        e.firstChild.getAttributeNS('http://www.w3.org/1999/xlink', 'href'),
+      ).toBe(EXPECTED_SAFE_URL);
+    },
+  );
+
+  it('rejects a javascript protocol href if it is added during an update', () => {
+    const container = document.createElement('div');
+    ReactDOM.render(<a href="http://thisisfine/">click me</a>, container);
+    expect(container.firstChild.href).toBe('http://thisisfine/');
+    ReactDOM.render(<a href="javascript:notfine">click me</a>, container);
+    expect(container.firstChild.href).toBe(EXPECTED_SAFE_URL);
+  });
+
   itRenders('only the first invocation of toString', async render => {
     let expectedToStringCalls = 1;
     if (render === clientRenderOnBadMarkup) {
@@ -255,9 +339,8 @@ describe('ReactDOMServerIntegration - Untrusted URLs - disableJavaScriptURLs', (
       // The hydration validation calls it one extra time.
       // TODO: It would be good if we only called toString once for
       // consistency but the code structure makes that hard right now.
-      expectedToStringCalls = 2;
-    }
-    if (__DEV__) {
+      expectedToStringCalls = 5;
+    } else if (__DEV__) {
       // Checking for string coercion problems results in double the
       // toString calls in DEV
       expectedToStringCalls *= 2;
@@ -283,14 +366,13 @@ describe('ReactDOMServerIntegration - Untrusted URLs - disableJavaScriptURLs', (
 
   it('rejects a javascript protocol href if it is added during an update twice', () => {
     const container = document.createElement('div');
-    ReactDOM.render(<a href="thisisfine">click me</a>, container);
-    expectToReject(() => {
-      ReactDOM.render(<a href="javascript:notfine">click me</a>, container);
-    });
+    ReactDOM.render(<a href="http://thisisfine/">click me</a>, container);
+    expect(container.firstChild.href).toBe('http://thisisfine/');
+    ReactDOM.render(<a href="javascript:notfine">click me</a>, container);
+    expect(container.firstChild.href).toBe(EXPECTED_SAFE_URL);
     // The second update ensures that a global flag hasn't been added to the regex
     // which would fail to match the second time it is called.
-    expectToReject(() => {
-      ReactDOM.render(<a href="javascript:notfine">click me</a>, container);
-    });
+    ReactDOM.render(<a href="javascript:notfine">click me</a>, container);
+    expect(container.firstChild.href).toBe(EXPECTED_SAFE_URL);
   });
 });