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

$http always transform response content '[abc}' to JSON even we already set resp content type as 'application/octet-stream' #10349

Closed
rebornix opened this Issue Dec 6, 2014 · 13 comments

Comments

Projects
None yet
6 participants
@rebornix

rebornix commented Dec 6, 2014

As title, we ran into this issue when we try to get blob content from azure, using $http.get(url).then();. Angular would throw exception "Syntax error: Invalid ...". Seems Angular always try to render our blob content as JSON. After reading code in /src/ng/http.js, I think I may know why.

var APPLICATION_JSON = 'application/json';
var CONTENT_TYPE_APPLICATION_JSON = {'Content-Type': APPLICATION_JSON + ';charset=utf-8'};
var JSON_START = /^\s*(\[|\{[^\{])/;
var JSON_END = /[\}\]]\s*$/;
var JSON_PROTECTION_PREFIX = /^\)\]\}',?\n/;

function defaultHttpResponseTransform(data, headers) {
  if (isString(data)) {
    // strip json vulnerability protection prefix
    data = data.replace(JSON_PROTECTION_PREFIX, '');
    var contentType = headers('Content-Type');
    if ((contentType && contentType.indexOf(APPLICATION_JSON) === 0 && data.trim()) ||
        (JSON_START.test(data) && JSON_END.test(data))) {
      data = fromJson(data);
    }
  }
  return data;
}

In this piece of code, $http will try to detect response's content. If the contentType is equal to application/json, $http will transform the content to JSON object then return to users. It does make sense.

But if the contentType is not application/json, it would use (JSON_START.test(data) && JSON_END.test(data)) to identify whether the content is a JSON object. This is not accurate and not intuitive either. A string starts/ends with '[ { ] }' doesn't mean it's JSON, you can't even say it's like JSON.

For example, when you are writing a markdown article, maye API document, you may write a piece of content like below:

[link](some links...)

function bar:
    function bar() {
      if (a > b) {
        foo()'
      }
    }

Obviously, it's not JSON while (JSON_START.test(data) && JSON_END.test(data) will return true. This kind of RegEx is weak.

BTW, if users set response content-type explicitly, like application/octet-stream or application/text-HTML, it means yeah I know I am returning you a byte array or a piece of HTML. But AngularJS still try to transform it to JSON, and booom, throw an exception...

For now I can only set the response type while making ajax calls: $http.get(url, {responseType: byteArray}). Can we remove this RegEx check and give users less bother like Jquery does?

@formulahendry

This comment has been minimized.

Show comment
Hide comment
@formulahendry

formulahendry Dec 6, 2014

I've met with the same issue.
Strongly agree with rebornix, two things need to be improved:

  1. If the response content-type was set explicitly from the server side, AngularJS need to treat the content-type as what the server side said first.
  2. Improve how AngularJS check whether the response content-type is JSON or not. Currently, the check is too weak.

formulahendry commented Dec 6, 2014

I've met with the same issue.
Strongly agree with rebornix, two things need to be improved:

  1. If the response content-type was set explicitly from the server side, AngularJS need to treat the content-type as what the server side said first.
  2. Improve how AngularJS check whether the response content-type is JSON or not. Currently, the check is too weak.
@pkozlowski-opensource

This comment has been minimized.

Show comment
Hide comment
@pkozlowski-opensource

pkozlowski-opensource Dec 6, 2014

Member

This is a valid issue. There are number of things that we could improve in 1.3 (ex. making sure that starting and ending chars are the same in JSON-like strings, doing JSON-like detection only for certain response headers etc.), but the "proper" fix would consist of doing JSON-deserialization only if application/json contentType header is present. But this would be a breaking change so we can't do this in 1.3.

@caitp @petebacondarwin what do you guys think of fixing it in 1.4? I still do afraid that it would break applications for people interacting with non-well-behaving backends but yeh, at least we would avoid issues like this one.

Member

pkozlowski-opensource commented Dec 6, 2014

This is a valid issue. There are number of things that we could improve in 1.3 (ex. making sure that starting and ending chars are the same in JSON-like strings, doing JSON-like detection only for certain response headers etc.), but the "proper" fix would consist of doing JSON-deserialization only if application/json contentType header is present. But this would be a breaking change so we can't do this in 1.3.

