Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 3 commits into from

Conversation

@petebacondarwin
Copy link
Member

commented Sep 20, 2016

This PR replaces #15143 which is too complicated.

Please check if the PR fulfills these requirements

Copy link
Contributor

left a comment

LGTM

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

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

This comment has been minimized.

Copy link
@mprobst

mprobst Sep 20, 2016

Contributor

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?

This comment has been minimized.

Copy link
@petebacondarwin

petebacondarwin Sep 20, 2016

Author Member

Sure thing

@petebacondarwin

This comment has been minimized.

Copy link
Member Author

commented Sep 20, 2016

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

This comment has been minimized.

Copy link
Member Author

commented Sep 20, 2016

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

This comment has been minimized.

Copy link
Contributor

commented Sep 20, 2016

@petebacondarwin

This comment has been minimized.

Copy link
Member Author

commented Sep 20, 2016

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

This comment has been minimized.

Copy link
Member Author

commented Sep 20, 2016

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.

Copy link
Member

left a comment

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

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);

This comment has been minimized.

Copy link
@gkalpak

gkalpak Sep 20, 2016

Member

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);
}

This comment has been minimized.

Copy link
@rjamet

rjamet Sep 20, 2016

Contributor

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

This comment has been minimized.

Copy link
@petebacondarwin

petebacondarwin Sep 20, 2016

Author Member

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

This comment has been minimized.

Copy link
@rjamet

rjamet Sep 20, 2016

Contributor

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

(it's just weirdly undocumented ?)

This comment has been minimized.

Copy link
@petebacondarwin

petebacondarwin Oct 3, 2016

Author Member

fixed

}).not.toThrow();
});

