sec(http): JSONP should require trusted resource URLs #11623
Conversation
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 angular#11352 Closes angular#11328
| @@ -255,7 +256,22 @@ describe('$httpBackend', function() { | |||
| }); | |||
|
|
|||
|
|
|||
| describe('JSONP', function() { | |||
| [true, false].forEach(function(trustAsResourceUrl) { | |||
There was a problem hiding this comment.
It seems like a good place for the they() helper :)
There was a problem hiding this comment.
Or maybe not (since it's on the describe level, not the it level)...
|
are the ng-mock edits enough to not break application tests? |
|
@chirayuk it generally looks good ok, a few nits --- I'll leave it up to you whether you think this can land today or not. |
|
@chirayuk - the description says DO NOT MERGE - is this still accurate or are you happy that this is ready to go in now? |
|
can we move this to 1.5? Also, I don't see an easy way to define the whitelist, do we piggy-back on the existing means to define whitelist or is there no current way to whitelist urls that are ok for jsonp? |
| @@ -1095,7 +1095,7 @@ angular.mock.dump = function(object) { | |||
| ``` | |||
| */ | |||
| angular.mock.$HttpBackendProvider = function() { | |||
| this.$get = ['$rootScope', '$timeout', createHttpBackendMock]; | |||
| this.$get = ['$sce', '$rootScope', '$timeout', createHttpBackendMock]; | |||
There was a problem hiding this comment.
I don't see why the changes in this file are needed...
| expect(function() { $backend.apply(null, args); }).toThrowMinErr( | ||
| '$sce', 'insecurl', 'Blocked loading resource from url not allowed by $sceDelegate policy. URL: ' + badUrlPrefix); | ||
| } | ||
| } |
There was a problem hiding this comment.
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.
|
Closing in favour of #15143 which is a rebased and tidied up version of this. |
WORK-IN-PROGRESS. Do NOT merge. More work needs to be done and the tests are currently broken.
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.)
Closes #11352
Closes #11328