Unicode 202E vuln fix #382
Conversation
| @@ -170,6 +170,9 @@ export class UrlMatch extends Match { | |||
| getAnchorHref() { | |||
| let url = this.getUrl(); | |||
|
|
|||
| //Strip malicious Unicode SNYK-AUTOLINKER-2438289 | |||
| url.replace('\u202E', ''); | |||
There was a problem hiding this comment.
Also, I suggest Strip the most RTLO common format U+202E and %E2%80%AE beside \u202E
There was a problem hiding this comment.
it should also be replaced globally with /\u202E/g
There was a problem hiding this comment.
would that be immune to ReDoS?
There was a problem hiding this comment.
Hey, not sure how this would actually fix the issue because the code isn't re-assigning the replacement result back to url. The code would need to be:
url = url.replace(/\u202E/g, '');Does your new test pass even without this new line of code?
There was a problem hiding this comment.
even after correction of the test (see remark in autolinker-url.spec.ts) the test is failing whatever the changes here:
- with this line
- with the update line according greg comment
- without this line
in my observation the \u202e char is never reaching this method because of parseHtml
Update so we can just merge everything
| describe('unicode exploits', () => { | ||
| it('should strip out Right-To-Left Override Unicode characters for security', () => { | ||
| var result = autolinker.link('https://legit.ok/files\u202E4pm.asia'); | ||
| expect(result).toBe('<a href="https://legit.ok/files4pm.asia"></a>'); |
There was a problem hiding this comment.
the expected result should be '<a href="https://legit.ok/files4pm.asia">legit.ok/files4pm.asia</a>'
I even added a test, wow TDD works!