@caitp @petebacondarwin what do you guys think of fixing it in 1.4? I still do afraid that it would break applications for people interacting with non-well-behaving backends but yeh, at least we would avoid issues like this one.

@petebacondarwin

This comment has been minimized.

Show comment
Hide comment
@petebacondarwin

petebacondarwin Dec 6, 2014

Member

I hope this can be solved elegantly in 1.4.

Member

petebacondarwin commented Dec 6, 2014

I hope this can be solved elegantly in 1.4.

@caitp

This comment has been minimized.

Show comment
Hide comment
@caitp

caitp Dec 6, 2014

Contributor

The content-type header was supposed to be respected in all cases in 1.3 --- if we keep saying it's too big of a breaking change, it's never going to be properly respected.

Contributor

caitp commented Dec 6, 2014

The content-type header was supposed to be respected in all cases in 1.3 --- if we keep saying it's too big of a breaking change, it's never going to be properly respected.

@caitp

This comment has been minimized.

Show comment
Hide comment
@caitp

caitp Dec 6, 2014

Contributor

However, there is a solution: $templateRequest will not try to parse as json.

Contributor

caitp commented Dec 6, 2014

However, there is a solution: $templateRequest will not try to parse as json.

@gkalpak

This comment has been minimized.

Show comment
Hide comment
@gkalpak

gkalpak Dec 7, 2014

Member

@caitp: Are you suggesting using $templateRequest instead of $http when you don't want JSON parsing (because that doesn't sound like a good idea) ?

Member

gkalpak commented Dec 7, 2014

@caitp: Are you suggesting using $templateRequest instead of $http when you don't want JSON parsing (because that doesn't sound like a good idea) ?

@rebornix

This comment has been minimized.

Show comment
Hide comment
@rebornix

rebornix Dec 7, 2014

Agree with @gkalpak , for HTML template using $templateRequest can solve this problem and it may even be a best practice. But for byte array, which is a common case I suppose, caching is not a good idea.

rebornix commented Dec 7, 2014

Agree with @gkalpak , for HTML template using $templateRequest can solve this problem and it may even be a best practice. But for byte array, which is a common case I suppose, caching is not a good idea.

@caitp

This comment has been minimized.

Show comment
Hide comment
@caitp

caitp Dec 7, 2014

Contributor

(because that doesn't sound like a good idea) ?

The only reason it's not a good idea is because you lose access to the rest of the $http API, but $templateRequest is just a thin wrapper around $http which removes the builtin JSON parsing transformer. (Well, it does some animation-specific other things, but these don't matter a whole lot).

While caching may be undesirable, it's easily circumvented

Contributor

caitp commented Dec 7, 2014

(because that doesn't sound like a good idea) ?

The only reason it's not a good idea is because you lose access to the rest of the $http API, but $templateRequest is just a thin wrapper around $http which removes the builtin JSON parsing transformer. (Well, it does some animation-specific other things, but these don't matter a whole lot).

While caching may be undesirable, it's easily circumvented

@gkalpak

This comment has been minimized.

Show comment
Hide comment
@gkalpak

gkalpak Dec 7, 2014

Member

While caching may be undesirable, it's easily circumvented

Looking at the code, I can't really see how it could be circumvented at all. (It seems to be hardcoded into the httpOptions object passed to $http.)

