From 7c0a2e1312afa5f17ee288298f18595c340ea404 Mon Sep 17 00:00:00 2001 From: Raphael Jamet Date: Wed, 21 Dec 2016 16:14:09 +0100 Subject: [PATCH 1/3] fix(urlUtils): Replace urlIsSameOrigin's use of location with baseURI When a base tag is set in the page, relative links will be resolved to its href attribute. However, isSameOrigin currently checks against the location, so relative links wouldn't be considered same-origin for security purposes. This commit changes to baseURI: it should behave the same when baseURI is left to the default value, but that preserves the implicit trust in relative URIs when baseURI is changed. --- src/ng/urlUtils.js | 14 +++++++++++--- test/ng/urlUtilsSpec.js | 24 ++++++++++++++++++++---- 2 files changed, 31 insertions(+), 7 deletions(-) diff --git a/src/ng/urlUtils.js b/src/ng/urlUtils.js index d8231ef078f3..7c8e7e3ad6ed 100644 --- a/src/ng/urlUtils.js +++ b/src/ng/urlUtils.js @@ -7,7 +7,13 @@ // exactly the behavior needed here. There is little value is mocking these out for this // service. var urlParsingNode = window.document.createElement('a'); -var originUrl = urlResolve(window.location.href); + +// We use the baseURI instead of the location. The behavior of relative URIs is more intuitive +// this way when mixed with a base tag, and the $sce security checks will rely on this. Also note +// that baseURI can evolve over time, so we need to reevaluate. +var getOriginUrl = function() { + return urlResolve(window.document.baseURI); +}; /** @@ -85,6 +91,8 @@ function urlResolve(url) { /** * Parse a request URL and determine whether this is a same-origin request as the application document. + * We rely on document.baseURI to determine our origin, to make applications that use base behave as + * expected with relative URIs. * * @param {string|object} requestUrl The url of the request as a string that will be resolved * or a parsed URL object. @@ -92,6 +100,6 @@ function urlResolve(url) { */ function urlIsSameOrigin(requestUrl) { var parsed = (isString(requestUrl)) ? urlResolve(requestUrl) : requestUrl; - return (parsed.protocol === originUrl.protocol && - parsed.host === originUrl.host); + return (parsed.protocol === getOriginUrl().protocol && + parsed.host === getOriginUrl().host); } diff --git a/test/ng/urlUtilsSpec.js b/test/ng/urlUtilsSpec.js index c1d24fe645d4..2839791d5e9c 100644 --- a/test/ng/urlUtilsSpec.js +++ b/test/ng/urlUtilsSpec.js @@ -24,11 +24,13 @@ describe('urlUtils', function() { }); describe('isSameOrigin', function() { + + function expectIsSameOrigin(url, expectedValue) { + expect(urlIsSameOrigin(url)).toBe(expectedValue); + expect(urlIsSameOrigin(urlResolve(url))).toBe(expectedValue); + } + it('should support various combinations of urls - both string and parsed', inject(function($document) { - function expectIsSameOrigin(url, expectedValue) { - expect(urlIsSameOrigin(url)).toBe(expectedValue); - expect(urlIsSameOrigin(urlResolve(url))).toBe(expectedValue); - } expectIsSameOrigin('path', true); var origin = urlResolve($document[0].location.href); expectIsSameOrigin('//' + origin.host + '/path', true); @@ -40,5 +42,19 @@ describe('urlUtils', function() { // This assumes that the test is *not* running on port 22 (very unlikely). expectIsSameOrigin('//' + origin.hostname + ':22/path', false); })); + + + it('should follow document.baseURI', inject(function($document) { + $document[0].body.appendChild($document[0].createElement('base')); + $document[0].body.lastChild.href = 'http://example.com/'; + expectIsSameOrigin('path', true); + var origin = urlResolve($document[0].location.href); + + // Real origin shouldn't be considered okay anymore. + expectIsSameOrigin('//' + origin.host + '/path', false); + + // But the baseURI should. + expectIsSameOrigin('http://example.com/path', true); + })); }); }); From 08665f4980b7b2284e837f34b2fb4b7b8547e170 Mon Sep 17 00:00:00 2001 From: Raphael Jamet Date: Wed, 21 Dec 2016 16:49:37 +0100 Subject: [PATCH 2/3] fix($sce) : Bump base href from URL context to RESOURCE_URL It's hard to understand the ramifications of base href, but it allows changing the destination of relative links elsewhere in the document. Spooky action at a distance is enough to put it on the list of dangerous attributes. --- src/ng/compile.js | 3 +++ src/ng/sce.js | 1 + test/ng/compileSpec.js | 16 ++++++++++++++++ 3 files changed, 20 insertions(+) diff --git a/src/ng/compile.js b/src/ng/compile.js index 90cedf61146e..d01479b3ae52 100644 --- a/src/ng/compile.js +++ b/src/ng/compile.js @@ -3256,6 +3256,9 @@ function $CompileProvider($provide, $$sanitizeUriProvider) { // maction[xlink:href] can source SVG. It's not limited to . } else if (attrNormalizedName === 'xlinkHref' || (tag === 'form' && attrNormalizedName === 'action') || + // base href is just plain weird and scary. If relative URLs don't go where they are + // expected to, all bets are off. + (tag === 'base' && attrNormalizedName === 'href') || // links can be stylesheets or imports, which can run script in the current origin (tag === 'link' && attrNormalizedName === 'href') ) { diff --git a/src/ng/sce.js b/src/ng/sce.js index acbfcea5cef5..7a139b66c9ea 100644 --- a/src/ng/sce.js +++ b/src/ng/sce.js @@ -235,6 +235,7 @@ function $SceDelegateProvider() { } function isResourceUrlAllowedByPolicy(url) { + // The urlResolve step should take care of baseURI if it's set. var parsedUrl = urlResolve(url.toString()); var i, n, allowed = false; // Ensure that at least one item from the whitelist allows this url. diff --git a/test/ng/compileSpec.js b/test/ng/compileSpec.js index e667ade6f3be..02ae5f497970 100644 --- a/test/ng/compileSpec.js +++ b/test/ng/compileSpec.js @@ -11162,6 +11162,22 @@ describe('$compile', function() { })); }); + describe('base[href]', function() { + it('should be a RESOURCE_URL context', inject(function($compile, $rootScope, $sce) { + element = $compile('')($rootScope); + + $rootScope.testUrl = $sce.trustAsResourceUrl('https://example.com/'); + $rootScope.$apply(); + expect(element.attr('href')).toContain('https://example.com/'); + + $rootScope.testUrl = 'https://not.example.com/'; + expect(function() { $rootScope.$apply(); }).toThrowMinErr( + '$interpolate', 'interr', 'Can\'t interpolate: {{testUrl}}\nError: [$sce:insecurl] Blocked ' + + 'loading resource from url not allowed by $sceDelegate policy. URL: ' + + 'https://not.example.com/'); + })); + }); + // Support: IE 9-10 only // IEs <11 don't support srcdoc if (!msie || msie === 11) { From 4926c6ccb86a5794782d5b4971723c82ddd94187 Mon Sep 17 00:00:00 2001 From: Raphael Jamet Date: Wed, 21 Dec 2016 17:18:46 +0100 Subject: [PATCH 3/3] fix(*) : Try and fix broken urlUtils tests on IE IE seems to require base to be in head, and clean up after testing. --- test/ng/urlUtilsSpec.js | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/test/ng/urlUtilsSpec.js b/test/ng/urlUtilsSpec.js index 2839791d5e9c..d1a46daa61e4 100644 --- a/test/ng/urlUtilsSpec.js +++ b/test/ng/urlUtilsSpec.js @@ -45,8 +45,8 @@ describe('urlUtils', function() { it('should follow document.baseURI', inject(function($document) { - $document[0].body.appendChild($document[0].createElement('base')); - $document[0].body.lastChild.href = 'http://example.com/'; + $document[0].head.appendChild($document[0].createElement('base')); + $document[0].head.lastChild.href = 'http://example.com/'; expectIsSameOrigin('path', true); var origin = urlResolve($document[0].location.href); @@ -55,6 +55,7 @@ describe('urlUtils', function() { // But the baseURI should. expectIsSameOrigin('http://example.com/path', true); + $document[0].head.lastChild.href = null; })); }); });