Don't use dangerous HTML in Linkify #10
Conversation
> ### Summary Fixed an issue with the `Linkify` component allowing execution of arbitrary HTML/JS that could be used for XSS attacks.
Codecov Report
@@ Coverage Diff @@
## master #10 +/- ##
==========================================
+ Coverage 69.29% 72.78% +3.49%
==========================================
Files 10 11 +1
Lines 127 147 +20
Branches 20 24 +4
==========================================
+ Hits 88 107 +19
- Misses 28 29 +1
Partials 11 11
Continue to review full report at Codecov.
|
src/Linkify/Linkify.js
Outdated
| escapeHtmlChars(string).replace( | ||
| REGEX, | ||
| url => `<a target="_blank" rel="noopener noreferrer" href="${url}">${url}</a>` | ||
| ); | ||
|
|
||
| const Linkify = props => ( | ||
| <Container style={props.style || {}} dangerouslySetInnerHTML={{ __html: parse(props.children) }} /> |
There was a problem hiding this comment.
i think you can do this without dangerouslySetInnerHTML if you iterate through regex matches and return a [React.Component]. see https://github.com/tasti/react-linkify/blob/master/src/components/Linkify.jsx#L75 (could also use that library directly, up to you)
There was a problem hiding this comment.
While I think using a library like this would be ideal, our use case for this component usually resolves parsing strings as opposed to React elements. So, for example, we currently use the component like this:
<Linkify>
{message.body} //=> string
</Linkify>as opposed to like this.
<Linkify>
<span>Hello this a tag</span>
<span>https://link.com</span>
</Linkify>While I definitely do agree about with the general notion of making code as general and extensible as possible, I think in this case I would hold off on addressing edge cases until and if we get to them.
What do you think? @choochootrain 😄
There was a problem hiding this comment.
discussed offline-but-actually-online-because-slack - i think if we defer html escaping to react then we can be p confident that future bugs will have a lot more eyeballs on them :)
erikshestopal
changed the title
|
@kylealwyn ready for final pass. |
src/Linkify/Linkify.spec.js
Outdated
| const component = renderWithTheme(<Linkify>{`<span onmouseover=alert('XSS')></span>`}</Linkify>); | ||
| const component = renderWithTheme( | ||
| <Linkify> | ||
| <img src="fake.jpg" onError={() => {}} alt="hacker" /> |
There was a problem hiding this comment.
shouldn't this all be in a template string to test escaping?
src/Linkify/Linkify.js
Outdated
| if (typeof child === 'string') { | ||
| const words = child.split(' '); | ||
| return words.map((word, index) => { | ||
| const isLastWord = words.length === index - 1; |
There was a problem hiding this comment.
copy pasta from my example but this should be words.length - 1 === index
|
@kylealwyn @choochootrain ready for final pass. |
| text-decoration: underline; | ||
| } | ||
| `; | ||
| const URL_REGEX = /(\b(https?|ftp|file):\/\/[-A-Z0-9+&@#/%?=~_|!:,.;]*[-A-Z0-9+&@#/%=~_|])/gi; |
There was a problem hiding this comment.
Do we want to add some rules around domain extensions so that URLs like duckduckgo.com in the playground below will also be converted into links?
There was a problem hiding this comment.
In the future, probably yes. We can add rules to our doctors for now :)
There was a problem hiding this comment.
@kylealwyn cool. LGTM 🚢
Fixed an issue with the
Linkifycomponent allowing execution of arbitrary HTML/JS that could be used for XSS attacks.Props to @kylealwyn for the actual implementation.