Skip to content
This repository has been archived by the owner on Oct 6, 2020. It is now read-only.

Don't use dangerous HTML in Linkify #10

Merged
merged 7 commits into from
Dec 7, 2018
Merged

Don't use dangerous HTML in Linkify #10

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
7354bf3
fix(Linkify): escape HTML characters
Dec 5, 2018
e6c82f4
Don't set HTML dangerously
Dec 7, 2018
6ad9d3b
Fix test and copy-pasta
Dec 7, 2018
297520f
Improve link parsing; Update docs to showcase more complex functionality
kylealwyn Dec 7, 2018
0252a69
Fix doc formatting
kylealwyn Dec 7, 2018
98742c1
Remove unnecessary filter
kylealwyn Dec 7, 2018
ab1637c
Add linkStyle test
kylealwyn Dec 7, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 14 additions & 2 deletions src/Linkify/Linkify.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,21 @@ const Container = styled.span`
}
`;

const REGEX = /(\b(https?|ftp|file):\/\/[-A-Z0-9+&@#\/%?=~_|!:,.;]*[-A-Z0-9+&@#\/%=~_|])/gi;
const escapeHtmlChars = (unsafe = '') =>
unsafe
.replace(/&/g, '&')
.replace(/</g, '&lt;')
.replace(/>/g, '&gt;')
.replace(/"/g, '&quot;')
.replace(/'/g, '&#039;');

const parse = string => string.replace(REGEX, url => `<a target="_blank" href="${url}">${url}</a>`);
const REGEX = /(\b(https?|ftp|file):\/\/[-A-Z0-9+&@#/%?=~_|!:,.;]*[-A-Z0-9+&@#/%=~_|])/gi;

const parse = string =>
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) }} />
Copy link

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 😄

Copy link

Choose a reason for hiding this comment

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

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

Expand Down
17 changes: 17 additions & 0 deletions src/Linkify/Linkify.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
import React from 'react';
import { renderWithTheme } from '../../test/utils';
import Linkify from './Linkify';

describe('Linkify', () => {
test('converts links to anchor tags', () => {
const component = renderWithTheme(<Linkify>Hello! https://google.com is a cool site.</Linkify>);

expect(component).toMatchSnapshot();
});

test('escapes HTML entities', () => {
const component = renderWithTheme(<Linkify>{`<span onmouseover=alert('XSS')></span>`}</Linkify>);

expect(component).toMatchSnapshot();
});
});
37 changes: 37 additions & 0 deletions src/Linkify/__snapshots__/Linkify.spec.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`Linkify converts links to anchor tags 1`] = `
.c0 a {
color: inherit;
-webkit-text-decoration: underline;
text-decoration: underline;
}

<span
className="c0"
dangerouslySetInnerHTML={
Object {
"__html": "Hello! <a target=\\"_blank\\" rel=\\"noopener noreferrer\\" href=\\"https://google.com\\">https://google.com</a> is a cool site.",
}
}
style={Object {}}
/>
`;

exports[`Linkify escapes HTML entities 1`] = `
.c0 a {
color: inherit;
-webkit-text-decoration: underline;
text-decoration: underline;
}

<span
className="c0"
dangerouslySetInnerHTML={
Object {
"__html": "&lt;span onmouseover=alert(&#039;XSS&#039;)&gt;&lt;/span&gt;",
}
}
style={Object {}}
/>
`;