Skip to content

Commit

Permalink
Add DEV-only string coercion checks to prod files
Browse files Browse the repository at this point in the history
This commit adds DEV-only function calls to to check if string coercion
using `'' + value` will throw, which it will if the value is a Temporal
object or a symbol because those types can't be added with `+`.

If it will throw, then in DEV these checks will show a console error
to help the user undertsand what went wrong and how to fix the
problem. After emitting the console error, the check functions will
retry the coercion which will throw with a call stack that's easy (or
at least easier!) to troubleshoot because the exception happens right
after a long comment explaining the issue. So whether the user is in
a debugger, looking at the browser console, or viewing the in-browser
DEV call stack, it should be easy to understand and fix the problem.

In most cases, the safe-string-coercion ESLint rule is smart enough to
detect when a coercion is safe. But in rare cases (e.g. when a coercion
is inside a ternary) this rule will have to be manually disabled.

This commit also switches error-handling code to use `String(value)`
for coercion, because it's bad to crash when you're trying to build
an error message or a call stack!  Because `String()` is usually
disallowed by the `safe-string-coercion` ESLint rule in production
code, the rule must be disabled when `String()` is used.
  • Loading branch information
justingrant committed Sep 27, 2021
1 parent de90529 commit c78cc77
Show file tree
Hide file tree
Showing 20 changed files with 201 additions and 7 deletions.
20 changes: 20 additions & 0 deletions packages/react-dom/src/client/DOMPropertyOperations.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import {
disableJavaScriptURLs,
enableTrustedTypesIntegration,
} from 'shared/ReactFeatureFlags';
import {checkAttributeStringCoercion} from 'shared/CheckStringCoercion';
import {isOpaqueHydratingObject} from './ReactDOMHostConfig';

import type {PropertyInfo} from '../shared/DOMProperty';
Expand All @@ -40,10 +41,18 @@ export function getValueForProperty(
const {propertyName} = propertyInfo;
return (node: any)[propertyName];
} else {
// This check protects multiple uses of `expected`, which is why the
// react-internal/safe-string-coercion rule is disabled in several spots
// below.
if (__DEV__) {
checkAttributeStringCoercion(expected, name);
}

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.
// eslint-disable-next-line react-internal/safe-string-coercion
sanitizeURL('' + (expected: any));
}

