Skip to content
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
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
57 changes: 41 additions & 16 deletions src/isomorphic/classic/element/ReactElement.js
Expand Up @@ -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;
Copy link
Collaborator

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)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gaearon o is not valid hex char :P

Copy link
Contributor

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

0xeac7 sorta kinda looks like React

Copy link
Contributor

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

Copy link
Collaborator Author

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

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.

Copy link
Contributor

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

Copy link
Collaborator Author

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:

loadString(function(stringData) {
  React.render(<div>Content: {stringData}</div>, container);
});


var RESERVED_PROPS = {
key: true,
ref: true,
Expand Down Expand Up @@ -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
Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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, __DEV__ and making them non-enumerable. Otherwise jest's equality helpers won't work on elements created in two different source locations.

$$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__) {
Expand All @@ -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;
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);
Expand Down Expand Up @@ -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
);
Expand All @@ -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
);
Expand All @@ -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;
Expand Down Expand Up @@ -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
);
};

Expand Down
51 changes: 51 additions & 0 deletions src/isomorphic/classic/element/__tests__/ReactElement-test.js
Expand Up @@ -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;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this is configurable?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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 window and document and a few more that can't be deleted.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

The 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?

Copy link
Contributor

Choose a reason for hiding this comment

The 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 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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what @sebmck said ☝️

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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');
Expand Down Expand Up @@ -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);
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

The 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 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?

Copy link
Collaborator

Choose a reason for hiding this comment

The 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() {
Expand Down Expand Up @@ -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);
});

});
Expand Up @@ -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() {
Expand Down