diff --git a/.changeset/eighty-yaks-jump.md b/.changeset/eighty-yaks-jump.md new file mode 100644 index 000000000000..4a06c8de5fe0 --- /dev/null +++ b/.changeset/eighty-yaks-jump.md @@ -0,0 +1,9 @@ +--- +"wrangler": patch +--- + +feat: non-TTY check for required variables +Added a check in non-TTY environments for `account_id`, `CLOUDFLARE_ACCOUNT_ID` and `CLOUDFLARE_API_TOKEN`. If `account_id` exists in `wrangler.toml` +then `CLOUDFLARE_ACCOUNT_ID` is not needed in non-TTY scope. The `CLOUDFLARE_API_TOKEN` is necessary in non-TTY scope and will always error if missing. + +resolves #827 diff --git a/packages/wrangler/src/__tests__/helpers/mock-istty.ts b/packages/wrangler/src/__tests__/helpers/mock-istty.ts index 77c66d6d76f7..4fca67a6ef55 100644 --- a/packages/wrangler/src/__tests__/helpers/mock-istty.ts +++ b/packages/wrangler/src/__tests__/helpers/mock-istty.ts @@ -1,4 +1,5 @@ -const ORIGINAL_ISTTY = process.stdout.isTTY; +const ORIGINAL_STDOUT_ISTTY = process.stdout.isTTY; +const ORIGINAL_STDIN_ISTTY = process.stdin.isTTY; /** * Mock `process.stdout.isTTY` @@ -9,14 +10,17 @@ export function useMockIsTTY() { */ const setIsTTY = (isTTY: boolean) => { process.stdout.isTTY = isTTY; + process.stdin.isTTY = isTTY; }; beforeEach(() => { - process.stdout.isTTY = ORIGINAL_ISTTY; + process.stdout.isTTY = ORIGINAL_STDOUT_ISTTY; + process.stdin.isTTY = ORIGINAL_STDIN_ISTTY; }); afterEach(() => { - process.stdout.isTTY = ORIGINAL_ISTTY; + process.stdout.isTTY = ORIGINAL_STDOUT_ISTTY; + process.stdin.isTTY = ORIGINAL_STDIN_ISTTY; }); return { setIsTTY }; diff --git a/packages/wrangler/src/__tests__/helpers/mock-oauth-flow.ts b/packages/wrangler/src/__tests__/helpers/mock-oauth-flow.ts index c037e02046e1..a6c3f6c6fc88 100644 --- a/packages/wrangler/src/__tests__/helpers/mock-oauth-flow.ts +++ b/packages/wrangler/src/__tests__/helpers/mock-oauth-flow.ts @@ -1,5 +1,6 @@ import fetchMock from "jest-fetch-mock"; import { Request } from "undici"; +import { getCloudflareApiBaseUrl } from "../../cfetch"; import openInBrowser from "../../open-in-browser"; import { mockHttpServer } from "./mock-http-server"; @@ -85,6 +86,28 @@ export const mockOAuthFlow = () => { return outcome; }; + const mockGetMemberships = (args: { + success: boolean; + result: { id: string; account: { id: string; name: string } }[]; + }) => { + const outcome = { + actual: new Request("https://example.org"), + expected: new Request(`${getCloudflareApiBaseUrl()}/memberships`, { + method: "GET", + }), + }; + + fetchMock.mockIf(outcome.expected.url, async (req) => { + outcome.actual = req; + return { + status: 200, + body: JSON.stringify(args), + }; + }); + + return outcome; + }; + const mockGrantAccessToken = ({ respondWith, }: { @@ -150,10 +173,11 @@ export const mockOAuthFlow = () => { }; return { - mockOAuthServerCallback, + mockGetMemberships, + mockGrantAccessToken, mockGrantAuthorization, + mockOAuthServerCallback, mockRevokeAuthorization, - mockGrantAccessToken, mockExchangeRefreshTokenForAccessToken, }; }; diff --git a/packages/wrangler/src/__tests__/publish.test.ts b/packages/wrangler/src/__tests__/publish.test.ts index da7903d62423..2eae5bdf3be6 100644 --- a/packages/wrangler/src/__tests__/publish.test.ts +++ b/packages/wrangler/src/__tests__/publish.test.ts @@ -11,6 +11,7 @@ import { unsetMockFetchKVGetValues, } from "./helpers/mock-cfetch"; import { mockConsoleMethods, normalizeSlashes } from "./helpers/mock-console"; +import { useMockIsTTY } from "./helpers/mock-istty"; import { mockKeyListRequest } from "./helpers/mock-kv"; import { mockOAuthFlow } from "./helpers/mock-oauth-flow"; import { runInTempDir } from "./helpers/run-in-tmp"; @@ -27,11 +28,13 @@ describe("publish", () => { mockAccountId(); mockApiToken(); runInTempDir({ homedir: "./home" }); + const { setIsTTY } = useMockIsTTY(); const std = mockConsoleMethods(); const { mockOAuthServerCallback, mockGrantAccessToken, mockGrantAuthorization, + mockGetMemberships, } = mockOAuthFlow(); beforeEach(() => { @@ -39,6 +42,7 @@ describe("publish", () => { jest.spyOn(global, "setTimeout").mockImplementation((fn, _period) => { setImmediate(fn); }); + setIsTTY(true); }); afterEach(() => { @@ -116,6 +120,150 @@ describe("publish", () => { `); expect(std.err).toMatchInlineSnapshot(`""`); }); + + describe("non-TTY", () => { + const ENV_COPY = process.env; + + afterEach(() => { + process.env = ENV_COPY; + }); + + it("should not throw an error in non-TTY if 'CLOUDFLARE_API_TOKEN' & 'account_id' are in scope", async () => { + process.env = { + CLOUDFLARE_API_TOKEN: "123456789", + }; + setIsTTY(false); + writeWranglerToml({ + account_id: "some-account-id", + }); + writeWorkerSource(); + mockSubDomainRequest(); + mockUploadWorkerRequest(); + mockOAuthServerCallback(); + + await runWrangler("publish index.js"); + + expect(std.out).toMatchInlineSnapshot(` + "Uploaded test-name (TIMINGS) + Published test-name (TIMINGS) + test-name.test-sub-domain.workers.dev" + `); + expect(std.err).toMatchInlineSnapshot(`""`); + }); + + it("should not throw an error if 'CLOUDFLARE_ACCOUNT_ID' & 'CLOUDFLARE_API_TOKEN' are in scope", async () => { + process.env = { + CLOUDFLARE_API_TOKEN: "hunter2", + CLOUDFLARE_ACCOUNT_ID: "some-account-id", + }; + setIsTTY(false); + writeWranglerToml(); + writeWorkerSource(); + mockSubDomainRequest(); + mockUploadWorkerRequest(); + mockOAuthServerCallback(); + mockGetMemberships({ + success: true, + result: [], + }); + + await runWrangler("publish index.js"); + + expect(std.out).toMatchInlineSnapshot(` + "Uploaded test-name (TIMINGS) + Published test-name (TIMINGS) + test-name.test-sub-domain.workers.dev" + `); + expect(std.err).toMatchInlineSnapshot(`""`); + }); + + it("should throw an error in non-TTY & there is more than one account associated with API token", async () => { + setIsTTY(false); + process.env = { + CLOUDFLARE_API_TOKEN: "hunter2", + CLOUDFLARE_ACCOUNT_ID: undefined, + }; + writeWranglerToml({ + account_id: undefined, + }); + writeWorkerSource(); + mockSubDomainRequest(); + mockUploadWorkerRequest(); + mockOAuthServerCallback(); + mockGetMemberships({ + success: true, + result: [ + { id: "IG-88", account: { id: "1701", name: "enterprise" } }, + { id: "R2-D2", account: { id: "nx01", name: "enterprise-nx" } }, + ], + }); + + await expect(runWrangler("publish index.js")).rejects + .toMatchInlineSnapshot(` + [Error: More than one account available but unable to select one in non-interactive mode. + Please set the appropriate \`account_id\` in your \`wrangler.toml\` file. + Available accounts are ("" - ""): + "enterprise" - "1701") + "enterprise-nx" - "nx01")] + `); + }); + + it("should throw error in non-TTY if 'CLOUDFLARE_API_TOKEN' is missing", async () => { + setIsTTY(false); + writeWranglerToml({ + account_id: undefined, + }); + process.env = { + CLOUDFLARE_API_TOKEN: undefined, + CLOUDFLARE_ACCOUNT_ID: "badwolf", + }; + writeWorkerSource(); + mockSubDomainRequest(); + mockUploadWorkerRequest(); + mockOAuthServerCallback(); + mockGetMemberships({ + success: true, + result: [ + { id: "IG-88", account: { id: "1701", name: "enterprise" } }, + { id: "R2-D2", account: { id: "nx01", name: "enterprise-nx" } }, + ], + }); + + await expect(runWrangler("publish index.js")).rejects.toThrowError(); + + expect(std.err).toMatchInlineSnapshot(` + "X [ERROR] In a non-interactive environment, it's necessary to set a CLOUDFLARE_API_TOKEN environment variable for wrangler to work. Please go to https://developers.cloudflare.com/api/tokens/create/ for instructions on how to create an api token, and assign its value to CLOUDFLARE_API_TOKEN. + + " + `); + }); + it("should throw error with no account ID provided and no members retrieved", async () => { + setIsTTY(false); + writeWranglerToml({ + account_id: undefined, + }); + process.env = { + CLOUDFLARE_API_TOKEN: "picard", + CLOUDFLARE_ACCOUNT_ID: undefined, + }; + writeWorkerSource(); + mockSubDomainRequest(); + mockUploadWorkerRequest(); + mockOAuthServerCallback(); + mockGetMemberships({ + success: false, + result: [], + }); + + await expect(runWrangler("publish index.js")).rejects.toThrowError(); + + expect(std.err).toMatchInlineSnapshot(` + "X [ERROR] Failed to automatically retrieve account IDs for the logged in user. In a non-interactive environment, it is mandatory to specify an account ID, either by assigning its value to CLOUDFLARE_ACCOUNT_ID, or as \`account_id\` in your \`wrangler.toml\` file. + + " + `); + }); + }); }); describe("environments", () => { diff --git a/packages/wrangler/src/__tests__/secret.test.ts b/packages/wrangler/src/__tests__/secret.test.ts index ab7a14bfe6ba..e7ca316eeedf 100644 --- a/packages/wrangler/src/__tests__/secret.test.ts +++ b/packages/wrangler/src/__tests__/secret.test.ts @@ -1,16 +1,18 @@ import * as fs from "node:fs"; import * as TOML from "@iarna/toml"; -import fetchMock from "jest-fetch-mock"; import { mockAccountId, mockApiToken } from "./helpers/mock-account-id"; import { setMockResponse, unsetAllMocks } from "./helpers/mock-cfetch"; import { mockConsoleMethods } from "./helpers/mock-console"; import { mockConfirm, mockPrompt } from "./helpers/mock-dialogs"; +import { mockOAuthFlow } from "./helpers/mock-oauth-flow"; import { useMockStdin } from "./helpers/mock-stdin"; import { runInTempDir } from "./helpers/run-in-tmp"; import { runWrangler } from "./helpers/run-wrangler"; describe("wrangler secret", () => { const std = mockConsoleMethods(); + const { mockGetMemberships } = mockOAuthFlow(); + runInTempDir(); mockAccountId(); mockApiToken(); @@ -169,10 +171,14 @@ describe("wrangler secret", () => { mockAccountId({ accountId: null }); it("should error if a user has no account", async () => { + mockGetMemberships({ + success: false, + result: [], + }); await expect( runWrangler("secret put the-key --name script-name") ).rejects.toThrowErrorMatchingInlineSnapshot( - `"No account id found, quitting..."` + `"Failed to automatically retrieve account IDs for the logged in user. In a non-interactive environment, it is mandatory to specify an account ID, either by assigning its value to CLOUDFLARE_ACCOUNT_ID, or as \`account_id\` in your \`wrangler.toml\` file."` ); }); @@ -196,28 +202,24 @@ describe("wrangler secret", () => { }); it("should error if a user has multiple accounts, and has not specified an account in wrangler.toml", async () => { - // This is a mock response for the request to the CF API memberships of the current user. - fetchMock.doMockOnce(async () => { - return { - body: JSON.stringify({ - success: true, - result: [ - { - id: "1", - account: { id: "account-id-1", name: "account-name-1" }, - }, - { - id: "2", - account: { id: "account-id-2", name: "account-name-2" }, - }, - { - id: "3", - account: { id: "account-id-3", name: "account-name-3" }, - }, - ], - }), - }; + mockGetMemberships({ + success: true, + result: [ + { + id: "1", + account: { id: "account-id-1", name: "account-name-1" }, + }, + { + id: "2", + account: { id: "account-id-2", name: "account-name-2" }, + }, + { + id: "3", + account: { id: "account-id-3", name: "account-name-3" }, + }, + ], }); + await expect(runWrangler("secret put the-key --name script-name")) .rejects.toThrowErrorMatchingInlineSnapshot(` "More than one account available but unable to select one in non-interactive mode. diff --git a/packages/wrangler/src/__tests__/sentry.test.ts b/packages/wrangler/src/__tests__/sentry.test.ts index bb5f3ed5126f..4e10bba7abf1 100644 --- a/packages/wrangler/src/__tests__/sentry.test.ts +++ b/packages/wrangler/src/__tests__/sentry.test.ts @@ -7,26 +7,27 @@ import * as Sentry from "@sentry/node"; import prompts from "prompts"; import { mockConsoleMethods } from "./helpers/mock-console"; +import { useMockIsTTY } from "./helpers/mock-istty"; import { runInTempDir } from "./helpers/run-in-tmp"; import { runWrangler } from "./helpers/run-wrangler"; const { reportError } = jest.requireActual("../reporting"); describe("Error Reporting", () => { + const { setIsTTY } = useMockIsTTY(); + runInTempDir({ homedir: "./home" }); mockConsoleMethods(); const reportingTOMLPath = ".wrangler/config/reporting.toml"; - const originalTTY = process.stdout.isTTY; beforeEach(() => { jest.mock("@sentry/node"); jest.spyOn(Sentry, "captureException"); - process.stdout.isTTY = true; + setIsTTY(true); }); afterEach(() => { jest.unmock("@sentry/node"); jest.clearAllMocks(); - process.stdout.isTTY = originalTTY; }); it("should confirm user will allow error reporting usage", async () => { @@ -127,20 +128,15 @@ describe("Error Reporting", () => { }); it("should not prompt in non-TTY environment", async () => { - process.stdout.isTTY = false; - + setIsTTY(false); await reportError(new Error("test error"), "testFalse"); - const { error_tracking_opt, error_tracking_opt_date } = TOML.parse( - await fsp.readFile(path.join(os.homedir(), reportingTOMLPath), "utf-8") - ); - - expect(error_tracking_opt).toBe(false); - expect(error_tracking_opt_date).toBeTruthy(); + expect( + fs.existsSync(path.join(os.homedir(), reportingTOMLPath, "utf-8")) + ).toBe(false); expect(Sentry.captureException).not.toHaveBeenCalledWith( new Error("test error") ); - process.stdout.isTTY = originalTTY; }); }); diff --git a/packages/wrangler/src/__tests__/user.test.ts b/packages/wrangler/src/__tests__/user.test.ts index 44a25e4db664..cf3354c629c1 100644 --- a/packages/wrangler/src/__tests__/user.test.ts +++ b/packages/wrangler/src/__tests__/user.test.ts @@ -9,6 +9,7 @@ import { writeAuthConfigFile, } from "../user"; import { mockConsoleMethods } from "./helpers/mock-console"; +import { useMockIsTTY } from "./helpers/mock-istty"; import { mockOAuthFlow } from "./helpers/mock-oauth-flow"; import { runInTempDir } from "./helpers/run-in-tmp"; import { runWrangler } from "./helpers/run-wrangler"; @@ -26,6 +27,8 @@ describe("User", () => { mockExchangeRefreshTokenForAccessToken, } = mockOAuthFlow(); + const { setIsTTY } = useMockIsTTY(); + describe("login", () => { it("should login a user when `wrangler login` is run", async () => { mockOAuthServerCallback(); @@ -88,6 +91,7 @@ describe("User", () => { // TODO: Improve OAuth mocking to handle `/token` endpoints from different calls it("should handle errors for failed token refresh", async () => { + setIsTTY(false); mockOAuthServerCallback(); writeAuthConfigFile({ oauth_token: "hunter2", @@ -101,13 +105,14 @@ describe("User", () => { // Handles the requireAuth error throw from failed login that is unhandled due to directly calling it here await expect( - requireAuth({} as Config, false) + requireAuth({} as Config) ).rejects.toThrowErrorMatchingInlineSnapshot( `"Did not login, quitting..."` ); }); it("should confirm no error message when refresh is successful", async () => { + setIsTTY(false); mockOAuthServerCallback(); writeAuthConfigFile({ oauth_token: "hunter2", @@ -120,7 +125,7 @@ describe("User", () => { }); // 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(); + await expect(requireAuth({} as Config)).rejects.toThrowError(); expect(std.err).toContain(""); }); }); diff --git a/packages/wrangler/src/__tests__/whoami.test.tsx b/packages/wrangler/src/__tests__/whoami.test.tsx index 719ba381c2e5..aa79a0e1227b 100644 --- a/packages/wrangler/src/__tests__/whoami.test.tsx +++ b/packages/wrangler/src/__tests__/whoami.test.tsx @@ -4,12 +4,18 @@ import { writeAuthConfigFile } from "../user"; import { getUserInfo, WhoAmI } from "../whoami"; import { setMockResponse } from "./helpers/mock-cfetch"; import { mockConsoleMethods } from "./helpers/mock-console"; +import { useMockIsTTY } from "./helpers/mock-istty"; import { runInTempDir } from "./helpers/run-in-tmp"; import type { UserInfo } from "../whoami"; describe("getUserInfo()", () => { runInTempDir({ homedir: "./home" }); const std = mockConsoleMethods(); + const { setIsTTY } = useMockIsTTY(); + + beforeEach(() => { + setIsTTY(true); + }); it("should return undefined if there is no config file", async () => { const userInfo = await getUserInfo(); diff --git a/packages/wrangler/src/index.tsx b/packages/wrangler/src/index.tsx index cb58c86932df..842495efaf0b 100644 --- a/packages/wrangler/src/index.tsx +++ b/packages/wrangler/src/index.tsx @@ -1485,9 +1485,9 @@ export async function main(argv: string[]): Promise { throw new Error("Missing script name"); } - const isInteractive = process.stdin.isTTY; - const accountId = await requireAuth(config, isInteractive); + const accountId = await requireAuth(config); + const isInteractive = process.stdin.isTTY; const secretValue = isInteractive ? await prompt("Enter a secret value:", "password") : await readFromStdin(); diff --git a/packages/wrangler/src/pages.tsx b/packages/wrangler/src/pages.tsx index 3986dd77e6ba..ff44b609ce66 100644 --- a/packages/wrangler/src/pages.tsx +++ b/packages/wrangler/src/pages.tsx @@ -842,11 +842,11 @@ const createDeployment: CommandModule< const config = getConfigCache( PAGES_CONFIG_CACHE_FILENAME ); - const isInteractive = process.stdin.isTTY; - const accountId = await requireAuth(config, isInteractive); + const accountId = await requireAuth(config); projectName ??= config.project_name; + const isInteractive = process.stdin.isTTY; if (!projectName && isInteractive) { const existingOrNew = await new Promise<"new" | "existing">((resolve) => { const { unmount } = render( @@ -1605,8 +1605,8 @@ export const pages: BuilderCallback = (yargs) => { const config = getConfigCache( PAGES_CONFIG_CACHE_FILENAME ); - const isInteractive = process.stdin.isTTY; - const accountId = await requireAuth(config, isInteractive); + + const accountId = await requireAuth(config); const projects: Array = await listProjects({ accountId }); @@ -1650,9 +1650,9 @@ export const pages: BuilderCallback = (yargs) => { const config = getConfigCache( PAGES_CONFIG_CACHE_FILENAME ); - const isInteractive = process.stdin.isTTY; - const accountId = await requireAuth(config, isInteractive); + const accountId = await requireAuth(config); + const isInteractive = process.stdin.isTTY; if (!projectName && isInteractive) { projectName = await prompt("Enter the name of your new project:"); } @@ -1735,11 +1735,11 @@ export const pages: BuilderCallback = (yargs) => { const config = getConfigCache( PAGES_CONFIG_CACHE_FILENAME ); - const isInteractive = process.stdin.isTTY; - const accountId = await requireAuth(config, isInteractive); + const accountId = await requireAuth(config); projectName ??= config.project_name; + const isInteractive = process.stdin.isTTY; if (!projectName && isInteractive) { const projects = await listProjects({ accountId }); projectName = await new Promise((resolve) => { diff --git a/packages/wrangler/src/reporting.ts b/packages/wrangler/src/reporting.ts index aa75e4ab02fa..13d82355bf28 100644 --- a/packages/wrangler/src/reporting.ts +++ b/packages/wrangler/src/reporting.ts @@ -93,7 +93,8 @@ function exceptionTransaction(error: Error, origin = "") { } export async function reportError(err: Error, origin = "") { - if (!process.stdout.isTTY) return await appendReportingDecision("false"); + // If the user has not opted in to error reporting, we don't want to do anything in CI or non-interactive environments + if (!process.stdout.isTTY) return; const errorTrackingOpt = await reportingPermission(); diff --git a/packages/wrangler/src/user.tsx b/packages/wrangler/src/user.tsx index 72aff6f9dcd6..7506fa6b3345 100644 --- a/packages/wrangler/src/user.tsx +++ b/packages/wrangler/src/user.tsx @@ -410,7 +410,20 @@ export function getAPIToken(): string | undefined { "If you wish to authenticate via an API token then please set the `CLOUDFLARE_API_TOKEN` environment variable." ); } - return LocalState.accessToken?.value; + + const localAPIToken = getCloudflareAPITokenFromEnv(); + + if ( + !process.stdout.isTTY && + !localAPIToken && + !LocalState.accessToken?.value + ) { + throw new Error( + "In a non-interactive environment, it's necessary to set a CLOUDFLARE_API_TOKEN environment variable for wrangler to work. Please go to https://developers.cloudflare.com/api/tokens/create/ for instructions on how to create an api token, and assign its value to CLOUDFLARE_API_TOKEN." + ); + } + + return localAPIToken ?? LocalState.accessToken?.value; } interface AccessContext { @@ -1127,6 +1140,11 @@ export async function getAccountId( .join("\n") ); } + } else { + if (!isInteractive) + throw new Error( + `Failed to automatically retrieve account IDs for the logged in user. In a non-interactive environment, it is mandatory to specify an account ID, either by assigning its value to CLOUDFLARE_ACCOUNT_ID, or as \`account_id\` in your \`wrangler.toml\` file.` + ); } return accountId; } @@ -1157,10 +1175,10 @@ export function ChooseAccount(props: { /** * Ensure that a user is logged in, and a valid account_id is available. */ -export async function requireAuth( - config: { account_id?: string }, - isInteractive = true -): Promise { +export async function requireAuth(config: { + account_id?: string; +}): Promise { + const isInteractive = process.stdin.isTTY; const loggedIn = await loginOrRefreshIfRequired(isInteractive); if (!loggedIn) { // didn't login, let's just quit