From 452a7ae88b3b1144369f884c10de044502dc3c65 Mon Sep 17 00:00:00 2001 From: Alex Rickabaugh Date: Tue, 1 Aug 2017 11:46:26 -0700 Subject: [PATCH] fix(common): fix XSSI prefix stripping by using JSON.parse always (#18466) 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 | 18 ++++++++++-------- packages/common/http/test/xhr_mock.ts | 4 ++-- packages/common/http/test/xhr_spec.ts | 18 +++++++++++++----- 3 files changed, 25 insertions(+), 15 deletions(-) diff --git a/packages/common/http/src/xhr.ts b/packages/common/http/src/xhr.ts index 4cbce48281..f03f87825d 100644 --- a/packages/common/http/src/xhr.ts +++ b/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) { diff --git a/packages/common/http/test/xhr_mock.ts b/packages/common/http/test/xhr_mock.ts index 743f3526a0..eceb729ebf 100644 --- a/packages/common/http/test/xhr_mock.ts +++ b/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; diff --git a/packages/common/http/test/xhr_spec.ts b/packages/common/http/test/xhr_spec.ts index fc9dff6422..6c37619333 100644 --- a/packages/common/http/test/xhr_spec.ts +++ b/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; + 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'); }); }); });