Skip to content

Commit

Permalink
fix(sanitizeUri): sanitize URIs that contain IDEOGRAPHIC SPACE chars
Browse files Browse the repository at this point in the history
Chrome 62 was not sanitizing dangerous URLs containing
JavaScript, if they started with these "whitespace" characters.

---

Browsers convert `&angular#12288;javascript:alert(1)` as an attribute value to
`javascript:alert(1)`. So the sanitizer gets the second string and is
able to strip the javascript from the attribute.

But Chrome (<62) only did this after you read it and wrote it back again.
So we added a bit of code that tried to get Chrome to do its conversion
before sanitizing it:
https://github.com/angular/angular.js/blob/817ac567/src/ngSanitize/sanitize.js#L406-L417

I believe that Chrome 62 now does not do this conversion any more, instead
it just leaves the attribute value alone, whatever you do to it.

This fix uses `trim()` to remove the problematic whitespace before
sanitizing, which appears to solve the problem.

Closes angular#16288
  • Loading branch information
petebacondarwin committed Nov 1, 2017
1 parent 817ac56 commit 769306d
Show file tree
Hide file tree
Showing 2 changed files with 4 additions and 6 deletions.
2 changes: 1 addition & 1 deletion src/ng/sanitizeUri.js
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ function $$SanitizeUriProvider() {
return function sanitizeUri(uri, isImage) {
var regex = isImage ? imgSrcSanitizationWhitelist : aHrefSanitizationWhitelist;
var normalizedVal;
normalizedVal = urlResolve(uri).href;
normalizedVal = urlResolve(uri && uri.trim()).href;
if (normalizedVal !== '' && !normalizedVal.match(regex)) {
return 'unsafe:' + normalizedVal;
}
Expand Down
8 changes: 3 additions & 5 deletions test/ngSanitize/sanitizeSpec.js
Original file line number Diff line number Diff line change
Expand Up @@ -237,11 +237,9 @@ describe('HTML', function() {
.toEqual('');
});

if (isChrome) {
it('should prevent mXSS attacks', function() {
expectHTML('<a href="&#x3000;javascript:alert(1)">CLICKME</a>').toBe('<a>CLICKME</a>');
});
}
it('should prevent mXSS attacks', function() {
expectHTML('<a href="&#x3000;javascript:alert(1)">CLICKME</a>').toBe('<a>CLICKME</a>');
});

it('should strip html comments', function() {
expectHTML('<!-- comment 1 --><p>text1<!-- comment 2 -->text2</p><!-- comment 3 -->')
Expand Down

0 comments on commit 769306d

Please sign in to comment.