diff --git a/.changeset/browser-launch-timeout.md b/.changeset/browser-launch-timeout.md new file mode 100644 index 0000000000..dd5f55d748 --- /dev/null +++ b/.changeset/browser-launch-timeout.md @@ -0,0 +1,7 @@ +--- +"miniflare": patch +--- + +Add timeout to browser-rendering browser launch to prevent infinite hangs + +The browser-rendering plugin's `launchBrowser()` function now passes a 5-minute timeout to `waitForLineOutput()` when waiting for Chrome to print its DevTools WebSocket URL. Previously, if Chrome failed to start or crashed before printing the URL, the promise would hang forever. This could cause CI pipelines and local dev sessions to get stuck indefinitely. diff --git a/.changeset/fix-cloudflared-macos-checksum.md b/.changeset/fix-cloudflared-macos-checksum.md new file mode 100644 index 0000000000..7345d15839 --- /dev/null +++ b/.changeset/fix-cloudflared-macos-checksum.md @@ -0,0 +1,9 @@ +--- +"@cloudflare/workers-utils": patch +--- + +Fix cloudflared SHA256 checksum mismatch on macOS + +The update service (`update.argotunnel.com`) returns a checksum for the extracted binary, not the `.tgz` tarball. We were computing the SHA256 of the tarball itself, which always mismatched on macOS where cloudflared is distributed as a compressed archive. + +This aligns with cloudflared's own auto-updater (`cmd/cloudflared/updater/workers_update.go`), which decompresses the tarball first, then checksums the resulting binary. We now do the same: extract, then verify. diff --git a/.changeset/sharp-containers-proxy.md b/.changeset/sharp-containers-proxy.md new file mode 100644 index 0000000000..0f8e3a7171 --- /dev/null +++ b/.changeset/sharp-containers-proxy.md @@ -0,0 +1,7 @@ +--- +"wrangler": minor +--- + +Add ProxyCommand support for `wrangler containers ssh` + +`wrangler containers ssh` now automatically switches to a stdio proxy when invoked by OpenSSH's `ProxyCommand`, and `--stdio` can force this mode. This lets users connect with `ssh ` when their SSH config uses Wrangler as the proxy command. diff --git a/.github/workflows/test-and-check-other-node.yml b/.github/workflows/test-and-check-other-node.yml index 8857cd6847..d2b39803d2 100644 --- a/.github/workflows/test-and-check-other-node.yml +++ b/.github/workflows/test-and-check-other-node.yml @@ -69,6 +69,18 @@ jobs: turbo-token: ${{ secrets.TURBO_TOKEN }} turbo-signature: ${{ secrets.TURBO_REMOTE_CACHE_SIGNATURE_KEY }} + # The miniflare browser-rendering tests download Chrome (~150 MB) via + # @puppeteer/browsers at runtime. Caching the binary avoids repeat + # downloads and reduces the chance of tests hanging while waiting for + # the download to finish within their timeout window. + - name: Restore Chrome browser cache (Browser Run) + if: steps.should_run.outputs.result == 'true' + uses: actions/cache@0057852bfaa89a56745cba8c7296529d2fc39830 # v4 + with: + key: chrome-${{ runner.os }}-${{ hashFiles('packages/miniflare/src/plugins/browser-rendering/browser-version.ts') }} + path: | + ~/.cache/.wrangler/chrome + - name: Run tests (packages) # We are running the package tests first be able to get early feedback on changes. # There is no point in running the fixtures if a package is broken. diff --git a/packages/miniflare/src/plugins/browser-rendering/index.ts b/packages/miniflare/src/plugins/browser-rendering/index.ts index 1bc698fdac..99ecfc20f8 100644 --- a/packages/miniflare/src/plugins/browser-rendering/index.ts +++ b/packages/miniflare/src/plugins/browser-rendering/index.ts @@ -238,7 +238,12 @@ export async function launchBrowser({ }, }); const wsEndpoint = await browserProcess.waitForLineOutput( - CDP_WEBSOCKET_ENDPOINT_REGEX + CDP_WEBSOCKET_ENDPOINT_REGEX, + // Note: we pass an explicit timeout so the promise rejects instead of hanging forever + // when Chrome fails to start or crashes before printing the DevTools URL. + // Five minutes is generous enough to cover on-demand browser downloads on slow + // connections while still failing within a reasonable window. + 5 * 60 * 1_000 ); // On Windows in particular, Chrome may print the DevTools URL slightly // before its listening socket is fully ready to accept connections. diff --git a/packages/workers-utils/src/cloudflared.ts b/packages/workers-utils/src/cloudflared.ts index bc2527abe0..d4681ae0d1 100644 --- a/packages/workers-utils/src/cloudflared.ts +++ b/packages/workers-utils/src/cloudflared.ts @@ -14,6 +14,7 @@ import { constants, existsSync, mkdirSync, + readFileSync, renameSync, unlinkSync, writeFileSync, @@ -516,7 +517,13 @@ async function downloadCloudflared( } /** - * Download and extract a tarball (for macOS) + * Download and extract a tarball (for macOS). + * + * The update service checksum is for the extracted binary, not the tarball + * itself. This matches cloudflared's own auto-update behavior — see + * cloudflared/cmd/cloudflared/updater/workers_update.go: the download() + * function decompresses .tgz into the raw binary, then Apply() checksums + * the resulting file. We do the same: extract first, then verify. */ async function downloadAndExtractTarball( response: Response, @@ -527,17 +534,6 @@ async function downloadAndExtractTarball( const tempTarPath = join(cacheDir, "cloudflared.tgz"); const buffer = Buffer.from(await response.arrayBuffer()); - if (expectedChecksum) { - const actualSha256 = sha256Hex(buffer); - if (actualSha256 !== expectedChecksum) { - throw new UserError( - `[cloudflared] SHA256 mismatch for downloaded cloudflared tarball.\n\n` + - `Expected: ${expectedChecksum}\n` + - `Actual: ${actualSha256}`, - { telemetryMessage: "tunnel cloudflared tarball checksum mismatch" } - ); - } - } writeFileSync(tempTarPath, buffer); try { @@ -549,6 +545,24 @@ async function downloadAndExtractTarball( if (extractedPath !== binPath && existsSync(extractedPath)) { renameSync(extractedPath, binPath); } + + // Verify checksum against the extracted binary, not the tarball. + // The update service provides checksums for the uncompressed binary. + if (expectedChecksum) { + const extractedBinary = readFileSync(binPath); + const actualSha256 = sha256Hex(extractedBinary); + if (actualSha256 !== expectedChecksum) { + throw new UserError( + `[cloudflared] SHA256 mismatch for downloaded cloudflared binary.\n\n` + + `Expected: ${expectedChecksum}\n` + + `Actual: ${actualSha256}`, + { + telemetryMessage: + "tunnel cloudflared extracted binary checksum mismatch", + } + ); + } + } } finally { try { if (existsSync(tempTarPath)) { diff --git a/packages/wrangler/src/__tests__/containers/ssh.test.ts b/packages/wrangler/src/__tests__/containers/ssh.test.ts index c9ade55b01..9def866a8f 100644 --- a/packages/wrangler/src/__tests__/containers/ssh.test.ts +++ b/packages/wrangler/src/__tests__/containers/ssh.test.ts @@ -1,14 +1,50 @@ +import { PassThrough } from "node:stream"; import { http, HttpResponse } from "msw"; -import { afterEach, beforeEach, describe, it } from "vitest"; -import MockWebSocketServer from "vitest-websocket-mock"; +import { afterEach, beforeEach, describe, it, vi } from "vitest"; +import { WebSocketServer } from "ws"; import { mockAccount, setWranglerConfig } from "../cloudchamber/utils"; import { mockAccountId, mockApiToken } from "../helpers/mock-account-id"; import { mockConsoleMethods } from "../helpers/mock-console"; import { msw } from "../helpers/msw"; import { runWrangler } from "../helpers/run-wrangler"; +import type * as childProcess from "node:child_process"; +import type * as nodeEvents from "node:events"; +import type { WebSocket } from "ws"; + +vi.mock("node:child_process", async () => { + const actual = + await vi.importActual("node:child_process"); + const { EventEmitter } = + await vi.importActual("node:events"); + + return { + ...actual, + spawn: vi.fn((...args: Parameters) => { + const [command, commandArgs] = args; + if (command !== "ssh") { + return actual.spawn(...args); + } + + const child = new EventEmitter() as ReturnType; + const sshArgs = Array.isArray(commandArgs) + ? commandArgs.map((arg) => arg.toString()) + : []; + const exitCode = sshArgs.length === 1 && sshArgs[0] === "-V" ? 0 : 255; + + setImmediate(() => { + child.emit("exit", exitCode, null); + child.emit("close", exitCode, null); + }); + + return child; + }), + }; +}); describe("containers ssh", () => { const std = mockConsoleMethods(); + const originalStdin = process.stdin; + const originalStdout = process.stdout; mockAccountId(); mockApiToken(); @@ -16,6 +52,14 @@ describe("containers ssh", () => { afterEach(() => { msw.resetHandlers(); + Object.defineProperty(process, "stdin", { + value: originalStdin, + configurable: true, + }); + Object.defineProperty(process, "stdout", { + value: originalStdout, + configurable: true, + }); }); it("should help", async ({ expect }) => { @@ -36,7 +80,10 @@ describe("containers ssh", () => { --env-file Path to an .env file to load - can be specified multiple times - values from earlier files are overridden by values in later files [array] -h, --help Show help [boolean] --install-skills Install Cloudflare agents skills, if not already present, without asking the user for confirmation [boolean] [default: false] - -v, --version Show version number [boolean]" + -v, --version Show version number [boolean] + + OPTIONS + --stdio Proxy SSH traffic over stdin/stdout [boolean]" `); }); @@ -79,27 +126,157 @@ describe("containers ssh", () => { // against, but everything up until that point is covered. it("should try ssh'ing into a container", async ({ expect }) => { const instanceId = "a".repeat(64); - const wsUrl = "ws://localhost:1234"; const sshJwt = "asd"; + const server = await createWsServer(); + mockStdio({ stdinIsTTY: true, stdoutIsTTY: true }); + + try { + setWranglerConfig({}); + msw.use( + http.get(`*/instances/:instanceId/ssh`, async () => { + return HttpResponse.json( + { success: true, result: { url: server.url, token: sshJwt } }, + { status: 200 } + ); + }) + ); + + const wrangler = runWrangler(`containers ssh ${instanceId}`); + const expectedFailure = expect(wrangler).rejects.toMatchInlineSnapshot(` + [Error: SSH exited unsuccessfully. Is the container running? + NOTE: SSH does not automatically wake a container or count as activity to keep a container alive] + `); + const socket = await server.connection; + socket.close(); + await expectedFailure; + } finally { + server.ws.close(); + } + }); + + it("should proxy stdin and stdout when stdio is forced", async ({ + expect, + }) => { + const instanceId = "a".repeat(64); + const sshJwt = "asd"; + const server = await createWsServer(); + const { stdin, stdout } = mockStdio({ + stdinIsTTY: true, + stdoutIsTTY: true, + }); setWranglerConfig({}); msw.use( http.get(`*/instances/:instanceId/ssh`, async () => { return HttpResponse.json( - { success: true, result: { url: wsUrl, token: sshJwt } }, + { success: true, result: { url: server.url, token: sshJwt } }, { status: 200 } ); }) ); - const mockWebSocket = new MockWebSocketServer(wsUrl); - await expect(runWrangler(`containers ssh ${instanceId}`)).rejects - .toMatchInlineSnapshot(` - [Error: SSH exited unsuccessfully. Is the container running? - NOTE: SSH does not automatically wake a container or count as activity to keep a container alive] - `); + const stdoutData = new Promise((resolve) => { + stdout.on("data", (chunk: Buffer) => resolve(chunk.toString())); + }); + const serverMessage = new Promise((resolve) => { + void server.connection.then((socket) => { + socket.on("message", (chunk) => resolve(chunk.toString())); + }); + }); + const wrangler = runWrangler(`containers ssh --stdio ${instanceId}`); + + const socket = await server.connection; + stdin.write("client-data"); + await expect(serverMessage).resolves.toBe("client-data"); - // We got a connection - expect(mockWebSocket.connected).toBeTruthy(); + socket.send("server-data"); + await expect(stdoutData).resolves.toBe("server-data"); + + stdin.end(); + socket.close(); + await wrangler; + server.ws.close(); + expect(std.out).toBe(""); + expect(stdin.listenerCount("data")).toBe(0); + expect(stdin.listenerCount("end")).toBe(0); + expect(stdin.listenerCount("error")).toBe(0); + }); + + it("should auto-detect proxy mode with extra args when stdin and stdout are not TTYs", async ({ + expect, + }) => { + const instanceId = "a".repeat(64); + const sshJwt = "asd"; + const server = await createWsServer(); + const { stdin, stdout } = mockStdio({}); + + setWranglerConfig({}); + msw.use( + http.get(`*/instances/:instanceId/ssh`, async () => { + return HttpResponse.json( + { success: true, result: { url: server.url, token: sshJwt } }, + { status: 200 } + ); + }) + ); + + const stdoutData = new Promise((resolve) => { + stdout.on("data", (chunk: Buffer) => resolve(chunk.toString())); + }); + const serverMessage = new Promise((resolve) => { + void server.connection.then((socket) => { + socket.on("message", (chunk) => resolve(chunk.toString())); + }); + }); + const wrangler = runWrangler(`containers ssh ${instanceId} -- 22`); + + const socket = await server.connection; + stdin.write("client-data"); + await expect(serverMessage).resolves.toBe("client-data"); + + socket.send("server-data"); + await expect(stdoutData).resolves.toBe("server-data"); + + stdin.end(); + socket.close(); + await wrangler; + server.ws.close(); + expect(std.out).toBe(""); }); }); + +async function createWsServer() { + const ws = new WebSocketServer({ port: 0 }); + await new Promise((resolve) => ws.on("listening", resolve)); + const address = ws.address(); + if (address === null || typeof address === "string") { + throw new Error("Expected WebSocket server to listen on a TCP port"); + } + const connection = new Promise((resolve) => { + ws.on("connection", resolve); + }); + return { ws, connection, url: `ws://127.0.0.1:${address.port}` }; +} + +function mockStdio({ + stdinIsTTY, + stdoutIsTTY, +}: { + stdinIsTTY?: boolean; + stdoutIsTTY?: boolean; +}) { + const stdin = new PassThrough(); + const stdout = new PassThrough(); + if (stdinIsTTY !== undefined) { + Object.defineProperty(stdin, "isTTY", { value: stdinIsTTY }); + } + if (stdoutIsTTY !== undefined) { + Object.defineProperty(stdout, "isTTY", { value: stdoutIsTTY }); + } + Object.defineProperty(process, "stdin", { value: stdin, configurable: true }); + Object.defineProperty(process, "stdout", { + value: stdout, + configurable: true, + }); + return { stdin, stdout }; +} diff --git a/packages/wrangler/src/containers/ssh.ts b/packages/wrangler/src/containers/ssh.ts index 8be7dd8dda..6181e01914 100644 --- a/packages/wrangler/src/containers/ssh.ts +++ b/packages/wrangler/src/containers/ssh.ts @@ -16,6 +16,7 @@ import type { HandlerArgs, NamedArgDefinitions } from "../core/types"; import type { WranglerSSHResponse } from "@cloudflare/containers-shared"; import type { Config } from "@cloudflare/workers-utils"; import type { Server } from "node:net"; +import type { RawData } from "ws"; // Deprecated SSH flags are hidden because a future SSH implementation // will not use OpenSSH, at which point these options will not work. @@ -91,6 +92,10 @@ const sshArgDefs = { hidden: true, deprecated: true, }, + stdio: { + describe: "Proxy SSH traffic over stdin/stdout", + type: "boolean", + }, } as const satisfies NamedArgDefinitions; type SshArgs = HandlerArgs; @@ -102,13 +107,19 @@ async function sshCommand(sshArgs: SshArgs, _config: Config) { }); } + // Get arguments passed to the SSH command itself. yargs includes + // "containers" and "ssh" as the first two elements of the array, which + // we don't want, so we don't include those. + const [, , ...rest] = sshArgs._; + const useStdio = shouldUseStdio(sshArgs); + // Check that ssh is enabled let sshResponse: WranglerSSHResponse; try { - sshResponse = await promiseSpinner( - DeploymentsService.containerWranglerSsh(sshArgs.ID), - { message: "Authenticating" } - ); + const sshPromise = DeploymentsService.containerWranglerSsh(sshArgs.ID); + sshResponse = useStdio + ? await sshPromise + : await promiseSpinner(sshPromise, { message: "Authenticating" }); } catch (e) { if (e instanceof ApiError) { if (e.status === 404) { @@ -126,7 +137,21 @@ async function sshCommand(sshArgs: SshArgs, _config: Config) { throw e; } - const proxy = createSshTcpProxy(sshResponse); + const ws = new WebSocket(sshResponse.url, { + headers: { + authorization: `Bearer ${sshResponse.token}`, + "user-agent": "wrangler", + }, + }); + + ws.binaryType = "arraybuffer"; + + if (useStdio) { + await stdioProxy(ws); + return; + } + + const proxy = tcpProxy(ws); const proxyController = new AbortController(); proxy.listen({ port: 0, signal: proxyController.signal }); @@ -137,11 +162,6 @@ async function sshCommand(sshArgs: SshArgs, _config: Config) { await verifySshInstalled("ssh"); - // Get arguments passed to the SSH command itself. yargs includes - // "containers" and "ssh" as the first two elements of the array, which - // we don't want, so we don't include those. - const [, , ...rest] = sshArgs._; - const child = spawn( "ssh", [ @@ -221,7 +241,7 @@ export function verifySshInstalled(sshPath: string): Promise { * Creates a local TCP proxy that wraps data sent in a websocket binary * websocket message */ -export function createSshTcpProxy(sshResponse: WranglerSSHResponse): Server { +function tcpProxy(ws: WebSocket): Server { let hasConnection = false; const proxy = createServer((inbound) => { if (hasConnection) { @@ -235,15 +255,6 @@ export function createSshTcpProxy(sshResponse: WranglerSSHResponse): Server { logger.error("Proxy error: ", err); }); - const ws = new WebSocket(sshResponse.url, { - headers: { - authorization: `Bearer ${sshResponse.token}`, - "user-agent": "wrangler", - }, - }); - - ws.binaryType = "arraybuffer"; - ws.addEventListener("error", (err) => { logger.error("Web socket error:", err.message); inbound.end(); @@ -251,9 +262,7 @@ export function createSshTcpProxy(sshResponse: WranglerSSHResponse): Server { }); ws.addEventListener("open", () => { - inbound.on("data", (data) => { - ws.send(data); - }); + inbound.on("data", (data) => ws.send(data)); inbound.on("close", () => { ws.close(); @@ -275,6 +284,57 @@ export function createSshTcpProxy(sshResponse: WranglerSSHResponse): Server { return proxy; } +function stdioProxy(ws: WebSocket): Promise { + return new Promise((resolve, reject) => { + const onData = (data: Buffer | string) => ws.send(data); + const onEnd = () => ws.close(); + const onError = (err: Error) => { + ws.close(); + reject(err); + }; + const cleanup = () => { + process.stdin.off("data", onData); + process.stdin.off("end", onEnd); + process.stdin.off("error", onError); + }; + + ws.addEventListener("error", (err) => { + cleanup(); + reject(new Error(`Web socket error: ${err.message}`)); + }); + + ws.addEventListener("open", () => { + process.stdin.on("data", onData); + process.stdin.on("end", onEnd); + process.stdin.on("error", onError); + process.stdin.resume(); + }); + + ws.on("message", (data: RawData) => { + process.stdout.write(formatWebSocketData(data)); + }); + + ws.addEventListener("close", () => { + cleanup(); + resolve(undefined); + }); + }); +} + +function formatWebSocketData(data: RawData) { + if (Array.isArray(data)) { + return Buffer.concat(data); + } + return data instanceof ArrayBuffer ? new Uint8Array(data) : data; +} + +function shouldUseStdio(sshArgs: SshArgs) { + return ( + sshArgs.stdio === true || + (process.stdin.isTTY !== true && process.stdout.isTTY !== true) + ); +} + function buildSshArgs(sshArgs: SshArgs): string[] { const flags = [ // Never use a control socket. @@ -345,6 +405,9 @@ export const containersSshCommand = createCommand({ status: "stable", owner: "Product: Cloudchamber", }, + behaviour: { + printBanner: (args) => !shouldUseStdio(args), + }, args: sshArgDefs, positionalArgs: ["ID"], async handler(args, { config }) { diff --git a/packages/wrangler/src/index.ts b/packages/wrangler/src/index.ts index 3313cab26b..bf0b4e65c6 100644 --- a/packages/wrangler/src/index.ts +++ b/packages/wrangler/src/index.ts @@ -2432,7 +2432,7 @@ export async function main(argv: string[]): Promise { // needed, so we can cleanly exit. Note, we don't want to disconnect if // this file was imported in Vitest, as that would stop communication with // the test runner. - if (typeof vitest === "undefined") { + if (typeof vitest === "undefined" && process.connected) { process.disconnect?.(); }