Added checks for incorrect usage of innerHTML. Fixes #1370 #2520
Conversation
| ); | ||
| invariant( | ||
| props.innerHTML == null, | ||
| 'Directly setting property `innerHTML` is not permitted. ' + |
There was a problem hiding this comment.
Does anyone try to do this? I've never heard of anyone trying to…
There was a problem hiding this comment.
It's the intuitive thing to try, if you haven't read about dangerouslySetInnerHTML, so it's worth checking if for no other reason than to hint to the user where they might look for the supported function.
jimfb
force-pushed
the
warn-for-dangerouslySetInnerHtml
branch
from
18cb2ba to
ef9fb89
Compare
|
This |
jimfb
force-pushed
the
warn-for-dangerouslySetInnerHtml
branch
from
ef9fb89 to
a92667a
Compare
|
Added if-check outside call to invariant call, as per @sebmarkbage |
| throw 'Invariant Violation: ' + | ||
| 'Directly setting property `innerHTML` is not permitted. ' + | ||
| 'For more information, lookup documentation on `dangerouslySetInnerHTML`.'; | ||
| } |
There was a problem hiding this comment.
We never throw strings because the stack is attached to the Error object. We also compress invariant error messages. So we should use the invariant module even if the first argument is false, although in this case it should be props == null.
sebmarkbage
added
GH Review: needs-revision
and removed
GH Review: review-needed
labels
jimfb
force-pushed
the
warn-for-dangerouslySetInnerHtml
branch
from
a92667a to
fb1cf27
Compare
jimfb
force-pushed
the
warn-for-dangerouslySetInnerHtml
branch
from
fb1cf27 to
4d02264
Compare
| ); | ||
| invariant( | ||
| props.dangerouslySetInnerHTML.__html != null, | ||
| '`props.dangerouslySetInnerHTML` must be in the form {__html: ...} ' + |
There was a problem hiding this comment.
Double space at the end here. One is enough. :)
There should also be a period. You can use the backticks it indicate what is code and what is period.
jimfb
force-pushed
the
warn-for-dangerouslySetInnerHtml
branch
from
4d02264 to
da3a900
Compare
|
Ready to merge? |
| 'Can only set one of `children` or `props.dangerouslySetInnerHTML`.' | ||
| props.innerHTML == null, | ||
| 'Directly setting property `innerHTML` is not permitted. ' + | ||
| 'For more information, lookup documentation on `dangerouslySetInnerHTML`.' |
There was a problem hiding this comment.
This should be a warning, not an invariant. There's no reason to stop execution flow.
|
Let's a get a better description in the summary here explaining the rational. It actually wasn't clear to me that we had settled on what we were doing with innerHTML. |
jimfb
force-pushed
the
warn-for-dangerouslySetInnerHtml
branch
from
da3a900 to
557e95f
Compare
| warning( | ||
| props.innerHTML == null, | ||
| 'Directly setting property `innerHTML` is not permitted. ' + | ||
| 'For more information, lookup documentation on `dangerouslySetInnerHTML`.' |
There was a problem hiding this comment.
Warnings should move into the __DEV__ block so that they never execute in prod.
We don't do that automatically, partially because we don't have good dead-code elimination but mostly because we don't know if there is extra work that can be removed in production rather than just this statement.
jimfb
force-pushed
the
warn-for-dangerouslySetInnerHtml
branch
from
557e95f to
aef7c4d
Compare
|
Better description added, any other feedback or ready to merge? |
Added checks for incorrect usage of innerHTML. Fixes #1370
This PR adds checks to prevent the incorrect setting of inner html, to make it easier for developers to spot hazards in their code and avoid security holes. There have been several discussions about how best to handle the innerHTML issue ( most notably, the public discussion here: #1515 ), and the conclusion of these discussions was the creation of issue #1370, which is fixed by this commit.