So, the only reasons I can think of (and I wasn't even aware of the animation-specific stuff):

  • Losing access to the rest of the $http API.
  • Having errors being handled differently (e.g. often ignored) or having misleading error messages.
  • Having the response cached (which is undesirable both from a memory-footprint point of view and from a business-logic point of view).
  • Being not future-proof (since $templateRequest is supposed to be handling templates, any future changes (which might be appropriate for handling templates) might break non-template requests).

Wouldn't it be a better idea to just remove the default transform ?

Member

gkalpak commented Dec 7, 2014

While caching may be undesirable, it's easily circumvented

Looking at the code, I can't really see how it could be circumvented at all. (It seems to be hardcoded into the httpOptions object passed to $http.)

So, the only reasons I can think of (and I wasn't even aware of the animation-specific stuff):

  • Losing access to the rest of the $http API.
  • Having errors being handled differently (e.g. often ignored) or having misleading error messages.
  • Having the response cached (which is undesirable both from a memory-footprint point of view and from a business-logic point of view).
  • Being not future-proof (since $templateRequest is supposed to be handling templates, any future changes (which might be appropriate for handling templates) might break non-template requests).

Wouldn't it be a better idea to just remove the default transform ?

@caitp

This comment has been minimized.

Show comment
Hide comment
@caitp

caitp Dec 7, 2014

Contributor

Looking at the code, I can't really see how it could be circumvented at all. (It seems to be hardcoded into the httpOptions object passed to $http.)

By deleting it from the cache...

Wouldn't it be a better idea to just remove the default transform ?

No, we have it for a good reason --- supported browsers don't all support responseType=json, and we wouldn't really know when to ask for responseType=json even if they did.

Being not future-proof (since $templateRequest is supposed to be handling templates, any future changes (which might be appropriate for handling templates) might break non-template requests).

Anyone could write a similar thin wrapper around http, it's just an example --- realistically, it's not going to change, because it doesn't need to do anything special

Contributor

caitp commented Dec 7, 2014

Looking at the code, I can't really see how it could be circumvented at all. (It seems to be hardcoded into the httpOptions object passed to $http.)

By deleting it from the cache...

Wouldn't it be a better idea to just remove the default transform ?

No, we have it for a good reason --- supported browsers don't all support responseType=json, and we wouldn't really know when to ask for responseType=json even if they did.

Being not future-proof (since $templateRequest is supposed to be handling templates, any future changes (which might be appropriate for handling templates) might break non-template requests).

Anyone could write a similar thin wrapper around http, it's just an example --- realistically, it's not going to change, because it doesn't need to do anything special

@gkalpak

This comment has been minimized.

Show comment
Hide comment
@gkalpak

gkalpak Dec 7, 2014

Member

Wouldn't it be a better idea to just remove the default transform ?

No, we have it for a good reason --- supported browsers don't all support responseType=json, and we wouldn't really know when to ask for responseType=json even if they did.

I didn't mean for us to remove the default transform. I meant for the user/developer (and not necessarily for all requests).

Member

gkalpak commented Dec 7, 2014

Wouldn't it be a better idea to just remove the default transform ?

No, we have it for a good reason --- supported browsers don't all support responseType=json, and we wouldn't really know when to ask for responseType=json even if they did.

I didn't mean for us to remove the default transform. I meant for the user/developer (and not necessarily for all requests).

gkalpak added a commit to gkalpak/angular.js that referenced this issue Dec 7, 2014

fix($http): only parse as JSON when opening/closing brackets match
Previously, due to weak JSON-detecting RegExp, string like `[...}` and
`{...]` would be considered JSON (even if they obviously aren't) and an
expection would be thrown while trying to parse them.

This commit makes sure the opening and closing brackets match. This
doesn't completely eliminate false positives (e.g. `[]{}[]`), but does
help reduce them.

Closes angular#10349

gkalpak added a commit to gkalpak/angular.js that referenced this issue Dec 7, 2014

fix($http): only parse as JSON when opening/closing brackets match
Previously, due to weak JSON-detecting RegExp, string like `[...}` and
`{...]` would be considered JSON (even if they obviously aren't) and an
expection would be thrown while trying to parse them.

This commit makes sure the opening and closing brackets match. This
doesn't completely eliminate false positives (e.g. `[]{}[]`), but does
help reduce them.

Closes angular#10349

gkalpak added a commit to gkalpak/angular.js that referenced this issue Dec 7, 2014

fix($http): only parse as JSON when opening/closing brackets match
Previously, due to weak JSON-detecting RegExp, string like `[...}` and
`{...]` would be considered JSON (even if they obviously aren't) and an
expection would be thrown while trying to parse them.

This commit makes sure the opening and closing brackets match. This
doesn't completely eliminate false positives (e.g. `[]{}[]`), but does
help reduce them.

Closes angular#10349
@petebacondarwin

This comment has been minimized.

Show comment
Hide comment
@petebacondarwin

petebacondarwin Dec 8, 2014

Member

Using $templateCache is definitely not a valid workaround. It is not supposed to be used for this and as pointed out even adds wrinkles of its own.
The correct workaround, before this is fixed is to change the transform responses.
If you don't provide one for a request then $http uses the $http.defaults.transformResponse array. Out of the box it contains [defaultHttpResponseTransform] which is the offending function that does the "guessing" of JSON.

If you provide a transformResponse property on your $http request object then this will be used instead of the default.

So your options are to provide a custom transform per request or replace $http.defaults.transformResponse with your custom transform.

See https://docs.angularjs.org/api/ng/service/$http#transforming-requests-and-responses

Member

petebacondarwin commented Dec 8, 2014

Using $templateCache is definitely not a valid workaround. It is not supposed to be used for this and as pointed out even adds wrinkles of its own.
The correct workaround, before this is fixed is to change the transform responses.
If you don't provide one for a request then $http uses the $http.defaults.transformResponse array. Out of the box it contains [defaultHttpResponseTransform] which is the offending function that does the "guessing" of JSON.

If you provide a transformResponse property on your $http request object then this will be used instead of the default.

So your options are to provide a custom transform per request or replace $http.defaults.transformResponse with your custom transform.

See https://docs.angularjs.org/api/ng/service/$http#transforming-requests-and-responses

gkalpak added a commit to gkalpak/angular.js that referenced this issue Dec 8, 2014

fix($http): only parse as JSON when opening/closing brackets match
Previously, due to weak JSON-detecting RegExp, string like `[...}` and
`{...]` would be considered JSON (even if they obviously aren't) and an
expection would be thrown while trying to parse them.

This commit makes sure the opening and closing brackets match. This
doesn't completely eliminate false positives (e.g. `[]{}[]`), but does
help reduce them.

Closes angular#10349

gkalpak added a commit to gkalpak/angular.js that referenced this issue Dec 8, 2014

fix($http): only parse as JSON when opening/closing brackets match
Previously, due to weak JSON-detecting RegExp, string like `[...}` and
`{...]` would be considered JSON (even if they obviously aren't) and an
expection would be thrown while trying to parse them.

This commit makes sure the opening and closing brackets match. This
doesn't completely eliminate false positives (e.g. `[]{}[]`), but does
help reduce them.

Closes angular#10349
@caitp

This comment has been minimized.

Show comment
Hide comment
@caitp

caitp Dec 8, 2014

Contributor

It is a valid workaround --- it does not add any wrinkles --- If you were to use $templateRequest (which deals with removing the transform) in addition to removing the cached value on resolve, the only issues you're left with is not having the exact same API.

The real best solution is just writing a thin wrapper service around $http native to your own app, but the point here is that you can use $templateRequest as a guide for doing this. This ain't rocket science folks, there are literally only two constraints here, and they're trivial to implement.

Contributor

caitp commented Dec 8, 2014

It is a valid workaround --- it does not add any wrinkles --- If you were to use $templateRequest (which deals with removing the transform) in addition to removing the cached value on resolve, the only issues you're left with is not having the exact same API.

The real best solution is just writing a thin wrapper service around $http native to your own app, but the point here is that you can use $templateRequest as a guide for doing this. This ain't rocket science folks, there are literally only two constraints here, and they're trivial to implement.

gkalpak added a commit to gkalpak/angular.js that referenced this issue Dec 10, 2014

fix($http): only parse as JSON when opening/closing brackets match
Previously, due to weak JSON-detecting RegExp, string like `[...}` and
`{...]` would be considered JSON (even if they obviously aren't) and an
expection would be thrown while trying to parse them.

This commit makes sure the opening and closing brackets match. This
doesn't completely eliminate false positives (e.g. `[]{}[]`), but does
help reduce them.

Closes angular#10349

gkalpak added a commit to gkalpak/angular.js that referenced this issue Dec 12, 2014

fix($http): only parse as JSON when opening/closing brackets match
Previously, due to weak JSON-detecting RegExp, string like `[...}` and
`{...]` would be considered JSON (even if they obviously aren't) and an
expection would be thrown while trying to parse them.

This commit makes sure the opening and closing brackets match. This
doesn't completely eliminate false positives (e.g. `[]{}[]`), but does
help reduce them.

Closes angular#10349

gkalpak added a commit to gkalpak/angular.js that referenced this issue Dec 12, 2014

fix($http): only parse as JSON when opening/closing brackets match
Previously, due to weak JSON-detecting RegExp, string like `[...}` and
`{...]` would be considered JSON (even if they obviously aren't) and an
expection would be thrown while trying to parse them.

This commit makes sure the opening and closing brackets match. This
doesn't completely eliminate false positives (e.g. `[]{}[]`), but does
help reduce them.

Closes angular#10349

@gkalpak gkalpak closed this in b9bdbe6 Dec 12, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment