From 4c2fc01900f50b5b1081a2fb8609ea2668bc05b6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Markb=C3=A5ge?= Date: Wed, 29 Mar 2023 23:39:02 -0400 Subject: [PATCH] Generate safe javascript url instead of throwing with disableJavaScriptURLs is on (#26507) We currently throw an error when disableJavaScriptURLs is on and trigger an error boundary. I kind of thought that's what would happen with CSP or Trusted Types anyway. However, that's not what happens. Instead, in those environments what happens is that the error is triggered when you try to actually visit those links. So if you `preventDefault()` or something it'll never show up and since the error just logs to the console or to a violation logger, it's effectively a noop to users. We can simulate the same without CSP by simply generating a different `javascript:` url that throws instead of executing the potential attack vector. This still allows these to be used - at least as long as you preventDefault before using them in practice. This might be legit for forms. We still don't recommend using them for links-as-buttons since it'll be possible to "Open in a New Tab" and other weird artifacts. For links we still recommend the technique of assigning a button role etc. It also is a little nicer when an attack actually happens because at least it doesn't allow an attacker to trigger error boundaries and effectively deny access to a page. --- .../src/client/DOMPropertyOperations.js | 45 +-- .../src/server/ReactDOMServerFormatConfig.js | 39 ++- .../src/shared/sanitizeURL.js | 19 +- ...ctDOMServerIntegrationUntrustedURL-test.js | 272 ++++++++++++------ 4 files changed, 233 insertions(+), 142 deletions(-) diff --git a/packages/react-dom-bindings/src/client/DOMPropertyOperations.js b/packages/react-dom-bindings/src/client/DOMPropertyOperations.js index bfb3eddfc5027..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: any)); - } 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; } @@ -395,19 +400,25 @@ export function setValueForProperty(node: Element, name: string, value: mixed) { } break; default: { + if (__DEV__) { + checkAttributeStringCoercion(value, attributeName); + } let attributeValue; // `setAttribute` with objects becomes only `[object]` in IE8/9, // ('' + value) makes it output the correct toString()-value. if (enableTrustedTypesIntegration) { - attributeValue = (value: any); - } else { - if (__DEV__) { - checkAttributeStringCoercion(value, attributeName); + if (propertyInfo.sanitizeURL) { + attributeValue = (sanitizeURL(value): any); + } else { + attributeValue = (value: any); } + } else { + // We have already verified this above. + // eslint-disable-next-line react-internal/safe-string-coercion attributeValue = '' + (value: any); - } - if (propertyInfo.sanitizeURL) { - sanitizeURL(attributeValue.toString()); + if (propertyInfo.sanitizeURL) { + attributeValue = sanitizeURL(attributeValue); + } } const attributeNamespace = propertyInfo.attributeNamespace; if (attributeNamespace) { diff --git a/packages/react-dom-bindings/src/server/ReactDOMServerFormatConfig.js b/packages/react-dom-bindings/src/server/ReactDOMServerFormatConfig.js index 09c879e8aa425..543fa064ea388 100644 --- a/packages/react-dom-bindings/src/server/ReactDOMServerFormatConfig.js +++ b/packages/react-dom-bindings/src/server/ReactDOMServerFormatConfig.js @@ -736,12 +736,13 @@ function pushAttribute( } break; default: + if (__DEV__) { + checkAttributeStringCoercion(value, attributeName); + } if (propertyInfo.sanitizeURL) { - if (__DEV__) { - checkAttributeStringCoercion(value, attributeName); - } - value = '' + (value: any); - sanitizeURL(value); + // We've already checked above. + // eslint-disable-next-line react-internal/safe-string-coercion + value = sanitizeURL('' + (value: any)); } target.push( attributeSeparator, @@ -3844,15 +3845,12 @@ function writeStyleResourceDependencyHrefOnlyInJS( function writeStyleResourceDependencyInJS( destination: Destination, - href: string, - precedence: string, + href: mixed, + precedence: mixed, props: Object, ) { - if (__DEV__) { - checkAttributeStringCoercion(href, 'href'); - } - const coercedHref = '' + (href: any); - sanitizeURL(coercedHref); + // eslint-disable-next-line react-internal/safe-string-coercion + const coercedHref = sanitizeURL('' + (href: any)); writeChunk( destination, stringToChunk(escapeJSObjectForInstructionScripts(coercedHref)), @@ -3939,8 +3937,7 @@ function writeStyleResourceAttributeInJS( if (__DEV__) { checkAttributeStringCoercion(value, attributeName); } - attributeValue = '' + (value: any); - sanitizeURL(attributeValue); + value = sanitizeURL(value); break; } default: { @@ -4041,15 +4038,12 @@ function writeStyleResourceDependencyHrefOnlyInAttr( function writeStyleResourceDependencyInAttr( destination: Destination, - href: string, - precedence: string, + href: mixed, + precedence: mixed, props: Object, ) { - if (__DEV__) { - checkAttributeStringCoercion(href, 'href'); - } - const coercedHref = '' + (href: any); - sanitizeURL(coercedHref); + // eslint-disable-next-line react-internal/safe-string-coercion + const coercedHref = sanitizeURL('' + (href: any)); writeChunk( destination, stringToChunk(escapeTextForBrowser(JSON.stringify(coercedHref))), @@ -4136,8 +4130,7 @@ function writeStyleResourceAttributeInAttr( if (__DEV__) { checkAttributeStringCoercion(value, attributeName); } - attributeValue = '' + (value: any); - sanitizeURL(attributeValue); + value = sanitizeURL(value); break; } default: { diff --git a/packages/react-dom-bindings/src/shared/sanitizeURL.js b/packages/react-dom-bindings/src/shared/sanitizeURL.js index f112ec2f1b572..b3de4657e9158 100644 --- a/packages/react-dom-bindings/src/shared/sanitizeURL.js +++ b/packages/react-dom-bindings/src/shared/sanitizeURL.js @@ -24,24 +24,29 @@ const isJavaScriptProtocol = let didWarn = false; -function sanitizeURL(url: string) { +function sanitizeURL(url: T): T | string { + // We should never have symbols here because they get filtered out elsewhere. + // eslint-disable-next-line react-internal/safe-string-coercion + const stringifiedURL = '' + (url: any); if (disableJavaScriptURLs) { - if (isJavaScriptProtocol.test(url)) { - throw new Error( - 'React has blocked a javascript: URL as a security precaution.', - ); + if (isJavaScriptProtocol.test(stringifiedURL)) { + // 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(url)) { + if (!didWarn && isJavaScriptProtocol.test(stringifiedURL)) { didWarn = true; console.error( '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 %s.', - JSON.stringify(url), + JSON.stringify(stringifiedURL), ); } } + return url; } export default sanitizeURL; 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( Click me, @@ -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(
@@ -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( - p0wned, - 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( + p0wned, + 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( @@ -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(
p0wned
, 1); expect(e.action).toBe('javascript:notfine'); }); - itRejectsRendering( - 'a javascript protocol button formAction', - async render => { - const e = await render(, 1); - expect(e.getAttribute('formAction')).toBe('javascript:notfine'); - }, - ); + itRenders('a javascript protocol button formAction', async render => { + const e = await render(, 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( , 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(