Skip to content

Commit 105d6a6

Browse files
ariabucklesariabuckles
committed
XSS: Fix sanitizeUrl vbscript/data xss
I believe this fixes https://www.npmjs.com/advisories/1219 if `options.disableParsingRawHTML` is set. NOTE: This does not handle script elements, etc., that may be rendered when `options.disableParsingRawHTML` is not enabled. We might be able to use something like [`dompurify`](https://github.com/cure53/DOMPurify) to solve that case? According to https://owasp.org/www-community/xss-filter-evasion-cheatsheet , the dangerous `javascript:` protocol can contain some whitespace characters and still be vulnerable, and sometimes when used in conjunction with images, some other special characters like ` or < before the javascript: protocol can also leave a url vulnerable. This change re-adds the sanitiation logic removed in 9c6c782 , and also adds the vbscript/data handling from github.com/ariabuckles/simple-markdown/pull/63 Test plan: Add tests and run `npm test`
1 parent 8dfbc86 commit 105d6a6

File tree

2 files changed

+187
-36
lines changed

2 files changed

+187
-36
lines changed

‎index.compiler.spec.js

+183-33
Original file line numberDiff line numberDiff line change
@@ -925,6 +925,48 @@ describe('links', () => {
925925
expect(console.warn).toHaveBeenCalled();
926926
});
927927

