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

Inconsistent handling of $sce trustedUrl with $http #11328

Closed
klhuillier opened this issue Mar 15, 2015 · 5 comments

Comments

Projects
None yet
5 participants
@klhuillier
Copy link

commented Mar 15, 2015

// Works:
$http.get($sce.trustAsUrl('/')).success(function(d){$log.log(d);});

// Fails:
$http.get($sce.trustAsUrl('/'), {params: {timestamp: 0}}).success(function(d){$log.log(d);});

In the first case, when the trusted URL reaches the XmlHttpRequest, its toString method is used to get the wrapped URL. In the second case, the buildUrl function attempts to add parameters, but the given URL is not a string as expected.

I feel that the buildUrl function should reject a non-string argument for url in all cases.

@petebacondarwin

This comment has been minimized.

Copy link
Member

commented Mar 16, 2015

Or perhaps buildUrl should understand and cope with "trusted" resources?

@petebacondarwin

This comment has been minimized.

Copy link
Member

commented Mar 16, 2015

@pkozlowski-opensource could you take a look at this as part of the buildUrl refactoring?

@klhuillier

This comment has been minimized.

Copy link
Author

commented Mar 16, 2015

I thought about suggesting buildUrl tolerate trusted URLs, but I couldn't think of a reason it should. I also can't think of a reason it should not, other than, "is it really necessary?" I would be agreeable to either option.

The main issue was trusted URLs were working some of the time, but failing with an unhelpful message at other times. According to the current documentation, it should be failing every time because url is specified as type string.

chirayuk added a commit to chirayuk/angular.js that referenced this issue Apr 16, 2015

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 angular#11352
Closes angular#11328
@chirayuk

This comment has been minimized.

Copy link
Contributor

commented Apr 16, 2015

I have a work in progress PR that will fix this as a side effect of fixing #11352. I started on it yesterday but didn't have time to finish it. I won't get to it anytime today or tomorrow. However, I created a work-in-progress PR to let folks know that some work has started on it.

@chirayuk chirayuk modified the milestones: 1.4.0-rc.1, 1.4.0-rc.2 Apr 24, 2015

@petebacondarwin petebacondarwin modified the milestones: 1.5.x - migration-facilitation, 1.4.0-rc.2 May 5, 2015

petebacondarwin added a commit to petebacondarwin/angular.js that referenced this issue Sep 15, 2016

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

@petebacondarwin petebacondarwin modified the milestones: 1.6.0, 1.5.x Sep 15, 2016

@petebacondarwin

This comment has been minimized.

Copy link
Member

commented Sep 16, 2016

In December we put in an explicit check for non-string URLs so that effectively closes this issue.
We are still looking at making JSONP more secure with $sce which might require us to relax that constraint going forward.

petebacondarwin added a commit that referenced this issue Oct 5, 2016

feat($http): JSONP callback must be specified by `jsonpCallbackParam`…
… 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 #15161
Closes #15143
Closes #11352
Closes #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'});
```

ellimist added a commit to ellimist/angular.js that referenced this issue Mar 15, 2017

feat($http): JSONP callback must be specified by `jsonpCallbackParam`…
… 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'});
```
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.