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

sec(http): JSONP should require trusted resource URLs #11623

Closed
wants to merge 1 commit into from
Closed

sec(http): JSONP should require trusted resource URLs #11623

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions src/ng/http.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1110,7 +1110,11 @@ function $HttpProvider() {
cache,
cachedResp,
reqHeaders = config.headers,
url = buildUrl(config.url, config.paramSerializer(config.params));
// origUrl could be a $sce trusted URL which used a wrapper class
origUrl = config.url,
url = $sce.valueOf(origUrl);

url = buildUrl(url, config.paramSerializer(config.params));

$http.pendingRequests.push(config);
promise.then(removePendingReq, removePendingReq);
Expand Down Expand Up @@ -1154,7 +1158,7 @@ function $HttpProvider() {
reqHeaders[(config.xsrfHeaderName || defaults.xsrfHeaderName)] = xsrfValue;
}

$httpBackend(config.method, url, reqData, done, reqHeaders, config.timeout,
$httpBackend(config.method, (url === $sce.valueOf(origUrl) ? origUrl : url), reqData, done, reqHeaders, config.timeout,
Copy link
Contributor

Choose a reason for hiding this comment

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

is this necessary? why not just url = $sce.valueOf(origUrl);

Copy link
Contributor

Choose a reason for hiding this comment

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

This is related to #11328

config.withCredentials, config.responseType);
}

Expand Down
14 changes: 10 additions & 4 deletions src/ng/httpBackend.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,14 +21,20 @@ function createXhr() {
* $httpBackend} which can be trained with responses.
*/
function $HttpBackendProvider() {
this.$get = ['$browser', '$window', '$document', function($browser, $window, $document) {
return createHttpBackend($browser, createXhr, $browser.defer, $window.angular.callbacks, $document[0]);
this.$get = ['$sce', '$browser', '$window', '$document', function($sce, $browser, $window, $document) {
return createHttpBackend($sce, $browser, createXhr, $browser.defer, $window.angular.callbacks, $document[0]);
}];
}

function createHttpBackend($browser, createXhr, $browserDefer, callbacks, rawDocument) {
function createHttpBackend($sce, $browser, createXhr, $browserDefer, callbacks, rawDocument) {
// TODO(vojta): fix the signature
return function(method, url, post, callback, headers, timeout, withCredentials, responseType) {
if (lowercase(method) == 'jsonp') {
// This is a pretty sensitive operation where we're allowing a script to have full access to
// our DOM and JS space. So we require that the URL satisfies SCE.RESOURCE_URL.
url = $sce.getTrustedResourceUrl(url);
}

$browser.$$incOutstandingRequestCount();
url = url || $browser.url();

Expand Down Expand Up @@ -142,8 +148,8 @@ function createHttpBackend($browser, createXhr, $browserDefer, callbacks, rawDoc
// - adds and immediately removes script elements from the document
var script = rawDocument.createElement('script'), callback = null;
script.type = "text/javascript";
script.src = url;
script.async = true;
script.src = url;
Copy link
Contributor

Choose a reason for hiding this comment

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

phantom edit?


callback = function(event) {
removeEventListenerFn(script, "load", callback);
Expand Down
6 changes: 3 additions & 3 deletions src/ngMock/angular-mocks.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1095,7 +1095,7 @@ angular.mock.dump = function(object) {
```
*/
angular.mock.$HttpBackendProvider = function() {
this.$get = ['$rootScope', '$timeout', createHttpBackendMock];
this.$get = ['$sce', '$rootScope', '$timeout', createHttpBackendMock];
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see why the changes in this file are needed...

};

/**
Expand All @@ -1112,7 +1112,7 @@ angular.mock.$HttpBackendProvider = function() {
* @param {Object=} $browser Auto-flushing enabled if specified
* @return {Object} Instance of $httpBackend mock
*/
function createHttpBackendMock($rootScope, $timeout, $delegate, $browser) {
function createHttpBackendMock($sce, $rootScope, $timeout, $delegate, $browser) {
var definitions = [],
expectations = [],
responses = [],
Expand Down Expand Up @@ -2092,7 +2092,7 @@ angular.module('ngMockE2E', ['ng']).config(['$provide', function($provide) {
*/
angular.mock.e2e = {};
angular.mock.e2e.$httpBackendDecorator =
['$rootScope', '$timeout', '$delegate', '$browser', createHttpBackendMock];
['$sce', '$rootScope', '$timeout', '$delegate', '$browser', createHttpBackendMock];


/**
Expand Down
54 changes: 39 additions & 15 deletions test/ng/httpBackendSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,13 @@

describe('$httpBackend', function() {

var $backend, $browser, callbacks,
var $sce, $backend, $browser, callbacks,
xhr, fakeDocument, callback;


beforeEach(inject(function($injector) {
callbacks = {counter: 0};
$sce = $injector.get('$sce');
$browser = $injector.get('$browser');
fakeDocument = {
$$scripts: [],
Expand All @@ -28,7 +29,7 @@ describe('$httpBackend', function() {
})
}
};
$backend = createHttpBackend($browser, createMockXhr, $browser.defer, callbacks, fakeDocument);
$backend = createHttpBackend($sce, $browser, createMockXhr, $browser.defer, callbacks, fakeDocument);
callback = jasmine.createSpy('done');
}));

Expand Down Expand Up @@ -255,7 +256,22 @@ describe('$httpBackend', function() {
});


describe('JSONP', function() {
[true, false].forEach(function(trustAsResourceUrl) {
Copy link
Member

Choose a reason for hiding this comment

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

It seems like a good place for the they() helper :)

Copy link
Member

Choose a reason for hiding this comment

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

Or maybe not (since it's on the describe level, not the it level)...

describe('JSONP: trustAsResourceUrl='+trustAsResourceUrl, function() {


function $backendCall() {
var args = Array.prototype.slice.call(arguments);
Copy link
Member

Choose a reason for hiding this comment

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

There is also the sliceArgs() function. (Just saying...)

var url = args[1];
if (trustAsResourceUrl || url == null) {
args[1] = $sce.trustAsResourceUrl(url);
return $backend.apply(null, args);
} else {
var badUrlPrefix = url ? url.replace('JSON_CALLBACK', '') : '';
expect(function() { $backend.apply(null, args); }).toThrowMinErr(
'$sce', 'insecurl', 'Blocked loading resource from url not allowed by $sceDelegate policy. URL: ' + badUrlPrefix);
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

There is too much work going on here for a test. We should just duplicate what is needed so that we can easily read the tests.


var SCRIPT_URL = /([^\?]*)\?cb=angular\.callbacks\.(.*)/;

Expand All @@ -266,7 +282,9 @@ describe('$httpBackend', function() {
expect(response).toBe('some-data');
});

$backend('JSONP', 'http://example.org/path?cb=JSON_CALLBACK', null, callback);
$backendCall('JSONP', 'http://example.org/path?cb=JSON_CALLBACK', null, callback);
if (!trustAsResourceUrl) return;

expect(fakeDocument.$$scripts.length).toBe(1);

var script = fakeDocument.$$scripts.shift(),
Expand All @@ -281,7 +299,9 @@ describe('$httpBackend', function() {


it('should clean up the callback and remove the script', function() {
$backend('JSONP', 'http://example.org/path?cb=JSON_CALLBACK', null, callback);
$backendCall('JSONP', 'http://example.org/path?cb=JSON_CALLBACK', null, callback);
if (!trustAsResourceUrl) return;

expect(fakeDocument.$$scripts.length).toBe(1);


Expand All @@ -297,11 +317,13 @@ describe('$httpBackend', function() {


it('should set url to current location if not specified or empty string', function() {
$backend('JSONP', undefined, null, callback);
$backendCall('JSONP', undefined, null, callback);
if (!trustAsResourceUrl) return;

expect(fakeDocument.$$scripts[0].src).toBe($browser.url());
fakeDocument.$$scripts.shift();

$backend('JSONP', '', null, callback);
$backendCall('JSONP', '', null, callback);
expect(fakeDocument.$$scripts[0].src).toBe($browser.url());
});

Expand All @@ -311,7 +333,9 @@ describe('$httpBackend', function() {
expect(status).toBe(-1);
});

$backend('JSONP', 'http://example.org/path?cb=JSON_CALLBACK', null, callback, null, 2000);
$backendCall('JSONP', 'http://example.org/path?cb=JSON_CALLBACK', null, callback, null, 2000);
if (!trustAsResourceUrl) return;

expect(fakeDocument.$$scripts.length).toBe(1);
expect($browser.deferredFns[0].time).toBe(2000);

Expand All @@ -328,7 +352,7 @@ describe('$httpBackend', function() {

// TODO(vojta): test whether it fires "async-start"
// TODO(vojta): test whether it fires "async-end" on both success and error
});
})});

describe('protocols that return 0 status code', function() {

Expand All @@ -341,7 +365,7 @@ describe('$httpBackend', function() {


it('should convert 0 to 200 if content and file protocol', function() {
$backend = createHttpBackend($browser, createMockXhr);
$backend = createHttpBackend($sce, $browser, createMockXhr);

$backend('GET', 'file:///whatever/index.html', null, callback);
respond(0, 'SOME CONTENT');
Expand All @@ -351,7 +375,7 @@ describe('$httpBackend', function() {
});

it('should convert 0 to 200 if content for protocols other than file', function() {
$backend = createHttpBackend($browser, createMockXhr);
$backend = createHttpBackend($sce, $browser, createMockXhr);

$backend('GET', 'someProtocol:///whatever/index.html', null, callback);
respond(0, 'SOME CONTENT');
Expand All @@ -361,7 +385,7 @@ describe('$httpBackend', function() {
});

it('should convert 0 to 404 if no content and file protocol', function() {
$backend = createHttpBackend($browser, createMockXhr);
$backend = createHttpBackend($sce, $browser, createMockXhr);

$backend('GET', 'file:///whatever/index.html', null, callback);
respond(0, '');
Expand All @@ -371,7 +395,7 @@ describe('$httpBackend', function() {
});

it('should not convert 0 to 404 if no content for protocols other than file', function() {
$backend = createHttpBackend($browser, createMockXhr);
$backend = createHttpBackend($sce, $browser, createMockXhr);

$backend('GET', 'someProtocol:///whatever/index.html', null, callback);
respond(0, '');
Expand Down Expand Up @@ -399,7 +423,7 @@ describe('$httpBackend', function() {

try {

$backend = createHttpBackend($browser, createMockXhr);
$backend = createHttpBackend($sce, $browser, createMockXhr);

$backend('GET', '/whatever/index.html', null, callback);
respond(0, '');
Expand All @@ -414,7 +438,7 @@ describe('$httpBackend', function() {


it('should return original backend status code if different from 0', function() {
$backend = createHttpBackend($browser, createMockXhr);
$backend = createHttpBackend($sce, $browser, createMockXhr);

// request to http://
$backend('POST', 'http://rest_api/create_whatever', null, callback);
Expand Down