-
Notifications
You must be signed in to change notification settings - Fork 50.2k
Use a Symbol to tag every ReactElement #4832
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.
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 from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
Fixes #3473 I tag each React element with `$$typeof: Symbol.for('react.element')`. We need this to be able to safely distinguish these from plain objects that might have come from user provided JSON. The idiomatic JavaScript way of tagging an object is for it to inherent some prototype and then use `instanceof` to test for it. However, this has limitations since it doesn't work with value types which require `typeof` checks. They also don't work across realms. Which is why there are alternative tag checks like `Array.isArray` or the `toStringTag`. Another problem is that different instances of React that might have been created not knowing about eachother. npm tends to make this kind of problem occur a lot. Additionally, it is our hope that ReactElement will one day be specified in terms of a "Value Type" style record instead of a plain Object. This Value Types proposal by @nikomatsakis is currently on hold but does satisfy all these requirements: https://github.com/nikomatsakis/typed-objects-explainer/blob/master/valuetypes.md#the-typeof-operator Additionally, there is already a system for coordinating tags across module systems and even realms in ES6. Namely using `Symbol.for`. Currently these objects are not able to transfer between Workers but there is nothing preventing that from being possible in the future. You could imagine even `Symbol.for` working across Worker boundaries. You could also build a system that coordinates Symbols and Value Types from server to client or through serialized forms. That's beyond the scope of React itself, and if it was built it seems like it would belong with the `Symbol` system. A system could override the `Symbol.for('react.element')` to return a plain yet cryptographically random or unique number. That would allow ReactElements to pass through JSON without risking the XSS issue. The fallback solution is a plain well-known number. This makes it unsafe with regard to the XSS issue described in #3473. We could have used a much more convoluted solution to protect against JSON specifically but that would require some kind of significant coordination, or change the check to do a `typeof element.$$typeof === 'function'` check which would not make it unique to React. It seems cleaner to just use a fixed number since the protection is just a secondary layer anyway. I'm not sure if this is the right tradeoff. In short, if you want the XSS protection, use a proper Symbol polyfill. Finally, the reason for calling it `$$typeof` is to avoid confusion with `.type` and the use case is to add a tag that the `typeof` operator would refer to. I would use `@@typeof` but that seems to deopt in JSC. I also don't use `__typeof` because this is more than a framework private. It should really be part of the polyfilling layer.
- Loading branch information
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,6 +15,11 @@ var ReactCurrentOwner = require('ReactCurrentOwner'); | |
|
|
||
| var assign = require('Object.assign'); | ||
|
|
||
| // The Symbol used to tag the ReactElement type. If there is no native Symbol | ||
| // nor polyfill, then a plain number is used for performance. | ||
| var TYPE_SYMBOL = (typeof Symbol === 'function' && Symbol.for && | ||
| Symbol.for('react.element')) || 0xeac7; | ||
|
|
||
| var RESERVED_PROPS = { | ||
| key: true, | ||
| ref: true, | ||
|
|
@@ -52,17 +57,17 @@ if (__DEV__) { | |
| */ | ||
| var ReactElement = function(type, key, ref, self, source, owner, props) { | ||
| var element = { | ||
| // This tag allow us to uniquely identify this as a React Element | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These weren't intended for production use, and definitely not public. So I'm moving this to an underscore, |
||
| $$typeof: TYPE_SYMBOL, | ||
|
|
||
| // Built-in properties that belong on the element | ||
| type: type, | ||
| key: key, | ||
| ref: ref, | ||
| self: self, | ||
| source: source, | ||
| props: props, | ||
|
|
||
| // Record the component responsible for creating this element. | ||
| _owner: owner, | ||
|
|
||
| props: props, | ||
| }; | ||
|
|
||
| if (__DEV__) { | ||
|
|
@@ -83,8 +88,25 @@ var ReactElement = function(type, key, ref, self, source, owner, props) { | |
| writable: true, | ||
| value: false, | ||
| }); | ||
| // self and source are DEV only properties. | ||
| Object.defineProperty(element, '_self', { | ||
| configurable: false, | ||
| enumerable: false, | ||
| writable: false, | ||
| value: self, | ||
| }); | ||
| // Two elements created in two different places should be considered | ||
| // equal for testing purposes and therefore we hide it from enumeration. | ||
| Object.defineProperty(element, '_source', { | ||
| configurable: false, | ||
| enumerable: false, | ||
| writable: false, | ||
| value: source, | ||
| }); | ||
| } else { | ||
| this._store.validated = false; | ||
| element._store.validated = false; | ||
| element._self = self; | ||
| element._source = source; | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I used a single underscore here for consistency with |
||
| } | ||
| Object.freeze(element.props); | ||
| Object.freeze(element); | ||
|
|
@@ -164,12 +186,12 @@ ReactElement.createFactory = function(type) { | |
| }; | ||
|
|
||
| ReactElement.cloneAndReplaceKey = function(oldElement, newKey) { | ||
| var newElement = new ReactElement( | ||
| var newElement = ReactElement( | ||
| oldElement.type, | ||
| newKey, | ||
| oldElement.ref, | ||
| oldElement.self, | ||
| oldElement.source, | ||
| oldElement._self, | ||
| oldElement._source, | ||
| oldElement._owner, | ||
| oldElement.props | ||
| ); | ||
|
|
@@ -182,8 +204,8 @@ ReactElement.cloneAndReplaceProps = function(oldElement, newProps) { | |
| oldElement.type, | ||
| oldElement.key, | ||
| oldElement.ref, | ||
| oldElement.self, | ||
| oldElement.source, | ||
| oldElement._self, | ||
| oldElement._source, | ||
| oldElement._owner, | ||
| newProps | ||
| ); | ||
|
|
@@ -205,8 +227,12 @@ ReactElement.cloneElement = function(element, config, children) { | |
| // Reserved names are extracted | ||
| var key = element.key; | ||
| var ref = element.ref; | ||
| var self = element.__self; | ||
| var source = element.__source; | ||
| // Self is preserved since the owner is preserved. | ||
| var self = element._self; | ||
| // Source is preserved since cloneElement is unlikely to be targeted by a | ||
| // transpiler, and the original source is probably a better indicator of the | ||
| // true owner. | ||
| var source = element._source; | ||
|
|
||
| // Owner will be preserved, unless ref is overridden | ||
| var owner = element._owner; | ||
|
|
@@ -259,11 +285,10 @@ ReactElement.cloneElement = function(element, config, children) { | |
| * @final | ||
| */ | ||
| ReactElement.isValidElement = function(object) { | ||
| return !!( | ||
| return ( | ||
| typeof object === 'object' && | ||
| object != null && | ||
| 'type' in object && | ||
| 'props' in object | ||
| object !== null && | ||
| object.$$typeof === TYPE_SYMBOL | ||
| ); | ||
| }; | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -24,6 +24,10 @@ describe('ReactElement', function() { | |
| beforeEach(function() { | ||
| require('mock-modules').dumpCache(); | ||
|
|
||
| // Delete the native Symbol if we have one to ensure we test the | ||
| // unpolyfilled environment. | ||
| delete global.Symbol; | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess this is configurable?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What do you mean?
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry: I was under the impression that lots of globals aren't configurable and can't be deleted, but maybe that's not true at all. Evidently this one can be, at least.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, yes. I was also not sure so I tested. I think it is generally just
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. jsdom is a little weird. Most fields are defined using Object.defineProperty with a getter only. In order to safely overwrite it, you need to use Object.defineProperty and set it's value to null. For forward compatibility with future jsdom updates I recommend doing that here.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ohhh.. right, it is from the engine. Let's see if this test will fail with newer versions of node! Don't you still use node 0.10 for jest right now? If yes, that doesn't have Symbol, does it?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you think it's worth requiring a symbol polyfill similar to how an ES5 one is required? Seems kinda dangerous to allow older browsers to be vulnerable. On Thu, Sep 10, 2015 at 6:12 PM, Christoph Pojer [email protected]
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what @sebmck said ☝️
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See the description in the PR summary. I don't know if it's the right tradeoff. This is a secondary layer of security so just because this hole doesn't exist doesn't mean it's exploitable. E.g. it has been in React since 0.13.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We have evaluated requiring various ES6 polyfills but decided against it in the past. It was too much of a hassle for people and didn't work well with node especially. |
||
|
|
||
| React = require('React'); | ||
| ReactDOM = require('ReactDOM'); | ||
| ReactTestUtils = require('ReactTestUtils'); | ||
|
|
@@ -190,6 +194,10 @@ describe('ReactElement', function() { | |
| expect(React.isValidElement('string')).toEqual(false); | ||
| expect(React.isValidElement(React.DOM.div)).toEqual(false); | ||
| expect(React.isValidElement(Component)).toEqual(false); | ||
| expect(React.isValidElement({ type: 'div', props: {} })).toEqual(false); | ||
|
|
||
| var jsonElement = JSON.stringify(React.createElement('div')); | ||
| expect(React.isValidElement(JSON.parse(jsonElement))).toBe(true); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this supposed to work? Seems like this is the case we explicitly don't want to work (in the ideal case anyway). I was giving node v4 a try where
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's why we try to delete global.Symbol to test the case where it doesn't exist but perhaps we need to shadow it rather than delete it. Didn't try Node 4.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, just so I know, all of these tests here are explicitly for an environment where we don't have a native BUT if we do have a native
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's right. See the next test case "identifies elements, but not JSON, if Symbols are supported" which tests this. |
||
| }); | ||
|
|
||
| it('allows the use of PropTypes validators in statics', function() { | ||
|
|
@@ -305,4 +313,47 @@ describe('ReactElement', function() { | |
| expect(console.error.argsForCall.length).toBe(0); | ||
| }); | ||
|
|
||
| it('identifies elements, but not JSON, if Symbols are supported', function() { | ||
| // Rudimentary polyfill | ||
| // Once all jest engines support Symbols natively we can swap this to test | ||
| // WITH native Symbols by default. | ||
| var TYPE_SYMBOL = function() {}; // fake Symbol | ||
| var OTHER_SYMBOL = function() {}; // another fake Symbol | ||
| global.Symbol = function(name) { | ||
| return OTHER_SYMBOL; | ||
| }; | ||
| global.Symbol.for = function(key) { | ||
| if (key === 'react.element') { | ||
| return TYPE_SYMBOL; | ||
| } | ||
| return OTHER_SYMBOL; | ||
| }; | ||
|
|
||
| require('mock-modules').dumpCache(); | ||
|
|
||
| React = require('React'); | ||
|
|
||
| var Component = React.createClass({ | ||
| render: function() { | ||
| return React.createElement('div'); | ||
| }, | ||
| }); | ||
|
|
||
| expect(React.isValidElement(React.createElement('div'))) | ||
| .toEqual(true); | ||
| expect(React.isValidElement(React.createElement(Component))) | ||
| .toEqual(true); | ||
|
|
||
| expect(React.isValidElement(null)).toEqual(false); | ||
| expect(React.isValidElement(true)).toEqual(false); | ||
| expect(React.isValidElement({})).toEqual(false); | ||
| expect(React.isValidElement('string')).toEqual(false); | ||
| expect(React.isValidElement(React.DOM.div)).toEqual(false); | ||
| expect(React.isValidElement(Component)).toEqual(false); | ||
| expect(React.isValidElement({ type: 'div', props: {} })).toEqual(false); | ||
|
|
||
| var jsonElement = JSON.stringify(React.createElement('div')); | ||
| expect(React.isValidElement(JSON.parse(jsonElement))).toBe(false); | ||
| }); | ||
|
|
||
| }); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
0xeac7 0x0cc5!
(edited for hexiness, thanks @RReverser for the tip)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gaearon
ois not valid hex char :PThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
someone please tell me why 0xeac7? I can't sleep
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
0xeac7sorta kinda looks likeReactThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not use Math.random()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't cover postMessage though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We want
postMessageto work, right? In my testing, it actually does, since structured cloning supports much more advanced serialization than JSON.stringify().This is confirmed in the w3c tests: https://github.com/w3c/web-platform-tests/blob/master/workers/interfaces/DedicatedWorkerGlobalScope/postMessage/structured-clone-message.html#L32
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't want it to work cross domain by default. That's still a security risk.
However, we do want it to work if you're able to coordinate the Symbol across the worker boundaries.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would argue that if you're deliberately catching cross-origin postMessage and inserting objects directly into your views without validation, you're asking for it.
This would be nice for webworkers though, as Symbols don't transfer via structured cloning (so you'd have to explicitly catch elements and re-tag them).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See #3473 for more context.
It is too easy to expect a string and not realize it might be an object: