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

Replace REACT_ELEMENT_TYPE magicnum with Infinity. #5830

Closed
wants to merge 1 commit into from

Conversation

STRML
Copy link
Contributor

@STRML STRML commented Jan 12, 2016

This closes the XSS hole on older browsers that don't support Symbol.

More discussion: #4832 (comment)

Figured this would be helpful to get in before 0.15 hits.

This closes the XSS hole on older browsers that don't support Symbol.

More discussion: facebook#4832 (comment)
@STRML
Copy link
Contributor Author

STRML commented Jan 12, 2016

Note that this doesn't protect against data coming in through structured cloning, but JSON is by far the most common vector.

The use of Symbol() protects against structured cloning / cross-realm communication, but causes very real issues like https://phabricator.babeljs.io/T2517 which can kill applications only in production, on the browsers devs often don't use. Getting the Symbol out completely would be great, but short of polluting globals I'm not quite sure how we would. Perhaps we will have to blacklist shimming Symbol inside React elements in the transform-runtime plugin.

Merging this PR will require coordination with Babel in order to not break inline-elements in 0.15.

@zpao
Copy link
Member

zpao commented Jan 12, 2016

cc @sebmarkbage

@sebmarkbage
Copy link
Collaborator

The downside here is that this is not a magic number. Meaning it is not unique among other frameworks nor within this framework if we add more.

@syranide
Copy link
Contributor

@sebmarkbage A tad overkill and might impact performance, but having both ([Infinity, 0xeac7]) would work right? (the object should be reused)

@STRML
Copy link
Contributor Author

STRML commented Jan 12, 2016

Why would we need to have a magicnum that is unique among other frameworks?

@quantizor
Copy link
Contributor

Why not just make it something like null? Is there a specific reason it has to be a real number?

@STRML
Copy link
Contributor Author

STRML commented Feb 1, 2016

@yaycmyk The idea is something that can't be JSON serialized, to avoid inadvertent XSS if, say, a server responds with something that looks like a React Element.

Infinity can still be cloned via Structured Cloning, but closes the most common hole (XHR/Fetch).

@quantizor
Copy link
Contributor

Ah! Does it have to be a primitive? A cached noop function could work otherwise and would definitely not serialize as far as I'm aware

@STRML
Copy link
Contributor Author

STRML commented Feb 1, 2016

Technically, yes, it would work, but it would be impossible to reference via e.g. Babel Helpers, which are meant to be pure and not rely on any external dependencies (such as importing React and grabbing that noop function). This limitation causes problems with Symbols, actually, since a native symbol does not equal a polyfilled one. Plenty of reading at #5138

@quantizor
Copy link
Contributor

Oh I see. Definitely an interesting problem to solve - thanks for the reading material! One other thought - what about window? It's an object yes, but it's also not stringifiable because it has circular references.

@STRML
Copy link
Contributor Author

STRML commented Feb 1, 2016

Sure, but how would that work on the server? :)

@quantizor
Copy link
Contributor

Well on the server there is the global object, which is replaced with window when using a tool like Browserify.

@sebmarkbage
Copy link
Collaborator

The guarantee here is not strong enough so I'm going to close out this PR. Let's continue in #5138 which has much more context.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants