fix(common): attempt to JSON.parse errors for JSON responses #19773
Conversation
|
You can preview 1679728 at https://pr19773-1679728.ngbuilds.io/. |
alxhub
force-pushed
the
http-error-parse
branch
from
1679728
to
fbc0297
Compare
|
You can preview fbc0297 at https://pr19773-fbc0297.ngbuilds.io/. |
IgorMinar
added
area: common/http
action: merge
Prior behavior for HttpClient was to parse the body as JSON when responseType was set to 'json', even if the response was unsuccessful. This changed due to a recent bugfix, and unsuccessful responses had their bodies treated as strings. There is no guarantee that if a service returns JSON in the successful case that it will do so for errors. However, users indicate that most APIs in the wild do work this way. Therefore, this commit changes the error case behavior to attempt a JSON parsing of the response body, and falls back on returning it as a string if that fails.
alxhub
force-pushed
the
http-error-parse
branch
from
fbc0297
to
2a539e6
Compare
|
You can preview 2a539e6 at https://pr19773-2a539e6.ngbuilds.io/. |
Prior behavior for HttpClient was to parse the body as JSON when responseType was set to 'json', even if the response was unsuccessful. This changed due to a recent bugfix, and unsuccessful responses had their bodies treated as strings. There is no guarantee that if a service returns JSON in the successful case that it will do so for errors. However, users indicate that most APIs in the wild do work this way. Therefore, this commit changes the error case behavior to attempt a JSON parsing of the response body, and falls back on returning it as a string if that fails. PR Close #19773
|
Should this be present for the error portion as well? https://github.com/angular/angular/pull/19773/files#diff-26683cfb34fa97883213282055aef1e7R185 |
|
Maybe I'm misunderstanding what you mean, but that error branch is
unrelated. Can you clarify?
…On Thu, Oct 19, 2017 at 1:13 PM Linskeyd ***@***.***> wrote:
@alxhub <https://github.com/alxhub>,
Should this be present for the error portion as well?
https://github.com/angular/angular/pull/19773/files#diff-26683cfb34fa97883213282055aef1e7R185
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#19773 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AANM6GOF1l5qrMnGbfbncUbTws5WanAEks5st62FgaJpZM4P9Bg6>
.
|
|
I have tested the issue. It fixed the issue when the response has content. However, when I return Ok() (Asp.net Core 2.0 - Status code 200). The issue still happen.
|
|
Ich kehre zurück am 01.11.2017.
Ich werde Ihre Nachricht nach meiner Rückkehr beantworten.
In dringenden Fällen senden Sie bitte eine Kopie Ihrer E-Mail für
technische Angelegenheiten an entwicklung@arxes-tolina.de, ansonsten an
info@arxes-tolina.de. Ein anderer Mitarbeiter wird sich dann Ihrer E-Mail
annehmen.
Hinweis: Dies ist eine automatische Antwort auf Ihre Nachricht "Re:
[angular/angular] fix(common): attempt to JSON.parse errors for JSON
responses (#19773)" gesendet am 23.10.2017 09:43:38.
Diese ist die einzige Benachrichtigung, die Sie empfangen werden, während
diese Person abwesend ist.
|
|
I'm talking about the XSSI prefix being only stripped for OK statuses. Take a look here (at a commit prior to the modifications to any of this code path: 452a7ae) You'll notice that for all statuses (except for 204) the prefix is attempted to be stripped. With the changes on this pull request that have been merged, only the OK status has the XSSI prefix stripped. Happy to submit a PR for this if you agree with the above. |
|
Looks like you are right. Please send a pr and tag @alxhub
…On Mon, Oct 23, 2017, 8:34 AM Linskeyd ***@***.***> wrote:
@IgorMinar <https://github.com/igorminar>,
I'm talking about the XSSI prefix being only stripped for OK statuses.
Take a look here (at a commit prior to the modifications to any of this
code path: 452a7ae
<452a7ae>
)
You'll notice that for all statuses (except for 204) the prefix is
attempted to be stripped. With the changes on this pull request that have
been merged, only the OK status has the XSSI prefix stripped.
Happy to submit a PR for this if you agree with the above.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#19773 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AANM6Ios8DkQVosqf3b5nWIS0JT-rfT0ks5svLIdgaJpZM4P9Bg6>
.
|
|
I believe you're correct, it should be stripped. |
| } else if (!ok && req.responseType === 'json' && typeof body === 'string') { | ||
| try { | ||
| // Attempt to parse the body as JSON. | ||
| body = JSON.parse(body); |
There was a problem hiding this comment.
Unfortunately this does not support XSSI, there should be similar approach as for OK flow with body.replace(XSSI_PREFIX, '')
see also https://angular.io/guide/security#xssi
|
Please file a new issue
…On Tue, Oct 31, 2017, 5:34 PM Kamil Pakur ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In packages/common/http/src/xhr.ts
<#19773 (comment)>:
> @@ -191,6 +191,14 @@ export class HttpXhrBackend implements HttpBackend {
// The parse error contains the text of the body that failed to parse.
body = { error, text: body } as HttpJsonParseError;
}
+ } else if (!ok && req.responseType === 'json' && typeof body === 'string') {
+ try {
+ // Attempt to parse the body as JSON.
+ body = JSON.parse(body);
Unfortunately this does not support XSSI, there should be similar approach
as for OK flow with body.replace(XSSI_PREFIX, '')
see also https://angular.io/guide/security#xssi
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#19773 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AANM6OEOnYgsRWfFv18Cv6gN-0xuRPSSks5sx0wJgaJpZM4P9Bg6>
.
|
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
No description provided.