Skip to content

Commit

Permalink
fix($http): parse JSON only if Content-Type header contains string 'j…
Browse files Browse the repository at this point in the history 
…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
  • Loading branch information
@caitp
caitp committed Jan 14, 2014
1 parent bbfafc0 commit 6c1a413
Show file tree
Hide file tree
Showing 3 changed files with 37 additions and 13 deletions.
6 changes: 4 additions & 2 deletions src/ng/http.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -91,11 +91,13 @@ function $HttpProvider() {

var defaults = this.defaults = {
// transform incoming response data
transformResponse: [function(data) {
transformResponse: [function(data, headers) {
if (isString(data)) {
// 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))
if (JSON_START.test(data) && JSON_END.test(data) &&
(contentType && contentType.indexOf('json') >= 0))

This comment has been minimized.

Sign in to view

Copy link
@pkozlowski-opensource

pkozlowski-opensource Feb 22, 2014

@caitp was just looking into number of issues related to this second-guessing of JSON response format and from what I could see people are mostly after having values like "foo", 1 or null to be parsed as JSON. I'm probably missing tons of things, but couldn't we improve the current situation by turning this condition into:

if ((contentType && contentType.indexOf('json') >= 0) || (JSON_START.test(data) && JSON_END.test(data))) {
  //let's parse it as JSON since either:
  //- backend says it is JSON, or
  //- it looks like JSON (current behaviour)
}

While not ideal it would probably address all the issues reported so far. I've crafted a simple commit based on this idea here: pkozlowski-opensource@5b1e5d7 and the CI build is green with this change. But yeh, I'm probably missing things here and there is still a potential for a breaking change here...

This comment has been minimized.

Sign in to view

Copy link
@caitp

caitp Feb 22, 2014

Author Owner

(one of) the problems with the JSON regexp tests is that they cause certain custom interpolation markers to look like JSON (and thus cause us to try to parse it as JSON, and end up throwing rather than using the text). There are a few things we could do, for instance denormalize interpolation markers in text data before testing. But I think it's probably a good idea to trust content-type headers for this.

This comment has been minimized.

Sign in to view

Copy link
@pkozlowski-opensource

pkozlowski-opensource Feb 22, 2014

@caitp oh, I see, I guess something like {[expr]} would fail, yes. So I guess we need to choose between a "proper" solution with bigger impact and "less proper" one with smaller impact... BTW, I'm a bit nervous about testing for json only in response headers as there are probably other ones that people are using (text/javascript for example). So it is hard to be 100% "right"....

This comment has been minimized.

Sign in to view

Copy link
@caitp

caitp Feb 22, 2014

Author Owner

I think text/javascript is most likely going to happen for JSONP requests, so I am not terribly concerned with that. I think json being present in the Content-Type response header for a response which is a plain JSON string, is a sign of a well-behaved backend, and we should probably try to encourage that behaviour.
data = fromJson(data);
}
return data;
Expand Down
40 changes: 30 additions & 10 deletions test/ng/httpSpec.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1014,8 +1014,10 @@ describe('$http', function() {

describe('default', function() {

it('should deserialize json objects', function() {
$httpBackend.expect('GET', '/url').respond('{"foo":"bar","baz":23}');
it('should deserialize json objects with json content-type', function() {
$httpBackend.expect('GET', '/url').respond('{"foo":"bar","baz":23}', {
'Content-Type': 'application/json'
});
$http({method: 'GET', url: '/url'}).success(callback);
$httpBackend.flush();

Expand All @@ -1024,8 +1026,10 @@ describe('$http', function() {
});


it('should deserialize json arrays', function() {
$httpBackend.expect('GET', '/url').respond('[1, "abc", {"foo":"bar"}]');
it('should deserialize json arrays with json content-type', function() {
$httpBackend.expect('GET', '/url').respond('[1, "abc", {"foo":"bar"}]', {
'Content-Type': 'application/json'
});
$http({method: 'GET', url: '/url'}).success(callback);
$httpBackend.flush();

Expand All @@ -1034,8 +1038,10 @@ describe('$http', function() {
});


it('should deserialize json with security prefix', function() {
$httpBackend.expect('GET', '/url').respond(')]}\',\n[1, "abc", {"foo":"bar"}]');
it('should deserialize json with security prefix with json content-type', function() {
$httpBackend.expect('GET', '/url').respond(')]}\',\n[1, "abc", {"foo":"bar"}]', {
'Content-Type': 'application/json'
});
$http({method: 'GET', url: '/url'}).success(callback);
$httpBackend.flush();

Expand All @@ -1044,8 +1050,10 @@ describe('$http', function() {
});


it('should deserialize json with security prefix ")]}\'"', function() {
$httpBackend.expect('GET', '/url').respond(')]}\'\n\n[1, "abc", {"foo":"bar"}]');
it('should deserialize json with security prefix ")]}\'" with json content-type', function() {
$httpBackend.expect('GET', '/url').respond(')]}\'\n\n[1, "abc", {"foo":"bar"}]', {
'Content-Type': 'application/json'
});
$http({method: 'GET', url: '/url'}).success(callback);
$httpBackend.flush();

Expand All @@ -1054,8 +1062,10 @@ describe('$http', function() {
});


it('should not deserialize tpl beginning with ng expression', function() {
$httpBackend.expect('GET', '/url').respond('{{some}}');
it('should not deserialize tpl beginning with ng expression with json content-type', function() {
$httpBackend.expect('GET', '/url').respond('{{some}}', {
'Content-Type': 'application/json'
});
$http.get('/url').success(callback);
$httpBackend.flush();

Expand All @@ -1065,6 +1075,16 @@ describe('$http', function() {
});


it('should not deserialize JSON response without Content-Type header matching \'json\'', function() {
$httpBackend.expect('GET', '/url').respond('{ "foo": "bar" }');
$http.get('/url').success(callback);
$httpBackend.flush();

expect(callback).toHaveBeenCalledOnce();
expect(callback.mostRecentCall.args[0]).toBe('{ "foo": "bar" }');
});


it('should have access to response headers', function() {
$httpBackend.expect('GET', '/url').respond(200, 'response', {h1: 'header1'});
$http.get('/url', {
Expand Down
4 changes: 3 additions & 1 deletion test/ngResource/resourceSpec.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -534,7 +534,9 @@ describe("resource", function() {
it('should exercise full stack', function() {
var Person = $resource('/Person/:id');

$httpBackend.expect('GET', '/Person/123').respond('\n{\n"name":\n"misko"\n}\n');
$httpBackend.expect('GET', '/Person/123').respond('\n{\n"name":\n"misko"\n}\n', {
'Content-Type': 'application/json'
});
var person = Person.get({id:123});
$httpBackend.flush();
expect(person.name).toEqual('misko');
Expand Down

0 comments on commit 6c1a413

Please sign in to comment.