Use a Symbol to tag every ReactElement #4832

Merged
merged 1 commit into from Sep 10, 2015

Projects

None yet
@sebmarkbage
Member

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. (thx @sebmck)

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.

Test Plan:

examples/basic/index.html works in Chrome and Firefox which both have native Symbols - and in Safari which doesn't.

React.createElement('div').$$typeof // Symbol(react.element).

@sebmarkbage sebmarkbage added this to the 0.14 milestone Sep 10, 2015
@sebmarkbage sebmarkbage commented on the diff Sep 10, 2015
src/isomorphic/classic/element/ReactElement.js
} else {
- this._store.validated = false;
@sebmarkbage
sebmarkbage Sep 10, 2015 Member

Fixed this bug in older browsers.

@sebmarkbage sebmarkbage commented on the diff Sep 10, 2015
src/isomorphic/classic/element/ReactElement.js
// Built-in properties that belong on the element
type: type,
key: key,
ref: ref,
- self: self,
- source: source,
@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.

@sebmarkbage sebmarkbage commented on the diff Sep 10, 2015
src/isomorphic/classic/element/ReactElement.js
} else {
- this._store.validated = false;
+ element._store.validated = false;
+ element._self = self;
+ element._source = source;
@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.

@spicyj spicyj and 1 other commented on an outdated diff Sep 10, 2015
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;
@spicyj
spicyj Sep 10, 2015 Member

nit: 0xeac7 looks more like React ;)

@spicyj
spicyj Sep 10, 2015 Member

Why react.element not ReactElement as we normally style it?

Edit: I guess before the dot is our npm package name which helps avoid collisions?

@sebmarkbage
sebmarkbage Sep 10, 2015 Member

Just because MDN recommended it: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Symbol/for

We could also do react/element to make it npm-like.

@spicyj spicyj commented on the diff Sep 10, 2015
...orphic/classic/element/__tests__/ReactElement-test.js
@@ -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;
@spicyj
spicyj Sep 10, 2015 Member

I guess this is configurable?

@sebmarkbage
sebmarkbage Sep 10, 2015 Member

What do you mean?

@spicyj
spicyj 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.

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

@cpojer
cpojer Sep 10, 2015 Member

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.

@spicyj
spicyj 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.

@cpojer
cpojer Sep 10, 2015 Member

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?

@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

@chicoxyzzy
chicoxyzzy Sep 10, 2015 Contributor

what @sebmck said ☝️

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

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

@sebmarkbage sebmarkbage Use a Symbol to tag every ReactElement
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.
031fc24
@sebmarkbage sebmarkbage merged commit 7a00239 into facebook:master Sep 10, 2015

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@glenjamin
Contributor

This is a great solution to the XSS issue! Just need to make sure no-one adds a way to run symbol.for via JSON parsing :)

@spicyj spicyj added a commit to spicyj/babel that referenced this pull request Sep 10, 2015
@spicyj spicyj Include $$typeof on inlined React elements e0871c9
@spicyj spicyj referenced this pull request in babel/babel Sep 10, 2015
Merged

Include $$typeof on inlined React elements #2352

@spicyj spicyj added a commit to spicyj/babel that referenced this pull request Sep 10, 2015
@spicyj spicyj Include $$typeof on inlined React elements 1d0e68f
@trueadm
Contributor
trueadm commented Sep 10, 2015

works in Chrome and Firefox which both have native Symbols - and in Safari which doesn't.

I do believe this is very soon to change with the latest Safari for iOS 9 and El Capitan (16th September and 30th September according to yesterday's Apple event). :)

@gaearon gaearon commented on the diff Sep 13, 2015
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;
@gaearon
gaearon Sep 13, 2015 Member

0xeac7 0x0cc5!

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

@RReverser
RReverser Sep 14, 2015 Member

@gaearon o is not valid hex char :P

@chicoxyzzy
chicoxyzzy Sep 14, 2015 Contributor

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

@zpao
zpao Sep 14, 2015 Member

0xeac7 sorta kinda looks like React

@yiminghe
yiminghe Sep 16, 2015 Contributor

why not use Math.random()

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

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

@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.]

@sebmarkbage
sebmarkbage Oct 14, 2015 Member

It doesn't cover postMessage though.

@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

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

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

@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);
});
@zpao zpao commented on the diff Sep 13, 2015
...orphic/classic/element/__tests__/ReactElement-test.js
@@ -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);
@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.

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

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

@spicyj
spicyj Sep 13, 2015 Member

That's right. See the next test case "identifies elements, but not JSON, if Symbols are supported" which tests this.

@minimal minimal added a commit to minimal/devcards that referenced this pull request Oct 16, 2015
@minimal minimal Support react v0.14
Adds minimal support for react 0.14 without affecting 0.13 users.

React 0.14 removes React.initializeTouchEvents as it's no longer
needed.

_isReactElement was removed and replaced with the $$typeof field which
contains an ES6 symbol if supported or a number.

Warnings are visible for 0.14 which can be stopped by using ReactDOM for
render and findDOMNode etc.

https://facebook.github.io/react/blog/2015/10/07/react-v0.14.html#breaking-changes
facebook/react#4832
b90b322
@minimal minimal referenced this pull request in bhauman/devcards Oct 16, 2015
Merged

Support react v0.14 #58

@minimal minimal added a commit to minimal/devcards that referenced this pull request Oct 17, 2015
@minimal minimal Support react v0.14
Adds minimal support for react 0.14 without affecting 0.13 users.

React 0.14 removes React.initializeTouchEvents as it's no longer
needed.

_isReactElement was removed and replaced with the $$typeof field which
contains an ES6 symbol if supported or a number.

Warnings are visible for 0.14 which can be stopped by using ReactDOM for
render and findDOMNode etc.

https://facebook.github.io/react/blog/2015/10/07/react-v0.14.html#breaking-changes
facebook/react#4832
89f5e6d
@yesvods yesvods referenced this pull request in yesvods/Blog Dec 20, 2015
Open

React组件/元素与实例分析 #5

@STRML STRML added a commit to STRML/react that referenced this pull request Jan 12, 2016
@STRML STRML Replace REACT_ELEMENT_TYPE magicnum with Infinity.
This closes the XSS hole on olders browsers that don't support Symbol.

More discussion: facebook#4832 (comment)
8d2de5c
@STRML STRML added a commit to STRML/react that referenced this pull request Jan 12, 2016
@STRML STRML Replace REACT_ELEMENT_TYPE magicnum with Infinity.
This closes the XSS hole on older browsers that don't support Symbol.

More discussion: facebook#4832 (comment)
e7e48fd
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment