Merge pull request #3067 from jimfb/arbitrary-attributes-for-dash-ele…

…ments Support arbitrary attributes on elements with dashes in the tag name.
facebook · Jun 3, 2015 · 9a02ea2 · 9a02ea2
2 parents 4c3e965 + b1db817
commit 9a02ea2
Show file tree

Hide file tree

Showing 5 changed files with 146 additions and 7 deletions.
diff --git a/src/renderers/dom/client/ReactDOMIDOperations.js b/src/renderers/dom/client/ReactDOMIDOperations.js
@@ -66,6 +66,24 @@ var ReactDOMIDOperations = {
     }
   },
 
+  /**
+   * Updates a DOM node with new property values.
+   *
+   * @param {string} id ID of the node to update.
+   * @param {string} name A valid property name.
+   * @param {*} value New value of the property.
+   * @internal
+   */
+  updateAttributeByID: function(id, name, value) {
+    var node = ReactMount.getNode(id);
+    invariant(
+      !INVALID_PROPERTY_ERRORS.hasOwnProperty(name),
+      'updatePropertyByID(...): %s',
+      INVALID_PROPERTY_ERRORS[name]
+    );
+    DOMPropertyOperations.setValueForAttribute(node, name, value);
+  },
+
   /**
    * Updates a DOM node to remove a property. This should only be used to remove
    * DOM properties in `DOMProperty`.

diff --git a/src/renderers/dom/shared/DOMPropertyOperations.js b/src/renderers/dom/shared/DOMPropertyOperations.js
@@ -17,6 +17,31 @@ var DOMProperty = require('DOMProperty');
 var quoteAttributeValueForBrowser = require('quoteAttributeValueForBrowser');
 var warning = require('warning');
 
+// Simplified subset
+var VALID_ATTRIBUTE_NAME_REGEX = /^[a-zA-Z_][a-zA-Z_\.\-\d]*$/;
+var illegalAttributeNameCache = {};
+var validatedAttributeNameCache = {};
+
+function isAttributeNameSafe(attributeName) {
+  if (validatedAttributeNameCache.hasOwnProperty(attributeName)) {
+    return true;
+  }
+  if (illegalAttributeNameCache.hasOwnProperty(attributeName)) {
+    return false;
+  }
+  if (VALID_ATTRIBUTE_NAME_REGEX.test(attributeName)) {
+    validatedAttributeNameCache[attributeName] = true;
+    return true;
+  }
+  illegalAttributeNameCache[attributeName] = true;
+  warning(
+    false,
+    'Invalid attribute name: `%s`',
+    attributeName
+  );
+  return false;
+}
+
 function shouldIgnoreValue(name, value) {
   return value == null ||
     (DOMProperty.hasBooleanValue[name] && !value) ||
@@ -110,6 +135,20 @@ var DOMPropertyOperations = {
     return null;
   },
 
+  /**
+   * Creates markup for a custom property.
+   *
+   * @param {string} name
+   * @param {*} value
+   * @return {string} Markup string, or empty string if the property was invalid.
+   */
+  createMarkupForCustomAttribute: function(name, value) {
+    if (!isAttributeNameSafe(name) || value == null) {
+      return '';
+    }
+    return name + '=' + quoteAttributeValueForBrowser(value);
+  },
+
   /**
    * Sets the value for a property on a node.
    *
@@ -147,16 +186,23 @@ var DOMPropertyOperations = {
         }
       }
     } else if (DOMProperty.isCustomAttribute(name)) {
-      if (value == null) {
-        node.removeAttribute(name);
-      } else {
-        node.setAttribute(name, '' + value);
-      }
+      DOMPropertyOperations.setValueForAttribute(node, name, value);
     } else if (__DEV__) {
       warnUnknownProperty(name);
     }
   },
 
+  setValueForAttribute: function(node, name, value) {
+    if (!isAttributeNameSafe(name)) {
+      return;
+    }
+    if (value == null) {
+      node.removeAttribute(name);
+    } else {
+      node.setAttribute(name, '' + value);
+    }
+  },
+
   /**
    * Deletes the value for a property on a node.
    *

diff --git a/src/renderers/dom/shared/ReactDOMComponent.js b/src/renderers/dom/shared/ReactDOMComponent.js
@@ -351,8 +351,12 @@ ReactDOMComponent.Mixin = {
           }
           propValue = CSSPropertyOperations.createMarkupForStyles(propValue);
         }
-        var markup =
-          DOMPropertyOperations.createMarkupForProperty(propKey, propValue);
+        var markup = null;
+        if (this._tag != null && this._tag.indexOf('-') >= 0) {
+          markup = DOMPropertyOperations.createMarkupForCustomAttribute(propKey, propValue);
+        } else {
+          markup = DOMPropertyOperations.createMarkupForProperty(propKey, propValue);
+        }
         if (markup) {
           ret += ' ' + markup;
         }
@@ -565,6 +569,12 @@ ReactDOMComponent.Mixin = {
         } else if (lastProp) {
           deleteListener(this._rootNodeID, propKey);
         }
+      } else if (this._tag.indexOf('-') >= 0) {
+        BackendIDOperations.updateAttributeByID(
+          this._rootNodeID,
+          propKey,
+          nextProp
+        );
       } else if (
           DOMProperty.isStandardName[propKey] ||
           DOMProperty.isCustomAttribute(propKey)) {

diff --git a/src/renderers/dom/shared/__tests__/DOMPropertyOperations-test.js b/src/renderers/dom/shared/__tests__/DOMPropertyOperations-test.js
@@ -179,6 +179,21 @@ describe('DOMPropertyOperations', function() {
 
   });
 
+  describe('createMarkupForProperty', function() {
+
+    it('should allow custom properties on web components', function() {
+      expect(DOMPropertyOperations.createMarkupForCustomAttribute(
+        'awesomeness',
+        5
+      )).toBe('awesomeness="5"');
+
+      expect(DOMPropertyOperations.createMarkupForCustomAttribute(
+        'dev',
+        'jim'
+      )).toBe('dev="jim"');
+    });
+  });
+
   describe('setValueForProperty', function() {
     var stubNode;
 

diff --git a/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js b/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js
@@ -21,6 +21,7 @@ describe('ReactDOMComponent', function() {
     var ReactTestUtils;
 
     beforeEach(function() {
+      require('mock-modules').dumpCache();
       React = require('React');
       ReactTestUtils = require('ReactTestUtils');
     });
@@ -203,6 +204,55 @@ describe('ReactDOMComponent', function() {
       expect(stubStyle.color).toEqual('green');
     });
 
+    it('should reject attribute key injection attack on markup', function() {
+      spyOn(console, 'error');
+      for (var i = 0; i < 3; i++) {
+        var container = document.createElement('div');
+        var element = React.createElement(
+          'x-foo-component',
+          {'blah" onclick="beevil" noise="hi': 'selected'},
+          null
+        );
+        React.render(element, container);
+      }
+      expect(console.error.argsForCall.length).toBe(1);
+      expect(console.error.argsForCall[0][0]).toEqual(
+        'Warning: Invalid attribute name: `blah" onclick="beevil" noise="hi`'
+      );
+    });
+
+    it('should reject attribute key injection attack on update', function() {
+      spyOn(console, 'error');
+      for (var i = 0; i < 3; i++) {
+        var container = document.createElement('div');
+        var beforeUpdate = React.createElement('x-foo-component', {}, null);
+        React.render(beforeUpdate, container);
+
+        var afterUpdate = React.createElement(
+          'x-foo-component',
+          {'blah" onclick="beevil" noise="hi': 'selected'},
+          null
+        );
+        React.render(afterUpdate, container);
+      }
+      expect(console.error.argsForCall.length).toBe(1);
+      expect(console.error.argsForCall[0][0]).toEqual(
+        'Warning: Invalid attribute name: `blah" onclick="beevil" noise="hi`'
+      );
+    });
+
+    it('should update arbitrary attributes for tags containing dashes', function() {
+      var container = document.createElement('div');
+
+      var beforeUpdate = React.createElement('x-foo-component', {}, null);
+      React.render(beforeUpdate, container);
+
+      var afterUpdate = <x-foo-component myattr="myval" />;
+      React.render(afterUpdate, container);
+
+      expect(container.childNodes[0].getAttribute('myattr')).toBe('myval');
+    });
+
     it('should clear all the styles when removing `style`', function() {
       var styles = {display: 'none', color: 'red'};
       var container = document.createElement('div');