fix: Error messaging from failed login would dump a JSON.parse erro…

…r in some situations. Added a fallback if `.json` fails to parse it will attempt `.text()` then throw result. If both attempts to parse fail it will throw an `UnknownError` with a message showing where it originated. should resolve #539
cloudflare · Apr 20, 2022 · 04c5730 · 04c5730
1 parent 025c722
commit 04c5730
Show file tree

Hide file tree

Showing 4 changed files with 91 additions and 2 deletions.
diff --git a/.changeset/early-ducks-travel.md b/.changeset/early-ducks-travel.md
@@ -0,0 +1,9 @@
+---
+"wrangler": patch
+---
+
+fix: Error messaging from failed login would dump a `JSON.parse` error in some situations. Added a fallback if `.json` fails to parse
+it will attempt `.text()` then throw result. If both attempts to parse fail it will throw an `UnknownError` with a message showing where
+it originated.
+
+resolves #539
diff --git a/packages/wrangler/src/__tests__/helpers/mock-oauth-flow.ts b/packages/wrangler/src/__tests__/helpers/mock-oauth-flow.ts
@@ -105,11 +105,50 @@ export const mockOAuthFlow = () => {
     return outcome;
   };
 
+  const mockExchangeRefreshTokenForAccessToken = ({
+    respondWith,
+  }: {
+    respondWith: "refreshSuccess" | "refreshError" | "badResponse";
+  }) => {
+    fetchMock.mockOnceIf(
+      "https://dash.cloudflare.com/oauth2/token",
+      async () => {
+        switch (respondWith) {
+          case "refreshSuccess":
+            return JSON.stringify({
+              access_token: "access_token_success_mock",
+              expires_in: 1701,
+              refresh_token: "refresh_token_sucess_mock",
+              scope: "scope_success_mock",
+              token_type: "bearer",
+            });
+          case "refreshError":
+            return JSON.stringify({
+              error: "invalid_request",
+              error_description: "error_description_mock",
+              error_hint: "error_hint_mock",
+              error_verbose: "error_verbose_mock",
+              status_code: 400,
+            });
+          case "badResponse":
+            return {
+              status: 400,
+              body: `<html> <body> This shouldn't be sent, but should be handled </body> </html>`,
+            };
+
+          default:
+            return "Not a respondWith option for `mockExchangeRefreshTokenForAccessToken`";
+        }
+      }
+    );
+  };
+
   return {
     mockOAuthServerCallback,
     mockGrantAuthorization,
     mockRevokeAuthorization,
     mockGrantAccessToken,
+    mockExchangeRefreshTokenForAccessToken,
   };
 };
 

diff --git a/packages/wrangler/src/__tests__/user.test.ts b/packages/wrangler/src/__tests__/user.test.ts
@@ -4,23 +4,26 @@ import path from "node:path";
 import fetchMock from "jest-fetch-mock";
 import {
   readAuthConfigFile,
+  requireAuth,
   USER_AUTH_CONFIG_FILE,
   writeAuthConfigFile,
 } from "../user";
 import { mockConsoleMethods } from "./helpers/mock-console";
 import { mockOAuthFlow } from "./helpers/mock-oauth-flow";
 import { runInTempDir } from "./helpers/run-in-tmp";
 import { runWrangler } from "./helpers/run-wrangler";
+import type { Config } from "../config";
 import type { UserAuthConfig } from "../user";
 
-describe("wrangler", () => {
+describe("User", () => {
   runInTempDir({ homedir: "./home" });
   const std = mockConsoleMethods();
   const {
     mockOAuthServerCallback,
     mockGrantAccessToken,
     mockGrantAuthorization,
     mockRevokeAuthorization,
+    mockExchangeRefreshTokenForAccessToken,
   } = mockOAuthFlow();
 
   describe("login", () => {
@@ -82,4 +85,23 @@ describe("wrangler", () => {
       expect(fs.existsSync(config)).toBeFalsy();
     });
   });
+
+  // TODO: Improve OAuth mocking to handle `/token` endpoints from different calls
+  it("should confirm error message and output for failed login", async () => {
+    mockOAuthServerCallback();
+    writeAuthConfigFile({
+      oauth_token: "hunter2",
+      refresh_token: "Order 66",
+    });
+    mockGrantAuthorization({ respondWith: "success" });
+
+    mockExchangeRefreshTokenForAccessToken({
+      respondWith: "badResponse",
+    });
+
+    await expect(requireAuth({} as Config, false)).rejects.toThrowError();
+    expect(std.err).toContain(
+      `Error: <html> <body> This shouldn't be sent, but should be handled </body> </html>`
+    );
+  });
 });
diff --git a/packages/wrangler/src/user.tsx b/packages/wrangler/src/user.tsx
@@ -661,8 +661,27 @@ async function exchangeRefreshTokenForAccessToken(): Promise<AccessContext> {
       "Content-Type": "application/x-www-form-urlencoded",
     },
   });
+
   if (response.status >= 400) {
-    throw await response.json();
+    let tokenExchangeResErr = undefined;
+
+    try {
+      tokenExchangeResErr = await response.text();
+      tokenExchangeResErr = JSON.parse(tokenExchangeResErr);
+    } catch (e) {
+      // If it can't parse to JSON ignore the error
+    }
+
+    if (tokenExchangeResErr !== undefined) {
+      // We will throw the parsed error if it parsed correctly, otherwise we throw an unknown error.
+      throw typeof tokenExchangeResErr === "string"
+        ? new Error(tokenExchangeResErr)
+        : tokenExchangeResErr;
+    } else {
+      throw new ErrorUnknown(
+        "Failed to parse Error from exchangeRefreshTokenForAccessToken"
+      );
+    }
   } else {
     try {
       const json = (await response.json()) as TokenResponse;