defaultHttpResponseTransform tries to parse as Json "text/plain" that isn't Json

[r20]

I'm submitting a ...

• [x ] bug report
• feature request
• other (Please do not submit support requests here (see above))

Current behavior:

function defaultHttpResponseTransform(data, headers) {
  if (isString(data)) {
    // Strip json vulnerability protection prefix and trim whitespace
    var tempData = data.replace(JSON_PROTECTION_PREFIX, '').trim();

    if (tempData) {
      var contentType = headers('Content-Type');
      if ((contentType && (contentType.indexOf(APPLICATION_JSON) === 0)) || isJsonLike(tempData)) {
        try {
          data = fromJson(tempData);
        } catch (e) {
          throw $httpMinErr('baddata', 'Data must be a valid JSON object. Received: "{0}". ' +
          'Parse error: "{1}"', data, e);
        }
      }
    }
  }

  return data;
}

In defaultHttpResponseTransform() if the response data passes the isJsonLike() function, the function tries to transform the data using fromJson(). This may fail if the data isn't actually Json. If a server sends contentType "text/plain" with data "{a}" or "[a]" for example, these will result in angular throwing an error.

Expected / new behavior:

If the contentType is "text/plain", the defaultHttpResponseTransform should return the data without trying to change it.

Angular version: 1.6.4

[r20]

If it was too big of a behavior change to stray from the documentation

If the Content-Type is application/json or the response looks like JSON, deserialize it using a JSON parser.

Then the function could continue to attempt to parse as json what it thinks looks like json. However, if that fails and the Content-Type is specified and not application/json, then it should just return the data rather than throwing an error about it not being valid json.

[Narretz]

Changing this generally would be a breaking change for people that serve JSON without the correct content type. But returning plain text if the content type is text/plain seems reasonable.

[katsos]

Is anyone working on this one? Do you need a PR?

[gkalpak]

I don't think anyone is. It's your if you want to take it 😃

[katsos]

@Narretz Do you believe that we have to check data with isJsonLike() for every content-type except text/plain or to skip the isJsonLike() at all?

[Narretz]

@katsos
I assume we check every content-type for JSON-ness is because at the time many servers were sending JSON in plain text responses. Or something to that effect.

So the problem is that we definitely have to continue parsing text/plain responses for JSON-ness. Stopping this would be a breaking change.

However, what we can do is return the plain response if json parsing fails and the content type is text/plain.

However, this would still mean that text/plain content that looks like JSON is still parsed, so you wouldn't be guaranteed a plain text response.

For this reason, I now think it's better not to touch this code. Instead, if you have a plain text response, you should add your own response transformer. Wdyt?

[katsos]

@Narretz I think for sure that a JSON-like plain-text doesn't have to throw an error if it is not parsable from JSON parser.

So with "no-breaking-change" in mind, I agree to return the response unprocessed if content-type is everything else than "application/json".

But isn't confusing for those programmers who develop their APIs with content-type in mind and their clients end up with a json object instead of the declared content-type? (if content looks like a JSON of course)

[Narretz]

But isn't confusing for those programmers who develop their APIs with content-type in mind and their clients end up with a json object instead of the declared content-type? (if content looks like a JSON of course)

Yes it is. But it's the status quo. And I assume there are not many people who use plain text APIs, otherwise this issue would have appeared more often.

[katsos]

Status quo is to use proper Http-headers also but yes I get what you are saying. I will reply with my PR 😃