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

feat(http): JSONP requests now require trusted resource URLs #15143

Closed
wants to merge 2 commits into from
Closed

feat(http): JSONP requests now require trusted resource URLs #15143

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f5a72fc
feat($httpBackend): JSONP requests now require trusted resource URLs
chirayuk Apr 16, 2015
800f9ad
feat($http): allow explicitly trusted URLs for JSONP requests
petebacondarwin Sep 16, 2016
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
20 changes: 14 additions & 6 deletions src/ng/http.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -379,8 +379,8 @@ function $HttpProvider() {
**/
var interceptorFactories = this.interceptors = [];

this.$get = ['$browser', '$httpBackend', '$$cookieReader', '$cacheFactory', '$rootScope', '$q', '$injector',
function($browser, $httpBackend, $$cookieReader, $cacheFactory, $rootScope, $q, $injector) {
this.$get = ['$browser', '$httpBackend', '$$cookieReader', '$cacheFactory', '$rootScope', '$q', '$injector', '$sce',
function($browser, $httpBackend, $$cookieReader, $cacheFactory, $rootScope, $q, $injector, $sce) {

var defaultCache = $cacheFactory('$http');

Expand Down Expand Up @@ -881,6 +881,12 @@ function $HttpProvider() {
</file>
<file name="script.js">
angular.module('httpExample', [])
.config(['$sceDelegateProvider', function($sceDelegateProvider) {
$sceDelegateProvider.resourceUrlWhitelist([
'self',
'https://angularjs.org/**'
]);
}])
.controller('FetchController', ['$scope', '$http', '$templateCache',
function($scope, $http, $templateCache) {
$scope.method = 'GET';
Expand Down Expand Up @@ -948,7 +954,7 @@ function $HttpProvider() {
throw minErr('$http')('badreq', 'Http request configuration must be an object. Received: {0}', requestConfig);
}

if (!isString(requestConfig.url)) {
if (!isString($sce.valueOf(requestConfig.url))) {
throw minErr('$http')('badreq', 'Http request configuration url must be a string. Received: {0}', requestConfig.url);
}

Expand Down Expand Up @@ -1249,12 +1255,14 @@ function $HttpProvider() {
cache,
cachedResp,
reqHeaders = config.headers,
url = buildUrl(config.url, config.paramSerializer(config.params));
needsTrustedUrl = (lowercase(config.method) === 'jsonp'),
Copy link
Contributor

Choose a reason for hiding this comment

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

needsTrustedUrl is a little misleading, since you have a $sce.trustAsUrl function for another context :/ needsTrustedResourceUrl ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right thanks!

url = needsTrustedUrl ? $sce.getTrustedResourceUrl(config.url) : config.url;

url = buildUrl(url, config.paramSerializer(config.params));
Copy link
Member

@gkalpak gkalpak Sep 16, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

I think it is simpler if all changes inside $http are done inside buildUrl. Something like this:

 function buildUrl(url, serializedParams) {
   if (serializedParams.length > 0) {
+    url = $sce.valueOf(url); 
     url += ((url.indexOf('?') === -1) ? '?' : '&') + serializedParams;
   }
   return url;
 }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree


$http.pendingRequests.push(config);
promise.then(removePendingReq, removePendingReq);


if ((config.cache || defaults.cache) && config.cache !== false &&
(config.method === 'GET' || config.method === 'JSONP')) {
cache = isObject(config.cache) ? config.cache
Expand Down Expand Up @@ -1293,7 +1301,7 @@ function $HttpProvider() {
reqHeaders[(config.xsrfHeaderName || defaults.xsrfHeaderName)] = xsrfValue;
}

$httpBackend(config.method, url, reqData, done, reqHeaders, config.timeout,
$httpBackend(config.method, needsTrustedUrl ? $sce.trustAsResourceUrl(url) : url, reqData, done, reqHeaders, config.timeout,
Copy link
Contributor

@rjamet rjamet Sep 19, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Slight nit, this is the first time AngularJS calls a trustAs function in the core library. That's not wrong, but it's a weird pattern: how about a parameter that will disable the check below instead?

(selfish reason for that: I maintain a custom $sce that uses the Closure's implementation of safe types and disables trustAs calls, and this is going to need ad-hoc patching. That's doable, so if you'd rather keep it this way it's fine)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we are going with not securing the params then I can do a tidy refactor on this PR that removes the need for this trustAs call

config.withCredentials, config.responseType,
createApplyHandlers(config.eventHandlers),
createApplyHandlers(config.uploadEventHandlers));
Expand Down
11 changes: 7 additions & 4 deletions src/ng/httpBackend.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,17 +49,19 @@ function $xhrFactoryProvider() {
* $httpBackend} which can be trained with responses.
*/
function $HttpBackendProvider() {
this.$get = ['$browser', '$jsonpCallbacks', '$document', '$xhrFactory', function($browser, $jsonpCallbacks, $document, $xhrFactory) {
return createHttpBackend($browser, $xhrFactory, $browser.defer, $jsonpCallbacks, $document[0]);
this.$get = ['$sce', '$browser', '$jsonpCallbacks', '$document', '$xhrFactory', function($sce, $browser, $jsonpCallbacks, $document, $xhrFactory) {
return createHttpBackend($sce, $browser, $xhrFactory, $browser.defer, $jsonpCallbacks, $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, eventHandlers, uploadEventHandlers) {
url = url || $browser.url();

Copy link
Member

Choose a reason for hiding this comment

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

Why this change? Why are we implicitly allowing JSONP requests to the current URL?
I don't expect this to be a security concern, but (a) it is most probably a programming error and (b) we are duplicating the || browser.url() part unnecessarily. Did I miss something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I originally moved the trust check inside the second if clause as you suggested but it broke stuff as we need to convert a null url to the current browser url after the trust check. So now I basically moved this line to lines 64 and 74 below.

Copy link
Member

Choose a reason for hiding this comment

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

Couldn't we leave this url = url || browser.url() here and just add url = $sce.getTrustedResourceUrl(url) in the JSONP case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The url = url || ... has to go after the getTrustedResource so you are back to earlier when you suggested that we move the check inside the if statement.

Anyway, I am hoping that we can accept that the param changes are not a security issue, in which case we can refactor the whole thing out and move the checks out from the $httpBackend, which means that we don't need to do the re-wrapping etc

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.url();
Copy link
Member

Choose a reason for hiding this comment

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

So, we are basically only allowing JSONP requests to whitelisted URLs (no support for trusting a URL "on-the-fly").
I am fine with this, except that we should document it 😃

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right - documentation ....

var callbackPath = callbacks.createCallback(url);
var jsonpDone = jsonpReq(url, callbackPath, function(status, text) {
// jsonpReq only ever sets status to 200 (OK), 404 (ERROR) or -1 (WAITING)
Expand All @@ -69,6 +71,7 @@ function createHttpBackend($browser, createXhr, $browserDefer, callbacks, rawDoc
});
} else {

url = url || $browser.url();
var xhr = createXhr(method, url);

xhr.open(method, url, true);
Expand Down
7 changes: 4 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 @@ -1296,7 +1296,7 @@ angular.mock.dump = function(object) {
```
*/
angular.mock.$HttpBackendProvider = function() {
this.$get = ['$rootScope', '$timeout', createHttpBackendMock];
this.$get = ['$sce', '$rootScope', '$timeout', createHttpBackendMock];
};

/**
Expand All @@ -1313,7 +1313,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 All @@ -1337,6 +1337,7 @@ function createHttpBackendMock($rootScope, $timeout, $delegate, $browser) {
expectation = expectations[0],
wasExpected = false;

url = $sce.valueOf(url);
xhr.$$events = eventHandlers;
xhr.upload.$$events = uploadEventHandlers;

Expand Down Expand Up @@ -2666,7 +2667,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
40 changes: 30 additions & 10 deletions test/ng/httpBackendSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,16 @@

describe('$httpBackend', function() {

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

beforeEach(inject(function($injector) {
beforeEach(module(function($sceDelegateProvider) {
// Setup a special whitelisted url that we can use in testing JSONP requests
$sceDelegateProvider.resourceUrlWhitelist(['http://special.whitelisted.resource.com/**']);
}));

beforeEach(inject(function($injector) {
$sce = $injector.get('$sce');
$browser = $injector.get('$browser');

fakeDocument = {
Expand Down Expand Up @@ -48,7 +53,7 @@ describe('$httpBackend', function() {
}
};

$backend = createHttpBackend($browser, createMockXhr, $browser.defer, $jsonpCallbacks, fakeDocument);
$backend = createHttpBackend($sce, $browser, createMockXhr, $browser.defer, $jsonpCallbacks, fakeDocument);
callback = jasmine.createSpy('done');
}));

Expand Down Expand Up @@ -273,7 +278,7 @@ describe('$httpBackend', function() {

it('should call $xhrFactory with method and url', function() {
var mockXhrFactory = jasmine.createSpy('mockXhrFactory').and.callFake(createMockXhr);
$backend = createHttpBackend($browser, mockXhrFactory, $browser.defer, $jsonpCallbacks, fakeDocument);
$backend = createHttpBackend($sce, $browser, mockXhrFactory, $browser.defer, $jsonpCallbacks, fakeDocument);
$backend('GET', '/some-url', 'some-data', noop);
expect(mockXhrFactory).toHaveBeenCalledWith('GET', '/some-url');
});
Expand Down Expand Up @@ -334,20 +339,20 @@ describe('$httpBackend', function() {

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


it('should add script tag for JSONP request', function() {
callback.and.callFake(function(status, response) {
expect(status).toBe(200);
expect(response).toBe('some-data');
});

$backend('JSONP', 'http://example.org/path?cb=JSON_CALLBACK', null, callback);
$backend('JSONP', 'http://special.whitelisted.resource.com/path?cb=JSON_CALLBACK', null, callback);

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

var script = fakeDocument.$$scripts.shift(),
url = script.src.match(SCRIPT_URL);

expect(url[1]).toBe('http://example.org/path');
expect(url[1]).toBe('http://special.whitelisted.resource.com/path');
$jsonpCallbacks[url[2]]('some-data');
browserTrigger(script, 'load');

Expand All @@ -358,7 +363,8 @@ describe('$httpBackend', function() {
it('should clean up the callback and remove the script', function() {
spyOn($jsonpCallbacks, 'removeCallback').and.callThrough();

$backend('JSONP', 'http://example.org/path?cb=JSON_CALLBACK', null, callback);
$backend('JSONP', 'http://special.whitelisted.resource.com/path?cb=JSON_CALLBACK', null, callback);

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


Expand All @@ -375,6 +381,7 @@ describe('$httpBackend', function() {

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

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

Expand All @@ -390,7 +397,8 @@ describe('$httpBackend', function() {
expect(status).toBe(-1);
});

$backend('JSONP', 'http://example.org/path?cb=JSON_CALLBACK', null, callback, null, 2000);
$backend('JSONP', 'http://special.whitelisted.resource.com/path?cb=JSON_CALLBACK', null, callback, null, 2000);

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

Expand All @@ -405,6 +413,18 @@ describe('$httpBackend', function() {
});


it('should throw error if the url is not a trusted resource', function() {
expect(function() {
$backend('JSONP', 'http://example.org/path?cb=JSON_CALLBACK', null, callback);
}).toThrowMinErr('$sce', 'insecurl');
});

it('should not throw error if the url is an explicitly trusted resource', function() {
expect(function() {
$backend('JSONP', $sce.trustAsResourceUrl('http://example.org/path?cb=JSON_CALLBACK'), null, callback);
}).not.toThrow();
});

// TODO(vojta): test whether it fires "async-start"
// TODO(vojta): test whether it fires "async-end" on both success and error
});
Expand All @@ -420,7 +440,7 @@ describe('$httpBackend', function() {
}

beforeEach(function() {
$backend = createHttpBackend($browser, createMockXhr);
$backend = createHttpBackend($sce, $browser, createMockXhr);
});


Expand Down
8 changes: 8 additions & 0 deletions test/ng/httpSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1018,6 +1018,14 @@ describe('$http', function() {
$httpBackend.expect('JSONP', '/url', undefined, checkHeader('Custom', 'Header')).respond('');
$http.jsonp('/url', {headers: {'Custom': 'Header'}});
});

it('jsonp() should allow trusted url', inject(['$sce', function($sce) {
$httpBackend.expect('JSONP', '/url').respond('');
$http.jsonp($sce.trustAsResourceUrl('/url'));

$httpBackend.expect('JSONP', '/url?a=b').respond('');
$http.jsonp($sce.trustAsResourceUrl('/url'), {params: {a: 'b'}});
}]));
});


Expand Down
5 changes: 3 additions & 2 deletions test/ngRoute/routeSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1016,9 +1016,10 @@ describe('$route', function() {
$routeProvider = _$routeProvider_;

$provide.decorator('$sce', function($delegate) {
function getVal(v) { return v.getVal ? v.getVal() : v; }
$delegate.trustAsResourceUrl = function(url) { return new MySafeResourceUrl(url); };
$delegate.getTrustedResourceUrl = function(v) { return v.getVal(); };
$delegate.valueOf = function(v) { return v.getVal(); };
$delegate.getTrustedResourceUrl = function(v) { return getVal(v); };
$delegate.valueOf = function(v) { return getVal(v); };
return $delegate;
});
});
Expand Down