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

fix(common): attempt to JSON.parse errors for JSON responses #19773

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
7 participants
@alxhub
Contributor

alxhub commented Oct 18, 2017

No description provided.

@googlebot googlebot added the cla: yes label Oct 18, 2017

@mary-poppins

This comment has been minimized.

Show comment
Hide comment
@mary-poppins

mary-poppins commented Oct 18, 2017

@mary-poppins

This comment has been minimized.

Show comment
Hide comment
@mary-poppins

mary-poppins commented Oct 18, 2017

fix(common): attempt to JSON.parse errors for JSON responses
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.
@mary-poppins

This comment has been minimized.

Show comment
Hide comment
@mary-poppins

mary-poppins commented Oct 18, 2017

@tbosch tbosch closed this in 04ab9f1 Oct 18, 2017

tbosch added a commit that referenced this pull request Oct 18, 2017

fix(common): attempt to JSON.parse errors for JSON responses (#19773)
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
@Linskeyd

This comment has been minimized.

Show comment
Hide comment
@Linskeyd
Contributor

Linskeyd commented Oct 19, 2017

@IgorMinar

This comment has been minimized.

Show comment
Hide comment
@IgorMinar

IgorMinar Oct 20, 2017

Member
Member

IgorMinar commented Oct 20, 2017

@PascalHonegger PascalHonegger referenced this pull request Oct 21, 2017

Closed

HttpClient doesn't parse an error response in IE11 #18682

1 of 8 tasks complete
@duykhuongkid

This comment has been minimized.

Show comment
Hide comment
@duykhuongkid

duykhuongkid Oct 23, 2017

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.
I found that because of JSON.Parse() can't parse the empty string. In this case, the body is string empty. So that I think we should check the Body is empty before try to parse.
Please correct me if I wrong

image

duykhuongkid commented Oct 23, 2017

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.
I found that because of JSON.Parse() can't parse the empty string. In this case, the body is string empty. So that I think we should check the Body is empty before try to parse.
Please correct me if I wrong

image

@Linskeyd

This comment has been minimized.

Show comment
Hide comment
@Linskeyd

Linskeyd Oct 23, 2017

Contributor

@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)

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.

Contributor

Linskeyd commented Oct 23, 2017

@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)

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.

@IgorMinar

This comment has been minimized.

Show comment
Hide comment
@IgorMinar

IgorMinar Oct 23, 2017

Member
Member

IgorMinar commented Oct 23, 2017

@alxhub

This comment has been minimized.

Show comment
Hide comment
@alxhub

alxhub Oct 23, 2017

Contributor

@Linskeyd @IgorMinar

I believe you're correct, it should be stripped.

Contributor

alxhub commented Oct 23, 2017

@Linskeyd @IgorMinar

I believe you're correct, it should be stripped.

@Linskeyd Linskeyd referenced this pull request Oct 25, 2017

Closed

fix(common): strip XSSI prefix for all JSON responses #19917

2 of 3 tasks complete
@Linskeyd

This comment has been minimized.

Show comment
Hide comment
@Linskeyd

Linskeyd Oct 25, 2017

Contributor

@alxhub,

Posted PR #19917 when you get a chance.

Contributor

Linskeyd commented Oct 25, 2017

@alxhub,

Posted PR #19917 when you get a chance.

} else if (!ok && req.responseType === 'json' && typeof body === 'string') {
try {
// Attempt to parse the body as JSON.
body = JSON.parse(body);

This comment has been minimized.

@kpakur

kpakur Oct 31, 2017

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

@kpakur

kpakur Oct 31, 2017

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

@IgorMinar

This comment has been minimized.

Show comment
Hide comment
@IgorMinar

IgorMinar Oct 31, 2017

Member
Member

IgorMinar commented Oct 31, 2017

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