From 2d8807bea65a163a829b8f19ea00338d92a6d655 Mon Sep 17 00:00:00 2001 From: Colum Ferry Date: Mon, 6 Jul 2026 21:56:43 +0100 Subject: [PATCH 1/2] fix(cli): warn but skip remote lookup for a malformed services project ref MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Go's `cmd/services.go` validates the linked ref but only warns on failure and still calls listRemoteImages with the malformed value. TS now warns the same way but deliberately does not proceed with the bad ref, since it gets embedded unescaped into the tenant gateway hostname — keeping the existing SSRF hardening instead of matching Go's warn-and-proceed behavior. Fixes CLI-1872 --- apps/cli/docs/go-cli-porting-status.md | 12 ++++ .../legacy/commands/services/SIDE_EFFECTS.md | 22 ++++++-- .../commands/services/services.handler.ts | 33 ++++++++--- .../services/services.integration.test.ts | 55 ++++++++++++++++--- 4 files changed, 98 insertions(+), 24 deletions(-) diff --git a/apps/cli/docs/go-cli-porting-status.md b/apps/cli/docs/go-cli-porting-status.md index 42d783057c..c1cba8620b 100644 --- a/apps/cli/docs/go-cli-porting-status.md +++ b/apps/cli/docs/go-cli-porting-status.md @@ -323,3 +323,15 @@ Flag divergences from the Go reference: `reveal=true` so the Management API returns the full secret keys (`sb_secret_...`) in full instead of redacting them, addressing issue #4775. Default behavior (omitted flag) matches Go exactly. + +Behavioral divergences from the Go reference: + +- `services` warns on a malformed linked project ref (matching Go's + `flags.LoadProjectRef` validation message) but, unlike Go, does not then use + that ref for the remote lookup. Go's `cmd/services.go` treats the validation + failure as non-fatal and still calls `listRemoteImages` with the malformed + value; TS skips the remote lookup instead, since the ref is embedded + unescaped into the tenant gateway hostname and a malformed value could + redirect the service-role key to an attacker-controlled host. Intentional + TS-only hardening, not a parity bug — see + [`services/SIDE_EFFECTS.md`](../src/legacy/commands/services/SIDE_EFFECTS.md). diff --git a/apps/cli/src/legacy/commands/services/SIDE_EFFECTS.md b/apps/cli/src/legacy/commands/services/SIDE_EFFECTS.md index 07092ca913..7544ade2b2 100644 --- a/apps/cli/src/legacy/commands/services/SIDE_EFFECTS.md +++ b/apps/cli/src/legacy/commands/services/SIDE_EFFECTS.md @@ -16,12 +16,22 @@ ## API Routes -The resolved project ref must match `^[a-z]{20}$` (Go's `utils.ProjectRefPattern`) -before any remote lookup runs; a malformed ref skips the linked-version checks -and only the local matrix is printed. Tenant calls send `apikey: ` -and additionally `Authorization: Bearer ` unless the key is a -new-style `sb_…` key (which authenticates via the `apikey` header alone), -matching `apps/cli-go/pkg/fetcher/gateway.go`. +**Divergence from Go on a malformed ref:** Go validates the resolved ref against +`utils.ProjectRefPattern` (`^[a-z]{20}$`) but only warns on failure +(`cmd/services.go`'s `Run` prints the validation error to stderr) and still +calls `listRemoteImages` with the malformed ref anyway (`services.go:61-62`). +TS prints the same warning ("Invalid project ref format. Must be like +`abcdefghijklmnopqrst`.") but deliberately skips the remote lookup instead of +reproducing Go's behavior — the ref is embedded unescaped into the tenant +gateway hostname below, so proceeding with a malformed value would let it +redirect the service-role key to an attacker-controlled host. Only the local +matrix is printed in this case. This is intentional TS-only hardening, not a +parity bug. + +Tenant calls send `apikey: ` and additionally +`Authorization: Bearer ` unless the key is a new-style `sb_…` key +(which authenticates via the `apikey` header alone), matching +`apps/cli-go/pkg/fetcher/gateway.go`. | Method | Path | Auth | Request body | Response (used fields) | | ------ | ---------------------------------------------- | ------------------------------ | ------------ | ------------------------------------------------------------------ | diff --git a/apps/cli/src/legacy/commands/services/services.handler.ts b/apps/cli/src/legacy/commands/services/services.handler.ts index f6dd731220..dfa5632b10 100644 --- a/apps/cli/src/legacy/commands/services/services.handler.ts +++ b/apps/cli/src/legacy/commands/services/services.handler.ts @@ -1,6 +1,10 @@ import { Effect, Exit, FileSystem, Option, Path } from "effect"; import { LegacyCliConfig } from "../../config/legacy-cli-config.service.ts"; import { LegacyCredentials } from "../../auth/legacy-credentials.service.ts"; +import { + INVALID_PROJECT_REF_MESSAGE, + PROJECT_REF_PATTERN, +} from "../../config/legacy-project-ref.service.ts"; import { LegacyLinkedProjectCache } from "../../telemetry/legacy-linked-project-cache.service.ts"; import { LegacyTelemetryState } from "../../telemetry/legacy-telemetry-state.service.ts"; import { legacyReadDbToml } from "../../shared/legacy-db-config.toml-read.ts"; @@ -109,15 +113,26 @@ export const legacyServices = Effect.fn("legacy.services")(function* (_flags: Le }; let rows = listLocalServiceVersions(localImageOptions); - if (Option.isSome(linkedProjectRef) && Option.isSome(accessToken)) { - const remote = yield* fetchLinkedServiceVersions({ - apiUrl: cliConfig.apiUrl, - projectHost: cliConfig.projectHost, - projectRef: linkedProjectRef.value, - accessToken: accessToken.value, - userAgent: cliConfig.userAgent, - }); - rows = mergeRemoteServiceVersions(remote, localImageOptions); + if (Option.isSome(linkedProjectRef)) { + if (!PROJECT_REF_PATTERN.test(linkedProjectRef.value)) { + // Go's `flags.LoadProjectRef` (project_ref.go:54-76) validates the ref but + // `cmd/services.go`'s Run only warns on the error and keeps going, so Go + // still calls `listRemoteImages` with the malformed ref (services.go:61-62). + // TS matches the warning but deliberately skips the remote call instead of + // reproducing it: the ref is embedded unescaped into the tenant gateway + // hostname in `fetchLinkedServiceVersions`, so proceeding would let a + // malformed ref redirect the service-role key to an attacker-controlled host. + yield* output.raw(`${INVALID_PROJECT_REF_MESSAGE}\n`, "stderr"); + } else if (Option.isSome(accessToken)) { + const remote = yield* fetchLinkedServiceVersions({ + apiUrl: cliConfig.apiUrl, + projectHost: cliConfig.projectHost, + projectRef: linkedProjectRef.value, + accessToken: accessToken.value, + userAgent: cliConfig.userAgent, + }); + rows = mergeRemoteServiceVersions(remote, localImageOptions); + } } const warning = renderServicesWarning(rows); diff --git a/apps/cli/src/legacy/commands/services/services.integration.test.ts b/apps/cli/src/legacy/commands/services/services.integration.test.ts index 7e03294354..bd96539fab 100644 --- a/apps/cli/src/legacy/commands/services/services.integration.test.ts +++ b/apps/cli/src/legacy/commands/services/services.integration.test.ts @@ -5,10 +5,11 @@ import { describe, expect, it } from "@effect/vitest"; import { BunServices } from "@effect/platform-bun"; import { CliOutput, Command } from "effect/unstable/cli"; import { Stdio } from "effect"; -import { Cause, Effect, Exit, Layer, Option } from "effect"; +import { Cause, Effect, Exit, Layer, Option, Redacted } from "effect"; import { FetchHttpClient } from "effect/unstable/http"; import { LegacyCredentials } from "../../auth/legacy-credentials.service.ts"; import { LegacyCliConfig } from "../../config/legacy-cli-config.service.ts"; +import { INVALID_PROJECT_REF_MESSAGE } from "../../config/legacy-project-ref.service.ts"; import { LegacyLinkedProjectCache } from "../../telemetry/legacy-linked-project-cache.service.ts"; import { LEGACY_GLOBAL_FLAGS, LegacyOutputFlag } from "../../../shared/legacy/global-flags.ts"; import { @@ -45,6 +46,7 @@ function setup( format?: "text" | "json" | "stream-json"; goOutput?: Option.Option<"env" | "pretty" | "json" | "toml" | "yaml">; workdir?: string; + accessToken?: string; } = {}, ) { const out = mockOutput({ @@ -77,7 +79,10 @@ function setup( userAgent: "SupabaseCLI/test", }), ), - Layer.succeed(LegacyCredentials, LegacyCredentials.of(legacyCredentialsMock)), + Layer.succeed( + LegacyCredentials, + LegacyCredentials.of(legacyCredentialsMock(opts.accessToken)), + ), Layer.succeed( LegacyLinkedProjectCache, LegacyLinkedProjectCache.of({ @@ -91,13 +96,19 @@ function setup( }; } -const legacyCredentialsMock = { - getAccessToken: Effect.succeed(Option.none()), - saveAccessToken: () => Effect.die("unexpected saveAccessToken"), - deleteAccessToken: Effect.die("unexpected deleteAccessToken"), - deleteAllProjectCredentials: Effect.void, - deleteProjectCredential: () => Effect.succeed(false), -}; +function legacyCredentialsMock(accessToken?: string) { + return { + getAccessToken: Effect.succeed( + accessToken === undefined + ? Option.none() + : Option.some(Redacted.make(accessToken, { label: "SUPABASE_ACCESS_TOKEN" })), + ), + saveAccessToken: () => Effect.die("unexpected saveAccessToken"), + deleteAccessToken: Effect.die("unexpected deleteAccessToken"), + deleteAllProjectCredentials: Effect.void, + deleteProjectCredential: () => Effect.succeed(false), + }; +} const legacyTestRoot = Command.make("supabase").pipe( Command.withGlobalFlags(LEGACY_GLOBAL_FLAGS), @@ -309,6 +320,32 @@ major_version = 15 }).pipe(Effect.ensuring(Effect.sync(() => rmSync(workdir, { recursive: true, force: true })))); }); + it.live("warns and skips the remote lookup for a malformed linked project ref", () => { + const workdir = mkdtempSync(join(tmpdir(), "supabase-services-")); + writeTempFile(workdir, "project-ref", "not-a-valid-ref"); + const { layer, out } = setup({ workdir }); + + return Effect.gen(function* () { + yield* legacyServices({}).pipe(Effect.provide(layer)); + + expect(out.stderrText).toContain(INVALID_PROJECT_REF_MESSAGE); + expect(out.stdoutText).toContain("supabase/postgres"); + }).pipe(Effect.ensuring(Effect.sync(() => rmSync(workdir, { recursive: true, force: true })))); + }); + + it.live("does not attempt the remote lookup for a malformed ref even when logged in", () => { + const workdir = mkdtempSync(join(tmpdir(), "supabase-services-")); + writeTempFile(workdir, "project-ref", "not-a-valid-ref"); + const { layer, out } = setup({ workdir, accessToken: "sbp_test-token" }); + + return Effect.gen(function* () { + yield* legacyServices({}).pipe(Effect.provide(layer)); + + expect(out.stderrText).toContain(INVALID_PROJECT_REF_MESSAGE); + expect(out.stdoutText).toContain("supabase/postgres"); + }).pipe(Effect.ensuring(Effect.sync(() => rmSync(workdir, { recursive: true, force: true })))); + }); + it.live("reports pinned legacy temp service versions", () => { const workdir = makeProjectWithDbMajorVersion(15); writeTempFile(workdir, "postgres-version", "15.1.0.117\n"); From 441342eebc86b1e5495e9f9181d6d6fea797bb92 Mon Sep 17 00:00:00 2001 From: Colum Ferry Date: Mon, 6 Jul 2026 22:14:55 +0100 Subject: [PATCH 2/2] fix(cli): close review findings on the services malformed-ref warning - Match Go's stderr ordering (ref-format warning before config-load warning) - Close a branch-coverage gap: add integration coverage for the valid-ref + logged-in remote-fetch path that this change restructured - Correct resolveOptional's doc comment, which claimed Go skips validation entirely on its soft-load path when Go actually validates and warns, just doesn't fail the command - Note the untrimmed SUPABASE_PROJECT_ID env var and cross-reference the divergence from the Notes section of SIDE_EFFECTS.md for discoverability Filed CLI-1900 to audit other resolveOptional callers (projects list, db dump/query) for the same missing warning, since each needs its own Go-parity verification rather than a blanket fix. --- .../legacy/commands/services/SIDE_EFFECTS.md | 1 + .../commands/services/services.handler.ts | 44 +++++----- .../services/services.integration.test.ts | 85 ++++++++++++++++++- .../config/legacy-project-ref.service.ts | 11 ++- 4 files changed, 116 insertions(+), 25 deletions(-) diff --git a/apps/cli/src/legacy/commands/services/SIDE_EFFECTS.md b/apps/cli/src/legacy/commands/services/SIDE_EFFECTS.md index 7544ade2b2..ac60ee2dfd 100644 --- a/apps/cli/src/legacy/commands/services/SIDE_EFFECTS.md +++ b/apps/cli/src/legacy/commands/services/SIDE_EFFECTS.md @@ -85,5 +85,6 @@ TS-only NDJSON success event with the same `{ services: [...] }` payload. - Local versions come from the command's baked-in service matrix; the command does not inspect Docker state or local config files. - Linked-version checks are best-effort. Remote lookup failures do not change the exit code; they only leave the `LINKED` column empty for unavailable services. +- A malformed linked ref is the one lookup failure that prints an explicit stderr warning (see API Routes above); every other remote failure (network error, expired token, etc.) still fails silently and just leaves `LINKED` empty. Most real-world malformed refs come from an untrimmed `SUPABASE_PROJECT_ID` env var (e.g. a trailing newline from a secrets manager or `.env` file) rather than actual file tampering — the env var is read raw and unlike the on-disk `project-ref` file is never trimmed, matching Go's own `viper.GetString("PROJECT_ID")` (`internal/utils/flags/project_ref.go:62`). - Version mismatches are reported to stderr as a warning. - `telemetry.json` is written on every invocation, including `--output env` failures, to match the legacy Go command lifecycle. diff --git a/apps/cli/src/legacy/commands/services/services.handler.ts b/apps/cli/src/legacy/commands/services/services.handler.ts index dfa5632b10..279575fbe6 100644 --- a/apps/cli/src/legacy/commands/services/services.handler.ts +++ b/apps/cli/src/legacy/commands/services/services.handler.ts @@ -66,6 +66,21 @@ export const legacyServices = Effect.fn("legacy.services")(function* (_flags: Le yield* Effect.gen(function* () { const accessTokenExit = yield* credentials.getAccessToken.pipe(Effect.exit); const accessToken = Exit.isSuccess(accessTokenExit) ? accessTokenExit.value : Option.none(); + + const validLinkedRef = Option.filter(linkedProjectRef, (ref) => PROJECT_REF_PATTERN.test(ref)); + if (Option.isSome(linkedProjectRef) && Option.isNone(validLinkedRef)) { + // Go's `flags.LoadProjectRef` (project_ref.go:54-76) validates the ref but + // `cmd/services.go`'s Run only warns on the error and keeps going, so Go + // still calls `listRemoteImages` with the malformed ref (services.go:61-62). + // TS matches the warning but deliberately skips the remote call instead of + // reproducing it: the ref is embedded unescaped into the tenant gateway + // hostname in `fetchLinkedServiceVersions`, so proceeding would let a + // malformed ref redirect the service-role key to an attacker-controlled host. + // Emitted before the config-load warning below to match the order Go's + // `Run` prints them in (services.go:18-24). + yield* output.raw(`${INVALID_PROJECT_REF_MESSAGE}\n`, "stderr"); + } + const tomlValues = yield* legacyReadDbToml( fs, path, @@ -113,26 +128,15 @@ export const legacyServices = Effect.fn("legacy.services")(function* (_flags: Le }; let rows = listLocalServiceVersions(localImageOptions); - if (Option.isSome(linkedProjectRef)) { - if (!PROJECT_REF_PATTERN.test(linkedProjectRef.value)) { - // Go's `flags.LoadProjectRef` (project_ref.go:54-76) validates the ref but - // `cmd/services.go`'s Run only warns on the error and keeps going, so Go - // still calls `listRemoteImages` with the malformed ref (services.go:61-62). - // TS matches the warning but deliberately skips the remote call instead of - // reproducing it: the ref is embedded unescaped into the tenant gateway - // hostname in `fetchLinkedServiceVersions`, so proceeding would let a - // malformed ref redirect the service-role key to an attacker-controlled host. - yield* output.raw(`${INVALID_PROJECT_REF_MESSAGE}\n`, "stderr"); - } else if (Option.isSome(accessToken)) { - const remote = yield* fetchLinkedServiceVersions({ - apiUrl: cliConfig.apiUrl, - projectHost: cliConfig.projectHost, - projectRef: linkedProjectRef.value, - accessToken: accessToken.value, - userAgent: cliConfig.userAgent, - }); - rows = mergeRemoteServiceVersions(remote, localImageOptions); - } + if (Option.isSome(validLinkedRef) && Option.isSome(accessToken)) { + const remote = yield* fetchLinkedServiceVersions({ + apiUrl: cliConfig.apiUrl, + projectHost: cliConfig.projectHost, + projectRef: validLinkedRef.value, + accessToken: accessToken.value, + userAgent: cliConfig.userAgent, + }); + rows = mergeRemoteServiceVersions(remote, localImageOptions); } const warning = renderServicesWarning(rows); diff --git a/apps/cli/src/legacy/commands/services/services.integration.test.ts b/apps/cli/src/legacy/commands/services/services.integration.test.ts index bd96539fab..7ea0e191c9 100644 --- a/apps/cli/src/legacy/commands/services/services.integration.test.ts +++ b/apps/cli/src/legacy/commands/services/services.integration.test.ts @@ -47,6 +47,7 @@ function setup( goOutput?: Option.Option<"env" | "pretty" | "json" | "toml" | "yaml">; workdir?: string; accessToken?: string; + apiUrl?: string; } = {}, ) { const out = mockOutput({ @@ -70,7 +71,7 @@ function setup( LegacyCliConfig, LegacyCliConfig.of({ profile: "supabase", - apiUrl: "https://api.supabase.com", + apiUrl: opts.apiUrl ?? "https://api.supabase.com", projectHost: "supabase.co", poolerHost: "supabase.com", accessToken: Option.none(), @@ -333,7 +334,10 @@ major_version = 15 }).pipe(Effect.ensuring(Effect.sync(() => rmSync(workdir, { recursive: true, force: true })))); }); - it.live("does not attempt the remote lookup for a malformed ref even when logged in", () => { + // A token present doesn't bypass the format guard (Go's warning is + // unconditional on login too) — same code path as the previous test, so this + // isn't new branch coverage, just pinning that login state can't skip it. + it.live("still warns on a malformed ref even when logged in", () => { const workdir = mkdtempSync(join(tmpdir(), "supabase-services-")); writeTempFile(workdir, "project-ref", "not-a-valid-ref"); const { layer, out } = setup({ workdir, accessToken: "sbp_test-token" }); @@ -346,6 +350,83 @@ major_version = 15 }).pipe(Effect.ensuring(Effect.sync(() => rmSync(workdir, { recursive: true, force: true })))); }); + it.live("fetches and merges remote versions for a valid ref when logged in", () => { + const workdir = mkdtempSync(join(tmpdir(), "supabase-services-")); + writeTempFile(workdir, "project-ref", "abcdefghijklmnopqrst"); + + const server = Bun.serve({ + port: 0, + fetch(request) { + const url = new URL(request.url); + if (url.pathname === "/v1/projects/abcdefghijklmnopqrst") { + return Response.json({ + id: "abcdefghijklmnopqrst", + ref: "abcdefghijklmnopqrst", + organization_id: "org-id", + organization_slug: "org", + name: "Linked Project", + region: "us-east-1", + created_at: "2026-03-13T12:00:00.000Z", + status: "ACTIVE_HEALTHY", + database: { + host: "db.supabase.internal", + version: "17.6.1.200", + postgres_engine: "17", + release_channel: "ga", + }, + }); + } + + if (url.pathname === "/v1/projects/abcdefghijklmnopqrst/api-keys") { + // Deliberately no service-role key: this test only needs to prove the + // handler wires the fetch+merge branch through, not re-test + // `fetchLinkedServiceVersions`'s own tenant-probe logic (already + // covered in services.shared.unit.test.ts). Omitting the + // service-role key keeps this test free of a second, tenant-gateway + // mock without weakening the assertion below. + return Response.json([ + { + name: "anon", + id: "publishable-id", + type: "publishable", + api_key: "publishable-key", + description: null, + }, + ]); + } + + return new Response("not found", { status: 404 }); + }, + }); + + const { layer, out } = setup({ + workdir, + accessToken: "sbp_test-token", + apiUrl: server.url.origin, + goOutput: Option.some("json"), + }); + + return Effect.gen(function* () { + yield* legacyServices({}).pipe(Effect.provide(layer)); + + expect(out.stderrText).not.toContain(INVALID_PROJECT_REF_MESSAGE); + const rows = JSON.parse(out.stdoutText) as Array<{ + name: string; + local: string; + remote: string; + }>; + expect(rows).toContainEqual( + expect.objectContaining({ name: "supabase/postgres", remote: "17.6.1.200" }), + ); + }).pipe( + Effect.ensuring( + Effect.promise(() => server.stop(true)).pipe( + Effect.andThen(Effect.sync(() => rmSync(workdir, { recursive: true, force: true }))), + ), + ), + ); + }); + it.live("reports pinned legacy temp service versions", () => { const workdir = makeProjectWithDbMajorVersion(15); writeTempFile(workdir, "postgres-version", "15.1.0.117\n"); diff --git a/apps/cli/src/legacy/config/legacy-project-ref.service.ts b/apps/cli/src/legacy/config/legacy-project-ref.service.ts index 5c5e74f94b..f54579c331 100644 --- a/apps/cli/src/legacy/config/legacy-project-ref.service.ts +++ b/apps/cli/src/legacy/config/legacy-project-ref.service.ts @@ -41,9 +41,14 @@ interface LegacyProjectRefResolverShape { * `projects list` (`list.go:31-33`), which ignores `ErrNotLinked` and only * uses the value as a "linked" marker. Returns `None` when nothing resolves. * - * Unlike `resolve`, the returned value is **not** format-validated — Go's - * soft load also skips validation here, and the value is only used as a - * display marker, never injected into an API path. + * Unlike `resolve`, the returned value is **not** format-validated. Note this + * is a caller-side simplification, not a Go behavior match: Go's + * `LoadProjectRef` still validates and warns to stderr on a malformed ref + * (see `services`'s equivalent handling, CLI-1872), it just doesn't fail the + * command. `resolveOptional` skips the warning too, which is safe only + * because every current caller uses the value purely as a display marker, + * never injected into an API path — a caller that needs the Go-parity + * warning should validate and warn itself rather than assume this does it. */ readonly resolveOptional: ( flagValue: Option.Option,