fix(common): strip XSSI prefix for all JSON responses#19917
fix(common): strip XSSI prefix for all JSON responses#19917Linskeyd wants to merge 1 commit intoangular:masterfrom
Conversation
This fixes a regression where the common XSSI prefix used to be stripped for all json responses, including error responses.
| body = { error, text: body } as HttpJsonParseError; | ||
| } else { | ||
| // Cannot be certain that the body was meant to be parsed as JSON. | ||
| // Leave the body as a string. |
There was a problem hiding this comment.
I wonder if it is worth preserving the XSSI_PREFIX in this case. (I think it is.)
There was a problem hiding this comment.
In the event that the XSSI prefix is present, it must be removed prior to even attempting the parse (or the parse would fail). Are you suggesting that if the parse fails (after removal of the prefix, assuming it is there) that the prefix should be re-added to the body? Why would we want to preserve the prefix when we already already confirmed above that the req.responseType is 'json'?
The scenario in which this would occur would have to be malformed json that exists after the prefix is already removed. I don't see a good reason to preserve the prefix in this case.
There was a problem hiding this comment.
Even if the requests responseType is json, there is no guarantee that the returned response will be in JSON format in case of an error (which is what this branch is for).
There was a problem hiding this comment.
I agree, XSSI should be here, I just approached this issue on our app.
|
@alxhub when you get a chance to review this, it looks like the lint is failing for a commit message unrelated to mine. |
|
BTW, there is overlap between this PR and #19958. |
|
Seems like the other PR (#19958) addresses this issue with additional changes on top of the ones I've made above. Happy to close this in favor of that PR - just want to see the issue fixed. |
|
@Linskeyd indeed, I've factored this change into the other. Thank you for the PR, I based my commit off of this change. |
|
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. |
This fixes a regression where the common XSSI prefix
used to be stripped for all json responses, including
error responses.
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Submitting in response to comment on #19773
Tagging @alxhub
What is the new behavior?
The common XSSI prefix will now be stripped, even in the error response scenario.
Does this PR introduce a breaking change?
Other information