Skip to content

Commit a5305ab

Browse files
chirayukpetebacondarwin
chirayuk
authored and
petebacondarwin
committed
feat(http): JSONP requests now require trusted resource URLs
- JSONP should require trusted resource URLs. This would be a breaking change but maybe not too onerous since same origin URLs are trusted in the default config and you can easily whitelist any 3rd party URLs you trust in one single place (your app/module config.) - fix a bug where $http can't handle $sce wrapper URLs. Closes angular#11352 Closes angular#11328
1 parent 32aa7e7 commit a5305ab

File tree

4 files changed

+50
-20
lines changed

4 files changed

+50
-20
lines changed

‎src/ng/http.js

+8-4
Original file line numberDiff line numberDiff line change
@@ -379,8 +379,8 @@ function $HttpProvider() {
379379
**/
380380
var interceptorFactories = this.interceptors = [];
381381

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

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

@@ -1249,7 +1249,11 @@ function $HttpProvider() {
12491249
cache,
12501250
cachedResp,
12511251
reqHeaders = config.headers,
1252-
url = buildUrl(config.url, config.paramSerializer(config.params));
1252+
// origUrl could be a $sce trusted URL which used a wrapper class
1253+
origUrl = config.url,
1254+
url = $sce.valueOf(origUrl);
1255+
1256+
url = buildUrl(url, config.paramSerializer(config.params));
12531257

12541258
$http.pendingRequests.push(config);
12551259
promise.then(removePendingReq, removePendingReq);
@@ -1293,7 +1297,7 @@ function $HttpProvider() {
12931297
reqHeaders[(config.xsrfHeaderName || defaults.xsrfHeaderName)] = xsrfValue;
12941298
}
12951299

1296-
$httpBackend(config.method, url, reqData, done, reqHeaders, config.timeout,
1300+
$httpBackend(config.method, (url === $sce.valueOf(origUrl) ? origUrl : url), reqData, done, reqHeaders, config.timeout,
12971301
config.withCredentials, config.responseType,
12981302
createApplyHandlers(config.eventHandlers),
12991303
createApplyHandlers(config.uploadEventHandlers));

‎src/ng/httpBackend.js

+8-3
Original file line numberDiff line numberDiff line change
@@ -49,14 +49,19 @@ function $xhrFactoryProvider() {
4949
* $httpBackend} which can be trained with responses.
5050
*/
5151
function $HttpBackendProvider() {
52-
this.$get = ['$browser', '$jsonpCallbacks', '$document', '$xhrFactory', function($browser, $jsonpCallbacks, $document, $xhrFactory) {
53-
return createHttpBackend($browser, $xhrFactory, $browser.defer, $jsonpCallbacks, $document[0]);
52+
this.$get = ['$sce', '$browser', '$jsonpCallbacks', '$document', '$xhrFactory', function($sce, $browser, $jsonpCallbacks, $document, $xhrFactory) {
53+
return createHttpBackend($sce, $browser, $xhrFactory, $browser.defer, $jsonpCallbacks, $document[0]);
5454
}];
5555
}
5656

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

6267
if (lowercase(method) === 'jsonp') {

‎test/ng/httpBackendSpec.js

+31-11
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,18 @@
11
/* global createHttpBackend: false, createMockXhr: false, MockXhr: false */
22
'use strict';
33

4-
describe('$httpBackend', function() {
4+
fdescribe('$httpBackend', function() {
55

6-
var $backend, $browser, $jsonpCallbacks,
6+
var $sce, $backend, $browser, $jsonpCallbacks,
77
xhr, fakeDocument, callback;
88

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

14+
beforeEach(inject(function($injector) {
15+
$sce = $injector.get('$sce');
1116
$browser = $injector.get('$browser');
1217

1318
fakeDocument = {
@@ -48,7 +53,7 @@ describe('$httpBackend', function() {
4853
}
4954
};
5055

51-
$backend = createHttpBackend($browser, createMockXhr, $browser.defer, $jsonpCallbacks, fakeDocument);
56+
$backend = createHttpBackend($sce, $browser, createMockXhr, $browser.defer, $jsonpCallbacks, fakeDocument);
5257
callback = jasmine.createSpy('done');
5358
}));
5459

@@ -273,7 +278,7 @@ describe('$httpBackend', function() {
273278

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

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

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

344-
$backend('JSONP', 'http://example.org/path?cb=JSON_CALLBACK', null, callback);
348+
$backend('JSONP', 'http://special.whitelisted.resource.com/path?cb=JSON_CALLBACK', null, callback);
349+
345350
expect(fakeDocument.$$scripts.length).toBe(1);
346351

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

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

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

361-
$backend('JSONP', 'http://example.org/path?cb=JSON_CALLBACK', null, callback);
366+
$backend('JSONP', 'http://special.whitelisted.resource.com/path?cb=JSON_CALLBACK', null, callback);
367+
362368
expect(fakeDocument.$$scripts.length).toBe(1);
363369

364370

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

376382
it('should set url to current location if not specified or empty string', function() {
377383
$backend('JSONP', undefined, null, callback);
384+
378385
expect(fakeDocument.$$scripts[0].src).toBe($browser.url());
379386
fakeDocument.$$scripts.shift();
380387

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

393-
$backend('JSONP', 'http://example.org/path?cb=JSON_CALLBACK', null, callback, null, 2000);
400+
$backend('JSONP', 'http://special.whitelisted.resource.com/path?cb=JSON_CALLBACK', null, callback, null, 2000);
401+
394402
expect(fakeDocument.$$scripts.length).toBe(1);
395403
expect($browser.deferredFns[0].time).toBe(2000);
396404

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

407415

416+
it('should throw error if the url is not a trusted resource', function() {
417+
expect(function() {
418+
$backend('JSONP', 'http://example.org/path?cb=JSON_CALLBACK', null, callback);
419+
}).toThrowMinErr('$sce', 'insecurl');
420+
});
421+
422+
it('should not throw error if the url is an explicitly trusted resource', function() {
423+
expect(function() {
424+
$backend('JSONP', $sce.trustAsResourceUrl('http://example.org/path?cb=JSON_CALLBACK'), null, callback);
425+
}).not.toThrowMinErr('$sce', 'insecurl');
426+
});
427+
408428
// TODO(vojta): test whether it fires "async-start"
409429
// TODO(vojta): test whether it fires "async-end" on both success and error
410430
});
@@ -420,7 +440,7 @@ describe('$httpBackend', function() {
420440
}
421441

422442
beforeEach(function() {
423-
$backend = createHttpBackend($browser, createMockXhr);
443+
$backend = createHttpBackend($sce, $browser, createMockXhr);
424444
});
425445

426446

‎test/ngRoute/routeSpec.js

+3-2
Original file line numberDiff line numberDiff line change
@@ -1016,9 +1016,10 @@ describe('$route', function() {
10161016
$routeProvider = _$routeProvider_;
10171017

10181018
$provide.decorator('$sce', function($delegate) {
1019+
function getVal(v) { return v.getVal ? v.getVal() : v; }
10191020
$delegate.trustAsResourceUrl = function(url) { return new MySafeResourceUrl(url); };
1020-
$delegate.getTrustedResourceUrl = function(v) { return v.getVal(); };
1021-
$delegate.valueOf = function(v) { return v.getVal(); };
1021+
$delegate.getTrustedResourceUrl = function(v) { return getVal(v); };
1022+
$delegate.valueOf = function(v) { return getVal(v); };
10221023
return $delegate;
10231024
});
10241025
});

0 commit comments

Comments
 (0)