From ee3475fc4204335f3659e9a045524e8dc9dc6b2c Mon Sep 17 00:00:00 2001 From: Jacob M-G Evans <27247160+JacobMGEvans@users.noreply.github.com> Date: Wed, 20 Apr 2022 10:53:13 -0500 Subject: [PATCH] fix: Error messaging from failed login would dump a `JSON.parse` error in some situations. Added a fallback if `.json` fails to parse (#794) 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 --- .changeset/early-ducks-travel.md | 9 ++++ .../src/__tests__/helpers/mock-oauth-flow.ts | 45 +++++++++++++++++++ packages/wrangler/src/__tests__/user.test.ts | 42 ++++++++++++++++- packages/wrangler/src/user.tsx | 21 ++++++++- 4 files changed, 115 insertions(+), 2 deletions(-) create mode 100644 .changeset/early-ducks-travel.md diff --git a/.changeset/early-ducks-travel.md b/.changeset/early-ducks-travel.md new file mode 100644 index 000000000000..ba4eeeec5318 --- /dev/null +++ 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 index e5d3a44ce9ef..c037e02046e1 100644 --- a/packages/wrangler/src/__tests__/helpers/mock-oauth-flow.ts +++ b/packages/wrangler/src/__tests__/helpers/mock-oauth-flow.ts @@ -105,11 +105,56 @@ 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 { + status: 200, + body: 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 { + status: 400, + body: 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: ` This shouldn't be sent, but should be handled `, + }; + + 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 index b4b5190fcc4a..e7d12ad0f349 100644 --- a/packages/wrangler/src/__tests__/user.test.ts +++ b/packages/wrangler/src/__tests__/user.test.ts @@ -4,6 +4,7 @@ import path from "node:path"; import fetchMock from "jest-fetch-mock"; import { readAuthConfigFile, + requireAuth, USER_AUTH_CONFIG_FILE, writeAuthConfigFile, } from "../user"; @@ -11,9 +12,10 @@ 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 { @@ -21,6 +23,7 @@ describe("wrangler", () => { mockGrantAccessToken, mockGrantAuthorization, mockRevokeAuthorization, + mockExchangeRefreshTokenForAccessToken, } = mockOAuthFlow(); describe("login", () => { @@ -82,4 +85,41 @@ describe("wrangler", () => { expect(fs.existsSync(config)).toBeFalsy(); }); }); + + // TODO: Improve OAuth mocking to handle `/token` endpoints from different calls + it("should report error message for failed token refresh", async () => { + mockOAuthServerCallback(); + writeAuthConfigFile({ + oauth_token: "hunter2", + refresh_token: "Order 66", + }); + mockGrantAuthorization({ respondWith: "success" }); + + mockExchangeRefreshTokenForAccessToken({ + respondWith: "badResponse", + }); + + // Handles the requireAuth error throw from failed login that is unhandled due to directly calling it here + await expect(requireAuth({} as Config, false)).rejects.toThrowError(); + expect(std.err).toContain( + `Error: This shouldn't be sent, but should be handled ` + ); + }); + + it("should confirm no error message when refresh is successful", async () => { + mockOAuthServerCallback(); + writeAuthConfigFile({ + oauth_token: "hunter2", + refresh_token: "Order 66", + }); + mockGrantAuthorization({ respondWith: "success" }); + + mockExchangeRefreshTokenForAccessToken({ + respondWith: "refreshSuccess", + }); + + // Handles the requireAuth error throw from failed login that is unhandled due to directly calling it here + await expect(requireAuth({} as Config, false)).rejects.toThrowError(); + expect(std.err).toContain(""); + }); }); diff --git a/packages/wrangler/src/user.tsx b/packages/wrangler/src/user.tsx index 4617fd9269fc..325bb1d8023d 100644 --- a/packages/wrangler/src/user.tsx +++ b/packages/wrangler/src/user.tsx @@ -661,8 +661,27 @@ async function exchangeRefreshTokenForAccessToken(): Promise { "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;