From 03e747487859a3f2bef26af5ee0e38f77e44ea0d Mon Sep 17 00:00:00 2001 From: Colum Ferry Date: Mon, 6 Jul 2026 12:33:21 +0100 Subject: [PATCH 1/8] fix(cli): default functions download --use-docker to true (Go parity) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Current Behavior Go defaults `--use-docker` to `true` on `functions download` (`cmd/functions.go:181`); the TS port omitted the default, so it resolved to `false`. This flipped the default download path (native HTTP vs. Docker-proxied) whenever no flags were passed, and once fixed naively, the mutex check between `--use-api`/`--use-docker`/ `--legacy-bundle` (truthiness-based) would start misfiring on the new default-true value, and `--use-api` alone would keep routing to Docker instead of overriding it the way Go's `if useApi { useDocker = false }` does. ## Expected Behavior - `useDocker` now defaults to `true` via `Flag.withDefault(true)`, matching Go. - The mutex check (`validateDownloadFlags`) now scans raw argv for explicit flag presence (`hasExplicitLongFlag`, hoisted out of `deploy.ts` into the shared `functions.shared.ts` family root, since it's now used by two commands) instead of checking parsed boolean truthiness — mirroring Go's `pflag.Changed`-based `MarkFlagsMutuallyExclusive`. - `downloadFunctions` gates the Docker/legacy-bundle proxy branch on `!explicitUseApi`, mirroring Go's `useApi` override. - The Go-proxy arg builder (`makeGoProxyDownloadArgs`) now forwards at most one of `--use-docker`/`--legacy-bundle` — previously it could forward both once `useDocker` defaults true, which the Go binary's own mutex check would then reject. - Slug validation now runs unconditionally, before the proxy-dispatch branch, so a malformed/traversal-shaped function name is rejected before ever reaching the Go proxy's argv. - `rawArgs` is threaded through `DownloadFunctionsDependencies` from the `Stdio.Stdio` service in both the legacy and next shell handlers (the shared `downloadFunctions` helper is used by both); `next`'s own `--use-docker` default is intentionally left unchanged, this is a legacy/Go-parity-only fix. ## Known follow-ups (not fixed here) - Go's own server-side unbundle extractor (`saveFile` in `apps/cli-go/internal/functions/download/download.go`) lacks the path-containment checks the native TS downloader already has — pre-existing Go CLI behavior, now reachable by default rather than via opt-in flags. Tracked as CLI-1891. - The Go-proxy path doesn't route through `execCapture` for `--output-format json|stream-json`, so machine output can be corrupted by inherited subprocess stdout (pre-existing, same as `deploy`'s already-shipped Docker default). - `hasExplicitLongFlag` detects flag token presence, not resolved value, so `--use-docker=false` isn't treated as disabling Docker. Pre-existing pattern shared with `deploy.ts`. Fixes CLI-1862 --- .../functions/download/SIDE_EFFECTS.md | 9 +- .../functions/download/download.command.ts | 1 + .../functions/download/download.e2e.test.ts | 83 ++++++++++ .../functions/download/download.handler.ts | 5 +- .../download/download.integration.test.ts | 154 +++++++++++++++++- .../functions/download/download.handler.ts | 5 +- .../download/download.integration.test.ts | 17 +- apps/cli/src/shared/functions/deploy.ts | 30 +--- apps/cli/src/shared/functions/download.ts | 45 +++-- .../src/shared/functions/functions.shared.ts | 30 ++++ 10 files changed, 329 insertions(+), 50 deletions(-) create mode 100644 apps/cli/src/legacy/commands/functions/download/download.e2e.test.ts diff --git a/apps/cli/src/legacy/commands/functions/download/SIDE_EFFECTS.md b/apps/cli/src/legacy/commands/functions/download/SIDE_EFFECTS.md index 7a70bcb517..f59462830d 100644 --- a/apps/cli/src/legacy/commands/functions/download/SIDE_EFFECTS.md +++ b/apps/cli/src/legacy/commands/functions/download/SIDE_EFFECTS.md @@ -22,9 +22,9 @@ ## Subprocesses -| Command | When | Purpose | -| ------------------------------------ | ----------------------------------- | ----------------------------------- | -| `supabase-go functions download ...` | `--use-docker` or `--legacy-bundle` | preserve hidden compatibility modes | +| Command | When | Purpose | +| ------------------------------------ | ----------------------------------------------------------------- | ----------------------------------- | +| `supabase-go functions download ...` | `--use-docker` (default) or `--legacy-bundle`, unless `--use-api` | preserve hidden compatibility modes | ## Environment Variables @@ -62,4 +62,7 @@ Prints a structured success result with the downloaded function slugs and projec - Requires a linked project (`--project-ref` or linked project config). - Native downloads reject path traversal and symlink escapes before writing source files. - `--use-docker` and `--legacy-bundle` are hidden flags forwarded to the Go binary for backward compatibility; they are mutually exclusive with `--use-api`. +- `--use-docker` defaults to `true` (Go parity), so a bare `supabase functions download` proxies to the Go binary's Docker-based unbundler unless `--use-api` is explicitly passed, which forces the native server-side download path instead. +- If Docker is not running, the Go binary itself prints `WARNING: Docker is not running` to stderr and falls back to its own server-side unbundler — the command still exits `0` without Docker installed or running. +- The mutual-exclusivity check only counts flags the user explicitly passed on the command line, not `--use-docker`'s default value — so `--use-api` alone never trips the "mutually exclusive" error. The Go proxy call itself also only ever forwards one of `--use-docker`/`--legacy-bundle`, never both, even though `--use-docker` defaults to `true`. - Refreshes the linked-project telemetry cache and flushes telemetry state after resolving a project ref. diff --git a/apps/cli/src/legacy/commands/functions/download/download.command.ts b/apps/cli/src/legacy/commands/functions/download/download.command.ts index 0d3f559dd0..1024b268fe 100644 --- a/apps/cli/src/legacy/commands/functions/download/download.command.ts +++ b/apps/cli/src/legacy/commands/functions/download/download.command.ts @@ -19,6 +19,7 @@ const config = { ), useDocker: Flag.boolean("use-docker").pipe( Flag.withDescription("Use Docker to unbundle functions locally."), + Flag.withDefault(true), Flag.withHidden, ), legacyBundle: Flag.boolean("legacy-bundle").pipe( diff --git a/apps/cli/src/legacy/commands/functions/download/download.e2e.test.ts b/apps/cli/src/legacy/commands/functions/download/download.e2e.test.ts new file mode 100644 index 0000000000..09f0405a95 --- /dev/null +++ b/apps/cli/src/legacy/commands/functions/download/download.e2e.test.ts @@ -0,0 +1,83 @@ +import { describe, expect, test } from "vitest"; +import { makeTempHome, runSupabase } from "../../../../../tests/helpers/cli.ts"; + +// Argument-validation negatives for `functions download`. This validation +// lives in the Go CLI today (the legacy TS command proxies to it); a +// black-box subprocess test keeps these assertions valid through the +// eventual native TS port — it guards behavior, not implementation. Mirrors +// `deploy.e2e.test.ts`'s coverage for the sibling `--use-docker` default bug +// (CLI-1862). +// +// All cases fail before any network call (flag parsing / mutex validation), +// so no auth or linked project is required. + +const E2E_TIMEOUT_MS = 30_000; +const SLUG = "download-e2e-basic"; +const FAKE_TOKEN = `sbp_${"0".repeat(40)}`; +const FAKE_REF = "a".repeat(20); + +describe("supabase functions download (legacy) — argument validation", () => { + const conflicts = [ + { name: "--use-api + --use-docker", flags: ["--use-api", "--use-docker"] }, + { name: "--use-api + --legacy-bundle", flags: ["--use-api", "--legacy-bundle"] }, + { name: "--use-docker + --legacy-bundle", flags: ["--use-docker", "--legacy-bundle"] }, + ] as const; + + for (const { name, flags } of conflicts) { + test(`rejects ${name} as mutually exclusive`, { timeout: E2E_TIMEOUT_MS }, async () => { + using home = makeTempHome(); + const { exitCode, stderr } = await runSupabase( + ["functions", "download", SLUG, "--project-ref", FAKE_REF, ...flags], + { + entrypoint: "legacy", + home: home.dir, + env: { HOME: home.dir, SUPABASE_ACCESS_TOKEN: FAKE_TOKEN }, + }, + ); + expect(exitCode).not.toBe(0); + expect(stderr).toMatch(/none of the others can be|mutually exclusive/i); + }); + } + + // CLI-1862: `--use-docker` now defaults to `true` (Go parity). Before the + // fix, that default was counted as "explicitly selected" by the mutex + // check, so passing `--use-api` alone was incorrectly rejected as + // conflicting with the (unpassed) `--use-docker` default. + test( + "does not reject --use-api alone despite the --use-docker default", + { timeout: E2E_TIMEOUT_MS }, + async () => { + using home = makeTempHome(); + const { stderr } = await runSupabase( + ["functions", "download", SLUG, "--project-ref", FAKE_REF, "--use-api"], + { + entrypoint: "legacy", + home: home.dir, + env: { HOME: home.dir, SUPABASE_ACCESS_TOKEN: FAKE_TOKEN }, + }, + ); + expect(stderr).not.toMatch(/none of the others can be|mutually exclusive/i); + }, + ); + + // CLI-1862: the TS→Go proxy call must not forward the now-defaulted + // `--use-docker` alongside an explicit `--legacy-bundle` — the Go binary + // re-parses this argv itself and enforces the same mutual exclusivity, + // so forwarding both breaks `--legacy-bundle` outright. + test( + "does not reject --legacy-bundle alone despite the --use-docker default", + { timeout: E2E_TIMEOUT_MS }, + async () => { + using home = makeTempHome(); + const { stderr } = await runSupabase( + ["functions", "download", SLUG, "--project-ref", FAKE_REF, "--legacy-bundle"], + { + entrypoint: "legacy", + home: home.dir, + env: { HOME: home.dir, SUPABASE_ACCESS_TOKEN: FAKE_TOKEN }, + }, + ); + expect(stderr).not.toMatch(/none of the others can be|mutually exclusive/i); + }, + ); +}); diff --git a/apps/cli/src/legacy/commands/functions/download/download.handler.ts b/apps/cli/src/legacy/commands/functions/download/download.handler.ts index 70cb9da73c..8da1559f90 100644 --- a/apps/cli/src/legacy/commands/functions/download/download.handler.ts +++ b/apps/cli/src/legacy/commands/functions/download/download.handler.ts @@ -1,4 +1,4 @@ -import { Effect, Option } from "effect"; +import { Effect, Option, Stdio } from "effect"; import { downloadFunctions, makeGoProxyDownloadArgs, @@ -20,11 +20,14 @@ export const legacyFunctionsDownload = Effect.fn("legacy.functions.download")(fu const linkedProjectCache = yield* LegacyLinkedProjectCache; const telemetryState = yield* LegacyTelemetryState; const proxy = yield* LegacyGoProxy; + const stdio = yield* Stdio.Stdio; + const rawArgs = yield* stdio.args; let resolvedProjectRef = Option.none(); yield* downloadFunctions(flags, { api, projectRoot: cliConfig.workdir, + rawArgs, resolveProjectRef: (projectRef) => resolver.resolve(projectRef).pipe( Effect.tap((ref) => diff --git a/apps/cli/src/legacy/commands/functions/download/download.integration.test.ts b/apps/cli/src/legacy/commands/functions/download/download.integration.test.ts index a6384d4e06..0643da0c99 100644 --- a/apps/cli/src/legacy/commands/functions/download/download.integration.test.ts +++ b/apps/cli/src/legacy/commands/functions/download/download.integration.test.ts @@ -1,7 +1,7 @@ import { describe, expect, it } from "@effect/vitest"; import { readFile } from "node:fs/promises"; import { join } from "node:path"; -import { Effect, Layer, Option } from "effect"; +import { Effect, Exit, Layer, Option, Stdio } from "effect"; import * as HttpClientResponse from "effect/unstable/http/HttpClientResponse"; import { @@ -86,6 +86,15 @@ describe("legacy functions download", () => { telemetry: telemetry.layer, }), proxy.layer, + Stdio.layerTest({ + args: Effect.succeed([ + "functions", + "download", + "hello-world", + "--project-ref", + "abcdefghijklmnopqrst", + ]), + }), ); return Effect.gen(function* () { @@ -108,7 +117,7 @@ describe("legacy functions download", () => { }).pipe(Effect.provide(layer)); }); - it.live("keeps hidden Docker compatibility mode behind the Go proxy", () => { + it.live("proxies to Docker by default (Go parity), with no flags passed", () => { const out = mockOutput({ format: "text" }); const api = mockLegacyPlatformApi(); const proxy = mockProxy(); @@ -119,9 +128,20 @@ describe("legacy functions download", () => { cliConfig: mockLegacyCliConfig({ workdir: tempRoot.current }), }), proxy.layer, + Stdio.layerTest({ + args: Effect.succeed([ + "functions", + "download", + "hello-world", + "--project-ref", + "abcdefghijklmnopqrst", + ]), + }), ); return Effect.gen(function* () { + // `useDocker: true` mirrors what the CLI parser now resolves to by + // default (CLI-1862) — no `--use-docker` flag appears in argv above. yield* legacyFunctionsDownload({ ...baseFlags, useDocker: true }); expect(api.requests).toEqual([]); @@ -137,4 +157,134 @@ describe("legacy functions download", () => { ]); }).pipe(Effect.provide(layer)); }); + + it.live( + "does not treat the --use-docker default as conflicting with an explicit --use-api", + () => { + const out = mockOutput({ format: "text" }); + const api = mockLegacyPlatformApi({ + handler: (request) => + request.url.endsWith("/body") + ? Effect.succeed(multipartResponse(request)) + : Effect.succeed(legacyJsonResponse(request, 200, {})), + }); + const proxy = mockProxy(); + const layer = Layer.mergeAll( + buildLegacyTestRuntime({ + out, + api, + cliConfig: mockLegacyCliConfig({ workdir: tempRoot.current }), + }), + proxy.layer, + Stdio.layerTest({ + args: Effect.succeed([ + "functions", + "download", + "hello-world", + "--use-api", + "--project-ref", + "abcdefghijklmnopqrst", + ]), + }), + ); + + return Effect.gen(function* () { + // The CLI parser resolves `useDocker: true` here too (its default), + // even though only `--use-api` was passed explicitly. Neither the + // mutex check nor the routing decision should treat that default as + // if the user had asked for Docker. + yield* legacyFunctionsDownload({ ...baseFlags, useApi: true, useDocker: true }); + + expect(proxy.calls).toEqual([]); + expect( + yield* Effect.tryPromise(() => + readFile( + join(tempRoot.current, "supabase", "functions", "hello-world", "index.ts"), + "utf8", + ), + ), + ).toBe("console.log('legacy native')"); + }).pipe(Effect.provide(layer)); + }, + ); + + it.live("forwards only --legacy-bundle to the Go proxy, not the --use-docker default too", () => { + const out = mockOutput({ format: "text" }); + const api = mockLegacyPlatformApi(); + const proxy = mockProxy(); + const layer = Layer.mergeAll( + buildLegacyTestRuntime({ + out, + api, + cliConfig: mockLegacyCliConfig({ workdir: tempRoot.current }), + }), + proxy.layer, + Stdio.layerTest({ + args: Effect.succeed([ + "functions", + "download", + "hello-world", + "--legacy-bundle", + "--project-ref", + "abcdefghijklmnopqrst", + ]), + }), + ); + + return Effect.gen(function* () { + // `useDocker: true` mirrors the CLI parser's default (CLI-1862) even + // though only `--legacy-bundle` was passed explicitly. The Go proxy + // call must not forward both, or the Go binary's own + // MarkFlagsMutuallyExclusive rejects the combination. + yield* legacyFunctionsDownload({ ...baseFlags, useDocker: true, legacyBundle: true }); + + expect(proxy.calls).toEqual([ + [ + "functions", + "download", + "hello-world", + "--project-ref", + "abcdefghijklmnopqrst", + "--legacy-bundle", + ], + ]); + }).pipe(Effect.provide(layer)); + }); + + it.live("rejects an invalid slug before ever reaching the Go proxy", () => { + const out = mockOutput({ format: "text" }); + const api = mockLegacyPlatformApi(); + const proxy = mockProxy(); + const layer = Layer.mergeAll( + buildLegacyTestRuntime({ + out, + api, + cliConfig: mockLegacyCliConfig({ workdir: tempRoot.current }), + }), + proxy.layer, + Stdio.layerTest({ + args: Effect.succeed([ + "functions", + "download", + "../../etc", + "--project-ref", + "abcdefghijklmnopqrst", + ]), + }), + ); + + return Effect.gen(function* () { + // `useDocker: true` is the real default (CLI-1862). Before this fix, + // slug validation only ran on the native path, so a malformed slug + // would sail past it and straight into the Go proxy argv. + const exit = yield* legacyFunctionsDownload({ + ...baseFlags, + functionName: Option.some("../../etc"), + useDocker: true, + }).pipe(Effect.exit); + + expect(Exit.isFailure(exit)).toBe(true); + expect(proxy.calls).toEqual([]); + }).pipe(Effect.provide(layer)); + }); }); diff --git a/apps/cli/src/next/commands/functions/download/download.handler.ts b/apps/cli/src/next/commands/functions/download/download.handler.ts index 02b9aed1b2..f24e04787e 100644 --- a/apps/cli/src/next/commands/functions/download/download.handler.ts +++ b/apps/cli/src/next/commands/functions/download/download.handler.ts @@ -1,4 +1,4 @@ -import { Effect } from "effect"; +import { Effect, Stdio } from "effect"; import { PlatformApi } from "../../../auth/platform-api.service.ts"; import { ProjectHome } from "../../../config/project-home.service.ts"; import { @@ -13,10 +13,13 @@ export const functionsDownload = Effect.fnUntraced(function* (flags: FunctionsDo const api = yield* PlatformApi; const projectHome = yield* ProjectHome; const proxy = yield* LegacyGoProxy; + const stdio = yield* Stdio.Stdio; + const rawArgs = yield* stdio.args; yield* downloadFunctions(flags, { api, projectRoot: projectHome.projectRoot, + rawArgs, resolveProjectRef, proxyDownload: (proxyFlags, projectRef) => proxy.exec(makeGoProxyDownloadArgs(proxyFlags, projectRef), { cwd: projectHome.projectRoot }), diff --git a/apps/cli/src/next/commands/functions/download/download.integration.test.ts b/apps/cli/src/next/commands/functions/download/download.integration.test.ts index b8095be511..68504e5a39 100644 --- a/apps/cli/src/next/commands/functions/download/download.integration.test.ts +++ b/apps/cli/src/next/commands/functions/download/download.integration.test.ts @@ -4,7 +4,7 @@ import { existsSync, mkdtempSync } from "node:fs"; import { mkdir, readFile, rm, symlink, writeFile } from "node:fs/promises"; import { tmpdir } from "node:os"; import { join } from "node:path"; -import { Effect, Layer, Option } from "effect"; +import { Effect, Layer, Option, Stdio } from "effect"; import * as HttpClient from "effect/unstable/http/HttpClient"; import * as HttpClientError from "effect/unstable/http/HttpClientError"; import * as HttpClientResponse from "effect/unstable/http/HttpClientResponse"; @@ -276,6 +276,7 @@ function setup( format?: "text" | "json" | "stream-json"; linked?: boolean; projectRoot?: string; + rawArgs?: ReadonlyArray; } = {}, ) { const out = mockOutput({ format: opts.format ?? "text", interactive: false }); @@ -289,6 +290,7 @@ function setup( mockRuntimeInfo({ cwd }), mockProjectLinkState(opts.linked === false ? undefined : LINK_STATE), mockProjectHome(opts.projectRoot ?? cwd), + Stdio.layerTest({ args: Effect.succeed(opts.rawArgs ?? []) }), ); return { out, api, layer, proxy }; @@ -803,6 +805,7 @@ describe("functions download", () => { bodyBySlug: { "hello-world": multipart, }, + rawArgs: ["functions", "download", "hello-world", "--use-api"], }); yield* functionsDownload({ @@ -821,7 +824,9 @@ describe("functions download", () => { return Effect.gen(function* () { yield* Effect.tryPromise(() => writeProjectConfig(tempDir)); - const { layer, proxy } = setup(tempDir); + const { layer, proxy } = setup(tempDir, { + rawArgs: ["functions", "download", "hello-world", "--legacy-bundle"], + }); yield* functionsDownload({ ...BASE_FLAGS, @@ -841,7 +846,9 @@ describe("functions download", () => { return Effect.gen(function* () { yield* Effect.tryPromise(() => writeProjectConfig(tempDir)); - const { layer, proxy } = setup(tempDir); + const { layer, proxy } = setup(tempDir, { + rawArgs: ["functions", "download", "hello-world", "--use-docker"], + }); yield* functionsDownload({ ...BASE_FLAGS, @@ -860,7 +867,9 @@ describe("functions download", () => { const tempDir = makeTempDir(); return Effect.gen(function* () { - const { api, layer, proxy } = setup(tempDir); + const { api, layer, proxy } = setup(tempDir, { + rawArgs: ["functions", "download", "hello-world", "--use-api", "--legacy-bundle"], + }); const error = yield* functionsDownload({ ...BASE_FLAGS, diff --git a/apps/cli/src/shared/functions/deploy.ts b/apps/cli/src/shared/functions/deploy.ts index 46bab06023..e0f828c340 100644 --- a/apps/cli/src/shared/functions/deploy.ts +++ b/apps/cli/src/shared/functions/deploy.ts @@ -16,7 +16,11 @@ import { Output } from "../output/output.service.ts"; import { spawnContainerCli } from "../../legacy/shared/legacy-container-cli.ts"; import { legacyGetRegistryImageUrl } from "../../legacy/shared/legacy-docker-registry.ts"; import { findGitRootPath } from "../git/git-root.ts"; -import { invalidFunctionSlugDetail, validateFunctionSlugMessage } from "./functions.shared.ts"; +import { + hasExplicitLongFlag, + invalidFunctionSlugDetail, + validateFunctionSlugMessage, +} from "./functions.shared.ts"; import { ConflictingFunctionDeployFlagsError, FunctionDeployCancelledError, @@ -192,30 +196,6 @@ function validateDeploySlug(slug: string): Effect.Effect, - commandPath: ReadonlyArray, - flagName: string, -): boolean { - const commandIndex = rawArgs.findIndex((_, index) => - commandPath.every((segment, offset) => rawArgs[index + offset] === segment), - ); - if (commandIndex === -1) { - return rawArgs.some((token) => token === `--${flagName}` || token.startsWith(`--${flagName}=`)); - } - - for (let index = commandIndex + commandPath.length; index < rawArgs.length; index += 1) { - const token = rawArgs[index]; - if (token === undefined || token === "--") { - return false; - } - if (token === `--${flagName}` || token.startsWith(`--${flagName}=`)) { - return true; - } - } - return false; -} - function explicitBooleanFlag( rawArgs: ReadonlyArray, commandPath: ReadonlyArray, diff --git a/apps/cli/src/shared/functions/download.ts b/apps/cli/src/shared/functions/download.ts index 562066ee4b..c9df500990 100644 --- a/apps/cli/src/shared/functions/download.ts +++ b/apps/cli/src/shared/functions/download.ts @@ -7,7 +7,11 @@ import { Effect, FileSystem, Option } from "effect"; import * as HttpClientError from "effect/unstable/http/HttpClientError"; import type * as HttpClientResponse from "effect/unstable/http/HttpClientResponse"; import { Output } from "../output/output.service.ts"; -import { invalidFunctionSlugDetail, validateFunctionSlugMessage } from "./functions.shared.ts"; +import { + hasExplicitLongFlag, + invalidFunctionSlugDetail, + validateFunctionSlugMessage, +} from "./functions.shared.ts"; import { ConflictingFunctionDownloadFlagsError, FunctionDownloadNotFoundError, @@ -34,6 +38,7 @@ export interface DownloadFunctionsDependencies< > { readonly api: ApiClient; readonly projectRoot: string; + readonly rawArgs: ReadonlyArray; readonly resolveProjectRef: ( projectRef: Option.Option, ) => Effect.Effect; @@ -57,11 +62,14 @@ export function makeGoProxyDownloadArgs( args.push(flags.functionName.value); } args.push("--project-ref", projectRef); - if (flags.useDocker) { - args.push("--use-docker"); - } + // At most one of these may reach the Go binary — it re-parses this argv + // fresh and enforces the same mutual exclusivity itself. `legacyBundle` + // takes priority since `useDocker` now defaults to `true` (CLI-1862) and + // would otherwise ride along on every `--legacy-bundle` invocation. if (flags.legacyBundle) { args.push("--legacy-bundle"); + } else if (flags.useDocker) { + args.push("--use-docker"); } return args; } @@ -107,13 +115,17 @@ function validateSlug(slug: string): Effect.Effect, ): Effect.Effect { const selected = [ - flags.useApi ? "--use-api" : undefined, - flags.useDocker ? "--use-docker" : undefined, - flags.legacyBundle ? "--legacy-bundle" : undefined, + hasExplicitLongFlag(rawArgs, downloadCommandPath, "use-api") ? "--use-api" : undefined, + hasExplicitLongFlag(rawArgs, downloadCommandPath, "use-docker") ? "--use-docker" : undefined, + hasExplicitLongFlag(rawArgs, downloadCommandPath, "legacy-bundle") + ? "--legacy-bundle" + : undefined, ].filter((flag) => flag !== undefined); return selected.length <= 1 @@ -734,17 +746,22 @@ export function downloadFunctions, + commandPath: ReadonlyArray, + flagName: string, +): boolean { + const commandIndex = rawArgs.findIndex((_, index) => + commandPath.every((segment, offset) => rawArgs[index + offset] === segment), + ); + if (commandIndex === -1) { + return rawArgs.some((token) => token === `--${flagName}` || token.startsWith(`--${flagName}=`)); + } + + for (let index = commandIndex + commandPath.length; index < rawArgs.length; index += 1) { + const token = rawArgs[index]; + if (token === undefined || token === "--") { + return false; + } + if (token === `--${flagName}` || token.startsWith(`--${flagName}=`)) { + return true; + } + } + return false; +} From 2e2496191ef310482aa96fe562677bad0af2db88 Mon Sep 17 00:00:00 2001 From: Colum Ferry Date: Mon, 6 Jul 2026 13:15:24 +0100 Subject: [PATCH 2/8] fix(cli): address Codex review feedback on functions download proxy path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four issues Codex raised against the --use-docker default-to-true change, each verified against apps/cli-go/ and fixed: - Gate the Docker/legacy-bundle proxy branch on the parsed `flags.useApi` value instead of raw-argv presence, matching Go's value-based `if useApi { useDocker = false }` (cmd/functions.go:51-53) — an explicit `--use-api=false` was being treated like `--use-api`, wrongly skipping the proxy path Go would still take. - Capture/discard the Go proxy's stdout via `execCapture` in machine-output mode instead of inheriting it, and emit the `Output` envelope from `downloadFunctions` itself, so `--output-format json|stream-json` no longer loses its structured payload now that the proxy branch is the default path (CLI-1546). - Run the delegated Go call with `SUPABASE_TELEMETRY_DISABLED=1` (mirrors `db pull`/`db diff`'s delegated-call pattern) so the child's own `cli_command_executed` doesn't double-count on top of this command's own telemetry now that the proxy branch is the default. - Drop the `--legacy-bundle`-alone e2e case: it delegates to Go's `RunLegacy`, which installs Deno before any network call, so every e2e run would trigger a real, uncached download from GitHub. Equivalent coverage already exists in download.integration.test.ts via a mocked proxy. --- .../functions/download/SIDE_EFFECTS.md | 16 ++- .../functions/download/download.e2e.test.ts | 33 ++--- .../functions/download/download.handler.ts | 19 ++- .../download/download.integration.test.ts | 117 +++++++++++++++++- .../functions/download/download.handler.ts | 12 +- .../download/download.integration.test.ts | 47 ++++++- apps/cli/src/shared/functions/download.ts | 39 ++++-- 7 files changed, 246 insertions(+), 37 deletions(-) diff --git a/apps/cli/src/legacy/commands/functions/download/SIDE_EFFECTS.md b/apps/cli/src/legacy/commands/functions/download/SIDE_EFFECTS.md index f59462830d..9ed1bce3aa 100644 --- a/apps/cli/src/legacy/commands/functions/download/SIDE_EFFECTS.md +++ b/apps/cli/src/legacy/commands/functions/download/SIDE_EFFECTS.md @@ -26,6 +26,14 @@ | ------------------------------------ | ----------------------------------------------------------------- | ----------------------------------- | | `supabase-go functions download ...` | `--use-docker` (default) or `--legacy-bundle`, unless `--use-api` | preserve hidden compatibility modes | +The delegated call runs with `SUPABASE_TELEMETRY_DISABLED=1` so the Go child's +own `cli_command_executed` doesn't double-count on top of this command's own +telemetry (mirrors `db pull`/`db diff`'s delegated-call pattern). In +`--output-format json|stream-json`, the child's stdout is captured and +discarded instead of inherited (`LegacyGoProxy.execCapture`) — the raw text +never reaches the terminal, and this command emits the `Output` envelope +itself once the child exits successfully. + ## Environment Variables | Variable | Purpose | Required? | @@ -50,11 +58,13 @@ Prints progress and success messages as functions are downloaded. ### `--output-format json` -Prints a structured success result with the downloaded function slugs and project ref. +Prints a structured success result with the downloaded function slugs and project ref. On the +Docker/legacy-bundle proxy path, the Go child's stdout is captured/discarded (never inherited) so +it can't corrupt the envelope; the slug list is resolved independently for the payload. ### `--output-format stream-json` -Prints a structured success result with the downloaded function slugs and project ref. +Same envelope as `json` above (including on the proxy path). ## Notes @@ -62,7 +72,7 @@ Prints a structured success result with the downloaded function slugs and projec - Requires a linked project (`--project-ref` or linked project config). - Native downloads reject path traversal and symlink escapes before writing source files. - `--use-docker` and `--legacy-bundle` are hidden flags forwarded to the Go binary for backward compatibility; they are mutually exclusive with `--use-api`. -- `--use-docker` defaults to `true` (Go parity), so a bare `supabase functions download` proxies to the Go binary's Docker-based unbundler unless `--use-api` is explicitly passed, which forces the native server-side download path instead. +- `--use-docker` defaults to `true` (Go parity), so a bare `supabase functions download` proxies to the Go binary's Docker-based unbundler unless `--use-api` resolves to `true`, which forces the native server-side download path instead (`apps/cli-go/cmd/functions.go:51-53`: `if useApi { useDocker = false }` reads the resolved flag value, not presence — `--use-api=false` still proxies). - If Docker is not running, the Go binary itself prints `WARNING: Docker is not running` to stderr and falls back to its own server-side unbundler — the command still exits `0` without Docker installed or running. - The mutual-exclusivity check only counts flags the user explicitly passed on the command line, not `--use-docker`'s default value — so `--use-api` alone never trips the "mutually exclusive" error. The Go proxy call itself also only ever forwards one of `--use-docker`/`--legacy-bundle`, never both, even though `--use-docker` defaults to `true`. - Refreshes the linked-project telemetry cache and flushes telemetry state after resolving a project ref. diff --git a/apps/cli/src/legacy/commands/functions/download/download.e2e.test.ts b/apps/cli/src/legacy/commands/functions/download/download.e2e.test.ts index 09f0405a95..3499bb2a8a 100644 --- a/apps/cli/src/legacy/commands/functions/download/download.e2e.test.ts +++ b/apps/cli/src/legacy/commands/functions/download/download.e2e.test.ts @@ -8,8 +8,8 @@ import { makeTempHome, runSupabase } from "../../../../../tests/helpers/cli.ts"; // `deploy.e2e.test.ts`'s coverage for the sibling `--use-docker` default bug // (CLI-1862). // -// All cases fail before any network call (flag parsing / mutex validation), -// so no auth or linked project is required. +// The mutex-conflict cases below fail before any network call (flag parsing +// / mutex validation), so no auth or linked project is required. const E2E_TIMEOUT_MS = 30_000; const SLUG = "download-e2e-basic"; @@ -62,22 +62,15 @@ describe("supabase functions download (legacy) — argument validation", () => { // CLI-1862: the TS→Go proxy call must not forward the now-defaulted // `--use-docker` alongside an explicit `--legacy-bundle` — the Go binary - // re-parses this argv itself and enforces the same mutual exclusivity, - // so forwarding both breaks `--legacy-bundle` outright. - test( - "does not reject --legacy-bundle alone despite the --use-docker default", - { timeout: E2E_TIMEOUT_MS }, - async () => { - using home = makeTempHome(); - const { stderr } = await runSupabase( - ["functions", "download", SLUG, "--project-ref", FAKE_REF, "--legacy-bundle"], - { - entrypoint: "legacy", - home: home.dir, - env: { HOME: home.dir, SUPABASE_ACCESS_TOKEN: FAKE_TOKEN }, - }, - ); - expect(stderr).not.toMatch(/none of the others can be|mutually exclusive/i); - }, - ); + // re-parses this argv itself and enforces the same mutual exclusivity, so + // forwarding both breaks `--legacy-bundle` outright. Covered in + // `download.integration.test.ts` ("forwards only --legacy-bundle to the Go + // proxy...") via a mocked `LegacyGoProxy` instead of here: unlike + // `--use-api`, `--legacy-bundle` routes to the Go binary's `RunLegacy` + // downloader, which calls `InstallOrUpgradeDeno` before any network call + // (`apps/cli-go/internal/functions/download/download.go`). Each e2e run + // gets a fresh `SUPABASE_HOME`, so this would trigger a real, uncached + // Deno download from GitHub on every run — a real cross-boundary + // dependency this suite shouldn't take on to prove a pure TS-side routing + // decision. }); diff --git a/apps/cli/src/legacy/commands/functions/download/download.handler.ts b/apps/cli/src/legacy/commands/functions/download/download.handler.ts index 8da1559f90..4b666da7f6 100644 --- a/apps/cli/src/legacy/commands/functions/download/download.handler.ts +++ b/apps/cli/src/legacy/commands/functions/download/download.handler.ts @@ -36,8 +36,23 @@ export const legacyFunctionsDownload = Effect.fn("legacy.functions.download")(fu }), ), ), - proxyDownload: (proxyFlags, projectRef) => - proxy.exec(makeGoProxyDownloadArgs(proxyFlags, projectRef)), + // The delegated Go binary runs its own `Execute()` and would otherwise + // fire its own `cli_command_executed` on top of this command's own + // `withLegacyCommandInstrumentation` wrapper. Suppress it so proxied + // invocations record exactly one event, matching Go (mirrors `db pull` / + // `db diff`'s delegated-call pattern). + // + // In machine-output mode the child's stdout is captured and discarded + // instead of inherited, matching `db pull`/`db diff`'s delegated-call + // pattern for the CLI-1546 "stdout is payload-only in machine mode" + // invariant — `downloadFunctions` emits the `Output` envelope itself. + proxyDownload: (proxyFlags, projectRef, captureOutput) => { + const args = makeGoProxyDownloadArgs(proxyFlags, projectRef); + const env = { SUPABASE_TELEMETRY_DISABLED: "1" }; + return captureOutput + ? Effect.asVoid(proxy.execCapture(args, { env, stdin: "ignore" })) + : proxy.exec(args, { env }); + }, }).pipe( Effect.ensuring( Effect.suspend(() => diff --git a/apps/cli/src/legacy/commands/functions/download/download.integration.test.ts b/apps/cli/src/legacy/commands/functions/download/download.integration.test.ts index 0643da0c99..e127235cf3 100644 --- a/apps/cli/src/legacy/commands/functions/download/download.integration.test.ts +++ b/apps/cli/src/legacy/commands/functions/download/download.integration.test.ts @@ -53,14 +53,26 @@ function multipartResponse(request: Parameters> = []; + const envs: Array | undefined> = []; + const captureCalls: Array> = []; + const captureEnvs: Array | undefined> = []; return { calls, + envs, + captureCalls, + captureEnvs, layer: Layer.succeed(LegacyGoProxy, { - exec: (args) => + exec: (args, opts) => Effect.sync(() => { calls.push([...args]); + envs.push(opts?.env); + }), + execCapture: (args, opts) => + Effect.sync(() => { + captureCalls.push([...args]); + captureEnvs.push(opts?.env); + return ""; }), - execCapture: () => Effect.succeed(""), }), }; } @@ -155,6 +167,9 @@ describe("legacy functions download", () => { "--use-docker", ], ]); + // The delegated Go binary must not also fire its own + // `cli_command_executed` on top of this command's own instrumentation. + expect(proxy.envs).toEqual([{ SUPABASE_TELEMETRY_DISABLED: "1" }]); }).pipe(Effect.provide(layer)); }); @@ -208,6 +223,103 @@ describe("legacy functions download", () => { }, ); + it.live("still proxies to Docker when --use-api=false is passed explicitly", () => { + const out = mockOutput({ format: "text" }); + const api = mockLegacyPlatformApi(); + const proxy = mockProxy(); + const layer = Layer.mergeAll( + buildLegacyTestRuntime({ + out, + api, + cliConfig: mockLegacyCliConfig({ workdir: tempRoot.current }), + }), + proxy.layer, + Stdio.layerTest({ + args: Effect.succeed([ + "functions", + "download", + "hello-world", + "--use-api=false", + "--project-ref", + "abcdefghijklmnopqrst", + ]), + }), + ); + + return Effect.gen(function* () { + // Go's override is value-based (`if useApi { useDocker = false }`, + // apps/cli-go/cmd/functions.go:51-53), not presence-based. An explicit + // `--use-api=false` must not be treated like `--use-api` — it should + // leave the `--use-docker` default (true) in effect and still proxy. + yield* legacyFunctionsDownload({ ...baseFlags, useApi: false, useDocker: true }); + + expect(api.requests).toEqual([]); + expect(proxy.calls).toEqual([ + [ + "functions", + "download", + "hello-world", + "--project-ref", + "abcdefghijklmnopqrst", + "--use-docker", + ], + ]); + expect(proxy.envs).toEqual([{ SUPABASE_TELEMETRY_DISABLED: "1" }]); + }).pipe(Effect.provide(layer)); + }); + + it.live("emits a JSON success envelope when proxying to Docker in machine-output mode", () => { + const out = mockOutput({ format: "json" }); + const api = mockLegacyPlatformApi(); + const proxy = mockProxy(); + const layer = Layer.mergeAll( + buildLegacyTestRuntime({ + out, + api, + cliConfig: mockLegacyCliConfig({ workdir: tempRoot.current }), + }), + proxy.layer, + Stdio.layerTest({ + args: Effect.succeed([ + "functions", + "download", + "hello-world", + "--project-ref", + "abcdefghijklmnopqrst", + "--output-format", + "json", + ]), + }), + ); + + return Effect.gen(function* () { + // CLI-1546: stdout is payload-only in machine mode, so the Go child's + // raw output must be captured/discarded (not inherited) and this + // command must emit the `Output` envelope itself, matching the native + // path's shape. + yield* legacyFunctionsDownload({ ...baseFlags, useDocker: true }); + + expect(proxy.calls).toEqual([]); + expect(proxy.captureCalls).toEqual([ + [ + "functions", + "download", + "hello-world", + "--project-ref", + "abcdefghijklmnopqrst", + "--use-docker", + ], + ]); + expect(proxy.captureEnvs).toEqual([{ SUPABASE_TELEMETRY_DISABLED: "1" }]); + expect(out.messages).toContainEqual( + expect.objectContaining({ + type: "success", + data: { function_slugs: ["hello-world"], project_ref: "abcdefghijklmnopqrst" }, + }), + ); + }).pipe(Effect.provide(layer)); + }); + it.live("forwards only --legacy-bundle to the Go proxy, not the --use-docker default too", () => { const out = mockOutput({ format: "text" }); const api = mockLegacyPlatformApi(); @@ -248,6 +360,7 @@ describe("legacy functions download", () => { "--legacy-bundle", ], ]); + expect(proxy.envs).toEqual([{ SUPABASE_TELEMETRY_DISABLED: "1" }]); }).pipe(Effect.provide(layer)); }); diff --git a/apps/cli/src/next/commands/functions/download/download.handler.ts b/apps/cli/src/next/commands/functions/download/download.handler.ts index f24e04787e..15c6238e84 100644 --- a/apps/cli/src/next/commands/functions/download/download.handler.ts +++ b/apps/cli/src/next/commands/functions/download/download.handler.ts @@ -21,7 +21,15 @@ export const functionsDownload = Effect.fnUntraced(function* (flags: FunctionsDo projectRoot: projectHome.projectRoot, rawArgs, resolveProjectRef, - proxyDownload: (proxyFlags, projectRef) => - proxy.exec(makeGoProxyDownloadArgs(proxyFlags, projectRef), { cwd: projectHome.projectRoot }), + // In machine-output mode the child's stdout is captured and discarded + // instead of inherited (CLI-1546: stdout is payload-only in machine + // mode) — `downloadFunctions` emits the `Output` envelope itself. + proxyDownload: (proxyFlags, projectRef, captureOutput) => { + const args = makeGoProxyDownloadArgs(proxyFlags, projectRef); + const cwd = projectHome.projectRoot; + return captureOutput + ? Effect.asVoid(proxy.execCapture(args, { cwd, stdin: "ignore" })) + : proxy.exec(args, { cwd }); + }, }); }); diff --git a/apps/cli/src/next/commands/functions/download/download.integration.test.ts b/apps/cli/src/next/commands/functions/download/download.integration.test.ts index 68504e5a39..31d8cd2d2b 100644 --- a/apps/cli/src/next/commands/functions/download/download.integration.test.ts +++ b/apps/cli/src/next/commands/functions/download/download.integration.test.ts @@ -298,17 +298,25 @@ function setup( function mockLegacyGoProxy() { const calls: string[][] = []; + const captureCalls: string[][] = []; return { layer: Layer.succeed(LegacyGoProxy, { exec: (args: ReadonlyArray) => Effect.sync(() => { calls.push([...args]); }), - execCapture: () => Effect.succeed(""), + execCapture: (args: ReadonlyArray) => + Effect.sync(() => { + captureCalls.push([...args]); + return ""; + }), }), get calls() { return calls; }, + get captureCalls() { + return captureCalls; + }, }; } @@ -863,6 +871,43 @@ describe("functions download", () => { ); }); + it.live("captures the Go proxy's output and emits a JSON envelope in machine mode", () => { + const tempDir = makeTempDir(); + + return Effect.gen(function* () { + yield* Effect.tryPromise(() => writeProjectConfig(tempDir)); + const { out, layer, proxy } = setup(tempDir, { + format: "json", + rawArgs: ["functions", "download", "hello-world", "--use-docker"], + }); + + // CLI-1546: stdout is payload-only in machine mode, so the delegated + // Go child's raw output must be captured/discarded (not inherited), + // and this command must emit the `Output` envelope itself. + yield* functionsDownload({ + ...BASE_FLAGS, + useDocker: true, + }).pipe(Effect.provide(layer)); + + expect(proxy.calls).toEqual([]); + expect(proxy.captureCalls).toEqual([ + ["functions", "download", "hello-world", "--project-ref", PROJECT_REF, "--use-docker"], + ]); + expect(out.messages).toContainEqual( + expect.objectContaining({ + type: "success", + message: "Downloaded Edge Function source.", + data: { + function_slugs: ["hello-world"], + project_ref: PROJECT_REF, + }, + }), + ); + }).pipe( + Effect.ensuring(Effect.tryPromise(() => rm(tempDir, { recursive: true, force: true }))), + ); + }); + it.live("rejects mutually exclusive compatibility flags", () => { const tempDir = makeTempDir(); diff --git a/apps/cli/src/shared/functions/download.ts b/apps/cli/src/shared/functions/download.ts index c9df500990..5ea14c3954 100644 --- a/apps/cli/src/shared/functions/download.ts +++ b/apps/cli/src/shared/functions/download.ts @@ -42,9 +42,17 @@ export interface DownloadFunctionsDependencies< readonly resolveProjectRef: ( projectRef: Option.Option, ) => Effect.Effect; + /** + * `captureOutput` is `true` whenever `output.format !== "text"`: the Go + * child's raw stdout must not reach the terminal (it would corrupt the + * JSON/NDJSON envelope, CLI-1546's "stdout is payload-only in machine + * mode" invariant), so the dependency must capture/discard it (e.g. via + * `LegacyGoProxy.execCapture`) instead of inheriting stdio. + */ readonly proxyDownload: ( flags: DownloadFunctionsOptions, projectRef: string, + captureOutput: boolean, ) => Effect.Effect; } @@ -752,14 +760,31 @@ export function downloadFunctions Date: Mon, 6 Jul 2026 14:05:04 +0100 Subject: [PATCH 3/8] fix(cli): address second round of Codex review on functions download MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two more issues from Codex's re-review of 2e249619, both fixed; a third was investigated and found to be a false positive (documented in the reply rather than "fixed"): - List remote function slugs before delegating to the Go proxy in machine-output mode, not after. The Go child never reports which slugs it downloaded, so this list only exists to build the JSON payload; doing it first mirrors Go's own `downloadAll` (which also lists before looping) and means a transient listing failure is reported before any download side effect, instead of masking an already-successful delegated download with an unrelated post-hoc listing failure. - Drop the "--use-api alone" e2e case: now that the mutex fix routes --use-api to the native downloader correctly, this case no longer fails during argument validation — it proceeds to a real Management API call with a fake token/ref. Equivalent (better) coverage already exists in download.integration.test.ts via a mocked platform API. - Investigated the claimed "missing Stdio service" regression in hidden-flag.unit.test.ts: confirmed via the actual test run (and a temporary debug trace) that it passes and does not crash — `withEnv()`'s `BunServices.layer` already satisfies the `Stdio.Stdio` requirement. No code change; explained in the review reply instead. --- .../functions/download/download.e2e.test.ts | 24 ++-- .../download/download.integration.test.ts | 104 ++++++++++++++++++ .../download/download.integration.test.ts | 69 ++++++++++++ apps/cli/src/shared/functions/download.ts | 18 ++- 4 files changed, 193 insertions(+), 22 deletions(-) diff --git a/apps/cli/src/legacy/commands/functions/download/download.e2e.test.ts b/apps/cli/src/legacy/commands/functions/download/download.e2e.test.ts index 3499bb2a8a..f52a23d3be 100644 --- a/apps/cli/src/legacy/commands/functions/download/download.e2e.test.ts +++ b/apps/cli/src/legacy/commands/functions/download/download.e2e.test.ts @@ -42,23 +42,13 @@ describe("supabase functions download (legacy) — argument validation", () => { // CLI-1862: `--use-docker` now defaults to `true` (Go parity). Before the // fix, that default was counted as "explicitly selected" by the mutex // check, so passing `--use-api` alone was incorrectly rejected as - // conflicting with the (unpassed) `--use-docker` default. - test( - "does not reject --use-api alone despite the --use-docker default", - { timeout: E2E_TIMEOUT_MS }, - async () => { - using home = makeTempHome(); - const { stderr } = await runSupabase( - ["functions", "download", SLUG, "--project-ref", FAKE_REF, "--use-api"], - { - entrypoint: "legacy", - home: home.dir, - env: { HOME: home.dir, SUPABASE_ACCESS_TOKEN: FAKE_TOKEN }, - }, - ); - expect(stderr).not.toMatch(/none of the others can be|mutually exclusive/i); - }, - ); + // conflicting with the (unpassed) `--use-docker` default. Covered in + // `download.integration.test.ts` ("does not treat the --use-docker default + // as conflicting with an explicit --use-api") via a mocked platform API + // instead of here: now that the mutex check is fixed, `--use-api` alone + // passes validation and proceeds to the native downloader, which calls the + // real Management API with the fake token/ref — an argument-validation + // test shouldn't depend on that network round-trip. // CLI-1862: the TS→Go proxy call must not forward the now-defaulted // `--use-docker` alongside an explicit `--legacy-bundle` — the Go binary diff --git a/apps/cli/src/legacy/commands/functions/download/download.integration.test.ts b/apps/cli/src/legacy/commands/functions/download/download.integration.test.ts index e127235cf3..b25bb51f99 100644 --- a/apps/cli/src/legacy/commands/functions/download/download.integration.test.ts +++ b/apps/cli/src/legacy/commands/functions/download/download.integration.test.ts @@ -320,6 +320,110 @@ describe("legacy functions download", () => { }).pipe(Effect.provide(layer)); }); + it.live( + "lists remote functions before delegating when no function name is given in machine mode", + () => { + const out = mockOutput({ format: "json" }); + const api = mockLegacyPlatformApi({ + handler: (request) => + request.url.endsWith("/functions") + ? Effect.succeed( + legacyJsonResponse(request, 200, [ + { slug: "hello-world" }, + { slug: "goodbye-world" }, + ]), + ) + : Effect.succeed(legacyJsonResponse(request, 200, {})), + }); + const proxy = mockProxy(); + const layer = Layer.mergeAll( + buildLegacyTestRuntime({ + out, + api, + cliConfig: mockLegacyCliConfig({ workdir: tempRoot.current }), + }), + proxy.layer, + Stdio.layerTest({ + args: Effect.succeed([ + "functions", + "download", + "--project-ref", + "abcdefghijklmnopqrst", + "--output-format", + "json", + ]), + }), + ); + + return Effect.gen(function* () { + yield* legacyFunctionsDownload({ + ...baseFlags, + functionName: Option.none(), + useDocker: true, + }); + + expect(proxy.calls).toEqual([]); + expect(proxy.captureCalls).toEqual([ + ["functions", "download", "--project-ref", "abcdefghijklmnopqrst", "--use-docker"], + ]); + expect(out.messages).toContainEqual( + expect.objectContaining({ + type: "success", + data: { + function_slugs: ["hello-world", "goodbye-world"], + project_ref: "abcdefghijklmnopqrst", + }, + }), + ); + }).pipe(Effect.provide(layer)); + }, + ); + + it.live("fails before delegating when the pre-flight function list fails in machine mode", () => { + const out = mockOutput({ format: "json" }); + const api = mockLegacyPlatformApi({ + handler: (request) => + request.url.endsWith("/functions") + ? Effect.succeed(legacyJsonResponse(request, 500, { message: "unavailable" })) + : Effect.succeed(legacyJsonResponse(request, 200, {})), + }); + const proxy = mockProxy(); + const layer = Layer.mergeAll( + buildLegacyTestRuntime({ + out, + api, + cliConfig: mockLegacyCliConfig({ workdir: tempRoot.current }), + }), + proxy.layer, + Stdio.layerTest({ + args: Effect.succeed([ + "functions", + "download", + "--project-ref", + "abcdefghijklmnopqrst", + "--output-format", + "json", + ]), + }), + ); + + return Effect.gen(function* () { + // The pre-flight list failure must be reported before any download + // side effect — the delegated proxy must never be invoked (CLI-1862 + // review: a listing failure after a successful delegated download + // must not mask that success). + const exit = yield* legacyFunctionsDownload({ + ...baseFlags, + functionName: Option.none(), + useDocker: true, + }).pipe(Effect.exit); + + expect(Exit.isFailure(exit)).toBe(true); + expect(proxy.calls).toEqual([]); + expect(proxy.captureCalls).toEqual([]); + }).pipe(Effect.provide(layer)); + }); + it.live("forwards only --legacy-bundle to the Go proxy, not the --use-docker default too", () => { const out = mockOutput({ format: "text" }); const api = mockLegacyPlatformApi(); diff --git a/apps/cli/src/next/commands/functions/download/download.integration.test.ts b/apps/cli/src/next/commands/functions/download/download.integration.test.ts index 31d8cd2d2b..7979fbc8b1 100644 --- a/apps/cli/src/next/commands/functions/download/download.integration.test.ts +++ b/apps/cli/src/next/commands/functions/download/download.integration.test.ts @@ -908,6 +908,75 @@ describe("functions download", () => { ); }); + it.live( + "lists remote functions before delegating when no function name is given in machine mode", + () => { + const tempDir = makeTempDir(); + + return Effect.gen(function* () { + yield* Effect.tryPromise(() => writeProjectConfig(tempDir)); + const { out, layer, proxy } = setup(tempDir, { + format: "json", + list: [makeFunction({ slug: "hello-world" }), makeFunction({ slug: "goodbye-world" })], + rawArgs: ["functions", "download", "--use-docker"], + }); + + yield* functionsDownload({ + ...BASE_FLAGS, + functionName: Option.none(), + useDocker: true, + }).pipe(Effect.provide(layer)); + + expect(proxy.calls).toEqual([]); + expect(proxy.captureCalls).toEqual([ + ["functions", "download", "--project-ref", PROJECT_REF, "--use-docker"], + ]); + expect(out.messages).toContainEqual( + expect.objectContaining({ + type: "success", + message: "Downloaded Edge Function source.", + data: { + function_slugs: ["hello-world", "goodbye-world"], + project_ref: PROJECT_REF, + }, + }), + ); + }).pipe( + Effect.ensuring(Effect.tryPromise(() => rm(tempDir, { recursive: true, force: true }))), + ); + }, + ); + + it.live("fails before delegating when the pre-flight function list fails in machine mode", () => { + const tempDir = makeTempDir(); + + return Effect.gen(function* () { + yield* Effect.tryPromise(() => writeProjectConfig(tempDir)); + const { layer, proxy } = setup(tempDir, { + format: "json", + listStatus: 503, + listBody: { message: "unavailable" }, + rawArgs: ["functions", "download", "--use-docker"], + }); + + // The pre-flight list failure must be reported before any download + // side effect — the delegated proxy must never be invoked (CLI-1862 + // review: a listing failure after a successful delegated download + // must not mask that success). + const error = yield* functionsDownload({ + ...BASE_FLAGS, + functionName: Option.none(), + useDocker: true, + }).pipe(Effect.provide(layer), Effect.flip); + + expect(error).toBeInstanceOf(Error); + expect(proxy.calls).toEqual([]); + expect(proxy.captureCalls).toEqual([]); + }).pipe( + Effect.ensuring(Effect.tryPromise(() => rm(tempDir, { recursive: true, force: true }))), + ); + }); + it.live("rejects mutually exclusive compatibility flags", () => { const tempDir = makeTempDir(); diff --git a/apps/cli/src/shared/functions/download.ts b/apps/cli/src/shared/functions/download.ts index 5ea14c3954..328dc46157 100644 --- a/apps/cli/src/shared/functions/download.ts +++ b/apps/cli/src/shared/functions/download.ts @@ -766,20 +766,28 @@ export function downloadFunctions Date: Mon, 6 Jul 2026 14:30:11 +0100 Subject: [PATCH 4/8] fix(cli): short-circuit empty projects before proxying in machine mode MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex re-review of 5df8d953 found a real bug: in the default Docker/proxy path with --output-format json|stream-json and no function name, an empty project would still delegate to the Go proxy (an unnecessary Docker/child round-trip) and then report success as "Downloaded Edge Function source." with an empty slug list — inconsistent with the native path just below, which correctly short-circuits empty projects as "No functions found." without delegating anywhere. Added the same short-circuit to the proxy branch, right after resolving the slug list and before calling proxyDownload, in both the legacy and next handlers. --- .../download/download.integration.test.ts | 54 +++++++++++++++++++ .../download/download.integration.test.ts | 38 +++++++++++++ apps/cli/src/shared/functions/download.ts | 14 +++++ 3 files changed, 106 insertions(+) diff --git a/apps/cli/src/legacy/commands/functions/download/download.integration.test.ts b/apps/cli/src/legacy/commands/functions/download/download.integration.test.ts index b25bb51f99..74948f7583 100644 --- a/apps/cli/src/legacy/commands/functions/download/download.integration.test.ts +++ b/apps/cli/src/legacy/commands/functions/download/download.integration.test.ts @@ -379,6 +379,60 @@ describe("legacy functions download", () => { }, ); + it.live( + "reports no functions found without delegating when the project is empty in machine mode", + () => { + const out = mockOutput({ format: "json" }); + const api = mockLegacyPlatformApi({ + handler: (request) => + request.url.endsWith("/functions") + ? Effect.succeed(legacyJsonResponse(request, 200, [])) + : Effect.succeed(legacyJsonResponse(request, 200, {})), + }); + const proxy = mockProxy(); + const layer = Layer.mergeAll( + buildLegacyTestRuntime({ + out, + api, + cliConfig: mockLegacyCliConfig({ workdir: tempRoot.current }), + }), + proxy.layer, + Stdio.layerTest({ + args: Effect.succeed([ + "functions", + "download", + "--project-ref", + "abcdefghijklmnopqrst", + "--output-format", + "json", + ]), + }), + ); + + return Effect.gen(function* () { + // An empty project has nothing to delegate — this must match the + // native path's "No functions found." short-circuit instead of + // still invoking the Go/Docker child and reporting a misleading + // "Downloaded Edge Function source." success with an empty list. + yield* legacyFunctionsDownload({ + ...baseFlags, + functionName: Option.none(), + useDocker: true, + }); + + expect(proxy.calls).toEqual([]); + expect(proxy.captureCalls).toEqual([]); + expect(out.messages).toContainEqual( + expect.objectContaining({ + type: "success", + message: "No functions found.", + data: { function_slugs: [], project_ref: "abcdefghijklmnopqrst" }, + }), + ); + }).pipe(Effect.provide(layer)); + }, + ); + it.live("fails before delegating when the pre-flight function list fails in machine mode", () => { const out = mockOutput({ format: "json" }); const api = mockLegacyPlatformApi({ diff --git a/apps/cli/src/next/commands/functions/download/download.integration.test.ts b/apps/cli/src/next/commands/functions/download/download.integration.test.ts index 7979fbc8b1..4aee254ab6 100644 --- a/apps/cli/src/next/commands/functions/download/download.integration.test.ts +++ b/apps/cli/src/next/commands/functions/download/download.integration.test.ts @@ -947,6 +947,44 @@ describe("functions download", () => { }, ); + it.live( + "reports no functions found without delegating when the project is empty in machine mode", + () => { + const tempDir = makeTempDir(); + + return Effect.gen(function* () { + yield* Effect.tryPromise(() => writeProjectConfig(tempDir)); + const { out, layer, proxy } = setup(tempDir, { + format: "json", + list: [], + rawArgs: ["functions", "download", "--use-docker"], + }); + + // An empty project has nothing to delegate — this must match the + // native path's "No functions found." short-circuit instead of + // still invoking the Go/Docker child and reporting a misleading + // "Downloaded Edge Function source." success with an empty list. + yield* functionsDownload({ + ...BASE_FLAGS, + functionName: Option.none(), + useDocker: true, + }).pipe(Effect.provide(layer)); + + expect(proxy.calls).toEqual([]); + expect(proxy.captureCalls).toEqual([]); + expect(out.messages).toContainEqual( + expect.objectContaining({ + type: "success", + message: "No functions found.", + data: { function_slugs: [], project_ref: PROJECT_REF }, + }), + ); + }).pipe( + Effect.ensuring(Effect.tryPromise(() => rm(tempDir, { recursive: true, force: true }))), + ); + }, + ); + it.live("fails before delegating when the pre-flight function list fails in machine mode", () => { const tempDir = makeTempDir(); diff --git a/apps/cli/src/shared/functions/download.ts b/apps/cli/src/shared/functions/download.ts index 328dc46157..1d6ea222a5 100644 --- a/apps/cli/src/shared/functions/download.ts +++ b/apps/cli/src/shared/functions/download.ts @@ -786,6 +786,20 @@ export function downloadFunctions Date: Tue, 7 Jul 2026 12:07:00 +0100 Subject: [PATCH 5/8] fix(cli): reject path-traversal and symlink-escape in functions download extractor (CLI-1891) The Go CLI's server-side-unbundle extractor joined server-controlled Supabase-Path/Content-Disposition metadata straight into a filesystem path and wrote to it with no validation, letting a malicious response traverse outside supabase/functions or write through a pre-existing symlink at the destination. Validate the resolved destination against utils.FunctionsDir before touching disk, reject paths that resolve through an existing symlink, and write via a temp file + atomic rename so an existing symlink at the destination is replaced rather than followed. --- .../internal/functions/download/download.go | 126 +++++++++++++++++- .../functions/download/download_test.go | 100 ++++++++++++++ 2 files changed, 225 insertions(+), 1 deletion(-) diff --git a/apps/cli-go/internal/functions/download/download.go b/apps/cli-go/internal/functions/download/download.go index 74d0c13e4d..f3e56963a5 100644 --- a/apps/cli-go/internal/functions/download/download.go +++ b/apps/cli-go/internal/functions/download/download.go @@ -23,6 +23,7 @@ import ( "github.com/docker/docker/api/types/network" "github.com/docker/go-units" "github.com/go-errors/errors" + "github.com/google/uuid" "github.com/spf13/afero" "github.com/spf13/viper" "github.com/supabase/cli/internal/utils" @@ -35,6 +36,15 @@ var ( legacyImportMapPath = "file:///src/import_map.json" ) +// ErrUnsafeDownloadPath is returned when a downloaded Function file's path, +// as reported by the server via the Supabase-Path header or the +// Content-Disposition filename, would resolve outside utils.FunctionsDir +// once joined and cleaned, or would require following an existing symlink +// to get there. The server response is not trusted input, so any path that +// looks like it's trying to escape the functions directory is rejected +// rather than sanitized. +var ErrUnsafeDownloadPath = errors.New("invalid path in server response") + func RunLegacy(ctx context.Context, slug string, projectRef string, fsys afero.Fs) error { // 1. Sanity checks. { @@ -406,12 +416,126 @@ func saveFile(file *multipart.FileHeader, entrypointPath, funcDir string, fsys a relPath = filepath.FromSlash(path.Join("..", partPath)) } + // partPath (and therefore relPath and dstPath) is derived from + // server-controlled multipart metadata (Supabase-Path header or + // Content-Disposition filename), so it must be validated before it + // touches the filesystem below. dstPath := filepath.Join(funcDir, path.Base(entrypointPath), relPath) + + // Containment is enforced against the shared functions directory + // rather than funcDir: the entrypoint-mismatch fallback above can + // legitimately resolve one level above funcDir (into a sibling + // function's directory), but must never resolve outside + // utils.FunctionsDir entirely, e.g. via a "../../../etc/passwd" style + // payload. + root := utils.FunctionsDir + if err := validateDownloadPath(root, dstPath); err != nil { + return err + } + + // A previous part could have planted a symlink at an intermediate + // directory component, which would otherwise let MkdirAll/OpenFile + // silently follow it out of root even though dstPath itself looks + // clean. Check before creating the directory, and again after the + // write lands in case a symlink was swapped in between the two checks. + dstDir := filepath.Dir(dstPath) + if err := ensureNoSymlinkInPath(fsys, root, dstDir); err != nil { + return err + } + if err := utils.MkdirIfNotExistFS(fsys, dstDir); err != nil { + return err + } + fmt.Fprintln(os.Stderr, "Extracting file:", dstPath) - if err := afero.WriteReader(fsys, dstPath, part); err != nil { + if err := writeFileNoFollowSymlink(fsys, dstPath, part); err != nil { + return err + } + + return ensureNoSymlinkInPath(fsys, root, dstPath) +} + +// validateDownloadPath rejects dstPath if, once lexically cleaned, it does +// not resolve inside root. This is the primary defense against a hostile +// "../../etc/passwd" style Supabase-Path/filename escaping the functions +// directory: filepath.Join already cleans ".." segments away, so this +// check must run against the cleaned result rather than by scanning the +// raw, attacker-controlled path for "..". +func validateDownloadPath(root, dstPath string) error { + rel, err := filepath.Rel(root, filepath.Clean(dstPath)) + if err != nil || rel == ".." || strings.HasPrefix(rel, ".."+string(filepath.Separator)) || filepath.IsAbs(rel) { + return errors.Errorf("failed to save file: %w", ErrUnsafeDownloadPath) + } + return nil +} + +// ensureNoSymlinkInPath rejects dir if it, or any existing ancestor of it +// down to (but excluding) root, is a symlink. +// +// This only has an effect on filesystems that expose real symlink +// semantics through afero.Lstater, i.e. afero.NewOsFs in production. The +// in-memory filesystem used in tests has no notion of symlinks, so +// LstatIfPossible never reports the symlink bit there and this check is +// effectively a no-op -- there is nothing to protect against on a +// filesystem that cannot contain symlinks in the first place. +func ensureNoSymlinkInPath(fsys afero.Fs, root, dir string) error { + lstater, ok := fsys.(afero.Lstater) + if !ok { + return nil + } + + rel, err := filepath.Rel(root, dir) + if err != nil || rel == "." || rel == "" { + return nil + } + + current := root + for _, segment := range strings.Split(rel, string(filepath.Separator)) { + current = filepath.Join(current, segment) + info, _, err := lstater.LstatIfPossible(current) + if err != nil { + if os.IsNotExist(err) { + // Nothing here yet, and therefore nothing further down + // this branch either: MkdirAll will create plain + // directories from this point on. + break + } + return errors.Errorf("failed to inspect extraction path: %w", err) + } + if info.Mode()&os.ModeSymlink != 0 { + return errors.Errorf("failed to save file: %w", ErrUnsafeDownloadPath) + } + } + return nil +} + +// writeFileNoFollowSymlink writes r to dstPath without following a symlink +// that might already occupy that path. It creates a randomly named +// temporary file next to dstPath with O_EXCL, refusing to write through +// anything already there, then atomically renames it onto dstPath. +// Rename replaces whatever directory entry currently exists at dstPath -- +// including a symlink -- rather than dereferencing it, which is exactly +// what a plain fs.Create/afero.WriteReader would otherwise do. +func writeFileNoFollowSymlink(fsys afero.Fs, dstPath string, r io.Reader) error { + tmpPath := filepath.Join(filepath.Dir(dstPath), fmt.Sprintf(".supabase-download-%s.tmp", uuid.NewString())) + tmp, err := fsys.OpenFile(tmpPath, os.O_WRONLY|os.O_CREATE|os.O_EXCL, 0o644) + if err != nil { + return errors.Errorf("failed to save file: %w", err) + } + + if _, err := io.Copy(tmp, r); err != nil { + _ = tmp.Close() + _ = fsys.Remove(tmpPath) + return errors.Errorf("failed to save file: %w", err) + } + if err := tmp.Close(); err != nil { + _ = fsys.Remove(tmpPath) return errors.Errorf("failed to save file: %w", err) } + if err := fsys.Rename(tmpPath, dstPath); err != nil { + _ = fsys.Remove(tmpPath) + return errors.Errorf("failed to save file: %w", err) + } return nil } diff --git a/apps/cli-go/internal/functions/download/download_test.go b/apps/cli-go/internal/functions/download/download_test.go index 6ec601d928..d1efeff3b1 100644 --- a/apps/cli-go/internal/functions/download/download_test.go +++ b/apps/cli-go/internal/functions/download/download_test.go @@ -453,6 +453,106 @@ func TestRunServerSideUnbundle(t *testing.T) { assert.Empty(t, apitest.ListUnmatchedRequests()) }) + + t.Run("rejects a path traversal payload in Supabase-Path", func(t *testing.T) { + fsys := afero.NewMemMapFs() + require.NoError(t, utils.WriteConfig(fsys, false)) + + defer gock.OffAll() + mockMultipartBody(t, project, slug, bundleMetadata{EntrypointPath: "source/index.ts"}, []multipartPart{ + {filename: "source/index.ts", contents: "console.log('hello')"}, + {filename: "leftover.ts", supabasePath: "../../../../../../etc/passwd", contents: "root:x:0:0::/root:/bin/bash"}, + }) + + err := Run(context.Background(), slug, project, false, false, fsys) + require.Error(t, err) + assert.ErrorIs(t, err, ErrUnsafeDownloadPath) + + // Nothing should have escaped the functions directory. + exists, err := afero.Exists(fsys, "/etc/passwd") + require.NoError(t, err) + assert.False(t, exists, "path traversal payload must not be written outside the functions directory") + + assert.Empty(t, apitest.ListUnmatchedRequests()) + }) + + t.Run("writes deeply nested legitimate paths", func(t *testing.T) { + fsys := afero.NewMemMapFs() + require.NoError(t, utils.WriteConfig(fsys, false)) + + defer gock.OffAll() + mockMultipartBody(t, project, slug, bundleMetadata{EntrypointPath: "source/index.ts"}, []multipartPart{ + {filename: "source/index.ts", contents: "console.log('hello')"}, + {filename: "source/a/b/c/d/deep.ts", contents: "export const deep = true;"}, + }) + + err := Run(context.Background(), slug, project, false, false, fsys) + require.NoError(t, err) + + root := filepath.Join(utils.FunctionsDir, slug) + data, err := afero.ReadFile(fsys, filepath.Join(root, "index.ts")) + require.NoError(t, err) + assert.Equal(t, "console.log('hello')", string(data)) + + data, err = afero.ReadFile(fsys, filepath.Join(root, "a", "b", "c", "d", "deep.ts")) + require.NoError(t, err) + assert.Equal(t, "export const deep = true;", string(data)) + + assert.Empty(t, apitest.ListUnmatchedRequests()) + }) + + t.Run("does not write through a pre-existing symlink at the destination", func(t *testing.T) { + // Symlink semantics only exist on a real filesystem, so this test + // exercises afero.NewOsFs against an isolated temp directory rather + // than the in-memory fs used elsewhere in this file. + tmpDir := t.TempDir() + t.Chdir(tmpDir) + fsys := afero.NewOsFs() + require.NoError(t, utils.WriteConfig(fsys, false)) + + // A file living outside the sandboxed functions directory that must + // never be touched by the download. + outsideDir := filepath.Join(tmpDir, "outside") + require.NoError(t, os.MkdirAll(outsideDir, 0o755)) + secretPath := filepath.Join(outsideDir, "secret.txt") + require.NoError(t, os.WriteFile(secretPath, []byte("original"), 0o600)) + + // Plant a symlink inside the function directory, before the + // download runs, pointing at the file outside the sandbox. A + // server response that resolves to this exact path must not be + // allowed to write through it. + funcDir := filepath.Join(utils.FunctionsDir, slug) + require.NoError(t, os.MkdirAll(funcDir, 0o755)) + linkPath := filepath.Join(funcDir, "evil.ts") + require.NoError(t, os.Symlink(secretPath, linkPath)) + + defer gock.OffAll() + mockMultipartBody(t, project, slug, bundleMetadata{EntrypointPath: "source/index.ts"}, []multipartPart{ + {filename: "source/index.ts", contents: "console.log('hello')"}, + {filename: "source/evil.ts", contents: "overwritten"}, + }) + + err := Run(context.Background(), slug, project, false, false, fsys) + require.NoError(t, err) + + // The symlink target outside the sandbox must be untouched. + data, err := os.ReadFile(secretPath) + require.NoError(t, err) + assert.Equal(t, "original", string(data), "file outside the functions directory must not be modified") + + // The destination itself should now be a plain file with the + // downloaded contents: the atomic rename replaces the symlink + // directory entry rather than following it. + info, err := os.Lstat(linkPath) + require.NoError(t, err) + assert.Zero(t, info.Mode()&os.ModeSymlink, "planted symlink should have been replaced, not written through") + + data, err = os.ReadFile(linkPath) + require.NoError(t, err) + assert.Equal(t, "overwritten", string(data)) + + assert.Empty(t, apitest.ListUnmatchedRequests()) + }) } func TestDownloadFunction(t *testing.T) { From 4b73ceb64388c6e1dcc2e8fefd49e97ece069390 Mon Sep 17 00:00:00 2001 From: Colum Ferry Date: Tue, 7 Jul 2026 12:35:03 +0100 Subject: [PATCH 6/8] fix(cli-go): validate function slug before use in download dispatch (CLI-1891) downloadAll's per-function loop handed f.Slug -- sourced from V1ListAllFunctionsWithResponse, the same Management API channel already treated as untrusted in this threat model -- straight to downloadOne (via downloadWithDockerUnbundle) and downloadWithServerSideUnbundle, both of which join it into a filesystem path. downloadOne had no validation at all: a slug like "../../../../../poc-escaped-outside-project" resolves, once joined with utils.TempDir and cleaned, to a path outside the project root, and afero.WriteReader would MkdirAll and write the attacker-controlled response body there. downloadWithServerSideUnbundle was only incidentally safe, via saveFile's unrelated validateDownloadPath check on the final joined path. Validate the slug once, at the single point where it enters this dispatch logic, before it reaches any downloader. This closes the downloadOne gap directly and turns downloadWithServerSideUnbundle's protection from an implicit side effect into an explicit guard. --- .../internal/functions/download/download.go | 16 ++++ .../functions/download/download_test.go | 87 +++++++++++++++++++ 2 files changed, 103 insertions(+) diff --git a/apps/cli-go/internal/functions/download/download.go b/apps/cli-go/internal/functions/download/download.go index f3e56963a5..4024f76595 100644 --- a/apps/cli-go/internal/functions/download/download.go +++ b/apps/cli-go/internal/functions/download/download.go @@ -170,6 +170,22 @@ func downloadAll(ctx context.Context, projectRef string, fsys afero.Fs, download fmt.Fprintf(os.Stderr, "Found %d function(s) to download\n", len(functions)) for _, f := range functions { + // f.Slug comes straight from the Management API response, which + // this threat model treats as untrusted: a malicious or corrupted + // response (or a MITM) could return a slug containing ".." or "/" + // segments. Every downloader below joins this value into a + // filesystem path (utils.TempDir for downloadOne, utils.FunctionsDir + // for downloadWithServerSideUnbundle) before any validation of its + // own, so it must be rejected here -- the single point where it + // enters this dispatch logic -- rather than relying on each + // downstream path-construction site to defend itself. + if err := utils.ValidateFunctionSlug(f.Slug); err != nil { + utils.CmdSuggestion = fmt.Sprintf( + "The Supabase API returned an unexpected function slug (%s). Retry the command, and if this keeps happening, verify your network connection is not being intercepted before contacting Supabase support.", + utils.Aqua(f.Slug), + ) + return errors.Errorf("failed to download function %s: %w", f.Slug, err) + } if err := downloader(ctx, f.Slug, projectRef, fsys); err != nil { return err } diff --git a/apps/cli-go/internal/functions/download/download_test.go b/apps/cli-go/internal/functions/download/download_test.go index d1efeff3b1..e9e04e1cbc 100644 --- a/apps/cli-go/internal/functions/download/download_test.go +++ b/apps/cli-go/internal/functions/download/download_test.go @@ -308,6 +308,93 @@ func TestRunDockerUnbundle(t *testing.T) { }) } +// TestDownloadAllRejectsMaliciousSlug is a regression test for CLI-1891: the +// per-function loop in downloadAll must reject a path-traversal payload in +// a function's Slug -- as returned by V1ListAllFunctionsWithResponse, which +// this threat model treats as untrusted (a malicious/compromised Management +// API response, or a MITM) -- before handing it to any downloader that +// joins it into a filesystem path. +// +// This mirrors an exploit independently confirmed against downloadOne: with +// slug = "../../../../../poc-escaped-outside-project", downloadOne's +// eszipPath := filepath.Join(utils.TempDir, fmt.Sprintf("output_%s.eszip", +// slug)) resolves (after filepath.Clean) to "../poc-escaped-outside-project.eszip", +// i.e. one directory level above the project root, and +// afero.WriteReader happily MkdirAll's its way there and writes the +// server-controlled response body outside the sandbox. +func TestDownloadAllRejectsMaliciousSlug(t *testing.T) { + const maliciousSlug = "../../../../../poc-escaped-outside-project" + + // Use a real OS filesystem rooted at an isolated temp directory so an + // escape can actually be observed landing outside the project root, + // exactly as in the reviewer's PoC. + tmpDir := t.TempDir() + t.Chdir(tmpDir) + fsys := afero.NewOsFs() + require.NoError(t, utils.WriteConfig(fsys, false)) + + project := apitest.RandomProjectRef() + token := apitest.RandomAccessToken(t) + t.Setenv("SUPABASE_ACCESS_TOKEN", string(token)) + + utils.CmdSuggestion = "" + t.Cleanup(func() { utils.CmdSuggestion = "" }) + + defer gock.OffAll() + gock.New(utils.DefaultApiHost). + Get("/v1/projects/" + project + "/functions"). + Reply(http.StatusOK). + JSON([]api.FunctionResponse{{ + Id: "poc-id", + Name: "poc", + Slug: maliciousSlug, + }}) + // Mocked so that, if the fix is removed, the malicious slug's request + // still succeeds and downloadOne proceeds all the way to writing the + // escaped file -- proving the escape, rather than masking it behind an + // unrelated network error. With the fix in place this mock is never hit. + gock.New(utils.DefaultApiHost). + Get("/v1/projects/" + project + "/functions/.*/body"). + Reply(http.StatusOK). + BodyString("fake eszip payload") + + // downloadOne is the exact sink the reviewer's PoC targeted directly; + // wrap it as a downloader so downloadAll's dispatch is exercised against + // the real vulnerable code, without also pulling in downloadWithDockerUnbundle's + // unrelated Docker extraction step (and its defer-cleanup of the eszip + // file, which would otherwise remove the escaped file before this test + // can observe it). + downloaderCalled := false + downloader := func(ctx context.Context, slug, projectRef string, fsys afero.Fs) error { + downloaderCalled = true + _, err := downloadOne(ctx, slug, projectRef, fsys) + return err + } + + err := downloadAll(context.Background(), project, fsys, downloader) + + // Check for the escape before any assertion below that could halt the + // test on failure (e.g. require.Error), so this is verified regardless + // of whether downloadAll happened to return an error for some other + // reason. t.TempDir()'s own cleanup RemoveAll's tmpDir's parent, which + // would otherwise sweep away the evidence once the test returns. + // + // The exploit resolves to "../poc-escaped-outside-project.eszip" + // relative to utils.TempDir, i.e. one level above the project root. + escapedPath := filepath.Join(tmpDir, "..", "poc-escaped-outside-project.eszip") + t.Cleanup(func() { _ = os.Remove(escapedPath) }) + exists, existsErr := afero.Exists(fsys, escapedPath) + require.NoError(t, existsErr) + assert.False(t, exists, "malicious slug must not be able to write outside the project directory") + + require.Error(t, err) + assert.ErrorIs(t, err, utils.ErrInvalidSlug) + assert.Contains(t, utils.CmdSuggestion, "unexpected function slug") + assert.False(t, downloaderCalled, "downloader must not be invoked with an unvalidated slug") + + assert.Empty(t, apitest.ListUnmatchedRequests()) +} + func TestRunServerSideUnbundle(t *testing.T) { const slug = "test-func" token := apitest.RandomAccessToken(t) From df5ab92a74dde83bcc8d1dd061602807d69e6986 Mon Sep 17 00:00:00 2001 From: Colum Ferry Date: Tue, 7 Jul 2026 13:42:23 +0100 Subject: [PATCH 7/8] fix(cli-go): address review findings on CLI-1891 (error messaging, symlink resolution policy) DX review: the ErrUnsafeDownloadPath error path in saveFile fell through to the generic --debug suggestion instead of an actionable one, unlike the other CmdSuggestion sites in this file. Add suggestUnsafeDownloadPath() and set it at each of saveFile's three ErrUnsafeDownloadPath return points. DX review: ensureNoSymlinkInPath rejected any symlink under utils.FunctionsDir unconditionally, which would break legitimate patterns like a monorepo symlinking a shared directory into the functions tree. Replace the reject-any-symlink walk with resolve-then-check: resolve both root and dir to their deepest existing ancestor via filepath.EvalSymlinks and re-validate the two resolved paths against each other, rejecting only when the resolved dir actually escapes the resolved root. Resolving root the same way as dir (rather than against a literal root) is required so an OS-level symlink above both of them doesn't produce a false mismatch, and also happens to naturally cover root itself being a symlink. Engineer review: gate the WARNING log on a failed entrypoint-relative Rel call behind --debug, since the raw error embeds the server-controlled partPath verbatim. Also gate the afero filesystem check on afero.LinkReader rather than afero.Lstater, since afero.MemMapFs implements the latter by delegating to Stat, which made the previous implementation's "no-op on in-memory fs" guarantee accidental rather than structural. --- .../internal/functions/download/download.go | 129 ++++++++++++++---- .../functions/download/download_test.go | 110 +++++++++++++++ 2 files changed, 209 insertions(+), 30 deletions(-) diff --git a/apps/cli-go/internal/functions/download/download.go b/apps/cli-go/internal/functions/download/download.go index 4024f76595..3e3737ba1e 100644 --- a/apps/cli-go/internal/functions/download/download.go +++ b/apps/cli-go/internal/functions/download/download.go @@ -315,6 +315,10 @@ func suggestLegacyBundle(slug string) string { return fmt.Sprintf("\nIf your function is deployed using CLI < 1.120.0, trying running %s instead.", utils.Aqua("supabase functions download --legacy-bundle "+slug)) } +func suggestUnsafeDownloadPath() string { + return "This usually indicates a malformed or unexpected API response. If you're using a self-hosted instance, verify your API URL is correct." +} + type bundleMetadata struct { EntrypointPath string `json:"deno2_entrypoint_path,omitempty"` } @@ -427,8 +431,12 @@ func saveFile(file *multipart.FileHeader, entrypointPath, funcDir string, fsys a relPath, err := filepath.Rel(filepath.FromSlash(entrypointPath), filepath.FromSlash(partPath)) if err != nil { - // Continue extracting without entrypoint - fmt.Fprintln(os.Stderr, utils.Yellow("WARNING:"), err) + // Continue extracting without entrypoint. Go's Rel error embeds + // both paths verbatim ("Rel: can't make relative to + // "), and partPath is server-controlled, so this is gated + // behind --debug like the "Resolving file path" log above rather + // than printed unconditionally to stderr. + fmt.Fprintln(logger, "WARNING:", err) relPath = filepath.FromSlash(path.Join("..", partPath)) } @@ -446,6 +454,7 @@ func saveFile(file *multipart.FileHeader, entrypointPath, funcDir string, fsys a // payload. root := utils.FunctionsDir if err := validateDownloadPath(root, dstPath); err != nil { + utils.CmdSuggestion = suggestUnsafeDownloadPath() return err } @@ -456,6 +465,7 @@ func saveFile(file *multipart.FileHeader, entrypointPath, funcDir string, fsys a // write lands in case a symlink was swapped in between the two checks. dstDir := filepath.Dir(dstPath) if err := ensureNoSymlinkInPath(fsys, root, dstDir); err != nil { + utils.CmdSuggestion = suggestUnsafeDownloadPath() return err } if err := utils.MkdirIfNotExistFS(fsys, dstDir); err != nil { @@ -467,7 +477,11 @@ func saveFile(file *multipart.FileHeader, entrypointPath, funcDir string, fsys a return err } - return ensureNoSymlinkInPath(fsys, root, dstPath) + if err := ensureNoSymlinkInPath(fsys, root, dstPath); err != nil { + utils.CmdSuggestion = suggestUnsafeDownloadPath() + return err + } + return nil } // validateDownloadPath rejects dstPath if, once lexically cleaned, it does @@ -484,44 +498,99 @@ func validateDownloadPath(root, dstPath string) error { return nil } -// ensureNoSymlinkInPath rejects dir if it, or any existing ancestor of it -// down to (but excluding) root, is a symlink. +// ensureNoSymlinkInPath rejects dir if resolving the symlinks along it would +// land outside root. +// +// Earlier versions of this check rejected the mere presence of a symlink +// anywhere under root, which also broke legitimate setups such as a +// monorepo symlinking a shared directory into place inside the functions +// tree. Instead, this resolves both root and dir the same way -- walking up +// to the deepest ancestor that already exists on disk, following any chain +// of symlinks there via filepath.EvalSymlinks (this also naturally covers +// root itself being a symlink, since dir's walk passes through root), and +// re-joining the still-nonexistent remainder back on -- and re-validates +// the two resolved paths against each other with validateDownloadPath. +// Resolving root the same way dir is resolved, rather than comparing a +// resolved dir against a literal root, matters in practice: it is what +// keeps an OS-level symlink that sits above both of them (e.g. macOS's +// /var -> /private/var, which every path under a t.TempDir() passes +// through) from producing a spurious mismatch. Only a resolved dir that +// escapes the resolved root is rejected; a symlink whose target still +// lands inside root, however deeply nested, is now allowed. // // This only has an effect on filesystems that expose real symlink -// semantics through afero.Lstater, i.e. afero.NewOsFs in production. The -// in-memory filesystem used in tests has no notion of symlinks, so -// LstatIfPossible never reports the symlink bit there and this check is -// effectively a no-op -- there is nothing to protect against on a -// filesystem that cannot contain symlinks in the first place. +// semantics through afero.LinkReader, i.e. afero.NewOsFs in production +// (afero.Lstater is not a reliable enough signal on its own: afero.MemMapFs +// also implements it, just by delegating straight to Stat). The in-memory +// filesystem used in tests implements neither, so this check is +// effectively a no-op there -- there is nothing to protect against on a +// filesystem that cannot contain symlinks in the first place. That same +// guard is also why it is safe for filepath.EvalSymlinks below to hit the +// real OS filesystem directly instead of going through fsys: the only +// afero.Fs implementation used in production, afero.NewOsFs, delegates to +// the real OS filesystem for every operation, so the two agree. func ensureNoSymlinkInPath(fsys afero.Fs, root, dir string) error { - lstater, ok := fsys.(afero.Lstater) - if !ok { + if _, ok := fsys.(afero.LinkReader); !ok { return nil } - rel, err := filepath.Rel(root, dir) - if err != nil || rel == "." || rel == "" { - return nil + // filepath.EvalSymlinks returns an absolute result as soon as it + // crosses one absolute symlink, but stays relative otherwise (per its + // doc comment), so root and dir must both start out absolute here -- + // otherwise root (no symlink crossed) and dir (crossing one) could + // resolve to a relative and an absolute path respectively, which + // filepath.Rel below cannot meaningfully compare. + absRoot, err := filepath.Abs(root) + if err != nil { + return errors.Errorf("failed to inspect extraction path: %w", err) + } + absDir, err := filepath.Abs(dir) + if err != nil { + return errors.Errorf("failed to inspect extraction path: %w", err) } - current := root - for _, segment := range strings.Split(rel, string(filepath.Separator)) { - current = filepath.Join(current, segment) - info, _, err := lstater.LstatIfPossible(current) - if err != nil { - if os.IsNotExist(err) { - // Nothing here yet, and therefore nothing further down - // this branch either: MkdirAll will create plain - // directories from this point on. - break - } - return errors.Errorf("failed to inspect extraction path: %w", err) + resolvedRoot, err := resolveExistingPath(fsys, absRoot) + if err != nil { + return errors.Errorf("failed to inspect extraction path: %w", err) + } + resolvedDir, err := resolveExistingPath(fsys, absDir) + if err != nil { + return errors.Errorf("failed to inspect extraction path: %w", err) + } + + return validateDownloadPath(resolvedRoot, resolvedDir) +} + +// resolveExistingPath resolves p by walking up to the deepest ancestor of it +// that already exists -- a path component that does not exist yet cannot +// itself be a symlink, so there is nothing there for EvalSymlinks to +// resolve -- following any chain of symlinks in that ancestor to its real +// target, then re-joining the still-nonexistent remainder of p back onto +// the result. +func resolveExistingPath(fsys afero.Fs, p string) (string, error) { + existing := p + var suffix []string + for { + if _, err := fsys.Stat(existing); err == nil { + break + } else if !os.IsNotExist(err) { + return "", err } - if info.Mode()&os.ModeSymlink != 0 { - return errors.Errorf("failed to save file: %w", ErrUnsafeDownloadPath) + parent := filepath.Dir(existing) + if parent == existing { + // Reached the filesystem root without finding anything that + // exists yet, so there is nothing left to resolve. + return filepath.Join(append([]string{existing}, suffix...)...), nil } + suffix = append([]string{filepath.Base(existing)}, suffix...) + existing = parent } - return nil + + resolved, err := filepath.EvalSymlinks(existing) + if err != nil { + return "", err + } + return filepath.Join(append([]string{resolved}, suffix...)...), nil } // writeFileNoFollowSymlink writes r to dstPath without following a symlink diff --git a/apps/cli-go/internal/functions/download/download_test.go b/apps/cli-go/internal/functions/download/download_test.go index e9e04e1cbc..355b0ec39c 100644 --- a/apps/cli-go/internal/functions/download/download_test.go +++ b/apps/cli-go/internal/functions/download/download_test.go @@ -545,6 +545,9 @@ func TestRunServerSideUnbundle(t *testing.T) { fsys := afero.NewMemMapFs() require.NoError(t, utils.WriteConfig(fsys, false)) + utils.CmdSuggestion = "" + t.Cleanup(func() { utils.CmdSuggestion = "" }) + defer gock.OffAll() mockMultipartBody(t, project, slug, bundleMetadata{EntrypointPath: "source/index.ts"}, []multipartPart{ {filename: "source/index.ts", contents: "console.log('hello')"}, @@ -555,6 +558,12 @@ func TestRunServerSideUnbundle(t *testing.T) { require.Error(t, err) assert.ErrorIs(t, err, ErrUnsafeDownloadPath) + // The generic "invalid path in server response" message on its own + // gives users nothing actionable, so this must carry a specific + // suggestion (DX finding on CLI-1891) rather than falling through + // to the generic --debug suggestion. + assert.Contains(t, utils.CmdSuggestion, "malformed or unexpected API response") + // Nothing should have escaped the functions directory. exists, err := afero.Exists(fsys, "/etc/passwd") require.NoError(t, err) @@ -640,6 +649,107 @@ func TestRunServerSideUnbundle(t *testing.T) { assert.Empty(t, apitest.ListUnmatchedRequests()) }) + + // Regression test for the DX finding on CLI-1891: ensureNoSymlinkInPath + // used to reject the mere presence of a symlink anywhere under + // utils.FunctionsDir, which would also have broken a legitimate + // monorepo pattern like this one, where a function directory + // symlinks in a shared directory that itself lives inside the + // functions tree. That symlink's target still resolves inside root, + // so it must be allowed end-to-end. + t.Run("writes through a symlink pointing to a legitimate location inside the functions directory", func(t *testing.T) { + tmpDir := t.TempDir() + t.Chdir(tmpDir) + fsys := afero.NewOsFs() + require.NoError(t, utils.WriteConfig(fsys, false)) + + sharedDir := filepath.Join(utils.FunctionsDir, "_shared") + require.NoError(t, os.MkdirAll(sharedDir, 0o755)) + // os.Symlink resolves a relative target against the symlink's own + // containing directory, not the process cwd, so the target must be + // made absolute here for the link to actually point at sharedDir. + absSharedDir, err := filepath.Abs(sharedDir) + require.NoError(t, err) + + funcDir := filepath.Join(utils.FunctionsDir, slug) + require.NoError(t, os.MkdirAll(funcDir, 0o755)) + require.NoError(t, os.Symlink(absSharedDir, filepath.Join(funcDir, "_shared"))) + + defer gock.OffAll() + mockMultipartBody(t, project, slug, bundleMetadata{EntrypointPath: "source/index.ts"}, []multipartPart{ + {filename: "source/index.ts", contents: "console.log('hello')"}, + {filename: "source/_shared/util.ts", contents: "export const util = 2;"}, + }) + + err = Run(context.Background(), slug, project, false, false, fsys) + require.NoError(t, err) + + data, err := afero.ReadFile(fsys, filepath.Join(funcDir, "index.ts")) + require.NoError(t, err) + assert.Equal(t, "console.log('hello')", string(data)) + + // The write landed through the symlink, in the real shared + // directory rather than being rejected. + data, err = os.ReadFile(filepath.Join(sharedDir, "util.ts")) + require.NoError(t, err) + assert.Equal(t, "export const util = 2;", string(data)) + + assert.Empty(t, apitest.ListUnmatchedRequests()) + }) +} + +// TestEnsureNoSymlinkInPath exercises ensureNoSymlinkInPath's resolve-then-check +// policy directly: it must still reject a symlink whose target escapes root, +// but must now allow one whose target resolves to a legitimate, even deeply +// nested, location that is still inside root. +func TestEnsureNoSymlinkInPath(t *testing.T) { + t.Run("rejects a symlink whose target resolves outside root", func(t *testing.T) { + tmpDir := t.TempDir() + fsys := afero.NewOsFs() + + root := filepath.Join(tmpDir, "supabase", "functions") + require.NoError(t, os.MkdirAll(root, 0o755)) + + outsideDir := filepath.Join(tmpDir, "outside") + require.NoError(t, os.MkdirAll(outsideDir, 0o755)) + + // Plant a symlink inside root whose target lives outside it + // entirely -- the actual attack this check defends against. + linkDir := filepath.Join(root, "escape") + require.NoError(t, os.Symlink(outsideDir, linkDir)) + + dir := filepath.Join(linkDir, "nested") + err := ensureNoSymlinkInPath(fsys, root, dir) + require.Error(t, err) + assert.ErrorIs(t, err, ErrUnsafeDownloadPath) + }) + + t.Run("allows a symlink whose target resolves to a nested location inside root", func(t *testing.T) { + tmpDir := t.TempDir() + fsys := afero.NewOsFs() + + root := filepath.Join(tmpDir, "supabase", "functions") + require.NoError(t, os.MkdirAll(root, 0o755)) + + // A legitimate monorepo-style symlink: a shared directory that + // lives elsewhere inside the functions tree, symlinked into place + // from a function's own directory. + sharedDir := filepath.Join(root, "_shared") + require.NoError(t, os.MkdirAll(sharedDir, 0o755)) + + funcDir := filepath.Join(root, "my-func") + require.NoError(t, os.MkdirAll(funcDir, 0o755)) + linkDir := filepath.Join(funcDir, "_shared") + require.NoError(t, os.Symlink(sharedDir, linkDir)) + + // dir itself does not exist yet -- ensureNoSymlinkInPath is called + // before MkdirIfNotExistFS creates it -- so this also proves the + // still-nonexistent remainder is correctly re-joined onto the + // resolved ancestor. + dir := filepath.Join(linkDir, "nested", "deeper") + err := ensureNoSymlinkInPath(fsys, root, dir) + assert.NoError(t, err, "a symlink resolving to a legitimate location inside root must be allowed") + }) } func TestDownloadFunction(t *testing.T) { From be603e30453b4a36e8647012ea6bb513865973cc Mon Sep 17 00:00:00 2001 From: Colum Ferry Date: Tue, 7 Jul 2026 14:30:18 +0100 Subject: [PATCH 8/8] chore(cli): fix oxfmt formatting nit --- apps/cli/src/shared/functions/download.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/apps/cli/src/shared/functions/download.ts b/apps/cli/src/shared/functions/download.ts index c99a464556..7b9882f6dd 100644 --- a/apps/cli/src/shared/functions/download.ts +++ b/apps/cli/src/shared/functions/download.ts @@ -135,7 +135,9 @@ function validateDownloadFlags( const changed = [ hasExplicitLongFlag(rawArgs, downloadCommandPath, "use-api") ? "use-api" : undefined, hasExplicitLongFlag(rawArgs, downloadCommandPath, "use-docker") ? "use-docker" : undefined, - hasExplicitLongFlag(rawArgs, downloadCommandPath, "legacy-bundle") ? "legacy-bundle" : undefined, + hasExplicitLongFlag(rawArgs, downloadCommandPath, "legacy-bundle") + ? "legacy-bundle" + : undefined, ].filter((flag): flag is string => flag !== undefined); return changed.length <= 1