Expand All @@ -60,6 +69,7 @@ export function getValueForProperty(
if (shouldRemoveAttribute(name, expected, propertyInfo, false)) {
return value;
}
// eslint-disable-next-line react-internal/safe-string-coercion
if (value === '' + (expected: any)) {
return expected;
}
Expand All @@ -85,6 +95,7 @@ export function getValueForProperty(

if (shouldRemoveAttribute(name, expected, propertyInfo, false)) {
return stringValue === null ? expected : stringValue;
// eslint-disable-next-line react-internal/safe-string-coercion
} else if (stringValue === '' + (expected: any)) {
return expected;
} else {
Expand Down Expand Up @@ -119,6 +130,9 @@ export function getValueForAttribute(
return expected === undefined ? undefined : null;
}
const value = node.getAttribute(name);
if (__DEV__) {
checkAttributeStringCoercion(expected, name);
}
if (value === '' + (expected: any)) {
return expected;
}
Expand Down Expand Up @@ -153,6 +167,9 @@ export function setValueForProperty(
if (value === null) {
node.removeAttribute(attributeName);
} else {
if (__DEV__) {
checkAttributeStringCoercion(value, name);
}
node.setAttribute(
attributeName,
enableTrustedTypesIntegration ? (value: any) : '' + (value: any),
Expand Down Expand Up @@ -191,6 +208,9 @@ export function setValueForProperty(
if (enableTrustedTypesIntegration) {
attributeValue = (value: any);
} else {
if (__DEV__) {
checkAttributeStringCoercion(value, attributeName);
}
attributeValue = '' + (value: any);
}
if (propertyInfo.sanitizeURL) {
Expand Down
4 changes: 4 additions & 0 deletions packages/react-dom/src/client/ReactDOMComponent.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import {

import {canUseDOM} from 'shared/ExecutionEnvironment';
import hasOwnProperty from 'shared/hasOwnProperty';
import {checkHtmlStringCoercion} from 'shared/CheckStringCoercion';

import {
getValueForAttribute,
Expand Down Expand Up @@ -139,6 +140,9 @@ if (__DEV__) {
const NORMALIZE_NULL_AND_REPLACEMENT_REGEX = /\u0000|\uFFFD/g;

normalizeMarkupForTextOrAttribute = function(markup: mixed): string {
if (__DEV__) {
checkHtmlStringCoercion(markup);
}
const markupString =
typeof markup === 'string' ? markup : '' + (markup: any);
return markupString
Expand Down
4 changes: 4 additions & 0 deletions packages/react-dom/src/client/ReactDOMInput.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import {checkControlledValueProps} from '../shared/ReactControlledValuePropTypes
import {updateValueIfChanged} from './inputValueTracking';
import getActiveElement from './getActiveElement';
import {disableInputAttributeSyncing} from 'shared/ReactFeatureFlags';
import {checkAttributeStringCoercion} from 'shared/CheckStringCoercion';

import type {ToStringValue} from './ToStringValue';

Expand Down Expand Up @@ -365,6 +366,9 @@ function updateNamedCousins(rootNode, props) {
// the input might not even be in a form. It might not even be in the
// document. Let's just use the local `querySelectorAll` to ensure we don't
// miss anything.
if (__DEV__) {
checkAttributeStringCoercion(name, 'name');
}
const group = queryRoot.querySelectorAll(
'input[name=' + JSON.stringify('' + name) + '][type="radio"]',
);
Expand Down
10 changes: 9 additions & 1 deletion packages/react-dom/src/client/ToStringValue.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@
* @flow
*/

import {checkFormFieldValueStringCoercion} from 'shared/CheckStringCoercion';

export opaque type ToStringValue =
| boolean
| number
Expand All @@ -19,17 +21,23 @@ export opaque type ToStringValue =
// around this limitation, we use an opaque type that can only be obtained by
// passing the value through getToStringValue first.
export function toString(value: ToStringValue): string {
// The coercion safety check is performed in getToStringValue().
// eslint-disable-next-line react-internal/safe-string-coercion
return '' + (value: any);
}

export function getToStringValue(value: mixed): ToStringValue {
switch (typeof value) {
case 'boolean':
case 'number':
case 'object':
case 'string':
case 'undefined':
return value;
case 'object':
if (__DEV__) {
checkFormFieldValueStringCoercion(value);
}
return value;
default:
// function, symbol are assigned as empty strings
return '';
Expand Down
11 changes: 11 additions & 0 deletions packages/react-dom/src/client/inputValueTracking.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@
* @flow
*/

import {checkFormFieldValueStringCoercion} from 'shared/CheckStringCoercion';

type ValueTracker = {|
getValue(): string,
setValue(value: string): void,
Expand Down Expand Up @@ -55,6 +57,9 @@ function trackValueOnNode(node: any): ?ValueTracker {
valueField,
);

if (__DEV__) {
checkFormFieldValueStringCoercion(node[valueField]);
}
let currentValue = '' + node[valueField];

// if someone has already defined a value or Safari, then bail
Expand All @@ -76,6 +81,9 @@ function trackValueOnNode(node: any): ?ValueTracker {
return get.call(this);
},
set: function(value) {
if (__DEV__) {
checkFormFieldValueStringCoercion(value);
}
currentValue = '' + value;
set.call(this, value);
},
Expand All @@ -93,6 +101,9 @@ function trackValueOnNode(node: any): ?ValueTracker {
return currentValue;
},
setValue(value) {
if (__DEV__) {
checkFormFieldValueStringCoercion(value);
}
currentValue = '' + value;
},
stopTracking() {
Expand Down
4 changes: 4 additions & 0 deletions packages/react-dom/src/server/DOMMarkupOperations.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import {
shouldRemoveAttribute,
} from '../shared/DOMProperty';
import sanitizeURL from '../shared/sanitizeURL';
import {checkAttributeStringCoercion} from 'shared/CheckStringCoercion';
import quoteAttributeValueForBrowser from './quoteAttributeValueForBrowser';

/**
Expand Down Expand Up @@ -44,6 +45,9 @@ export function createMarkupForProperty(name: string, value: mixed): string {
return attributeName + '=""';
} else {
if (propertyInfo.sanitizeURL) {
if (__DEV__) {
checkAttributeStringCoercion(value, attributeName);
}
value = '' + (value: any);
sanitizeURL(value);
}
Expand Down
35 changes: 35 additions & 0 deletions packages/react-dom/src/server/ReactDOMServerFormatConfig.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,12 @@

import type {ReactNodeList} from 'shared/ReactTypes';

import {
checkHtmlStringCoercion,
checkCSSPropertyStringCoercion,
checkAttributeStringCoercion,
} from 'shared/CheckStringCoercion';

import {Children} from 'react';

import {enableFilterEmptyStringAttributesDOM} from 'shared/ReactFeatureFlags';
Expand Down Expand Up @@ -272,6 +278,9 @@ function pushStyle(
const isCustomProperty = styleName.indexOf('--') === 0;
if (isCustomProperty) {
nameChunk = stringToChunk(escapeTextForBrowser(styleName));
if (__DEV__) {
checkCSSPropertyStringCoercion(styleValue, styleName);
}
valueChunk = stringToChunk(
escapeTextForBrowser(('' + styleValue).trim()),
);
Expand All @@ -291,6 +300,9 @@ function pushStyle(
valueChunk = stringToChunk('' + styleValue);
}
} else {
if (__DEV__) {
checkCSSPropertyStringCoercion(styleValue, styleName);
}
valueChunk = stringToChunk(
escapeTextForBrowser(('' + styleValue).trim()),
);
Expand Down Expand Up @@ -439,6 +451,9 @@ function pushAttribute(
break;
default:
if (propertyInfo.sanitizeURL) {
if (__DEV__) {
checkAttributeStringCoercion(value, attributeName);
}
value = '' + (value: any);
sanitizeURL(value);
}
Expand Down Expand Up @@ -496,6 +511,9 @@ function pushInnerHTML(
);
const html = innerHTML.__html;
if (html !== null && html !== undefined) {
if (__DEV__) {
checkHtmlStringCoercion(html);
}
target.push(stringToChunk('' + html));
}
}
Expand Down Expand Up @@ -679,6 +697,9 @@ function pushStartOption(
if (selectedValue !== null) {
let stringValue;
if (value !== null) {
if (__DEV__) {
checkAttributeStringCoercion(value, 'value');
}
stringValue = '' + value;
} else {
if (__DEV__) {
Expand All @@ -697,6 +718,9 @@ function pushStartOption(
if (isArray(selectedValue)) {
// multiple
for (let i = 0; i < selectedValue.length; i++) {
if (__DEV__) {
checkAttributeStringCoercion(selectedValue[i], 'value');
}
const v = '' + selectedValue[i];
if (v === stringValue) {
target.push(selectedMarkerAttribute);
Expand Down Expand Up @@ -895,8 +919,16 @@ function pushStartTextArea(
children.length <= 1,
'<textarea> can only have at most one child.',
);
// TODO: remove the coercion and the DEV check below because it will
// always be overwritten by the coercion several lines below it. #22309
if (__DEV__) {
checkHtmlStringCoercion(children[0]);
}
value = '' + children[0];
}
if (__DEV__) {
checkHtmlStringCoercion(children);
}
value = '' + children;
}

Expand Down Expand Up @@ -1142,6 +1174,9 @@ function pushStartPreformattedElement(
if (typeof html === 'string' && html.length > 0 && html[0] === '\n') {
target.push(leadingNewline, stringToChunk(html));
} else {
if (__DEV__) {
checkHtmlStringCoercion(html);
}
target.push(stringToChunk('' + html));
}
}
Expand Down
19 changes: 19 additions & 0 deletions packages/react-dom/src/server/ReactPartialRenderer.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,10 @@ import {
enableSuspenseServerRenderer,
enableScopeAPI,
} from 'shared/ReactFeatureFlags';
import {
checkPropStringCoercion,
checkFormFieldValueStringCoercion,
} from 'shared/CheckStringCoercion';

import {
REACT_DEBUG_TRACING_MODE_TYPE,
Expand Down Expand Up @@ -1472,6 +1476,9 @@ class ReactDOMServerRenderer {
textareaChildren = textareaChildren[0];
}

if (__DEV__) {
checkPropStringCoercion(textareaChildren, 'children');
}
defaultValue = '' + textareaChildren;
}
if (defaultValue == null) {
Expand All @@ -1480,6 +1487,9 @@ class ReactDOMServerRenderer {
initialValue = defaultValue;
}

if (__DEV__) {
checkFormFieldValueStringCoercion(initialValue);
}
props = Object.assign({}, props, {
value: undefined,
children: '' + initialValue,
Expand Down Expand Up @@ -1535,6 +1545,9 @@ class ReactDOMServerRenderer {
if (selectValue != null) {
let value;
if (props.value != null) {
if (__DEV__) {
checkFormFieldValueStringCoercion(props.value);
}
value = props.value + '';
} else {
if (__DEV__) {
Expand All @@ -1554,12 +1567,18 @@ class ReactDOMServerRenderer {
if (isArray(selectValue)) {
// multiple
for (let j = 0; j < selectValue.length; j++) {
if (__DEV__) {
checkFormFieldValueStringCoercion(selectValue[j]);
}
if ('' + selectValue[j] === value) {
selected = true;
break;
}
}
} else {
if (__DEV__) {
checkFormFieldValueStringCoercion(selectValue);
}
selected = '' + selectValue === value;
}

Expand Down
5 changes: 5 additions & 0 deletions packages/react-dom/src/server/escapeTextForBrowser.js
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,8 @@
* @private
*/

import {checkHtmlStringCoercion} from 'shared/CheckStringCoercion';

const matchHtmlRegExp = /["'&<>]/;

/**
Expand All @@ -47,6 +49,9 @@ const matchHtmlRegExp = /["'&<>]/;
*/

function escapeHtml(string) {
if (__DEV__) {
checkHtmlStringCoercion(string);
}
const str = '' + string;
const match = matchHtmlRegExp.exec(str);

Expand Down
4 changes: 4 additions & 0 deletions packages/react-dom/src/shared/dangerousStyleValue.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
*/

import {isUnitlessNumber} from './CSSProperty';
import {checkCSSPropertyStringCoercion} from 'shared/CheckStringCoercion';

/**
* Convert a value into the proper css writable value. The style name `name`
Expand Down Expand Up @@ -41,6 +42,9 @@ function dangerousStyleValue(name, value, isCustomProperty) {
return value + 'px'; // Presumes implicit 'px' suffix for unitless numbers
}

if (__DEV__) {
checkCSSPropertyStringCoercion(value, name);
}
return ('' + value).trim();
}

Expand Down
Loading

0 comments on commit c78cc77

Please sign in to comment.