The library for web and native user interfaces.
Don't keep the HTML escaped ID internally, only in HTML generation
A dynamic value can be provided as a key to a child. Either as part of an object or key property. This becomes part of the component's ID. We have to be careful to escape this key before inserting it into the DOM since it could become a vulnerability. We fixed this by escaping just the keys. However, the current implementation breaks when you used escaped keys. The internal value is escaped and the value used by getAttributeNode and getElementById are both unescaped. This fixes that by keeping the unescaped value internally but escaping it right before the HTML is generated (like any other attribute). This is important since business logic IDs (that should be used as keys) contains characters that need to be escaped.
CommitSyncScript committed
Jun 28, 2013 at 16:33 UTC
15272f30f426dc02349856f4a4dc0f0dad8afb9a
4 files changed
+53
-10
src/core/ReactNativeComponent.js
+2
-1
@@ -139,7 +139,8 @@ ReactNativeComponent.Mixin = {
139
}
140
}
141
142
- return ret + ' ' + ReactID.ATTR_NAME + '="' + this._rootNodeID + '">';
142
+ var escapedID = escapeTextForBrowser(this._rootNodeID);
143
+ return ret + ' ' + ReactID.ATTR_NAME + '="' + escapedID + '">';
144
},
145
146
/**
src/core/__tests__/ReactIdentity-test.js
+46
@@ -106,6 +106,52 @@ describe('ReactIdentity', function() {
106
checkId(node.childNodes[2].firstChild, '.r[0].[0:2].[0:chipmunk]');
107
});
108
109
+ function renderAComponentWithKeyIntoContainer(key, container) {
110
+ var span1 = <span key={key} />;
111
+ var span2 = <span />;
112
+
113
+ var map = {};
114
+ map[key] = span2;
115
+
116
+ React.renderComponent(<div>{[span1, map]}</div>, container);
117
+
118
+ expect(span1.getDOMNode()).not.toBe(null);
119
+ expect(span2.getDOMNode()).not.toBe(null);
120
+
121
+ checkId(span1.getDOMNode(), '.r[0].[0:0:' + key + ']');
122
+ checkId(span2.getDOMNode(), '.r[0].[1]{' + key + '}');
123
+ }
124
+
125
+ it('should allow any character as a key, in a detached parent', function() {
126
+ var detachedContainer = document.createElement('div');
127
+ renderAComponentWithKeyIntoContainer("<'WEIRD/&\\key'>", detachedContainer);
128
+ });
129
+
130
+ it('should allow any character as a key, in an attached parent', function() {
131
+ // This test exists to protect against implementation details that
132
+ // incorrectly query escaped IDs using DOM tools like getElementById.
133
+ var attachedContainer = document.createElement('div');
134
+ document.body.appendChild(attachedContainer);
135
+
136
+ renderAComponentWithKeyIntoContainer("<'WEIRD/&\\key'>", attachedContainer);
137
+
138
+ document.body.removeChild(attachedContainer);
139
+ });
140
+
141
+ it('should not allow scripts in keys to execute', function() {
142
+ var h4x0rKey = '"><script>window.YOUVEBEENH4X0RED=true;</script><div id="';
143
+
144
+ var attachedContainer = document.createElement('div');
145
+ document.body.appendChild(attachedContainer);
146
+
147
+ renderAComponentWithKeyIntoContainer(h4x0rKey, attachedContainer);
148
+
149
+ document.body.removeChild(attachedContainer);
150
+
151
+ // If we get this far, make sure we haven't executed the code
152
+ expect(window.YOUVEBEENH4X0RED).toBe(undefined);
153
+ });
154
+
155
it('should let restructured components retain their uniqueness', function() {
156
var instance0 = <span />;
157
var instance1 = <span />;
src/utils/__tests__/mapChildren-test.js
+2
-2
@@ -94,7 +94,7 @@ describe('mapChildren', function() {
94
95
expect(rendered.props.children[0]).not.toBe(simpleKid);
96
expect(rendered.props.children[0].props.children[0]).toBe(simpleKid);
97
- expect(rendered.props.children).toHaveKeys(['0:0:0:simple']);
97
+ expect(rendered.props.children).toHaveKeys(['1:0:0:simple']);
98
expect(rendered.props.children[0].props.children)
99
.toHaveKeys(['0:simple']);
100
});
@@ -128,7 +128,7 @@ describe('mapChildren', function() {
128
expect(mapFn).toHaveBeenCalledWith(kidThree, '0:three', 2);
129
expect(rendered.props.children).not.toEqual(instance.props.children);
130
expect(rendered.props.children).toHaveKeys([
131
- '0:0:0:one', '0:0:0:two', '0:0:0:three'
131
+ '1:0:0:one', '1:0:0:two', '1:0:0:three'
132
]);
133
});
134
});
src/utils/flattenChildren.js
+3
-7
@@ -20,8 +20,6 @@
20
21
var ReactTextComponent = require('ReactTextComponent');
22
23
-var escapeTextForBrowser = require('escapeTextForBrowser');
24
-
23
var throwIf = require('throwIf');
24
25
/**
@@ -47,16 +45,15 @@ var DUPLICATE_KEY_ERROR =
45
var ONLY_CHILD_NAME = '0';
46
47
var flattenChildrenImpl = function(res, children, nameSoFar) {
50
- var key, escapedKey;
48
+ var key;
49
if (Array.isArray(children)) {
50
for (var i = 0; i < children.length; i++) {
51
var child = children[i];
52
key = child && (child._key || (child.props && child.props.key));
55
- escapedKey = key ? escapeTextForBrowser(key) : ('' + i);
53
flattenChildrenImpl(
54
res,
55
child,
59
- nameSoFar + '[' + escapedKey + ']'
56
+ nameSoFar + '[' + (key ? key : ('' + i)) + ']'
57
);
58
}
59
} else {
@@ -76,11 +73,10 @@ var flattenChildrenImpl = function(res, children, nameSoFar) {
73
throwIf(children && children.nodeType === 1, INVALID_CHILD);
74
for (key in children) {
75
if (children.hasOwnProperty(key)) {
79
- escapedKey = escapeTextForBrowser(key);
76
flattenChildrenImpl(
77
res,
78
children[key],
83
- nameSoFar + '{' + escapedKey + '}'
79
+ nameSoFar + '{' + key + '}'
80
);
81
}
82
}