fix(common): fix XSSI prefix stripping by using JSON.parse always (#1…

…8466)

Currently HttpClient sends requests for JSON data with the
XMLHttpRequest.responseType set to 'json'. With this flag, the browser
will attempt to parse the response as JSON, but will return 'null' on
any errors. If the JSON response contains an XSSI-prevention prefix,
this will cause the browser's parsing to fail, which is unrecoverable.

The only compelling reason to use the responseType 'json' is for
performance (especially if the browser offloads JSON parsing to a
separate thread). I'm not aware of any browser which does this currently,
nor of any plans to do so. JSON.parse and responseType 'json' both
end up using the same V8 code path in Chrome to implement the parse.

Thus, this change switches all JSON parsing in HttpClient to use
JSON.parse directly.

Fixes #18396, #18453.

PR Close #18466

packages/common/http/src/xhr.ts

@@ -107,7 +107,14 @@ export class HttpXhrBackend implements HttpBackend {
 
       // Set the responseType if one was requested.
       if (req.responseType) {
-        xhr.responseType = req.responseType.toLowerCase() as any;
+        const responseType = req.responseType.toLowerCase();
+
+        // JSON responses need to be processed as text. This is because if the server
+        // returns an XSSI-prefixed JSON response, the browser will fail to parse it,
+        // xhr.response will be null, and xhr.responseText cannot be accessed to
+        // retrieve the prefixed JSON data in order to strip the prefix. Thus, all JSON
+        // is parsed by first requesting text and then applying JSON.parse.
+        xhr.responseType = ((responseType !== 'json') ? responseType : 'text') as any;
       }
 
       // Serialize the request body if one is present. If not, this will be set to null.
@@ -158,12 +165,6 @@ export class HttpXhrBackend implements HttpBackend {
         if (status !== 204) {
           // Use XMLHttpRequest.response if set, responseText otherwise.
           body = (typeof xhr.response === 'undefined') ? xhr.responseText : xhr.response;
-
-          // Strip a common XSSI prefix from string responses.
-          // TODO: determine if this behavior should be optional and moved to an interceptor.
-          if (typeof body === 'string') {
-            body = body.replace(XSSI_PREFIX, '');
-          }
         }
 
         // Normalize another potential bug (this one comes from CORS).
@@ -179,8 +180,9 @@ export class HttpXhrBackend implements HttpBackend {
 
         // Check whether the body needs to be parsed as JSON (in many cases the browser
         // will have done that already).
-        if (ok && typeof body === 'string' && req.responseType === 'json') {
+        if (ok && req.responseType === 'json' && typeof body === 'string') {
           // Attempt the parse. If it fails, a parse error should be delivered to the user.
+          body = body.replace(XSSI_PREFIX, '');
           try {
             body = JSON.parse(body);
           } catch (error) {

packages/common/http/test/xhr_mock.ts

@@ -79,8 +79,8 @@ export class MockXMLHttpRequest {
     return new HttpHeaders(this.mockResponseHeaders).get(header);
   }
 
-  mockFlush(status: number, statusText: string, body: any|null) {
-    if (this.responseType === 'text') {
+  mockFlush(status: number, statusText: string, body?: string) {
+    if (typeof body === 'string') {
       this.responseText = body;
     } else {
       this.response = body;

packages/common/http/test/xhr_spec.ts

@@ -6,7 +6,7 @@
  * found in the LICENSE file at https://angular.io/license
  */
 
-import {ddescribe, describe, it} from '@angular/core/testing/src/testing_internal';
+import {ddescribe, describe, iit, it} from '@angular/core/testing/src/testing_internal';
 import {Observable} from 'rxjs/Observable';
 
 import {HttpRequest} from '../src/request';
@@ -87,14 +87,22 @@ export function main() {
     });
     it('handles a json response', () => {
       const events = trackEvents(backend.handle(TEST_POST.clone({responseType: 'json'})));
-      factory.mock.mockFlush(200, 'OK', {data: 'some data'});
+      factory.mock.mockFlush(200, 'OK', JSON.stringify({data: 'some data'}));
       expect(events.length).toBe(2);
       const res = events[1] as HttpResponse<{data: string}>;
       expect(res.body !.data).toBe('some data');
     });
-    it('handles a json response that comes via responseText', () => {
+    it('handles a json string response', () => {
       const events = trackEvents(backend.handle(TEST_POST.clone({responseType: 'json'})));
-      factory.mock.mockFlush(200, 'OK', JSON.stringify({data: 'some data'}));
+      expect(factory.mock.responseType).toEqual('text');
+      factory.mock.mockFlush(200, 'OK', JSON.stringify('this is a string'));
+      expect(events.length).toBe(2);
+      const res = events[1] as HttpResponse<string>;
+      expect(res.body).toEqual('this is a string');
+    });
+    it('handles a json response with an XSSI prefix', () => {
+      const events = trackEvents(backend.handle(TEST_POST.clone({responseType: 'json'})));
+      factory.mock.mockFlush(200, 'OK', ')]}\'\n' + JSON.stringify({data: 'some data'}));
       expect(events.length).toBe(2);
       const res = events[1] as HttpResponse<{data: string}>;
       expect(res.body !.data).toBe('some data');
@@ -299,7 +307,7 @@ export function main() {
           expect(error.status).toBe(0);
           done();
         });
-        factory.mock.mockFlush(0, 'CORS 0 status', null);
+        factory.mock.mockFlush(0, 'CORS 0 status');
       });
     });
   });