fix($http): parse JSON only if Content-Type header contains string 'json' #5765
Conversation
|
Hmmm, this breaks docs unfortunately :( I guess there needs to be a better way to do this |
|
this is going to break a ton of tests as you demonstrated with this PR. I think we'll need to come up with a different solution :-/ or somehow make the tests generally compatible with this change. since it's a breaking change we also can't merge it in for a 1.2.x release. so I'm marking it as 1.3 |
|
I considered wrapping the JSON parsing in a try/catch block, but I think this would hide legitimate errors and be a problem. I'm not sure if there's any other way to do this without some breaking changes. This won't affect JSONP at all, so it's probably for the better to take the Content-Type header into consideration. The script directive tests which were broken are broken because the cached value has to be an array, otherwise if the value is an array, $http treats it as an array of response values and resolves with those. This could be avoided by storing the string and calling JSON.parse in transformResponse, but I think doing this we could improve performance a little bit (at the cost of memory) --- however it would cause a minor bug in that it would always return the same object and not a copy... So that's kind of not good. So, maybe the scriptDirective stuff could be improved, but I think the rest of it is probably how it's going to be unless we decide we want to hide legitimate errors --- something that could be added to make migration for this feature a bit more painless is to make $http methods which specifically expect JSON values and set up the content-type header themselves, so that it's like a 4 letter change per test. And maybe a helper to put JSON into $templateCache, too. But yeah, no matter what it's still going to break some apps. |
This change is needed so that $http can avoid parsing JSON if an inline
template is in use.
BREAKING CHANGE: inline templates which were previously parsed as JSON by
default, now require the attribute `content-type` to contain the text 'json'.
Additionally, the data pushed into $templateCache by the script directive is
now always an array, with the following items:
[ status (200), data (script text or parsed JSON), headers (the empty object) ]
BEFORE:
```
<script type="text/ng-template" id="famous-foods.json">
{
"elvis presley": "peanut butter, banana & bacon sandwich",
"shirley temple": "whipped potatoes & roast beef",
"marilyn monroe": "grilled steak"
}
</script>
```
AFTER:
```
<script type="text/ng-template" id="famous-foods.json" content-type="application/json">
{
"elvis presley": "peanut butter, banana & bacon sandwich",
"shirley temple": "whipped potatoes & roast beef",
"marilyn monroe": "grilled steak"
}
</script>
…son' The default behaviour breaks a wide variety of custom interpolation markers. Even if the JSON text regexps were strengthened to closer match the standard JSON format, it would still limit the possibilities of different custom interpolation markers. Instead, $http will no longer parse JSON if the response Content-Type header does not include the term 'json', or if the $templateCache is used. For inline templates, use the `content-type="json"` attribute to ensure that inline JSON templates are parsed. BREAKING CHANGE: Previously, responses would be parsed as JSON if their content looked vaguely like JSON. Now, they are only parsed as JSON if their Content-Type response header contains the term 'json', and if they are not coming from the $templateCache. Closes angular#5756 Closes angular#2973
Helper to assist in migrating to the new $http behaviour. Usage:
```
angular.module('dummy-app', [])
.config(function($httpProvider) {
$httpProvider.autoParseJSON(true);
});
```
|
hmmm ...bitrot.... sigh I'll look at fixing this tomorrow, I think it would be good to ship for one of the 1.3 releases, so people have time to complain about it before it goes stable. |
| // strip json vulnerability protection prefix | ||
| var contentType = isFunction(headers) && headers('Content-Type'); | ||
| data = data.replace(PROTECTION_PREFIX, ''); | ||
| if (JSON_START.test(data) && JSON_END.test(data) && |
There was a problem hiding this comment.
Don't use the regexp tests here.
There was a problem hiding this comment.
can you remove the start and end checks
petebacondarwin
force-pushed
the
master
branch
from
02dc2aa
to
fd2d6c0
Compare
|
what's up with this PR? do we want to ship it in 1.3? |
|
it definitely makes sense to take content-type response headers into consideration IMO |
The default behaviour causes issues with a wide variety of custom
interpolation markers. Even if the JSON test regexps were strengthened to
closer match the standard JSON format, it would still limit the possibilities
of different custom interpolation markers.
BREAKING CHANGE: Previously, responses would be parsed as JSON if their
content looked vaguely like JSON. Now, they are only parsed as JSON if their
Content-Type response header contains the term 'json', and if they are not
coming from the $templateCache.
Closes #5756
Closes #2973