feat($httpBackend): JSONP requests now require trusted resource #15161

petebacondarwin · 2016-09-20T09:04:59Z

This PR replaces #15143 which is too complicated.

Please check if the PR fulfills these requirements

The commit message follows our guidelines: https://github.com/angular/angular.js/blob/master/CONTRIBUTING.md#commit-message-format
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

mprobst

LGTM

mprobst · 2016-09-20T09:23:38Z

src/ng/http.js

@@ -1088,7 +1094,12 @@ function $HttpProvider() {
        }

        // send request
-        return sendReq(config, reqData).then(transformResponse, transformResponse);
+        try {
+          var promise = sendReq(config, reqData);


nit: I find it a bit more readable to define vars in the block scope that they are used in, even if they are function scoped. That is, move the declaration of var promise up one line?

petebacondarwin · 2016-09-20T09:38:34Z

If it is possible to add a parameter with both user generated key and value then it would be possible to completely control the browser when the JSONP response returns, assuming that the server does not do any sanitizing of its callback parameter value.

Given that every JSONP endpoint could define its own way of specifying the callback, i.e. it is not necessarily a param with key callback then it is very hard to generally constrain the param at the client.

It seems to me that in JSONP requests it is more likely that an app developer would inadvertently allow user generated content in the params than to allow user generated content in the URL itself (which is what we are targeting in this PR).

Perhaps we could change the $http JSONP to require that the callback parameter key is specified in the method call. We could then delete any user provided parameters that have this key before we attach our own callback parameter internally? This would remove the need for the clunky JSONP_CALLBACK pattern match and lock out the param attack vector.

An example API would look like:

var promise = $http({
  method: 'JSONP',
  url: $sce.trustAsResourceUrl('https://trusted.com/path'),
  callbackKey: 'callback'
});

Without the callbackKey the call would fail. This param could be specified in the $http.defaults.jsonp.callbackKey='callback' to save it needing to be added to every request.

petebacondarwin · 2016-09-20T09:42:19Z

Of course this puts the onus on the app developer to remember to specify the correct callback param key, since if it doesn't match then the real callback param would still be vulnerable.

mprobst · 2016-09-20T09:45:51Z

That sounds reasonable, and arguably like a nicer API than JSONP_CALLBACK. But wouldn't this cause quite some migration pain?

petebacondarwin · 2016-09-20T09:50:40Z

If we setup the default callback param key to be callback, which is very much the defacto standard for JSONP, then there would be no breaking change... All the current JSONP calls that look like:

$http.jsonp('some/trusted/url', { callback: 'JSONP_CALLBACK' });

or

$http.jsonp('some/trusted/url?callback=JSONP_CALLBACK');

would simply have that callback deleted before the service adds it back in itself.

We could even log a warning message if we find a param with the callback key name...

petebacondarwin · 2016-09-20T09:54:13Z

The problem comes in the case where the callback key is not callback:

$http.jsonp('some/trusted/url', { cb: 'JSONP_CALLBACK' });

In this case, we would not automatically delete the cb parameter, since by default we would be looking for a callback param.

We could parse the params ones that have a value of JSONP_CALLBACK and log a warning again in that case, that some migration needs to be done.

There might be some rare case where callback is a valid param name for the JSONP call, while the expected callback param key is something else, (e.g. cb). In this case we would delete a valid parameter causing the request to fail in some manner.

gkalpak

Basically LGTM. It's obviously not a full-proof solution, but it is better that what we have now.

gkalpak · 2016-09-20T11:41:33Z

src/ng/http.js

+      if (lowercase(config.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);


So, if I understand correctly, config.url must be a string for all non-JSONP requests (i.e. it can't be an $sce-wrapped value - not that it needs to). And for JSONP requests it can be either an $sce-trusted resourceUrl or matched by a rule in the resourceUrlWhiteList (but not in the blackList). Right?

It might be more staight-forward if we allowed wrapped values for other methods too. E.g.:

if (... === 'jsonp') { ... } else { url = $sce.valueOf(url); }

+1: accepting safe types where they make sense but aren't technically needed is a good thing.

Sadly there is no $sce.valueOf method but I will implement this suggestion.

There is: https://github.com/angular/angular.js/blob/master/src/ng/sce.js#L389.

(it's just weirdly undocumented ?)

gkalpak · 2016-09-20T11:43:56Z

test/ng/httpSpec.js

+        }).not.toThrow();
+      });
+
+      it('jsonp() should allow trusted url', inject(['$sce', function($sce) {


This description is misleading.

rjamet · 2016-09-20T12:47:06Z

LGTM too, both for the current and the discussed modified version. It's still JSONP, so it's still going to result in bugs, but at least the framework does what it can that way. Thanks a lot !

petebacondarwin · 2016-09-20T20:12:58Z

I've added the callback parameter checking feature in 308f3bf.
@mprobst, @rjamet, @gkalpak and @IgorMinar - PTAL

Narretz · 2016-09-21T08:49:32Z

src/ng/http.js

+      var callbackParamRegex = new RegExp('([&?]' + key + '=)');
+      if (callbackParamRegex.test(url)) {
+        // Throw if the callback param was already provided
+        throw $httpMinErr('badjsonp', 'Illegal use of callback param, "{0}", in url, "{1}"', key, url);


Is it necessary to throw here? Couldn't we just remove the superfluous param?

The aim of throwing is to a) make it more obvious where something has been missed in a migration and b) to indicate that a malicious attacker is trying to inject their own callback.

But perhaps this is a bit harsh?

I think we should go with the harsh BC of throwing and always rollback to just ignoring in a patch release if we get lots of complaints.

Narretz · 2016-09-21T08:59:53Z

Some change suggestions to the commit message:

feat($http): specify the JSONP callback via the callbackParam config value

The query parameter that will be used to transmit the JSONP callback to the
server is now specified via the callbackParam config value of the $httpProvider, instead of
using the JSON_CALLBACK placeholder. instead of adding a query param to the url that has the JSON_CALLBACK placeholder value?

Any use of JSON_CALLBACK in a JSONP request URL will cause an error.
Any request that provides a parameter with the same name as that given
by the callbackParam config property will cause an error.

This is to prevent malicious attack via the response from an app inadvertently
allowing untrusted data to be used to generate the callback parameter.

BREAKING CHANGE

You can no longer use the JSON_CALLBACK placeholder in your JSONP requests.
Instead you must provide the name of the query parameter that will pass the
callback via the callbackParam property of the config object, or app-wide via
the $http.defaults.callbackParam property, which is callback by default.

Before this change:

// Default callback param
$http.jsonp('trusted/url?callback=JSON_CALLBACK');
// Custom callback param
$http.jsonp('other/trusted/url', {params:cb:'JSON_CALLBACK'});

After this change:

// Default callback param is taken from the provider config, no need to add it
$http.jsonp('trusted/url');
// Custom callback param must be added via http config option
$http.jsonp('other/trusted/url', {callbackParam:'cb'});
...
// Or set a custom callback param via httpProvider config
$httpProvider.callbackParam = 'cb';

Narretz · 2016-09-21T08:52:39Z

src/ng/http.js

-    paramSerializer: '$httpParamSerializer'
+    paramSerializer: '$httpParamSerializer',
+
+    callbackParam: 'callback'


I think this should be jsonpCallbackParam to make it explicit, because httpProvider is bundling a bunch of different config options, and the bulk of the jsonp customization is actually part of the $jsonpCallbacks service

Done in 022fc44

Narretz · 2016-09-21T09:08:21Z

The Travis failure is real, and is caused by the jsonp e2e tests in the docs

petebacondarwin · 2016-09-21T09:16:48Z

@Narretz I fixed the e2e test that was failing in 9be46a7

petebacondarwin · 2016-09-21T09:24:28Z

@Narretz regarding the changes to the commit message. I agree with adding the code comments to the examples.

In the earlier description though:

The query parameter that will be used to transmit the JSONP callback to the
server is now specified via the callbackParam config value of the $httpProvider, instead of
using the JSON_CALLBACK placeholder. instead of adding a query param to the url that has the JSON_CALLBACK placeholder value?

I mostly disagree - because it was possible before that the callback was passed in some other form, such as part of the URL (e.g. https://some-domain.org/end/point/JSON_CALLBACK). I know that this probably never happened but it was possible and we supported it. Now we will indeed only support a parameter. If you have to deal with the breaking change then you will already understand what the purpose and use of JSON_CALLBACK was so I don't think that the way I described it was confusing.

gkalpak · 2016-09-21T13:18:36Z

test/ng/httpSpec.js

+      expect(function() {
+        $httpBackend.expect('GET', '/url').respond('');
+        $http({url: $sce.trustAsResourceUrl('/url')});
+      }).not.toThrowMinErr('$http','badreq', 'Http request configuration url must be a string.  Received: false');


This assertion is too weak (because a negation of a too specific assertion).
Better use the stricter assertion possible. E.g. in order of preference:

.not.toThrow()

.not.toThrow('$http')

.not.toThrow('$http', 'badreq')

Yes, OK, OK. In fact even better is to remove the not.toThrow altogether!

gkalpak · 2016-09-21T13:37:59Z

docs/content/error/$http/badjsonp.ngdoc

+This error occurs when the URL generated from the configuration object contains a parameter with the same name as the configured `jsonpCallbackParam`
+property; or when it contains a parameter whose value is `JSON_CALLBACK`.
+
+`$http` JSON requests need to attach a callback query parameter to the URL. The name of this parameter is specified in the configuration


JSON --.> JSONP

gkalpak · 2016-09-21T13:40:37Z

src/ng/http.js

@@ -286,6 +286,10 @@ function $HttpProvider() {
   *  If specified as string, it is interpreted as a function registered with the {@link auto.$injector $injector}.
   *  Defaults to {@link ng.$httpParamSerializer $httpParamSerializer}.
   *
+   * - **`defaults.jsonpCallbackParam`** - `{string} - the name of the query parameter that passes the callback in a JSONP
+   * request. The value of this parameter will be replaced with the expression generated by the {@link jsonpCallbacks}


Does this link work? Shouldn't it be $jsonpCallbacks or something?

gkalpak · 2016-09-21T13:44:47Z

src/ng/http.js

@@ -1146,11 +1168,34 @@ function $HttpProvider() {
     *
     * @description
     * Shortcut method to perform `JSONP` request.
+     *
+     * Note that, since JSONP requests are sensitive because the response is given full acces to the browser,


acces --> access

gkalpak · 2016-09-21T13:45:22Z

src/ng/http.js

+     * the url must be declared, via {@link $sce} as a trusted resource URL.
+     * You can trust a URL by adding it to the whitelist via
+     * {@link $sceDelegateProvider#resourceUrlWhitelist  `$sceDelegateProvider.resourceUrlWhitelist`} or
+     * by explicitly trusted the URL via {@link $sce#trustAsResourceUrl `$sce.trustAsResourceUrl(url)`}.


trusted --> trusting

gkalpak · 2016-09-21T13:45:41Z

src/ng/http.js

+     * by explicitly trusted the URL via {@link $sce#trustAsResourceUrl `$sce.trustAsResourceUrl(url)`}.
+     *
+     * JSONP requests must specify a callback to be used in the response from the server. This callback
+     * is passed as as a query parameter in the request. You must specify the name of this parameter by


as as --> as

gkalpak · 2016-09-21T13:46:34Z

src/ng/http.js

+     * ```
+     *
+     * You can also specify a global callback parameter key in `$http.defaults.jsonpCallbackParam`.
+     * By default this is set to `callback`.


(callback --> 'callback')

gkalpak · 2016-09-21T13:46:58Z

src/ng/http.js

+     * ```
+     *
+     * You can also specify a global callback parameter key in `$http.defaults.jsonpCallbackParam`.
+     * By default this is set to `callback`.


(callback --> 'callback')

gkalpak · 2016-09-21T13:52:09Z

src/ng/http.js

+        throw $httpMinErr('badjsonp', 'Illegal use of JSON_CALLBACK in url, "{0}"', url);
+      }
+
+      var callbackParamRegex = new RegExp('([&?]' + key + '=)');


Why the parenthesis?

Because I was originally using it in a replace. Good spot.

gkalpak

LGTM (other than a couple of nitpicks and the occasional >100chars-long lines 😛)

gkalpak · 2016-10-04T08:43:38Z

docs/content/error/$http/badjsonp.ngdoc

+@fullName Bad JSONP Request Configuration
+@description
+
+This error occurs when the URL generated from the configuration object contains a parameter with the same name as the configured `jsonpCallbackParam`


Nit: Wrapping lines at 100 chars is sooo cool 😁

gkalpak · 2016-10-04T08:56:32Z

src/ng/http.js

@@ -286,6 +286,10 @@ function $HttpProvider() {
   *  If specified as string, it is interpreted as a function registered with the {@link auto.$injector $injector}.
   *  Defaults to {@link ng.$httpParamSerializer $httpParamSerializer}.
   *
+   * - **`defaults.jsonpCallbackParam`** - `{string} - the name of the query parameter that passes the callback in a JSONP


Maybe "that passes the name of the callback" would be more clear.

gkalpak · 2016-10-04T08:59:42Z

src/ng/http.js

+        try {
+          return sendReq(config, reqData).then(transformResponse, transformResponse);
+        } catch (e) {
+          return $q.reject(e);


This won't be necessary if #15213 lands. Maybe remove it and wait for #15213 or add a TODO note to remove it later.

I'll wait. I have reviewed #15213 - I expect we can land that today

Actually this is needed. We are not trying to catch exceptions from the then handlers, we are catching exceptions thrown synchronously inside sendReq. E.g. url = $sce.getTrustedResourceUrl(url); can throw.

This is inside serverRequest, which is passed as an "onFulfilled" callback to promise: promise.then(serverRequest).
So, even synchronous errors in sendReq() will be converted to rejections.

Oh OK. I'll remove it after 15213 then

gkalpak · 2016-10-04T09:02:47Z

src/ng/http.js

+     * $http.jsonp('some/trusted/url', {jsonpCallbackParam: 'callback'})
+     * ```
+     *
+     * You can also specify a global callback parameter key in `$http.defaults.jsonpCallbackParam`.


global sounds a little confusing to me. How about default?

how about "application-wide"?

default is OK.

gkalpak

LGTM

The mock calls to `valueOf(v)` and `getTrustedResourceUrl(v)` were not dealing with the case where `v` was null.

The $http service will reject JSONP requests that are not trusted by `$sce` as "ResourceUrl". This change makes is easier for developers to see clearly where in their code they are making JSONP calls that may be to untrusted endpoings and forces them to think about how these URLs are generated. Be aware that this commit does not put any constraint on the parameters that will be appended to the URL. Developers should be mindful of what parameters can be attached and how they are generated. Closes angular#11352 BREAKING CHANGE All JSONP requests now require the URL to be trusted as resource URLs. There are two approaches to trust a URL: **Whitelisting with the `$sceDelegateProvider.resourceUrlWhitelist()` method.** You configure this list in a module configuration block: ``` appModule.config(['$sceDelegateProvider', function($sceDelegateProvider) { $sceDelegateProvider.resourceUrlWhiteList([ // Allow same origin resource loads. 'self', // Allow JSONP calls that match this pattern 'https://some.dataserver.com/**.jsonp?**` ]); }]); ``` **Explicitly trusting the URL via the `$sce.trustAsResourceUrl(url)` method** You can pass a trusted object instead of a string as a URL to the `$http` service: ``` var promise = $http.jsonp($sce.trustAsResourceUrl(url)); ```

… config The query parameter that will be used to transmit the JSONP callback to the server is now specified via the `jsonpCallbackParam` config value, instead of using the `JSON_CALLBACK` placeholder. * Any use of `JSON_CALLBACK` in a JSONP request URL will cause an error. * Any request that provides a parameter with the same name as that given by the `jsonpCallbackParam` config property will cause an error. This is to prevent malicious attack via the response from an app inadvertently allowing untrusted data to be used to generate the callback parameter. BREAKING CHANGE You can no longer use the `JSON_CALLBACK` placeholder in your JSONP requests. Instead you must provide the name of the query parameter that will pass the callback via the `jsonpCallbackParam` property of the config object, or app-wide via the `$http.defaults.jsonpCallbackParam` property, which is `"callback"` by default. Before this change: ``` $http.json('trusted/url?callback=JSON_CALLBACK'); $http.json('other/trusted/url', {params:cb:'JSON_CALLBACK'}); ``` After this change: ``` $http.json('trusted/url'); $http.json('other/trusted/url', {callbackParam:'cb'}); ```

… config The query parameter that will be used to transmit the JSONP callback to the server is now specified via the `jsonpCallbackParam` config value, instead of using the `JSON_CALLBACK` placeholder. * Any use of `JSON_CALLBACK` in a JSONP request URL will cause an error. * Any request that provides a parameter with the same name as that given by the `jsonpCallbackParam` config property will cause an error. This is to prevent malicious attack via the response from an app inadvertently allowing untrusted data to be used to generate the callback parameter. Closes angular#15161 Closes angular#15143 Closes angular#11352 Closes angular#11328 BREAKING CHANGE You can no longer use the `JSON_CALLBACK` placeholder in your JSONP requests. Instead you must provide the name of the query parameter that will pass the callback via the `jsonpCallbackParam` property of the config object, or app-wide via the `$http.defaults.jsonpCallbackParam` property, which is `"callback"` by default. Before this change: ``` $http.json('trusted/url?callback=JSON_CALLBACK'); $http.json('other/trusted/url', {params:cb:'JSON_CALLBACK'}); ``` After this change: ``` $http.json('trusted/url'); $http.json('other/trusted/url', {callbackParam:'cb'}); ```

…ed types Follows angular/angular.js#15161 ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=152795936

petebacondarwin added type: feature component: $http needs: review severity: security branch: 1.6.x labels Sep 20, 2016

petebacondarwin added this to the 1.6.0-rc.1 milestone Sep 20, 2016

petebacondarwin self-assigned this Sep 20, 2016

googlebot added the cla: yes label Sep 20, 2016

petebacondarwin mentioned this pull request Sep 20, 2016

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

Closed

mprobst approved these changes Sep 20, 2016

View reviewed changes

gkalpak reviewed Sep 20, 2016

View reviewed changes

Narretz reviewed Sep 21, 2016

View reviewed changes

Narretz suggested changes Sep 21, 2016

View reviewed changes

gkalpak reviewed Sep 21, 2016

View reviewed changes

petebacondarwin force-pushed the pr-11623a branch from 25d72c4 to a69729d Compare October 3, 2016 19:16

gkalpak reviewed Oct 4, 2016

View reviewed changes

gkalpak approved these changes Oct 5, 2016

View reviewed changes

petebacondarwin added 2 commits October 5, 2016 13:05

test($route): ensure mock $sce delegate is implemented correctly

9d08b33

The mock calls to `valueOf(v)` and `getTrustedResourceUrl(v)` were not dealing with the case where `v` was null.

petebacondarwin force-pushed the pr-11623a branch from cb2db20 to 3c1cd3b Compare October 5, 2016 13:30

petebacondarwin closed this in fb66341 Oct 5, 2016

This was referenced Mar 18, 2021

WS-2016-7105 (Medium) detected in angularjs-1.4.3.jar tamirverthim/fitbit-api-example-java#140

Open

WS-2016-7105 (Medium) detected in angularjs-1.4.3.jar Nehamaefi/fitbit-api-example-java#146

Open

mend-bolt-for-github bot mentioned this pull request Mar 18, 2021

WS-2016-7105 (Medium) detected in angularjs-1.4.3.jar mibo32/fitbit-api-example-java#133

Open

mend-bolt-for-github bot mentioned this pull request May 24, 2021

WS-2016-7105 (Medium) detected in angularjs-1.4.3.jar william31212/fitbit-api-example-java2#98

Open

mend-bolt-for-github bot mentioned this pull request Jun 19, 2021

WS-2016-7105 (Medium) detected in angularjs-1.4.3.jar bharathirajatut/fitbit-api-example-java2#98

Open

mend-bolt-for-github bot mentioned this pull request Mar 20, 2025

WS-2016-7105 (Medium) detected in angularjs-1.4.9.jar MidnightBSD/midnightbsd-app-store#398

Open

feat($httpBackend): JSONP requests now require trusted resource #15161

feat($httpBackend): JSONP requests now require trusted resource #15161

Conversation

petebacondarwin commented Sep 20, 2016

mprobst left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petebacondarwin commented Sep 20, 2016 • edited Loading

petebacondarwin commented Sep 20, 2016

mprobst commented Sep 20, 2016 via email

petebacondarwin commented Sep 20, 2016 • edited Loading

petebacondarwin commented Sep 20, 2016

gkalpak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rjamet Sep 20, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rjamet commented Sep 20, 2016

petebacondarwin commented Sep 20, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Narretz commented Sep 21, 2016 • edited Loading

Narretz Sep 21, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petebacondarwin Sep 21, 2016 • edited Loading

Choose a reason for hiding this comment

Narretz commented Sep 21, 2016

petebacondarwin commented Sep 21, 2016 • edited Loading

petebacondarwin commented Sep 21, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkalpak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkalpak left a comment

Choose a reason for hiding this comment

petebacondarwin commented Sep 20, 2016 •

edited

Loading

petebacondarwin commented Sep 20, 2016 •

edited

Loading

rjamet Sep 20, 2016 •

edited

Loading

Narretz commented Sep 21, 2016 •

edited

Loading

Narretz Sep 21, 2016 •

edited

Loading

petebacondarwin Sep 21, 2016 •

edited

Loading

petebacondarwin commented Sep 21, 2016 •

edited

Loading