Skip to content

Commit

Permalink
Generate safe javascript url instead of throwing with disableJavaScri…
Browse files Browse the repository at this point in the history
…ptURLs 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.
  • Loading branch information
sebmarkbage authored Mar 30, 2023
1 parent f0aafa1 commit 4c2fc01
Show file tree
Hide file tree
Showing 4 changed files with 233 additions and 142 deletions.
45 changes: 28 additions & 17 deletions packages/react-dom-bindings/src/client/DOMPropertyOperations.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ import {
} from '../shared/DOMProperty';
import sanitizeURL from '../shared/sanitizeURL';
import {
disableJavaScriptURLs,
enableTrustedTypesIntegration,
enableCustomElementPropertySupport,
enableFilterEmptyStringAttributesDOM,
Expand All @@ -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;

Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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)),
Expand Down Expand Up @@ -3939,8 +3937,7 @@ function writeStyleResourceAttributeInJS(
if (__DEV__) {
checkAttributeStringCoercion(value, attributeName);
}
attributeValue = '' + (value: any);
sanitizeURL(attributeValue);
value = sanitizeURL(value);
break;
}
default: {
Expand Down Expand Up @@ -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))),
Expand Down Expand Up @@ -4136,8 +4130,7 @@ function writeStyleResourceAttributeInAttr(
if (__DEV__) {
checkAttributeStringCoercion(value, attributeName);
}
attributeValue = '' + (value: any);
sanitizeURL(attributeValue);
value = sanitizeURL(value);
break;
}
default: {
Expand Down
19 changes: 12 additions & 7 deletions packages/react-dom-bindings/src/shared/sanitizeURL.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,24 +24,29 @@ const isJavaScriptProtocol =

let didWarn = false;

function sanitizeURL(url: string) {
function sanitizeURL<T>(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;
Loading

0 comments on commit 4c2fc01

Please sign in to comment.