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

Use a Symbol to tag every ReactElement #4832

Merged
merged 1 commit into from Sep 10, 2015
Jump to file or symbol
Failed to load files and symbols.
+93 −16
Diff settings

Always

Just for now

@@ -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;

This comment has been minimized.

@gaearon

gaearon Sep 13, 2015

Member

0xeac7 0x0cc5!

(edited for hexiness, thanks @RReverser for the tip)

This comment has been minimized.

@RReverser

RReverser Sep 14, 2015

Contributor

@gaearon o is not valid hex char :P

This comment has been minimized.

@chicoxyzzy

chicoxyzzy Sep 14, 2015

Contributor

someone please tell me why 0xeac7? I can't sleep

This comment has been minimized.

@zpao

zpao Sep 14, 2015

Member

0xeac7 sorta kinda looks like React

This comment has been minimized.

@yiminghe

yiminghe Sep 16, 2015

Contributor

why not use Math.random()

This comment has been minimized.

@sebmarkbage

sebmarkbage Sep 16, 2015

Member

Won't work between multiple instances of React and across realms.

On Sep 16, 2015, at 8:08 AM, yiminghe notifications@github.com wrote:

In src/isomorphic/classic/element/ReactElement.js:

@@ -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;
    
    why not use Math.random()


Reply to this email directly or view it on GitHub.

This comment has been minimized.

@STRML

STRML Oct 14, 2015

Contributor

Wouldn't Infinity be a better magicnum? It can't be JSON-encoded, so we still get the XSS protection on browsers that don't support Symbols.

In fact, it would be a lot simpler in general, because we're running into a lot of issues with the use of Symbol here, such as #5138, and the above jsdom issues.

This comment has been minimized.

@sebmarkbage

sebmarkbage Oct 14, 2015

Member

@STRML That's a good point. I was hoping to avoid boxing. NaN or something like that would work too. [I wish this came last week because now multiple versions of React/Babel will break if we change it. 😞 Probably should find a way to change it anyway.]

This comment has been minimized.

@sebmarkbage

sebmarkbage Oct 14, 2015

Member

It doesn't cover postMessage though.

This comment has been minimized.

@STRML

STRML Oct 14, 2015

Contributor

We want postMessage to 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

This comment has been minimized.

@sebmarkbage

sebmarkbage Oct 14, 2015

Member

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.

This comment has been minimized.

@STRML

STRML Oct 14, 2015

Contributor

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).

This comment has been minimized.

@sebmarkbage

sebmarkbage Oct 14, 2015

Member

See #3473 for more context.

It is too easy to expect a string and not realize it might be an object:

loadString(function(stringData) {
  React.render(<div>Content: {stringData}</div>, container);
});
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
$$typeof: TYPE_SYMBOL,
// Built-in properties that belong on the element
type: type,
key: key,
ref: ref,
self: self,
source: source,

This comment has been minimized.

@sebmarkbage

sebmarkbage Sep 10, 2015

Member

These weren't intended for production use, and definitely not public. So I'm moving this to an underscore, __DEV__ and making them non-enumerable. Otherwise jest's equality helpers won't work on elements created in two different source locations.

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;

This comment has been minimized.

@sebmarkbage

sebmarkbage Sep 10, 2015

Member

Fixed this bug in older browsers.

element._store.validated = false;
element._self = self;
element._source = source;

This comment has been minimized.

@sebmarkbage

sebmarkbage Sep 10, 2015

Member

I used a single underscore here for consistency with _owner and _store while the properties in config use the double underscore.

}
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
);
};
@@ -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;

This comment has been minimized.

@sophiebits

sophiebits Sep 10, 2015

Member

I guess this is configurable?

This comment has been minimized.

@sebmarkbage

sebmarkbage Sep 10, 2015

Member

What do you mean?

This comment has been minimized.

@sophiebits

sophiebits Sep 10, 2015

Member

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.

This comment has been minimized.

@sebmarkbage

sebmarkbage Sep 10, 2015

Member

Ah, yes. I was also not sure so I tested. I think it is generally just window and document and a few more that can't be deleted.

This comment has been minimized.

@cpojer

cpojer Sep 10, 2015

Contributor

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.

This comment has been minimized.

@sophiebits

sophiebits Sep 10, 2015

Member

Well Symbol isn't from jsdom, right? Could add an expect(typeof Symbol).toBe('undefined'); right after to be safe.

Also: we should probably restore the old Symbol global after this test.

This comment has been minimized.

@cpojer

cpojer Sep 10, 2015

Contributor

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?

This comment has been minimized.

@kittens

kittens Sep 10, 2015

Member

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 notifications@github.com
wrote:

@@ -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;
    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?

Reply to this email directly or view it on GitHub:
https://github.com/facebook/react/pull/4832/files#r39185824

This comment has been minimized.

@chicoxyzzy

chicoxyzzy Sep 10, 2015

Contributor

what @sebmck said ☝️

This comment has been minimized.

@sebmarkbage

sebmarkbage Sep 10, 2015

Member

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.

This comment has been minimized.

@sebmarkbage

sebmarkbage Sep 10, 2015

Member

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);

This comment has been minimized.

@zpao

zpao Sep 13, 2015

Member

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 Symbol exists and this fails because $$typeof gets removed when stringifying.

This comment has been minimized.

@sebmarkbage

sebmarkbage Sep 13, 2015

Member

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.

This comment has been minimized.

@zpao

zpao Sep 13, 2015

Member

Ok, just so I know, all of these tests here are explicitly for an environment where we don't have a native Symbol. In that case we expect React.isValidElement(JSON.parse(JSON.stringify(element))) to be true.

BUT if we do have a native Symbol (and weren't deleting it properly), then we would not expect this test to pass?

This comment has been minimized.

@sophiebits

sophiebits Sep 13, 2015

Member

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);
});
});
@@ -153,6 +153,7 @@ describe('ReactJSXElement', function() {
expect(React.isValidElement({})).toEqual(false);
expect(React.isValidElement('string')).toEqual(false);
expect(React.isValidElement(Component)).toEqual(false);
expect(React.isValidElement({ type: 'div', props: {} })).toEqual(false);
});
it('is indistinguishable from a plain object', function() {
ProTip! Use n and p to navigate between commits in a pull request.