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..ac60ee2dfd 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) | | ------ | ---------------------------------------------- | ------------------------------ | ------------ | ------------------------------------------------------------------ | @@ -75,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 f6dd731220..279575fbe6 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"; @@ -62,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, @@ -109,11 +128,11 @@ export const legacyServices = Effect.fn("legacy.services")(function* (_flags: Le }; let rows = listLocalServiceVersions(localImageOptions); - if (Option.isSome(linkedProjectRef) && Option.isSome(accessToken)) { + if (Option.isSome(validLinkedRef) && Option.isSome(accessToken)) { const remote = yield* fetchLinkedServiceVersions({ apiUrl: cliConfig.apiUrl, projectHost: cliConfig.projectHost, - projectRef: linkedProjectRef.value, + projectRef: validLinkedRef.value, accessToken: accessToken.value, userAgent: cliConfig.userAgent, }); 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..7ea0e191c9 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,8 @@ function setup( format?: "text" | "json" | "stream-json"; goOutput?: Option.Option<"env" | "pretty" | "json" | "toml" | "yaml">; workdir?: string; + accessToken?: string; + apiUrl?: string; } = {}, ) { const out = mockOutput({ @@ -68,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(), @@ -77,7 +80,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 +97,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 +321,112 @@ 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 })))); + }); + + // 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" }); + + 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("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,