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 19, 2022 · ab4ef3b · ab4ef3b
1 parent 025c722
commit ab4ef3b
Show file tree

Hide file tree

Showing 4 changed files with 94 additions and 1 deletion.
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.
+
+should resolve #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,53 @@ export const mockOAuthFlow = () => {
     return outcome;
   };
 
+  const mockExchangeRefreshTokenForAccessToken = ({
+    respondWith,
+  }: {
+    respondWith: "refreshSuccess" | "refreshError" | "badResponse";
+  }) => {
+    fetchMock.mockOnceIf(
+      (req) =>
+        (req as Request).url === "https://dash.cloudflare.com/oauth2/token" &&
+        (req as Request).headers.get("Content-Type") ===
+          "application/x-www-form-urlencoded",
+      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,13 +4,15 @@ 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", () => {
@@ -21,6 +23,7 @@ describe("wrangler", () => {
     mockGrantAccessToken,
     mockGrantAuthorization,
     mockRevokeAuthorization,
+    mockExchangeRefreshTokenForAccessToken,
   } = mockOAuthFlow();
 
   describe("login", () => {
@@ -50,6 +53,26 @@ describe("wrangler", () => {
         expiration_time: expect.any(String),
       });
     });
+
+    // 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: "jklkl",
+        // expiration_time: undefined,
+        refresh_token: "jkl",
+      });
+      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>`
+      );
+    });
   });
 
   describe("logout", () => {

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 tokenExhangeResErr = undefined;
+
+    tokenExhangeResErr = await response.text();
+    try {
+      tokenExhangeResErr = JSON.parse(tokenExhangeResErr);
+    } catch (e) {
+      // If it can't parse to JSON ignore the error
+    }
+
+    if (tokenExhangeResErr !== undefined) {
+      // We will throw the parsed error if it parsed correctly, otherwise we throw an unknown error.
+      throw typeof tokenExhangeResErr === "string"
+        ? new Error(tokenExhangeResErr)
+        : tokenExhangeResErr;
+    } else {
+      throw new ErrorUnknown(
+        "Failed to parse Error from exchangeRefreshTokenForAccessToken"
+      );
+    }
   } else {
     try {
       const json = (await response.json()) as TokenResponse;