928+
it('should sanitize markdown links containing JS expressions', () => {
929+
jest.spyOn(console, 'warn').mockImplementation(() => {});
930+
931+
render(compiler('![foo](javascript:doSomethingBad)'));
932+
933+
expect(root.innerHTML).toMatchInlineSnapshot(`
934+
935+
<img data-reactroot
936+
alt="foo"
937+
>
938+
939+
`);
940+
941+
expect(console.warn).toHaveBeenCalled();
942+
});
943+
944+
it('should sanitize markdown links containing Data expressions', () => {
945+
jest.spyOn(console, 'warn').mockImplementation(() => {});
946+
render(compiler('[foo](data:doSomethingBad)'));
947+
expect(root.innerHTML).toMatchInlineSnapshot(`
948+
949+
<a data-reactroot>
950+
foo
951+
</a>
952+
953+
`);
954+
expect(console.warn).toHaveBeenCalled();
955+
});
956+
957+
it('should sanitize markdown links containing VBScript expressions', () => {
958+
jest.spyOn(console, 'warn').mockImplementation(() => {});
959+
render(compiler('[foo](vbScript:doSomethingBad)'));
960+
expect(root.innerHTML).toMatchInlineSnapshot(`
961+
962+
<a data-reactroot>
963+
foo
964+
</a>
965+
966+
`);
967+
expect(console.warn).toHaveBeenCalled();
968+
});
969+
928970
it('should sanitize markdown links containing encoded JS expressions', () => {
929971
jest.spyOn(console, 'warn').mockImplementation(() => {});
930972

@@ -957,6 +999,60 @@ describe('links', () => {
957999
expect(console.warn).toHaveBeenCalled();
9581000
});
9591001

1002+
it('should sanitize markdown links containing padded encoded vscript expressions', () => {
1003+
jest.spyOn(console, 'warn').mockImplementation(() => {});
1004+
1005+
render(compiler('[foo]( VBScript%3AdoSomethingBad)'));
1006+
1007+
expect(root.innerHTML).toMatchInlineSnapshot(`
1008+
1009+
<a data-reactroot>
1010+
foo
1011+
</a>
1012+
1013+
`);
1014+
expect(console.warn).toHaveBeenCalled();
1015+
});
1016+
1017+
it('should sanitize markdown images containing padded encoded vscript expressions', () => {
1018+
jest.spyOn(console, 'warn').mockImplementation(() => {});
1019+
render(compiler('![foo]( VBScript%3AdoSomethingBad)'));
1020+
expect(root.innerHTML).toMatchInlineSnapshot(`
1021+
1022+
<img data-reactroot
1023+
alt="foo"
1024+
>
1025+
1026+
`);
1027+
expect(console.warn).toHaveBeenCalled();
1028+
});
1029+
1030+
it('should sanitize markdown links containing padded encoded data expressions', () => {
1031+
jest.spyOn(console, 'warn').mockImplementation(() => {});
1032+
render(compiler('[foo](`<data:doSomethingBad)'));
1033+
expect(root.innerHTML).toMatchInlineSnapshot(`
1034+
1035+
<a data-reactroot>
1036+
foo
1037+
</a>
1038+
1039+
`);
1040+
expect(console.warn).toHaveBeenCalled();
1041+
});
1042+
1043+
it('should sanitize markdown images containing padded encoded data expressions', () => {
1044+
jest.spyOn(console, 'warn').mockImplementation(() => {});
1045+
render(compiler('![foo](`<data:doSomethingBad)'));
1046+
expect(root.innerHTML).toMatchInlineSnapshot(`
1047+
1048+
<img data-reactroot
1049+
alt="foo"
1050+
>
1051+
1052+
`);
1053+
expect(console.warn).toHaveBeenCalled();
1054+
});
1055+
9601056
it('should sanitize markdown links containing invalid characters', () => {
9611057
jest.spyOn(console, 'warn').mockImplementation(() => {});
9621058

@@ -973,20 +1069,65 @@ describe('links', () => {
9731069
});
9741070

9751071
it('should sanitize html links containing JS expressions', () => {
976-
jest.spyOn(console, 'warn').mockImplementation(() => {});
1072+
jest.spyOn(console, 'warn').mockImplementation(() => {});
1073+
1074+
render(compiler('<a href="javascript:doSomethingBad">foo</a>'));
1075+
1076+
expect(root.innerHTML).toMatchInlineSnapshot(`
1077+
1078+
<a data-reactroot>
1079+
foo
1080+
</a>
1081+
1082+
`);
9771083

978-
render(compiler('<a href="javascript:doSomethingBad">foo</a>'));
1084+
expect(console.warn).toHaveBeenCalled();
1085+
});
9791086

980-
expect(root.innerHTML).toMatchInlineSnapshot(`
1087+
it('should sanitize html links containing encoded, prefixed data expressions', () => {
1088+
jest.spyOn(console, 'warn').mockImplementation(() => {});
1089+
render(compiler('<a href="<`data:doSomethingBad">foo</a>'));
1090+
expect(root.innerHTML).toMatchInlineSnapshot(`
9811091
9821092
<a data-reactroot>
9831093
foo
9841094
</a>
9851095
9861096
`);
1097+
expect(console.warn).toHaveBeenCalled();
1098+
});
1099+
1100+
it('should sanitize html images containing encoded, prefixed JS expressions', () => {
1101+
jest.spyOn(console, 'warn').mockImplementation(() => {});
1102+
1103+
// TODO: something is off on parsing here, because this prints:
1104+
// console.error("Warning: Unknown prop `javascript:alert` on <img> tag"...)
1105+
// Which it shouldn't
1106+
render(compiler('<img src="`<javascript:alert>`(\'alertstr\')"'));
1107+
expect(root.innerHTML).toMatchInlineSnapshot(`
9871108
988-
expect(console.warn).toHaveBeenCalled();
989-
});
1109+
<span data-reactroot>
1110+
<img src="true">
1111+
\`('alertstr')"
1112+
</span>
1113+
1114+
`);
1115+
expect(console.warn).toHaveBeenCalled();
1116+
});
1117+
1118+
it('should sanitize html images containing weird parsing src=s', () => {
1119+
jest.spyOn(console, 'warn').mockImplementation(() => {});
1120+
render(compiler('<img src="`<src="javascript:alert(`xss`)">`'));
1121+
expect(root.innerHTML).toMatchInlineSnapshot(`
1122+
1123+
<span data-reactroot>
1124+
<img src="\`<src=">
1125+
\`
1126+
</span>
1127+
1128+
`);
1129+
expect(console.warn).toHaveBeenCalled();
1130+
});
9901131

9911132
it('should handle a link with a URL in the text', () => {
9921133
render(
@@ -1582,14 +1723,14 @@ describe('GFM tables', () => {
15821723

15831724
it('#241 should not ignore the first cell when its contents is empty', () => {
15841725
render(
1585-
compiler(
1586-
[
1587-
'| Foo | Bar | Baz |',
1588-
'| --- | --- | --- |',
1589-
'| | 2 | 3 |',
1590-
'| | 5 | 6 |',
1591-
].join('\n')
1592-
)
1726+
compiler(
1727+
[
1728+
'| Foo | Bar | Baz |',
1729+
'| --- | --- | --- |',
1730+
'| | 2 | 3 |',
1731+
'| | 5 | 6 |',
1732+
].join('\n')
1733+
)
15931734
);
15941735

15951736
expect(root.innerHTML).toMatchInlineSnapshot(`
@@ -1780,7 +1921,6 @@ describe('GFM tables', () => {
17801921
17811922
`);
17821923
});
1783-
17841924
});
17851925

17861926
describe('arbitrary HTML', () => {
@@ -1976,10 +2116,12 @@ describe('arbitrary HTML', () => {
19762116
});
19772117

19782118
it('throws out multiline HTML comments', () => {
1979-
render(compiler(`Foo\n<!-- this is
2119+
render(
2120+
compiler(`Foo\n<!-- this is
19802121
a
19812122
multiline
1982-
comment -->`));
2123+
comment -->`)
2124+
);
19832125

19842126
expect(root.innerHTML).toMatchInlineSnapshot(`
19852127
@@ -2800,7 +2942,7 @@ fun main() {
28002942
28012943
`);
28022944
});
2803-
it('should not fail with lots of \\n in the middle of the text', () => {
2945+
it('should not fail with lots of \\n in the middle of the text', () => {
28042946
render(
28052947
compiler(
28062948
'Text\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\ntext',
@@ -2825,12 +2967,9 @@ fun main() {
28252967

28262968
it('should not render html if disableParsingRawHTML is true', () => {
28272969
render(
2828-
compiler(
2829-
'Text with <span>html</span> inside',
2830-
{
2831-
disableParsingRawHTML: true
2832-
}
2833-
)
2970+
compiler('Text with <span>html</span> inside', {
2971+
disableParsingRawHTML: true,
2972+
})
28342973
);
28352974
expect(root.innerHTML).toMatchInlineSnapshot(`
28362975
@@ -2843,12 +2982,9 @@ fun main() {
28432982

28442983
it('should render html if disableParsingRawHTML is false', () => {
28452984
render(
2846-
compiler(
2847-
'Text with <span>html</span> inside',
2848-
{
2849-
disableParsingRawHTML: false
2850-
}
2851-
)
2985+
compiler('Text with <span>html</span> inside', {
2986+
disableParsingRawHTML: false,
2987+
})
28522988
);
28532989
expect(root.innerHTML).toMatchInlineSnapshot(`
28542990
@@ -2976,7 +3112,15 @@ describe('footnotes', () => {
29763112
});
29773113

29783114
it('should handle complex references', () => {
2979-
render(compiler(['foo[^referencé heré 123] bar', '', '[^referencé heré 123]: Baz baz'].join('\n')));
3115+
render(
3116+
compiler(
3117+
[
3118+
'foo[^referencé heré 123] bar',
3119+
'',
3120+
'[^referencé heré 123]: Baz baz',
3121+
].join('\n')
3122+
)
3123+
);
29803124

29813125
expect(root.innerHTML).toMatchInlineSnapshot(`
29823126
@@ -3002,7 +3146,13 @@ describe('footnotes', () => {
30023146
});
30033147

30043148
it('should handle conversion of multiple references into links', () => {
3005-
render(compiler(['foo[^abc] bar. baz[^def]', '', '[^abc]: Baz baz', '[^def]: Def'].join('\n')));
3149+
render(
3150+
compiler(
3151+
['foo[^abc] bar. baz[^def]', '', '[^abc]: Baz baz', '[^def]: Def'].join(
3152+
'\n'
3153+
)
3154+
)
3155+
);
30063156

30073157
expect(root.innerHTML).toMatchInlineSnapshot(`
30083158
@@ -3410,7 +3560,7 @@ describe('overrides', () => {
34103560
render(
34113561
compiler('Hello.\n\n', {
34123562
overrides: { p: { component: FakeParagraph } },
3413-
options: { disableParsingRawHTML: true }
3563+
options: { disableParsingRawHTML: true },
34143564
})
34153565
);
34163566

@@ -3424,7 +3574,7 @@ describe('overrides', () => {
34243574
render(
34253575
compiler('Hello.\n\n<FakeSpan>I am a fake span</FakeSpan>', {
34263576
overrides: { FakeSpan },
3427-
options: { disableParsingRawHTML: true }
3577+
options: { disableParsingRawHTML: true },
34283578
})
34293579
);
34303580

‎index.js

+4-3
Original file line numberDiff line numberDiff line change
@@ -586,12 +586,13 @@ function reactFor(outputFunc) {
586586

587587
function sanitizeUrl(url) {
588588
try {
589-
const decoded = decodeURIComponent(url);
589+
const decoded = decodeURIComponent(url)
590+
.replace(/[^A-Za-z0-9/:]/g, '');
590591

591-
if (decoded.match(/^\s*javascript:/i)) {
592+
if (decoded.match(/^\s*(javascript|vbscript|data):/i)) {
592593
if (process.env.NODE_ENV !== 'production') {
593594
console.warn(
594-
'Anchor URL contains an unsafe JavaScript expression, it will not be rendered.',
595+
'Anchor URL contains an unsafe JavaScript/VBScript/data expression, it will not be rendered.',
595596
decoded
596597
);
597598
}

0 commit comments

Comments
 (0)