From 666589e75433c79e767954b377b3f5f9acf0a09a Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Wed, 29 Mar 2023 13:20:25 -0400 Subject: [PATCH] Let sanitizeURL return a sanitized value Don't unnecessarily checkAttributeStringCoercion since these have already been checked by us treating symbols as null. --- .../src/client/DOMPropertyOperations.js | 22 ++++++---- .../src/server/ReactDOMServerFormatConfig.js | 42 +++++++------------ .../src/shared/sanitizeURL.js | 12 ++++-- 3 files changed, 38 insertions(+), 38 deletions(-) diff --git a/packages/react-dom-bindings/src/client/DOMPropertyOperations.js b/packages/react-dom-bindings/src/client/DOMPropertyOperations.js index bfb3eddfc5027..7b5178dda8303 100644 --- a/packages/react-dom-bindings/src/client/DOMPropertyOperations.js +++ b/packages/react-dom-bindings/src/client/DOMPropertyOperations.js @@ -50,7 +50,7 @@ export function getValueForProperty( if (__DEV__) { checkAttributeStringCoercion(expected, name); } - sanitizeURL('' + (expected: any)); + sanitizeURL(expected); } const attributeName = propertyInfo.attributeName; @@ -395,19 +395,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..4fd32e41e0fc5 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)), @@ -3936,11 +3934,7 @@ function writeStyleResourceAttributeInJS( // Santized URLs case 'src': case 'href': { - if (__DEV__) { - checkAttributeStringCoercion(value, attributeName); - } - attributeValue = '' + (value: any); - sanitizeURL(attributeValue); + value = sanitizeURL(value); break; } default: { @@ -4041,15 +4035,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 +4127,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..8cde90e7dfc0c 100644 --- a/packages/react-dom-bindings/src/shared/sanitizeURL.js +++ b/packages/react-dom-bindings/src/shared/sanitizeURL.js @@ -24,24 +24,28 @@ 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)) { + if (isJavaScriptProtocol.test(stringifiedURL)) { 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;