From 3092f63c6bbd6190c3b4410fb41643e4959bfd22 Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Tue, 13 Dec 2016 14:52:51 -0800 Subject: [PATCH 1/3] Warn if upper-case tags are used In #2756 we ended up using toLowerCase to allow case insensitive HTML tags. However, this requires extra processing every time we access the tag or at least we need to process it for mount and store it in an extra field which wastes memory. So instead, we can just enforce case sensitivity for HTML since this might matter for the XML namespaces like SVG anyway. --- scripts/fiber/tests-passing.txt | 1 + .../dom/fiber/ReactDOMFiberComponent.js | 15 +++++-- .../__tests__/ReactDOMComponent-test.js | 41 +++++++++++-------- .../dom/stack/client/ReactDOMComponent.js | 9 ++++ 4 files changed, 46 insertions(+), 20 deletions(-) diff --git a/scripts/fiber/tests-passing.txt b/scripts/fiber/tests-passing.txt index 6eca915bf16..ed380ce55e1 100644 --- a/scripts/fiber/tests-passing.txt +++ b/scripts/fiber/tests-passing.txt @@ -640,6 +640,7 @@ src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js * should handle dangerouslySetInnerHTML * should work error event on element * should not duplicate uppercased selfclosing tags +* should warn on upper case HTML tags, not SVG nor custom tags * should warn against children for void elements * should warn against dangerouslySetInnerHTML for void elements * should emit a warning once for an unnamed custom component using shady DOM diff --git a/src/renderers/dom/fiber/ReactDOMFiberComponent.js b/src/renderers/dom/fiber/ReactDOMFiberComponent.js index 826dcc5b6a0..18b3341920e 100644 --- a/src/renderers/dom/fiber/ReactDOMFiberComponent.js +++ b/src/renderers/dom/fiber/ReactDOMFiberComponent.js @@ -489,8 +489,6 @@ var ReactDOMFiberComponent = { parentNamespace : string | null ) : Element { validateDangerousTag(type); - // TODO: - // const tag = type.toLowerCase(); Do we need to apply lower case only on non-custom elements? // We create tags in the namespace of their parent container, except HTML // tags get no namespace. @@ -498,8 +496,17 @@ var ReactDOMFiberComponent = { var domElement : Element; var namespaceURI = parentNamespace || getIntrinsicNamespace(type); if (namespaceURI == null) { - const tag = type.toLowerCase(); - if (tag === 'script') { + if (__DEV__) { + warning( + type === type.toLowerCase() || + isCustomComponent(type, props), + '<%s /> is using uppercase HTML. Always use lowercase HTML tags ' + + 'in React.', + type + ); + } + + if (type === 'script') { // Create the script via .innerHTML so its "parser-inserted" flag is // set to true and it does not execute var div = ownerDocument.createElement('div'); diff --git a/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js b/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js index 2a263cc63d6..0ab5a21e389 100644 --- a/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js +++ b/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js @@ -14,6 +14,7 @@ describe('ReactDOMComponent', () => { var React; + var ReactTestUtils; var ReactDOM; var ReactDOMFeatureFlags; var ReactDOMServer; @@ -29,16 +30,11 @@ describe('ReactDOMComponent', () => { ReactDOM = require('ReactDOM'); ReactDOMFeatureFlags = require('ReactDOMFeatureFlags'); ReactDOMServer = require('ReactDOMServer'); + ReactTestUtils = require('ReactTestUtils'); inputValueTracking = require('inputValueTracking'); }); describe('updateDOM', () => { - var ReactTestUtils; - - beforeEach(() => { - ReactTestUtils = require('ReactTestUtils'); - }); - it('should handle className', () => { var container = document.createElement('div'); ReactDOM.render(
, container); @@ -795,6 +791,7 @@ describe('ReactDOMComponent', () => { }); it('should not duplicate uppercased selfclosing tags', () => { + spyOn(console, 'error'); class Container extends React.Component { render() { return React.createElement('BR', null); @@ -803,6 +800,27 @@ describe('ReactDOMComponent', () => { var returnedValue = ReactDOMServer.renderToString(); expect(returnedValue).not.toContain('
'); + expectDev(console.error.calls.count()).toBe(1); + expectDev(console.error.calls.argsFor(0)[0]).toContain( + '
is using uppercase HTML.' + ); + }); + + it('should warn on upper case HTML tags, not SVG nor custom tags', () => { + spyOn(console, 'error'); + ReactTestUtils.renderIntoDocument( + React.createElement('svg', null, React.createElement('PATH')) + ); + expectDev(console.error.calls.count()).toBe(0); + ReactTestUtils.renderIntoDocument( + React.createElement('CUSTOM-TAG') + ); + expectDev(console.error.calls.count()).toBe(0); + ReactTestUtils.renderIntoDocument(React.createElement('IMG')); + expectDev(console.error.calls.count()).toBe(1); + expectDev(console.error.calls.argsFor(0)[0]).toContain( + ' is using uppercase HTML.' + ); }); it('should warn against children for void elements', () => { @@ -1171,8 +1189,7 @@ describe('ReactDOMComponent', () => { .mock('isEventSupported'); var isEventSupported = require('isEventSupported'); isEventSupported.mockReturnValueOnce(false); - - var ReactTestUtils = require('ReactTestUtils'); + ReactTestUtils = require('ReactTestUtils'); spyOn(console, 'error'); ReactTestUtils.renderIntoDocument(
); @@ -1191,7 +1208,6 @@ describe('ReactDOMComponent', () => { describe('tag sanitization', () => { it('should throw when an invalid tag name is used', () => { - var ReactTestUtils = require('ReactTestUtils'); var hackzor = React.createElement('script tag'); expect( () => ReactTestUtils.renderIntoDocument(hackzor) @@ -1201,7 +1217,6 @@ describe('ReactDOMComponent', () => { }); it('should throw when an attack vector is used', () => { - var ReactTestUtils = require('ReactTestUtils'); var hackzor = React.createElement('div> ReactTestUtils.renderIntoDocument(hackzor) @@ -1212,12 +1227,6 @@ describe('ReactDOMComponent', () => { }); describe('nesting validation', () => { - var ReactTestUtils; - - beforeEach(() => { - ReactTestUtils = require('ReactTestUtils'); - }); - it('warns on invalid nesting', () => { spyOn(console, 'error'); ReactTestUtils.renderIntoDocument(
); diff --git a/src/renderers/dom/stack/client/ReactDOMComponent.js b/src/renderers/dom/stack/client/ReactDOMComponent.js index 13f61ae8c4c..ee88dce2136 100644 --- a/src/renderers/dom/stack/client/ReactDOMComponent.js +++ b/src/renderers/dom/stack/client/ReactDOMComponent.js @@ -524,6 +524,15 @@ ReactDOMComponent.Mixin = { namespaceURI = DOMNamespaces.html; } if (namespaceURI === DOMNamespaces.html) { + if (__DEV__) { + warning( + isCustomComponent(this._tag, props) || + this._tag === this._currentElement.type, + '<%s /> is using uppercase HTML. Always use lowercase HTML tags ' + + 'in React.', + this._currentElement.type + ); + } if (this._tag === 'svg') { namespaceURI = DOMNamespaces.svg; } else if (this._tag === 'math') { From db489a4adeea8edd2aab1d47b3ce55dd28198e4b Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Mon, 12 Dec 2016 19:38:34 -0800 Subject: [PATCH 2/3] Only do runtime validation of tag names when createElement is not used We introduced runtime validation of tag names because we used to generate HTML that was supposed to be inserted into a HTML string which could've been an XSS attack. However, these days we use document.createElement in most cases. That already does its internal validation in the browser which throws. We're now double validating it. Stack still has a path where innerHTML is used and we still need it there. However in Fiber we can remove it completely. --- scripts/fiber/tests-passing.txt | 2 ++ .../dom/fiber/ReactDOMFiberComponent.js | 17 -------------- .../__tests__/ReactDOMComponent-test.js | 22 +++++++++++++++---- .../dom/stack/client/ReactDOMComponent.js | 2 +- 4 files changed, 21 insertions(+), 22 deletions(-) diff --git a/scripts/fiber/tests-passing.txt b/scripts/fiber/tests-passing.txt index ed380ce55e1..002f3c042ff 100644 --- a/scripts/fiber/tests-passing.txt +++ b/scripts/fiber/tests-passing.txt @@ -664,6 +664,8 @@ src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js * should properly escape text content and attributes values * unmounts children before unsetting DOM node info * should warn about the `onScroll` issue when unsupported (IE8) +* should throw when an invalid tag name is used server-side +* should throw when an attack vector is used server-side * should throw when an invalid tag name is used * should throw when an attack vector is used * should warn about props that are no longer supported diff --git a/src/renderers/dom/fiber/ReactDOMFiberComponent.js b/src/renderers/dom/fiber/ReactDOMFiberComponent.js index 18b3341920e..44afff611e8 100644 --- a/src/renderers/dom/fiber/ReactDOMFiberComponent.js +++ b/src/renderers/dom/fiber/ReactDOMFiberComponent.js @@ -279,21 +279,6 @@ var voidElementTags = { ...omittedCloseTags, }; -// We accept any tag to be rendered but since this gets injected into arbitrary -// HTML, we want to make sure that it's a safe tag. -// http://www.w3.org/TR/REC-xml/#NT-Name - -var VALID_TAG_REGEX = /^[a-zA-Z][a-zA-Z:_\.\-\d]*$/; // Simplified subset -var validatedTagCache = {}; -var hasOwnProperty = {}.hasOwnProperty; - -function validateDangerousTag(tag) { - if (!hasOwnProperty.call(validatedTagCache, tag)) { - invariant(VALID_TAG_REGEX.test(tag), 'Invalid tag: %s', tag); - validatedTagCache[tag] = true; - } -} - function isCustomComponent(tagName, props) { return tagName.indexOf('-') >= 0 || props.is != null; } @@ -488,8 +473,6 @@ var ReactDOMFiberComponent = { rootContainerElement : Element, parentNamespace : string | null ) : Element { - validateDangerousTag(type); - // We create tags in the namespace of their parent container, except HTML // tags get no namespace. var ownerDocument = rootContainerElement.ownerDocument; diff --git a/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js b/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js index 0ab5a21e389..6c5661e6663 100644 --- a/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js +++ b/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js @@ -1207,23 +1207,37 @@ describe('ReactDOMComponent', () => { }); describe('tag sanitization', () => { - it('should throw when an invalid tag name is used', () => { + it('should throw when an invalid tag name is used server-side', () => { var hackzor = React.createElement('script tag'); expect( - () => ReactTestUtils.renderIntoDocument(hackzor) + () => ReactDOMServer.renderToString(hackzor) ).toThrowError( 'Invalid tag: script tag' ); }); - it('should throw when an attack vector is used', () => { + it('should throw when an attack vector is used server-side', () => { var hackzor = React.createElement('div> ReactTestUtils.renderIntoDocument(hackzor) + () => ReactDOMServer.renderToString(hackzor) ).toThrowError( 'Invalid tag: div> { + var hackzor = React.createElement('script tag'); + expect( + () => ReactTestUtils.renderIntoDocument(hackzor) + ).toThrow(); + }); + + it('should throw when an attack vector is used', () => { + var hackzor = React.createElement('div> ReactTestUtils.renderIntoDocument(hackzor) + ).toThrow(); + }); }); describe('nesting validation', () => { diff --git a/src/renderers/dom/stack/client/ReactDOMComponent.js b/src/renderers/dom/stack/client/ReactDOMComponent.js index ee88dce2136..b23a736b083 100644 --- a/src/renderers/dom/stack/client/ReactDOMComponent.js +++ b/src/renderers/dom/stack/client/ReactDOMComponent.js @@ -413,7 +413,6 @@ var globalIdCounter = 1; */ function ReactDOMComponent(element) { var tag = element.type; - validateDangerousTag(tag); this._currentElement = element; this._tag = tag.toLowerCase(); this._namespaceURI = null; @@ -605,6 +604,7 @@ ReactDOMComponent.Mixin = { this._createInitialChildren(transaction, props, context, lazyTree); mountImage = lazyTree; } else { + validateDangerousTag(this._tag); var tagOpen = this._createOpenTagMarkupAndPutListeners(transaction, props); var tagContent = this._createContentMarkup(transaction, props, context); if (!tagContent && omittedCloseTags[this._tag]) { From 6c1592f3842fb2bee94e82e84d64b17dec39b38b Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Tue, 13 Dec 2016 14:37:06 -0800 Subject: [PATCH 3/3] Pass type to the relevant host config methods Instead of reading it from the DOM node. --- src/renderers/art/ReactARTFiber.js | 4 ++-- src/renderers/dom/fiber/ReactDOMFiber.js | 19 +++++-------------- src/renderers/noop/ReactNoop.js | 6 +++--- .../shared/fiber/ReactFiberCommitWork.js | 3 ++- .../shared/fiber/ReactFiberCompleteWork.js | 7 ++++--- .../shared/fiber/ReactFiberReconciler.js | 6 +++--- 6 files changed, 19 insertions(+), 26 deletions(-) diff --git a/src/renderers/art/ReactARTFiber.js b/src/renderers/art/ReactARTFiber.js index c4fa6d7f5ea..ffa9cc419cb 100644 --- a/src/renderers/art/ReactARTFiber.js +++ b/src/renderers/art/ReactARTFiber.js @@ -408,7 +408,7 @@ const ARTRenderer = ReactFiberReconciler({ // Noop }, - commitUpdate(instance, oldProps, newProps) { + commitUpdate(instance, type, oldProps, newProps) { instance._applyProps(instance, newProps, oldProps); }, @@ -467,7 +467,7 @@ const ARTRenderer = ReactFiberReconciler({ // Noop }, - prepareUpdate(domElement, oldProps, newProps) { + prepareUpdate(domElement, type, oldProps, newProps) { return true; }, diff --git a/src/renderers/dom/fiber/ReactDOMFiber.js b/src/renderers/dom/fiber/ReactDOMFiber.js index eecdb8181e7..4c38e66f6be 100644 --- a/src/renderers/dom/fiber/ReactDOMFiber.js +++ b/src/renderers/dom/fiber/ReactDOMFiber.js @@ -97,20 +97,16 @@ var DOMRenderer = ReactFiberReconciler({ finalizeInitialChildren( domElement : Instance, + type : string, props : Props, rootContainerInstance : Container, ) : void { - // TODO: we normalize here because DOM renderer expects tag to be lowercase. - // We can change DOM renderer to compare special case against upper case, - // and use tagName (which is upper case for HTML DOM elements). Or we could - // let the renderer "normalize" the fiber type so we don't have to read - // the type from DOM. However we need to remember SVG is case-sensitive. - var tag = domElement.tagName.toLowerCase(); - setInitialProperties(domElement, tag, props, rootContainerInstance); + setInitialProperties(domElement, type, props, rootContainerInstance); }, prepareUpdate( domElement : Instance, + type : string, oldProps : Props, newProps : Props ) : boolean { @@ -119,21 +115,16 @@ var DOMRenderer = ReactFiberReconciler({ commitUpdate( domElement : Instance, + type : string, oldProps : Props, newProps : Props, rootContainerInstance : Container, internalInstanceHandle : Object, ) : void { - // TODO: we normalize here because DOM renderer expects tag to be lowercase. - // We can change DOM renderer to compare special case against upper case, - // and use tagName (which is upper case for HTML DOM elements). Or we could - // let the renderer "normalize" the fiber type so we don't have to read - // the type from DOM. However we need to remember SVG is case-sensitive. - var tag = domElement.tagName.toLowerCase(); // Update the internal instance handle so that we know which props are // the current ones. precacheFiberNode(internalInstanceHandle, domElement); - updateProperties(domElement, tag, oldProps, newProps, rootContainerInstance); + updateProperties(domElement, type, oldProps, newProps, rootContainerInstance); }, shouldSetTextContent(props : Props) : boolean { diff --git a/src/renderers/noop/ReactNoop.js b/src/renderers/noop/ReactNoop.js index 97cb1578faa..d16b6e7bf2f 100644 --- a/src/renderers/noop/ReactNoop.js +++ b/src/renderers/noop/ReactNoop.js @@ -60,15 +60,15 @@ var NoopRenderer = ReactFiberReconciler({ parentInstance.children.push(child); }, - finalizeInitialChildren(domElement : Instance, props : Props) : void { + finalizeInitialChildren(domElement : Instance, type : string, props : Props) : void { // Noop }, - prepareUpdate(instance : Instance, oldProps : Props, newProps : Props) : boolean { + prepareUpdate(instance : Instance, type : string, oldProps : Props, newProps : Props) : boolean { return true; }, - commitUpdate(instance : Instance, oldProps : Props, newProps : Props) : void { + commitUpdate(instance : Instance, type : string, oldProps : Props, newProps : Props) : void { instance.prop = newProps.prop; }, diff --git a/src/renderers/shared/fiber/ReactFiberCommitWork.js b/src/renderers/shared/fiber/ReactFiberCommitWork.js index 46f0fdc94b5..5ff31e2d514 100644 --- a/src/renderers/shared/fiber/ReactFiberCommitWork.js +++ b/src/renderers/shared/fiber/ReactFiberCommitWork.js @@ -373,7 +373,8 @@ module.exports = function( const newProps = finishedWork.memoizedProps; const oldProps = current.memoizedProps; const rootContainerInstance = getRootHostContainer(); - commitUpdate(instance, oldProps, newProps, rootContainerInstance, finishedWork); + const type = finishedWork.type; + commitUpdate(instance, type, oldProps, newProps, rootContainerInstance, finishedWork); } detachRefIfNeeded(current, finishedWork); return; diff --git a/src/renderers/shared/fiber/ReactFiberCompleteWork.js b/src/renderers/shared/fiber/ReactFiberCompleteWork.js index 84af8a456d8..ea85f1fe60a 100644 --- a/src/renderers/shared/fiber/ReactFiberCompleteWork.js +++ b/src/renderers/shared/fiber/ReactFiberCompleteWork.js @@ -215,6 +215,7 @@ module.exports = function( } case HostComponent: popHostContext(workInProgress); + const type = workInProgress.type; let newProps = workInProgress.pendingProps; if (current && workInProgress.stateNode != null) { // If we have an alternate, that means this is an update and we need to @@ -228,7 +229,7 @@ module.exports = function( newProps = workInProgress.memoizedProps || oldProps; } const instance : I = workInProgress.stateNode; - if (prepareUpdate(instance, oldProps, newProps)) { + if (prepareUpdate(instance, type, oldProps, newProps)) { // This returns true if there was something to update. markUpdate(workInProgress); } @@ -249,14 +250,14 @@ module.exports = function( // or completeWork depending on we want to add then top->down or // bottom->up. Top->down is faster in IE11. const instance = createInstance( - workInProgress.type, + type, newProps, rootContainerInstance, currentHostContext, workInProgress ); appendAllChildren(instance, workInProgress); - finalizeInitialChildren(instance, newProps, rootContainerInstance); + finalizeInitialChildren(instance, type, newProps, rootContainerInstance); workInProgress.stateNode = instance; if (workInProgress.ref) { diff --git a/src/renderers/shared/fiber/ReactFiberReconciler.js b/src/renderers/shared/fiber/ReactFiberReconciler.js index 08204a048be..571d9aa921d 100644 --- a/src/renderers/shared/fiber/ReactFiberReconciler.js +++ b/src/renderers/shared/fiber/ReactFiberReconciler.js @@ -46,10 +46,10 @@ export type HostConfig = { createInstance(type : T, props : P, rootContainerInstance : C, hostContext : CX | null, internalInstanceHandle : OpaqueNode) : I, appendInitialChild(parentInstance : I, child : I | TI) : void, - finalizeInitialChildren(parentInstance : I, props : P, rootContainerInstance : C) : void, + finalizeInitialChildren(parentInstance : I, type : T, props : P, rootContainerInstance : C) : void, - prepareUpdate(instance : I, oldProps : P, newProps : P) : boolean, - commitUpdate(instance : I, oldProps : P, newProps : P, rootContainerInstance : C, internalInstanceHandle : OpaqueNode) : void, + prepareUpdate(instance : I, type : T, oldProps : P, newProps : P) : boolean, + commitUpdate(instance : I, type : T, oldProps : P, newProps : P, rootContainerInstance : C, internalInstanceHandle : OpaqueNode) : void, shouldSetTextContent(props : P) : boolean, resetTextContent(instance : I) : void,