fix: fetch: handle invalid server responses, set responseType (#329)

CHANGELOG.md

@@ -30,6 +30,8 @@
 
 - [#325](https://github.com/okta/okta-auth-js/pull/325) - Previously, the default `responseMode` for [PKCE](README.md#pkce-oauth-20-flow) was `"fragment"`. It is now `"query"`. Unless explicitly specified using the `responseMode` option, the `response_mode` parameter is no longer passed by `token.getWithRedirect` to the `/authorize` endpoint. The `response_mode` will be set by the backend according to the [OpenID specification](https://openid.net/specs/openid-connect-core-1_0.html#Authentication). [Implicit flow](README.md#implicit-oauth-20-flow) will use `"fragment"` and [PKCE](README.md#pkce-oauth-20-flow) will use `"query"`. If previous behavior is desired, [PKCE](README.md#pkce-oauth-20-flow) can set the `responseMode` option to `"fragment"`.
 
+- [#329](https://github.com/okta/okta-auth-js/pull/329) - Fix internal fetch implementation. `responseText` will always be a string, regardless of headers or response type. If a JSON object was returned, the object will be returned as `responseJSON` and `responseType` will be set to "json". Invalid/malformed JSON server response will no longer throw a raw TypeError but will return a well structured error response which includes the `status` code returned from the server.
+
 ### Other
 
 - [#306](https://github.com/okta/okta-auth-js/pull/306) - Now using babel for ES5 compatibility. [All polyfills have been removed](README.md#browser-compatibility).

packages/okta-auth-js/lib/fetch/fetchRequest.js

@@ -12,6 +12,36 @@
 
 var fetch = require('cross-fetch');
 
+
+function readData(response) {
+  if (response.headers.get('Content-Type') &&
+    response.headers.get('Content-Type').toLowerCase().indexOf('application/json') >= 0) {
+  return response.json()
+    // JSON parse can fail if response is not a valid object
+    .catch(e => {
+      return {
+        error: e,
+        errorSummary: 'Could not parse server response'
+      };
+    });
+  } else {
+    return response.text();
+  }
+}
+
+function formatResult(status, data) {
+  const isObject = typeof data === 'object'; 
+  const result = {
+    responseText: isObject ? JSON.stringify(data) : data,
+    status: status
+  };
+  if (isObject) {
+    result.responseType = 'json';
+    result.responseJSON = data;
+  }
+  return result;
+}
+
 /* eslint-disable complexity */
 function fetchRequest(method, url, args) {
   var body = args.data;
@@ -32,23 +62,17 @@ function fetchRequest(method, url, args) {
   .then(function(response) {
     var error = !response.ok;
     var status = response.status;
-    var respHandler = function(resp) {
-      var result = {
-        responseText: resp,
-        status: status
-      };
-      if (error) {
-        // Throwing response object since error handling is done in http.js
-        throw result;
-      }
-      return result;
-    };
-    if (response.headers.get('Content-Type') &&
-        response.headers.get('Content-Type').toLowerCase().indexOf('application/json') >= 0) {
-      return response.json().then(respHandler);
-    } else {
-      return response.text().then(respHandler);
-    }
+    return readData(response)
+      .then(data => {
+        return formatResult(status, data);
+      })
+      .then(result => {
+        if (error) {
+          // Throwing result object since error handling is done in http.js
+          throw result;
+        }
+        return result;
+      });
   });
   return fetchPromise;
 }

packages/okta-auth-js/test/spec/fetch-request.js

@@ -1,86 +1,178 @@
+/* global Map */
 describe('fetchRequest', function () {
-  var mockFetchResult;
-  var mockFetchObj = {
+  let fetchSpy;
+
+  let requestHeaders;
+  let requestMethod;
+  let requestUrl;
+  let response;
+  let responseHeaders;
+  let responseJSON;
+  let responseText;
+
+  const mockFetchObj = {
     fetch: function mockFetchFunc() {
-      return Promise.resolve(mockFetchResult);
+      return Promise.resolve(response);
     }
   }
   jest.setMock('cross-fetch', function() {
     return mockFetchObj.fetch.apply(null, arguments);
   });
-
-  var fetchRequest = require('../../lib/fetch/fetchRequest');
+  const fetchRequest = require('../../lib/fetch/fetchRequest');
 
   beforeEach(function() {
-    /* global Map */
-    mockFetchResult = {
-      headers: new Map(),
+    fetchSpy = jest.spyOn(mockFetchObj, 'fetch');
+    responseHeaders = new Map();
+    responseHeaders.set('Content-Type', 'application/json');
+    responseJSON = { isFakeResponse: true };
+    responseText = JSON.stringify(responseJSON);
+    response = {
+      headers: responseHeaders,
+      status: 200,
+      ok: true,
       json: function() {
-        return Promise.resolve();
+        return Promise.resolve(responseJSON);
       },
       text: function() {
-        return Promise.resolve();
+        return Promise.resolve(responseText);
       }
+    };
+
+    requestHeaders = {
+      'Accept': 'application/json',
+      'Content-Type': 'application/json',
     }
+    requestMethod = 'GET';
+    requestUrl = 'http://fakey.local';
   });
 
-  it('JSON encodes request body if request Content-Type is application/json', function() {
-    var spy = jest.spyOn(mockFetchObj, 'fetch');
-    var method = 'GET';
-    var url = 'http://fakey.local';
-    var headers = {
-      'Content-Type': 'application/json'
-    };
-    var obj = {
-      foo: 'bar'
-    };
-    var jsonObj = JSON.stringify(obj);
+  describe('request', () => {
+    it('JSON encodes request body if request header Content-Type is application/json', function() {
+      const requestJSON = {
+        foo: 'bar'
+      };
+      return fetchRequest(requestMethod, requestUrl, {
+        headers: requestHeaders,
+        data: requestJSON
+      })
+      .then(() => {
+        expect(fetchSpy).toHaveBeenCalledWith(requestUrl, {
+          method: requestMethod,
+          headers: requestHeaders,
+          body: JSON.stringify(requestJSON),
+          credentials: 'include'
+        });
+      });
+    });
 
-    fetchRequest(method, url, {
-      headers: headers,
-      data: obj
+    it('Leaves request body unchanged if request header Content-Type is NOT application/json', function() {
+      requestHeaders = {
+        'Accept': 'application/json',
+        'Content-Type': 'application/x-www-form-urlencoded',
+      };
+      const requestText = 'string=1&fake=2';
+      return fetchRequest(requestMethod, requestUrl, {
+        headers: requestHeaders,
+        data: requestText
+      })
+      .then(() => {
+        expect(fetchSpy).toHaveBeenCalledWith(requestUrl, {
+          method: requestMethod,
+          headers: requestHeaders,
+          body: requestText,
+          credentials: 'include'
+        });
+      });
     });
 
-    expect(spy).toHaveBeenCalledWith(url, {
-      method: method,
-      headers: headers,
-      body: jsonObj,
-      credentials: 'include'
+
+    it('Can omit credentials', function() {
+      return fetchRequest(requestMethod, requestUrl, {
+        withCredentials: false
+      })
+      .then(() => {
+        expect(fetchSpy).toHaveBeenCalledWith(requestUrl, {
+          method: requestMethod,
+          credentials: 'omit'
+        });
+      });
     });
   });
 
-  it('Leaves request body unchanged if request Content-Type is NOT application/json', function() {
-    var spy = jest.spyOn(mockFetchObj, 'fetch');
-    var method = 'GET';
-    var url = 'http://fakey.local';
-    var obj = {
-      foo: 'bar'
-    };
+  describe('response', () => {
 
-    fetchRequest(method, url, {
-      data: obj
+    it('Returns JSON if response header Content-Type is application/json', function() {
+      return fetchRequest(requestMethod, requestUrl, {})
+      .then(res => {
+        expect(res).toEqual({
+          status: response.status,
+          responseJSON,
+          responseText,
+          responseType: 'json'
+        });
+      });
     });
 
-    expect(spy).toHaveBeenCalledWith(url, {
-      method: method,
-      body: obj,
-      credentials: 'include'
+    it('Returns text if response header Content-Type is NOT application/json', function() {
+      responseHeaders.set('Content-Type', 'application/x-www-form-urlencoded');
+      return fetchRequest(requestMethod, requestUrl, {})
+      .then(res => {
+        expect(res).toEqual({
+          status: response.status,
+          responseText
+        });
+      });
     });
-  });
 
-  it('Can omit credentials', function() {
-    var spy = jest.spyOn(mockFetchObj, 'fetch');
-    var method = 'GET';
-    var url = 'http://fakey.local';
+    it('Throws the response if response.ok is false (JSON)', () => {
+      response.status = 401;
+      response.ok = false;
+      return fetchRequest(requestMethod, requestUrl, {})
+      .catch(err => {
+        expect(err).toEqual({
+          status: response.status,
+          responseText,
+          responseType: 'json',
+          responseJSON
+        });
+      });
+    });
 
-    fetchRequest(method, url, {
-      withCredentials: false
+    it('Throws the response if response.ok is false (text)', () => {
+      response.status = 401;
+      response.ok = false;
+      responseHeaders.set('Content-Type', 'application/x-www-form-urlencoded');
+      return fetchRequest(requestMethod, requestUrl, {})
+      .catch(err => {
+        expect(err).toEqual({
+          status: response.status,
+          responseText
+        });
+      });
     });
 
-    expect(spy).toHaveBeenCalledWith(url, {
-      method: method,
-      credentials: 'omit'
+    it('Throws the response if response.ok is false (invalid JSON)', () => {
+      var error = new Error('A fake error, ignore me');
+      response.status = 401;
+      response.ok = false;
+      response.json = function() {
+        return Promise.reject(error);
+      };
+
+      var errorJSON = {
+        error: error,
+        errorSummary: 'Could not parse server response'
+      };
+
+      return fetchRequest(requestMethod, requestUrl, {})
+      .catch(err => {
+        expect(err).toEqual({
+          status: response.status,
+          responseText: JSON.stringify(errorJSON),
+          responseJSON: errorJSON,
+          responseType: 'json'
+        });
+      });
     });
   });
-
 });