Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Changes to attribute whitelist logic #10564

Merged
merged 36 commits into from
Aug 31, 2017
Merged

Changes to attribute whitelist logic #10564

Show file tree
Hide file tree
Changes from 23 commits
Commits
Show all changes
36 commits
Select commit Hold shift + click to select a range
e10694a
Remove HTMLPropertyConfig entries for non-boolean values
nhunzaker Aug 24, 2017
711bafe
Only HAS_BOOLEAN_VALUE attribute flag can assign booleans
nhunzaker Aug 29, 2017
4c5dfbb
Use a non-boolean attribute in object assignment tests
nhunzaker Aug 29, 2017
e086ff1
Add HAS_STRING_BOOLEAN_VALUE attribute flag
nhunzaker Aug 29, 2017
0410651
Fix boolean tests, add boolean warning.
nhunzaker Aug 29, 2017
ab75d9a
Reserved props should allow booleans
nhunzaker Aug 29, 2017
6077e0b
Remove outdated comments
gaearon Aug 29, 2017
6f913ed
Style tweaks
gaearon Aug 29, 2017
aca8d9c
Don't treat dashed SVG tags as custom elements
gaearon Aug 29, 2017
af7d035
SVG elements like font-face are not custom attributes
nhunzaker Aug 29, 2017
9c0751f
Move namespace check to isCustomAttribute. Add caveat for stack.
nhunzaker Aug 29, 2017
f8da44e
Remove unused namespace variable assignment
nhunzaker Aug 29, 2017
b93e093
Fix the DEV-only whitelist
gaearon Aug 29, 2017
fbcced1
Don't read property twice
gaearon Aug 29, 2017
a270e03
Ignore and warn about non-string `is` attribute
gaearon Aug 29, 2017
ed92af0
Blacklist "aria" and "data" attributes
gaearon Aug 29, 2017
5a339a1
Don't pass unknown on* attributes through
gaearon Aug 29, 2017
0b2ba65
Remove dead code
gaearon Aug 29, 2017
3f85316
Avoid accessing namespace when possible
nhunzaker Aug 29, 2017
76a6318
Drop .only in ReactDOMComponent-test
nhunzaker Aug 29, 2017
b29bb74
Make isCustomComponent logic more solid
gaearon Aug 29, 2017
0a2aec4
Do attribute name check earlier
gaearon Aug 29, 2017
501e86d
Fix fbjs import
gaearon Aug 29, 2017
8d1f487
Revert unintentional edit
gaearon Aug 29, 2017
72666fa
Re-allow "data" attribute
gaearon Aug 29, 2017
cb687ed
Use stricter check when attaching events
gaearon Aug 29, 2017
1d61379
Pass SVG boolean attributes with correct casing
gaearon Aug 29, 2017
2b0f61a
Fix the test
gaearon Aug 29, 2017
1590c2a
Undo the SVG dashed-name fix
gaearon Aug 30, 2017
cb883a6
Prettier
gaearon Aug 30, 2017
32f6321
Fix lint
gaearon Aug 30, 2017
6d9c0e0
Fix flow
gaearon Aug 30, 2017
10e0c09
Pass "aria" through but still warn
gaearon Aug 30, 2017
ba71ec1
Remove special cases for onfocusin, onfocusout
gaearon Aug 30, 2017
dc760af
Add a more specific warning for unknown events
gaearon Aug 31, 2017
10950c4
Pass badly cased React attributes through with warning
gaearon Aug 31, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions fixtures/packaging/babel-standalone/dev.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
<html>
<body>
<script src="../../../build/dist/react.development.js"></script>
<script src="../../../build/dist/react-dom.development.js"></script>
<script src="../../attribute-behavior/public/react.development.js"></script>
<script src="../../attribute-behavior/public/react-dom.development.js"></script>
<script src="https://unpkg.com/babel-standalone@6/babel.js"></script>
<div id="container"></div>
<script type="text/babel">
ReactDOM.render(
<h1>Hello World!</h1>,
<svg><hkern K={'lo;'} /></svg>,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We probably don't intend to merge this part, right?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope :P

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lol. I honestly thought that was an edge case test. 😨

document.getElementById('container')
);
</script>
Expand Down
57 changes: 40 additions & 17 deletions src/renderers/dom/fiber/ReactDOMFiberComponent.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,10 +40,10 @@ if (__DEV__) {
var ReactDOMUnknownPropertyHook = require('ReactDOMUnknownPropertyHook');
var {validateProperties: validateARIAProperties} = ReactDOMInvalidARIAHook;
var {
validateProperties: validateInputPropertes,
validateProperties: validateInputProperties,
} = ReactDOMNullInputValuePropHook;
var {
validateProperties: validateUnknownPropertes,
validateProperties: validateUnknownProperties,
} = ReactDOMUnknownPropertyHook;
}

