Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

fix($sce): Consider document base URL for 'self' URL policy. #15145

Closed
wants to merge 13 commits into from
1 change: 1 addition & 0 deletions src/.eslintrc.json
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,7 @@
/* urlUtils.js */
"urlResolve": false,
"urlIsSameOrigin": false,
"urlIsSameOriginAsBaseUrl": false,

/* ng/controller.js */
"identifierForController": false,
Expand Down
2 changes: 1 addition & 1 deletion src/ng/sce.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
72 changes: 61 additions & 11 deletions src/ng/urlUtils.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
// service.
var urlParsingNode = window.document.createElement('a');
var originUrl = urlResolve(window.location.href);
var baseUrlParsingNode;


/**
Expand Down Expand Up @@ -43,16 +44,16 @@ var originUrl = urlResolve(window.location.href);
* @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) {
Expand All @@ -68,7 +69,6 @@ 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(/:$/, '') : '',
Expand All @@ -91,7 +91,57 @@ 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 (parsed.protocol === originUrl.protocol &&
parsed.host === originUrl.host);
return urlsAreSameOrigin(requestUrl, originUrl);
}

/**
* 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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can --> can be

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Done.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the same the document location --> the same as ...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

* be overriden by using the `<base>` 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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: I would rather not duplicate the code. Maybe have a base function that urlIsSameOrigin/urlIsSameOriginAsBase can delegate two (and pass the originUrl/baseUrl as parameters).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

return urlsAreSameOrigin(requestUrl, getBaseUrl());
}

/**
* Determines if two URLs share the same origin.
*
* @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) {
url1 = (isString(url1)) ? urlResolve(url1) : url1;
url2 = (isString(url2)) ? urlResolve(url2) : url2;

return (url1.protocol === url2.protocol &&
url1.host === url2.host);
}

/**
* Returns the current document base URL.
* @return {string}
*/
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;
}
1 change: 1 addition & 0 deletions test/.eslintrc.json
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,7 @@
/* urlUtils.js */
"urlResolve": false,
"urlIsSameOrigin": false,
"urlIsSameOriginAsBaseUrl": true,

/* karma */
"dump": false,
Expand Down
10 changes: 10 additions & 0 deletions test/e2e/fixtures/base-tag/index.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
<!DOCTYPE html>
<html ng-app="test">
<head>
<base href="http://www.example.com/">
<script src="http://localhost:8000/build/angular.js"></script>
<script src="http://localhost:8000/e2e/fixtures/base-tag/script.js"></script>
</head>
<body>
</body>
</html>
14 changes: 14 additions & 0 deletions test/e2e/fixtures/base-tag/script.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
'use strict';

angular.
module('test', []).
run(function($sce) {
window.isTrustedUrl = function(url) {
try {
$sce.getTrustedResourceUrl(url);
} catch (e) {
return false;
}
return true;
};
});
35 changes: 35 additions & 0 deletions test/e2e/tests/base-tag.spec.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
'use strict';

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(allowed);
}

beforeAll(function() {
loadFixture('base-tag');
});

it('allows the page URL (location.href)', function() {
checkUrl(browser.getLocationAbsUrl(), true);
});

it('blocks off-origin URLs', function() {
checkUrl('http://evil.com', false);
});

it('allows relative URLs ("/relative")', function() {
checkUrl('/relative', true);
});

it('allows absolute URLs from the base origin', function() {
checkUrl('http://www.example.com/path/to/file.html', true);
});

it('tracks changes to the base URL', 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);
});
});
32 changes: 32 additions & 0 deletions test/ng/sceSpecs.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 = window.document.createElement('BASE');
var cfg = {whitelist: ['self'], blacklist: []};
baseElem.setAttribute('href', window.location.protocol + '//foo.example.com/path/');
beforeAll(function() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there is no particular reason, I would prefer re-creating/adding/removing the <base> element for each test.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

window.document.head.appendChild(baseElem);
});
afterAll(function() {
window.document.head.removeChild(baseElem);
});
function expectAllowed($sce, url) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This helper (and expectBlocked below) seem redundant in this case. Let's remove them and inline the code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

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(
Expand Down
8 changes: 7 additions & 1 deletion test/ng/urlUtilsSpec.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down