diff --git a/.changeset/fix-miniflare-hang-on-workerd-exit.md b/.changeset/fix-miniflare-hang-on-workerd-exit.md new file mode 100644 index 0000000000..91f1b7e82c --- /dev/null +++ b/.changeset/fix-miniflare-hang-on-workerd-exit.md @@ -0,0 +1,9 @@ +--- +"miniflare": patch +--- + +Detect early workerd exit instead of hanging indefinitely + +When `workerd` exits during startup before writing all expected listen events to the control file descriptor (e.g. due to an IPv6 bind failure, permission error, or missing library), Miniflare's `waitForPorts()` would block forever. This caused `wrangler dev` to stall at "Starting local server..." with no error and no timeout. + +The fix races `waitForPorts()` against the child process exit event so that any unexpected `workerd` termination is detected immediately. When `workerd` exits early, Miniflare now throws `ERR_RUNTIME_FAILURE` with the runtime's stderr output included in the error message, making the root cause diagnosable without external tools. diff --git a/.changeset/update-secret-bulk-description.md b/.changeset/update-secret-bulk-description.md new file mode 100644 index 0000000000..b756a154e8 --- /dev/null +++ b/.changeset/update-secret-bulk-description.md @@ -0,0 +1,7 @@ +--- +"wrangler": patch +--- + +Update `wrangler secret bulk` command description to reflect create/update/delete capabilities + +The help text for `wrangler secret bulk` now accurately describes that the command can create, update, or delete multiple secrets in a single request, with up to 100 secrets per command. The file argument description also clarifies that setting a key to `null` in JSON will delete it, and that deletion is not supported with `.env` files. diff --git a/.changeset/wrangler-auth-config-file-mode-0600.md b/.changeset/wrangler-auth-config-file-mode-0600.md new file mode 100644 index 0000000000..e49fa028c5 --- /dev/null +++ b/.changeset/wrangler-auth-config-file-mode-0600.md @@ -0,0 +1,8 @@ +--- +"wrangler": patch +"@cloudflare/workers-auth": patch +--- + +Tighten on-disk permissions of the OAuth credentials file to `0600` + +The user auth config file written by `wrangler login` (typically `~/.config/.wrangler/config/default.toml` on Linux/macOS, or `.toml` for non-production Cloudflare API environments) is now written with mode `0600` and re-`chmod`-ed on every save. This prevents other local users on shared hosts from reading the stored OAuth tokens. Existing files with looser permissions written by older Wrangler versions are tightened the next time Wrangler refreshes the token or the user logs in again. The change is a no-op on Windows, which does not honour POSIX mode bits. diff --git a/.changeset/wrangler-login-oauth-callback-error-hang.md b/.changeset/wrangler-login-oauth-callback-error-hang.md new file mode 100644 index 0000000000..0ff9686424 --- /dev/null +++ b/.changeset/wrangler-login-oauth-callback-error-hang.md @@ -0,0 +1,17 @@ +--- +"wrangler": patch +"@cloudflare/workers-auth": patch +--- + +Show the actual OAuth error instead of hanging when `wrangler login` is rejected by the OAuth provider (for example with `invalid_scope`). + +Previously, if the OAuth callback returned with an `error` other than `access_denied`, Wrangler would never respond to the browser. Because `server.close()`'s callback only fires once all open connections have ended, the login command would hang until the 120 second OAuth timeout — at which point it would print a generic timeout message rather than the actual OAuth failure. The same gap existed for the case where the OAuth provider redirected back without an authorisation code, and for failures during the auth-code-to-access-token exchange. + +The OAuth provider's `error_description` (RFC 6749 §4.1.2.1) is now also surfaced, so the message includes the specific reason for the failure rather than just the bare `error` code. For example, a misconfigured staging scope now surfaces as: + +``` +OAuth error: invalid_scope + The OAuth 2.0 Client is not allowed to request scope 'browser:write'. +``` + +instead of hanging silently. diff --git a/packages/miniflare/src/runtime/index.ts b/packages/miniflare/src/runtime/index.ts index 05894e4168..7bd790cd3e 100644 --- a/packages/miniflare/src/runtime/index.ts +++ b/packages/miniflare/src/runtime/index.ts @@ -209,6 +209,17 @@ class StartupLogBuffer { `Address already in use (${match[1]}:${match[2]}). Please check that you are not already running a server on this address or specify a different port with --port.` ); } + + // If stderr contains any output, surface it so the user can diagnose + // the failure (e.g. bind errors on IPv6 addresses, permission denied, + // missing libraries, etc.) + const stderr = this.stderrBuffer.join("").trim(); + if (stderr.length > 0) { + throw new MiniflareCoreError( + "ERR_RUNTIME_FAILURE", + `The Workers runtime failed to start. There was likely a problem with the workerd binary or your configuration.\nRuntime stderr:\n${stderr}` + ); + } } } @@ -246,7 +257,8 @@ export class Runtime { }); const startupLogBuffer = new StartupLogBuffer(); this.#process = runtimeProcess; - this.#processExitPromise = waitForExit(runtimeProcess); + const processExitPromise = waitForExit(runtimeProcess); + this.#processExitPromise = processExitPromise; const handleRuntimeStdio = options.handleRuntimeStdio ?? @@ -279,8 +291,15 @@ export class Runtime { runtimeProcess.stdin.end(); await once(runtimeProcess.stdin, "finish"); - // 4. Wait for sockets to start listening - const ports = await waitForPorts(controlPipe, options); + // 4. Wait for sockets to start listening, racing against the process + // exiting early. If workerd exits before all required sockets report + // their ports (e.g. due to a bind failure), `waitForPorts()` would + // hang indefinitely. Racing against the exit promise ensures we + // detect this and return `undefined` promptly. + const ports = await Promise.race([ + waitForPorts(controlPipe, options), + processExitPromise.then(() => undefined), + ]); if (ports?.has(kInspectorSocket) && process.env.VSCODE_INSPECTOR_OPTIONS) { // We have an inspector socket and we're in a VSCode Debug Terminal. // Let's startup a watchdog service to register ourselves as a debuggable target diff --git a/packages/miniflare/test/fixtures/crashing-workerd.mjs b/packages/miniflare/test/fixtures/crashing-workerd.mjs new file mode 100755 index 0000000000..ab1259b54a --- /dev/null +++ b/packages/miniflare/test/fixtures/crashing-workerd.mjs @@ -0,0 +1,14 @@ +#!/usr/bin/env node +// A fake workerd that exits immediately without writing any control messages. +// Used to test that Miniflare detects early workerd exits instead of hanging. + +import { arrayBuffer } from "stream/consumers"; + +// Consume stdin (config passed via stdin) to avoid EPIPE +await arrayBuffer(process.stdin); + +// Write an error to stderr to simulate a startup failure +process.stderr.write("error: bind(::1, 0): Address not available\n"); + +// Exit with non-zero code without writing any listen events to FD3 +process.exit(1); diff --git a/packages/miniflare/test/index.spec.ts b/packages/miniflare/test/index.spec.ts index 8b1e75df17..87c3c1f19a 100644 --- a/packages/miniflare/test/index.spec.ts +++ b/packages/miniflare/test/index.spec.ts @@ -2752,6 +2752,32 @@ unixSerialTest( } ); +// When workerd exits before sending all listen events on FD3, Miniflare should +// detect this and throw ERR_RUNTIME_FAILURE instead of hanging indefinitely. +// See https://github.com/cloudflare/workers-sdk/issues/14077 +unixSerialTest( + "Miniflare: throws ERR_RUNTIME_FAILURE when workerd exits before all sockets are ready", + async ({ expect, onTestFinished }) => { + const workerdPath = path.join(FIXTURES_PATH, "crashing-workerd.mjs"); + + const original = process.env.MINIFLARE_WORKERD_PATH; + process.env.MINIFLARE_WORKERD_PATH = workerdPath; + onTestFinished(() => { + if (original === undefined) { + delete process.env.MINIFLARE_WORKERD_PATH; + } else { + process.env.MINIFLARE_WORKERD_PATH = original; + } + }); + + const mf = new Miniflare({ script: "" }); + onTestFinished(() => mf.dispose().catch(() => {})); + + await expect(mf.ready).rejects.toThrow(MiniflareCoreError); + await expect(mf.ready).rejects.toThrow(/Workers runtime failed to start/); + } +); + const TIMEZONE_WORKER = ` export default { fetch() { diff --git a/packages/workers-auth/src/auth-config-file.ts b/packages/workers-auth/src/auth-config-file.ts index 4f10c02e95..5752d82953 100644 --- a/packages/workers-auth/src/auth-config-file.ts +++ b/packages/workers-auth/src/auth-config-file.ts @@ -1,4 +1,4 @@ -import { mkdirSync, writeFileSync } from "node:fs"; +import { chmodSync, mkdirSync, writeFileSync } from "node:fs"; import path from "node:path"; import { getCloudflareApiEnvironmentFromEnv, @@ -52,9 +52,17 @@ export function writeAuthConfigFile(config: UserAuthConfig): void { mkdirSync(path.dirname(configPath), { recursive: true, }); + // Write with mode 0o600 on creation and re-`chmod` on every save so + // other local users on shared hosts can't read the OAuth tokens. + // `writeFileSync`'s `mode` option only applies when the file is + // being created — the explicit `chmodSync` ensures that pre-existing + // files (e.g. written by an older Wrangler version with the process + // umask) get tightened on the next save too. writeFileSync(configPath, TOML.stringify(config), { encoding: "utf-8", + mode: 0o600, }); + chmodSync(configPath, 0o600); } /** diff --git a/packages/workers-auth/src/callback-server.ts b/packages/workers-auth/src/callback-server.ts index e275c7f5a0..68cfd70d37 100644 --- a/packages/workers-auth/src/callback-server.ts +++ b/packages/workers-auth/src/callback-server.ts @@ -16,7 +16,7 @@ import assert from "node:assert"; import http from "node:http"; import url from "node:url"; import { UserError } from "@cloudflare/workers-utils"; -import { ErrorAccessDenied, ErrorNoAuthCode } from "./errors"; +import { ErrorAccessDenied, ErrorNoAuthCode, ErrorOAuth2 } from "./errors"; import { exchangeAuthCodeForAccessToken, getAuthURL, @@ -85,6 +85,15 @@ export async function getOauthToken( function finish(token: AccessContext): void; function finish(token: AccessContext | null, error?: Error) { clearTimeout(loginTimeoutHandle); + // Defensive: every code path that calls `finish()` should already + // have written a response, but if not, end the connection so that + // `server.close()` can complete (its callback only fires once all + // open connections have ended). Without this, a future code path + // that forgets to send a response could cause `wrangler login` to + // hang until the OAuth timeout. + if (!res.writableEnded) { + res.end(); + } server.close((closeErr?: Error) => { if (error || closeErr) { reject(error || closeErr); @@ -95,6 +104,52 @@ export async function getOauthToken( }); } + function renderErrorPage(detail: { + code?: string; + description?: string; + }): void { + const escape = (s: string) => + s.replace( + /[&<>"']/g, + (c) => + ({ + "&": "&", + "<": "<", + ">": ">", + '"': """, + "'": "'", + })[c] as string + ); + const codeRow = detail.code + ? `

