From fa112d3c52d9f6d33a772daddce2a84836191516 Mon Sep 17 00:00:00 2001 From: ltrillaud Date: Mon, 8 Sep 2014 11:03:28 +0200 Subject: [PATCH 1/9] fix(compile): use sanitizeUri also for srcset image attribut --- src/ng/compile.js | 6 +++--- test/ng/directive/ngSrcSpec.js | 23 ++++++++++++++++------- test/ng/directive/ngSrcsetSpec.js | 7 +++++++ 3 files changed, 26 insertions(+), 10 deletions(-) diff --git a/src/ng/compile.js b/src/ng/compile.js index 13d0488aa59a..baf55d2dc93b 100644 --- a/src/ng/compile.js +++ b/src/ng/compile.js @@ -845,9 +845,9 @@ function $CompileProvider($provide, $$sanitizeUriProvider) { nodeName = nodeName_(this.$$element); // sanitize a[href] and img[src] values - if ((nodeName === 'a' && key === 'href') || - (nodeName === 'img' && key === 'src')) { - this[key] = value = $$sanitizeUri(value, key === 'src'); + var isImage = nodeName === 'img' && (key === 'src' || key === 'srcset'); + if ((nodeName === 'a' && key === 'href') || isImage) { + this[key] = value = $$sanitizeUri(value, isImage); } if (writeAttr !== false) { diff --git a/test/ng/directive/ngSrcSpec.js b/test/ng/directive/ngSrcSpec.js index 8f21d6e941b3..4b8b14bd46f7 100644 --- a/test/ng/directive/ngSrcSpec.js +++ b/test/ng/directive/ngSrcSpec.js @@ -8,13 +8,22 @@ describe('ngSrc', function() { dealoc(element); }); - it('should not result empty string in img src', inject(function($rootScope, $compile) { - $rootScope.image = {}; - element = $compile('')($rootScope); - $rootScope.$digest(); - expect(element.attr('src')).not.toBe(''); - expect(element.attr('src')).toBe(undefined); - })); + describe('img[ng-src]', function() { + it('should not result empty string in img src', inject(function($rootScope, $compile) { + $rootScope.image = {}; + element = $compile('')($rootScope); + $rootScope.$digest(); + expect(element.attr('src')).not.toBe(''); + expect(element.attr('src')).toBe(undefined); + })); + + it('should sanitize url', inject(function($rootScope, $compile) { + $rootScope.imageUrl = 'javascript:alert(1);'; + element = $compile('')($rootScope); + $rootScope.$digest(); + expect(element.attr('src')).toBe('unsafe:javascript:alert(1);'); + })); + }); describe('iframe[ng-src]', function() { it('should pass through src attributes for the same domain', inject(function($compile, $rootScope) { diff --git a/test/ng/directive/ngSrcsetSpec.js b/test/ng/directive/ngSrcsetSpec.js index a95a108b9ba6..c176d350aeff 100644 --- a/test/ng/directive/ngSrcsetSpec.js +++ b/test/ng/directive/ngSrcsetSpec.js @@ -13,4 +13,11 @@ describe('ngSrcset', function() { $rootScope.$digest(); expect(element.attr('srcset')).toBeUndefined(); })); + + it('should sanitize url', inject(function($rootScope, $compile) { + $rootScope.imageUrl = 'javascript:alert(1);'; + element = $compile('')($rootScope); + $rootScope.$digest(); + expect(element.attr('srcset')).toBe('unsafe:javascript:alert(1);'); + })); }); From dd56b98992ff50069362769b922255bef52cc4ff Mon Sep 17 00:00:00 2001 From: ltrillaud Date: Mon, 8 Sep 2014 11:47:50 +0200 Subject: [PATCH 2/9] fix(test): ignore jshint script url for this tests --- test/ng/directive/ngSrcsetSpec.js | 1 + 1 file changed, 1 insertion(+) diff --git a/test/ng/directive/ngSrcsetSpec.js b/test/ng/directive/ngSrcsetSpec.js index c176d350aeff..bbd0dd02cd0f 100644 --- a/test/ng/directive/ngSrcsetSpec.js +++ b/test/ng/directive/ngSrcsetSpec.js @@ -1,3 +1,4 @@ +/*jshint scripturl:true*/ 'use strict'; describe('ngSrcset', function() { From a8ed13b7d18e49e5973a78d9ca4d5d6c5b7afe9a Mon Sep 17 00:00:00 2001 From: ltrillaud Date: Tue, 9 Sep 2014 10:33:05 +0200 Subject: [PATCH 3/9] fix(compile): sanitize all urls in a srcset --- src/ng/compile.js | 40 +++++++++++++++++-- test/ng/compileSpec.js | 64 +++++++++++++++++++++++++++++++ test/ng/directive/ngSrcsetSpec.js | 13 +++++-- 3 files changed, 110 insertions(+), 7 deletions(-) diff --git a/src/ng/compile.js b/src/ng/compile.js index baf55d2dc93b..354418b7445f 100644 --- a/src/ng/compile.js +++ b/src/ng/compile.js @@ -844,10 +844,42 @@ function $CompileProvider($provide, $$sanitizeUriProvider) { nodeName = nodeName_(this.$$element); - // sanitize a[href] and img[src] values - var isImage = nodeName === 'img' && (key === 'src' || key === 'srcset'); - if ((nodeName === 'a' && key === 'href') || isImage) { - this[key] = value = $$sanitizeUri(value, isImage); + if ((nodeName === 'a' && key === 'href') || + (nodeName === 'img' && key === 'src')) { + // sanitize a[href] and img[src] values + this[key] = value = $$sanitizeUri(value, key === 'src'); + } else if (nodeName === 'img' && key === 'srcset') { + // sanitize img[srcset] values + var result = ""; + + // first check if there are spaces because it's not the same pattern + var trimedSrcset = trim(value); + var pattern = /\s/.test( trimedSrcset) ? /(\d+x\s*,|\d+w\s*,|\s+,|,\s+)/ : /(,)/; + + // split srcset into tupple of uri and descriptor except for the last item + var rawUris = trimedSrcset.split(pattern); + + // for each tupples + var nbrUrisWith2parts = Math.floor(rawUris.length / 2); + for (var i=0; i')($rootScope); + $rootScope.testUrl = 'http://example.com/image.png'; + $rootScope.$digest(); + expect(element.attr('srcset')).toEqual('http://example.com/image.png'); + // But it should accept trusted values anyway. + $rootScope.testUrl = $sce.trustAsUrl('http://example.com/image2.png'); + $rootScope.$digest(); + expect(element.attr('srcset')).toEqual('http://example.com/image2.png'); + })); + + it('should use $$sanitizeUri', function() { + var $$sanitizeUri = jasmine.createSpy('$$sanitizeUri'); + module(function($provide) { + $provide.value('$$sanitizeUri', $$sanitizeUri); + }); + inject(function($compile, $rootScope) { + element = $compile('')($rootScope); + $rootScope.testUrl = "someUrl"; + + $$sanitizeUri.andReturn('someSanitizedUrl'); + $rootScope.$apply(); + expect(element.attr('srcset')).toBe('someSanitizedUrl'); + expect($$sanitizeUri).toHaveBeenCalledWith($rootScope.testUrl, true); + }); + }); + + it('should sanitize all uris in srcset', inject(function($rootScope, $compile) { + /*jshint scripturl:true*/ + element = $compile('')($rootScope); + var testSet = { + 'http://example.com/image.png':'http://example.com/image.png', + ' http://example.com/image.png':'http://example.com/image.png', + 'http://example.com/image.png ':'http://example.com/image.png', + 'http://example.com/image.png 128w':'http://example.com/image.png 128w', + 'http://example.com/image.png 2x':'http://example.com/image.png 2x', + 'http://example.com/image.png 1.5x':'http://example.com/image.png 1.5x', + 'http://example.com/image1.png 1x,http://example.com/image2.png 2x':'http://example.com/image1.png 1x,http://example.com/image2.png 2x', + 'http://example.com/image1.png 1x ,http://example.com/image2.png 2x':'http://example.com/image1.png 1x ,http://example.com/image2.png 2x', + 'http://example.com/image1.png 1x, http://example.com/image2.png 2x':'http://example.com/image1.png 1x,http://example.com/image2.png 2x', + 'http://example.com/image1.png 1x , http://example.com/image2.png 2x':'http://example.com/image1.png 1x ,http://example.com/image2.png 2x', + 'http://example.com/image1.png 48w,http://example.com/image2.png 64w':'http://example.com/image1.png 48w,http://example.com/image2.png 64w', + 'http://example.com/image1.png 1x,http://example.com/image2.png 64w':'http://example.com/image1.png 1x,http://example.com/image2.png 64w', + 'http://example.com/image1.png,http://example.com/image2.png':'http://example.com/image1.png ,http://example.com/image2.png', + 'http://example.com/image1.png ,http://example.com/image2.png':'http://example.com/image1.png ,http://example.com/image2.png', + 'http://example.com/image1.png, http://example.com/image2.png':'http://example.com/image1.png ,http://example.com/image2.png', + 'http://example.com/image1.png , http://example.com/image2.png':'http://example.com/image1.png ,http://example.com/image2.png', + 'http://example.com/image1.png 1x, http://example.com/image2.png 2x, http://example.com/image3.png 3x': + 'http://example.com/image1.png 1x,http://example.com/image2.png 2x,http://example.com/image3.png 3x', + 'javascript:doEvilStuff() 2x': 'unsafe:javascript:doEvilStuff() 2x', + 'http://example.com/image1.png 1x,javascript:doEvilStuff() 2x':'http://example.com/image1.png 1x,unsafe:javascript:doEvilStuff() 2x', + 'http://example.com/image1.jpg?x=a,b 1x,http://example.com/ima,ge2.jpg 2x':'http://example.com/image1.jpg?x=a,b 1x,http://example.com/ima,ge2.jpg 2x' + }; + + forEach( testSet, function( ref, url) { + $rootScope.testUrl = url; + $rootScope.$digest(); + expect(element.attr('srcset')).toEqual(ref); + }); + + })); + }); describe('a[href] sanitization', function() { diff --git a/test/ng/directive/ngSrcsetSpec.js b/test/ng/directive/ngSrcsetSpec.js index bbd0dd02cd0f..2345f8264d02 100644 --- a/test/ng/directive/ngSrcsetSpec.js +++ b/test/ng/directive/ngSrcsetSpec.js @@ -15,10 +15,17 @@ describe('ngSrcset', function() { expect(element.attr('srcset')).toBeUndefined(); })); - it('should sanitize url', inject(function($rootScope, $compile) { - $rootScope.imageUrl = 'javascript:alert(1);'; + it('should sanitize good urls', inject(function($rootScope, $compile) { + $rootScope.imageUrl = 'http://example.com/image1.png 1x, http://example.com/image2.png 2x'; element = $compile('')($rootScope); $rootScope.$digest(); - expect(element.attr('srcset')).toBe('unsafe:javascript:alert(1);'); + expect(element.attr('srcset')).toBe('http://example.com/image1.png 1x,http://example.com/image2.png 2x'); + })); + + it('should sanitize evil url', inject(function($rootScope, $compile) { + $rootScope.imageUrl = 'http://example.com/image1.png 1x, javascript:doEvilStuff() 2x'; + element = $compile('')($rootScope); + $rootScope.$digest(); + expect(element.attr('srcset')).toBe('http://example.com/image1.png 1x,unsafe:javascript:doEvilStuff() 2x'); })); }); From 2b0bfe76543111c55f75e32ab227d7667912d1e9 Mon Sep 17 00:00:00 2001 From: ltrillaud Date: Tue, 9 Sep 2014 11:15:02 +0200 Subject: [PATCH 4/9] fix(test): remove trailling whitespaces --- test/ng/compileSpec.js | 2 +- test/ng/directive/ngSrcsetSpec.js | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/test/ng/compileSpec.js b/test/ng/compileSpec.js index ed4e50d36156..49d59e6dab06 100755 --- a/test/ng/compileSpec.js +++ b/test/ng/compileSpec.js @@ -5658,7 +5658,7 @@ describe('$compile', function() { 'http://example.com/image1.png 1x,javascript:doEvilStuff() 2x':'http://example.com/image1.png 1x,unsafe:javascript:doEvilStuff() 2x', 'http://example.com/image1.jpg?x=a,b 1x,http://example.com/ima,ge2.jpg 2x':'http://example.com/image1.jpg?x=a,b 1x,http://example.com/ima,ge2.jpg 2x' }; - + forEach( testSet, function( ref, url) { $rootScope.testUrl = url; $rootScope.$digest(); diff --git a/test/ng/directive/ngSrcsetSpec.js b/test/ng/directive/ngSrcsetSpec.js index 2345f8264d02..8c66b0d664ff 100644 --- a/test/ng/directive/ngSrcsetSpec.js +++ b/test/ng/directive/ngSrcsetSpec.js @@ -20,7 +20,7 @@ describe('ngSrcset', function() { element = $compile('')($rootScope); $rootScope.$digest(); expect(element.attr('srcset')).toBe('http://example.com/image1.png 1x,http://example.com/image2.png 2x'); - })); + })); it('should sanitize evil url', inject(function($rootScope, $compile) { $rootScope.imageUrl = 'http://example.com/image1.png 1x, javascript:doEvilStuff() 2x'; From d0b99ab794f5c636899cc73489ad57fa0943bac4 Mon Sep 17 00:00:00 2001 From: ltrillaud Date: Wed, 10 Sep 2014 07:58:12 +0200 Subject: [PATCH 5/9] this commit is only to trigger a new restart of Travis jobs --- test/ng/directive/ngSrcsetSpec.js | 1 + 1 file changed, 1 insertion(+) diff --git a/test/ng/directive/ngSrcsetSpec.js b/test/ng/directive/ngSrcsetSpec.js index 8c66b0d664ff..0a07720d310f 100644 --- a/test/ng/directive/ngSrcsetSpec.js +++ b/test/ng/directive/ngSrcsetSpec.js @@ -28,4 +28,5 @@ describe('ngSrcset', function() { $rootScope.$digest(); expect(element.attr('srcset')).toBe('http://example.com/image1.png 1x,unsafe:javascript:doEvilStuff() 2x'); })); + }); From 339109031f32d8c023043869abea724bf4f985d0 Mon Sep 17 00:00:00 2001 From: ltrillaud Date: Wed, 10 Sep 2014 08:21:38 +0200 Subject: [PATCH 6/9] restore the previous fake commit to restart Travis --- test/ng/directive/ngSrcsetSpec.js | 1 - 1 file changed, 1 deletion(-) diff --git a/test/ng/directive/ngSrcsetSpec.js b/test/ng/directive/ngSrcsetSpec.js index 0a07720d310f..8c66b0d664ff 100644 --- a/test/ng/directive/ngSrcsetSpec.js +++ b/test/ng/directive/ngSrcsetSpec.js @@ -28,5 +28,4 @@ describe('ngSrcset', function() { $rootScope.$digest(); expect(element.attr('srcset')).toBe('http://example.com/image1.png 1x,unsafe:javascript:doEvilStuff() 2x'); })); - }); From 1786f3d08bace97b83e2cbe35ab30e79931413f5 Mon Sep 17 00:00:00 2001 From: ltrillaud Date: Fri, 19 Sep 2014 08:40:34 +0200 Subject: [PATCH 7/9] style(compile): use 2 spaces instead of tabs --- src/ng/compile.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ng/compile.js b/src/ng/compile.js index 354418b7445f..1a8d3cbcc952 100644 --- a/src/ng/compile.js +++ b/src/ng/compile.js @@ -844,7 +844,7 @@ function $CompileProvider($provide, $$sanitizeUriProvider) { nodeName = nodeName_(this.$$element); - if ((nodeName === 'a' && key === 'href') || + if ((nodeName === 'a' && key === 'href') || (nodeName === 'img' && key === 'src')) { // sanitize a[href] and img[src] values this[key] = value = $$sanitizeUri(value, key === 'src'); From f0cc05cd815e1b181a09ad88578533a4eec24c0d Mon Sep 17 00:00:00 2001 From: ltrillaud Date: Fri, 19 Sep 2014 08:57:44 +0200 Subject: [PATCH 8/9] style(compile): convert all my tabs by 2 spaces --- src/ng/compile.js | 68 +++++++++++++++++++++++------------------------ 1 file changed, 34 insertions(+), 34 deletions(-) diff --git a/src/ng/compile.js b/src/ng/compile.js index 1a8d3cbcc952..79074eb27f6f 100644 --- a/src/ng/compile.js +++ b/src/ng/compile.js @@ -845,41 +845,41 @@ function $CompileProvider($provide, $$sanitizeUriProvider) { nodeName = nodeName_(this.$$element); if ((nodeName === 'a' && key === 'href') || - (nodeName === 'img' && key === 'src')) { - // sanitize a[href] and img[src] values - this[key] = value = $$sanitizeUri(value, key === 'src'); + (nodeName === 'img' && key === 'src')) { + // sanitize a[href] and img[src] values + this[key] = value = $$sanitizeUri(value, key === 'src'); } else if (nodeName === 'img' && key === 'srcset') { - // sanitize img[srcset] values - var result = ""; - - // first check if there are spaces because it's not the same pattern - var trimedSrcset = trim(value); - var pattern = /\s/.test( trimedSrcset) ? /(\d+x\s*,|\d+w\s*,|\s+,|,\s+)/ : /(,)/; - - // split srcset into tupple of uri and descriptor except for the last item - var rawUris = trimedSrcset.split(pattern); - - // for each tupples - var nbrUrisWith2parts = Math.floor(rawUris.length / 2); - for (var i=0; i Date: Thu, 25 Sep 2014 09:41:20 +0200 Subject: [PATCH 9/9] this commit is only to trigger a new restart of Travis jobs --- test/ng/directive/ngSrcsetSpec.js | 1 + 1 file changed, 1 insertion(+) diff --git a/test/ng/directive/ngSrcsetSpec.js b/test/ng/directive/ngSrcsetSpec.js index 8c66b0d664ff..8d14ca5f1b79 100644 --- a/test/ng/directive/ngSrcsetSpec.js +++ b/test/ng/directive/ngSrcsetSpec.js @@ -29,3 +29,4 @@ describe('ngSrcset', function() { expect(element.attr('srcset')).toBe('http://example.com/image1.png 1x,unsafe:javascript:doEvilStuff() 2x'); })); }); +