From 525067d804eb177b7163a6bb12f2015b1a2307c2 Mon Sep 17 00:00:00 2001 From: Alex Dobkin Date: Thu, 15 Sep 2016 14:45:47 -0700 Subject: [PATCH 01/13] fix($sce): Consider document base URL when implementing 'self' URL policy. Page authors can use the base tag in HTML to specify URL to use as a base when resovling relative URLs. This can cause SCE to start rejecting relative URLs on the page because the fail the same-origin test. To improve compatiblity with the base tag, this commit changes the logic for matching urls to 'self' to allow URLs that match the protocol and domain of the base URL in addition to URLs that match the loading origin. Security note: If an attacker can inject a base tag into the page, they can circumvent SCE protections. Howerver, injecting a base tag typically requires the ability to inject arbitrary HTML into the page, which is a more serious vulnerabilty than bypassing SCE. --- src/ng/sce.js | 2 +- src/ng/urlUtils.js | 20 ++++++++++++++++++++ test/e2e/fixtures/base_tag/index.html | 10 ++++++++++ test/e2e/fixtures/base_tag/script.js | 12 ++++++++++++ test/e2e/tests/base_tag.spec.js | 14 ++++++++++++++ 5 files changed, 57 insertions(+), 1 deletion(-) create mode 100644 test/e2e/fixtures/base_tag/index.html create mode 100644 test/e2e/fixtures/base_tag/script.js create mode 100644 test/e2e/tests/base_tag.spec.js diff --git a/src/ng/sce.js b/src/ng/sce.js index acbfcea5cef5..e536bdc4d86b 100644 --- a/src/ng/sce.js +++ b/src/ng/sce.js @@ -227,7 +227,7 @@ function $SceDelegateProvider() { function matchUrl(matcher, parsedUrl) { if (matcher === 'self') { - return urlIsSameOrigin(parsedUrl); + return urlIsSameOrigin(parsedUrl) || urlIsSameOriginAsBaseUrl(parsedUrl); } else { // definitely a regex. See adjustMatchers() return !!matcher.exec(parsedUrl.href); diff --git a/src/ng/urlUtils.js b/src/ng/urlUtils.js index d8231ef078f3..e5b3aeffd0d1 100644 --- a/src/ng/urlUtils.js +++ b/src/ng/urlUtils.js @@ -8,6 +8,7 @@ // service. var urlParsingNode = window.document.createElement('a'); var originUrl = urlResolve(window.location.href); +var baseUrl; /** @@ -95,3 +96,22 @@ function urlIsSameOrigin(requestUrl) { return (parsed.protocol === originUrl.protocol && parsed.host === originUrl.host); } + +/** + * Parse a request URL and determine whether it is same-origin as the document base URL. + * + * Note: The base URL is usually the same the document location (`location.href`) but can + * overriden by using the `` tag. + * + * @param {string|object} requestUrl The url of the request as a string that will be resolved + * or a parsed URL object. + * @returns {boolean} Whether the URL is same-origin as the document base URL. + */ +function urlIsSameOriginAsBaseUrl(requestUrl) { + if (!baseUrl) { + baseUrl = urlResolve('.'); + } + var parsed = (isString(requestUrl)) ? urlResolve(requestUrl) : requestUrl; + return (parsed.protocol === baseUrl.protocol && + parsed.host === baseUrl.host); +} diff --git a/test/e2e/fixtures/base_tag/index.html b/test/e2e/fixtures/base_tag/index.html new file mode 100644 index 000000000000..22aba8baf49a --- /dev/null +++ b/test/e2e/fixtures/base_tag/index.html @@ -0,0 +1,10 @@ + + + + + + + + + + diff --git a/test/e2e/fixtures/base_tag/script.js b/test/e2e/fixtures/base_tag/script.js new file mode 100644 index 000000000000..73e414d3e0cd --- /dev/null +++ b/test/e2e/fixtures/base_tag/script.js @@ -0,0 +1,12 @@ +angular + .module('test', []) + .run(function($sce) { + window.isTrustedUrl = function(url) { + try { + $sce.getTrustedResourceUrl(url); + } catch (e) { + return false; + } + return true; + } + }); diff --git a/test/e2e/tests/base_tag.spec.js b/test/e2e/tests/base_tag.spec.js new file mode 100644 index 000000000000..595c463f2967 --- /dev/null +++ b/test/e2e/tests/base_tag.spec.js @@ -0,0 +1,14 @@ +'use strict'; + +describe('base_tag', function() { + it('SCE self URL policy should correctly handle base tags', function() { + loadFixture('base_tag'); + + var url = browser.getLocationAbsUrl(); + var urlIsTrusted = browser.executeScript('return isTrustedUrl(arguments[0])', url); + expect(urlIsTrusted).toBe(true); // sanity check + + urlIsTrusted = browser.executeScript('return isTrustedUrl("/relative")'); + expect(urlIsTrusted).toBe(true); + }); +}); From 08432a0ebc04c78fbd5d09a1a8714c927ef99f85 Mon Sep 17 00:00:00 2001 From: Alex Dobkin Date: Thu, 15 Sep 2016 15:41:54 -0700 Subject: [PATCH 02/13] improve test --- test/e2e/tests/base_tag.spec.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/test/e2e/tests/base_tag.spec.js b/test/e2e/tests/base_tag.spec.js index 595c463f2967..7836f26fbb7b 100644 --- a/test/e2e/tests/base_tag.spec.js +++ b/test/e2e/tests/base_tag.spec.js @@ -8,6 +8,9 @@ describe('base_tag', function() { var urlIsTrusted = browser.executeScript('return isTrustedUrl(arguments[0])', url); expect(urlIsTrusted).toBe(true); // sanity check + var urlIsTrusted = browser.executeScript('return isTrustedUrl("//evil.com/")'); + expect(urlIsTrusted).toBe(false); // sanity check + urlIsTrusted = browser.executeScript('return isTrustedUrl("/relative")'); expect(urlIsTrusted).toBe(true); }); From 382c4148d308676324c46b8ef2e510621032bb23 Mon Sep 17 00:00:00 2001 From: Aleksandr Dobkin Date: Wed, 21 Sep 2016 17:14:26 -0700 Subject: [PATCH 03/13] track changes to base URL and other refactorings --- src/.eslintrc.json | 1 + src/ng/urlUtils.js | 88 ++++++++++++++++++--------- test/e2e/fixtures/base_tag/index.html | 2 +- test/e2e/fixtures/base_tag/script.js | 10 +-- test/e2e/tests/base_tag.spec.js | 37 ++++++++--- 5 files changed, 95 insertions(+), 43 deletions(-) diff --git a/src/.eslintrc.json b/src/.eslintrc.json index 6022ac25baa9..dc505098cfed 100644 --- a/src/.eslintrc.json +++ b/src/.eslintrc.json @@ -155,6 +155,7 @@ /* urlUtils.js */ "urlResolve": false, "urlIsSameOrigin": false, + "urlIsSameOriginAsBaseUrl": false, /* ng/controller.js */ "identifierForController": false, diff --git a/src/ng/urlUtils.js b/src/ng/urlUtils.js index e5b3aeffd0d1..e04189aec1ff 100644 --- a/src/ng/urlUtils.js +++ b/src/ng/urlUtils.js @@ -8,7 +8,7 @@ // service. var urlParsingNode = window.document.createElement('a'); var originUrl = urlResolve(window.location.href); -var baseUrl; +var baseUrlParsingNode; /** @@ -44,16 +44,16 @@ var baseUrl; * @description Normalizes and parses a URL. * @returns {object} Returns the normalized URL as a dictionary. * - * | member name | Description | - * |---------------|----------------| + * | member name | Description | + * |---------------|------------------------------------------------------------------------| * | href | A normalized version of the provided URL if it was not an absolute URL | - * | protocol | The protocol including the trailing colon | + * | protocol | The protocol without the trailing colon | * | host | The host and port (if the port is non-default) of the normalizedUrl | * | search | The search params, minus the question mark | - * | hash | The hash string, minus the hash symbol - * | hostname | The hostname - * | port | The port, without ":" - * | pathname | The pathname, beginning with "/" + * | hash | The hash string, minus the hash symbol | + * | hostname | The hostname | + * | port | The port, without ":" | + * | pathname | The pathname, beginning with "/" | * */ function urlResolve(url) { @@ -69,19 +69,7 @@ function urlResolve(url) { urlParsingNode.setAttribute('href', href); - // urlParsingNode provides the UrlUtils interface - http://url.spec.whatwg.org/#urlutils - return { - href: urlParsingNode.href, - protocol: urlParsingNode.protocol ? urlParsingNode.protocol.replace(/:$/, '') : '', - host: urlParsingNode.host, - search: urlParsingNode.search ? urlParsingNode.search.replace(/^\?/, '') : '', - hash: urlParsingNode.hash ? urlParsingNode.hash.replace(/^#/, '') : '', - hostname: urlParsingNode.hostname, - port: urlParsingNode.port, - pathname: (urlParsingNode.pathname.charAt(0) === '/') - ? urlParsingNode.pathname - : '/' + urlParsingNode.pathname - }; + return anchorElementToObject(urlParsingNode); } /** @@ -93,12 +81,11 @@ function urlResolve(url) { */ function urlIsSameOrigin(requestUrl) { var parsed = (isString(requestUrl)) ? urlResolve(requestUrl) : requestUrl; - return (parsed.protocol === originUrl.protocol && - parsed.host === originUrl.host); + return urlsAreSameOrigin(parsed, originUrl); } /** - * Parse a request URL and determine whether it is same-origin as the document base URL. + * Parse a request URL and determine whether it is same-origin as the current document base URL. * * Note: The base URL is usually the same the document location (`location.href`) but can * overriden by using the `` tag. @@ -108,10 +95,55 @@ function urlIsSameOrigin(requestUrl) { * @returns {boolean} Whether the URL is same-origin as the document base URL. */ function urlIsSameOriginAsBaseUrl(requestUrl) { - if (!baseUrl) { - baseUrl = urlResolve('.'); + if (!baseUrlParsingNode) { + baseUrlParsingNode = window.document.createElement('a'); + baseUrlParsingNode.href = '.'; + + if (msie) { + // Work-around for IE bug described in Implementation Notes. The fix in urlResolve() is not + // suitable here because we need to track changes to the base URL. + baseUrlParsingNode = baseUrlParsingNode.cloneNode(false); + } } var parsed = (isString(requestUrl)) ? urlResolve(requestUrl) : requestUrl; - return (parsed.protocol === baseUrl.protocol && - parsed.host === baseUrl.host); + return urlsAreSameOrigin(parsed, anchorElementToObject(baseUrlParsingNode)); +} + +/** + * Determines if two URLs share the same origin. + * + * @param {object} url1 First URL to compare. Must be a normalized URL in the form of a + * dictionary object returned by `urlResolve()`. + * @param {object} url2 Second URL to compare. Must be a normalized URL in the form of a + * dictionary object returned by `urlResolve()`. + * @return {boolean} True if both URLs have the same origin, and false otherwise. + */ +function urlsAreSameOrigin(url1, url2) { + // IE sometimes includes a port in the 'host' property, even if it is the default 80 port so + // we check hostname and port separately. + return url1.protocol === url2.protocol && + url1.hostname === url2.hostname && + url1.port === url2.port; +} + +/** + * Converts properties in the given anchor element into a dictionary object as described in the + * documentation for `urlResolve()`. + * @param {HTMLAnchorElement} elem + * @returns {object} + */ +function anchorElementToObject(elem) { + // elem provides the UrlUtils interface - http://url.spec.whatwg.org/#urlutils + return { + href: elem.href, + protocol: elem.protocol ? elem.protocol.replace(/:$/, '') : '', + host: elem.host, + search: elem.search ? elem.search.replace(/^\?/, '') : '', + hash: elem.hash ? elem.hash.replace(/^#/, '') : '', + hostname: elem.hostname, + port: elem.port, + pathname: (elem.pathname.charAt(0) === '/') + ? elem.pathname + : '/' + elem.pathname + }; } diff --git a/test/e2e/fixtures/base_tag/index.html b/test/e2e/fixtures/base_tag/index.html index 22aba8baf49a..6eea88042cc2 100644 --- a/test/e2e/fixtures/base_tag/index.html +++ b/test/e2e/fixtures/base_tag/index.html @@ -1,7 +1,7 @@ - + diff --git a/test/e2e/fixtures/base_tag/script.js b/test/e2e/fixtures/base_tag/script.js index 73e414d3e0cd..79a3d81bdb8c 100644 --- a/test/e2e/fixtures/base_tag/script.js +++ b/test/e2e/fixtures/base_tag/script.js @@ -1,6 +1,8 @@ -angular - .module('test', []) - .run(function($sce) { +'use strict'; + +angular. + module('test', []). + run(function($sce) { window.isTrustedUrl = function(url) { try { $sce.getTrustedResourceUrl(url); @@ -8,5 +10,5 @@ angular return false; } return true; - } + }; }); diff --git a/test/e2e/tests/base_tag.spec.js b/test/e2e/tests/base_tag.spec.js index 7836f26fbb7b..b21f044a7585 100644 --- a/test/e2e/tests/base_tag.spec.js +++ b/test/e2e/tests/base_tag.spec.js @@ -1,17 +1,34 @@ 'use strict'; -describe('base_tag', function() { - it('SCE self URL policy should correctly handle base tags', function() { - loadFixture('base_tag'); - - var url = browser.getLocationAbsUrl(); +describe('SCE URL policy when base tags are present', function() { + function checkUrl(url, allowed) { var urlIsTrusted = browser.executeScript('return isTrustedUrl(arguments[0])', url); - expect(urlIsTrusted).toBe(true); // sanity check + expect(urlIsTrusted).toBe(allowed); + } + loadFixture('base_tag'); + + // sanity checks + it('allows the page URL (location.href)', function() { + checkUrl(browser.getLocationAbsUrl(), true); + }); + it('blocks off-origin URLs', function() { + //browser.pause(); + checkUrl('http://evil.com', false); + }); - var urlIsTrusted = browser.executeScript('return isTrustedUrl("//evil.com/")'); - expect(urlIsTrusted).toBe(false); // sanity check + it('allows relative URLs ("/relative")', function() { + //browser.pause(); + checkUrl('/relative', true); + }); + + it('allows absolute URLs from the base origin', function() { + checkUrl('http://www.example.com/path/to/file.html', true); + }); - urlIsTrusted = browser.executeScript('return isTrustedUrl("/relative")'); - expect(urlIsTrusted).toBe(true); + it('tracks changes to the base URL', function() { + browser.executeScript( + 'document.getElementsByTagName("base")[0].href = "http://xxx.example.com/";'); + //browser.pause(); + checkUrl('http://xxx.example.com/path/to/file.html', true); }); }); From 49b8e0d53cd56be3ae0a1440f0b73aba6ec65287 Mon Sep 17 00:00:00 2001 From: Aleksandr Dobkin Date: Wed, 21 Sep 2016 17:52:50 -0700 Subject: [PATCH 04/13] fix tests --- test/e2e/tests/base_tag.spec.js | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/test/e2e/tests/base_tag.spec.js b/test/e2e/tests/base_tag.spec.js index b21f044a7585..e10a9753a9a2 100644 --- a/test/e2e/tests/base_tag.spec.js +++ b/test/e2e/tests/base_tag.spec.js @@ -5,19 +5,20 @@ describe('SCE URL policy when base tags are present', function() { var urlIsTrusted = browser.executeScript('return isTrustedUrl(arguments[0])', url); expect(urlIsTrusted).toBe(allowed); } - loadFixture('base_tag'); - // sanity checks + beforeAll(function() { + loadFixture('base_tag'); + }); + it('allows the page URL (location.href)', function() { checkUrl(browser.getLocationAbsUrl(), true); }); + it('blocks off-origin URLs', function() { - //browser.pause(); checkUrl('http://evil.com', false); }); it('allows relative URLs ("/relative")', function() { - //browser.pause(); checkUrl('/relative', true); }); @@ -28,7 +29,6 @@ describe('SCE URL policy when base tags are present', function() { it('tracks changes to the base URL', function() { browser.executeScript( 'document.getElementsByTagName("base")[0].href = "http://xxx.example.com/";'); - //browser.pause(); checkUrl('http://xxx.example.com/path/to/file.html', true); }); }); From abd7dd7cb59feeda2f6159fa442fb83023c653d8 Mon Sep 17 00:00:00 2001 From: Aleksandr Dobkin Date: Wed, 21 Sep 2016 19:51:19 -0700 Subject: [PATCH 05/13] make urlIsSameOriginAsBaseUrl() run on Linux --- src/ng/urlUtils.js | 61 +++++++++++++++++++++++----------------------- 1 file changed, 31 insertions(+), 30 deletions(-) diff --git a/src/ng/urlUtils.js b/src/ng/urlUtils.js index e04189aec1ff..679599f68387 100644 --- a/src/ng/urlUtils.js +++ b/src/ng/urlUtils.js @@ -69,7 +69,18 @@ function urlResolve(url) { urlParsingNode.setAttribute('href', href); - return anchorElementToObject(urlParsingNode); + return { + href: urlParsingNode.href, + protocol: urlParsingNode.protocol ? urlParsingNode.protocol.replace(/:$/, '') : '', + host: urlParsingNode.host, + search: urlParsingNode.search ? urlParsingNode.search.replace(/^\?/, '') : '', + hash: urlParsingNode.hash ? urlParsingNode.hash.replace(/^#/, '') : '', + hostname: urlParsingNode.hostname, + port: urlParsingNode.port, + pathname: (urlParsingNode.pathname.charAt(0) === '/') + ? urlParsingNode.pathname + : '/' + urlParsingNode.pathname + }; } /** @@ -95,18 +106,9 @@ function urlIsSameOrigin(requestUrl) { * @returns {boolean} Whether the URL is same-origin as the document base URL. */ function urlIsSameOriginAsBaseUrl(requestUrl) { - if (!baseUrlParsingNode) { - baseUrlParsingNode = window.document.createElement('a'); - baseUrlParsingNode.href = '.'; - - if (msie) { - // Work-around for IE bug described in Implementation Notes. The fix in urlResolve() is not - // suitable here because we need to track changes to the base URL. - baseUrlParsingNode = baseUrlParsingNode.cloneNode(false); - } - } var parsed = (isString(requestUrl)) ? urlResolve(requestUrl) : requestUrl; - return urlsAreSameOrigin(parsed, anchorElementToObject(baseUrlParsingNode)); + var base = urlResolve(getBaseUrl()); + return urlsAreSameOrigin(parsed, base); } /** @@ -127,23 +129,22 @@ function urlsAreSameOrigin(url1, url2) { } /** - * Converts properties in the given anchor element into a dictionary object as described in the - * documentation for `urlResolve()`. - * @param {HTMLAnchorElement} elem - * @returns {object} + * Returns the current document base URL. + * @return {string} */ -function anchorElementToObject(elem) { - // elem provides the UrlUtils interface - http://url.spec.whatwg.org/#urlutils - return { - href: elem.href, - protocol: elem.protocol ? elem.protocol.replace(/:$/, '') : '', - host: elem.host, - search: elem.search ? elem.search.replace(/^\?/, '') : '', - hash: elem.hash ? elem.hash.replace(/^#/, '') : '', - hostname: elem.hostname, - port: elem.port, - pathname: (elem.pathname.charAt(0) === '/') - ? elem.pathname - : '/' + elem.pathname - }; +function getBaseUrl() { + if (window.document.baseURI) { + return window.document.baseURI; + } + + // document.baseURI is available everywhere except IE + if (!baseUrlParsingNode) { + baseUrlParsingNode = window.document.createElement('a'); + baseUrlParsingNode.href = '.'; + + // Work-around for IE bug described in Implementation Notes. The fix in urlResolve() is not + // suitable here because we need to track changes to the base URL. + baseUrlParsingNode = baseUrlParsingNode.cloneNode(false); + } + return baseUrlParsingNode.href; } From 38ddb97586f187fbf38a0d88e19b8660cc3764ac Mon Sep 17 00:00:00 2001 From: Aleksandr Dobkin Date: Fri, 30 Sep 2016 15:38:50 -0700 Subject: [PATCH 06/13] review feedback --- src/ng/urlUtils.js | 20 +++++++++---------- .../{base_tag => base-tag}/index.html | 2 +- .../fixtures/{base_tag => base-tag}/script.js | 0 .../{base_tag.spec.js => base-tag.spec.js} | 3 ++- 4 files changed, 13 insertions(+), 12 deletions(-) rename test/e2e/fixtures/{base_tag => base-tag}/index.html (76%) rename test/e2e/fixtures/{base_tag => base-tag}/script.js (100%) rename test/e2e/tests/{base_tag.spec.js => base-tag.spec.js} (91%) diff --git a/src/ng/urlUtils.js b/src/ng/urlUtils.js index 679599f68387..4e23ddd0e88e 100644 --- a/src/ng/urlUtils.js +++ b/src/ng/urlUtils.js @@ -99,7 +99,7 @@ function urlIsSameOrigin(requestUrl) { * Parse a request URL and determine whether it is same-origin as the current document base URL. * * Note: The base URL is usually the same the document location (`location.href`) but can - * overriden by using the `` tag. + * be overriden by using the `` tag. * * @param {string|object} requestUrl The url of the request as a string that will be resolved * or a parsed URL object. @@ -114,18 +114,18 @@ function urlIsSameOriginAsBaseUrl(requestUrl) { /** * Determines if two URLs share the same origin. * - * @param {object} url1 First URL to compare. Must be a normalized URL in the form of a - * dictionary object returned by `urlResolve()`. - * @param {object} url2 Second URL to compare. Must be a normalized URL in the form of a - * dictionary object returned by `urlResolve()`. + * @param {string|object} url1 First URL to compare as a string or a normalized URL in the form of + * a dictionary object returned by `urlResolve()`. + * @param {string|object} url2 Second URL to compare as a string or a normalized URL in the form of + * a dictionary object returned by `urlResolve()`. * @return {boolean} True if both URLs have the same origin, and false otherwise. */ function urlsAreSameOrigin(url1, url2) { - // IE sometimes includes a port in the 'host' property, even if it is the default 80 port so - // we check hostname and port separately. - return url1.protocol === url2.protocol && - url1.hostname === url2.hostname && - url1.port === url2.port; + url1 = (isString(url1)) ? urlResolve(url1) : url1; + url2 = (isString(url2)) ? urlResolve(url2) : url2; + + return (url1.protocol === url2.protocol && + url1.host === url2.host); } /** diff --git a/test/e2e/fixtures/base_tag/index.html b/test/e2e/fixtures/base-tag/index.html similarity index 76% rename from test/e2e/fixtures/base_tag/index.html rename to test/e2e/fixtures/base-tag/index.html index 6eea88042cc2..929cc1c18b44 100644 --- a/test/e2e/fixtures/base_tag/index.html +++ b/test/e2e/fixtures/base-tag/index.html @@ -3,7 +3,7 @@ - + diff --git a/test/e2e/fixtures/base_tag/script.js b/test/e2e/fixtures/base-tag/script.js similarity index 100% rename from test/e2e/fixtures/base_tag/script.js rename to test/e2e/fixtures/base-tag/script.js diff --git a/test/e2e/tests/base_tag.spec.js b/test/e2e/tests/base-tag.spec.js similarity index 91% rename from test/e2e/tests/base_tag.spec.js rename to test/e2e/tests/base-tag.spec.js index e10a9753a9a2..5780c4df4c79 100644 --- a/test/e2e/tests/base_tag.spec.js +++ b/test/e2e/tests/base-tag.spec.js @@ -7,7 +7,7 @@ describe('SCE URL policy when base tags are present', function() { } beforeAll(function() { - loadFixture('base_tag'); + loadFixture('base-tag'); }); it('allows the page URL (location.href)', function() { @@ -30,5 +30,6 @@ describe('SCE URL policy when base tags are present', function() { browser.executeScript( 'document.getElementsByTagName("base")[0].href = "http://xxx.example.com/";'); checkUrl('http://xxx.example.com/path/to/file.html', true); + checkUrl('http://www.example.com/path/to/file.html', false); }); }); From 48a0123017729448aa799c68220a3384ba56c24a Mon Sep 17 00:00:00 2001 From: Aleksandr Dobkin Date: Fri, 30 Sep 2016 16:59:14 -0700 Subject: [PATCH 07/13] remove redundant url parsing calls --- src/ng/urlUtils.js | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/src/ng/urlUtils.js b/src/ng/urlUtils.js index 4e23ddd0e88e..82574f6c8230 100644 --- a/src/ng/urlUtils.js +++ b/src/ng/urlUtils.js @@ -91,8 +91,7 @@ function urlResolve(url) { * @returns {boolean} Whether the request is for the same origin as the application document. */ function urlIsSameOrigin(requestUrl) { - var parsed = (isString(requestUrl)) ? urlResolve(requestUrl) : requestUrl; - return urlsAreSameOrigin(parsed, originUrl); + return urlsAreSameOrigin(requestUrl, originUrl); } /** @@ -106,9 +105,7 @@ function urlIsSameOrigin(requestUrl) { * @returns {boolean} Whether the URL is same-origin as the document base URL. */ function urlIsSameOriginAsBaseUrl(requestUrl) { - var parsed = (isString(requestUrl)) ? urlResolve(requestUrl) : requestUrl; - var base = urlResolve(getBaseUrl()); - return urlsAreSameOrigin(parsed, base); + return urlsAreSameOrigin(requestUrl, getBaseUrl()); } /** From d2b2d375b4b3344bb05f7b2770c23f2e50b5c4f6 Mon Sep 17 00:00:00 2001 From: Aleksandr Dobkin Date: Mon, 3 Oct 2016 15:05:38 -0700 Subject: [PATCH 08/13] Add unit test --- test/.eslintrc.json | 1 + test/ng/urlUtilsSpec.js | 8 +++++++- 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/test/.eslintrc.json b/test/.eslintrc.json index 4ac15d192fa3..62f21300db30 100644 --- a/test/.eslintrc.json +++ b/test/.eslintrc.json @@ -148,6 +148,7 @@ /* urlUtils.js */ "urlResolve": false, "urlIsSameOrigin": false, + "urlIsSameOriginAsBaseUrl": true, /* karma */ "dump": false, diff --git a/test/ng/urlUtilsSpec.js b/test/ng/urlUtilsSpec.js index c1d24fe645d4..233181c80eb7 100644 --- a/test/ng/urlUtilsSpec.js +++ b/test/ng/urlUtilsSpec.js @@ -23,11 +23,17 @@ describe('urlUtils', function() { }); }); - describe('isSameOrigin', function() { + describe('isSameOrigin and urlIsSameOriginAsBaseUrl', function() { 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); + + // urlIsSameOriginAsBaseUrl() should behave the same as urlIsSameOrigin() by default. + // Behavior when there is a non-default base URL or when the base URL changes dynamically + // is tested in the end-to-end tests in e2e/tests/base-tag.spec.js. + expect(urlIsSameOriginAsBaseUrl(url)).toBe(expectedValue); + expect(urlIsSameOriginAsBaseUrl(urlResolve(url))).toBe(expectedValue); } expectIsSameOrigin('path', true); var origin = urlResolve($document[0].location.href); From 9db4eafa5d42dd7cd33258ee66aea98e758a86c6 Mon Sep 17 00:00:00 2001 From: Aleksandr Dobkin Date: Wed, 4 Jan 2017 15:21:07 -0800 Subject: [PATCH 09/13] adding unit test to sceSpec.js --- test/ng/sceSpecs.js | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/test/ng/sceSpecs.js b/test/ng/sceSpecs.js index 97f8f338f5cd..18782e157e3a 100644 --- a/test/ng/sceSpecs.js +++ b/test/ng/sceSpecs.js @@ -464,6 +464,38 @@ describe('SCE', function() { '$sce', 'insecurl', 'Blocked loading resource from url not allowed by $sceDelegate policy. URL: foo'); } )); + + describe('when the document base URL has changed', function() { + var baseElem = document.createElement('BASE'); + var cfg = {whitelist: ['self'], blacklist: []}; + baseElem.setAttribute('href', '//foo.example.com/path/'); + beforeAll(function() { + document.head.appendChild(baseElem); + }); + afterAll(function() {; + document.head.removeChild(baseElem); + }); + function expectAllowed($sce, url) { + expect($sce.getTrustedResourceUrl(url)).toEqual(url); + } + + function expectBlocked($sce, url) { + expect(function() { $sce.getTrustedResourceUrl(url); }).toThrowMinErr( + '$sce', 'insecurl', 'Blocked loading resource from url not allowed by $sceDelegate policy. URL: ' + url); + } + + it('should allow relative URLs', runTest(cfg, function($sce) { + expectAllowed($sce, 'foo'); + })); + + it('should allow absolute URLs', runTest(cfg, function($sce) { + expectAllowed($sce, '//foo.example.com/bar'); + })); + + it('should still block some URLs', runTest(cfg, function($sce) { + expectBlocked($sce, '//bad.example.com'); + })); + }); }); it('should have blacklist override the whitelist', runTest( From 01fc4f11c47eeb6b4950ca3650681d49eb8e543b Mon Sep 17 00:00:00 2001 From: Aleksandr Dobkin Date: Wed, 4 Jan 2017 17:35:11 -0800 Subject: [PATCH 10/13] lint fixups --- test/ng/sceSpecs.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/test/ng/sceSpecs.js b/test/ng/sceSpecs.js index 18782e157e3a..0bcf5fe9b3e3 100644 --- a/test/ng/sceSpecs.js +++ b/test/ng/sceSpecs.js @@ -466,14 +466,14 @@ describe('SCE', function() { )); describe('when the document base URL has changed', function() { - var baseElem = document.createElement('BASE'); + var baseElem = window.document.createElement('BASE'); var cfg = {whitelist: ['self'], blacklist: []}; baseElem.setAttribute('href', '//foo.example.com/path/'); beforeAll(function() { - document.head.appendChild(baseElem); + window.document.head.appendChild(baseElem); }); - afterAll(function() {; - document.head.removeChild(baseElem); + afterAll(function() { + window.document.head.removeChild(baseElem); }); function expectAllowed($sce, url) { expect($sce.getTrustedResourceUrl(url)).toEqual(url); From ebf45a44b42bff2d4016715f7df95b6344ea4881 Mon Sep 17 00:00:00 2001 From: Aleksandr Dobkin Date: Wed, 4 Jan 2017 18:08:27 -0800 Subject: [PATCH 11/13] fix test for IE9 --- test/ng/sceSpecs.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/ng/sceSpecs.js b/test/ng/sceSpecs.js index 0bcf5fe9b3e3..48f70cd243fd 100644 --- a/test/ng/sceSpecs.js +++ b/test/ng/sceSpecs.js @@ -468,7 +468,7 @@ describe('SCE', function() { describe('when the document base URL has changed', function() { var baseElem = window.document.createElement('BASE'); var cfg = {whitelist: ['self'], blacklist: []}; - baseElem.setAttribute('href', '//foo.example.com/path/'); + baseElem.setAttribute('href', location.protocol + '//foo.example.com/path/'); beforeAll(function() { window.document.head.appendChild(baseElem); }); From 79ab27acae9a23e1d3929353ec28584b9b199265 Mon Sep 17 00:00:00 2001 From: Aleksandr Dobkin Date: Thu, 5 Jan 2017 10:57:26 -0800 Subject: [PATCH 12/13] fix lint issue --- test/ng/sceSpecs.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/ng/sceSpecs.js b/test/ng/sceSpecs.js index 48f70cd243fd..5e19a9803958 100644 --- a/test/ng/sceSpecs.js +++ b/test/ng/sceSpecs.js @@ -468,7 +468,7 @@ describe('SCE', function() { describe('when the document base URL has changed', function() { var baseElem = window.document.createElement('BASE'); var cfg = {whitelist: ['self'], blacklist: []}; - baseElem.setAttribute('href', location.protocol + '//foo.example.com/path/'); + baseElem.setAttribute('href', window.location.protocol + '//foo.example.com/path/'); beforeAll(function() { window.document.head.appendChild(baseElem); }); From f6f5d9eb8b1ef55ac40798657c2535bfd8c5d35e Mon Sep 17 00:00:00 2001 From: Aleksandr Dobkin Date: Mon, 9 Jan 2017 17:36:17 -0800 Subject: [PATCH 13/13] fix typo and refactor tests as per code review --- src/ng/urlUtils.js | 2 +- test/ng/sceSpecs.js | 24 +++++++++--------------- 2 files changed, 10 insertions(+), 16 deletions(-) diff --git a/src/ng/urlUtils.js b/src/ng/urlUtils.js index 82574f6c8230..0aa7d35b6fef 100644 --- a/src/ng/urlUtils.js +++ b/src/ng/urlUtils.js @@ -97,7 +97,7 @@ function urlIsSameOrigin(requestUrl) { /** * Parse a request URL and determine whether it is same-origin as the current document base URL. * - * Note: The base URL is usually the same the document location (`location.href`) but can + * Note: The base URL is usually the same as the document location (`location.href`) but can * be overriden by using the `` tag. * * @param {string|object} requestUrl The url of the request as a string that will be resolved diff --git a/test/ng/sceSpecs.js b/test/ng/sceSpecs.js index 5e19a9803958..0b4d4249685c 100644 --- a/test/ng/sceSpecs.js +++ b/test/ng/sceSpecs.js @@ -466,34 +466,28 @@ describe('SCE', function() { )); describe('when the document base URL has changed', function() { - var baseElem = window.document.createElement('BASE'); + var baseElem; var cfg = {whitelist: ['self'], blacklist: []}; - baseElem.setAttribute('href', window.location.protocol + '//foo.example.com/path/'); - beforeAll(function() { + beforeEach(function() { + baseElem = window.document.createElement('BASE'); + baseElem.setAttribute('href', window.location.protocol + '//foo.example.com/path/'); window.document.head.appendChild(baseElem); }); - afterAll(function() { + afterEach(function() { window.document.head.removeChild(baseElem); }); - function expectAllowed($sce, url) { - expect($sce.getTrustedResourceUrl(url)).toEqual(url); - } - - function expectBlocked($sce, url) { - expect(function() { $sce.getTrustedResourceUrl(url); }).toThrowMinErr( - '$sce', 'insecurl', 'Blocked loading resource from url not allowed by $sceDelegate policy. URL: ' + url); - } it('should allow relative URLs', runTest(cfg, function($sce) { - expectAllowed($sce, 'foo'); + expect($sce.getTrustedResourceUrl('foo')).toEqual('foo'); })); it('should allow absolute URLs', runTest(cfg, function($sce) { - expectAllowed($sce, '//foo.example.com/bar'); + expect($sce.getTrustedResourceUrl('//foo.example.com/bar')).toEqual('//foo.example.com/bar'); })); it('should still block some URLs', runTest(cfg, function($sce) { - expectBlocked($sce, '//bad.example.com'); + expect(function() { $sce.getTrustedResourceUrl('//bad.example.com'); }).toThrowMinErr( + '$sce', 'insecurl', 'Blocked loading resource from url not allowed by $sceDelegate policy. URL: //bad.example.com'); })); }); });