Code: ${escape(detail.code)}

` + : ""; + const descriptionRow = detail.description + ? `

${escape(detail.description)}

` + : ""; + const body = ` + + + + Wrangler login failed + + + +

Wrangler login failed

+

The Cloudflare OAuth provider returned an error.

+ ${codeRow} + ${descriptionRow} +

You can close this tab and return to your terminal for more details.

+ +`; + res.writeHead(400, { "Content-Type": "text/html; charset=utf-8" }); + res.end(body); + } + assert(req.url, "This request doesn't have a URL"); // This should never happen const { pathname, query } = url.parse(req.url, true); if (req.method !== "GET") { @@ -119,46 +174,65 @@ export async function getOauthToken( ); }); - return; - } else { - finish(null, err as Error); return; } + const oauthErr = err as ErrorOAuth2; + renderErrorPage({ + code: oauthErr.code, + description: oauthErr.description ?? oauthErr.message, + }); + finish(null, oauthErr); + return; } if (!hasAuthCode) { - // render an error page here + const noCodeMessage = + "The Cloudflare OAuth provider did not return an authorisation code."; + renderErrorPage({ description: noCodeMessage }); finish( null, - new ErrorNoAuthCode("", { + new ErrorNoAuthCode(noCodeMessage, { telemetryMessage: "user oauth missing auth code", }) ); return; - } else { - // `exchangeAuthCodeForAccessToken` can reject (network error, - // invalid JSON, OAuth error response, etc.). Without this - // `try/catch` the rejection would become an unhandled promise - // rejection inside an `http.createServer` callback, which is - // not promise-aware — Node.js >= 15 terminates the process on - // unhandled rejection by default. Route the error through - // `finish` so the caller's promise rejects cleanly. - try { - const exchange = await exchangeAuthCodeForAccessToken( - state, - ctx.logger, - ctx.isNonInteractiveOrCI - ); - res.writeHead(307, { - Location: options.granted.url, - }); - res.end(() => { - finish(exchange); - }); - } catch (err) { - finish(null, err as Error); - } - return; } + // `exchangeAuthCodeForAccessToken` can reject (network error, + // invalid JSON, OAuth error response, etc.). Without this + // `try/catch` the rejection would become an unhandled promise + // rejection inside an `http.createServer` callback, which is + // not promise-aware — Node.js >= 15 terminates the process on + // unhandled rejection by default. Route the error through + // `finish` so the caller's promise rejects cleanly. + try { + const exchange = await exchangeAuthCodeForAccessToken( + state, + ctx.logger, + ctx.isNonInteractiveOrCI + ); + res.writeHead(307, { + Location: options.granted.url, + }); + res.end(() => { + finish(exchange); + }); + } catch (err: unknown) { + // `exchangeAuthCodeForAccessToken` can throw an `ErrorOAuth2` + // (for provider-side errors), or a plain `Error` (for JSON + // parse failures in `getJSONFromResponse`, network errors + // from `fetchAuthToken`, etc.). Only read the structured + // OAuth fields when we know we have them. + const exchangeErr = err as Error; + const isOAuthError = exchangeErr instanceof ErrorOAuth2; + renderErrorPage({ + code: isOAuthError ? exchangeErr.code : undefined, + description: + (isOAuthError ? exchangeErr.description : undefined) ?? + exchangeErr.message ?? + "Failed to exchange the authorisation code for an access token.", + }); + finish(null, exchangeErr); + } + return; } } }); diff --git a/packages/workers-auth/src/errors.ts b/packages/workers-auth/src/errors.ts index 450f12b9c6..c69373d886 100644 --- a/packages/workers-auth/src/errors.ts +++ b/packages/workers-auth/src/errors.ts @@ -16,16 +16,26 @@ import { UserError } from "@cloudflare/workers-utils"; /** * A list of OAuth2AuthCodePKCE errors. + * + * Instances may carry the structured details from the OAuth provider's + * `error`, `error_description` and `error_uri` query parameters (RFC 6749 + * §4.1.2.1) so callers can render them — see {@link toErrorClass}. */ // To "namespace" all errors. export class ErrorOAuth2 extends UserError { + /** The OAuth `error` code returned by the provider (e.g. `invalid_scope`). */ + code?: string; + /** The OAuth `error_description` returned by the provider, if any. */ + description?: string; + /** The OAuth `error_uri` returned by the provider, if any. */ + uri?: string; toString(): string { return "ErrorOAuth2"; } } // Unclassified Oauth errors -export class ErrorUnknown extends UserError { +export class ErrorUnknown extends ErrorOAuth2 { toString(): string { return "ErrorUnknown"; } @@ -125,61 +135,108 @@ export class ErrorUnsupportedGrantType extends ErrorAccessTokenResponse { } /** - * Translate the raw error strings returned from the server into error classes. + * Format the parts of an OAuth provider error response into a single, + * user-facing message. + */ +function formatOAuthErrorMessage( + code: string, + description: string | undefined, + uri: string | undefined +): string { + let message = `OAuth error: ${code}`; + if (description) { + message += `\n ${description}`; + } + if (uri) { + message += `\n See: ${uri}`; + } + return message; +} + +/** + * Translate an OAuth error response from the provider into one of our error + * classes. The `error_description` and `error_uri` parameters (RFC 6749 + * §4.1.2.1) are included in the message when present so the user sees the + * specific reason for the failure rather than just the bare error code, and + * are also attached as structured fields so the HTTP callback handler can + * render them on the browser-facing error page. */ -export function toErrorClass(rawError: string): ErrorOAuth2 | ErrorUnknown { +export function toErrorClass( + rawError: string, + description?: string, + uri?: string +): ErrorOAuth2 | ErrorUnknown { + const message = formatOAuthErrorMessage(rawError, description, uri); + let error: ErrorOAuth2 | ErrorUnknown; switch (rawError) { case "invalid_request": - return new ErrorInvalidRequest(rawError, { + error = new ErrorInvalidRequest(message, { telemetryMessage: "user oauth invalid request", }); + break; case "invalid_grant": - return new ErrorInvalidGrant(rawError, { + error = new ErrorInvalidGrant(message, { telemetryMessage: "user oauth invalid grant", }); + break; case "unauthorized_client": - return new ErrorUnauthorizedClient(rawError, { + error = new ErrorUnauthorizedClient(message, { telemetryMessage: "user oauth unauthorized client", }); + break; case "access_denied": - return new ErrorAccessDenied(rawError, { + error = new ErrorAccessDenied(message, { telemetryMessage: "user oauth access denied", }); + break; case "unsupported_response_type": - return new ErrorUnsupportedResponseType(rawError, { + error = new ErrorUnsupportedResponseType(message, { telemetryMessage: "user oauth unsupported response type", }); + break; case "invalid_scope": - return new ErrorInvalidScope(rawError, { + error = new ErrorInvalidScope(message, { telemetryMessage: "user oauth invalid scope", }); + break; case "server_error": - return new ErrorServerError(rawError, { + error = new ErrorServerError(message, { telemetryMessage: "user oauth server error", }); + break; case "temporarily_unavailable": - return new ErrorTemporarilyUnavailable(rawError, { + error = new ErrorTemporarilyUnavailable(message, { telemetryMessage: "user oauth temporarily unavailable", }); + break; case "invalid_client": - return new ErrorInvalidClient(rawError, { + error = new ErrorInvalidClient(message, { telemetryMessage: "user oauth invalid client", }); + break; case "unsupported_grant_type": - return new ErrorUnsupportedGrantType(rawError, { + error = new ErrorUnsupportedGrantType(message, { telemetryMessage: "user oauth unsupported grant type", }); + break; case "invalid_json": - return new ErrorInvalidJson(rawError, { + error = new ErrorInvalidJson(message, { telemetryMessage: "user oauth invalid json", }); + break; case "invalid_token": - return new ErrorInvalidToken(rawError, { + error = new ErrorInvalidToken(message, { telemetryMessage: "user oauth invalid token", }); + break; default: - return new ErrorUnknown(rawError, { + error = new ErrorUnknown(message, { telemetryMessage: "user oauth unknown error", }); + break; } + error.code = rawError; + error.description = description; + error.uri = uri; + return error; } diff --git a/packages/workers-auth/src/token-exchange.ts b/packages/workers-auth/src/token-exchange.ts index f91c1c8c9a..ac0ca6e8d3 100644 --- a/packages/workers-auth/src/token-exchange.ts +++ b/packages/workers-auth/src/token-exchange.ts @@ -64,10 +64,14 @@ export function isReturningFromAuthServer( logger: OAuthFlowContext["logger"] ): boolean { if (query.error) { - if (Array.isArray(query.error)) { - throw toErrorClass(query.error[0]); - } - throw toErrorClass(query.error); + const error = Array.isArray(query.error) ? query.error[0] : query.error; + const description = Array.isArray(query.error_description) + ? query.error_description[0] + : query.error_description; + const uri = Array.isArray(query.error_uri) + ? query.error_uri[0] + : query.error_uri; + throw toErrorClass(error, description, uri); } const code = query.code; @@ -253,7 +257,10 @@ export async function exchangeAuthCodeForAccessToken( } const json = (await getJSONFromResponse(response, logger)) as TokenResponse; if ("error" in json) { - throw new Error(json.error); + // The token endpoint normally returns OAuth errors via a 4xx status + // (handled above), but be defensive: surface a 2xx-with-error-body as + // a structured OAuth error too so the catch site can render the code. + throw toErrorClass(json.error); } const { access_token, expires_in, refresh_token, scope } = json; state.hasAuthCodeBeenExchangedForAccessToken = true; diff --git a/packages/workers-auth/tests/auth-config-file.test.ts b/packages/workers-auth/tests/auth-config-file.test.ts new file mode 100644 index 0000000000..5b2bd065e0 --- /dev/null +++ b/packages/workers-auth/tests/auth-config-file.test.ts @@ -0,0 +1,54 @@ +import { chmodSync, statSync } from "node:fs"; +import { runInTempDir } from "@cloudflare/workers-utils/test-helpers"; +import { describe, it } from "vitest"; +import { + getAuthConfigFilePath, + readAuthConfigFile, + writeAuthConfigFile, +} from "../src/auth-config-file"; +import type { UserAuthConfig } from "../src/auth-config-file"; + +const SAMPLE_CONFIG: UserAuthConfig = { + oauth_token: "test-oauth-token", + refresh_token: "test-refresh-token", + expiration_time: "2099-01-01T00:00:00.000Z", + scopes: ["account:read"], +}; + +describe("writeAuthConfigFile", () => { + runInTempDir(); + + it("round-trips a UserAuthConfig through the TOML file", ({ expect }) => { + writeAuthConfigFile(SAMPLE_CONFIG); + expect(readAuthConfigFile()).toEqual(SAMPLE_CONFIG); + }); + + // POSIX-only: Windows doesn't honour POSIX mode bits — `chmodSync` only + // touches the read-only flag there, so the `& 0o777 === 0o600` assertion + // would be meaningless. The hardening still runs unconditionally; we + // just skip the assertion that wouldn't hold cross-platform. + it.skipIf(process.platform === "win32")( + "writes a new auth config file with mode 0o600", + ({ expect }) => { + writeAuthConfigFile(SAMPLE_CONFIG); + const mode = statSync(getAuthConfigFilePath()).mode & 0o777; + expect(mode).toBe(0o600); + } + ); + + it.skipIf(process.platform === "win32")( + "tightens permissions on a pre-existing auth config file with looser mode", + ({ expect }) => { + // Simulate a file left behind by an older Wrangler version that + // wrote with the process umask (typically 0o644). + writeAuthConfigFile(SAMPLE_CONFIG); + chmodSync(getAuthConfigFilePath(), 0o644); + expect(statSync(getAuthConfigFilePath()).mode & 0o777).toBe(0o644); + + // Re-saving must restore the tight 0o600 permissions even though + // `writeFileSync`'s `mode` option is ignored for existing files. + writeAuthConfigFile(SAMPLE_CONFIG); + expect(statSync(getAuthConfigFilePath()).mode & 0o777).toBe(0o600); + } + ); +}); diff --git a/packages/wrangler/src/__tests__/helpers/mock-http-server.ts b/packages/wrangler/src/__tests__/helpers/mock-http-server.ts index 67263832df..8ed9b3f659 100644 --- a/packages/wrangler/src/__tests__/helpers/mock-http-server.ts +++ b/packages/wrangler/src/__tests__/helpers/mock-http-server.ts @@ -5,18 +5,33 @@ import type { Request } from "undici"; /** * Setup a mock HTTP server that can be triggered directly without interacting with any network. * + * The mock mirrors production semantics for `server.close()`: its callback only + * fires once `res.end()` has been observed. A code path that forgets to send a + * response cannot rely on `server.close()` to clean up — exactly as in + * production — and tests for such code paths will fail at the vitest test + * timeout rather than passing by accident. + * * @returns a `fetch`-like function that will trigger the mock server to handle the request. */ export function mockHttpServer() { let listener: http.RequestListener; + let pendingCloseCallback: ((err?: Error) => void) | undefined; + let responseEnded = false; beforeEach(() => { + pendingCloseCallback = undefined; + responseEnded = false; + vi.spyOn(http, "createServer").mockImplementation((...args: unknown[]) => { listener = args.pop() as http.RequestListener; return { listen: vi.fn(), close(callback?: (err?: Error) => void) { - callback?.(); + if (responseEnded) { + callback?.(); + } else { + pendingCloseCallback = callback; + } return this; }, // The OAuth callback server registers an `error` listener so it @@ -34,16 +49,35 @@ export function mockHttpServer() { req as unknown as http.IncomingMessage ); - // The listener will attache a callback to the response by calling `resp.end(callback)`. - // We want to capture that so that we can trigger it after the listener has completed its work. + // The listener may attach a callback to the response by calling `resp.end(callback)`. + // We capture that so we can trigger it once the listener has finished its async work, + // and then fire any pending `server.close()` callback (mirroring production semantics + // where `server.close()` only fires once all open connections have ended). const endSpy = vi.spyOn(resp, "end"); - // The `await` here is important to allow the listener to complete its async work before we end the response. + // The `await` here is important to allow the listener to complete its async work before + // we end the response. await listener(req as unknown as http.IncomingMessage, resp); - // Now trigger the end callback. - const endCallback = endSpy.mock.calls[0].pop(); - endCallback?.(); + if (endSpy.mock.calls.length > 0) { + // Invoke the optional callback passed to `res.end()` (if any). + const lastCall = endSpy.mock.calls[endSpy.mock.calls.length - 1]; + const lastArg = lastCall[lastCall.length - 1]; + if (typeof lastArg === "function") { + (lastArg as () => void)(); + } + // Mark the response as ended and fire the pending `server.close()` callback, + // if any. The close callback may have been registered synchronously by the + // `res.end()` callback above (e.g. via `finish()` in `getOauthToken`), so we + // read it after invoking that callback. + responseEnded = true; + const cb = pendingCloseCallback; + pendingCloseCallback = undefined; + cb?.(); + } + // If `res.end()` was never called, this mimics production: `server.close()`'s + // callback never fires, and the caller will time out. Tests that hit this path + // will fail at the vitest test timeout. return resp; }; diff --git a/packages/wrangler/src/__tests__/helpers/mock-oauth-flow.ts b/packages/wrangler/src/__tests__/helpers/mock-oauth-flow.ts index 2c4ee4262a..b11fa0a69a 100644 --- a/packages/wrangler/src/__tests__/helpers/mock-oauth-flow.ts +++ b/packages/wrangler/src/__tests__/helpers/mock-oauth-flow.ts @@ -191,10 +191,12 @@ export function mockExchangeRefreshTokenForAccessToken({ type GrantResponseOptions = { code?: string; error?: ErrorType | ErrorType[]; + error_description?: string; + error_uri?: string; }; const toQueryParams = ( - { code, error }: GrantResponseOptions, + { code, error, error_description, error_uri }: GrantResponseOptions, wranglerRequestParams: URLSearchParams ): string => { const queryParams = []; @@ -205,6 +207,14 @@ const toQueryParams = ( const stringifiedErr = Array.isArray(error) ? error.join(",") : error; queryParams.push(`error=${stringifiedErr}`); } + if (error_description) { + queryParams.push( + `error_description=${encodeURIComponent(error_description)}` + ); + } + if (error_uri) { + queryParams.push(`error_uri=${encodeURIComponent(error_uri)}`); + } queryParams.push(`state=${wranglerRequestParams.get("state")}`); diff --git a/packages/wrangler/src/__tests__/index.test.ts b/packages/wrangler/src/__tests__/index.test.ts index 543c992ea1..52b3d38c05 100644 --- a/packages/wrangler/src/__tests__/index.test.ts +++ b/packages/wrangler/src/__tests__/index.test.ts @@ -267,7 +267,7 @@ describe("wrangler", () => { wrangler secret put Create or update a secret for a Worker wrangler secret delete Delete a secret from a Worker wrangler secret list List all secrets for a Worker - wrangler secret bulk [file] Upload multiple secrets for a Worker at once + wrangler secret bulk [file] Create, update, or delete multiple secrets for a Worker in a single request, with up to 100 secrets per command. GLOBAL FLAGS -c, --config Path to Wrangler configuration file [string] diff --git a/packages/wrangler/src/__tests__/user.test.ts b/packages/wrangler/src/__tests__/user.test.ts index 67e0401547..4bf07424e8 100644 --- a/packages/wrangler/src/__tests__/user.test.ts +++ b/packages/wrangler/src/__tests__/user.test.ts @@ -291,6 +291,99 @@ describe("User", () => { Please use a Cloudflare API token (\`CLOUDFLARE_API_TOKEN\` environment variable) or remove the \`CLOUDFLARE_API_ENVIRONMENT\` environment variable.] `); }); + + // Regression coverage for the OAuth callback hang. When the OAuth + // provider redirected to `/oauth/callback?error=...` with any error + // other than `access_denied`, Wrangler used to never write a response, + // causing `server.close()` to wait forever for the connection to drain + // and the login command to hang until the 120s OAuth timeout. The + // tightened `mock-http-server.ts` faithfully reproduces those + // production semantics, so a regression would cause these tests to + // fail at the vitest test timeout rather than passing by accident. + describe("OAuth callback error handling", () => { + it("rejects with the bare OAuth error code when no description is provided", async ({ + expect, + }) => { + mockOAuthServerCallback({ error: "invalid_scope" }); + + await expect( + runWrangler("login") + ).rejects.toThrowErrorMatchingInlineSnapshot( + `[Error: OAuth error: invalid_scope]` + ); + }); + + it("rejects with both the OAuth error code and error_description", async ({ + expect, + }) => { + mockOAuthServerCallback({ + error: "invalid_scope", + error_description: + "The OAuth 2.0 Client is not allowed to request scope 'browser:write'.", + }); + + await expect(runWrangler("login")).rejects + .toThrowErrorMatchingInlineSnapshot(` + [Error: OAuth error: invalid_scope + The OAuth 2.0 Client is not allowed to request scope 'browser:write'.] + `); + }); + + it("rejects with the existing consent-denied message for access_denied", async ({ + expect, + }) => { + mockOAuthServerCallback({ error: "access_denied" }); + + await expect(runWrangler("login")).rejects + .toThrowErrorMatchingInlineSnapshot(` + [Error: Error: Consent denied. You must grant consent to Wrangler in order to login. + If you don't want to do this consider passing an API token via the \`CLOUDFLARE_API_TOKEN\` environment variable] + `); + }); + + it("rejects with the provider error when /oauth2/token fails after a valid auth code", async ({ + expect, + }) => { + mockOAuthServerCallback("success"); + msw.use( + http.post( + "*/oauth2/token", + () => + HttpResponse.json({ error: "invalid_grant" }, { status: 400 }), + { once: true } + ) + ); + + await expect( + runWrangler("login") + ).rejects.toThrowErrorMatchingInlineSnapshot( + `[Error: OAuth error: invalid_grant]` + ); + }); + + it("rejects with the provider error when /oauth2/token returns 2xx with an error body", async ({ + expect, + }) => { + // Defensive handling of the (non-standard) case where the token + // endpoint returns a success status but the body still carries an + // OAuth `error` field. The error should still be surfaced via + // `toErrorClass` rather than as a plain `Error`. + mockOAuthServerCallback("success"); + msw.use( + http.post( + "*/oauth2/token", + () => HttpResponse.json({ error: "invalid_token" }), + { once: true } + ) + ); + + await expect( + runWrangler("login") + ).rejects.toThrowErrorMatchingInlineSnapshot( + `[Error: OAuth error: invalid_token]` + ); + }); + }); }); it("should handle errors for failed token refresh in a non-interactive environment", async ({ diff --git a/packages/wrangler/src/secret/index.ts b/packages/wrangler/src/secret/index.ts index 623b2adc42..d1dcbafe12 100644 --- a/packages/wrangler/src/secret/index.ts +++ b/packages/wrangler/src/secret/index.ts @@ -425,7 +425,8 @@ async function putBulkSecrets( export const secretBulkCommand = createCommand({ metadata: { - description: "Upload multiple secrets for a Worker at once", + description: + "Create, update, or delete multiple secrets for a Worker in a single request, with up to 100 secrets per command.", status: "stable", owner: "Workers: Deploy and Config", }, @@ -435,7 +436,7 @@ export const secretBulkCommand = createCommand({ }, args: { file: { - describe: `The file of key-value pairs to upload, as JSON in form {"key": value, ...} or .env file in the form KEY=VALUE. If omitted, Wrangler expects to receive input from stdin rather than a file.`, + describe: `The file of key-value pairs to create, update, or delete, as JSON in form {"key": "value", ...} or .env file in the form KEY=VALUE. Set a key to null in the JSON file to delete it. Deletion is not supported with .env files. If omitted, Wrangler expects to receive input from stdin rather than a file.`, type: "string", }, name: {