it('jsonp() should allow trusted url', inject(['$sce', function($sce) {

This comment has been minimized.

Copy link
@gkalpak

gkalpak Sep 20, 2016

Member

This description is misleading.

This comment has been minimized.

Copy link
@petebacondarwin

petebacondarwin Oct 3, 2016

Author Member

changed

@rjamet

This comment has been minimized.

Copy link
Contributor

commented Sep 20, 2016

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

This comment has been minimized.

Copy link
Member Author

commented Sep 20, 2016

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

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);

This comment has been minimized.

Copy link
@Narretz

Narretz Sep 21, 2016

Contributor

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

This comment has been minimized.

Copy link
@petebacondarwin

petebacondarwin Oct 3, 2016

Author Member

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?

This comment has been minimized.

Copy link
@petebacondarwin

petebacondarwin Oct 3, 2016

Author Member

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

This comment has been minimized.

Copy link
Contributor

commented Sep 21, 2016

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';

paramSerializer: '$httpParamSerializer'
paramSerializer: '$httpParamSerializer',

callbackParam: 'callback'

This comment has been minimized.

Copy link
@Narretz

Narretz Sep 21, 2016

Contributor

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

This comment has been minimized.

Copy link
@petebacondarwin

This comment has been minimized.

Copy link
@petebacondarwin

petebacondarwin Sep 21, 2016

Author Member

Done in 022fc44

@Narretz

This comment has been minimized.

Copy link
Contributor

commented Sep 21, 2016

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

@petebacondarwin

This comment has been minimized.

Copy link
Member Author

commented Sep 21, 2016

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

@petebacondarwin

This comment has been minimized.

Copy link
Member Author

commented Sep 21, 2016

@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.

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 comment has been minimized.

Copy link
@gkalpak

gkalpak Sep 21, 2016

Member

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')

This comment has been minimized.

Copy link
@petebacondarwin

petebacondarwin Sep 21, 2016

Author Member

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

This comment has been minimized.

Copy link
@petebacondarwin

petebacondarwin Oct 3, 2016

Author Member

fixed

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

This comment has been minimized.

Copy link
@gkalpak

gkalpak Sep 21, 2016

Member

JSON --.> JSONP

This comment has been minimized.

Copy link
@petebacondarwin

petebacondarwin Oct 3, 2016

Author Member

fixed

@@ -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}

This comment has been minimized.

Copy link
@gkalpak

gkalpak Sep 21, 2016

Member

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

This comment has been minimized.

Copy link
@petebacondarwin

petebacondarwin Oct 3, 2016

Author Member

fixed

@@ -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,

This comment has been minimized.

Copy link
@gkalpak

gkalpak Sep 21, 2016

Member

acces --> access

This comment has been minimized.

Copy link
@petebacondarwin

petebacondarwin Oct 3, 2016

Author Member

fixed

* 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)`}.

This comment has been minimized.

Copy link
@gkalpak

gkalpak Sep 21, 2016

Member

trusted --> trusting

This comment has been minimized.

Copy link
@petebacondarwin

petebacondarwin Oct 3, 2016

Author Member

fixed

* 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

This comment has been minimized.

Copy link
@gkalpak

gkalpak Sep 21, 2016

Member

as as --> as

This comment has been minimized.

Copy link
@petebacondarwin

petebacondarwin Oct 3, 2016

Author Member

fixed

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

This comment has been minimized.

Copy link
@gkalpak

gkalpak Sep 21, 2016

Member

(callback --> 'callback')

This comment has been minimized.

Copy link
@petebacondarwin

petebacondarwin Oct 3, 2016

Author Member

fixed

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

This comment has been minimized.

Copy link
@gkalpak

gkalpak Sep 21, 2016

Member

(callback --> 'callback')

This comment has been minimized.

Copy link
@petebacondarwin

petebacondarwin Oct 3, 2016

Author Member

fixed

throw $httpMinErr('badjsonp', 'Illegal use of JSON_CALLBACK in url, "{0}"', url);
}

var callbackParamRegex = new RegExp('([&?]' + key + '=)');

This comment has been minimized.

Copy link
@gkalpak

gkalpak Sep 21, 2016

Member

Why the parenthesis?

This comment has been minimized.

Copy link
@petebacondarwin

petebacondarwin Sep 21, 2016

Author Member

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

This comment has been minimized.

Copy link
@petebacondarwin

petebacondarwin Oct 3, 2016

Author Member

fixed

@petebacondarwin petebacondarwin force-pushed the petebacondarwin:pr-11623a branch from 25d72c4 to a69729d Oct 3, 2016
Copy link
Member

left a comment

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

@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`

This comment has been minimized.

Copy link
@gkalpak

gkalpak Oct 4, 2016

Member

Nit: Wrapping lines at 100 chars is sooo cool 😁

This comment has been minimized.

Copy link
@petebacondarwin
@@ -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

This comment has been minimized.

Copy link
@gkalpak

gkalpak Oct 4, 2016

Member

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

This comment has been minimized.

Copy link
@petebacondarwin
try {
return sendReq(config, reqData).then(transformResponse, transformResponse);
} catch (e) {
return $q.reject(e);

This comment has been minimized.

Copy link
@gkalpak

gkalpak Oct 4, 2016

Member

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

This comment has been minimized.

Copy link
@petebacondarwin

petebacondarwin Oct 4, 2016

Author Member

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

This comment has been minimized.

Copy link
@petebacondarwin

petebacondarwin Oct 4, 2016

Author Member

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 comment has been minimized.

Copy link
@gkalpak

gkalpak Oct 4, 2016

Member

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.

This comment has been minimized.

Copy link
@petebacondarwin

petebacondarwin Oct 5, 2016

Author Member

Oh OK. I'll remove it after 15213 then

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

This comment has been minimized.

Copy link
@gkalpak

gkalpak Oct 4, 2016

Member

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

This comment has been minimized.

Copy link
@petebacondarwin

petebacondarwin Oct 4, 2016

Author Member

how about "application-wide"?

This comment has been minimized.

Copy link
@petebacondarwin

petebacondarwin Oct 4, 2016

Author Member

default is OK.

@gkalpak
gkalpak approved these changes Oct 5, 2016
Copy link
Member

left a comment

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 #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'});
```
@petebacondarwin petebacondarwin force-pushed the petebacondarwin:pr-11623a branch from cb2db20 to 3c1cd3b Oct 5, 2016
ellimist added a commit to ellimist/angular.js that referenced this pull request Mar 15, 2017
… 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'});
```
brad4d added a commit to google/closure-compiler that referenced this pull request Apr 11, 2017
Yannic added a commit to Yannic/com_google_closure_compiler that referenced this pull request Jul 6, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.