From 9274acf06ca7bff6a38238007ee95b660b6f280b Mon Sep 17 00:00:00 2001 From: Chirayu Krishnappa Date: Wed, 15 Apr 2015 17:00:08 -0700 Subject: [PATCH] sec(http): JSONP should require trusted resource URLs WORK-IN-PROGRESS. Do **NOT** merge. More work needs to be done and the tests are currently broken. - 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 #11352 Closes #11328 --- src/ng/http.js | 8 ++++-- src/ng/httpBackend.js | 14 +++++++--- src/ngMock/angular-mocks.js | 6 ++--- test/ng/httpBackendSpec.js | 54 ++++++++++++++++++++++++++----------- 4 files changed, 58 insertions(+), 24 deletions(-) diff --git a/src/ng/http.js b/src/ng/http.js index 1b295f9908b7..625df3af5bd8 100644 --- a/src/ng/http.js +++ b/src/ng/http.js @@ -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); @@ -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, config.withCredentials, config.responseType); } diff --git a/src/ng/httpBackend.js b/src/ng/httpBackend.js index 4eb872cd5f11..6c86a67d2a33 100644 --- a/src/ng/httpBackend.js +++ b/src/ng/httpBackend.js @@ -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(); @@ -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; callback = function(event) { removeEventListenerFn(script, "load", callback); diff --git a/src/ngMock/angular-mocks.js b/src/ngMock/angular-mocks.js index dd11e45701ab..5afb8e7e3ee0 100644 --- a/src/ngMock/angular-mocks.js +++ b/src/ngMock/angular-mocks.js @@ -1095,7 +1095,7 @@ angular.mock.dump = function(object) { ``` */ angular.mock.$HttpBackendProvider = function() { - this.$get = ['$rootScope', '$timeout', createHttpBackendMock]; + this.$get = ['$sce', '$rootScope', '$timeout', createHttpBackendMock]; }; /** @@ -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 = [], @@ -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]; /** diff --git a/test/ng/httpBackendSpec.js b/test/ng/httpBackendSpec.js index 12ceed5beb01..f90189c6554a 100644 --- a/test/ng/httpBackendSpec.js +++ b/test/ng/httpBackendSpec.js @@ -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: [], @@ -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'); })); @@ -255,7 +256,22 @@ describe('$httpBackend', function() { }); - describe('JSONP', function() { + [true, false].forEach(function(trustAsResourceUrl) { + describe('JSONP: trustAsResourceUrl='+trustAsResourceUrl, function() { + + + function $backendCall() { + var args = Array.prototype.slice.call(arguments); + 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); + } + } var SCRIPT_URL = /([^\?]*)\?cb=angular\.callbacks\.(.*)/; @@ -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(), @@ -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); @@ -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()); }); @@ -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); @@ -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() { @@ -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'); @@ -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'); @@ -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, ''); @@ -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, ''); @@ -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, ''); @@ -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);