Expand All @@ -70,10 +70,10 @@ if (__DEV__) {
time: true,
};

var validatePropertiesInDevelopment = function(type, props) {
validateARIAProperties(type, props);
validateInputPropertes(type, props);
validateUnknownPropertes(type, props);
var validatePropertiesInDevelopment = function(type, props, domElement) {
validateARIAProperties(type, props, domElement);
validateInputProperties(type, props);
validateUnknownProperties(type, props, domElement);
};

var warnForTextDifference = function(serverText: string, clientText: string) {
Expand Down Expand Up @@ -305,11 +305,14 @@ var ReactDOMFiberComponent = {
if (namespaceURI === HTML_NAMESPACE) {
namespaceURI = getIntrinsicNamespace(type);
}
if (__DEV__) {
var isCustomComponentTag = isCustomComponent(type, props);
}
if (namespaceURI === HTML_NAMESPACE) {
if (__DEV__) {
var isCustomComponentTag = isCustomComponent(
type,
props,
null,
namespaceURI,
);
// Should this check be gated by parent namespace? Not sure we want to
// allow <SVG> or <mATH>.
warning(
Expand All @@ -328,7 +331,7 @@ var ReactDOMFiberComponent = {
// This is guaranteed to yield a script element.
var firstChild = ((div.firstChild: any): HTMLScriptElement);
domElement = div.removeChild(firstChild);
} else if (props.is) {
} else if (typeof props.is === 'string') {
// $FlowIssue `createElement` should be updated for Web Components
domElement = ownerDocument.createElement(type, {is: props.is});
} else {
Expand Down Expand Up @@ -370,9 +373,14 @@ var ReactDOMFiberComponent = {
rawProps: Object,
rootContainerElement: Element | Document,
): void {
var isCustomComponentTag = isCustomComponent(tag, rawProps);
var isCustomComponentTag = isCustomComponent(
tag,
rawProps,
domElement,
null,
);
if (__DEV__) {
validatePropertiesInDevelopment(tag, rawProps);
validatePropertiesInDevelopment(tag, rawProps, domElement);
if (isCustomComponentTag && !didWarnShadyDOM && domElement.shadyRoot) {
warning(
false,
Expand Down Expand Up @@ -543,7 +551,7 @@ var ReactDOMFiberComponent = {
rootContainerElement: Element | Document,
): null | Array<mixed> {
if (__DEV__) {
validatePropertiesInDevelopment(tag, nextRawProps);
validatePropertiesInDevelopment(tag, nextRawProps, domElement);
}

var updatePayload: null | Array<any> = null;
Expand Down Expand Up @@ -740,8 +748,18 @@ var ReactDOMFiberComponent = {
lastRawProps: Object,
nextRawProps: Object,
): void {
var wasCustomComponentTag = isCustomComponent(tag, lastRawProps);
var isCustomComponentTag = isCustomComponent(tag, nextRawProps);
var wasCustomComponentTag = isCustomComponent(
tag,
lastRawProps,
domElement,
null,
);
var isCustomComponentTag = isCustomComponent(
tag,
nextRawProps,
domElement,
null,
);
// Apply the diff.
updateDOMProperties(
domElement,
Expand Down Expand Up @@ -781,8 +799,13 @@ var ReactDOMFiberComponent = {
rootContainerElement: Element | Document,
): null | Array<mixed> {
if (__DEV__) {
var isCustomComponentTag = isCustomComponent(tag, rawProps);
validatePropertiesInDevelopment(tag, rawProps);
var isCustomComponentTag = isCustomComponent(
tag,
rawProps,
domElement,
null,
);
validatePropertiesInDevelopment(tag, rawProps, domElement);
if (isCustomComponentTag && !didWarnShadyDOM && domElement.shadyRoot) {
warning(
false,
Expand Down
6 changes: 5 additions & 1 deletion src/renderers/dom/shared/DOMMarkupOperations.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,8 +99,12 @@ var DOMMarkupOperations = {
(propertyInfo.hasOverloadedBooleanValue && value === true)
) {
return attributeName + '=""';
} else if (
typeof value !== 'boolean' ||
DOMProperty.shouldAttributeAcceptBooleanValue(name)
) {
return attributeName + '=' + quoteAttributeValueForBrowser(value);
}
return attributeName + '=' + quoteAttributeValueForBrowser(value);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd love to just lean on DOMProperty.shouldSetAttribute, but reserved props pass through here, and style is in there. Maybe we could remove style from reserved props.

} else if (DOMProperty.shouldSetAttribute(name, value)) {
if (value == null) {
return '';
Expand Down
42 changes: 30 additions & 12 deletions src/renderers/dom/shared/DOMProperty.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,9 @@ var invariant = require('fbjs/lib/invariant');
// These attributes should be all lowercase to allow for
// case insensitive checks
var RESERVED_PROPS = {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realize this isn't part of this change but this seems problematic to me. This is a non-trivial case insensitive look up (as opposed to say a super quick interned string comparison).

We should make reserved props case sensitive and pass other ones through but warn just like we do for any other "possible standard name".

aria: true,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where is data?

We might not need to reserve these. A warning is probably enough.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It turned out we actually use data and have it in whitelist. Apparently it's useful for <object>. Yes, could make it a warning.

children: true,
data: true,
dangerouslysetinnerhtml: true,
autofocus: true,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't comment on the unchanged line but onfocusin and onfocusout can be removed since they're now covered by the on prefix rule.

defaultvalue: true,
Expand All @@ -42,6 +44,7 @@ var DOMPropertyInjection = {
HAS_NUMERIC_VALUE: 0x8,
HAS_POSITIVE_NUMERIC_VALUE: 0x10 | 0x8,
HAS_OVERLOADED_BOOLEAN_VALUE: 0x20,
HAS_STRING_BOOLEAN_VALUE: 0x40,

/**
* Inject some specialized knowledge about the DOM. This takes a config object
Expand Down Expand Up @@ -103,6 +106,10 @@ var DOMPropertyInjection = {
propConfig,
Injection.HAS_OVERLOADED_BOOLEAN_VALUE,
),
hasStringBooleanValue: checkMask(
propConfig,
Injection.HAS_STRING_BOOLEAN_VALUE,
),
};
invariant(
propertyInfo.hasBooleanValue +
Expand Down Expand Up @@ -201,26 +208,21 @@ var DOMProperty = {
if (DOMProperty.isReservedProp(name)) {
return false;
}

if (
(name[0] === 'o' || name[0] === 'O') &&
(name[1] === 'n' || name[1] === 'N')
) {
return false;
}
if (value === null) {
return true;
}

var lowerCased = name.toLowerCase();

var propertyInfo = DOMProperty.properties[name];

switch (typeof value) {
case 'boolean':
if (propertyInfo) {
return true;
}
var prefix = lowerCased.slice(0, 5);
return prefix === 'data-' || prefix === 'aria-';
return DOMProperty.shouldAttributeAcceptBooleanValue(name);
case 'undefined':
case 'number':
case 'string':
return true;
case 'object':
return true;
default:
Expand All @@ -235,6 +237,22 @@ var DOMProperty = {
: null;
},

shouldAttributeAcceptBooleanValue(name) {
if (DOMProperty.isReservedProp(name)) {
return true;
}
let propertyInfo = DOMProperty.getPropertyInfo(name);
if (propertyInfo) {
return (
propertyInfo.hasBooleanValue ||
propertyInfo.hasStringBooleanValue ||
propertyInfo.hasOverloadedBooleanValue
);
}
var prefix = name.toLowerCase().slice(0, 5);
return prefix === 'data-' || prefix === 'aria-';
},

/**
* Checks to see if a property name is within the list of properties
* reserved for internal React operations. These properties should
Expand Down
25 changes: 9 additions & 16 deletions src/renderers/dom/shared/HTMLDOMPropertyConfig.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,13 +20,17 @@ var HAS_POSITIVE_NUMERIC_VALUE =
DOMProperty.injection.HAS_POSITIVE_NUMERIC_VALUE;
var HAS_OVERLOADED_BOOLEAN_VALUE =
DOMProperty.injection.HAS_OVERLOADED_BOOLEAN_VALUE;
var HAS_STRING_BOOLEAN_VALUE = DOMProperty.injection.HAS_STRING_BOOLEAN_VALUE;

var HTMLDOMPropertyConfig = {
// When adding attributes to this list, be sure to also add them to
// the `possibleStandardNames` module to ensure casing and incorrect
// name warnings.
Properties: {
allowFullScreen: HAS_BOOLEAN_VALUE,
// IE only true/false iFrame attribute
// https://msdn.microsoft.com/en-us/library/ms533072(v=vs.85).aspx
allowTransparency: HAS_STRING_BOOLEAN_VALUE,
// specifies target context for links with `preload` type
async: HAS_BOOLEAN_VALUE,
// autoFocus is polyfilled/normalized by AutoFocusUtils
Expand All @@ -35,11 +39,13 @@ var HTMLDOMPropertyConfig = {
capture: HAS_BOOLEAN_VALUE,
checked: MUST_USE_PROPERTY | HAS_BOOLEAN_VALUE,
cols: HAS_POSITIVE_NUMERIC_VALUE,
contentEditable: HAS_STRING_BOOLEAN_VALUE,
controls: HAS_BOOLEAN_VALUE,
default: HAS_BOOLEAN_VALUE,
defer: HAS_BOOLEAN_VALUE,
disabled: HAS_BOOLEAN_VALUE,
download: HAS_OVERLOADED_BOOLEAN_VALUE,
draggable: HAS_STRING_BOOLEAN_VALUE,
formNoValidate: HAS_BOOLEAN_VALUE,
hidden: HAS_BOOLEAN_VALUE,
loop: HAS_BOOLEAN_VALUE,
Expand All @@ -62,6 +68,7 @@ var HTMLDOMPropertyConfig = {
start: HAS_NUMERIC_VALUE,
// support for projecting regular DOM Elements via V1 named slots ( shadow dom )
span: HAS_POSITIVE_NUMERIC_VALUE,
spellCheck: HAS_STRING_BOOLEAN_VALUE,
// Style must be explicitly set in the attribute list. React components
// expect a style object
style: 0,
Expand All @@ -75,22 +82,8 @@ var HTMLDOMPropertyConfig = {
htmlFor: 0,
httpEquiv: 0,
// Attributes with mutation methods must be specified in the whitelist
value: 0,
// The following attributes expect boolean values. They must be in
// the whitelist to allow boolean attribute assignment:
autoComplete: 0,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed autoComplete. It is actually on/off, not true/false. Thanks @syranide!

// IE only true/false iFrame attribute
// https://msdn.microsoft.com/en-us/library/ms533072(v=vs.85).aspx
allowTransparency: 0,
contentEditable: 0,
draggable: 0,
spellCheck: 0,
// autoCapitalize and autoCorrect are supported in Mobile Safari for
// keyboard hints.
autoCapitalize: 0,
autoCorrect: 0,
// autoSave allows WebKit/Blink to persist values of input fields on page reloads
autoSave: 0,
Copy link
Contributor Author

@nhunzaker nhunzaker Aug 29, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same deal with autoSave, autoCorrect, and autoCapitalize. I pulled this over from: #10531

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we special case these so that they work? It's odd when the form <x yyy /> doesn't work like the serialized HTML form.

Copy link
Contributor Author

@nhunzaker nhunzaker Aug 31, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just for context, "on" and "off" were deprecated for autocapitalize in iOS5. Acceptable values 1 are "none", "sentences", "words", and "characters". When no value is given, it defaults to "sentence" for form tags and "none" for password input elements, but otherwise uses the attribute on the related form.

This default value appears to be an empty string, at least when I log out input.outerHTML in Safari. It also enables capitalization, at least in a very quick check in the ios simulator.

Hmm. It is frustrating that true is the assignment type for implicit attributes (like <input autocapitalize />). Maybe in a breaking release of JSX there could be a symbol for it, or use an empty string.

In the mean time, should we want to parse true as an empty string for cases like this? Maybe false should warn. Is this behavior safe to generalize on all attributes that don't have the HAS_STRING_BOOLEAN_VALUE flag?

@aweary is this in line what what you were thinking for boolean attributes?

  1. https://developer.apple.com/library/content/documentation/AppleApplications/Reference/SafariHTMLRef/Articles/Attributes.html#//apple_ref/doc/uid/TP40008058-autocapitalize

Random fun aside: This is my first time using a footnote in a reply on Github. What a time to be alive.

// Set the string boolean flag to allow the behavior
value: HAS_STRING_BOOLEAN_VALUE,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This must be in here for backwards compatibility.

},
DOMAttributeNames: {
acceptCharset: 'accept-charset',
Expand Down
15 changes: 9 additions & 6 deletions src/renderers/dom/shared/SVGDOMPropertyConfig.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,10 @@

'use strict';

var DOMProperty = require('DOMProperty');

var {HAS_STRING_BOOLEAN_VALUE} = DOMProperty.injection;

var NS = {
xlink: 'http://www.w3.org/1999/xlink',
xml: 'http://www.w3.org/XML/1998/namespace',
Expand Down Expand Up @@ -113,15 +117,14 @@ var ATTRS = [
'xmlns:xlink',
'xml:lang',
'xml:space',
// The following attributes expect boolean values. They must be in
// the whitelist to allow boolean attribute assignment:
'autoReverse',
'externalResourcesRequired',
'preserveAlpha',
];

var SVGDOMPropertyConfig = {
Properties: {},
Properties: {
autoReverse: HAS_STRING_BOOLEAN_VALUE,
externalResourcesRequired: HAS_STRING_BOOLEAN_VALUE,
preserveAlpha: HAS_STRING_BOOLEAN_VALUE,
},
DOMAttributeNamespaces: {
xlinkActuate: NS.xlink,
xlinkArcrole: NS.xlink,
Expand Down
Loading