diff --git a/apps/cli/src/legacy/auth/legacy-access-token.ts b/apps/cli/src/legacy/auth/legacy-access-token.ts index 0aadb70bfd..78a3865e02 100644 --- a/apps/cli/src/legacy/auth/legacy-access-token.ts +++ b/apps/cli/src/legacy/auth/legacy-access-token.ts @@ -1,10 +1,20 @@ import { Effect } from "effect"; +import { legacyAqua } from "../shared/legacy-colors.ts"; import { LegacyInvalidAccessTokenError } from "./legacy-errors.ts"; /** Go's `utils.AccessTokenPattern` (`apps/cli-go/internal/utils/access_token.go:16`). */ export const LEGACY_ACCESS_TOKEN_PATTERN = /^sbp_(oauth_)?[a-f0-9]{40}$/; +/** + * Go's `utils.ErrMissingToken` message (`internal/utils/access_token.go:18`), + * with `supabase login` through the Aqua colour gate exactly like Go's + * `Aqua(...)`. Built lazily because the gate inspects the target stream at + * call time. Shared by `db advisors` and the sso reconciled-credentials gate. + */ +export const legacyMissingAccessTokenMessage = (): string => + `Access token not provided. Supply an access token by running ${legacyAqua("supabase login")} or setting the SUPABASE_ACCESS_TOKEN environment variable.`; + /** Go's `utils.ErrInvalidToken` message (`internal/utils/access_token.go:17`). */ const LEGACY_INVALID_ACCESS_TOKEN_MESSAGE = "Invalid access token format. Must be like `sbp_0102...1920`."; diff --git a/apps/cli/src/legacy/auth/legacy-credentials.layer.ts b/apps/cli/src/legacy/auth/legacy-credentials.layer.ts index ffddf8b017..2f72c8bb4c 100644 --- a/apps/cli/src/legacy/auth/legacy-credentials.layer.ts +++ b/apps/cli/src/legacy/auth/legacy-credentials.layer.ts @@ -2,7 +2,10 @@ import { Effect, FileSystem, Layer, Option, Path, Redacted, Result } from "effec import { RuntimeInfo } from "../../shared/runtime/runtime-info.service.ts"; import { normalizeKeyringToken } from "../../shared/auth/keyring-token.ts"; -import { LegacyDebugLogger } from "../shared/legacy-debug-logger.service.ts"; +import { + LegacyDebugLogger, + type LegacyDebugLoggerShape, +} from "../shared/legacy-debug-logger.service.ts"; import { LegacyCliConfig } from "../config/legacy-cli-config.service.ts"; import { legacySupabaseHome } from "../config/legacy-profile-file.ts"; import { LEGACY_ACCESS_TOKEN_PATTERN, validateLegacyAccessToken } from "./legacy-access-token.ts"; @@ -345,36 +348,33 @@ const deleteAllKeyringEntries = ( } }); -const makeLegacyCredentials = Effect.gen(function* () { - const fs = yield* FileSystem.FileSystem; - const path = yield* Path.Path; - const runtimeInfo = yield* RuntimeInfo; - const cliConfig = yield* LegacyCliConfig; - const debugLogger = yield* LegacyDebugLogger; - const profileAccount = cliConfig.profile; - - // /access-token — fallback file path - const fallbackDir = legacySupabaseHome(runtimeInfo.homeDir); - const fallbackPath = path.join(fallbackDir, "access-token"); - - // `SUPABASE_NO_KEYRING=1` disables the OS keyring entirely (matches `next/`'s - // credentials layer and the cli-e2e harness, which sets it). Without this, any - // unconditional keyring access — e.g. `unlink`'s credential delete — blocks on a - // Keychain authorization prompt in non-interactive / CI contexts. - const noKeyring = process.env["SUPABASE_NO_KEYRING"] === "1"; - const wsl = yield* detectWsl(fs); - const keyringModule = - wsl || noKeyring +// `SUPABASE_NO_KEYRING=1` disables the OS keyring entirely (matches `next/`'s +// credentials layer and the cli-e2e harness, which sets it). Without this, any +// unconditional keyring access — e.g. `unlink`'s credential delete — blocks on a +// Keychain authorization prompt in non-interactive / CI contexts. +const loadKeyringModule = ( + fs: FileSystem.FileSystem, +): Effect.Effect> => + Effect.gen(function* () { + const noKeyring = process.env["SUPABASE_NO_KEYRING"] === "1"; + const wsl = yield* detectWsl(fs); + return wsl || noKeyring ? Option.none() : yield* Effect.tryPromise(() => import("@napi-rs/keyring")).pipe(Effect.option); + }); - const readKeyring = Effect.gen(function* () { +// Keyring chain for a given profile account: profile key first, then the +// legacy `access-token` key. The account parameter matters because Go keys +// the read on the RECONCILED `CurrentProfile.Name` (`access_token.go:43`). +const readKeyringForAccount = ( + keyringModule: Option.Option, + profileAccount: string, + platform: RuntimePlatform, + debugLogger: LegacyDebugLoggerShape, +): Effect.Effect> => + Effect.gen(function* () { if (Option.isNone(keyringModule)) return Option.none(); - const profileResult = yield* tryKeyringRead( - keyringModule.value, - profileAccount, - runtimeInfo.platform, - ); + const profileResult = yield* tryKeyringRead(keyringModule.value, profileAccount, platform); if (Option.isSome(profileResult)) { yield* debugLogger.debug(`Using access token for profile: ${profileAccount}`); return profileResult; @@ -382,7 +382,7 @@ const makeLegacyCredentials = Effect.gen(function* () { const legacyResult = yield* tryKeyringRead( keyringModule.value, LEGACY_KEYRING_ACCOUNT, - runtimeInfo.platform, + platform, ); if (Option.isSome(legacyResult)) { yield* debugLogger.debug("Using access token from credentials store..."); @@ -390,7 +390,11 @@ const makeLegacyCredentials = Effect.gen(function* () { return legacyResult; }); - const readFile = Effect.gen(function* () { +const readFallbackFile = ( + fs: FileSystem.FileSystem, + fallbackPath: string, +): Effect.Effect> => + Effect.gen(function* () { const exists = yield* fs.exists(fallbackPath).pipe(Effect.orElseSucceed(() => false)); if (!exists) return Option.none(); const content = yield* fs.readFileString(fallbackPath).pipe(Effect.orElseSucceed(() => "")); @@ -398,6 +402,82 @@ const makeLegacyCredentials = Effect.gen(function* () { return trimmed.length === 0 ? Option.none() : Option.some(trimmed); }); +/** + * Token resolution for an explicit profile account, mirroring the service's + * `getAccessToken` chain exactly: env token → keyring (profile account, then + * legacy account) → fallback file. Go resolves credentials AFTER + * `LoadProfile`, so the keyring account is the reconciled + * `CurrentProfile.Name` (`access_token.go:43`) — but the `LegacyCredentials` + * service captures the config layer's profile at construction. Commands that + * reconcile a pflag-effective profile (sso add/update, PR #5974 round 9) + * resolve their token through this instead, keyed on the reconciled name. + * Fails with the same validation error as the service; callers absorb it the + * same way `resolveLegacyAccessToken` does. + */ +export const legacyAccessTokenForProfile = Effect.fnUntraced(function* (profileAccount: string) { + const fs = yield* FileSystem.FileSystem; + const path = yield* Path.Path; + const runtimeInfo = yield* RuntimeInfo; + const cliConfig = yield* LegacyCliConfig; + // `serviceOption` keeps the logger optional (no-op outside the real CLI + // tree), same as the sso pflag-reconcile module's optional services. + const debugLogger: LegacyDebugLoggerShape = Option.getOrElse( + yield* Effect.serviceOption(LegacyDebugLogger), + () => ({ debug: () => Effect.void, http: () => Effect.void }), + ); + + if (Option.isSome(cliConfig.accessToken)) { + yield* debugLogger.debug("Using access token from env var..."); + yield* validateLegacyAccessToken(Redacted.value(cliConfig.accessToken.value)); + return Option.some(cliConfig.accessToken.value); + } + + const keyringModule = yield* loadKeyringModule(fs); + const keyringValue = yield* readKeyringForAccount( + keyringModule, + profileAccount, + runtimeInfo.platform, + debugLogger, + ); + if (Option.isSome(keyringValue)) { + yield* validateLegacyAccessToken(keyringValue.value); + return Option.some(Redacted.make(keyringValue.value)); + } + + const fallbackPath = path.join(legacySupabaseHome(runtimeInfo.homeDir), "access-token"); + const fileValue = yield* readFallbackFile(fs, fallbackPath); + if (Option.isSome(fileValue)) { + yield* debugLogger.debug(`Using access token from file: ${fallbackPath}`); + yield* validateLegacyAccessToken(fileValue.value); + return Option.some(Redacted.make(fileValue.value)); + } + + return Option.none>(); +}); + +const makeLegacyCredentials = Effect.gen(function* () { + const fs = yield* FileSystem.FileSystem; + const path = yield* Path.Path; + const runtimeInfo = yield* RuntimeInfo; + const cliConfig = yield* LegacyCliConfig; + const debugLogger = yield* LegacyDebugLogger; + const profileAccount = cliConfig.profile; + + // /access-token — fallback file path + const fallbackDir = legacySupabaseHome(runtimeInfo.homeDir); + const fallbackPath = path.join(fallbackDir, "access-token"); + + const keyringModule = yield* loadKeyringModule(fs); + + const readKeyring = readKeyringForAccount( + keyringModule, + profileAccount, + runtimeInfo.platform, + debugLogger, + ); + + const readFile = readFallbackFile(fs, fallbackPath); + return LegacyCredentials.of({ getAccessToken: Effect.gen(function* () { // Env takes precedence (matches access_token.go:38). diff --git a/apps/cli/src/legacy/commands/db/advisors/advisors.handler.ts b/apps/cli/src/legacy/commands/db/advisors/advisors.handler.ts index b84ca59906..f3cf7b40c9 100644 --- a/apps/cli/src/legacy/commands/db/advisors/advisors.handler.ts +++ b/apps/cli/src/legacy/commands/db/advisors/advisors.handler.ts @@ -7,6 +7,7 @@ import { ProcessControl } from "../../../../shared/runtime/process-control.servi import { LegacyCredentials } from "../../../auth/legacy-credentials.service.ts"; import { LegacyProjectRefResolver } from "../../../config/legacy-project-ref.service.ts"; import { legacyAqua } from "../../../shared/legacy-colors.ts"; +import { legacyMissingAccessTokenMessage } from "../../../auth/legacy-access-token.ts"; import { legacyFailsOn } from "../../../shared/legacy-fail-on.ts"; import { LegacyIdentityStitch } from "../../../shared/legacy-identity-stitch.ts"; import { LegacyDbConfigResolver } from "../../../shared/legacy-db-config.service.ts"; @@ -36,10 +37,6 @@ import { import { legacyFetchPerformanceAdvisors, legacyFetchSecurityAdvisors } from "./advisors.linked.ts"; import { splitLegacyLintsSql } from "./advisors.lints-sql.ts"; -/** Go's `utils.ErrMissingToken` (`internal/utils/access_token.go:18`). */ -const missingTokenMessage = (): string => - `Access token not provided. Supply an access token by running ${legacyAqua("supabase login")} or setting the SUPABASE_ACCESS_TOKEN environment variable.`; - /** Go's advisors PreRunE `utils.CmdSuggestion` (`cmd/db.go`). */ const loginSuggestion = (): string => `Run ${legacyAqua("supabase login")} first.`; @@ -164,7 +161,7 @@ const runLinked = Effect.fnUntraced(function* ( if (Option.isNone(tokenOpt)) { return yield* Effect.fail( new LegacyDbAdvisorsNotLoggedInError({ - message: missingTokenMessage(), + message: legacyMissingAccessTokenMessage(), suggestion: loginSuggestion(), }), ); diff --git a/apps/cli/src/legacy/commands/sso/add/SIDE_EFFECTS.md b/apps/cli/src/legacy/commands/sso/add/SIDE_EFFECTS.md index 350a2d34c9..209a143670 100644 --- a/apps/cli/src/legacy/commands/sso/add/SIDE_EFFECTS.md +++ b/apps/cli/src/legacy/commands/sso/add/SIDE_EFFECTS.md @@ -40,15 +40,20 @@ same shape via an inline anonymous struct with `Default *any`. ## Exit Codes -| Code | Condition | -| ---- | -------------------------------------------------------------------------------------------------------------------- | -| `0` | success | -| `1` | `LegacySsoMutexFlagError` — `--metadata-file` and `--metadata-url` both set | -| `1` | `LegacySsoAddMetadataFileError` — metadata file unreadable, non-UTF-8, or metadata URL invalid/unreachable/non-UTF-8 | -| `1` | `LegacySsoAddAttributeMappingFileError` — JSON file unreadable or malformed | -| `1` | `LegacySsoAddSamlDisabledError` — 404 from POST | -| `1` | `LegacySsoAddUnexpectedStatusError` — other non-2xx | -| `1` | `LegacySsoAddNetworkError` — transport-level failure | +| Code | Condition | +| ---- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `0` | success | +| `1` | `LegacySsoInvalidFlagValueError` — a `--type`/`--skip-url-validation`/`--name-id-format` occurrence pflag's `Value.Set` would reject (enum membership / `strconv.ParseBool`; fails before every validation; no request) | +| `1` | `LegacySsoFlagNeedsArgumentError` — a bare value-taking flag is the final argv token (pflag `ValueRequiredError`, fails before every validation; no request) | +| `1` | `LegacySsoProfileError` — the pflag/viper-effective `--profile`/`SUPABASE_PROFILE` cannot be loaded the way Go's `LoadProfile` loads it (root `PersistentPreRunE`, before `ChangeWorkDir`; beats the workdir, required-flag, and mutex checks; no request) | +| `1` | `LegacySsoWorkdirError` — the pflag/viper-effective `--workdir`/`SUPABASE_WORKDIR` is not an existing directory (Go `ChangeWorkDir` in root `PersistentPreRunE`; beats the required-flag and mutex checks; no request) | +| `1` | `LegacySsoAddRequiredFlagError` — pflag consumed the `--type`/`-t` token as another flag's value (cobra `ValidateRequiredFlags`) | +| `1` | `LegacySsoMutexFlagError` — `--metadata-file` and `--metadata-url` both set | +| `1` | `LegacySsoAddMetadataFileError` — metadata file unreadable, non-UTF-8, or metadata URL invalid/unreachable/non-UTF-8 | +| `1` | `LegacySsoAddAttributeMappingFileError` — JSON file unreadable or malformed | +| `1` | `LegacySsoAddSamlDisabledError` — 404 from POST | +| `1` | `LegacySsoAddUnexpectedStatusError` — other non-2xx | +| `1` | `LegacySsoAddNetworkError` — transport-level failure | ## Telemetry Events Fired @@ -78,7 +83,12 @@ Single `success` event with the parsed response as data. ## Notes - `--type saml` is **required** (Go's `MarkFlagRequired("type")`). -- `--metadata-file` and `--metadata-url` are mutually exclusive. +- `--metadata-file` and `--metadata-url` are mutually exclusive (Go's `MarkFlagsMutuallyExclusive`, `cmd/sso.go:164`). Violations emit cobra's exact template: `if any flags in the group [metadata-file metadata-url] are set none of the others can be; [metadata-file metadata-url] were all set`. "Set" follows `pflag.Changed` semantics — an explicit empty value (`--metadata-file=`) still counts. +- Flag values follow pflag's consumption rules, not the TS parser's: every value the handler acts on (`--project-ref`, `--metadata-file`, `--metadata-url`, `--attribute-mapping-file`, `--domains`, `--name-id-format`, `--skip-url-validation`) is reconciled against a pflag-faithful raw-argv scan. E.g. `--project-ref --metadata-file x.xml --metadata-url u` hands `--metadata-file` to `--project-ref` as its value and fails ref validation — the metadata file is never read (CLI-1982). Repeated flags resolve last-wins (pflag Sets every occurrence; the TS parser is first-wins), and an occurrence pflag's `Value.Set` would reject — `--type` outside `[ saml ]`, a boolean outside Go's `strconv.ParseBool` set (`--skip-url-validation=yes`), or a `--name-id-format` outside the enum — fails with pflag's exact `invalid argument …` message before every validation and request. +- Required-ness follows pflag too: when the `--type` token is itself consumed as another flag's value (`--domains --type saml`), the command fails with cobra's exact `required flag(s) "type" not set` before any request (cobra `ValidateRequiredFlags` runs before `ValidateFlagGroups`). `-t` shorthand occurrences are recognised by the scan and never trip this. +- The workdir follows pflag/viper too: Go's `ChangeWorkDir` (root `PersistentPreRunE`) chdir's to the effective `--workdir` (last occurrence, even a flag-shaped consumed token like `--workdir --metadata-file`) or `SUPABASE_WORKDIR`, and a missing directory aborts with Go's exact `failed to change workdir: chdir …` before the required-flag check, the mutex check, and any request. A changed-but-empty `--workdir=` shadows the env var and falls back to the always-valid project-root walk-up, exactly like viper. +- The profile follows pflag/viper too (PR #5974 round 7): whenever the pflag-effective `--profile`/`SUPABASE_PROFILE` token differs from the one the Effect parser gave the config layer (a `--profile` token consumed by another flag — `--domains --profile alternate.yml` targets the env/default profile, not `alternate.yml`; a flag-shaped consumed value — `--profile --metadata-url`; repeats, which pflag resolves last-wins; an explicit `--profile supabase` shadowing the env; an untrimmed/empty persisted `~/.supabase/profile` file), the handler re-runs Go's `LoadProfile` on the effective token (`sso.load-profile.ts`) — the POST targets that profile's `api_url`, and a token Go cannot load aborts with Go's error (`failed to read profile: …` / `failed to parse profile: …` / `invalid profile: …`, byte-exact for the deterministic classes) before the workdir check and any request. Where the scan and the parser agree — every normal invocation — the config layer's resolution (including its pre-existing lenient missing/malformed-file fallback, which predates CLI-1982 and applies shell-wide) is used unchanged. The upgrade-gate fallback GETs and the linked-project cache fill also target the reconciled host (Go's `CurrentProfile` is process-wide). +- Accepted micro-divergences of the profile emulation (each fail-closed: both CLIs exit 1 with zero requests; only stderr detail can differ): YAML parse-failure detail text (JS `yaml` vs go-yaml, shared `failed to read profile: While parsing config: ` prefix); non-YAML/JSON viper config types (`.toml`, `.env`, …) parsed as YAML; `http_url`/`hostname_rfc1123`/`uuid4` validator tags approximated; the final line of a padded multi-line error loses its trailing spaces to the shared error normalizer's trim. Also: when the effective and layer profiles differ AND the token is keyring-relevant, the keyring token lookup still uses the layer profile's name (env-token flows, e.g. the cli-e2e harness, are unaffected), and the upgrade-suggestion billing URL keeps the layer profile's dashboard host. - `--skip-url-validation` skips the HTTPS-only + 10s GET + UTF-8 body validation against the metadata URL. - Metadata URL validation error message: `only HTTPS Metadata URLs are supported Use --skip-url-validation to suppress this error` (no trailing period — matches Go's `create.go:47`; differs from `sso update`'s variant). - The `## Attribute Mapping` / `## SAML 2.0 Metadata XML` sections are emitted as plain markdown (heading + fence). Visual styling of the headings does not match Go's Glamour-rendered output; the XML body inside the fence is byte-parity via `formatSsoMetadataXml`. diff --git a/apps/cli/src/legacy/commands/sso/add/add.command.ts b/apps/cli/src/legacy/commands/sso/add/add.command.ts index e48071520e..403a636de6 100644 --- a/apps/cli/src/legacy/commands/sso/add/add.command.ts +++ b/apps/cli/src/legacy/commands/sso/add/add.command.ts @@ -5,15 +5,9 @@ import { withJsonErrorHandling } from "../../../../shared/output/json-error-hand import { legacyManagementApiRuntimeLayer } from "../../../shared/legacy-management-api-runtime.layer.ts"; import { legacyParseStringSliceFlag } from "../../../shared/legacy-string-slice-flag.ts"; import { withLegacyCommandInstrumentation } from "../../../telemetry/legacy-command-instrumentation.ts"; +import { LEGACY_SSO_NAME_ID_FORMATS } from "../sso.saml.ts"; import { legacySsoAdd } from "./add.handler.ts"; -const NAME_ID_FORMATS = [ - "urn:oasis:names:tc:SAML:1.1:nameid-format:emailAddress", - "urn:oasis:names:tc:SAML:1.1:nameid-format:unspecified", - "urn:oasis:names:tc:SAML:2.0:nameid-format:persistent", - "urn:oasis:names:tc:SAML:2.0:nameid-format:transient", -] as const; - export const legacySsoAddDomainsFlag = Flag.string("domains").pipe( Flag.atLeast(0), Flag.withDescription( @@ -61,7 +55,7 @@ const config = { ), Flag.optional, ), - nameIdFormat: Flag.choice("name-id-format", NAME_ID_FORMATS).pipe( + nameIdFormat: Flag.choice("name-id-format", LEGACY_SSO_NAME_ID_FORMATS).pipe( Flag.withDescription( "URI reference representing the classification of string-based identifier information.", ), diff --git a/apps/cli/src/legacy/commands/sso/add/add.handler.ts b/apps/cli/src/legacy/commands/sso/add/add.handler.ts index 29b3592ba9..55534b2d89 100644 --- a/apps/cli/src/legacy/commands/sso/add/add.handler.ts +++ b/apps/cli/src/legacy/commands/sso/add/add.handler.ts @@ -1,10 +1,16 @@ -import { Effect, Option } from "effect"; +import { Effect, Option, Redacted, Result, Stdio } from "effect"; import * as HttpClient from "effect/unstable/http/HttpClient"; import * as HttpClientRequest from "effect/unstable/http/HttpClientRequest"; import { LegacyCliConfig } from "../../../config/legacy-cli-config.service.ts"; import { LegacyProjectRefResolver } from "../../../config/legacy-project-ref.service.ts"; import { LegacyOutputFlag } from "../../../../shared/legacy/global-flags.ts"; +import { + cobraMutuallyExclusiveErrorMessage, + PERSISTENT_VALUE_FLAG_NAMES, + PERSISTENT_VALUE_FLAG_SHORTHANDS, + pflagArgvScan, +} from "../../../../shared/cli/cobra-flag-groups.ts"; import { Output } from "../../../../shared/output/output.service.ts"; import { encodeGoJson, @@ -14,6 +20,8 @@ import { } from "../../../shared/legacy-go-output.encoders.ts"; import { sanitizeLegacyErrorBody } from "../../../shared/legacy-http-errors.ts"; import { resolveLegacyAccessToken } from "../../../shared/legacy-resolve-token.ts"; +import { legacyAccessTokenForProfile } from "../../../auth/legacy-credentials.layer.ts"; +import { legacyMissingAccessTokenMessage } from "../../../auth/legacy-access-token.ts"; import { LegacyLinkedProjectCache } from "../../../telemetry/legacy-linked-project-cache.service.ts"; import { LegacyTelemetryState } from "../../../telemetry/legacy-telemetry-state.service.ts"; import { legacySuggestUpgrade } from "../../../shared/legacy-upgrade-suggest.ts"; @@ -21,13 +29,29 @@ import { LegacySsoAddAttributeMappingFileError, LegacySsoAddMetadataFileError, LegacySsoAddNetworkError, + LegacySsoAddRequiredFlagError, LegacySsoAddSamlDisabledError, LegacySsoAddUnexpectedStatusError, + LegacySsoFlagNeedsArgumentError, + LegacySsoInvalidFlagValueError, LegacySsoMutexFlagError, + LegacySsoAccessTokenError, } from "../sso.errors.ts"; import { renderSingleProvider, toLegacySsoProviderView } from "../sso.format.ts"; import { validateMetadataUrl } from "../sso.metadata-url.ts"; -import { readAttributeMappingFile, readMetadataFile } from "../sso.saml.ts"; +import { + legacySsoPflagBoolValue, + legacySsoPflagEnumValue, + legacySsoPflagSliceValue, + legacySsoPflagStringValue, + legacySsoResolvePflagProfile, + legacySsoValidatePflagWorkdir, +} from "../sso.pflag-reconcile.ts"; +import { + LEGACY_SSO_NAME_ID_FORMATS, + readAttributeMappingFile, + readMetadataFile, +} from "../sso.saml.ts"; import type { LegacySsoAddFlags } from "./add.command.ts"; const SAML_DISABLED_MESSAGE = @@ -42,6 +66,40 @@ const readAttributeMapping = readAttributeMappingFile({ openError: (args) => new LegacySsoAddAttributeMappingFileError(args), }); +const SSO_ADD_COMMAND_PATH = ["sso", "add"] as const; + +/** + * `sso add`'s single mutually-exclusive group, in Go's registration order + * (`cmd/sso.go:164` — `MarkFlagsMutuallyExclusive("metadata-file", + * "metadata-url")`). Registration order determines the first bracket of + * cobra's error template; only the violating subset gets sorted. + */ +const SSO_ADD_MUTEX_GROUP = ["metadata-file", "metadata-url"] as const; + +/** + * Every value-taking (non-boolean) flag reachable when `sso add` parses: + * the command's own (`add.command.ts`) plus the root's persistent value + * flags — these tell `pflagArgvScan` which bare tokens consume the next + * argv token as their value. `--skip-url-validation` is this command's only + * boolean flag and is deliberately excluded; booleans never consume a + * following token. `--type`'s `-t` shorthand (Go `cmd/sso.go:157` `VarP`) + * is covered via the shorthand map so a genuine `-t saml` invocation is + * seen exactly as pflag sees it. + */ +const SSO_ADD_SCAN_SPEC = { + valueFlagNames: new Set([ + "project-ref", + "type", + "domains", + "metadata-file", + "metadata-url", + "attribute-mapping-file", + "name-id-format", + ...PERSISTENT_VALUE_FLAG_NAMES, + ]), + valueFlagShorthands: new Map([["t", "type"], ...PERSISTENT_VALUE_FLAG_SHORTHANDS]), +} as const; + export const legacySsoAdd = Effect.fn("legacy.sso.add")(function* (flags: LegacySsoAddFlags) { const output = yield* Output; const goOutputFlag = yield* LegacyOutputFlag; @@ -50,17 +108,181 @@ export const legacySsoAdd = Effect.fn("legacy.sso.add")(function* (flags: Legacy const resolver = yield* LegacyProjectRefResolver; const linkedProjectCache = yield* LegacyLinkedProjectCache; const telemetryState = yield* LegacyTelemetryState; + const stdio = yield* Stdio.Stdio; + const rawArgs = yield* stdio.args; yield* Effect.gen(function* () { - if (Option.isSome(flags.metadataFile) && Option.isSome(flags.metadataUrl)) { + // cobra runs `ValidateRequiredFlags` (`command.go:1007`) and + // `ValidateFlagGroups` (`command.go:1010`) — in that order — before + // `RunE` (`command.go:1015`), so these checks must precede everything Go + // does inside `RunE`. Keep this block first. + // + // "Set" follows cobra's `pflag.Changed` — whether the flag was passed at + // all — not the resulting value: `--metadata-file= --metadata-url x` must + // still trip the error even though the file path is empty. Scanning raw + // argv keeps detection aligned with pflag's semantics rather than with + // whatever the TS parser produced — e.g. a bare + // `--metadata-file --metadata-url` parses to two `none`s here but is a + // single consumed value in pflag (CLI-1982). + const scan = pflagArgvScan(rawArgs, SSO_ADD_COMMAND_PATH, SSO_ADD_SCAN_SPEC); + const occurrences = scan.occurrences; + + // pflag calls `Value.Set` for every occurrence in argv order, and an + // invalid value fails `ParseFlags` (cobra `command.go:919`) before + // `ValidateArgs`, every hook, `ValidateRequiredFlags`, and `RunE` — + // reachable here because the Effect parser resolves repeated flags + // first-wins without validating later occurrences (`--type saml --type + // bogus` parses, then Go rejects `bogus` and never POSTs) and accepts + // `yes`/`no`, which `strconv.ParseBool` rejects (binary-verified, PR + // #5974 review round 4). These checks precede the missing-value check + // because a missing value can only arise at the final argv token, so + // every recorded occurrence pflag would reject sits earlier in its + // sequential walk. Flags are checked in Go registration order + // (`cmd/sso.go:157-163`); pflag itself errors in argv order when several + // flags carry invalid occurrences at once — accepted micro-divergence. + // The enum helpers also yield the pflag-effective (last-occurrence) + // values; `--type`'s stays unused because every valid occurrence is the + // enum's single member, so the parsed `flags.type` is already + // pflag-effective whenever this validation passes. + yield* Result.match(legacySsoPflagEnumValue(occurrences, "type", ["saml"], "-t, --type"), { + onFailure: (message: string) => Effect.fail(new LegacySsoInvalidFlagValueError({ message })), + onSuccess: Effect.succeed, + }); + const skipUrlValidation = yield* Result.match( + legacySsoPflagBoolValue(occurrences, "skip-url-validation"), + { + onFailure: (message: string) => + Effect.fail(new LegacySsoInvalidFlagValueError({ message })), + onSuccess: Effect.succeed, + }, + ); + const nameIdFormat = yield* Result.match( + legacySsoPflagEnumValue(occurrences, "name-id-format", LEGACY_SSO_NAME_ID_FORMATS), + { + onFailure: (message: string) => + Effect.fail(new LegacySsoInvalidFlagValueError({ message })), + onSuccess: Effect.succeed, + }, + ); + + // pflag fails `ParseFlags` (cobra `command.go:919`) when a bare + // value-taking flag is the final token (`sso add --type saml --domains`) + // — before every validation, hook, and `RunE`, so no POST is ever made. + // The Effect parser accepts that argv (the flag parses as unset), hence + // the emulation. Binary-verified against `apps/cli-go` (PR #5974 review + // round 3). Keep this the very first check. + if (scan.missingValueError !== undefined) { + return yield* Effect.fail( + new LegacySsoFlagNeedsArgumentError({ message: scan.missingValueError }), + ); + } + + // Go's root `PersistentPreRunE` loads the pflag/viper-effective + // `--profile`/`SUPABASE_PROFILE` (`LoadProfile`, `cmd/root.go:98-102`, + // `internal/utils/profile.go:94-118`) immediately BEFORE `ChangeWorkDir`, + // so an unloadable profile aborts before the workdir check, the + // required-type check, the mutex check, and any POST — and a loadable one + // decides which API host receives the POST. Reachable exactly where the + // scan and the parser disagree: in `sso add --type saml --domains + // --profile alternate.yml` pflag hands `--profile` to `--domains` and Go + // targets the env/default profile, while the Effect parser read + // `alternate.yml` as the profile and built `LegacyCliConfig` from it — + // without this reconciliation the POST goes to an API host Go never + // contacts (binary-verified, PR #5974 review round 7). Where the scan + // and the parser agree, this resolves to `none` and the config layer's + // apiUrl below is already pflag-effective. + const reconciledProfile = yield* legacySsoResolvePflagProfile(scan); + const profileApiUrl = Option.map(reconciledProfile, (profile) => profile.apiUrl); + // Reconciled-profile credentials, resolved ONCE for the main request and + // every auxiliary call (linked-project cache fill, upgrade-gate fallback + // GETs): Go's reconciled `CurrentProfile` + `GetAccessToken` apply + // process-wide (`access_token.go:43`, review r3684524241). `undefined` + // when the scan and the parser agree — every consumer then resolves from + // the config-layer services as before. + // Reconciled-profile credentials, resolved LAZILY (memoized) so the first + // read happens at the request site — Go's token gate is `GetSupabase` + // inside RunE (`api.go:119-124`), AFTER required/mutex/workdir + // validation, so a missing or invalid reconciled token must not pre-empt + // those errors (review r3686720488). Missing → Go's ErrMissingToken; + // invalid → ErrInvalidToken (the validation failure propagates). The + // auxiliary calls (cache fill, upgrade-gate GETs) use the absorbed + // variant: failures skip like Go's best-effort `ensureProjectGroupsCached`. + const reconciledTokenCached = Option.isSome(reconciledProfile) + ? yield* Effect.cached(legacyAccessTokenForProfile(reconciledProfile.value.name)) + : undefined; + const reconciledTokenForAux = + reconciledTokenCached === undefined + ? Effect.succeed> | undefined>(undefined) + : Effect.catch(reconciledTokenCached, () => + Effect.succeed(Option.none>()), + ); + + // Go's root `PersistentPreRunE` chdir's to the pflag/viper-effective + // `--workdir`/`SUPABASE_WORKDIR` (`ChangeWorkDir`, `cmd/root.go:104`, + // `internal/utils/misc.go:238-257`) after `ParseFlags` and before + // `ValidateRequiredFlags` (`command.go:1007`) and `ValidateFlagGroups` + // (`command.go:1010`), so a missing directory aborts before the + // required-type check, the mutex check, and any POST. Reachable exactly + // where the scan and the parser disagree: in `sso add --type saml + // --project-ref --workdir --metadata-file missing.xml` pflag binds + // `"--metadata-file"` to `--workdir` and Go exits at chdir, while the + // Effect parser refused that flag-shaped value and read `missing.xml` as + // metadata — without this check the reconciliation below would silently + // drop the metadata source and POST a provider Go never creates + // (binary-verified, PR #5974 review round 6). + yield* legacySsoValidatePflagWorkdir(scan); + + // `MarkFlagRequired("type")` (`cmd/sso.go:165`): when pflag consumed the + // `--type` or `-t` token as another flag's value (e.g. `--domains --type + // saml` or `--domains -t saml`), pflag never marks `type` changed and Go + // fails the required-flag check before `RunE` — no POST is ever made. + // The Effect parser can't see this (it refuses flag-shaped values, so it + // read `--type saml` / `-t saml` as a normal flag), hence the emulation + // here. A genuine `-t saml` records a `type` occurrence via the scan's + // shorthand map and never trips this. + if (!occurrences.has("type") && scan.consumedFlagNames.has("type")) { + return yield* Effect.fail( + new LegacySsoAddRequiredFlagError({ message: `required flag(s) "type" not set` }), + ); + } + + const changed = SSO_ADD_MUTEX_GROUP.filter((flagName) => occurrences.has(flagName)); + if (changed.length > 1) { return yield* Effect.fail( new LegacySsoMutexFlagError({ - message: "only one of --metadata-file or --metadata-url may be set", + message: cobraMutuallyExclusiveErrorMessage(SSO_ADD_MUTEX_GROUP, changed), }), ); } - const ref = yield* resolver.resolve(flags.projectRef); + // The scan and the Effect parser can disagree on more than the mutex: + // pflag consumes flag-shaped tokens as values, the Effect parser does + // not. Everything the handler acts on below is therefore reconciled to + // the pflag-effective values from the same scan, so a suppressed mutex + // can never pair with a metadata source pflag never set (e.g. + // `--project-ref --metadata-file x.xml --metadata-url u`, where Go hands + // `--metadata-file` to `--project-ref` and fails ref validation without + // ever touching metadata). `--type` keeps its parsed value: the enum has + // a single member every occurrence was validated against above, so + // whenever the handler runs at all the parsed value equals the + // pflag-effective one (the consumed-token case is rejected by the + // required-flag check above). `--name-id-format` and + // `--skip-url-validation` were reconciled above, alongside their pflag + // value validation. + const projectRef = legacySsoPflagStringValue(occurrences, "project-ref"); + const metadataFile = legacySsoPflagStringValue(occurrences, "metadata-file"); + const metadataUrl = legacySsoPflagStringValue(occurrences, "metadata-url"); + const attributeMappingFile = legacySsoPflagStringValue(occurrences, "attribute-mapping-file"); + const domains = legacySsoPflagSliceValue(occurrences, "domains", flags.domains); + + const ref = yield* resolver.resolve(projectRef); + + // Effective API base URL: the pflag-reconciled profile's when the scan + // and the parser disagreed on `--profile`, the config layer's otherwise. + // Go's reconciled `CurrentProfile` applies process-wide, so the POST, the + // upgrade-gate fallback GETs, and the linked-project cache GET all target + // the same host (PR #5974 round 7). + const apiUrl = Option.getOrElse(profileApiUrl, () => cliConfig.apiUrl); yield* Effect.gen(function* () { // Permissive request body. We POST as raw JSON to preserve any @@ -71,12 +293,12 @@ export const legacySsoAdd = Effect.fn("legacy.sso.add")(function* (flags: Legacy type: flags.type, }; - if (Option.isSome(flags.metadataFile)) { - const xml = yield* readMetadata(flags.metadataFile.value); + if (Option.isSome(metadataFile)) { + const xml = yield* readMetadata(metadataFile.value); body["metadata_xml"] = xml; - } else if (Option.isSome(flags.metadataUrl)) { - if (!flags.skipUrlValidation) { - yield* validateMetadataUrl(flags.metadataUrl.value).pipe( + } else if (Option.isSome(metadataUrl)) { + if (!skipUrlValidation) { + yield* validateMetadataUrl(metadataUrl.value).pipe( // Note: Go suffixes with no trailing period (matches `create.go:47`). Effect.mapError( (cause) => @@ -86,33 +308,42 @@ export const legacySsoAdd = Effect.fn("legacy.sso.add")(function* (flags: Legacy ), ); } - body["metadata_url"] = flags.metadataUrl.value; + body["metadata_url"] = metadataUrl.value; } - if (Option.isSome(flags.attributeMappingFile)) { - const mapping = yield* readAttributeMapping(flags.attributeMappingFile.value); + if (Option.isSome(attributeMappingFile)) { + const mapping = yield* readAttributeMapping(attributeMappingFile.value); body["attribute_mapping"] = mapping; } - if (flags.domains.length > 0) { - body["domains"] = [...flags.domains]; + if (domains.length > 0) { + body["domains"] = [...domains]; } - if (Option.isSome(flags.nameIdFormat)) { - body["name_id_format"] = flags.nameIdFormat.value; + if (Option.isSome(nameIdFormat)) { + body["name_id_format"] = nameIdFormat.value; } const creating = output.format === "text" ? yield* output.task("Adding SSO provider...") : undefined; - const tokenOpt = yield* resolveLegacyAccessToken; + const tokenOpt = + reconciledTokenCached !== undefined + ? yield* Effect.flatMap(reconciledTokenCached, (resolved) => + Option.isSome(resolved) + ? Effect.succeed(resolved) + : Effect.fail( + new LegacySsoAccessTokenError({ message: legacyMissingAccessTokenMessage() }), + ), + ) + : yield* resolveLegacyAccessToken; // Use `HttpClientRequest.bearerToken(Redacted)` rather than unwrapping the // redacted token into a plain string ourselves — this preserves the // redaction marker on the Authorization header so that any future debug // serialisation of the request stays opaque about the bearer token value. const request = HttpClientRequest.post( - `${cliConfig.apiUrl}/v1/projects/${ref}/config/auth/sso/providers`, + `${apiUrl}/v1/projects/${ref}/config/auth/sso/providers`, ).pipe( Option.isSome(tokenOpt) ? HttpClientRequest.bearerToken(tokenOpt.value) : (req) => req, HttpClientRequest.setHeader("User-Agent", cliConfig.userAgent), @@ -142,6 +373,10 @@ export const legacySsoAdd = Effect.fn("legacy.sso.add")(function* (flags: Legacy featureKey: "auth.saml_2", statusCode: response.status, response, + apiUrl, + ...(yield* Effect.map(reconciledTokenForAux, (token) => + token !== undefined ? { accessToken: token } : {}, + )), }); yield* creating?.fail() ?? Effect.void; if (response.status === 404) { @@ -191,6 +426,16 @@ export const legacySsoAdd = Effect.fn("legacy.sso.add")(function* (flags: Legacy } yield* output.raw(renderSingleProvider(toLegacySsoProviderView(parsedJson))); - }).pipe(Effect.ensuring(linkedProjectCache.cache(ref))); + }).pipe( + // Go's `ensureProjectGroupsCached` GETs `/v1/projects/{ref}` through the + // process-wide `CurrentProfile` — the reconciled host, never the layer's. + Effect.ensuring( + // Resolved INSIDE the ensuring effect — the memoized token read must + // not run before the handler body (Go's gate order, see above). + Effect.flatMap(reconciledTokenForAux, (token) => + linkedProjectCache.cache(ref, undefined, Option.getOrUndefined(profileApiUrl), token), + ), + ), + ); }).pipe(Effect.ensuring(telemetryState.flush)); }); diff --git a/apps/cli/src/legacy/commands/sso/add/add.integration.test.ts b/apps/cli/src/legacy/commands/sso/add/add.integration.test.ts index 5ee8f835cd..4cc0151703 100644 --- a/apps/cli/src/legacy/commands/sso/add/add.integration.test.ts +++ b/apps/cli/src/legacy/commands/sso/add/add.integration.test.ts @@ -2,12 +2,13 @@ import { writeFileSync } from "node:fs"; import { join } from "node:path"; import { describe, expect, it } from "@effect/vitest"; -import { Effect, Exit, Option } from "effect"; +import { Effect, Exit, Layer, Option, Stdio } from "effect"; import * as HttpClientResponse from "effect/unstable/http/HttpClientResponse"; import { mockAnalytics, mockOutput } from "../../../../../tests/helpers/mocks.ts"; import { buildLegacyTestRuntime, + LEGACY_DEFAULT_API_URL, LEGACY_VALID_REF, mockLegacyCliConfig, mockLegacyLinkedProjectCacheTracked, @@ -15,6 +16,7 @@ import { mockLegacyTelemetryStateTracked, useLegacyTempWorkdir, } from "../../../../../tests/helpers/legacy-mocks.ts"; +import { LegacyProfileFlag } from "../../../../shared/legacy/global-flags.ts"; import { EventUpgradeSuggested } from "../../../../shared/telemetry/event-catalog.ts"; import { legacySsoAdd } from "./add.handler.ts"; @@ -39,6 +41,21 @@ interface SetupOpts { upgradeGate?: "gated" | "notGated"; // Metadata-URL fetch responses keyed by URL prefix. metadataUrlResponse?: { status: number; body: string }; + /** + * Raw argv the handler sees via `Stdio.Stdio` — drives the pflag-faithful + * scan (`pflagArgvScan`) behind the required-flag check, the mutex check, + * and the value reconciliation. Defaults to a bare invocation with no optional + * flags present; tests that pass flags must pass matching argv here + * (usually via `cliArgsFor`), exactly as the real parser guarantees. + */ + cliArgs?: ReadonlyArray; + /** + * The Effect-parsed `--profile` value (`LegacyProfileFlag`), which the real + * parser sets for any `--profile` it accepted. Tests whose `cliArgs` carry a + * `--profile` the parser would have consumed must provide it, exactly as the + * real CLI tree would. + */ + profileFlag?: string; } function jsonResponse( @@ -132,15 +149,23 @@ function setup(opts: SetupOpts = {}) { }); const cliConfig = mockLegacyCliConfig({ workdir: tempRoot.current }); - const layer = buildLegacyTestRuntime({ - out, - api: { layer: api.layer, httpClientLayer: api.httpClientLayer }, - cliConfig, - telemetry: telemetry.layer, - linkedProjectCache: cache.layer, - analytics, - goOutput: opts.goOutput === undefined ? Option.none() : Option.some(opts.goOutput), - }); + const layer = Layer.mergeAll( + buildLegacyTestRuntime({ + out, + api: { layer: api.layer, httpClientLayer: api.httpClientLayer }, + cliConfig, + telemetry: telemetry.layer, + linkedProjectCache: cache.layer, + analytics, + goOutput: opts.goOutput === undefined ? Option.none() : Option.some(opts.goOutput), + }), + Stdio.layerTest({ + args: Effect.succeed(opts.cliArgs ?? ["sso", "add", "--type", "saml"]), + }), + opts.profileFlag === undefined + ? Layer.empty + : Layer.succeed(LegacyProfileFlag, opts.profileFlag), + ); return { layer, out, api, analytics, telemetry, cache }; } @@ -161,6 +186,39 @@ const defaultFlags = { >(), }; +/** + * Serializes a flags record into the raw argv the real CLI would have been + * invoked with. The handler reconciles every value it acts on against a + * pflag-faithful scan of this argv, so tests must keep the two consistent — + * a flag passed in the record but absent from argv reconciles to "not set", + * exactly as it would be for a real invocation. + */ +function cliArgsFor(flags: typeof defaultFlags): ReadonlyArray { + const argv: string[] = ["sso", "add", "--type", flags.type]; + if (Option.isSome(flags.projectRef)) { + argv.push("--project-ref", flags.projectRef.value); + } + for (const domain of flags.domains) { + argv.push("--domains", domain); + } + if (Option.isSome(flags.metadataFile)) { + argv.push("--metadata-file", flags.metadataFile.value); + } + if (Option.isSome(flags.metadataUrl)) { + argv.push("--metadata-url", flags.metadataUrl.value); + } + if (flags.skipUrlValidation) { + argv.push("--skip-url-validation"); + } + if (Option.isSome(flags.attributeMappingFile)) { + argv.push("--attribute-mapping-file", flags.attributeMappingFile.value); + } + if (Option.isSome(flags.nameIdFormat)) { + argv.push("--name-id-format", flags.nameIdFormat.value); + } + return argv; +} + describe("legacy sso add integration", () => { it.live("POSTs to /v1/projects/{ref}/config/auth/sso/providers with type=saml", () => { const { layer, api } = setup(); @@ -173,27 +231,617 @@ describe("legacy sso add integration", () => { }).pipe(Effect.provide(layer)); }); - it.live("fails with mutex-flag error when both metadata flags set", () => { - const { layer } = setup(); + it.live( + "mutex check: --metadata-file + --metadata-url fails with cobra's exact error text", + () => { + const { layer } = setup({ + cliArgs: [ + "sso", + "add", + "--type", + "saml", + "--metadata-file", + "/tmp/missing.xml", + "--metadata-url", + "https://idp.example.com/m", + ], + }); + return Effect.gen(function* () { + const exit = yield* Effect.exit( + legacySsoAdd({ + ...defaultFlags, + metadataFile: Option.some("/tmp/missing.xml"), + metadataUrl: Option.some("https://idp.example.com/m"), + }), + ); + expect(Exit.isFailure(exit)).toBe(true); + if (Exit.isFailure(exit)) { + const dump = JSON.stringify(exit.cause); + expect(dump).toContain("LegacySsoMutexFlagError"); + // Byte-matches cobra's `validateExclusiveFlagGroups` template + // (`flag_groups.go:204`): group in Go's registration order + // (`cmd/sso.go:164`), changed flags sorted alphabetically. + expect(dump).toContain( + "if any flags in the group [metadata-file metadata-url] are set none of the others can be; [metadata-file metadata-url] were all set", + ); + } + }).pipe(Effect.provide(layer)); + }, + ); + + it.live( + "mutex check: an explicit but empty --metadata-file= still conflicts with --metadata-url (changed, not truthy)", + () => { + // `--metadata-file=` parses to an empty string, but cobra's + // `pflag.Changed` tracks that the flag was passed at all, not the + // resulting value — the mutex must trip on an explicit empty value, + // and with cobra's exact template, not the old hand-written message. + const { layer } = setup({ + cliArgs: [ + "sso", + "add", + "--type", + "saml", + "--metadata-file=", + "--metadata-url", + "https://idp.example.com/m", + ], + }); + return Effect.gen(function* () { + const exit = yield* Effect.exit( + legacySsoAdd({ + ...defaultFlags, + metadataFile: Option.some(""), + metadataUrl: Option.some("https://idp.example.com/m"), + }), + ); + expect(Exit.isFailure(exit)).toBe(true); + if (Exit.isFailure(exit)) { + const dump = JSON.stringify(exit.cause); + expect(dump).toContain("LegacySsoMutexFlagError"); + expect(dump).toContain( + "if any flags in the group [metadata-file metadata-url] are set none of the others can be; [metadata-file metadata-url] were all set", + ); + } + }).pipe(Effect.provide(layer)); + }, + ); + + it.live( + "mutex check: a bare --metadata-file followed by --metadata-url is not a violation, and the consumed token is the file", + () => { + // pflag's `--flag arg` branch consumes the very next argv token as the + // value unconditionally (`flag.go:1013-1031`), so real cobra parses + // this as `metadata-file` receiving the literal value + // `"--metadata-url"` — `metadata-url` is never parsed as its own flag + // and stays unset. The raw-argv scan must reach the same conclusion: + // no mutex violation, and the handler must then behave exactly like Go + // — try to open a file literally named `--metadata-url` (Go: `failed + // to open metadata file: open --metadata-url: no such file or + // directory`), not silently succeed with no metadata at all. + const { layer, api } = setup({ + cliArgs: ["sso", "add", "--type", "saml", "--metadata-file", "--metadata-url"], + }); + return Effect.gen(function* () { + const exit = yield* Effect.exit(legacySsoAdd(defaultFlags)); + expect(Exit.isFailure(exit)).toBe(true); + if (Exit.isFailure(exit)) { + const dump = JSON.stringify(exit.cause); + expect(dump).toContain("LegacySsoAddMetadataFileError"); + expect(dump).toContain("failed to open metadata file"); + } + expect(api.requests.some((r) => r.method === "POST")).toBe(false); + }).pipe(Effect.provide(layer)); + }, + ); + + it.live( + "reconciles project-ref consuming --metadata-file: fails ref validation like Go, never reads metadata", + () => { + // `sso add --type saml --project-ref --metadata-file file.xml + // --metadata-url URL`: pflag hands `--metadata-file` to `--project-ref` + // as its value, `file.xml` becomes a positional, and only + // `metadata-url` is set — no mutex violation. The Effect parser instead + // drops the bare `--project-ref` and parses both metadata options, so + // without reconciliation the handler would read `file.xml` and POST + // `metadata_xml` — an API call Go never makes: Go fails + // `AssertProjectRefIsValid` on the value `--metadata-file` + // (`internal/utils/flags/project_ref.go:57-59`) before touching + // metadata. The reconciled handler must fail with Go's exact + // invalid-ref error and make no API request. + const { layer, api } = setup({ + cliArgs: [ + "sso", + "add", + "--type", + "saml", + "--project-ref", + "--metadata-file", + "file.xml", + "--metadata-url", + "https://idp.example.com/m", + ], + }); + return Effect.gen(function* () { + const exit = yield* Effect.exit( + legacySsoAdd({ + ...defaultFlags, + metadataFile: Option.some("file.xml"), + metadataUrl: Option.some("https://idp.example.com/m"), + }), + ); + expect(Exit.isFailure(exit)).toBe(true); + if (Exit.isFailure(exit)) { + const dump = JSON.stringify(exit.cause); + expect(dump).toContain("LegacyInvalidProjectRefError"); + expect(dump).toContain("Invalid project ref format. Must be like"); + } + expect(api.requests.some((r) => r.method === "POST")).toBe(false); + }).pipe(Effect.provide(layer)); + }, + ); + + it.live( + "required emulation: a bare --domains consuming --type fails the required-flag check, no POST", + () => { + // `sso add --domains --type saml`: pflag hands `--type` to `--domains` + // as its value and `saml` becomes a positional — `type` is never + // marked changed, so Go fails cobra's `ValidateRequiredFlags` + // (`command.go:1007`, `MarkFlagRequired("type")` at `cmd/sso.go:165`) + // before `RunE` and never POSTs. The Effect parser read `--type saml` + // as a normal flag, so the handler must re-derive the required check + // from the scan (PR #5974 review). + const { layer, api } = setup({ + cliArgs: ["sso", "add", "--domains", "--type", "saml"], + }); + return Effect.gen(function* () { + const exit = yield* Effect.exit(legacySsoAdd(defaultFlags)); + expect(Exit.isFailure(exit)).toBe(true); + if (Exit.isFailure(exit)) { + const dump = JSON.stringify(exit.cause); + expect(dump).toContain("LegacySsoAddRequiredFlagError"); + expect(dump).toContain('required flag(s) \\"type\\" not set'); + } + expect(api.requests.length).toBe(0); + }).pipe(Effect.provide(layer)); + }, + ); + + it.live("required emulation: the required-flag error wins over a mutex violation", () => { + // cobra runs `ValidateRequiredFlags` (`command.go:1007`) before + // `ValidateFlagGroups` (`command.go:1010`), so when `--domains` swallows + // `--type` AND both metadata flags are set, Go reports the required-flag + // error, not the mutex template (binary-verified). + const { layer, api } = setup({ + cliArgs: [ + "sso", + "add", + "--domains", + "--type", + "saml", + "--metadata-file", + "a.xml", + "--metadata-url", + "https://idp.example.com/m", + ], + }); return Effect.gen(function* () { const exit = yield* Effect.exit( legacySsoAdd({ ...defaultFlags, - metadataFile: Option.some("/tmp/missing.xml"), + metadataFile: Option.some("a.xml"), metadataUrl: Option.some("https://idp.example.com/m"), }), ); expect(Exit.isFailure(exit)).toBe(true); if (Exit.isFailure(exit)) { - expect(JSON.stringify(exit.cause)).toContain("LegacySsoMutexFlagError"); + const dump = JSON.stringify(exit.cause); + expect(dump).toContain("LegacySsoAddRequiredFlagError"); + expect(dump).not.toContain("LegacySsoMutexFlagError"); + } + expect(api.requests.length).toBe(0); + }).pipe(Effect.provide(layer)); + }); + + it.live( + "workdir emulation: --workdir consuming --metadata-file fails at Go's chdir, never POSTs", + () => { + // `sso add --type saml --project-ref --workdir --metadata-file + // missing.xml`: pflag binds `"--metadata-file"` to the persistent + // `--workdir` and Go's `ChangeWorkDir` (`cmd/root.go:104`, + // `misc.go:238-257`) exits before `RunE` with zero HTTP traffic. The + // Effect parser refused the flag-shaped value (workdir stayed unset) + // and read `missing.xml` as metadata — without the workdir emulation + // the reconciliation discarded that metadata and POSTed a provider Go + // never creates (binary-verified, PR #5974 review round 6). + const { layer, api } = setup({ + cliArgs: [ + "sso", + "add", + "--type", + "saml", + "--project-ref", + LEGACY_VALID_REF, + "--workdir", + "--metadata-file", + "missing.xml", + ], + }); + return Effect.gen(function* () { + const exit = yield* Effect.exit( + legacySsoAdd({ + ...defaultFlags, + projectRef: Option.some(LEGACY_VALID_REF), + metadataFile: Option.some("missing.xml"), + }), + ); + expect(Exit.isFailure(exit)).toBe(true); + if (Exit.isFailure(exit)) { + const dump = JSON.stringify(exit.cause); + expect(dump).toContain("LegacySsoWorkdirError"); + expect(dump).toContain( + "failed to change workdir: chdir --metadata-file: no such file or directory", + ); + } + expect(api.requests.length).toBe(0); + }).pipe(Effect.provide(layer)); + }, + ); + + it.live( + "workdir emulation: the chdir failure wins over required-type and mutex violations", + () => { + // Go's `ChangeWorkDir` runs from `PersistentPreRunE` (`command.go:986`) + // — before `ValidateRequiredFlags` (`command.go:1007`) and + // `ValidateFlagGroups` (`command.go:1010`) — so a missing workdir beats + // both the missing required `--type` and the metadata mutex + // (binary-verified against apps/cli-go, PR #5974 review round 6). + const { layer, api } = setup({ + cliArgs: [ + "sso", + "add", + "--workdir", + "/nonexistent-sso-add-workdir", + "--metadata-file", + "a.xml", + "--metadata-url", + "https://idp.example.com/m", + ], + }); + return Effect.gen(function* () { + const exit = yield* Effect.exit( + legacySsoAdd({ + ...defaultFlags, + metadataFile: Option.some("a.xml"), + metadataUrl: Option.some("https://idp.example.com/m"), + }), + ); + expect(Exit.isFailure(exit)).toBe(true); + if (Exit.isFailure(exit)) { + const dump = JSON.stringify(exit.cause); + expect(dump).toContain("LegacySsoWorkdirError"); + expect(dump).toContain( + "failed to change workdir: chdir /nonexistent-sso-add-workdir: no such file or directory", + ); + expect(dump).not.toContain("LegacySsoAddRequiredFlagError"); + expect(dump).not.toContain("LegacySsoMutexFlagError"); + } + expect(api.requests.length).toBe(0); + }).pipe(Effect.provide(layer)); + }, + ); + + it.live("workdir emulation: an existing --workdir directory proceeds to the POST", () => { + // Go chdir's into an existing workdir and continues to `RunE` — the + // emulation must only reject what `os.Chdir` would reject. + const { layer, api } = setup({ + cliArgs: ["sso", "add", "--type", "saml", "--workdir", tempRoot.current], + }); + return Effect.gen(function* () { + yield* legacySsoAdd(defaultFlags); + const req = api.requests.find((r) => r.method === "POST"); + expect((req?.body as { type?: string })?.type).toBe("saml"); + }).pipe(Effect.provide(layer)); + }); + + it.live("required emulation: a -t shorthand invocation POSTs normally", () => { + // The scan resolves `-t saml` to a `type` occurrence via the shorthand + // map (`cmd/sso.go:157` registers `VarP`), so a genuine shorthand + // invocation must never trip the emulated required-flag check. + const { layer, api } = setup({ + cliArgs: ["sso", "add", "-t", "saml"], + }); + return Effect.gen(function* () { + yield* legacySsoAdd(defaultFlags); + const req = api.requests.find((r) => r.method === "POST"); + expect((req?.body as { type?: string })?.type).toBe("saml"); + }).pipe(Effect.provide(layer)); + }); + + it.live( + "required emulation: -t saml plus a consumed --type still POSTs, like pflag (type IS changed)", + () => { + // `-t saml --domains --type saml`: pflag sets `type` via the shorthand + // first, then `--domains` swallows the long `--type` token — the flag + // is changed, so Go proceeds and POSTs with `domains: ["--type"]`. + const { layer, api } = setup({ + cliArgs: ["sso", "add", "-t", "saml", "--domains", "--type", "saml"], + }); + return Effect.gen(function* () { + yield* legacySsoAdd(defaultFlags); + const req = api.requests.find((r) => r.method === "POST"); + expect(req).toBeDefined(); + const body = req?.body as { type?: string; domains?: string[] }; + expect(body?.type).toBe("saml"); + expect(body?.domains).toEqual(["--type"]); + }).pipe(Effect.provide(layer)); + }, + ); + + it.live( + "required emulation: a bare --domains consuming -t fails the required-flag check, no POST", + () => { + // Binary-verified: `sso add --domains -t saml` (and `-t=saml`) fails + // Go's required-flag check — pflag hands the `-t` token to `--domains` + // as its value, so `type` is never marked changed and `saml` becomes a + // positional (Go's add command has no Args validation and accepts it). + // The Effect parser read `-t saml` as a normal flag, so without the + // scan's consumed-shorthand tracking the handler would POST + // `type: "saml"`, `domains: ["-t"]` (PR #5974 review round 3). + const { layer, api } = setup({ + cliArgs: ["sso", "add", "--domains", "-t", "saml"], + }); + return Effect.gen(function* () { + const exit = yield* Effect.exit(legacySsoAdd(defaultFlags)); + expect(Exit.isFailure(exit)).toBe(true); + if (Exit.isFailure(exit)) { + const dump = JSON.stringify(exit.cause); + expect(dump).toContain("LegacySsoAddRequiredFlagError"); + expect(dump).toContain('required flag(s) \\"type\\" not set'); + } + expect(api.requests.length).toBe(0); + }).pipe(Effect.provide(layer)); + }, + ); + + it.live( + "invalid-value emulation: a later invalid --type occurrence fails with pflag's shorthand-labelled error, no POST", + () => { + // `--type saml --type bogus`: the Effect parser resolves repeats + // first-wins and never validates the rest, so it parses; pflag Sets + // every occurrence in order and rejects `bogus` at ParseFlags — + // before every hook, the required-flag check, and the POST + // (binary-verified, PR #5974 review round 4). pflag names the flag + // with its shorthand (`-t, --type`, errors.go:39-41). + const { layer, api } = setup({ + cliArgs: ["sso", "add", "--type", "saml", "--type", "bogus"], + }); + return Effect.gen(function* () { + const exit = yield* Effect.exit(legacySsoAdd(defaultFlags)); + expect(Exit.isFailure(exit)).toBe(true); + if (Exit.isFailure(exit)) { + const dump = JSON.stringify(exit.cause); + expect(dump).toContain("LegacySsoInvalidFlagValueError"); + expect(dump).toContain( + 'invalid argument \\"bogus\\" for \\"-t, --type\\" flag: must be one of [ saml ]', + ); + } + expect(api.requests.length).toBe(0); + }).pipe(Effect.provide(layer)); + }, + ); + + it.live( + "invalid-value emulation: a later inline-empty --skip-url-validation= fails like pflag, no POST", + () => { + // `--skip-url-validation=false --skip-url-validation=`: the Effect + // parser resolves repeats first-wins and never validates the second + // occurrence, so it parses; pflag hands `""` to strconv.ParseBool + // (`flag.go:1014-1016`) and aborts ParseFlags before every hook and + // the POST — only a *bare* repeat means NoOptDefVal true + // (binary-verified, PR #5974 review round 5). + const { layer, api } = setup({ + cliArgs: [ + "sso", + "add", + "--type", + "saml", + "--skip-url-validation=false", + "--skip-url-validation=", + "--metadata-url", + "https://idp.example.com/m", + ], + }); + return Effect.gen(function* () { + const exit = yield* Effect.exit( + legacySsoAdd({ + ...defaultFlags, + skipUrlValidation: false, // Effect's first-wins parse + metadataUrl: Option.some("https://idp.example.com/m"), + }), + ); + expect(Exit.isFailure(exit)).toBe(true); + if (Exit.isFailure(exit)) { + const dump = JSON.stringify(exit.cause); + expect(dump).toContain("LegacySsoInvalidFlagValueError"); + expect(dump).toContain( + 'invalid argument \\"\\" for \\"--skip-url-validation\\" flag: strconv.ParseBool: parsing \\"\\": invalid syntax', + ); + } + expect(api.requests.length).toBe(0); + }).pipe(Effect.provide(layer)); + }, + ); + + it.live( + "value reconciliation: repeated --skip-url-validation resolves last-wins like pflag and skips validation", + () => { + // `--skip-url-validation=false --skip-url-validation` ends true for + // pflag (Sets every occurrence) but false for the Effect parser + // (first-wins) — Go skips URL validation and POSTs the non-HTTPS URL + // (binary-verified, PR #5974 review round 4). + const { layer, api } = setup({ + cliArgs: [ + "sso", + "add", + "--type", + "saml", + "--skip-url-validation=false", + "--skip-url-validation", + "--metadata-url", + "http://insecure.example.com/md", + ], + }); + return Effect.gen(function* () { + yield* legacySsoAdd({ + ...defaultFlags, + skipUrlValidation: false, // Effect's first-wins parse + metadataUrl: Option.some("http://insecure.example.com/md"), + }); + const req = api.requests.find((r) => r.method === "POST"); + expect((req?.body as { metadata_url?: string })?.metadata_url).toBe( + "http://insecure.example.com/md", + ); + }).pipe(Effect.provide(layer)); + }, + ); + + it.live( + "value reconciliation: repeated --name-id-format resolves last-wins like pflag in the POST body", + () => { + // pflag's Set runs per occurrence, so the last one wins; the Effect + // parser resolved first-wins (binary-verified, PR #5974 review + // round 4). + const transient = "urn:oasis:names:tc:SAML:2.0:nameid-format:transient" as const; + const persistent = "urn:oasis:names:tc:SAML:2.0:nameid-format:persistent"; + const { layer, api } = setup({ + cliArgs: [ + "sso", + "add", + "--type", + "saml", + `--name-id-format=${transient}`, + `--name-id-format=${persistent}`, + ], + }); + return Effect.gen(function* () { + yield* legacySsoAdd({ + ...defaultFlags, + nameIdFormat: Option.some(transient), // Effect's first-wins parse + }); + const req = api.requests.find((r) => r.method === "POST"); + expect((req?.body as { name_id_format?: string })?.name_id_format).toBe(persistent); + }).pipe(Effect.provide(layer)); + }, + ); + + it.live("missing-value emulation: a trailing bare --domains fails pflag parse, no POST", () => { + // Binary-verified: `sso add --type saml --domains` errors + // `flag needs an argument: --domains` — pflag fails `ParseFlags` (cobra + // `command.go:919`) before the required-flag and mutex validations and + // Go never POSTs. The Effect parser accepts the argv (the flag parses + // as unset), so the handler must reject it before any side effect + // (PR #5974 review round 3). + const { layer, api } = setup({ + cliArgs: ["sso", "add", "--type", "saml", "--domains"], + }); + return Effect.gen(function* () { + const exit = yield* Effect.exit(legacySsoAdd(defaultFlags)); + expect(Exit.isFailure(exit)).toBe(true); + if (Exit.isFailure(exit)) { + const dump = JSON.stringify(exit.cause); + expect(dump).toContain("LegacySsoFlagNeedsArgumentError"); + expect(dump).toContain("flag needs an argument: --domains"); } + expect(api.requests.length).toBe(0); + }).pipe(Effect.provide(layer)); + }); + + it.live( + "reconciles a bare --domains consuming --metadata-file: POSTs the domain pflag saw, no metadata", + () => { + // `--domains --metadata-file x.xml`: pflag appends the literal string + // `--metadata-file` to the domains slice and `x.xml` becomes a + // positional — `metadata-file` is never set. The Effect parser drops + // the bare `--domains` and parses `--metadata-file x.xml` instead, so + // without reconciliation the request body would carry `metadata_xml` + // and no domains — the opposite of Go's body. + const { layer, api } = setup({ + cliArgs: ["sso", "add", "--type", "saml", "--domains", "--metadata-file", "x.xml"], + }); + return Effect.gen(function* () { + yield* legacySsoAdd({ ...defaultFlags, metadataFile: Option.some("x.xml") }); + const req = api.requests.find((r) => r.method === "POST"); + expect(req).toBeDefined(); + const body = req?.body as { domains?: string[]; metadata_xml?: string }; + expect(body?.domains).toEqual(["--metadata-file"]); + expect(body?.metadata_xml).toBeUndefined(); + }).pipe(Effect.provide(layer)); + }, + ); + + it.live( + "reconciles a bare --metadata-url consuming --name-id-format: validates the consumed token as the URL", + () => { + // `--metadata-url --name-id-format urn:…`: pflag hands + // `--name-id-format` to `--metadata-url` as its value and the urn + // becomes a positional — `name-id-format` is never set. Go then fails + // URL validation on the literal string `--name-id-format`; the body + // must not pick up the parsed name-id-format either. + const { layer, api } = setup({ + cliArgs: [ + "sso", + "add", + "--type", + "saml", + "--metadata-url", + "--name-id-format", + "urn:oasis:names:tc:SAML:2.0:nameid-format:persistent", + ], + }); + return Effect.gen(function* () { + const exit = yield* Effect.exit( + legacySsoAdd({ + ...defaultFlags, + nameIdFormat: Option.some("urn:oasis:names:tc:SAML:2.0:nameid-format:persistent"), + }), + ); + expect(Exit.isFailure(exit)).toBe(true); + if (Exit.isFailure(exit)) { + const dump = JSON.stringify(exit.cause); + // URL validation runs against the consumed token, not the parsed + // Option — `--name-id-format` is not a valid HTTPS URL, so the + // command fails before any request, like Go. + expect(dump).toContain("LegacySsoAddMetadataFileError"); + expect(dump).toContain("--name-id-format"); + expect(dump).toContain("Use --skip-url-validation to suppress this error"); + } + expect(api.requests.some((r) => r.method === "POST")).toBe(false); + }).pipe(Effect.provide(layer)); + }, + ); + + it.live("falls back to the parsed domains when the scan's raw values are malformed CSV", () => { + // Unreachable through the real CLI (the parser rejects malformed CSV at + // parse time), but the reconciliation must not crash if the consumed + // token is un-parseable — it keeps the parser's values instead. + const { layer, api } = setup({ + cliArgs: ["sso", "add", "--type", "saml", "--domains", '--x"y'], + }); + return Effect.gen(function* () { + yield* legacySsoAdd({ ...defaultFlags, domains: ["fallback.example.com"] }); + const req = api.requests.find((r) => r.method === "POST"); + expect((req?.body as { domains?: string[] })?.domains).toEqual(["fallback.example.com"]); }).pipe(Effect.provide(layer)); }); it.live("reads metadata file and sends as metadata_xml", () => { const path = join(tempRoot.current, "good.xml"); writeFileSync(path, ''); - const { layer, api } = setup(); + // A single metadata flag on the raw argv must sail through the mutex scan. + const { layer, api } = setup({ + cliArgs: ["sso", "add", "--type", "saml", "--metadata-file", path], + }); return Effect.gen(function* () { yield* legacySsoAdd({ ...defaultFlags, metadataFile: Option.some(path) }); const req = api.requests.find((r) => r.method === "POST"); @@ -204,11 +852,10 @@ describe("legacy sso add integration", () => { it.live("rejects non-UTF8 metadata file", () => { const path = join(tempRoot.current, "bad.xml"); writeFileSync(path, Buffer.from([0xff, 0xfe, 0xfd])); - const { layer } = setup(); + const flags = { ...defaultFlags, metadataFile: Option.some(path) }; + const { layer } = setup({ cliArgs: cliArgsFor(flags) }); return Effect.gen(function* () { - const exit = yield* Effect.exit( - legacySsoAdd({ ...defaultFlags, metadataFile: Option.some(path) }), - ); + const exit = yield* Effect.exit(legacySsoAdd(flags)); expect(Exit.isFailure(exit)).toBe(true); if (Exit.isFailure(exit)) { expect(JSON.stringify(exit.cause)).toContain("LegacySsoAddMetadataFileError"); @@ -217,7 +864,18 @@ describe("legacy sso add integration", () => { }); it.live("sends metadata_url verbatim when --skip-url-validation", () => { - const { layer, api } = setup(); + // A single metadata flag on the raw argv must sail through the mutex scan. + const { layer, api } = setup({ + cliArgs: [ + "sso", + "add", + "--type", + "saml", + "--metadata-url", + "https://idp.example.com/m", + "--skip-url-validation", + ], + }); return Effect.gen(function* () { yield* legacySsoAdd({ ...defaultFlags, @@ -232,15 +890,17 @@ describe("legacy sso add integration", () => { }); it.live("validates HTTPS metadata URL when not skipped — success path", () => { + const flags = { + ...defaultFlags, + metadataUrl: Option.some("https://idp.example.com/m"), + skipUrlValidation: false, + }; const { layer, api } = setup({ metadataUrlResponse: { status: 200, body: '' }, + cliArgs: cliArgsFor(flags), }); return Effect.gen(function* () { - yield* legacySsoAdd({ - ...defaultFlags, - metadataUrl: Option.some("https://idp.example.com/m"), - skipUrlValidation: false, - }); + yield* legacySsoAdd(flags); const req = api.requests.find((r) => r.method === "POST"); expect((req?.body as { metadata_url?: string })?.metadata_url).toBe( "https://idp.example.com/m", @@ -249,15 +909,14 @@ describe("legacy sso add integration", () => { }); it.live("rejects non-HTTPS metadata URL with Go-format message", () => { - const { layer } = setup(); + const flags = { + ...defaultFlags, + metadataUrl: Option.some("http://idp.example.com/m"), + skipUrlValidation: false, + }; + const { layer } = setup({ cliArgs: cliArgsFor(flags) }); return Effect.gen(function* () { - const exit = yield* Effect.exit( - legacySsoAdd({ - ...defaultFlags, - metadataUrl: Option.some("http://idp.example.com/m"), - skipUrlValidation: false, - }), - ); + const exit = yield* Effect.exit(legacySsoAdd(flags)); expect(Exit.isFailure(exit)).toBe(true); if (Exit.isFailure(exit)) { const dump = JSON.stringify(exit.cause); @@ -270,9 +929,10 @@ describe("legacy sso add integration", () => { it.live("reads attribute mapping JSON and preserves user-defined `default` field", () => { const path = join(tempRoot.current, "mapping.json"); writeFileSync(path, JSON.stringify({ keys: { a: { default: 3 } } })); - const { layer, api } = setup(); + const flags = { ...defaultFlags, attributeMappingFile: Option.some(path) }; + const { layer, api } = setup({ cliArgs: cliArgsFor(flags) }); return Effect.gen(function* () { - yield* legacySsoAdd({ ...defaultFlags, attributeMappingFile: Option.some(path) }); + yield* legacySsoAdd(flags); const req = api.requests.find((r) => r.method === "POST"); const mapping = (req?.body as { attribute_mapping?: { keys: { a: { default: number } } } }) ?.attribute_mapping; @@ -281,9 +941,10 @@ describe("legacy sso add integration", () => { }); it.live("sends domains array verbatim", () => { - const { layer, api } = setup(); + const flags = { ...defaultFlags, domains: ["a.com", "b.com"] }; + const { layer, api } = setup({ cliArgs: cliArgsFor(flags) }); return Effect.gen(function* () { - yield* legacySsoAdd({ ...defaultFlags, domains: ["a.com", "b.com"] }); + yield* legacySsoAdd(flags); const req = api.requests.find((r) => r.method === "POST"); expect((req?.body as { domains?: string[] })?.domains).toEqual(["a.com", "b.com"]); }).pipe(Effect.provide(layer)); @@ -382,9 +1043,10 @@ describe("legacy sso add integration", () => { it.live("preserves attribute_mapping `default` field in POST body", () => { const path = join(tempRoot.current, "mapping.json"); writeFileSync(path, JSON.stringify({ keys: { a: { default: 42 } } })); - const { layer, api } = setup(); + const flags = { ...defaultFlags, attributeMappingFile: Option.some(path) }; + const { layer, api } = setup({ cliArgs: cliArgsFor(flags) }); return Effect.gen(function* () { - yield* legacySsoAdd({ ...defaultFlags, attributeMappingFile: Option.some(path) }); + yield* legacySsoAdd(flags); const req = api.requests.find((r) => r.method === "POST"); const mapping = (req?.body as { attribute_mapping?: { keys: { a: { default: number } } } }) ?.attribute_mapping; @@ -393,15 +1055,17 @@ describe("legacy sso add integration", () => { }); it.live("metadata URL fetch failure surfaces as add metadata file error", () => { - const { layer } = setup({ metadataUrlResponse: { status: 503, body: "" } }); + const flags = { + ...defaultFlags, + metadataUrl: Option.some("https://idp.example.com/m"), + skipUrlValidation: false, + }; + const { layer } = setup({ + metadataUrlResponse: { status: 503, body: "" }, + cliArgs: cliArgsFor(flags), + }); return Effect.gen(function* () { - const exit = yield* Effect.exit( - legacySsoAdd({ - ...defaultFlags, - metadataUrl: Option.some("https://idp.example.com/m"), - skipUrlValidation: false, - }), - ); + const exit = yield* Effect.exit(legacySsoAdd(flags)); expect(Exit.isFailure(exit)).toBe(true); if (Exit.isFailure(exit)) { const dump = JSON.stringify(exit.cause); @@ -419,15 +1083,14 @@ describe("legacy sso add integration", () => { // invalid sequence cannot be expressed without bypassing the Response API. it.live("malformed metadata URL surfaces invalid URI error", () => { - const { layer } = setup(); + const flags = { + ...defaultFlags, + metadataUrl: Option.some("::::not a url::::"), + skipUrlValidation: false, + }; + const { layer } = setup({ cliArgs: cliArgsFor(flags) }); return Effect.gen(function* () { - const exit = yield* Effect.exit( - legacySsoAdd({ - ...defaultFlags, - metadataUrl: Option.some("::::not a url::::"), - skipUrlValidation: false, - }), - ); + const exit = yield* Effect.exit(legacySsoAdd(flags)); expect(Exit.isFailure(exit)).toBe(true); if (Exit.isFailure(exit)) { expect(JSON.stringify(exit.cause)).toContain("LegacySsoAddMetadataFileError"); @@ -436,12 +1099,13 @@ describe("legacy sso add integration", () => { }); it.live("nameIdFormat is forwarded in the request body when provided", () => { - const { layer, api } = setup(); + const flags = { + ...defaultFlags, + nameIdFormat: Option.some("urn:oasis:names:tc:SAML:2.0:nameid-format:persistent" as const), + }; + const { layer, api } = setup({ cliArgs: cliArgsFor(flags) }); return Effect.gen(function* () { - yield* legacySsoAdd({ - ...defaultFlags, - nameIdFormat: Option.some("urn:oasis:names:tc:SAML:2.0:nameid-format:persistent"), - }); + yield* legacySsoAdd(flags); const req = api.requests.find((r) => r.method === "POST"); expect((req?.body as { name_id_format?: string })?.name_id_format).toBe( "urn:oasis:names:tc:SAML:2.0:nameid-format:persistent", @@ -452,15 +1116,211 @@ describe("legacy sso add integration", () => { it.live("attribute mapping parse failure surfaces a tagged error", () => { const path = join(tempRoot.current, "malformed.json"); writeFileSync(path, "{not json}"); - const { layer } = setup(); + const flags = { ...defaultFlags, attributeMappingFile: Option.some(path) }; + const { layer } = setup({ cliArgs: cliArgsFor(flags) }); return Effect.gen(function* () { - const exit = yield* Effect.exit( - legacySsoAdd({ ...defaultFlags, attributeMappingFile: Option.some(path) }), - ); + const exit = yield* Effect.exit(legacySsoAdd(flags)); expect(Exit.isFailure(exit)).toBe(true); if (Exit.isFailure(exit)) { expect(JSON.stringify(exit.cause)).toContain("LegacySsoAddAttributeMappingFileError"); } }).pipe(Effect.provide(layer)); }); + + // ------------------------------------------------------------------------- + // Profile emulation (PR #5974 review round 7): Go's `LoadProfile` runs from + // the root `PersistentPreRunE` (`cmd/root.go:98-102`) on the pflag/viper- + // effective `--profile`/`SUPABASE_PROFILE`, immediately before + // `ChangeWorkDir` — it decides which API host receives the POST and aborts + // the command when the profile cannot be loaded. + // ------------------------------------------------------------------------- + + const writeProfileYaml = (name: string, apiUrl: string): string => { + const path = join(tempRoot.current, name); + writeFileSync( + path, + [ + `name: ${name.replace(/\.[^.]*$/, "")}`, + `api_url: ${apiUrl}`, + `dashboard_url: ${apiUrl}/dashboard`, + "project_host: supabase.co", + ].join("\n"), + ); + return path; + }; + + const withProfileEnv = (value: string | undefined) => { + const previous = process.env["SUPABASE_PROFILE"]; + if (value === undefined) { + delete process.env["SUPABASE_PROFILE"]; + } else { + process.env["SUPABASE_PROFILE"] = value; + } + return Effect.sync(() => { + if (previous === undefined) { + delete process.env["SUPABASE_PROFILE"]; + } else { + process.env["SUPABASE_PROFILE"] = previous; + } + }); + }; + + it.live( + "profile emulation: --domains consuming --profile POSTs to the env profile's host, not the parsed file's", + () => { + // `sso add --type saml --domains --profile alternate.yml`: pflag hands + // `--profile` to `--domains` and never marks profile changed, so viper + // falls to SUPABASE_PROFILE — while the Effect parser read + // `alternate.yml` as the profile and built `LegacyCliConfig` from it. + // Binary-verified (the demonstrated divergent input, PR #5974 round 7): + // Go POSTs `{"domains":["--profile"],"type":"saml"}` to the env + // profile's api_url; the parsed file's host receives nothing. + const envProfile = writeProfileYaml("env-profile.yml", "http://reconciled.example"); + const alternate = writeProfileYaml("alternate.yml", "http://alternate.example"); + const restoreEnv = withProfileEnv(envProfile); + const { layer, api, cache } = setup({ + cliArgs: ["sso", "add", "--type", "saml", "--domains", "--profile", alternate], + profileFlag: alternate, + }); + return Effect.gen(function* () { + yield* legacySsoAdd(defaultFlags); + const posts = api.requests.filter((r) => r.method === "POST"); + expect(posts.length).toBe(1); + expect(posts[0]?.url).toBe( + `http://reconciled.example/v1/projects/${LEGACY_VALID_REF}/config/auth/sso/providers`, + ); + expect((posts[0]?.body as { domains?: ReadonlyArray })?.domains).toEqual([ + "--profile", + ]); + // The linked-project cache fill targets the reconciled host too + // (Go's ensureProjectGroupsCached uses the process-wide profile). + expect(cache.cachedApiUrl).toBe("http://reconciled.example"); + }).pipe(Effect.ensuring(restoreEnv), Effect.provide(layer)); + }, + ); + + it.live( + "profile emulation: --profile consuming a flag-shaped token fails LoadProfile, never POSTs", + () => { + // `sso add --type saml --profile --metadata-url u`: pflag binds + // `"--metadata-url"` as the profile value; viper's extension gate + // rejects it before any request (binary-verified: `failed to read + // profile: Unsupported Config Type ""`). + const restoreEnv = withProfileEnv(undefined); + const { layer, api } = setup({ + cliArgs: [ + "sso", + "add", + "--type", + "saml", + "--profile", + "--metadata-url", + "https://idp.example.com/m", + ], + }); + return Effect.gen(function* () { + const exit = yield* Effect.exit( + legacySsoAdd({ + ...defaultFlags, + metadataUrl: Option.some("https://idp.example.com/m"), + }), + ); + expect(Exit.isFailure(exit)).toBe(true); + if (Exit.isFailure(exit)) { + const dump = JSON.stringify(exit.cause); + expect(dump).toContain("LegacySsoProfileError"); + expect(dump).toContain(`failed to read profile: Unsupported Config Type \\"\\"`); + } + expect(api.requests.length).toBe(0); + }).pipe(Effect.ensuring(restoreEnv), Effect.provide(layer)); + }, + ); + + it.live("profile emulation: repeated --profile resolves last-wins, matching pflag", () => { + // `--profile a.yml --profile b.yml`: the Effect parser is first-wins (the + // config layer resolved a.yml) while pflag Sets every occurrence and ends + // on b.yml — binary-verified: Go POSTs to b.yml's api_url. + const first = writeProfileYaml("first.yml", "http://first.example"); + const second = writeProfileYaml("second.yml", "http://second.example"); + const restoreEnv = withProfileEnv(undefined); + const { layer, api } = setup({ + cliArgs: ["sso", "add", "--type", "saml", "--profile", first, "--profile", second], + profileFlag: first, + }); + return Effect.gen(function* () { + yield* legacySsoAdd(defaultFlags); + const posts = api.requests.filter((r) => r.method === "POST"); + expect(posts.length).toBe(1); + expect(posts[0]?.url).toBe( + `http://second.example/v1/projects/${LEGACY_VALID_REF}/config/auth/sso/providers`, + ); + }).pipe(Effect.ensuring(restoreEnv), Effect.provide(layer)); + }); + + it.live( + "profile emulation: the LoadProfile failure wins over the workdir, required-type, and mutex checks", + () => { + // Go loads the profile BEFORE ChangeWorkDir (`cmd/root.go:98-105` — + // "Load profile before changing workdir"), and both run before + // `ValidateRequiredFlags` and `ValidateFlagGroups`. + const restoreEnv = withProfileEnv(undefined); + const { layer, api } = setup({ + cliArgs: [ + "sso", + "add", + "--profile", + "--metadata-url", + "https://idp.example.com/m", + "--metadata-file", + "a.xml", + "--workdir", + "/nonexistent-sso-add-workdir", + ], + }); + return Effect.gen(function* () { + const exit = yield* Effect.exit( + legacySsoAdd({ + ...defaultFlags, + metadataUrl: Option.some("https://idp.example.com/m"), + metadataFile: Option.some("a.xml"), + }), + ); + expect(Exit.isFailure(exit)).toBe(true); + if (Exit.isFailure(exit)) { + const dump = JSON.stringify(exit.cause); + expect(dump).toContain("LegacySsoProfileError"); + expect(dump).not.toContain("LegacySsoWorkdirError"); + expect(dump).not.toContain("LegacySsoAddRequiredFlagError"); + expect(dump).not.toContain("LegacySsoMutexFlagError"); + } + expect(api.requests.length).toBe(0); + }).pipe(Effect.ensuring(restoreEnv), Effect.provide(layer)); + }, + ); + + it.live( + "profile emulation: an agreeing --profile keeps the config layer's resolution (no override)", + () => { + // When the scan and the parser saw the same token (every normal + // invocation), the reconciliation resolves to `none` and the POST + // targets `LegacyCliConfig.apiUrl` — the layer already loaded exactly + // the profile Go would. + const agreed = writeProfileYaml("agreed.yml", "http://agreed.example"); + const restoreEnv = withProfileEnv(undefined); + const { layer, api } = setup({ + cliArgs: ["sso", "add", "--type", "saml", "--profile", agreed], + profileFlag: agreed, + }); + return Effect.gen(function* () { + yield* legacySsoAdd(defaultFlags); + const posts = api.requests.filter((r) => r.method === "POST"); + expect(posts.length).toBe(1); + // The mock layer's apiUrl, NOT agreed.yml's — the layer is authoritative + // when there is no scan/parser disagreement. + expect(posts[0]?.url).toBe( + `${LEGACY_DEFAULT_API_URL}/v1/projects/${LEGACY_VALID_REF}/config/auth/sso/providers`, + ); + }).pipe(Effect.ensuring(restoreEnv), Effect.provide(layer)); + }, + ); }); diff --git a/apps/cli/src/legacy/commands/sso/sso.errors.ts b/apps/cli/src/legacy/commands/sso/sso.errors.ts index 55630760c0..375bfa2eaa 100644 --- a/apps/cli/src/legacy/commands/sso/sso.errors.ts +++ b/apps/cli/src/legacy/commands/sso/sso.errors.ts @@ -63,6 +63,68 @@ export class LegacySsoMutexFlagError extends Data.TaggedError("LegacySsoMutexFla readonly message: string; }> {} +// pflag's `ValueRequiredError` (`errors.go:63-78`), emulated for the case the +// Effect parser accepts but pflag rejects: a bare value-taking flag as the +// final argv token (`sso update --domains`). pflag fails `ParseFlags` +// (cobra `command.go:919`) before `ValidateArgs`, every hook, and `RunE`, so +// Go exits without any API call. Shared across add + update; message +// byte-matches pflag's template. +export class LegacySsoFlagNeedsArgumentError extends Data.TaggedError( + "LegacySsoFlagNeedsArgumentError", +)<{ + readonly message: string; +}> {} + +// pflag's `InvalidValueError` (`errors.go:32-48`, raised when a flag's +// `Value.Set` rejects an occurrence), emulated for values the Effect parser +// accepts but pflag does not: a repeated flag whose later occurrence is +// invalid (the Effect parser resolves repeats first-wins and never validates +// the rest — `--type saml --type bogus`), and boolean literals outside Go's +// `strconv.ParseBool` set (`--skip-url-validation=yes`). pflag fails +// `ParseFlags` (cobra `command.go:919`) before `ValidateArgs`, every hook, +// and `RunE`, so Go exits without any API call. Shared across add + update; +// message byte-matches pflag's template. +export class LegacySsoInvalidFlagValueError extends Data.TaggedError( + "LegacySsoInvalidFlagValueError", +)<{ + readonly message: string; +}> {} + +// cobra's `ValidateRequiredFlags` (`command.go:1007`), emulated for the case +// the Effect parser cannot see: pflag consumed the required flag's own token +// as another flag's value, so pflag never marks it `Changed` and Go exits +// before `RunE` (CLI-1982). Message byte-matches cobra's template. +export class LegacySsoAddRequiredFlagError extends Data.TaggedError( + "LegacySsoAddRequiredFlagError", +)<{ + readonly message: string; +}> {} + +// Go's `ChangeWorkDir` (`internal/utils/misc.go:238-257`), run from the root +// `PersistentPreRunE` (`cmd/root.go:104`) — after `ParseFlags` and +// `ValidateArgs`, before `ValidateRequiredFlags`, `ValidateFlagGroups`, and +// `RunE` — so a missing workdir directory aborts with no API call ever made. +// Emulated for the pflag/viper-effective `--workdir`/`SUPABASE_WORKDIR` the +// Effect layer never validates (and, when `--workdir` consumed a flag-shaped +// token, never even saw — PR #5974 review round 6). Shared across add + +// update; message byte-matches Go's template. +export class LegacySsoWorkdirError extends Data.TaggedError("LegacySsoWorkdirError")<{ + readonly message: string; +}> {} + +// Go's `LoadProfile` (`internal/utils/profile.go:94-118`), run from the root +// `PersistentPreRunE` (`cmd/root.go:98-102`) immediately BEFORE +// `ChangeWorkDir` — so a profile Go cannot load aborts before the workdir +// check, `ValidateRequiredFlags`, `ValidateFlagGroups`, and `RunE`, with no +// API call ever made. Emulated for the pflag/viper-effective `--profile`/ +// `SUPABASE_PROFILE` whenever it differs from the token the Effect config +// layer resolved (PR #5974 review round 7). Shared across add + update; +// message byte-matches Go for the deterministic failure classes (see +// `sso.load-profile.ts`). +export class LegacySsoProfileError extends Data.TaggedError("LegacySsoProfileError")<{ + readonly message: string; +}> {} + // Shared across add + update — metadata URL validation. export class LegacySsoMetadataUrlInvalidError extends Data.TaggedError( "LegacySsoMetadataUrlInvalidError", @@ -106,6 +168,15 @@ export class LegacySsoShowEnvNotSupportedError extends Data.TaggedError( }> {} // `sso update` +// cobra's `ValidateArgs` / `ExactArgs(1)` (`command.go:968`, `cmd/sso.go:87`), +// emulated for the case the Effect parser cannot see: pflag consumed a flag +// token as a value, shifting what the parser read as a flag's value into the +// positional list, so Go rejects the arg count before any hook or request +// (CLI-1982). Message byte-matches cobra's `ExactArgs` template. +export class LegacySsoUpdateArityError extends Data.TaggedError("LegacySsoUpdateArityError")<{ + readonly message: string; +}> {} + export class LegacySsoUpdateNetworkError extends Data.TaggedError("LegacySsoUpdateNetworkError")<{ readonly message: string; }> {} @@ -150,3 +221,13 @@ export class LegacySsoRemoveUnexpectedStatusError extends Data.TaggedError( readonly body: string; readonly message: string; }> {} + +/** + * Go's `GetSupabase` token gate (`internal/utils/api.go:119-124`): + * `log.Fatalln(utils.ErrMissingToken)` when the reconciled profile's token + * lookup finds nothing — fired at first client use inside `RunE`, AFTER + * required/mutex/workdir validation (PR #5974 review round 10). + */ +export class LegacySsoAccessTokenError extends Data.TaggedError("LegacySsoAccessTokenError")<{ + readonly message: string; +}> {} diff --git a/apps/cli/src/legacy/commands/sso/sso.load-profile.ts b/apps/cli/src/legacy/commands/sso/sso.load-profile.ts new file mode 100644 index 0000000000..8bd8998ddc --- /dev/null +++ b/apps/cli/src/legacy/commands/sso/sso.load-profile.ts @@ -0,0 +1,337 @@ +import { Effect, FileSystem } from "effect"; +import { parse as parseYaml } from "yaml"; + +import { legacyApiUrl, legacyIsBuiltinProfileName } from "../../shared/legacy-profile.ts"; +import { LegacySsoProfileError } from "./sso.errors.ts"; + +/** + * Emulates Go's `LoadProfile` (`apps/cli-go/internal/utils/profile.go:94-118`) + * for a viper-effective profile token, returning the profile's API URL or + * failing exactly where — and, for the deterministic classes, byte-for-byte + * how — the Go binary fails. Go runs this from the root `PersistentPreRunE` + * (`cmd/root.go:98-102`) BEFORE `ChangeWorkDir`, so a load failure aborts the + * command before the workdir check, the required-flag check, the mutex check, + * and any API request. + * + * Resolution, mirroring Go (binary-verified, PR #5974 review round 7): + * + * 1. A token that case-insensitively (`strings.EqualFold`) matches a built-in + * profile name resolves to that profile's API URL. + * 2. Anything else is a config-file path handed to viper (`SetConfigFile` + + * `ReadInConfig`): + * - viper ignores an empty path and falls into search mode, which fails + * with `Config File "config" Not Found in "[]"`; + * - a path whose extension (Go `filepath.Ext` semantics: last `.` in the + * final path segment, ANY position — `.yml` alone is extension `yml`) + * is not in viper's `SupportedExts` fails with + * `Unsupported Config Type ""`; + * - an unreadable path fails with the OS error + * (`open : no such file or directory`, `read : is a + * directory`, …). + * 3. The content is parsed and decoded into Go's `Profile` struct with + * `UnmarshalExact` — unknown keys fail with mapstructure's + * `'utils.Profile' has invalid keys: ` block. Viper decodes + * with `WeaklyTypedInput`, so scalar YAML values (numbers, booleans) are + * stringified, not rejected (binary-verified: `api_url: 123` reaches the + * validator and fails the `http_url` tag, not decoding). + * 4. `validator.StructCtx` checks the struct tags in field order and fails + * with one `Key: 'Profile.' Error:Field validation for '' + * failed on the '' tag` line per failing field. + * + * Multi-line Go errors are rendered with every line padded to the longest + * line's width (lipgloss block layout, binary-verified) — the padding is baked + * into the message so stderr matches the Go binary byte-for-byte. One caveat: + * the shared error normalizer (`normalize-error.ts` `readString`) trims the + * message ends, so the FINAL line's trailing padding is stripped before + * rendering; interior lines (including blank ones) keep it. Binary-diffed: + * only that final-line whitespace differs, on inputs where both CLIs already + * exit 1 with zero requests. + * + * Accepted micro-divergences (all fail-closed: both sides exit 1 with no API + * request; only the detail text can differ): + * - YAML parse-failure detail text comes from the JS `yaml` package, not + * go-yaml (the `failed to read profile: While parsing config: ` prefix + * matches). + * - Non-YAML/JSON `SupportedExts` contents (`.toml`, `.env`, `.ini`, …) are + * parsed as YAML rather than with their native viper codecs. + * - Array/object values on string fields render the offending value + * approximately (Go's `%v` formatting emulated, not guaranteed). + * - The `http_url`/`hostname_rfc1123`/`uuid4` tag checks approximate + * go-playground/validator with WHATWG `URL` parsing and the validator's own + * published regexes. + */ +export interface LegacySsoLoadedProfile { + readonly apiUrl: string; + /** + * Go's `CurrentProfile.Name` — the canonical built-in name (EqualFold + * match) or the file's required `name:` field. Credential resolution keys + * the keyring account on this name (`access_token.go:43`), so the + * reconciled request must read the reconciled profile's token, not the + * config layer's (review r3684153345). + */ + readonly name: string; +} + +export function legacySsoLoadProfile( + token: string, + fs: FileSystem.FileSystem, +): Effect.Effect { + return Effect.gen(function* () { + // Go: `strings.EqualFold(p.Name, prof)` — the built-in names are all + // ASCII lower-case, so folding is plain lower-casing here. + const folded = token.toLowerCase(); + if (legacyIsBuiltinProfileName(folded)) { + return { apiUrl: legacyApiUrl(folded), name: folded }; + } + + // viper `SetConfigFile("")` is a no-op, so `ReadInConfig` falls back to + // its (empty) search-path mode — byte-exact per the Go binary. + if (token === "") { + return yield* failRead(`Config File "config" Not Found in "[]"`); + } + + const ext = goFilepathExt(token); + if (!VIPER_SUPPORTED_EXTS.has(ext)) { + return yield* failRead(`Unsupported Config Type ${JSON.stringify(ext)}`); + } + + const content = yield* fs + .readFileString(token) + .pipe( + Effect.catch((error) => + failRead( + error.reason._tag === "NotFound" + ? `open ${token}: no such file or directory` + : error.reason._tag === "PermissionDenied" + ? `open ${token}: permission denied` + : error.reason._tag === "BadResource" + ? `read ${token}: is a directory` + : error.message, + ), + ), + ); + + let parsed: unknown; + try { + parsed = parseYaml(content); + } catch (cause) { + return yield* failRead(`While parsing config: ${parseDetail(cause)}`); + } + if (parsed === null || parsed === undefined) { + parsed = {}; + } + if (typeof parsed !== "object" || Array.isArray(parsed)) { + // Go: yaml unmarshals into `map[string]interface{}` and rejects + // non-mapping documents. Detail text is best-effort (see doc comment). + return yield* failRead( + `While parsing config: yaml: unmarshal errors:\n cannot unmarshal into map[string]interface {}`, + ); + } + // Viper lowercases configuration keys before decoding + // (`insensitiviseMap`), so `API_URL:` / `Name:` decode exactly like + // their lowercase spellings, and UnmarshalExact reports unknown keys + // LOWERCASED (probed: `BOGUS_KEY` → `bogus_key`; review r3689635101). + // A same-key case collision is nondeterministic in Go (map iteration + // order) — document order (last wins) is used here. + const config: Record = {}; + for (const [key, value] of Object.entries(parsed as Record)) { + config[key.toLowerCase()] = value; + } + + // `UnmarshalExact` — unknown keys abort decoding. mapstructure reports + // them sorted, in a padded multi-line block (binary-verified). + const invalidKeys = Object.keys(config) + .filter((key) => !PROFILE_STRUCT_KEYS.has(key)) + .sort(); + if (invalidKeys.length > 0) { + return yield* failDecode(`'utils.Profile' has invalid keys: ${invalidKeys.join(", ")}`); + } + + // Weak scalar decoding + per-field tag validation, in struct field order. + const decodeErrors: string[] = []; + const validationErrors: string[] = []; + const values = new Map(); + for (const field of PROFILE_STRING_FIELDS) { + const raw = config[field.key]; + const weak = weakString(raw); + if (weak === undefined) { + decodeErrors.push( + `'${field.goName}' expected type 'string', got unconvertible type '${goTypeName(raw)}', value: '${goValueString(raw)}'`, + ); + continue; + } + values.set(field.key, weak); + if (weak === "") { + if (field.required) { + validationErrors.push(validatorLine(field.goName, "required")); + } + continue; + } + if (field.format !== undefined && !FORMAT_TAG_CHECKS[field.format](weak)) { + validationErrors.push(validatorLine(field.goName, field.format)); + } + } + if (decodeErrors.length > 0) { + return yield* failDecode(decodeErrors.join("\n")); + } + if (validationErrors.length > 0) { + return yield* fail(legacyPadGoErrorBlock(`invalid profile: ${validationErrors.join("\n")}`)); + } + + // `api_url` and `name` both passed the `required` tag above, so they are + // present and non-empty. + return { apiUrl: values.get("api_url") ?? "", name: values.get("name") ?? "" }; + }); +} + +const fail = (message: string) => Effect.fail(new LegacySsoProfileError({ message })); + +const failRead = (detail: string) => fail(`failed to read profile: ${detail}`); + +/** mapstructure's aggregate error template (`Decode` → `joinedError`). */ +const failDecode = (detail: string) => + fail( + legacyPadGoErrorBlock( + `failed to parse profile: decoding failed due to the following error(s):\n\n${detail}`, + ), + ); + +/** viper v1.21 `SupportedExts` (checked case-sensitively, like viper). */ +const VIPER_SUPPORTED_EXTS: ReadonlySet = new Set([ + "json", + "toml", + "yaml", + "yml", + "properties", + "props", + "prop", + "hcl", + "tfvars", + "dotenv", + "env", + "ini", +]); + +/** + * Go `filepath.Ext` minus the leading dot: scans the final path segment from + * the end and returns everything after the last `.` — including for + * dot-files (`.yml` → `yml`), where Node's `path.extname` returns `""`. + */ +function goFilepathExt(token: string): string { + for (let i = token.length - 1; i >= 0 && token[i] !== "/"; i--) { + if (token[i] === ".") { + return token.slice(i + 1); + } + } + return ""; +} + +/** Every mapstructure key of Go's `Profile` struct (`profile.go:17-27`). */ +const PROFILE_STRUCT_KEYS: ReadonlySet = new Set([ + "name", + "api_url", + "dashboard_url", + "docs_url", + "project_host", + "pooler_host", + "client_id", + "studio_image", + "regions", +]); + +type FormatTag = "http_url" | "hostname_rfc1123" | "uuid4"; + +interface ProfileStringField { + readonly key: string; + readonly goName: string; + readonly required: boolean; + readonly format?: FormatTag; +} + +/** + * Go's `Profile` string fields in struct order (`profile.go:17-27`) with + * their `validate:` tags — the order determines the order of the validator's + * error lines. `regions` (a slice, no validate tag) is exempt from weak + * string decoding and never validated, matching Go. + */ +const PROFILE_STRING_FIELDS: ReadonlyArray = [ + { key: "name", goName: "Name", required: true }, + { key: "api_url", goName: "APIURL", required: true, format: "http_url" }, + { key: "dashboard_url", goName: "DashboardURL", required: true, format: "http_url" }, + { key: "docs_url", goName: "DocsURL", required: false, format: "http_url" }, + { key: "project_host", goName: "ProjectHost", required: true, format: "hostname_rfc1123" }, + { key: "pooler_host", goName: "PoolerHost", required: false, format: "hostname_rfc1123" }, + { key: "client_id", goName: "AuthClientID", required: false, format: "uuid4" }, + { key: "studio_image", goName: "StudioImage", required: false }, +]; + +function validatorLine(goName: string, tag: string): string { + return `Key: 'Profile.${goName}' Error:Field validation for '${goName}' failed on the '${tag}' tag`; +} + +/** validator v10's `hostnameRegexRFC1123`, copied verbatim. */ +const HOSTNAME_RFC1123 = /^([a-zA-Z0-9][a-zA-Z0-9-]{0,62})(\.[a-zA-Z0-9][a-zA-Z0-9-]{0,62})*?$/; + +/** validator v10's `uuid4Regex`, copied verbatim (lower-case only). */ +const UUID4 = /^[0-9a-f]{8}-[0-9a-f]{4}-4[0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$/; + +const FORMAT_TAG_CHECKS: Record boolean> = { + http_url: (value) => { + let url: URL; + try { + url = new URL(value); + } catch { + return false; + } + return (url.protocol === "http:" || url.protocol === "https:") && url.host.length > 0; + }, + hostname_rfc1123: (value) => HOSTNAME_RFC1123.test(value), + uuid4: (value) => UUID4.test(value), +}; + +/** + * mapstructure `WeaklyTypedInput` string decoding: strings pass through, + * bools become `"1"`/`"0"`, numbers are stringified, `null` decodes to the + * zero value. Arrays/objects are unconvertible → `undefined` (decode error). + */ +function weakString(value: unknown): string | undefined { + if (value === null || value === undefined) return ""; + if (typeof value === "string") return value; + if (typeof value === "boolean") return value ? "1" : "0"; + if (typeof value === "number" || typeof value === "bigint") return String(value); + return undefined; +} + +function goTypeName(value: unknown): string { + if (Array.isArray(value)) return "[]interface {}"; + if (typeof value === "object" && value !== null) return "map[string]interface {}"; + return typeof value; +} + +/** Approximates Go's `%v` for the YAML values reachable here. */ +function goValueString(value: unknown): string { + if (Array.isArray(value)) return `[${value.map(goValueString).join(" ")}]`; + if (typeof value === "object" && value !== null) { + const entries = Object.entries(value as Record) + .map(([key, entry]) => `${key}:${goValueString(entry)}`) + .join(" "); + return `map[${entries}]`; + } + return String(value); +} + +/** + * Go renders these multi-line errors through a lipgloss block, which pads + * every line (including blank ones) with trailing spaces to the longest + * line's width (binary-verified via `od`). Baked into the message so the + * final stderr bytes match the Go binary exactly. + */ +export function legacyPadGoErrorBlock(message: string): string { + const lines = message.split("\n"); + const width = Math.max(...lines.map((line) => line.length)); + return lines.map((line) => line.padEnd(width)).join("\n"); +} + +function parseDetail(cause: unknown): string { + return cause instanceof Error ? cause.message : String(cause); +} diff --git a/apps/cli/src/legacy/commands/sso/sso.load-profile.unit.test.ts b/apps/cli/src/legacy/commands/sso/sso.load-profile.unit.test.ts new file mode 100644 index 0000000000..2380672320 --- /dev/null +++ b/apps/cli/src/legacy/commands/sso/sso.load-profile.unit.test.ts @@ -0,0 +1,304 @@ +import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; + +import { BunServices } from "@effect/platform-bun"; +import { afterAll, describe, expect, it } from "@effect/vitest"; +import { Effect, FileSystem } from "effect"; + +import type { LegacySsoProfileError } from "./sso.errors.ts"; +import { legacyPadGoErrorBlock, legacySsoLoadProfile } from "./sso.load-profile.ts"; + +const tempRoot = mkdtempSync(join(tmpdir(), "supabase-sso-load-profile-")); +afterAll(() => rmSync(tempRoot, { recursive: true, force: true })); + +const load = (token: string) => + Effect.gen(function* () { + const fs = yield* FileSystem.FileSystem; + return (yield* legacySsoLoadProfile(token, fs)).apiUrl; + }).pipe(Effect.provide(BunServices.layer)); + +const loadError = (token: string) => + load(token).pipe( + Effect.flip, + Effect.map((error: LegacySsoProfileError) => error.message), + ); + +const writeProfile = (name: string, content: string): string => { + const filePath = join(tempRoot, name); + writeFileSync(filePath, content); + return filePath; +}; + +describe("legacySsoLoadProfile", () => { + it.effect("resolves built-in profile names case-insensitively (Go strings.EqualFold)", () => + Effect.gen(function* () { + // Binary-verified: `--profile SUPABASE-LOCAL` targets localhost:8080. + expect(yield* load("SUPABASE-LOCAL")).toBe("http://localhost:8080"); + expect(yield* load("supabase")).toBe("https://api.supabase.com"); + expect(yield* load("supabase-staging")).toBe("https://api.supabase.green"); + expect(yield* load("snap")).toBe("https://cloudapi.snap.com"); + }), + ); + + it.effect("fails on an empty token with viper's search-mode error (Go `--profile=`)", () => + Effect.gen(function* () { + expect(yield* loadError("")).toBe( + `failed to read profile: Config File "config" Not Found in "[]"`, + ); + }), + ); + + it.effect("fails on a token without a supported extension (flag-shaped tokens)", () => + Effect.gen(function* () { + // `--profile --metadata-url …`: pflag binds the flag-shaped token and Go + // fails viper's extension gate (binary-verified, PR #5974 round 7). + expect(yield* loadError("--metadata-url")).toBe( + `failed to read profile: Unsupported Config Type ""`, + ); + expect(yield* loadError("profile.txt")).toBe( + `failed to read profile: Unsupported Config Type "txt"`, + ); + }), + ); + + it.effect("uses Go filepath.Ext semantics for dot-files (`.yml` IS extension `yml`)", () => + Effect.gen(function* () { + // Node's `path.extname(".yml")` is "" — Go's filepath.Ext is ".yml", so + // viper accepts the type and fails at the open() instead. + expect(yield* loadError(join(tempRoot, ".yml"))).toBe( + `failed to read profile: open ${join(tempRoot, ".yml")}: no such file or directory`, + ); + }), + ); + + it.effect("fails on a missing file with Go's os.Open error", () => + Effect.gen(function* () { + expect(yield* loadError("missing.yml")).toBe( + `failed to read profile: open missing.yml: no such file or directory`, + ); + }), + ); + + it.effect("fails on a directory with Go's read error", () => + Effect.gen(function* () { + const dir = join(tempRoot, "dir.yml"); + mkdirSync(dir, { recursive: true }); + expect(yield* loadError(dir)).toBe(`failed to read profile: read ${dir}: is a directory`); + }), + ); + + it.effect("resolves a valid YAML profile to its api_url", () => + Effect.gen(function* () { + const file = writeProfile( + "valid.yml", + [ + "name: harness", + "api_url: http://127.0.0.1:44444", + "dashboard_url: http://127.0.0.1:44444/dashboard", + "project_host: supabase.co", + ].join("\n"), + ); + expect(yield* load(file)).toBe("http://127.0.0.1:44444"); + }), + ); + + it.effect("accepts mixed-case keys like viper's insensitive decode (probed on go1.26)", () => + Effect.gen(function* () { + const fs = yield* FileSystem.FileSystem; + // `Name:` / `API_URL:` decode exactly like their lowercase spellings + // (viper `insensitiviseMap`; review r3689635101). + const file = writeProfile( + "mixed-case.yml", + [ + "Name: harness", + "API_URL: http://127.0.0.1:44444", + "dashboard_url: http://127.0.0.1:44444/dashboard", + "Project_Host: supabase.co", + ].join("\n"), + ); + const profile = yield* legacySsoLoadProfile(file, fs); + expect(profile.apiUrl).toBe("http://127.0.0.1:44444"); + expect(profile.name).toBe("harness"); + }).pipe(Effect.provide(BunServices.layer)), + ); + + it.effect("reports unknown keys LOWERCASED, like viper's pre-decode normalization", () => + Effect.gen(function* () { + const file = writeProfile( + "bogus-upper.yml", + [ + "name: harness", + "api_url: http://127.0.0.1:44444", + "dashboard_url: http://127.0.0.1:44444/dashboard", + "project_host: supabase.co", + "BOGUS_KEY: x", + ].join("\n"), + ); + expect(yield* loadError(file)).toContain("'utils.Profile' has invalid keys: bogus_key"); + }), + ); + + it.effect( + "returns Go's CurrentProfile.Name — the canonical built-in or the file's name field", + () => + Effect.gen(function* () { + const fs = yield* FileSystem.FileSystem; + // Built-in: EqualFold match resolves to the canonical (lower-case) + // table name — the keyring account Go reads (`access_token.go:43`). + expect((yield* legacySsoLoadProfile("SUPABASE-LOCAL", fs)).name).toBe("supabase-local"); + // File profile: `UnmarshalExact` populates Name from the required + // `name:` key, NOT from the file path. + const file = writeProfile( + "named.yml", + [ + "name: harness", + "api_url: http://127.0.0.1:44444", + "dashboard_url: http://127.0.0.1:44444/dashboard", + "project_host: supabase.co", + ].join("\n"), + ); + expect((yield* legacySsoLoadProfile(file, fs)).name).toBe("harness"); + }).pipe(Effect.provide(BunServices.layer)), + ); + + it.effect("rejects unknown keys with mapstructure's padded UnmarshalExact block", () => + Effect.gen(function* () { + // Byte-captured from the Go binary (`od -c`, PR #5974 round 7): keys + // sorted, every line padded with spaces to the longest line's width. + const file = writeProfile( + "extra-keys.yml", + [ + "name: extra", + "api_url: http://127.0.0.1:44444", + "dashboard_url: http://127.0.0.1:44444/dashboard", + "project_host: supabase.co", + "gotrue_url: http://127.0.0.1:44444/auth", + "db_url: postgres://localhost:5432/db", + ].join("\n"), + ); + const line1 = "failed to parse profile: decoding failed due to the following error(s):"; + const line3 = "'utils.Profile' has invalid keys: db_url, gotrue_url"; + expect(yield* loadError(file)).toBe( + [line1, "".padEnd(line1.length), line3.padEnd(line1.length)].join("\n"), + ); + }), + ); + + it.effect( + "reports missing required fields with the validator's padded lines, in struct order", + () => + Effect.gen(function* () { + // Byte-captured from the Go binary: `invalid profile: ` + one line per + // failing field (struct order), padded to the longest line's width. + const file = writeProfile("incomplete.yml", "name: incomplete\n"); + const lines = [ + "invalid profile: Key: 'Profile.APIURL' Error:Field validation for 'APIURL' failed on the 'required' tag", + "Key: 'Profile.DashboardURL' Error:Field validation for 'DashboardURL' failed on the 'required' tag", + "Key: 'Profile.ProjectHost' Error:Field validation for 'ProjectHost' failed on the 'required' tag", + ]; + const width = Math.max(...lines.map((line) => line.length)); + expect(yield* loadError(file)).toBe(lines.map((line) => line.padEnd(width)).join("\n")); + }), + ); + + it.effect("reports a missing name (only) — required covers empty strings", () => + Effect.gen(function* () { + const file = writeProfile( + "noname.yml", + [ + "api_url: http://127.0.0.1:44444", + "dashboard_url: http://127.0.0.1:44444/dashboard", + "project_host: supabase.co", + ].join("\n"), + ); + expect(yield* loadError(file)).toBe( + "invalid profile: Key: 'Profile.Name' Error:Field validation for 'Name' failed on the 'required' tag", + ); + }), + ); + + it.effect( + "weakly stringifies scalars like viper, so `api_url: 123` fails http_url, not decoding", + () => + Effect.gen(function* () { + // Binary-verified: viper decodes with WeaklyTypedInput, so the int + // reaches go-playground/validator and fails the `http_url` tag. + const file = writeProfile( + "typebad.yml", + [ + "name: t", + "api_url: 123", + "dashboard_url: http://127.0.0.1:44444/dashboard", + "project_host: supabase.co", + ].join("\n"), + ); + expect(yield* loadError(file)).toBe( + "invalid profile: Key: 'Profile.APIURL' Error:Field validation for 'APIURL' failed on the 'http_url' tag", + ); + }), + ); + + it.effect("validates the hostname_rfc1123 and http_url format tags", () => + Effect.gen(function* () { + const file = writeProfile( + "badhost.yml", + [ + "name: t", + "api_url: not-a-url", + "dashboard_url: http://127.0.0.1:44444/dashboard", + "project_host: 'bad host!'", + ].join("\n"), + ); + const lines = [ + "invalid profile: Key: 'Profile.APIURL' Error:Field validation for 'APIURL' failed on the 'http_url' tag", + "Key: 'Profile.ProjectHost' Error:Field validation for 'ProjectHost' failed on the 'hostname_rfc1123' tag", + ]; + const width = Math.max(...lines.map((line) => line.length)); + expect(yield* loadError(file)).toBe(lines.map((line) => line.padEnd(width)).join("\n")); + }), + ); + + it.effect("fails a malformed YAML file closed with viper's parse prefix", () => + Effect.gen(function* () { + // Detail text comes from the JS yaml package (documented micro- + // divergence); the class — abort before any request — matches Go. + const file = writeProfile("malformed.yml", "name: [broken\n api_url"); + const message = yield* loadError(file); + expect(message).toMatch(/^failed to read profile: While parsing config: /); + }), + ); + + it.effect("fails closed on unconvertible values (array on a string field)", () => + Effect.gen(function* () { + const file = writeProfile( + "arrayval.yml", + [ + "name: t", + "api_url: [http://a, http://b]", + "dashboard_url: http://127.0.0.1:44444/dashboard", + "project_host: supabase.co", + ].join("\n"), + ); + const message = yield* loadError(file); + expect(message).toContain( + "failed to parse profile: decoding failed due to the following error(s):", + ); + expect(message).toContain( + "'APIURL' expected type 'string', got unconvertible type '[]interface {}'", + ); + }), + ); +}); + +describe("legacyPadGoErrorBlock", () => { + it("pads every line — including blank ones — to the longest line's width", () => { + expect(legacyPadGoErrorBlock("abc\n\nlonger line")).toBe( + "abc \n \nlonger line", + ); + }); + + it("leaves single-line messages untouched", () => { + expect(legacyPadGoErrorBlock("only line")).toBe("only line"); + }); +}); diff --git a/apps/cli/src/legacy/commands/sso/sso.pflag-reconcile.ts b/apps/cli/src/legacy/commands/sso/sso.pflag-reconcile.ts new file mode 100644 index 0000000000..6f8f252600 --- /dev/null +++ b/apps/cli/src/legacy/commands/sso/sso.pflag-reconcile.ts @@ -0,0 +1,383 @@ +import { Effect, FileSystem, Option, Path, Result } from "effect"; + +import type { PflagArgvScan } from "../../../shared/cli/cobra-flag-groups.ts"; +import { LegacyProfileFlag, LegacyWorkdirFlag } from "../../../shared/legacy/global-flags.ts"; +import { RuntimeInfo } from "../../../shared/runtime/runtime-info.service.ts"; +import { legacyProfileFilePath } from "../../config/legacy-profile-file.ts"; +import { legacyParseStringSliceFlag } from "../../shared/legacy-string-slice-flag.ts"; +import { legacyValidateWorkdirIsDirectory } from "../../shared/legacy-workdir-validation.ts"; +import { LegacySsoWorkdirError } from "./sso.errors.ts"; +import { legacySsoLoadProfile, type LegacySsoLoadedProfile } from "./sso.load-profile.ts"; + +/** + * Reconciles an Effect-parsed option flag with pflag semantics + * (`pflagArgvScan`): the flag is only set when the raw-argv scan + * says pflag would have set it, and its value is the scan's — for a pflag + * `StringVar`, the last occurrence wins. + * + * This matters because the vendored Effect parser refuses to consume a + * flag-shaped token as a value while pflag consumes it unconditionally + * (`run.unit.test.ts`, CLI-1982). In + * `--project-ref --metadata-file x.xml --metadata-url u`, pflag hands + * `--metadata-file` to `--project-ref` as its value and never sets + * `metadata-file`; acting on the parsed options there would suppress the + * mutex error yet still read the metadata file — an API call the Go CLI + * never makes. When the scan and the parser agree (every normal invocation), + * the scan's value is byte-identical to the parsed one. + */ +export function legacySsoPflagStringValue( + occurrences: ReadonlyMap>, + flagName: string, +): Option.Option { + const values = occurrences.get(flagName); + return values === undefined ? Option.none() : Option.some(values[values.length - 1] ?? ""); +} + +/** + * Like `legacySsoPflagStringValue`, but for pflag `StringSliceVar` flags: + * every occurrence is CSV-split and accumulated, matching pflag's + * `stringSliceValue.Set`. An absent flag reconciles to `[]` even when the + * Effect parser produced values (its tokens were consumed by another flag). + * + * `parsedFallback` is only returned if the scan's raw values are malformed + * CSV — unreachable through the real CLI, because the Effect parser sees the + * same raw values and rejects the command at parse time before the handler + * runs; the fallback just keeps a handler-level disagreement from crashing. + */ +export function legacySsoPflagSliceValue( + occurrences: ReadonlyMap>, + flagName: string, + parsedFallback: ReadonlyArray, +): ReadonlyArray { + const values = occurrences.get(flagName); + if (values === undefined) { + return []; + } + try { + return legacyParseStringSliceFlag(values); + } catch { + return parsedFallback; + } +} + +/** + * The workdir Go's `ChangeWorkDir` (`internal/utils/misc.go:238-257`) would + * `os.Chdir` to: `viper.GetString("WORKDIR")` resolves the pflag-effective + * `--workdir` first (a changed flag wins even when its value is empty — + * `--workdir=` falls through to the always-existing project-root walk-up, + * never to the env var) and `SUPABASE_WORKDIR` otherwise. `Option.none` + * means Go would chdir to the walk-up default, which cannot fail. + * + * Resolution order (binary-verified against `apps/cli-go`, PR #5974 review + * round 6): + * - the scan's last `--workdir` occurrence wins — pflag consumes flag-shaped + * tokens the Effect parser refuses (`--workdir --metadata-file` binds + * `"--metadata-file"`), so the parsed flag cannot be trusted; + * - when the `--workdir` token itself was consumed as another flag's value + * (`--domains --workdir`), pflag never marks it changed and viper falls to + * the env var — the parsed flag (which read the following token as a + * normal value) must be ignored; + * - otherwise the Effect-parsed value covers what the anchored scan cannot + * see: `--workdir` placed before the command path (`supabase --workdir x + * sso add …`), which cobra's `Find`/`stripFlags` routes to the same + * persistent flag. + */ +export function legacySsoPflagWorkdirValue( + scan: Pick, + parsedWorkdir: Option.Option, + envWorkdir: string | undefined, +): Option.Option { + const scanned = legacySsoPflagStringValue(scan.occurrences, "workdir"); + // Same last-wins order as the profile resolver: post-path occurrence → + // pre-path occurrence (pflag parses persistent flags before the command + // path and repeats resolve last-wins, while the Effect parser is + // first-wins) → consumed-discard → parsed fallback (review r3690…, the + // pre-path workdir twin of r3686720491). + const prePathValues = scan.prePathOccurrences.get("workdir"); + const prePath = + prePathValues !== undefined && prePathValues.length > 0 + ? Option.some(prePathValues[prePathValues.length - 1] as string) + : Option.none(); + const flagValue = Option.isSome(scanned) + ? scanned + : Option.isSome(prePath) + ? prePath + : scan.consumedFlagNames.has("workdir") + ? Option.none() + : parsedWorkdir; + if (Option.isSome(flagValue)) { + return flagValue.value.length > 0 ? flagValue : Option.none(); + } + return envWorkdir !== undefined && envWorkdir.length > 0 + ? Option.some(envWorkdir) + : Option.none(); +} + +/** + * Emulates Go's `ChangeWorkDir` (`cmd/root.go:104`, `internal/utils/ + * misc.go:238-257`) for the workdir {@link legacySsoPflagWorkdirValue} + * resolves: `os.Chdir` on a missing path or a non-directory aborts the + * command from the root `PersistentPreRunE` — after `ParseFlags` and + * `ValidateArgs`, before `ValidateRequiredFlags`, `ValidateFlagGroups`, and + * `RunE` — so no API call is ever made. The Effect layer neither validates + * the resolved workdir (`legacy-cli-config.layer.ts` only path-resolves it) + * nor sees the value at all when `--workdir` consumed a flag-shaped token, + * hence the emulation here (PR #5974 review round 6). + * + * Accepted micro-divergence: when the pflag-bound workdir names a directory + * that EXISTS, Go chdir's into it (printing `Using workdir …`) while the + * config layer keeps the workdir it resolved from the parsed flag — both + * sides then issue the identical request for these inputs. + */ +export const legacySsoValidatePflagWorkdir = Effect.fnUntraced(function* ( + scan: Pick, +) { + // `serviceOption`: absent outside the real CLI tree (handler-level tests + // provide argv via `Stdio.layerTest`, not the global flag settings). + const parsedWorkdir = Option.flatten(yield* Effect.serviceOption(LegacyWorkdirFlag)); + const workdir = legacySsoPflagWorkdirValue(scan, parsedWorkdir, process.env["SUPABASE_WORKDIR"]); + if (Option.isNone(workdir)) { + return; + } + const fs = yield* FileSystem.FileSystem; + yield* legacyValidateWorkdirIsDirectory(workdir.value, fs).pipe( + Effect.mapError((cause) => new LegacySsoWorkdirError({ message: cause.message })), + ); +}); + +/** + * The explicit (flag-or-env) profile token viper's `GetString("PROFILE")` / + * `IsSet("PROFILE")` would resolve (`getProfileName`, `profile.go:121-136`). + * `Option.none` means Go would fall through to the persisted + * `~/.supabase/profile` file and then the `supabase` default. + * + * Resolution order mirrors {@link legacySsoPflagWorkdirValue} (same viper + * semantics, binary-verified for `--profile` in PR #5974 review round 7): + * - the scan's last `--profile` occurrence wins — pflag consumes flag-shaped + * tokens the Effect parser refuses (`--profile --metadata-url` binds + * `"--metadata-url"`), is last-wins where the parser is first-wins, and a + * scanned occurrence marks the flag changed even when its value is the + * `supabase` default or empty; + * - when the `--profile` token itself was consumed as another flag's value + * (`--domains --profile alternate.yml`), pflag never marks it changed and + * viper falls to `SUPABASE_PROFILE` — the parsed flag (which read the + * following token as a normal value) must be ignored; + * - otherwise the Effect-parsed value covers pre-command-path placement + * (`supabase --profile x sso add …`) the anchored scan cannot see. The + * parsed flag cannot distinguish an explicit `--profile supabase` from the + * flag's default, so that value is treated as unset — the same proxy the + * config layer uses (`legacy-cli-config.layer.ts`). + */ +export function legacySsoPflagProfileValue( + scan: Pick, + parsedProfile: Option.Option, + envProfile: string | undefined, +): Option.Option { + const scanned = legacySsoPflagStringValue(scan.occurrences, "profile"); + // pflag's effective value is the LAST parsed occurrence anywhere in argv: + // a post-path occurrence wins outright; otherwise a persistent pre-path + // occurrence (`--profile A sso add …`) stays effective even when a later + // profile-shaped token was CONSUMED as another flag's value — discarding + // it here fell through to env/file/default and targeted a host Go never + // contacts (review r3686720491). + const prePathValues = scan.prePathOccurrences.get("profile"); + const prePath = + prePathValues !== undefined && prePathValues.length > 0 + ? Option.some(prePathValues[prePathValues.length - 1] as string) + : Option.none(); + const flagValue = Option.isSome(scanned) + ? scanned + : Option.isSome(prePath) + ? prePath + : scan.consumedFlagNames.has("profile") + ? Option.none() + : Option.filter(parsedProfile, (value) => value !== "supabase"); + if (Option.isSome(flagValue)) { + return flagValue; + } + return envProfile !== undefined && envProfile.length > 0 + ? Option.some(envProfile) + : Option.none(); +} + +/** + * Emulates Go's `LoadProfile` (`cmd/root.go:98-102`, `profile.go:94-118`) for + * the pflag/viper-effective profile, returning the API URL the request must + * target when it differs from the one the Effect config layer resolved — + * `Option.none` means the layer's `LegacyCliConfig.apiUrl` already matches + * Go. Go loads the profile immediately BEFORE `ChangeWorkDir`, so a load + * failure here must precede the workdir check (and, like it, the + * required-flag check, the mutex check, and any API request). + * + * The emulation only takes over when the viper-effective token disagrees + * with the token the config layer resolved from the parsed flag — i.e. + * exactly where the Effect parser and pflag diverge (consumed tokens, + * flag-shaped values, repeat resolution, explicit `--profile supabase` + * shadowing the env, an untrimmed persisted-file token) — or when the token + * is empty, which Go deterministically rejects. Where the two agree (every + * normal invocation), the layer's resolution stands unchanged, including its + * pre-existing lenient missing/malformed-file fallback, which predates this + * PR and applies shell-wide (tracked separately from CLI-1982). + * + * `serviceOption` throughout: outside the real CLI tree (handler-level tests + * provide argv via `Stdio.layerTest`) the flag settings and `RuntimeInfo` + * may be absent; the emulation then only acts on what the scan itself shows. + */ +export const legacySsoResolvePflagProfile = Effect.fnUntraced(function* ( + scan: Pick, +) { + const parsedRaw = yield* Effect.serviceOption(LegacyProfileFlag); + const parsedProfile = Option.filter(parsedRaw, (value) => value !== "supabase"); + const env = process.env["SUPABASE_PROFILE"]; + const envProfile = env !== undefined && env.length > 0 ? env : undefined; + + // viper-effective explicit token vs the config layer's explicit token + // (`resolveProfile`, `legacy-cli-config.layer.ts`: parsed flag ≠ default → + // env). When both agree on a non-empty explicit token, the layer resolved + // the exact same profile the Go binary would target. + const goExplicit = legacySsoPflagProfileValue(scan, parsedProfile, envProfile); + const layerExplicit = Option.isSome(parsedProfile) + ? parsedProfile + : envProfile !== undefined + ? Option.some(envProfile) + : Option.none(); + if ( + Option.isSome(goExplicit) && + Option.isSome(layerExplicit) && + goExplicit.value === layerExplicit.value && + goExplicit.value !== "" + ) { + return Option.none(); + } + + const fs = yield* Effect.serviceOption(FileSystem.FileSystem); + const path = yield* Effect.serviceOption(Path.Path); + const runtimeInfo = yield* Effect.serviceOption(RuntimeInfo); + if (Option.isNone(fs) || Option.isNone(path) || Option.isNone(runtimeInfo)) { + return Option.none(); + } + + // Lowest precedence: the persisted `~/.supabase/profile` file. Go uses the + // raw bytes (`string(content)`, `profile.go:130-131`); the config layer + // trims and maps empty to the default — a real divergence the token + // comparison below surfaces (e.g. a trailing newline makes the Go binary + // fail with `Unsupported Config Type ""`, binary-verified). + const fileRaw = yield* fs.value + .readFileString(legacyProfileFilePath(path.value, runtimeInfo.value.homeDir)) + .pipe(Effect.option); + + const goToken = Option.isSome(goExplicit) + ? goExplicit.value + : Option.isSome(fileRaw) + ? fileRaw.value + : "supabase"; + const layerToken = Option.isSome(layerExplicit) + ? layerExplicit.value + : Option.match(fileRaw, { + onNone: () => "supabase", + onSome: (content) => { + const trimmed = content.trim(); + return trimmed.length === 0 ? "supabase" : trimmed; + }, + }); + + if (goToken === layerToken && goToken !== "") { + return Option.none(); + } + return Option.some(yield* legacySsoLoadProfile(goToken, fs.value)); +}); + +/** Go's `strconv.ParseBool` accepted literals (`strconv/atob.go:10-19`). */ +const GO_PARSE_BOOL: ReadonlyMap = new Map([ + ["1", true], + ["t", true], + ["T", true], + ["TRUE", true], + ["true", true], + ["True", true], + ["0", false], + ["f", false], + ["F", false], + ["FALSE", false], + ["false", false], + ["False", false], +]); + +/** + * Like `legacySsoPflagStringValue`, but for pflag `BoolVar` flags. pflag + * calls `Value.Set` for every occurrence in argv order: a bare occurrence + * sets `NoOptDefVal` (`"true"`), an inline `=value` goes through + * `strconv.ParseBool`, an invalid literal aborts `ParseFlags` with + * `invalid argument …` (pflag `errors.go:32-48`) before `ValidateArgs`, + * every hook, and `RunE` — the failure branch here must therefore win over + * every later handler check. The last occurrence wins; an absent flag is + * `false` (the Go default). + * + * This cannot be read off the Effect-parsed boolean for two reasons + * (binary-verified against `apps/cli-go`, PR #5974 review round 4): + * - the Effect parser resolves repeated flags first-wins while pflag is + * last-wins (`--skip-url-validation=false --skip-url-validation` is `true` + * to Go, `false` to the parser), and + * - the Effect parser accepts `yes`/`no`, which `strconv.ParseBool` rejects. + * + * The scan records a *bare* occurrence as pflag's `NoOptDefVal` `"true"` + * (pflag `flag.go:1017-1019`) and an inline-empty `--flag=` as `""`, so the + * two stay distinguishable here: `""` goes through the ParseBool table and + * fails exactly like Go. Reachable despite the Effect parser rejecting an + * explicit empty boolean at parse time, because first-wins parsing never + * validates later occurrences (binary-verified, PR #5974 review round 5: + * `--skip-url-validation=false --skip-url-validation=` aborts Go's + * ParseFlags before any request; the parser accepts the argv). + */ +export function legacySsoPflagBoolValue( + occurrences: ReadonlyMap>, + flagName: string, +): Result.Result { + const values = occurrences.get(flagName); + if (values === undefined) { + return Result.succeed(false); + } + let effective = false; + for (const raw of values) { + const parsed = GO_PARSE_BOOL.get(raw); + if (parsed === undefined) { + return Result.fail( + `invalid argument ${JSON.stringify(raw)} for "--${flagName}" flag: strconv.ParseBool: parsing ${JSON.stringify(raw)}: invalid syntax`, + ); + } + effective = parsed; + } + return Result.succeed(effective); +} + +/** + * Like `legacySsoPflagStringValue`, but for Go enum-valued flags + * (`ssoProviderType`, `ssoNameIDFormat` — `cmd/sso.go:157-158,176`), whose + * `Value.Set` rejects anything outside the allowed set. pflag Sets every + * occurrence in argv order and aborts `ParseFlags` on the first invalid one + * — reachable here because the Effect parser resolves repeats first-wins and + * never validates later occurrences (`--type saml --type bogus` parses). + * The last occurrence wins; an absent flag is `Option.none`. + * + * `flagLabel` is how pflag names the flag in the error: `--name` without a + * shorthand, `-s, --name` with one (pflag `errors.go:39-41`). + */ +export function legacySsoPflagEnumValue( + occurrences: ReadonlyMap>, + flagName: string, + allowed: ReadonlyArray, + flagLabel: string = `--${flagName}`, +): Result.Result, string> { + const values = occurrences.get(flagName); + if (values === undefined) { + return Result.succeed(Option.none()); + } + for (const raw of values) { + if (!allowed.includes(raw)) { + return Result.fail( + `invalid argument ${JSON.stringify(raw)} for "${flagLabel}" flag: must be one of [ ${allowed.join(" | ")} ]`, + ); + } + } + return Result.succeed(Option.some(values[values.length - 1] ?? "")); +} diff --git a/apps/cli/src/legacy/commands/sso/sso.pflag-reconcile.unit.test.ts b/apps/cli/src/legacy/commands/sso/sso.pflag-reconcile.unit.test.ts new file mode 100644 index 0000000000..5a68ffdb71 --- /dev/null +++ b/apps/cli/src/legacy/commands/sso/sso.pflag-reconcile.unit.test.ts @@ -0,0 +1,345 @@ +import { describe, expect, it } from "@effect/vitest"; +import { Option, Result } from "effect"; + +import { + legacySsoPflagBoolValue, + legacySsoPflagEnumValue, + legacySsoPflagProfileValue, + legacySsoPflagWorkdirValue, +} from "./sso.pflag-reconcile.ts"; +import { LEGACY_SSO_NAME_ID_FORMATS } from "./sso.saml.ts"; + +const occ = (entries: ReadonlyArray]>) => + new Map(entries.map(([name, values]) => [name, [...values]])); + +describe("legacySsoPflagBoolValue", () => { + it("is false when the flag never occurs (Go default)", () => { + expect(legacySsoPflagBoolValue(occ([]), "skip-url-validation")).toEqual(Result.succeed(false)); + }); + + it('treats a bare occurrence (recorded as pflag\'s NoOptDefVal "true") as true', () => { + expect( + legacySsoPflagBoolValue(occ([["skip-url-validation", ["true"]]]), "skip-url-validation"), + ).toEqual(Result.succeed(true)); + }); + + it("resolves repeats last-wins, not first-wins (pflag Sets every occurrence)", () => { + // `--skip-url-validation=false --skip-url-validation` — Go ends up true. + expect( + legacySsoPflagBoolValue( + occ([["skip-url-validation", ["false", "true"]]]), + "skip-url-validation", + ), + ).toEqual(Result.succeed(true)); + // `--skip-url-validation --skip-url-validation=false` — Go ends up false. + expect( + legacySsoPflagBoolValue( + occ([["skip-url-validation", ["true", "false"]]]), + "skip-url-validation", + ), + ).toEqual(Result.succeed(false)); + }); + + it("fails on an inline-empty occurrence exactly like Go's ParseBool", () => { + // `--skip-url-validation=false --skip-url-validation=` — the Effect + // parser resolves repeats first-wins and never validates the second + // occurrence, but pflag hands `""` to strconv.ParseBool and aborts + // ParseFlags before any request (binary-verified, PR #5974 round 5). + expect( + legacySsoPflagBoolValue(occ([["skip-url-validation", ["false", ""]]]), "skip-url-validation"), + ).toEqual( + Result.fail( + `invalid argument "" for "--skip-url-validation" flag: strconv.ParseBool: parsing "": invalid syntax`, + ), + ); + }); + + it("accepts exactly Go's strconv.ParseBool literal set", () => { + for (const raw of ["1", "t", "T", "TRUE", "true", "True"]) { + expect(legacySsoPflagBoolValue(occ([["f", [raw]]]), "f")).toEqual(Result.succeed(true)); + } + for (const raw of ["0", "f", "F", "FALSE", "false", "False"]) { + expect(legacySsoPflagBoolValue(occ([["f", [raw]]]), "f")).toEqual(Result.succeed(false)); + } + }); + + it("fails with pflag's byte-exact invalid-argument message on the first bad occurrence", () => { + // The Effect parser accepts `yes`/`no`; Go's strconv.ParseBool does not. + expect( + legacySsoPflagBoolValue(occ([["skip-url-validation", ["yes"]]]), "skip-url-validation"), + ).toEqual( + Result.fail( + `invalid argument "yes" for "--skip-url-validation" flag: strconv.ParseBool: parsing "yes": invalid syntax`, + ), + ); + // A later invalid occurrence still fails — pflag Sets each one in order. + expect( + legacySsoPflagBoolValue( + occ([["skip-url-validation", ["true", "no"]]]), + "skip-url-validation", + ), + ).toEqual( + Result.fail( + `invalid argument "no" for "--skip-url-validation" flag: strconv.ParseBool: parsing "no": invalid syntax`, + ), + ); + }); +}); + +describe("legacySsoPflagEnumValue", () => { + it("is none when the flag never occurs", () => { + expect(legacySsoPflagEnumValue(occ([]), "name-id-format", LEGACY_SSO_NAME_ID_FORMATS)).toEqual( + Result.succeed(Option.none()), + ); + }); + + it("resolves repeats last-wins, matching pflag", () => { + const persistent = "urn:oasis:names:tc:SAML:2.0:nameid-format:persistent"; + const transient = "urn:oasis:names:tc:SAML:2.0:nameid-format:transient"; + expect( + legacySsoPflagEnumValue( + occ([["name-id-format", [transient, persistent]]]), + "name-id-format", + LEGACY_SSO_NAME_ID_FORMATS, + ), + ).toEqual(Result.succeed(Option.some(persistent))); + }); + + it("fails with the Go enum Set message when any occurrence is invalid", () => { + const persistent = "urn:oasis:names:tc:SAML:2.0:nameid-format:persistent"; + expect( + legacySsoPflagEnumValue( + occ([["name-id-format", [persistent, "bogus"]]]), + "name-id-format", + LEGACY_SSO_NAME_ID_FORMATS, + ), + ).toEqual( + Result.fail( + `invalid argument "bogus" for "--name-id-format" flag: must be one of [ ${LEGACY_SSO_NAME_ID_FORMATS.join(" | ")} ]`, + ), + ); + }); + + it("names the flag with its shorthand when a label is given (pflag errors.go:39-41)", () => { + expect( + legacySsoPflagEnumValue(occ([["type", ["bogus"]]]), "type", ["saml"], "-t, --type"), + ).toEqual( + Result.fail(`invalid argument "bogus" for "-t, --type" flag: must be one of [ saml ]`), + ); + }); +}); + +describe("legacySsoPflagWorkdirValue", () => { + const scan = ( + entries: ReadonlyArray]>, + consumed: ReadonlyArray = [], + prePath: ReadonlyArray]> = [], + ) => ({ + occurrences: occ(entries), + consumedFlagNames: new Set(consumed), + prePathOccurrences: occ(prePath), + }); + + it("resolves pre-path repeats last-wins, like pflag (the parser is first-wins)", () => { + // `--workdir /existing --workdir /missing sso add …`: pflag uses the + // LAST pre-path occurrence (/missing → Go's chdir aborts) while the + // Effect parser bound the first (pre-path workdir twin of the profile + // fix, PR #5974 review round 11). + expect( + legacySsoPflagWorkdirValue( + scan([], [], [["workdir", ["/existing", "/missing"]]]), + Option.some("/existing"), + undefined, + ), + ).toEqual(Option.some("/missing")); + }); + + it("keeps a pre-path occurrence when the only post-path workdir token was consumed", () => { + expect( + legacySsoPflagWorkdirValue( + scan([], ["workdir"], [["workdir", ["/pre"]]]), + Option.some("/pre"), + "/env", + ), + ).toEqual(Option.some("/pre")); + }); + + it("post-path occurrences still win over pre-path ones (argv-order last-wins)", () => { + expect( + legacySsoPflagWorkdirValue( + scan([["workdir", ["/post"]]], [], [["workdir", ["/pre"]]]), + Option.some("/pre"), + undefined, + ), + ).toEqual(Option.some("/post")); + }); + + it("resolves nothing when no flag, parsed value, or env var is present (Go walks up)", () => { + expect(legacySsoPflagWorkdirValue(scan([]), Option.none(), undefined)).toEqual(Option.none()); + }); + + it("prefers the scan's occurrence over the parsed flag and the env var", () => { + // `--workdir --metadata-file …`: pflag binds the flag-shaped token; the + // Effect parser refused it and left the flag unset (PR #5974 round 6). + expect( + legacySsoPflagWorkdirValue(scan([["workdir", ["--metadata-file"]]]), Option.none(), "/env"), + ).toEqual(Option.some("--metadata-file")); + }); + + it("resolves repeats last-wins, matching pflag StringVar", () => { + expect( + legacySsoPflagWorkdirValue(scan([["workdir", ["/a", "/b"]]]), Option.some("/a"), undefined), + ).toEqual(Option.some("/b")); + }); + + it("falls back to the parsed flag when the anchored scan saw no occurrence (pre-path --workdir)", () => { + expect(legacySsoPflagWorkdirValue(scan([]), Option.some("/pre-path"), "/env")).toEqual( + Option.some("/pre-path"), + ); + }); + + it("ignores the parsed flag when the --workdir token was consumed by another flag, falling to the env var", () => { + // `--domains --workdir /x`: pflag hands `--workdir` to `--domains` and + // never marks workdir changed, so viper falls to SUPABASE_WORKDIR + // (binary-verified against apps/cli-go, PR #5974 round 6). + expect(legacySsoPflagWorkdirValue(scan([], ["workdir"]), Option.some("/x"), "/env")).toEqual( + Option.some("/env"), + ); + expect(legacySsoPflagWorkdirValue(scan([], ["workdir"]), Option.some("/x"), undefined)).toEqual( + Option.none(), + ); + }); + + it("uses the env var when neither the scan nor the parser saw the flag", () => { + expect(legacySsoPflagWorkdirValue(scan([]), Option.none(), "/env")).toEqual( + Option.some("/env"), + ); + }); + + it("treats a changed-but-empty flag as the walk-up default, shadowing the env var (viper precedence)", () => { + // `--workdir=`: viper returns the changed flag's empty value and Go falls + // through to the always-existing project root, never to SUPABASE_WORKDIR + // (binary-verified: the command proceeds to POST). + expect(legacySsoPflagWorkdirValue(scan([["workdir", [""]]]), Option.none(), "/env")).toEqual( + Option.none(), + ); + expect(legacySsoPflagWorkdirValue(scan([]), Option.some(""), "/env")).toEqual(Option.none()); + }); + + it("treats an empty env var as unset", () => { + expect(legacySsoPflagWorkdirValue(scan([]), Option.none(), "")).toEqual(Option.none()); + }); +}); + +describe("legacySsoPflagProfileValue", () => { + const scan = ( + entries: ReadonlyArray]>, + consumed: ReadonlyArray = [], + prePath: ReadonlyArray]> = [], + ) => ({ + occurrences: occ(entries), + consumedFlagNames: new Set(consumed), + prePathOccurrences: occ(prePath), + }); + + it("keeps a pre-path occurrence when the only post-path profile token was consumed", () => { + // `--profile A sso add --type saml --domains --profile`: pflag parsed A + // pre-path (cobra Find strips persistent flags while routing) and never + // parsed the consumed token, so A stays effective — falling through to + // env/default targeted a host Go never contacts (review r3686720491). + expect( + legacySsoPflagProfileValue( + scan([], ["profile"], [["profile", ["a.yml"]]]), + Option.some("a.yml"), + "env.yml", + ), + ).toEqual(Option.some("a.yml")); + }); + + it("resolves pre-path repeats last-wins, like pflag (the parser is first-wins)", () => { + expect( + legacySsoPflagProfileValue( + scan([], [], [["profile", ["a.yml", "b.yml"]]]), + Option.some("a.yml"), + undefined, + ), + ).toEqual(Option.some("b.yml")); + }); + + it("post-path occurrences still win over pre-path ones (argv-order last-wins)", () => { + expect( + legacySsoPflagProfileValue( + scan([["profile", ["post.yml"]]], [], [["profile", ["pre.yml"]]]), + Option.some("pre.yml"), + undefined, + ), + ).toEqual(Option.some("post.yml")); + }); + + it("resolves nothing when no flag, parsed value, or env var is present (Go falls to the file/default)", () => { + expect(legacySsoPflagProfileValue(scan([]), Option.none(), undefined)).toEqual(Option.none()); + }); + + it("prefers the scan's occurrence over the parsed flag and the env var", () => { + // `--profile --metadata-url …`: pflag binds the flag-shaped token; the + // Effect parser refused it and left the flag at its default (PR #5974 + // round 7). + expect( + legacySsoPflagProfileValue(scan([["profile", ["--metadata-url"]]]), Option.none(), "env.yml"), + ).toEqual(Option.some("--metadata-url")); + }); + + it("resolves repeats last-wins, matching pflag StringVar (the parser is first-wins)", () => { + expect( + legacySsoPflagProfileValue( + scan([["profile", ["a.yml", "b.yml"]]]), + Option.some("a.yml"), + undefined, + ), + ).toEqual(Option.some("b.yml")); + }); + + it("keeps an explicit scanned `supabase` — pflag marks it changed, shadowing the env var", () => { + // viper: a changed flag wins even at its default value; the config layer + // cannot see this (its parsed flag can't distinguish default from + // explicit), so the scan is authoritative post-command-path. + expect( + legacySsoPflagProfileValue(scan([["profile", ["supabase"]]]), Option.none(), "env.yml"), + ).toEqual(Option.some("supabase")); + }); + + it("keeps a changed-but-empty occurrence — Go fails LoadProfile on it, never falling to the env", () => { + expect(legacySsoPflagProfileValue(scan([["profile", [""]]]), Option.none(), "env.yml")).toEqual( + Option.some(""), + ); + }); + + it("falls back to the parsed flag when the anchored scan saw no occurrence (pre-path --profile)", () => { + expect(legacySsoPflagProfileValue(scan([]), Option.some("pre.yml"), "env.yml")).toEqual( + Option.some("pre.yml"), + ); + }); + + it("ignores the parsed flag when the --profile token was consumed by another flag, falling to the env var", () => { + // `--domains --profile alternate.yml`: pflag hands `--profile` to + // `--domains` and never marks profile changed, so viper falls to + // SUPABASE_PROFILE (binary-verified against apps/cli-go, PR #5974 + // round 7 — the demonstrated divergent input). + expect( + legacySsoPflagProfileValue(scan([], ["profile"]), Option.some("alternate.yml"), "env.yml"), + ).toEqual(Option.some("env.yml")); + expect( + legacySsoPflagProfileValue(scan([], ["profile"]), Option.some("alternate.yml"), undefined), + ).toEqual(Option.none()); + }); + + it("uses the env var when neither the scan nor the parser saw the flag", () => { + expect(legacySsoPflagProfileValue(scan([]), Option.none(), "env.yml")).toEqual( + Option.some("env.yml"), + ); + }); + + it("treats an empty env var as unset", () => { + expect(legacySsoPflagProfileValue(scan([]), Option.none(), "")).toEqual(Option.none()); + }); +}); diff --git a/apps/cli/src/legacy/commands/sso/sso.saml.ts b/apps/cli/src/legacy/commands/sso/sso.saml.ts index 972bb253a3..118f10a52b 100644 --- a/apps/cli/src/legacy/commands/sso/sso.saml.ts +++ b/apps/cli/src/legacy/commands/sso/sso.saml.ts @@ -1,5 +1,19 @@ import { Effect, FileSystem } from "effect"; +/** + * The `--name-id-format` value set, shared by `sso add` and `sso update` + * (both commands bind the same Go `ssoNameIDFormat` enum var, + * `cmd/sso.go:158,176`). Order matters twice: it drives the CLI help text + * and it is joined verbatim into pflag's `invalid argument … must be one of + * [ … ]` error (`legacySsoPflagEnumValue`), which must byte-match Go. + */ +export const LEGACY_SSO_NAME_ID_FORMATS = [ + "urn:oasis:names:tc:SAML:1.1:nameid-format:emailAddress", + "urn:oasis:names:tc:SAML:1.1:nameid-format:unspecified", + "urn:oasis:names:tc:SAML:2.0:nameid-format:persistent", + "urn:oasis:names:tc:SAML:2.0:nameid-format:transient", +] as const; + /** * Validates that raw bytes decode as strict UTF-8. Mirrors Go's * `unicode/utf8.Valid(data)` check inside `saml.ValidateMetadata`. Using diff --git a/apps/cli/src/legacy/commands/sso/update/SIDE_EFFECTS.md b/apps/cli/src/legacy/commands/sso/update/SIDE_EFFECTS.md index 4fa58fb871..cd10ca995e 100644 --- a/apps/cli/src/legacy/commands/sso/update/SIDE_EFFECTS.md +++ b/apps/cli/src/legacy/commands/sso/update/SIDE_EFFECTS.md @@ -41,16 +41,21 @@ GET still uses the typed client. ## Exit Codes -| Code | Condition | -| ---- | ------------------------------------------------------------------------------------------------------------------------------------ | -| `0` | success | -| `1` | `LegacySsoInvalidUuidError` — provider ID is not a canonical UUID | -| `1` | `LegacySsoMutexFlagError` — flag combinations: `--domains` with `--add/--remove-domains`, or `--metadata-file` with `--metadata-url` | -| `1` | `LegacySsoUpdateMetadataFileError` — metadata file unreadable, non-UTF-8, or metadata URL invalid/unreachable/non-UTF-8 | -| `1` | `LegacySsoUpdateAttributeMappingFileError` — JSON file unreadable or malformed | -| `1` | `LegacySsoUpdateNotFoundError` — 404 from GET | -| `1` | `LegacySsoUpdateUnexpectedStatusError` — non-2xx from GET or PUT | -| `1` | `LegacySsoUpdateNetworkError` — transport-level failure | +| Code | Condition | +| ---- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `0` | success | +| `1` | `LegacySsoInvalidFlagValueError` — a `--skip-url-validation`/`--name-id-format` occurrence pflag's `Value.Set` would reject (`strconv.ParseBool` / enum membership; fails before every validation; no request) | +| `1` | `LegacySsoFlagNeedsArgumentError` — a bare value-taking flag is the final argv token (pflag `ValueRequiredError`, fails before `ValidateArgs`; no request) | +| `1` | `LegacySsoUpdateArityError` — pflag-effective positional count ≠ 1 (cobra `ValidateArgs`/`ExactArgs(1)`; a consumed flag token orphans its parser-value into the positionals) | +| `1` | `LegacySsoProfileError` — the pflag/viper-effective `--profile`/`SUPABASE_PROFILE` cannot be loaded the way Go's `LoadProfile` loads it (root `PersistentPreRunE`, before `ChangeWorkDir`; loses to the arity check, beats the workdir and mutex checks; no request) | +| `1` | `LegacySsoWorkdirError` — the pflag/viper-effective `--workdir`/`SUPABASE_WORKDIR` is not an existing directory (Go `ChangeWorkDir` in root `PersistentPreRunE`; loses to the arity check, beats the mutex checks; no request) | +| `1` | `LegacySsoInvalidUuidError` — provider ID is not a canonical UUID | +| `1` | `LegacySsoMutexFlagError` — flag combinations: `--domains` with `--add/--remove-domains`, or `--metadata-file` with `--metadata-url` | +| `1` | `LegacySsoUpdateMetadataFileError` — metadata file unreadable, non-UTF-8, or metadata URL invalid/unreachable/non-UTF-8 | +| `1` | `LegacySsoUpdateAttributeMappingFileError` — JSON file unreadable or malformed | +| `1` | `LegacySsoUpdateNotFoundError` — 404 from GET | +| `1` | `LegacySsoUpdateUnexpectedStatusError` — non-2xx from GET or PUT | +| `1` | `LegacySsoUpdateNetworkError` — transport-level failure | ## Telemetry Events Fired @@ -81,6 +86,11 @@ Single `success` event with the parsed response as data. - `--domains` is mutually exclusive with `--add-domains` and `--remove-domains`. - `--metadata-file` and `--metadata-url` are mutually exclusive. +- Flag values follow pflag's consumption rules, not the TS parser's: every value the handler acts on (`--project-ref`, `--metadata-file`, `--metadata-url`, `--attribute-mapping-file`, the three domain slices, `--name-id-format`, `--skip-url-validation`) is reconciled against a pflag-faithful raw-argv scan — same mechanism as `sso add` (CLI-1982). Repeated flags resolve last-wins (pflag Sets every occurrence; the TS parser is first-wins), and an occurrence pflag's `Value.Set` would reject — a boolean outside Go's `strconv.ParseBool` set (`--skip-url-validation=yes`), or a `--name-id-format` outside the enum — fails with pflag's exact `invalid argument …` message before any validation or request. +- Positional arity follows pflag too: cobra's `ExactArgs(1)` is re-counted over pflag-effective positionals (`ValidateArgs` runs before every hook and flag validation), so a consumed flag token that orphans its parser-value (`--domains --metadata-url u `) fails with cobra's exact `accepts 1 arg(s), received 2` before the GET — and wins over both the mutex and invalid-UUID errors. +- The workdir follows pflag/viper too: Go's `ChangeWorkDir` (root `PersistentPreRunE`) chdir's to the effective `--workdir` (last occurrence, even a flag-shaped consumed token like `--workdir --metadata-file`) or `SUPABASE_WORKDIR`, and a missing directory aborts with Go's exact `failed to change workdir: chdir …` after the arity check but before the mutex checks and any request. A changed-but-empty `--workdir=` shadows the env var and falls back to the always-valid project-root walk-up, exactly like viper. +- The profile follows pflag/viper too (PR #5974 round 7): whenever the pflag-effective `--profile`/`SUPABASE_PROFILE` token differs from the one the Effect parser gave the config layer (repeats — pflag is last-wins where the parser is first-wins, so `--profile a.yml --profile b.yml` GETs and PUTs `b.yml`'s `api_url`; a flag-shaped consumed value — `--profile --add-domains`; an explicit `--profile supabase` shadowing the env; an untrimmed/empty persisted `~/.supabase/profile` file), the handler re-runs Go's `LoadProfile` on the effective token (`sso.load-profile.ts`). Both the initial GET and the PUT then target that profile's `api_url` — the GET is issued through the raw HTTP client because the typed client bakes the layer's `api_url` in at construction — and a token Go cannot load aborts with Go's error (`failed to read profile: …` / `failed to parse profile: …` / `invalid profile: …`, byte-exact for the deterministic classes) after the arity check, before the workdir and mutex checks and any request. The raw GET mirrors Go's generated client on the 200 path too: an undecodable JSON body aborts with `failed to get sso provider: ` before any PUT (`update.go:42-45`; detail text is JS `JSON.parse`'s — micro-divergence), a 200 without a JSON content type falls into the gate + unexpected-status branch like Go's nil `JSON200`, and the response is stitched through the shared per-command identity guard exactly like the typed client (Go's `identityTransport` wraps every Management API response). The upgrade-gate fallback GETs and the linked-project cache fill also target the reconciled host (Go's `CurrentProfile` is process-wide). Where the scan and the parser agree — every normal invocation — the config layer's resolution (including its pre-existing lenient missing/malformed-file fallback, which predates CLI-1982 and applies shell-wide) is used unchanged, via the typed client. +- Accepted micro-divergences of the profile emulation (each fail-closed: both CLIs exit 1 with zero requests; only stderr detail can differ): YAML parse-failure detail text (JS `yaml` vs go-yaml, shared `failed to read profile: While parsing config: ` prefix); non-YAML/JSON viper config types (`.toml`, `.env`, …) parsed as YAML; `http_url`/`hostname_rfc1123`/`uuid4` validator tags approximated; the final line of a padded multi-line error loses its trailing spaces to the shared error normalizer's trim. Also: when the effective and layer profiles differ AND the token is keyring-relevant, the keyring token lookup still uses the layer profile's name (env-token flows, e.g. the cli-e2e harness, are unaffected), and the upgrade-suggestion billing URL keeps the layer profile's dashboard host. - Always performs the GET pre-check (matches Go's `update.go:42`), regardless of whether `--add-domains` / `--remove-domains` are used. - Domain merge: removals are applied first, then additions. Go uses a `map[string]bool` so the resulting order is **unordered**; consumers must sort if comparing. - **`domains` is always present in the PUT body** (CLI-1981): Go's `--add-domains`/`--remove-domains` default to a non-nil `[]string{}` (`cmd/sso.go:171-172`), so `update.go:84`'s `!= nil` merge gate is always true from the CLI. With no domain flags (or an explicit empty `--domains=`) the body carries the recomputed existing set; when the provider has no domains it is the literal `"domains":[]` (non-nil `*[]string` under `omitempty` — verified by live capture against the Go binary). diff --git a/apps/cli/src/legacy/commands/sso/update/update.command.ts b/apps/cli/src/legacy/commands/sso/update/update.command.ts index 2f61eecc19..9e76775c4b 100644 --- a/apps/cli/src/legacy/commands/sso/update/update.command.ts +++ b/apps/cli/src/legacy/commands/sso/update/update.command.ts @@ -5,15 +5,9 @@ import { withJsonErrorHandling } from "../../../../shared/output/json-error-hand import { legacyManagementApiRuntimeLayer } from "../../../shared/legacy-management-api-runtime.layer.ts"; import { legacyParseStringSliceFlag } from "../../../shared/legacy-string-slice-flag.ts"; import { withLegacyCommandInstrumentation } from "../../../telemetry/legacy-command-instrumentation.ts"; +import { LEGACY_SSO_NAME_ID_FORMATS } from "../sso.saml.ts"; import { legacySsoUpdate } from "./update.handler.ts"; -const NAME_ID_FORMATS = [ - "urn:oasis:names:tc:SAML:1.1:nameid-format:emailAddress", - "urn:oasis:names:tc:SAML:1.1:nameid-format:unspecified", - "urn:oasis:names:tc:SAML:2.0:nameid-format:persistent", - "urn:oasis:names:tc:SAML:2.0:nameid-format:transient", -] as const; - export const legacySsoUpdateDomainsFlag = Flag.string("domains").pipe( Flag.atLeast(0), Flag.withDescription("Replace domains with this comma separated list of email domains."), @@ -77,7 +71,7 @@ const config = { ), Flag.optional, ), - nameIdFormat: Flag.choice("name-id-format", NAME_ID_FORMATS).pipe( + nameIdFormat: Flag.choice("name-id-format", LEGACY_SSO_NAME_ID_FORMATS).pipe( Flag.withDescription( "URI reference representing the classification of string-based identifier information.", ), diff --git a/apps/cli/src/legacy/commands/sso/update/update.handler.ts b/apps/cli/src/legacy/commands/sso/update/update.handler.ts index 77e86d906f..7fc315aa77 100644 --- a/apps/cli/src/legacy/commands/sso/update/update.handler.ts +++ b/apps/cli/src/legacy/commands/sso/update/update.handler.ts @@ -1,15 +1,18 @@ import type { SupabaseApiError } from "@supabase/api/effect"; -import { Effect, Option, Result, Stdio } from "effect"; +import { Effect, Option, Redacted, Result, Stdio } from "effect"; import * as HttpClient from "effect/unstable/http/HttpClient"; import * as HttpClientRequest from "effect/unstable/http/HttpClientRequest"; import { LegacyPlatformApi } from "../../../auth/legacy-platform-api.service.ts"; import { LegacyCliConfig } from "../../../config/legacy-cli-config.service.ts"; +import { LegacyIdentityStitch } from "../../../shared/legacy-identity-stitch.ts"; import { LegacyProjectRefResolver } from "../../../config/legacy-project-ref.service.ts"; import { LegacyOutputFlag } from "../../../../shared/legacy/global-flags.ts"; import { cobraMutuallyExclusiveErrorMessage, - hasExplicitValueFlag, + PERSISTENT_VALUE_FLAG_NAMES, + PERSISTENT_VALUE_FLAG_SHORTHANDS, + pflagArgvScan, } from "../../../../shared/cli/cobra-flag-groups.ts"; import { Output } from "../../../../shared/output/output.service.ts"; import { @@ -20,6 +23,8 @@ import { } from "../../../shared/legacy-go-output.encoders.ts"; import { mapLegacyHttpError, sanitizeLegacyErrorBody } from "../../../shared/legacy-http-errors.ts"; import { resolveLegacyAccessToken } from "../../../shared/legacy-resolve-token.ts"; +import { legacyAccessTokenForProfile } from "../../../auth/legacy-credentials.layer.ts"; +import { legacyMissingAccessTokenMessage } from "../../../auth/legacy-access-token.ts"; import { LegacyLinkedProjectCache } from "../../../telemetry/legacy-linked-project-cache.service.ts"; import { LegacyTelemetryState } from "../../../telemetry/legacy-telemetry-state.service.ts"; import { @@ -27,16 +32,32 @@ import { legacySuggestUpgrade, } from "../../../shared/legacy-upgrade-suggest.ts"; import { + LegacySsoFlagNeedsArgumentError, + LegacySsoInvalidFlagValueError, LegacySsoMutexFlagError, + LegacySsoUpdateArityError, LegacySsoUpdateAttributeMappingFileError, LegacySsoUpdateMetadataFileError, LegacySsoUpdateNetworkError, LegacySsoUpdateNotFoundError, LegacySsoUpdateUnexpectedStatusError, + LegacySsoAccessTokenError, } from "../sso.errors.ts"; import { renderSingleProvider, toLegacySsoProviderView, validateUuid } from "../sso.format.ts"; import { validateMetadataUrl } from "../sso.metadata-url.ts"; -import { readAttributeMappingFile, readMetadataFile } from "../sso.saml.ts"; +import { + legacySsoPflagBoolValue, + legacySsoPflagEnumValue, + legacySsoPflagSliceValue, + legacySsoPflagStringValue, + legacySsoResolvePflagProfile, + legacySsoValidatePflagWorkdir, +} from "../sso.pflag-reconcile.ts"; +import { + LEGACY_SSO_NAME_ID_FORMATS, + readAttributeMappingFile, + readMetadataFile, +} from "../sso.saml.ts"; import type { LegacySsoUpdateFlags } from "./update.command.ts"; const readMetadata = readMetadataFile({ @@ -72,22 +93,29 @@ const SSO_UPDATE_MUTEX_GROUPS = [ ] as const; /** - * Every value-taking (non-boolean) flag `sso update` declares - * (`update.command.ts`) — tells `hasExplicitValueFlag` which bare tokens - * consume the next argv token as their value. `--skip-url-validation` is this - * command's only boolean flag and is deliberately excluded; booleans never - * consume a following token. + * Every value-taking (non-boolean) flag reachable when `sso update` parses: + * the command's own (`update.command.ts`) plus the root's persistent value + * flags — these tell `pflagArgvScan` which bare tokens consume the next argv + * token as their value (and therefore which tokens are pflag-effective + * positionals). `--skip-url-validation` is this command's only boolean flag + * and is deliberately excluded; booleans never consume a following token. + * `sso update` declares no shorthands of its own (`cmd/sso.go:170-176`), so + * only the persistent `-o` is mapped. */ -const SSO_UPDATE_VALUE_FLAG_NAMES = new Set([ - "project-ref", - "domains", - "add-domains", - "remove-domains", - "metadata-file", - "metadata-url", - "attribute-mapping-file", - "name-id-format", -]); +const SSO_UPDATE_SCAN_SPEC = { + valueFlagNames: new Set([ + "project-ref", + "domains", + "add-domains", + "remove-domains", + "metadata-file", + "metadata-url", + "attribute-mapping-file", + "name-id-format", + ...PERSISTENT_VALUE_FLAG_NAMES, + ]), + valueFlagShorthands: PERSISTENT_VALUE_FLAG_SHORTHANDS, +} as const; const handleGetError = (ref: string, providerId: string, cause: SupabaseApiError) => Effect.gen(function* () { @@ -114,6 +142,28 @@ interface ExistingDomainItem { readonly domain?: string; } +/** + * Narrows a raw GET-provider JSON body to the `domains` shape `mergeDomains` + * consumes — the untyped counterpart of the generated client's provider + * schema, for the reconciled-profile GET path. + */ +function extractDomainItems(parsed: unknown): ReadonlyArray | undefined { + if (parsed === null || typeof parsed !== "object") { + return undefined; + } + const domains = (parsed as Record)["domains"]; + if (!Array.isArray(domains)) { + return undefined; + } + return domains.map((item): ExistingDomainItem => { + if (item === null || typeof item !== "object") { + return {}; + } + const domain = (item as Record)["domain"]; + return typeof domain === "string" ? { domain } : {}; + }); +} + function mergeDomains( existing: ReadonlyArray | undefined, add: ReadonlyArray, @@ -148,15 +198,17 @@ export const legacySsoUpdate = Effect.fn("legacy.sso.update")(function* ( const resolver = yield* LegacyProjectRefResolver; const linkedProjectCache = yield* LegacyLinkedProjectCache; const telemetryState = yield* LegacyTelemetryState; + const identityStitch = yield* Effect.serviceOption(LegacyIdentityStitch); const stdio = yield* Stdio.Stdio; const rawArgs = yield* stdio.args; yield* Effect.gen(function* () { - // cobra runs `ValidateFlagGroups` (`command.go:1010`) before `RunE` - // (`command.go:1014`), and Go's provider-ID format check lives inside - // `RunE` (`cmd/sso.go:90-91`) — so a mutex violation must win over an - // invalid provider ID when both apply. Keep this block ahead of - // `validateUuid` below to match that precedence. + // cobra runs `ValidateArgs` (`command.go:968`, before every hook), then + // `ValidateFlagGroups` (`command.go:1010`), before `RunE` + // (`command.go:1015`), and Go's provider-ID format check lives inside + // `RunE` (`cmd/sso.go:90-91`) — so an arity violation must win over a + // mutex violation, and both must win over an invalid provider ID. Keep + // this block ahead of `validateUuid` below to match that precedence. // // "Set" follows cobra's `pflag.Changed` — whether the flag was passed at // all — not the resulting value. `--domains`/`--add-domains`/ @@ -165,20 +217,122 @@ export const legacySsoUpdate = Effect.fn("legacy.sso.update")(function* ( // miss it, the same "changed vs truthy" gap CLI-1860 fixed for // `functions download`'s `--use-docker`. // - // `hasExplicitValueFlag` (not the simpler `hasExplicitLongFlag`) is - // required here because every flag in these groups takes a value: a bare - // `--metadata-file --metadata-url` is pflag consuming `--metadata-url` as - // `metadata-file`'s (oddly named) value, not two flags being set — see - // that function's doc comment. - for (const group of SSO_UPDATE_MUTEX_GROUPS) { - const changed = group.filter((flagName) => - hasExplicitValueFlag( - rawArgs, - SSO_UPDATE_COMMAND_PATH, - SSO_UPDATE_VALUE_FLAG_NAMES, - flagName, - ), + // The scan is pflag-faithful: a bare `--metadata-file --metadata-url` is + // pflag consuming `--metadata-url` as `metadata-file`'s (oddly named) + // value, not two flags being set — see `pflagArgvScan`. + const scan = pflagArgvScan(rawArgs, SSO_UPDATE_COMMAND_PATH, SSO_UPDATE_SCAN_SPEC); + const occurrences = scan.occurrences; + + // pflag calls `Value.Set` for every occurrence in argv order, and an + // invalid value fails `ParseFlags` (cobra `command.go:919`) before + // `ValidateArgs`, every hook, and `RunE` — reachable here because the + // Effect parser resolves repeated flags first-wins without validating + // later occurrences (`--name-id-format= --name-id-format=bogus` + // parses) and accepts `yes`/`no`, which `strconv.ParseBool` rejects + // (binary-verified, PR #5974 review round 4). These checks precede the + // missing-value check because a missing value can only arise at the + // final argv token, so every recorded occurrence pflag would reject sits + // earlier in its sequential walk (binary-verified: + // `--skip-url-validation=yes --domains` names the invalid argument, not + // the missing one). Flags are checked in Go registration order + // (`cmd/sso.go:170-176`); when a single argv holds invalid occurrences + // of BOTH flags, pflag names whichever comes first in argv — a + // divergence this fixed order cannot see, accepted as unreachable + // through sane usage. The same helpers yield the pflag-effective + // (last-occurrence) values the handler acts on below. + const skipUrlValidation = yield* Result.match( + legacySsoPflagBoolValue(occurrences, "skip-url-validation"), + { + onFailure: (message: string) => + Effect.fail(new LegacySsoInvalidFlagValueError({ message })), + onSuccess: Effect.succeed, + }, + ); + const nameIdFormat = yield* Result.match( + legacySsoPflagEnumValue(occurrences, "name-id-format", LEGACY_SSO_NAME_ID_FORMATS), + { + onFailure: (message: string) => + Effect.fail(new LegacySsoInvalidFlagValueError({ message })), + onSuccess: Effect.succeed, + }, + ); + + // pflag fails `ParseFlags` (cobra `command.go:919`) when a bare + // value-taking flag is the final token (`sso update --domains`) — + // before `ValidateArgs`, every hook, and `RunE`, so Go reports the + // missing argument even when the arg count is also wrong + // (binary-verified: `sso update a b --domains`). The Effect parser + // accepts that argv (the flag parses as unset), so no GET/PUT may happen + // here either. Keep this ahead of the arity check. + if (scan.missingValueError !== undefined) { + return yield* Effect.fail( + new LegacySsoFlagNeedsArgumentError({ message: scan.missingValueError }), + ); + } + + // `ExactArgs(1)` (`cmd/sso.go:87`) counts pflag-effective positionals, + // which shift away from what the Effect parser saw whenever pflag + // consumed a flag token as a value: `--domains --metadata-url u ` is + // pflag handing `--metadata-url` to `--domains` and leaving BOTH `u` and + // `` positional — Go rejects the arg count before any hook, flag + // validation, or request. The parser's own arity check can't see this, + // so re-count from the scan (gated on `anchored`: an unscoped scan has + // no positional information). + if (scan.anchored && scan.positionals.length !== 1) { + return yield* Effect.fail( + new LegacySsoUpdateArityError({ + message: `accepts 1 arg(s), received ${scan.positionals.length}`, + }), ); + } + + // Go's root `PersistentPreRunE` loads the pflag/viper-effective + // `--profile`/`SUPABASE_PROFILE` (`LoadProfile`, `cmd/root.go:98-102`) + // immediately BEFORE `ChangeWorkDir`, so an unloadable profile loses to + // an arity violation but beats the workdir check, the mutex checks, and + // any GET/PUT — and a loadable one decides which API host receives them. + // Reachable exactly where the scan and the parser disagree (see + // `add.handler.ts` and `legacySsoResolvePflagProfile` — PR #5974 + // review round 7); where they agree this is `none` and the config + // layer's client/apiUrl below are already pflag-effective. + const reconciledProfile = yield* legacySsoResolvePflagProfile(scan); + const profileApiUrl = Option.map(reconciledProfile, (profile) => profile.apiUrl); + // Reconciled-profile credentials, resolved ONCE for the main request and + // every auxiliary call (linked-project cache fill, upgrade-gate fallback + // GETs): Go's reconciled `CurrentProfile` + `GetAccessToken` apply + // process-wide (`access_token.go:43`, review r3684524241). `undefined` + // when the scan and the parser agree — every consumer then resolves from + // the config-layer services as before. + // Reconciled-profile credentials, resolved LAZILY (memoized) so the first + // read happens at the request site — Go's token gate is `GetSupabase` + // inside RunE (`api.go:119-124`), AFTER required/mutex/workdir + // validation, so a missing or invalid reconciled token must not pre-empt + // those errors (review r3686720488). Missing → Go's ErrMissingToken; + // invalid → ErrInvalidToken (the validation failure propagates). The + // auxiliary calls (cache fill, upgrade-gate GETs) use the absorbed + // variant: failures skip like Go's best-effort `ensureProjectGroupsCached`. + const reconciledTokenCached = Option.isSome(reconciledProfile) + ? yield* Effect.cached(legacyAccessTokenForProfile(reconciledProfile.value.name)) + : undefined; + const reconciledTokenForAux = + reconciledTokenCached === undefined + ? Effect.succeed> | undefined>(undefined) + : Effect.catch(reconciledTokenCached, () => + Effect.succeed(Option.none>()), + ); + + // Go's root `PersistentPreRunE` chdir's to the pflag/viper-effective + // `--workdir`/`SUPABASE_WORKDIR` (`ChangeWorkDir`, `cmd/root.go:104`, + // `internal/utils/misc.go:238-257`) after `ValidateArgs` and before + // `ValidateFlagGroups` (`command.go:1010`), so a missing directory loses + // to an arity violation but beats a mutex violation and any GET/PUT + // (binary-verified: `sso update a b --workdir /missing` reports the + // arity error; `sso update --workdir /missing --domains a + // --add-domains b` reports the chdir failure — PR #5974 review round 6). + yield* legacySsoValidatePflagWorkdir(scan); + + for (const group of SSO_UPDATE_MUTEX_GROUPS) { + const changed = group.filter((flagName) => occurrences.has(flagName)); if (changed.length > 1) { return yield* Effect.fail( new LegacySsoMutexFlagError({ @@ -188,30 +342,161 @@ export const legacySsoUpdate = Effect.fn("legacy.sso.update")(function* ( } } + // Reconcile everything the handler acts on to the pflag-effective values + // from the same scan — the Effect parser refuses to consume flag-shaped + // tokens as values while pflag consumes them unconditionally, and + // resolves repeated flags first-wins while pflag is last-wins, so the + // two can disagree on which flags are set and what they hold. See + // `add.handler.ts` and `sso.pflag-reconcile.ts` for the full rationale + // (CLI-1982). `--name-id-format` and `--skip-url-validation` were + // reconciled above, alongside their pflag value validation. + const projectRefFlag = legacySsoPflagStringValue(occurrences, "project-ref"); + const metadataFile = legacySsoPflagStringValue(occurrences, "metadata-file"); + const metadataUrl = legacySsoPflagStringValue(occurrences, "metadata-url"); + const attributeMappingFile = legacySsoPflagStringValue(occurrences, "attribute-mapping-file"); + const domains = legacySsoPflagSliceValue(occurrences, "domains", flags.domains); + const addDomains = legacySsoPflagSliceValue(occurrences, "add-domains", flags.addDomains); + const removeDomains = legacySsoPflagSliceValue( + occurrences, + "remove-domains", + flags.removeDomains, + ); + const providerId = yield* validateUuid(flags.providerId).pipe( Result.match({ onFailure: Effect.fail, onSuccess: Effect.succeed }), ); - const ref = yield* resolver.resolve(flags.projectRef); + const ref = yield* resolver.resolve(projectRefFlag); + + // Effective API base URL: the pflag-reconciled profile's when the scan + // and the parser disagreed on `--profile`, the config layer's otherwise. + const apiUrl = Option.getOrElse(profileApiUrl, () => cliConfig.apiUrl); yield* Effect.gen(function* () { const fetching = output.format === "text" ? yield* output.task("Updating SSO provider...") : undefined; + // The typed client bakes the layer's apiUrl in at construction + // (`legacy-platform-api.layer.ts:73`), so when the reconciled profile + // differs the GET must be issued raw against the effective host — Go + // GETs and PUTs the same viper-effective profile host (`update.go:42`), + // and a GET to the layer's host would be a request Go never makes. The + // error mapping and the spinner-fail/suggestion stderr ordering mirror + // the typed path (`handleGetError`) exactly. + const rawGetProvider = Effect.gen(function* () { + const tokenOpt = + reconciledTokenCached !== undefined + ? yield* Effect.flatMap(reconciledTokenCached, (resolved) => + Option.isSome(resolved) + ? Effect.succeed(resolved) + : Effect.fail( + new LegacySsoAccessTokenError({ message: legacyMissingAccessTokenMessage() }), + ), + ) + : yield* resolveLegacyAccessToken; + const request = HttpClientRequest.get( + `${apiUrl}/v1/projects/${ref}/config/auth/sso/providers/${providerId}`, + ).pipe( + Option.isSome(tokenOpt) ? HttpClientRequest.bearerToken(tokenOpt.value) : (req) => req, + HttpClientRequest.setHeader("User-Agent", cliConfig.userAgent), + ); + const response = yield* httpClient.execute(request).pipe( + Effect.tapError(() => fetching?.fail() ?? Effect.void), + Effect.mapError( + (cause) => + new LegacySsoUpdateNetworkError({ + message: `failed to get sso provider: ${String(cause)}`, + }), + ), + ); + // Go's `identityTransport` wraps EVERY Management API response + // (`cmd/root.go:146-154`); the typed client stitches via its response + // transform (`legacy-platform-api.layer.ts`), so this raw GET must + // stitch through the same once-per-command guard — before the status + // gate, like the linked-project cache's raw GET. `serviceOption`: + // absent outside the real CLI tree (handler-level tests), where no + // telemetry runtime exists to stitch into. + if (Option.isSome(identityStitch)) { + yield* identityStitch.value.stitch(response); + } + // Go's generated `ParseV1GetASsoProviderResponse` reads the body up + // front; the read error surfaces as `failed to get sso provider: %w` + // (`update.go:42-45`). + const rawBody = yield* response.text.pipe( + Effect.tapError(() => fetching?.fail() ?? Effect.void), + Effect.mapError( + (cause) => + new LegacySsoUpdateNetworkError({ + message: `failed to get sso provider: ${String(cause)}`, + }), + ), + ); + const contentType = response.headers["content-type"] ?? ""; + if (response.status === 200 && contentType.includes("json")) { + // Go unmarshals a 200 JSON body and exits with the unmarshal error + // before any PUT (`update.go:42-45`); detail text is JS + // `JSON.parse`'s, not encoding/json's (documented micro-divergence + // — both CLIs exit 1 with no PUT). + let parsed: unknown; + try { + parsed = JSON.parse(rawBody); + } catch (cause) { + yield* fetching?.fail() ?? Effect.void; + return yield* Effect.fail( + new LegacySsoUpdateNetworkError({ + message: `failed to get sso provider: ${cause instanceof Error ? cause.message : String(cause)}`, + }), + ); + } + return { domains: extractDomainItems(parsed) }; + } + // Non-200 — or a 200 without a JSON content type, which leaves Go's + // `JSON200` nil and falls into the same branch (`update.go:47-55`): + // gate check, then 404 / unexpected-status. + yield* fetching?.fail() ?? Effect.void; + const bodyText = sanitizeLegacyErrorBody(rawBody); + yield* legacySuggestUpgrade({ + projectRef: ref, + featureKey: "auth.saml_2", + statusCode: response.status, + response, + apiUrl, + ...(yield* Effect.map(reconciledTokenForAux, (token) => + token !== undefined ? { accessToken: token } : {}, + )), + }); + if (response.status === 404) { + return yield* Effect.fail( + new LegacySsoUpdateNotFoundError({ + message: `An identity provider with ID ${JSON.stringify(providerId)} could not be found.`, + }), + ); + } + return yield* Effect.fail( + new LegacySsoUpdateUnexpectedStatusError({ + status: response.status, + body: bodyText, + message: `unexpected error fetching identity provider: ${bodyText}`, + }), + ); + }); + // Go's `update.go:42` always GETs first, regardless of which flags are set. - const existing = yield* api.v1.getASsoProvider({ ref, provider_id: providerId }).pipe( - Effect.tapError(() => fetching?.fail() ?? Effect.void), - Effect.catch((cause) => handleGetError(ref, providerId, cause)), - ); + const existing = yield* Option.isSome(profileApiUrl) + ? rawGetProvider + : api.v1.getASsoProvider({ ref, provider_id: providerId }).pipe( + Effect.tapError(() => fetching?.fail() ?? Effect.void), + Effect.catch((cause) => handleGetError(ref, providerId, cause)), + ); const body: Record = {}; - if (Option.isSome(flags.metadataFile)) { - const xml = yield* readMetadata(flags.metadataFile.value); + if (Option.isSome(metadataFile)) { + const xml = yield* readMetadata(metadataFile.value); body["metadata_xml"] = xml; - } else if (Option.isSome(flags.metadataUrl)) { - if (!flags.skipUrlValidation) { - yield* validateMetadataUrl(flags.metadataUrl.value).pipe( + } else if (Option.isSome(metadataUrl)) { + if (!skipUrlValidation) { + yield* validateMetadataUrl(metadataUrl.value).pipe( // Go's `update.go:69` wraps the cause with `%w Use --skip-url-validation to // suppress this error.` — note the single space between cause and `Use` and // the trailing period. Go's `create.go:47` uses the same format minus the @@ -224,16 +509,16 @@ export const legacySsoUpdate = Effect.fn("legacy.sso.update")(function* ( ), ); } - body["metadata_url"] = flags.metadataUrl.value; + body["metadata_url"] = metadataUrl.value; } - if (Option.isSome(flags.attributeMappingFile)) { - const mapping = yield* readAttributeMapping(flags.attributeMappingFile.value); + if (Option.isSome(attributeMappingFile)) { + const mapping = yield* readAttributeMapping(attributeMappingFile.value); body["attribute_mapping"] = mapping; } - if (flags.domains.length > 0) { - body["domains"] = [...flags.domains]; + if (domains.length > 0) { + body["domains"] = [...domains]; } else { // Go's `update.go:84` reads as gating the merge on // `params.AddDomains != nil || params.RemoveDomains != nil`, but @@ -244,18 +529,27 @@ export const legacySsoUpdate = Effect.fn("legacy.sso.update")(function* ( // is a non-nil `*[]string` under `json:"domains,omitempty"`, so an // empty merged set serializes as `"domains":[]`, never omitted // (CLI-1981; live-captured against the Go binary). - body["domains"] = mergeDomains(existing.domains, flags.addDomains, flags.removeDomains); + body["domains"] = mergeDomains(existing.domains, addDomains, removeDomains); } - if (Option.isSome(flags.nameIdFormat)) { - body["name_id_format"] = flags.nameIdFormat.value; + if (Option.isSome(nameIdFormat)) { + body["name_id_format"] = nameIdFormat.value; } - const tokenOpt = yield* resolveLegacyAccessToken; + const tokenOpt = + reconciledTokenCached !== undefined + ? yield* Effect.flatMap(reconciledTokenCached, (resolved) => + Option.isSome(resolved) + ? Effect.succeed(resolved) + : Effect.fail( + new LegacySsoAccessTokenError({ message: legacyMissingAccessTokenMessage() }), + ), + ) + : yield* resolveLegacyAccessToken; // See `add.handler.ts` for the rationale behind `bearerToken(Redacted)`. const request = HttpClientRequest.put( - `${cliConfig.apiUrl}/v1/projects/${ref}/config/auth/sso/providers/${providerId}`, + `${apiUrl}/v1/projects/${ref}/config/auth/sso/providers/${providerId}`, ).pipe( Option.isSome(tokenOpt) ? HttpClientRequest.bearerToken(tokenOpt.value) : (req) => req, HttpClientRequest.setHeader("User-Agent", cliConfig.userAgent), @@ -283,6 +577,10 @@ export const legacySsoUpdate = Effect.fn("legacy.sso.update")(function* ( featureKey: "auth.saml_2", statusCode: response.status, response, + apiUrl, + ...(yield* Effect.map(reconciledTokenForAux, (token) => + token !== undefined ? { accessToken: token } : {}, + )), }); yield* fetching?.fail() ?? Effect.void; return yield* Effect.fail( @@ -327,6 +625,16 @@ export const legacySsoUpdate = Effect.fn("legacy.sso.update")(function* ( } yield* output.raw(renderSingleProvider(toLegacySsoProviderView(parsedJson))); - }).pipe(Effect.ensuring(linkedProjectCache.cache(ref))); + }).pipe( + // Go's `ensureProjectGroupsCached` GETs `/v1/projects/{ref}` through the + // process-wide `CurrentProfile` — the reconciled host, never the layer's. + Effect.ensuring( + // Resolved INSIDE the ensuring effect — the memoized token read must + // not run before the handler body (Go's gate order, see above). + Effect.flatMap(reconciledTokenForAux, (token) => + linkedProjectCache.cache(ref, undefined, Option.getOrUndefined(profileApiUrl), token), + ), + ), + ); }).pipe(Effect.ensuring(telemetryState.flush)); }); diff --git a/apps/cli/src/legacy/commands/sso/update/update.integration.test.ts b/apps/cli/src/legacy/commands/sso/update/update.integration.test.ts index 757e7b46de..8c057f87e7 100644 --- a/apps/cli/src/legacy/commands/sso/update/update.integration.test.ts +++ b/apps/cli/src/legacy/commands/sso/update/update.integration.test.ts @@ -2,7 +2,7 @@ import { writeFileSync } from "node:fs"; import { join } from "node:path"; import { describe, expect, it } from "@effect/vitest"; -import { Effect, Exit, Layer, Option, Stdio } from "effect"; +import { Effect, Exit, Layer, Option, Redacted, Stdio } from "effect"; import * as HttpClientResponse from "effect/unstable/http/HttpClientResponse"; import { mockAnalytics, mockOutput } from "../../../../../tests/helpers/mocks.ts"; @@ -15,6 +15,8 @@ import { mockLegacyTelemetryStateTracked, useLegacyTempWorkdir, } from "../../../../../tests/helpers/legacy-mocks.ts"; +import { LegacyProfileFlag } from "../../../../shared/legacy/global-flags.ts"; +import { LegacyIdentityStitch } from "../../../shared/legacy-identity-stitch.ts"; import { EventUpgradeSuggested } from "../../../../shared/telemetry/event-catalog.ts"; import { legacySsoUpdate } from "./update.handler.ts"; @@ -42,16 +44,36 @@ interface SetupOpts { goOutput?: "env" | "pretty" | "json" | "toml" | "yaml"; getStatus?: number; getBody?: unknown; + /** + * Serves the provider GET as a raw body + content type instead of + * `jsonResponse` — for the reconciled-profile raw GET's decode branches + * (invalid JSON, non-JSON content type). + */ + getRaw?: { status: number; body: string; contentType: string }; putStatus?: number; putBody?: unknown; upgradeGate?: "gated" | "notGated"; /** - * Raw argv the handler sees via `Stdio.Stdio` — drives the - * `hasExplicitLongFlag`-based mutex checks. Defaults to a bare invocation - * with none of the mutually-exclusive domain flags present; tests that - * exercise those checks must pass the matching flags explicitly here. + * Raw argv the handler sees via `Stdio.Stdio` — drives the pflag-faithful + * scan (`pflagArgvScan`) behind the arity check, the mutex checks, and the + * value reconciliation. Defaults to a bare invocation with no optional + * flags present; tests that pass flags must pass matching argv here + * (usually via `cliArgsFor`), exactly as the real parser guarantees. */ cliArgs?: ReadonlyArray; + /** + * The Effect-parsed `--profile` value (`LegacyProfileFlag`), which the real + * parser sets for any `--profile` it accepted. Tests whose `cliArgs` carry a + * `--profile` the parser would have consumed must provide it, exactly as the + * real CLI tree would. + */ + profileFlag?: string; + /** + * Overrides the config layer's env-shaped access token. `Option.none()` + * models a machine with no SUPABASE_ACCESS_TOKEN — with a reconciled + * profile and no keyring/file token, Go's `GetSupabase` gate aborts. + */ + accessToken?: Option.Option>; } function jsonResponse( @@ -84,8 +106,20 @@ function setup(opts: SetupOpts = {}) { handler: (request) => { const url = request.url; if (url.includes("/config/auth/sso/providers/")) { - if (request.method === "GET") + if (request.method === "GET") { + if (opts.getRaw !== undefined) { + return Effect.succeed( + HttpClientResponse.fromWeb( + request, + new Response(opts.getRaw.body, { + status: opts.getRaw.status, + headers: { "content-type": opts.getRaw.contentType }, + }), + ), + ); + } return Effect.succeed(jsonResponse(request, getStatus, getBody)); + } if (request.method === "PUT") return Effect.succeed(jsonResponse(request, putStatus, putBody)); } @@ -128,7 +162,22 @@ function setup(opts: SetupOpts = {}) { }, }); - const cliConfig = mockLegacyCliConfig({ workdir: tempRoot.current }); + // Tracked identity stitcher: the reconciled-profile raw GET must stitch + // through the shared per-command guard exactly like the typed client's + // response transform (Go's identityTransport wraps every response). + let stitchedResponses = 0; + const stitchLayer = Layer.succeed(LegacyIdentityStitch, { + stitch: () => + Effect.sync(() => { + stitchedResponses += 1; + }), + stitchedDistinctId: () => undefined, + }); + + const cliConfig = mockLegacyCliConfig({ + workdir: tempRoot.current, + ...(opts.accessToken !== undefined ? { accessToken: opts.accessToken } : {}), + }); const layer = Layer.mergeAll( buildLegacyTestRuntime({ out, @@ -142,9 +191,23 @@ function setup(opts: SetupOpts = {}) { Stdio.layerTest({ args: Effect.succeed(opts.cliArgs ?? ["sso", "update", VALID_PROVIDER_ID]), }), + stitchLayer, + opts.profileFlag === undefined + ? Layer.empty + : Layer.succeed(LegacyProfileFlag, opts.profileFlag), ); - return { layer, out, api, analytics, telemetry, cache }; + return { + layer, + out, + api, + analytics, + telemetry, + cache, + get stitchedResponses() { + return stitchedResponses; + }, + }; } const defaultFlags = { @@ -165,6 +228,45 @@ const defaultFlags = { providerId: VALID_PROVIDER_ID, }; +/** + * Serializes a flags record into the raw argv the real CLI would have been + * invoked with. The handler reconciles every value it acts on against a + * pflag-faithful scan of this argv, so tests must keep the two consistent — + * a flag passed in the record but absent from argv reconciles to "not set", + * exactly as it would be for a real invocation. + */ +function cliArgsFor(flags: typeof defaultFlags): ReadonlyArray { + const argv: string[] = ["sso", "update", flags.providerId]; + if (Option.isSome(flags.projectRef)) { + argv.push("--project-ref", flags.projectRef.value); + } + for (const domain of flags.domains) { + argv.push("--domains", domain); + } + for (const domain of flags.addDomains) { + argv.push("--add-domains", domain); + } + for (const domain of flags.removeDomains) { + argv.push("--remove-domains", domain); + } + if (Option.isSome(flags.metadataFile)) { + argv.push("--metadata-file", flags.metadataFile.value); + } + if (Option.isSome(flags.metadataUrl)) { + argv.push("--metadata-url", flags.metadataUrl.value); + } + if (flags.skipUrlValidation) { + argv.push("--skip-url-validation"); + } + if (Option.isSome(flags.attributeMappingFile)) { + argv.push("--attribute-mapping-file", flags.attributeMappingFile.value); + } + if (Option.isSome(flags.nameIdFormat)) { + argv.push("--name-id-format", flags.nameIdFormat.value); + } + return argv; +} + describe("legacy sso update integration", () => { it.live("rejects bad UUID", () => { const { layer } = setup(); @@ -412,7 +514,7 @@ describe("legacy sso update integration", () => { ); it.live( - "mutex check: a bare --metadata-file followed by --metadata-url is not a violation", + "mutex check: a bare --metadata-file followed by --metadata-url is not a violation, and the consumed token is the file", () => { // pflag's `--flag arg` branch consumes the very next argv token as the // value unconditionally (`flag.go:1013-1031`), so real cobra parses this @@ -420,150 +522,835 @@ describe("legacy sso update integration", () => { // `metadata-url` is never parsed as its own flag and stays unset. The // TS CLI's own parser (unlike pflag) never hands a dash-prefixed token // to a non-boolean flag as a bare value, so here both flags resolve to - // `Option.none()` — but the raw-argv mutex scan must reach the same - // "not a violation" conclusion pflag does, not double-count the - // `--metadata-url` token as a second explicit flag. - const { layer } = setup({ + // `Option.none()` — the raw-argv scan must reach the same "not a + // violation" conclusion pflag does, and the handler must then behave + // like Go: try to open a file literally named `--metadata-url` instead + // of silently PUTting with no metadata at all. + const { layer, api } = setup({ cliArgs: ["sso", "update", VALID_PROVIDER_ID, "--metadata-file", "--metadata-url"], }); + return Effect.gen(function* () { + const exit = yield* Effect.exit(legacySsoUpdate(defaultFlags)); + expect(Exit.isFailure(exit)).toBe(true); + if (Exit.isFailure(exit)) { + const dump = JSON.stringify(exit.cause); + expect(dump).toContain("LegacySsoUpdateMetadataFileError"); + expect(dump).toContain("failed to open metadata file"); + } + expect(api.requests.some((r) => r.method === "PUT")).toBe(false); + }).pipe(Effect.provide(layer)); + }, + ); + + it.live( + "mutex check: a bare --add-domains followed by --domains=... is not a violation, and the consumed token is the domain", + () => { + // Same consumed-value class as the metadata-file/metadata-url case + // above, but for the domains group: pflag hands `add-domains` the + // literal value `"--domains=x.com"` and never parses `--domains` at + // all — so Go merges that odd-looking string into the existing domain + // list and PUTs it. The reconciled handler must produce the same body. + const { layer, api } = setup({ + cliArgs: ["sso", "update", VALID_PROVIDER_ID, "--add-domains", "--domains=x.com"], + }); return Effect.gen(function* () { const exit = yield* Effect.exit(legacySsoUpdate(defaultFlags)); expect(Exit.isSuccess(exit)).toBe(true); + const putReq = api.requests.find((r) => r.method === "PUT"); + const domains = (putReq?.body as { domains: string[] })?.domains; + expect([...domains].sort()).toEqual(["--domains=x.com", "old1.com", "old2.com"]); }).pipe(Effect.provide(layer)); }, ); - it.live("mutex check: a bare --add-domains followed by --domains=... is not a violation", () => { - // Same consumed-value class as the metadata-file/metadata-url case - // above, but for the domains group: pflag would hand `add-domains` the - // literal value `"--domains=x.com"` and never parse `--domains` at all. - const { layer } = setup({ - cliArgs: ["sso", "update", VALID_PROVIDER_ID, "--add-domains", "--domains=x.com"], - }); - return Effect.gen(function* () { - const exit = yield* Effect.exit(legacySsoUpdate(defaultFlags)); - expect(Exit.isSuccess(exit)).toBe(true); - }).pipe(Effect.provide(layer)); - }); + it.live( + "arity emulation: project-ref consuming --metadata-file orphans x.xml — fails ExactArgs like Go, no API calls", + () => { + // ` --project-ref --metadata-file x.xml --metadata-url u`: pflag + // hands `--metadata-file` to `--project-ref` as its value, which makes + // `x.xml` a positional — cobra's `ValidateArgs`/`ExactArgs(1)` + // (`command.go:968`, `cmd/sso.go:87`) then rejects the arg count + // before any hook, mutex check, or request. The Effect parser read + // `--metadata-file x.xml` as a normal flag and saw exactly one + // positional, so the handler must re-count from the scan (PR #5974 + // review; this refines the earlier ref-validation expectation — Go + // never even reaches the ref check here). + const { layer, api } = setup({ + cliArgs: [ + "sso", + "update", + VALID_PROVIDER_ID, + "--project-ref", + "--metadata-file", + "x.xml", + "--metadata-url", + "https://idp.example.com/m", + ], + }); + return Effect.gen(function* () { + const exit = yield* Effect.exit( + legacySsoUpdate({ + ...defaultFlags, + metadataFile: Option.some("x.xml"), + metadataUrl: Option.some("https://idp.example.com/m"), + }), + ); + expect(Exit.isFailure(exit)).toBe(true); + if (Exit.isFailure(exit)) { + const dump = JSON.stringify(exit.cause); + expect(dump).toContain("LegacySsoUpdateArityError"); + expect(dump).toContain("accepts 1 arg(s), received 2"); + } + expect(api.requests.length).toBe(0); + }).pipe(Effect.provide(layer)); + }, + ); - it.live("--domains replaces domains verbatim", () => { - const { layer, api } = setup(); - return Effect.gen(function* () { - yield* legacySsoUpdate({ ...defaultFlags, domains: ["new.com"] }); - const putReq = api.requests.find((r) => r.method === "PUT"); - expect((putReq?.body as { domains?: string[] })?.domains).toEqual(["new.com"]); - }).pipe(Effect.provide(layer)); - }); + it.live( + "arity emulation: a bare --domains consuming --metadata-url orphans the URL — fails ExactArgs like Go, no GET/PUT", + () => { + // `--domains --metadata-url https://… `: pflag consumes + // `--metadata-url` as the domains value, leaving BOTH the URL and the + // provider ID positional — Go rejects via `ExactArgs(1)` before any + // request. The Effect parser instead read the URL as metadata-url's + // value and saw one positional, so without the re-count the handler + // would GET and PUT (PR #5974 review, Codex thread). + const { layer, api } = setup({ + cliArgs: [ + "sso", + "update", + "--domains", + "--metadata-url", + "https://idp.example.com/m", + VALID_PROVIDER_ID, + ], + }); + return Effect.gen(function* () { + const exit = yield* Effect.exit( + legacySsoUpdate({ + ...defaultFlags, + metadataUrl: Option.some("https://idp.example.com/m"), + }), + ); + expect(Exit.isFailure(exit)).toBe(true); + if (Exit.isFailure(exit)) { + const dump = JSON.stringify(exit.cause); + expect(dump).toContain("LegacySsoUpdateArityError"); + expect(dump).toContain("accepts 1 arg(s), received 2"); + } + expect(api.requests.length).toBe(0); + }).pipe(Effect.provide(layer)); + }, + ); - it.live("--add-domains merges with existing GET domains", () => { - const { layer, api } = setup(); - return Effect.gen(function* () { - yield* legacySsoUpdate({ ...defaultFlags, addDomains: ["new.com"] }); - const putReq = api.requests.find((r) => r.method === "PUT"); - const domains = (putReq?.body as { domains: string[] })?.domains; - // Go map iteration is unordered — sort before asserting. - expect([...domains].sort()).toEqual(["new.com", "old1.com", "old2.com"]); - }).pipe(Effect.provide(layer)); - }); + it.live( + "arity emulation: a bare --domains consuming a persistent global flag orphans its value", + () => { + // Binary-verified Go behaviour: `--domains --profile staging ` + // arity-errors because pflag hands `--profile` to `--domains` and + // `staging` becomes positional. The scan must know the root's + // persistent value flags (`cmd/root.go:324-333`) to see this. + const { layer, api } = setup({ + cliArgs: ["sso", "update", "--domains", "--profile", "staging", VALID_PROVIDER_ID], + }); + return Effect.gen(function* () { + const exit = yield* Effect.exit(legacySsoUpdate(defaultFlags)); + expect(Exit.isFailure(exit)).toBe(true); + if (Exit.isFailure(exit)) { + const dump = JSON.stringify(exit.cause); + expect(dump).toContain("LegacySsoUpdateArityError"); + expect(dump).toContain("accepts 1 arg(s), received 2"); + } + expect(api.requests.length).toBe(0); + }).pipe(Effect.provide(layer)); + }, + ); - it.live("--remove-domains strips from existing GET domains", () => { - const { layer, api } = setup(); - return Effect.gen(function* () { - yield* legacySsoUpdate({ ...defaultFlags, removeDomains: ["old1.com"] }); - const putReq = api.requests.find((r) => r.method === "PUT"); - const domains = (putReq?.body as { domains: string[] })?.domains; - expect([...domains].sort()).toEqual(["old2.com"]); - }).pipe(Effect.provide(layer)); - }); + it.live( + "arity emulation: persistent global value flags and -o do not miscount positionals", + () => { + // Regression guards for the re-count: pflag consumes these globals' + // values (`--workdir .`, `--output-format json`, `-o json`), so none + // of them may register as a second positional — each invocation must + // sail through to the PUT exactly as before. + const argvVariants: ReadonlyArray> = [ + ["sso", "update", "--workdir", ".", VALID_PROVIDER_ID], + ["sso", "update", "--output-format", "json", VALID_PROVIDER_ID], + ["sso", "update", "-o", "json", VALID_PROVIDER_ID], + ]; + return Effect.gen(function* () { + for (const cliArgs of argvVariants) { + const { layer, api } = setup({ cliArgs }); + yield* legacySsoUpdate(defaultFlags).pipe(Effect.provide(layer)); + expect(api.requests.some((r) => r.method === "PUT")).toBe(true); + } + }); + }, + ); - it.live("no domain flag set → PUT still sends the recomputed existing domain set", () => { - // Go's `--add-domains`/`--remove-domains` default to a non-nil `[]string{}` - // (`cmd/sso.go:171-172`), so `update.go:84`'s `!= nil` gate is always true - // from the CLI — every `sso update` enters the merge and sends `domains`, - // even when no domain flag was passed (CLI-1981). Live-captured Go PUT: - // `{"domains":["old1.com","old2.com"]}`. - const { layer, api } = setup(); + it.live("arity emulation: the arity error wins over a mutex violation", () => { + // cobra's `ValidateArgs` (`command.go:968`) runs before + // `ValidateFlagGroups` (`command.go:1010`): with `--domains` + + // `--add-domains` both set AND `--metadata-file` swallowing + // `--metadata-url` (orphaning `u` as a second positional), Go reports + // the arg-count error, not the mutex template. + const { layer, api } = setup({ + cliArgs: [ + "sso", + "update", + "--domains", + "a.com", + "--add-domains", + "b.com", + "--metadata-file", + "--metadata-url", + "u", + VALID_PROVIDER_ID, + ], + }); return Effect.gen(function* () { - yield* legacySsoUpdate(defaultFlags); - const putReq = api.requests.find((r) => r.method === "PUT"); - const domains = (putReq?.body as { domains: string[] })?.domains; - // Go map iteration is unordered — sort before asserting. - expect([...domains].sort()).toEqual(["old1.com", "old2.com"]); + const exit = yield* Effect.exit( + legacySsoUpdate({ + ...defaultFlags, + domains: ["a.com"], + addDomains: ["b.com"], + metadataUrl: Option.some("u"), + }), + ); + expect(Exit.isFailure(exit)).toBe(true); + if (Exit.isFailure(exit)) { + const dump = JSON.stringify(exit.cause); + expect(dump).toContain("LegacySsoUpdateArityError"); + expect(dump).not.toContain("LegacySsoMutexFlagError"); + } + expect(api.requests.length).toBe(0); }).pipe(Effect.provide(layer)); }); it.live( - "no domain flags + provider with no domains → PUT sends domains: [] (not omitted)", + "workdir emulation: --workdir consuming a trailing --metadata-file fails at Go's chdir, no GET/PUT", () => { - // Go sets `body.Domains` to a non-nil pointer to `make([]string, 0)`, and - // `json:"domains,omitempty"` never omits a non-nil pointer — live-captured - // Go PUT body is exactly `{"domains":[]}`. - const { layer, api } = setup({ getBody: { ...EXISTING_PROVIDER, domains: [] } }); + // `sso update --project-ref --workdir --metadata-file`: + // pflag binds `"--metadata-file"` to the persistent `--workdir` (the + // positional count stays 1) and Go's `ChangeWorkDir` + // (`cmd/root.go:104`, `misc.go:238-257`) exits before `RunE` with zero + // HTTP traffic. The Effect parser refused the flag-shaped value and + // left both flags unset — without the workdir emulation the handler + // proceeded to GET + PUT (binary-verified, PR #5974 review round 6). + const { layer, api } = setup({ + cliArgs: [ + "sso", + "update", + VALID_PROVIDER_ID, + "--project-ref", + LEGACY_VALID_REF, + "--workdir", + "--metadata-file", + ], + }); return Effect.gen(function* () { - yield* legacySsoUpdate(defaultFlags); - const putReq = api.requests.find((r) => r.method === "PUT"); - const body = putReq?.body as Record; - expect(Object.keys(body)).toContain("domains"); - expect(body["domains"]).toEqual([]); + const exit = yield* Effect.exit( + legacySsoUpdate({ ...defaultFlags, projectRef: Option.some(LEGACY_VALID_REF) }), + ); + expect(Exit.isFailure(exit)).toBe(true); + if (Exit.isFailure(exit)) { + const dump = JSON.stringify(exit.cause); + expect(dump).toContain("LegacySsoWorkdirError"); + expect(dump).toContain( + "failed to change workdir: chdir --metadata-file: no such file or directory", + ); + } + expect(api.requests.length).toBe(0); }).pipe(Effect.provide(layer)); }, ); - it.live("no domain flags + GET response missing domains entirely → PUT sends domains: []", () => { - // Go's seed loop is skipped when `getResp.JSON200.Domains == nil`, leaving - // the merged set empty — same `{"domains":[]}` bytes as the empty-list case. - const { domains: _omitted, ...providerWithoutDomains } = EXISTING_PROVIDER; - const { layer, api } = setup({ getBody: providerWithoutDomains }); - return Effect.gen(function* () { - yield* legacySsoUpdate(defaultFlags); - const putReq = api.requests.find((r) => r.method === "PUT"); - const body = putReq?.body as Record; - expect(Object.keys(body)).toContain("domains"); - expect(body["domains"]).toEqual([]); - }).pipe(Effect.provide(layer)); - }); + it.live( + "workdir emulation: the chdir failure loses to an arity violation but wins over a mutex violation", + () => { + // Go's `ChangeWorkDir` runs from `PersistentPreRunE` (`command.go:986`) + // — after `ValidateArgs` (`command.go:968`), before + // `ValidateFlagGroups` (`command.go:1010`). Binary-verified: `sso + // update a b --workdir /missing` reports the arity error, while `sso + // update --workdir /missing --domains a --add-domains b` reports + // the chdir failure (PR #5974 review round 6). + const { layer, api } = setup({ + cliArgs: [ + "sso", + "update", + VALID_PROVIDER_ID, + "--workdir", + "/nonexistent-sso-update-workdir", + "--domains", + "a.com", + "--add-domains", + "b.com", + ], + }); + return Effect.gen(function* () { + const exit = yield* Effect.exit( + legacySsoUpdate({ ...defaultFlags, domains: ["a.com"], addDomains: ["b.com"] }), + ); + expect(Exit.isFailure(exit)).toBe(true); + if (Exit.isFailure(exit)) { + const dump = JSON.stringify(exit.cause); + expect(dump).toContain("LegacySsoWorkdirError"); + expect(dump).toContain( + "failed to change workdir: chdir /nonexistent-sso-update-workdir: no such file or directory", + ); + expect(dump).not.toContain("LegacySsoMutexFlagError"); + } + expect(api.requests.length).toBe(0); + }).pipe(Effect.provide(layer)); + }, + ); - it.live("explicit empty --domains= falls into the merge and resends the existing set", () => { - // `--domains=` parses to an empty slice, so Go's `len(params.Domains) != 0` - // replace gate is false and the merge branch runs with no add/remove — - // live-captured Go PUT resends the existing domains, it does NOT replace - // them with an empty list. + it.live("workdir emulation: the arity error wins over the chdir failure", () => { const { layer, api } = setup({ - cliArgs: ["sso", "update", VALID_PROVIDER_ID, "--domains="], + cliArgs: ["sso", "update", "a", "b", "--workdir", "/nonexistent-sso-update-workdir"], }); return Effect.gen(function* () { - yield* legacySsoUpdate({ ...defaultFlags, domains: [] }); - const putReq = api.requests.find((r) => r.method === "PUT"); - const domains = (putReq?.body as { domains: string[] })?.domains; - expect([...domains].sort()).toEqual(["old1.com", "old2.com"]); + const exit = yield* Effect.exit(legacySsoUpdate({ ...defaultFlags, providerId: "a" })); + expect(Exit.isFailure(exit)).toBe(true); + if (Exit.isFailure(exit)) { + const dump = JSON.stringify(exit.cause); + expect(dump).toContain("LegacySsoUpdateArityError"); + expect(dump).toContain("accepts 1 arg(s), received 2"); + expect(dump).not.toContain("LegacySsoWorkdirError"); + } + expect(api.requests.length).toBe(0); }).pipe(Effect.provide(layer)); }); - it.live("merge keeps empty-string domains and skips entries without a domain field", () => { - // Go's seed check is nil-ness only (`domain.Domain != nil`, - // `update.go:89`): an empty-string domain from the GET response stays in - // the merged set, while an entry missing the field entirely is skipped. + it.live("arity emulation: the arity error wins over an invalid provider ID", () => { + // Go's provider-ID format check lives inside `RunE` (`cmd/sso.go:90-91`), + // long after `ValidateArgs` — a bad UUID must not mask the arg-count + // error. const { layer, api } = setup({ - getBody: { - ...EXISTING_PROVIDER, - domains: [{ id: "d1", domain: "" }, { id: "d2", domain: "old1.com" }, { id: "d3" }], - }, + cliArgs: ["sso", "update", "--domains", "--metadata-url", "u", "not-a-uuid"], }); return Effect.gen(function* () { - yield* legacySsoUpdate(defaultFlags); - const putReq = api.requests.find((r) => r.method === "PUT"); - const domains = (putReq?.body as { domains: string[] })?.domains; - expect([...domains].sort()).toEqual(["", "old1.com"]); - }).pipe(Effect.provide(layer)); - }); - - it.live("reads metadata file and sends as metadata_xml on PUT", () => { - const path = join(tempRoot.current, "good.xml"); - writeFileSync(path, ''); - const { layer, api } = setup(); - return Effect.gen(function* () { - yield* legacySsoUpdate({ ...defaultFlags, metadataFile: Option.some(path) }); - const putReq = api.requests.find((r) => r.method === "PUT"); + const exit = yield* Effect.exit( + legacySsoUpdate({ + ...defaultFlags, + providerId: "not-a-uuid", + metadataUrl: Option.some("u"), + }), + ); + expect(Exit.isFailure(exit)).toBe(true); + if (Exit.isFailure(exit)) { + const dump = JSON.stringify(exit.cause); + expect(dump).toContain("LegacySsoUpdateArityError"); + expect(dump).not.toContain("LegacySsoInvalidUuidError"); + } + expect(api.requests.length).toBe(0); + }).pipe(Effect.provide(layer)); + }); + + it.live( + "arity emulation: a consumed boolean global keeps the count at 1 and PUTs, like Go", + () => { + // `--domains --yes `: pflag hands `--yes` to `--domains` (a + // consumed token is a value no matter what it looks like), so only the + // provider ID stays positional — Go proceeds and PUTs + // `domains: ["--yes"]` (binary-verified). The re-count must not turn + // this into an arity error. + const { layer, api } = setup({ + cliArgs: ["sso", "update", "--domains", "--yes", VALID_PROVIDER_ID], + }); + return Effect.gen(function* () { + yield* legacySsoUpdate(defaultFlags); + const putReq = api.requests.find((r) => r.method === "PUT"); + expect((putReq?.body as { domains?: string[] })?.domains).toEqual(["--yes"]); + }).pipe(Effect.provide(layer)); + }, + ); + + it.live( + "missing-value emulation: a trailing bare --domains fails pflag parse, no GET/PUT", + () => { + // Binary-verified: `sso update --domains` errors + // `flag needs an argument: --domains` — pflag fails `ParseFlags` + // (cobra `command.go:919`) before `ValidateArgs`, every hook, and + // `RunE`, so Go makes no API call. The Effect parser accepts the argv + // (the flag parses as unset), so without this check the handler would + // GET and PUT with an empty domain list (PR #5974 review round 3). + const { layer, api } = setup({ + cliArgs: ["sso", "update", VALID_PROVIDER_ID, "--domains"], + }); + return Effect.gen(function* () { + const exit = yield* Effect.exit(legacySsoUpdate(defaultFlags)); + expect(Exit.isFailure(exit)).toBe(true); + if (Exit.isFailure(exit)) { + const dump = JSON.stringify(exit.cause); + expect(dump).toContain("LegacySsoFlagNeedsArgumentError"); + expect(dump).toContain("flag needs an argument: --domains"); + } + expect(api.requests.length).toBe(0); + }).pipe(Effect.provide(layer)); + }, + ); + + it.live("missing-value emulation: the pflag parse error wins over an arity violation", () => { + // pflag fails parsing (`command.go:919`) before cobra's `ValidateArgs` + // (`command.go:968`), so when `--domains` swallows `--metadata-url` + // (orphaning `u` as a second positional) AND `--add-domains` trails + // bare, Go reports the missing argument, not the arg count + // (binary-verified: `sso update a b --domains`). + const { layer, api } = setup({ + cliArgs: [ + "sso", + "update", + "--domains", + "--metadata-url", + "https://idp.example.com/m", + VALID_PROVIDER_ID, + "--add-domains", + ], + }); + return Effect.gen(function* () { + const exit = yield* Effect.exit( + legacySsoUpdate({ + ...defaultFlags, + metadataUrl: Option.some("https://idp.example.com/m"), + }), + ); + expect(Exit.isFailure(exit)).toBe(true); + if (Exit.isFailure(exit)) { + const dump = JSON.stringify(exit.cause); + expect(dump).toContain("LegacySsoFlagNeedsArgumentError"); + expect(dump).toContain("flag needs an argument: --add-domains"); + expect(dump).not.toContain("LegacySsoUpdateArityError"); + } + expect(api.requests.length).toBe(0); + }).pipe(Effect.provide(layer)); + }); + + it.live( + "anchoring: a persistent flag between sso and update still enforces arity, like Go", + () => { + // Binary-verified: `sso --profile foo update --domains --metadata-url + // u ` errors `accepts 1 arg(s), received 2` — cobra routes through + // the interspersed persistent flag (`Find`/`stripFlags`) and pflag + // still hands `--metadata-url` to `--domains`. The scan must anchor + // across the interspersed flag or the arity re-count silently + // vanishes and the handler GETs/PUTs (PR #5974 review round 3). + const { layer, api } = setup({ + cliArgs: [ + "sso", + "--profile", + "supabase", + "update", + "--domains", + "--metadata-url", + "https://idp.example.com/m", + VALID_PROVIDER_ID, + ], + }); + return Effect.gen(function* () { + const exit = yield* Effect.exit( + legacySsoUpdate({ + ...defaultFlags, + metadataUrl: Option.some("https://idp.example.com/m"), + }), + ); + expect(Exit.isFailure(exit)).toBe(true); + if (Exit.isFailure(exit)) { + const dump = JSON.stringify(exit.cause); + expect(dump).toContain("LegacySsoUpdateArityError"); + expect(dump).toContain("accepts 1 arg(s), received 2"); + } + expect(api.requests.length).toBe(0); + }).pipe(Effect.provide(layer)); + }, + ); + + it.live("anchoring: a persistent flag between sso and update sails through to the PUT", () => { + // Regression guard for the anchor walk: a well-formed interspersed + // invocation (`sso --profile supabase update `) must behave exactly + // like the contiguous one. + const { layer, api } = setup({ + cliArgs: ["sso", "--profile", "supabase", "update", VALID_PROVIDER_ID], + }); + return Effect.gen(function* () { + yield* legacySsoUpdate(defaultFlags); + expect(api.requests.some((r) => r.method === "PUT")).toBe(true); + }).pipe(Effect.provide(layer)); + }); + + it.live( + "value reconciliation: repeated --skip-url-validation resolves last-wins like pflag (=false then bare ends true, skips validation, PUTs)", + () => { + // `--skip-url-validation=false --skip-url-validation --metadata-url + // http://…`: pflag Sets every occurrence in order, ending true, so Go + // skips URL validation and PUTs. The Effect parser resolves repeats + // first-wins (false) and would have validated — and rejected — the + // non-HTTPS URL (PR #5974 review round 4, binary-verified). + const { layer, api } = setup({ + cliArgs: [ + "sso", + "update", + VALID_PROVIDER_ID, + "--skip-url-validation=false", + "--skip-url-validation", + "--metadata-url", + "http://insecure.example.com/md", + ], + }); + return Effect.gen(function* () { + yield* legacySsoUpdate({ + ...defaultFlags, + skipUrlValidation: false, // Effect's first-wins parse + metadataUrl: Option.some("http://insecure.example.com/md"), + }); + const putReq = api.requests.find((r) => r.method === "PUT"); + expect((putReq?.body as { metadata_url?: string })?.metadata_url).toBe( + "http://insecure.example.com/md", + ); + }).pipe(Effect.provide(layer)); + }, + ); + + it.live( + "value reconciliation: bare then =false ends false like pflag — URL validation runs and rejects, no PUT", + () => { + // The mirror case: `--skip-url-validation --skip-url-validation=false` + // is false to pflag (last-wins) but true to the Effect parser + // (first-wins), so without reconciliation the handler would skip the + // validation Go performs and PUT an unvalidated URL. + const { layer, api } = setup({ + cliArgs: [ + "sso", + "update", + VALID_PROVIDER_ID, + "--skip-url-validation", + "--skip-url-validation=false", + "--metadata-url", + "http://insecure.example.com/md", + ], + }); + return Effect.gen(function* () { + const exit = yield* Effect.exit( + legacySsoUpdate({ + ...defaultFlags, + skipUrlValidation: true, // Effect's first-wins parse + metadataUrl: Option.some("http://insecure.example.com/md"), + }), + ); + expect(Exit.isFailure(exit)).toBe(true); + if (Exit.isFailure(exit)) { + const dump = JSON.stringify(exit.cause); + expect(dump).toContain("LegacySsoUpdateMetadataFileError"); + expect(dump).toContain("only HTTPS Metadata URLs are supported"); + } + // Go GETs first (`update.go:42`), then fails validation before the PUT. + expect(api.requests.some((r) => r.method === "PUT")).toBe(false); + }).pipe(Effect.provide(layer)); + }, + ); + + it.live( + "value reconciliation: --domains consuming one --name-id-format leaves the other as pflag's effective value in the PUT", + () => { + // `--domains --name-id-format=T --name-id-format P`: pflag hands the + // first name-id-format token to `--domains` as its value, so the only + // occurrence it Sets is P. The Effect parser read both and resolved + // first-wins to T — the PUT body must carry P, exactly what the Go + // binary sends (PR #5974 review round 4). + const transient = "urn:oasis:names:tc:SAML:2.0:nameid-format:transient" as const; + const persistent = "urn:oasis:names:tc:SAML:2.0:nameid-format:persistent"; + const { layer, api } = setup({ + cliArgs: [ + "sso", + "update", + VALID_PROVIDER_ID, + "--skip-url-validation", + "--domains", + `--name-id-format=${transient}`, + "--name-id-format", + persistent, + "--metadata-url", + "http://insecure.example.com/md", + ], + }); + return Effect.gen(function* () { + yield* legacySsoUpdate({ + ...defaultFlags, + skipUrlValidation: true, + nameIdFormat: Option.some(transient), // Effect's first-wins parse + metadataUrl: Option.some("http://insecure.example.com/md"), + }); + const putReq = api.requests.find((r) => r.method === "PUT"); + const body = putReq?.body as { name_id_format?: string; domains?: string[] }; + expect(body?.name_id_format).toBe(persistent); + // The consumed token is pflag's literal domains value. + expect(body?.domains).toEqual([`--name-id-format=${transient}`]); + }).pipe(Effect.provide(layer)); + }, + ); + + it.live( + "invalid-value emulation: --skip-url-validation=yes fails with pflag's strconv.ParseBool error, no API calls", + () => { + // The Effect parser accepts `yes`; Go's strconv.ParseBool does not — + // pflag fails ParseFlags (cobra `command.go:919`) before every hook + // and request (binary-verified, PR #5974 review round 4). + const { layer, api } = setup({ + cliArgs: [ + "sso", + "update", + VALID_PROVIDER_ID, + "--skip-url-validation=yes", + "--metadata-url", + "https://idp.example.com/m", + ], + }); + return Effect.gen(function* () { + const exit = yield* Effect.exit( + legacySsoUpdate({ + ...defaultFlags, + skipUrlValidation: true, // the Effect parser reads yes as true + metadataUrl: Option.some("https://idp.example.com/m"), + }), + ); + expect(Exit.isFailure(exit)).toBe(true); + if (Exit.isFailure(exit)) { + const dump = JSON.stringify(exit.cause); + expect(dump).toContain("LegacySsoInvalidFlagValueError"); + expect(dump).toContain( + 'invalid argument \\"yes\\" for \\"--skip-url-validation\\" flag: strconv.ParseBool: parsing \\"yes\\": invalid syntax', + ); + } + expect(api.requests.length).toBe(0); + }).pipe(Effect.provide(layer)); + }, + ); + + it.live( + "invalid-value emulation: a later inline-empty --skip-url-validation= fails like pflag, no API calls", + () => { + // `--skip-url-validation=false --skip-url-validation=`: the Effect + // parser resolves repeats first-wins and never validates the second + // occurrence, so it parses; pflag hands `""` to strconv.ParseBool + // (`flag.go:1014-1016`) and aborts ParseFlags before every hook and + // any GET/PUT — only a *bare* repeat means NoOptDefVal true + // (binary-verified, PR #5974 review round 5). + const { layer, api } = setup({ + cliArgs: [ + "sso", + "update", + VALID_PROVIDER_ID, + "--skip-url-validation=false", + "--skip-url-validation=", + "--metadata-url", + "https://idp.example.com/m", + ], + }); + return Effect.gen(function* () { + const exit = yield* Effect.exit( + legacySsoUpdate({ + ...defaultFlags, + skipUrlValidation: false, // Effect's first-wins parse + metadataUrl: Option.some("https://idp.example.com/m"), + }), + ); + expect(Exit.isFailure(exit)).toBe(true); + if (Exit.isFailure(exit)) { + const dump = JSON.stringify(exit.cause); + expect(dump).toContain("LegacySsoInvalidFlagValueError"); + expect(dump).toContain( + 'invalid argument \\"\\" for \\"--skip-url-validation\\" flag: strconv.ParseBool: parsing \\"\\": invalid syntax', + ); + } + expect(api.requests.length).toBe(0); + }).pipe(Effect.provide(layer)); + }, + ); + + it.live( + "invalid-value emulation: a later invalid --name-id-format occurrence fails like pflag, no API calls", + () => { + // The Effect parser resolves repeats first-wins and never validates + // the rest, so `--name-id-format= --name-id-format=bogus` + // parses; pflag Sets every occurrence and aborts on `bogus` + // (binary-verified, PR #5974 review round 4). + const persistent = "urn:oasis:names:tc:SAML:2.0:nameid-format:persistent" as const; + const { layer, api } = setup({ + cliArgs: [ + "sso", + "update", + VALID_PROVIDER_ID, + `--name-id-format=${persistent}`, + "--name-id-format=bogus", + ], + }); + return Effect.gen(function* () { + const exit = yield* Effect.exit( + legacySsoUpdate({ + ...defaultFlags, + nameIdFormat: Option.some(persistent), // Effect's first-wins parse + }), + ); + expect(Exit.isFailure(exit)).toBe(true); + if (Exit.isFailure(exit)) { + const dump = JSON.stringify(exit.cause); + expect(dump).toContain("LegacySsoInvalidFlagValueError"); + expect(dump).toContain( + 'invalid argument \\"bogus\\" for \\"--name-id-format\\" flag: must be one of [ urn:oasis', + ); + expect(dump).toContain("nameid-format:transient ]"); + } + expect(api.requests.length).toBe(0); + }).pipe(Effect.provide(layer)); + }, + ); + + it.live( + "invalid-value emulation: an invalid occurrence beats a trailing missing value, matching pflag's sequential walk", + () => { + // `--skip-url-validation=yes --domains`: pflag walks argv in order and + // rejects `yes` before ever reaching the bare trailing `--domains` + // (binary-verified: Go names the invalid argument, not the missing one). + const { layer, api } = setup({ + cliArgs: ["sso", "update", VALID_PROVIDER_ID, "--skip-url-validation=yes", "--domains"], + }); + return Effect.gen(function* () { + const exit = yield* Effect.exit( + legacySsoUpdate({ ...defaultFlags, skipUrlValidation: true }), + ); + expect(Exit.isFailure(exit)).toBe(true); + if (Exit.isFailure(exit)) { + const dump = JSON.stringify(exit.cause); + expect(dump).toContain("LegacySsoInvalidFlagValueError"); + expect(dump).not.toContain("LegacySsoFlagNeedsArgumentError"); + } + expect(api.requests.length).toBe(0); + }).pipe(Effect.provide(layer)); + }, + ); + + it.live("--domains replaces domains verbatim", () => { + const flags = { ...defaultFlags, domains: ["new.com"] }; + const { layer, api } = setup({ cliArgs: cliArgsFor(flags) }); + return Effect.gen(function* () { + yield* legacySsoUpdate(flags); + const putReq = api.requests.find((r) => r.method === "PUT"); + expect((putReq?.body as { domains?: string[] })?.domains).toEqual(["new.com"]); + }).pipe(Effect.provide(layer)); + }); + + it.live("--add-domains merges with existing GET domains", () => { + const flags = { ...defaultFlags, addDomains: ["new.com"] }; + const { layer, api } = setup({ cliArgs: cliArgsFor(flags) }); + return Effect.gen(function* () { + yield* legacySsoUpdate(flags); + const putReq = api.requests.find((r) => r.method === "PUT"); + const domains = (putReq?.body as { domains: string[] })?.domains; + // Go map iteration is unordered — sort before asserting. + expect([...domains].sort()).toEqual(["new.com", "old1.com", "old2.com"]); + }).pipe(Effect.provide(layer)); + }); + + it.live("--remove-domains strips from existing GET domains", () => { + const flags = { ...defaultFlags, removeDomains: ["old1.com"] }; + const { layer, api } = setup({ cliArgs: cliArgsFor(flags) }); + return Effect.gen(function* () { + yield* legacySsoUpdate(flags); + const putReq = api.requests.find((r) => r.method === "PUT"); + const domains = (putReq?.body as { domains: string[] })?.domains; + expect([...domains].sort()).toEqual(["old2.com"]); + }).pipe(Effect.provide(layer)); + }); + + it.live("no domain flag set → PUT still sends the recomputed existing domain set", () => { + // Go's `--add-domains`/`--remove-domains` default to a non-nil `[]string{}` + // (`cmd/sso.go:171-172`), so `update.go:84`'s `!= nil` gate is always true + // from the CLI — every `sso update` enters the merge and sends `domains`, + // even when no domain flag was passed (CLI-1981). Live-captured Go PUT: + // `{"domains":["old1.com","old2.com"]}`. + const { layer, api } = setup(); + return Effect.gen(function* () { + yield* legacySsoUpdate(defaultFlags); + const putReq = api.requests.find((r) => r.method === "PUT"); + const domains = (putReq?.body as { domains: string[] })?.domains; + // Go map iteration is unordered — sort before asserting. + expect([...domains].sort()).toEqual(["old1.com", "old2.com"]); + }).pipe(Effect.provide(layer)); + }); + + it.live( + "no domain flags + provider with no domains → PUT sends domains: [] (not omitted)", + () => { + // Go sets `body.Domains` to a non-nil pointer to `make([]string, 0)`, and + // `json:"domains,omitempty"` never omits a non-nil pointer — live-captured + // Go PUT body is exactly `{"domains":[]}`. + const { layer, api } = setup({ getBody: { ...EXISTING_PROVIDER, domains: [] } }); + return Effect.gen(function* () { + yield* legacySsoUpdate(defaultFlags); + const putReq = api.requests.find((r) => r.method === "PUT"); + const body = putReq?.body as Record; + expect(Object.keys(body)).toContain("domains"); + expect(body["domains"]).toEqual([]); + }).pipe(Effect.provide(layer)); + }, + ); + + it.live("no domain flags + GET response missing domains entirely → PUT sends domains: []", () => { + // Go's seed loop is skipped when `getResp.JSON200.Domains == nil`, leaving + // the merged set empty — same `{"domains":[]}` bytes as the empty-list case. + const { domains: _omitted, ...providerWithoutDomains } = EXISTING_PROVIDER; + const { layer, api } = setup({ getBody: providerWithoutDomains }); + return Effect.gen(function* () { + yield* legacySsoUpdate(defaultFlags); + const putReq = api.requests.find((r) => r.method === "PUT"); + const body = putReq?.body as Record; + expect(Object.keys(body)).toContain("domains"); + expect(body["domains"]).toEqual([]); + }).pipe(Effect.provide(layer)); + }); + + it.live("explicit empty --domains= falls into the merge and resends the existing set", () => { + // `--domains=` parses to an empty slice, so Go's `len(params.Domains) != 0` + // replace gate is false and the merge branch runs with no add/remove — + // live-captured Go PUT resends the existing domains, it does NOT replace + // them with an empty list. + const { layer, api } = setup({ + cliArgs: ["sso", "update", VALID_PROVIDER_ID, "--domains="], + }); + return Effect.gen(function* () { + yield* legacySsoUpdate({ ...defaultFlags, domains: [] }); + const putReq = api.requests.find((r) => r.method === "PUT"); + const domains = (putReq?.body as { domains: string[] })?.domains; + expect([...domains].sort()).toEqual(["old1.com", "old2.com"]); + }).pipe(Effect.provide(layer)); + }); + + it.live("merge keeps empty-string domains and skips entries without a domain field", () => { + // Go's seed check is nil-ness only (`domain.Domain != nil`, + // `update.go:89`): an empty-string domain from the GET response stays in + // the merged set, while an entry missing the field entirely is skipped. + const { layer, api } = setup({ + getBody: { + ...EXISTING_PROVIDER, + domains: [{ id: "d1", domain: "" }, { id: "d2", domain: "old1.com" }, { id: "d3" }], + }, + }); + return Effect.gen(function* () { + yield* legacySsoUpdate(defaultFlags); + const putReq = api.requests.find((r) => r.method === "PUT"); + const domains = (putReq?.body as { domains: string[] })?.domains; + expect([...domains].sort()).toEqual(["", "old1.com"]); + }).pipe(Effect.provide(layer)); + }); + + it.live("reads metadata file and sends as metadata_xml on PUT", () => { + const path = join(tempRoot.current, "good.xml"); + writeFileSync(path, ''); + const flags = { ...defaultFlags, metadataFile: Option.some(path) }; + const { layer, api } = setup({ cliArgs: cliArgsFor(flags) }); + return Effect.gen(function* () { + yield* legacySsoUpdate(flags); + const putReq = api.requests.find((r) => r.method === "PUT"); expect((putReq?.body as { metadata_xml?: string })?.metadata_xml).toContain(""); }).pipe(Effect.provide(layer)); }); @@ -571,9 +1358,10 @@ describe("legacy sso update integration", () => { it.live("preserves attribute_mapping `default` field in PUT body", () => { const path = join(tempRoot.current, "map.json"); writeFileSync(path, JSON.stringify({ keys: { a: { default: 3 } } })); - const { layer, api } = setup(); + const flags = { ...defaultFlags, attributeMappingFile: Option.some(path) }; + const { layer, api } = setup({ cliArgs: cliArgsFor(flags) }); return Effect.gen(function* () { - yield* legacySsoUpdate({ ...defaultFlags, attributeMappingFile: Option.some(path) }); + yield* legacySsoUpdate(flags); const putReq = api.requests.find((r) => r.method === "PUT"); const mapping = (putReq?.body as { attribute_mapping?: { keys: { a: { default: number } } } }) ?.attribute_mapping; @@ -655,12 +1443,13 @@ describe("legacy sso update integration", () => { }); it.live("nameIdFormat is forwarded in PUT body when provided", () => { - const { layer, api } = setup(); + const flags = { + ...defaultFlags, + nameIdFormat: Option.some("urn:oasis:names:tc:SAML:2.0:nameid-format:persistent" as const), + }; + const { layer, api } = setup({ cliArgs: cliArgsFor(flags) }); return Effect.gen(function* () { - yield* legacySsoUpdate({ - ...defaultFlags, - nameIdFormat: Option.some("urn:oasis:names:tc:SAML:2.0:nameid-format:persistent"), - }); + yield* legacySsoUpdate(flags); const putReq = api.requests.find((r) => r.method === "PUT"); expect((putReq?.body as { name_id_format?: string })?.name_id_format).toBe( "urn:oasis:names:tc:SAML:2.0:nameid-format:persistent", @@ -669,15 +1458,14 @@ describe("legacy sso update integration", () => { }); it.live("malformed metadata URL surfaces as update metadata file error", () => { - const { layer } = setup(); + const flags = { + ...defaultFlags, + metadataUrl: Option.some("::::not a url::::"), + skipUrlValidation: false, + }; + const { layer } = setup({ cliArgs: cliArgsFor(flags) }); return Effect.gen(function* () { - const exit = yield* Effect.exit( - legacySsoUpdate({ - ...defaultFlags, - metadataUrl: Option.some("::::not a url::::"), - skipUrlValidation: false, - }), - ); + const exit = yield* Effect.exit(legacySsoUpdate(flags)); expect(Exit.isFailure(exit)).toBe(true); if (Exit.isFailure(exit)) { const dump = JSON.stringify(exit.cause); @@ -692,11 +1480,10 @@ describe("legacy sso update integration", () => { it.live("malformed attribute-mapping JSON surfaces a tagged error", () => { const path = join(tempRoot.current, "malformed.json"); writeFileSync(path, "{not json}"); - const { layer } = setup(); + const flags = { ...defaultFlags, attributeMappingFile: Option.some(path) }; + const { layer } = setup({ cliArgs: cliArgsFor(flags) }); return Effect.gen(function* () { - const exit = yield* Effect.exit( - legacySsoUpdate({ ...defaultFlags, attributeMappingFile: Option.some(path) }), - ); + const exit = yield* Effect.exit(legacySsoUpdate(flags)); expect(Exit.isFailure(exit)).toBe(true); if (Exit.isFailure(exit)) { expect(JSON.stringify(exit.cause)).toContain("LegacySsoUpdateAttributeMappingFileError"); @@ -705,17 +1492,416 @@ describe("legacy sso update integration", () => { }); it.live("--add-domains + --remove-domains combined apply remove then add", () => { - const { layer, api } = setup(); + const flags = { ...defaultFlags, addDomains: ["new.com"], removeDomains: ["old1.com"] }; + const { layer, api } = setup({ cliArgs: cliArgsFor(flags) }); return Effect.gen(function* () { - yield* legacySsoUpdate({ - ...defaultFlags, - addDomains: ["new.com"], - removeDomains: ["old1.com"], - }); + yield* legacySsoUpdate(flags); const putReq = api.requests.find((r) => r.method === "PUT"); const domains = (putReq?.body as { domains: string[] })?.domains; // Go uses map iteration → unordered; sort before asserting. expect([...domains].sort()).toEqual(["new.com", "old2.com"]); }).pipe(Effect.provide(layer)); }); + + // ------------------------------------------------------------------------- + // Profile emulation (PR #5974 review round 7): Go's `LoadProfile` runs from + // the root `PersistentPreRunE` (`cmd/root.go:98-102`) on the pflag/viper- + // effective `--profile`/`SUPABASE_PROFILE`, immediately before + // `ChangeWorkDir` — it decides which API host receives the GET *and* the + // PUT (Go targets the same host for both, `update.go:42`), and aborts the + // command when the profile cannot be loaded. + // ------------------------------------------------------------------------- + + const writeProfileYaml = (name: string, apiUrl: string): string => { + const path = join(tempRoot.current, name); + writeFileSync( + path, + [ + `name: ${name.replace(/\.[^.]*$/, "")}`, + `api_url: ${apiUrl}`, + `dashboard_url: ${apiUrl}/dashboard`, + "project_host: supabase.co", + ].join("\n"), + ); + return path; + }; + + const withProfileEnv = (value: string | undefined) => { + const previous = process.env["SUPABASE_PROFILE"]; + if (value === undefined) { + delete process.env["SUPABASE_PROFILE"]; + } else { + process.env["SUPABASE_PROFILE"] = value; + } + return Effect.sync(() => { + if (previous === undefined) { + delete process.env["SUPABASE_PROFILE"]; + } else { + process.env["SUPABASE_PROFILE"] = previous; + } + }); + }; + + it.live( + "profile emulation: repeated --profile resolves last-wins — GET and PUT both target the last file's host", + () => { + // `sso update --profile first.yml --profile second.yml`: the + // Effect parser is first-wins (the config layer — and the typed client + // — resolved first.yml) while pflag Sets every occurrence and ends on + // second.yml. Go GETs and PUTs second.yml's api_url (`update.go:42`); + // first.yml's host receives nothing. + const first = writeProfileYaml("first.yml", "http://first.example"); + const second = writeProfileYaml("second.yml", "http://second.example"); + const restoreEnv = withProfileEnv(undefined); + const testSetup = setup({ + cliArgs: ["sso", "update", VALID_PROVIDER_ID, "--profile", first, "--profile", second], + profileFlag: first, + }); + const { layer, api, cache } = testSetup; + return Effect.gen(function* () { + yield* legacySsoUpdate(defaultFlags); + const providerUrl = `http://second.example/v1/projects/${LEGACY_VALID_REF}/config/auth/sso/providers/${VALID_PROVIDER_ID}`; + const get = api.requests.find((r) => r.method === "GET"); + const put = api.requests.find((r) => r.method === "PUT"); + expect(get?.url).toBe(providerUrl); + expect(put?.url).toBe(providerUrl); + // The merge seeds from the reconciled host's GET response. + const domains = (put?.body as { domains?: string[] })?.domains ?? []; + expect([...domains].sort()).toEqual(["old1.com", "old2.com"]); + expect(api.requests.some((r) => r.url.startsWith("http://first.example"))).toBe(false); + // The raw GET stitches identity through the shared per-command guard, + // like Go's identityTransport on every Management API response. + expect(testSetup.stitchedResponses).toBeGreaterThan(0); + // The linked-project cache fill targets the reconciled host too + // (Go's ensureProjectGroupsCached uses the process-wide profile). + expect(cache.cachedApiUrl).toBe("http://second.example"); + }).pipe(Effect.ensuring(restoreEnv), Effect.provide(layer)); + }, + ); + + it.live( + "profile emulation: the reconciled-host GET maps a 404 exactly like the typed client", + () => { + const first = writeProfileYaml("first-404.yml", "http://first.example"); + const second = writeProfileYaml("second-404.yml", "http://second.example"); + const restoreEnv = withProfileEnv(undefined); + const { layer, api } = setup({ + getStatus: 404, + getBody: {}, + cliArgs: ["sso", "update", VALID_PROVIDER_ID, "--profile", first, "--profile", second], + profileFlag: first, + }); + return Effect.gen(function* () { + const exit = yield* Effect.exit(legacySsoUpdate(defaultFlags)); + expect(Exit.isFailure(exit)).toBe(true); + if (Exit.isFailure(exit)) { + const dump = JSON.stringify(exit.cause); + expect(dump).toContain("LegacySsoUpdateNotFoundError"); + expect(dump).toContain( + `An identity provider with ID \\"${VALID_PROVIDER_ID}\\" could not be found.`, + ); + } + expect(api.requests.some((r) => r.method === "PUT")).toBe(false); + }).pipe(Effect.ensuring(restoreEnv), Effect.provide(layer)); + }, + ); + + it.live( + "profile emulation: the reconciled-host GET maps a non-404 status exactly like the typed client", + () => { + const first = writeProfileYaml("first-500.yml", "http://first.example"); + const second = writeProfileYaml("second-500.yml", "http://second.example"); + const restoreEnv = withProfileEnv(undefined); + const { layer, api } = setup({ + getStatus: 500, + getBody: { error: "boom" }, + cliArgs: ["sso", "update", VALID_PROVIDER_ID, "--profile", first, "--profile", second], + profileFlag: first, + }); + return Effect.gen(function* () { + const exit = yield* Effect.exit(legacySsoUpdate(defaultFlags)); + expect(Exit.isFailure(exit)).toBe(true); + if (Exit.isFailure(exit)) { + const dump = JSON.stringify(exit.cause); + expect(dump).toContain("LegacySsoUpdateUnexpectedStatusError"); + expect(dump).toContain("unexpected error fetching identity provider:"); + } + expect(api.requests.some((r) => r.method === "PUT")).toBe(false); + }).pipe(Effect.ensuring(restoreEnv), Effect.provide(layer)); + }, + ); + + it.live( + "profile emulation: the reconciled GET narrows odd JSON shapes when merging domains", + () => { + // Covers the raw GET's JSON-narrowing fallbacks: a `domains` entry + // that isn't an object and one whose `domain` isn't a string are + // skipped, matching the typed client's schema behavior. + const first = writeProfileYaml("first-merge.yml", "http://first.example"); + const second = writeProfileYaml("second-merge.yml", "http://second.example"); + const restoreEnv = withProfileEnv(undefined); + const { layer, api, cache } = setup({ + getBody: { + id: VALID_PROVIDER_ID, + domains: [{ domain: "old1.com" }, "not-an-object", { domain: 42 }], + }, + cliArgs: [ + "sso", + "update", + VALID_PROVIDER_ID, + "--add-domains", + "new.com", + "--profile", + first, + "--profile", + second, + ], + profileFlag: first, + }); + return Effect.gen(function* () { + yield* legacySsoUpdate({ ...defaultFlags, addDomains: ["new.com"] }); + const put = api.requests.find((r) => r.method === "PUT"); + expect(put?.url).toBe( + `http://second.example/v1/projects/${LEGACY_VALID_REF}/config/auth/sso/providers/${VALID_PROVIDER_ID}`, + ); + const domains = (put?.body as { domains?: string[] })?.domains ?? []; + expect([...domains].sort()).toEqual(["new.com", "old1.com"]); + // The linked-project cache fill receives the RECONCILED profile's + // token explicitly (here the profile-independent env token) — the + // stale profile's keyring token must never follow the reconciled URL + // (review r3684524241). `undefined` would fall back to the config + // layer's credentials service. + expect(cache.cachedAccessToken).toBeDefined(); + }).pipe(Effect.ensuring(restoreEnv), Effect.provide(layer)); + }, + ); + + it.live("profile emulation: a reconciled profile with no resolvable token aborts like Go", () => { + // Go's `GetSupabase` gate (`api.go:119-124`) `log.Fatalln`s ErrMissingToken + // at first client use when the RECONCILED profile's lookup finds nothing — + // the stale profile's token must never be substituted, and no request may + // be issued (PR #5974 review round 10, r3686720488). + const first = writeProfileYaml("first-notoken.yml", "http://first.example"); + const second = writeProfileYaml("second-notoken.yml", "http://second.example"); + const restoreEnv = withProfileEnv(undefined); + const { layer, api } = setup({ + accessToken: Option.none(), + cliArgs: [ + "sso", + "update", + VALID_PROVIDER_ID, + "--add-domains", + "new.com", + "--profile", + first, + "--profile", + second, + ], + profileFlag: first, + }); + return Effect.gen(function* () { + const exit = yield* Effect.exit( + legacySsoUpdate({ ...defaultFlags, addDomains: ["new.com"] }), + ); + expect(Exit.isFailure(exit)).toBe(true); + if (Exit.isFailure(exit)) { + const dump = JSON.stringify(exit.cause); + expect(dump).toContain("LegacySsoAccessTokenError"); + expect(dump).toContain("Access token not provided. Supply an access token by running"); + } + expect(api.requests).toHaveLength(0); + }).pipe(Effect.ensuring(restoreEnv), Effect.provide(layer)); + }); + + it.live("profile emulation: the missing-token gate fires AFTER the mutex check, like Go", () => { + // cobra: ParseFlags → PreRunE → required → GROUPS → RunE(GetSupabase) — + // the token gate lives in RunE, so a mutex violation must win even when + // the reconciled profile has no token (validation-order parity). + const first = writeProfileYaml("first-order.yml", "http://first.example"); + const second = writeProfileYaml("second-order.yml", "http://second.example"); + const restoreEnv = withProfileEnv(undefined); + const { layer, api } = setup({ + accessToken: Option.none(), + cliArgs: [ + "sso", + "update", + VALID_PROVIDER_ID, + "--domains", + "a.com", + "--add-domains", + "new.com", + "--profile", + first, + "--profile", + second, + ], + profileFlag: first, + }); + return Effect.gen(function* () { + const exit = yield* Effect.exit( + legacySsoUpdate({ ...defaultFlags, domains: ["a.com"], addDomains: ["new.com"] }), + ); + expect(Exit.isFailure(exit)).toBe(true); + if (Exit.isFailure(exit)) { + const dump = JSON.stringify(exit.cause); + expect(dump).toContain("[add-domains domains] were all set"); + expect(dump).not.toContain("Access token not provided"); + } + expect(api.requests).toHaveLength(0); + }).pipe(Effect.ensuring(restoreEnv), Effect.provide(layer)); + }); + + it.live("profile emulation: the reconciled GET tolerates a body without a domains array", () => { + const first = writeProfileYaml("first-nodom.yml", "http://first.example"); + const second = writeProfileYaml("second-nodom.yml", "http://second.example"); + const restoreEnv = withProfileEnv(undefined); + const { layer, api } = setup({ + getBody: { id: VALID_PROVIDER_ID }, + cliArgs: [ + "sso", + "update", + VALID_PROVIDER_ID, + "--add-domains", + "new.com", + "--profile", + first, + "--profile", + second, + ], + profileFlag: first, + }); + return Effect.gen(function* () { + yield* legacySsoUpdate({ ...defaultFlags, addDomains: ["new.com"] }); + const put = api.requests.find((r) => r.method === "PUT"); + expect((put?.body as { domains?: string[] })?.domains).toEqual(["new.com"]); + }).pipe(Effect.ensuring(restoreEnv), Effect.provide(layer)); + }); + + it.live( + "profile emulation: --profile consuming a trailing flag token fails LoadProfile, never GETs", + () => { + // `sso update --profile --add-domains`: pflag binds + // `"--add-domains"` as the profile value (positional count stays 1); + // viper's extension gate rejects it before any request. + const restoreEnv = withProfileEnv(undefined); + const { layer, api } = setup({ + cliArgs: ["sso", "update", VALID_PROVIDER_ID, "--profile", "--add-domains"], + }); + return Effect.gen(function* () { + const exit = yield* Effect.exit(legacySsoUpdate(defaultFlags)); + expect(Exit.isFailure(exit)).toBe(true); + if (Exit.isFailure(exit)) { + const dump = JSON.stringify(exit.cause); + expect(dump).toContain("LegacySsoProfileError"); + expect(dump).toContain(`failed to read profile: Unsupported Config Type \\"\\"`); + } + expect(api.requests.length).toBe(0); + }).pipe(Effect.ensuring(restoreEnv), Effect.provide(layer)); + }, + ); + + it.live( + "profile emulation: an undecodable 200 body from the reconciled GET aborts before the PUT", + () => { + // Go's generated `ParseV1GetASsoProviderResponse` unmarshals the 200 + // JSON body; the unmarshal error exits `update.Run` with `failed to + // get sso provider: %w` before any PUT (`update.go:42-45`). + const first = writeProfileYaml("first-badjson.yml", "http://first.example"); + const second = writeProfileYaml("second-badjson.yml", "http://second.example"); + const restoreEnv = withProfileEnv(undefined); + const { layer, api } = setup({ + getRaw: { status: 200, body: "{not json", contentType: "application/json" }, + cliArgs: ["sso", "update", VALID_PROVIDER_ID, "--profile", first, "--profile", second], + profileFlag: first, + }); + return Effect.gen(function* () { + const exit = yield* Effect.exit(legacySsoUpdate(defaultFlags)); + expect(Exit.isFailure(exit)).toBe(true); + if (Exit.isFailure(exit)) { + const dump = JSON.stringify(exit.cause); + expect(dump).toContain("LegacySsoUpdateNetworkError"); + expect(dump).toContain("failed to get sso provider:"); + } + expect(api.requests.some((r) => r.method === "PUT")).toBe(false); + }).pipe(Effect.ensuring(restoreEnv), Effect.provide(layer)); + }, + ); + + it.live( + "profile emulation: a 200 without a JSON content type maps to the unexpected-status branch, like Go's nil JSON200", + () => { + // `update.go:47-55`: a 200 whose content type isn't JSON leaves + // `JSON200` nil, so Go runs the gate check and errors with the raw + // body — no PUT. + const first = writeProfileYaml("first-nonjson.yml", "http://first.example"); + const second = writeProfileYaml("second-nonjson.yml", "http://second.example"); + const restoreEnv = withProfileEnv(undefined); + const { layer, api } = setup({ + getRaw: { status: 200, body: "plain text body", contentType: "text/plain" }, + cliArgs: ["sso", "update", VALID_PROVIDER_ID, "--profile", first, "--profile", second], + profileFlag: first, + }); + return Effect.gen(function* () { + const exit = yield* Effect.exit(legacySsoUpdate(defaultFlags)); + expect(Exit.isFailure(exit)).toBe(true); + if (Exit.isFailure(exit)) { + const dump = JSON.stringify(exit.cause); + expect(dump).toContain("LegacySsoUpdateUnexpectedStatusError"); + expect(dump).toContain("unexpected error fetching identity provider: plain text body"); + } + expect(api.requests.some((r) => r.method === "PUT")).toBe(false); + }).pipe(Effect.ensuring(restoreEnv), Effect.provide(layer)); + }, + ); + + it.live( + "profile emulation: a gated 4xx on the reconciled GET sends the fallback gate requests to the reconciled host", + () => { + // Go's `SuggestUpgradeOnError` goes through `GetSupabase()` and the + // process-wide reconciled `CurrentProfile`; the project + entitlement + // fallback GETs must hit the same host as the main call. + const first = writeProfileYaml("first-gate.yml", "http://first.example"); + const second = writeProfileYaml("second-gate.yml", "http://second.example"); + const restoreEnv = withProfileEnv(undefined); + const { layer, api } = setup({ + getStatus: 403, + getBody: {}, + upgradeGate: "gated", + cliArgs: ["sso", "update", VALID_PROVIDER_ID, "--profile", first, "--profile", second], + profileFlag: first, + }); + return Effect.gen(function* () { + const exit = yield* Effect.exit(legacySsoUpdate(defaultFlags)); + expect(Exit.isFailure(exit)).toBe(true); + const project = api.requests.find((r) => + r.url.endsWith(`/v1/projects/${LEGACY_VALID_REF}`), + ); + const entitlements = api.requests.find((r) => r.url.includes("/entitlements")); + expect(project?.url).toBe(`http://second.example/v1/projects/${LEGACY_VALID_REF}`); + expect(entitlements?.url).toBe("http://second.example/v1/organizations/acme/entitlements"); + expect(api.requests.some((r) => r.url.startsWith("http://first.example"))).toBe(false); + }).pipe(Effect.ensuring(restoreEnv), Effect.provide(layer)); + }, + ); + + it.live("profile emulation: the LoadProfile failure loses to the arity check, like Go", () => { + // cobra: `ValidateArgs` runs before every hook (`command.go:968`), so a + // wrong arg count is reported even when the profile is also unloadable + // (binary-verified for workdir in round 6; LoadProfile sits in the same + // PersistentPreRunE, before ChangeWorkDir). + const restoreEnv = withProfileEnv(undefined); + const { layer, api } = setup({ + cliArgs: ["sso", "update", "a", "b", "--profile", "--metadata-url", "u"], + }); + return Effect.gen(function* () { + const exit = yield* Effect.exit(legacySsoUpdate({ ...defaultFlags, providerId: "a" })); + expect(Exit.isFailure(exit)).toBe(true); + if (Exit.isFailure(exit)) { + const dump = JSON.stringify(exit.cause); + expect(dump).toContain("LegacySsoUpdateArityError"); + expect(dump).not.toContain("LegacySsoProfileError"); + } + expect(api.requests.length).toBe(0); + }).pipe(Effect.ensuring(restoreEnv), Effect.provide(layer)); + }); }); diff --git a/apps/cli/src/legacy/config/legacy-cli-config.layer.ts b/apps/cli/src/legacy/config/legacy-cli-config.layer.ts index 843059bf59..9c042bd087 100644 --- a/apps/cli/src/legacy/config/legacy-cli-config.layer.ts +++ b/apps/cli/src/legacy/config/legacy-cli-config.layer.ts @@ -3,7 +3,9 @@ import { parse as parseYaml } from "yaml"; import { CLI_VERSION } from "../../shared/cli/version.ts"; import { LegacyProfileFlag, LegacyWorkdirFlag } from "../../shared/legacy/global-flags.ts"; import { + legacyApiUrl, legacyDashboardUrl, + legacyIsBuiltinProfileName, legacyPoolerHost, legacyProjectHost, } from "../shared/legacy-profile.ts"; @@ -23,24 +25,14 @@ interface ResolvedProfile { readonly dashboardUrl: string; } -const BUILTIN_PROFILE_API_URLS: Record = { - supabase: "https://api.supabase.com", - "supabase-staging": "https://api.supabase.green", - "supabase-local": "http://localhost:8080", - snap: "https://cloudapi.snap.com", -}; - -function isBuiltinProfileName(value: string): value is LegacyProfileName { - return value in BUILTIN_PROFILE_API_URLS; -} - -// `projectHost` is sourced from `legacy-profile.ts` (the single source of truth that -// mirrors Go's `allProfiles` table and is also consumed by `branches get`), so the -// per-profile host mapping is not duplicated here. +// All per-profile endpoints are sourced from `legacy-profile.ts` (the single +// source of truth that mirrors Go's `allProfiles` table and is also consumed +// by `branches get` and the sso pflag-profile reconciliation), so no mapping +// is duplicated here. function resolvedBuiltin(name: LegacyProfileName): ResolvedProfile { return { name, - apiUrl: BUILTIN_PROFILE_API_URLS[name], + apiUrl: legacyApiUrl(name), projectHost: legacyProjectHost(name), poolerHost: legacyPoolerHost(name), dashboardUrl: legacyDashboardUrl(name), @@ -133,7 +125,7 @@ function resolveProfile( }); } - if (isBuiltinProfileName(token)) { + if (legacyIsBuiltinProfileName(token)) { return resolvedBuiltin(token); } diff --git a/apps/cli/src/legacy/shared/legacy-profile.ts b/apps/cli/src/legacy/shared/legacy-profile.ts index c81cb70a7f..b1bb007a0c 100644 --- a/apps/cli/src/legacy/shared/legacy-profile.ts +++ b/apps/cli/src/legacy/shared/legacy-profile.ts @@ -10,7 +10,11 @@ * behavior when an external profile YAML omits those keys. */ +import type { LegacyProfileName } from "../config/legacy-cli-config.service.ts"; + interface LegacyProfileEndpoints { + /** Management API base URL (Go's `Profile.APIURL`, `profile.go:19`). */ + readonly apiUrl: string; readonly projectHost: string; readonly dashboardUrl: string; /** @@ -24,29 +28,47 @@ interface LegacyProfileEndpoints { const BUILT_IN: Readonly> = { supabase: { + apiUrl: "https://api.supabase.com", projectHost: "supabase.co", dashboardUrl: "https://supabase.com/dashboard", poolerHost: "supabase.com", }, "supabase-staging": { + apiUrl: "https://api.supabase.green", projectHost: "supabase.red", dashboardUrl: "https://supabase.green/dashboard", poolerHost: "supabase.green", }, "supabase-local": { + apiUrl: "http://localhost:8080", projectHost: "supabase.red", dashboardUrl: "http://localhost:8082", poolerHost: "", }, snap: { + apiUrl: "https://cloudapi.snap.com", projectHost: "snapcloud.dev", dashboardUrl: "https://cloud.snap.com/dashboard", poolerHost: "snapcloud.co", }, }; +/** + * Exact-match (case-sensitive) built-in profile-name guard. Go matches + * built-in names with `strings.EqualFold` (`profile.go:96-99`); callers that + * need Go's fold semantics lower-case the candidate first (all four built-in + * names are already lower-case). + */ +export function legacyIsBuiltinProfileName(profile: string): profile is LegacyProfileName { + return profile in BUILT_IN; +} + const DEFAULT_ENDPOINTS: LegacyProfileEndpoints = BUILT_IN.supabase!; +export function legacyApiUrl(profile: string): string { + return (BUILT_IN[profile] ?? DEFAULT_ENDPOINTS).apiUrl; +} + export function legacyProjectHost(profile: string): string { return (BUILT_IN[profile] ?? DEFAULT_ENDPOINTS).projectHost; } diff --git a/apps/cli/src/legacy/shared/legacy-upgrade-suggest.ts b/apps/cli/src/legacy/shared/legacy-upgrade-suggest.ts index 0ff7eb7f2c..da13bad0b4 100644 --- a/apps/cli/src/legacy/shared/legacy-upgrade-suggest.ts +++ b/apps/cli/src/legacy/shared/legacy-upgrade-suggest.ts @@ -1,7 +1,7 @@ import { styleText } from "node:util"; import type { SupabaseApiError } from "@supabase/api/effect"; -import { Effect, Option } from "effect"; +import { Effect, Option, type Redacted } from "effect"; import * as HttpClient from "effect/unstable/http/HttpClient"; import * as HttpClientError from "effect/unstable/http/HttpClientError"; import * as HttpClientRequest from "effect/unstable/http/HttpClientRequest"; @@ -90,6 +90,26 @@ export const legacySuggestUpgrade = Effect.fnUntraced(function* (opts: { readonly featureKey?: string; readonly statusCode: number; readonly response?: HttpClientResponse.HttpClientResponse; + /** + * Overrides the API base URL of the fallback project + entitlement GETs. + * Go's `SuggestUpgradeOnError` calls `GetSupabase()`, which targets the + * process-wide `CurrentProfile` — commands that reconcile a pflag-effective + * profile differing from the config layer's (sso add/update, PR #5974 + * round 7) pass that profile's URL so the gate requests hit the same host + * as their main calls. Defaults to `LegacyCliConfig.apiUrl`. + */ + readonly apiUrl?: string; + /** + * Overrides the bearer token of the fallback GETs, complementing `apiUrl`: + * Go resolves credentials for the process-wide reconciled `CurrentProfile` + * (`access_token.go:43`), so callers that pass a reconciled `apiUrl` must + * pass the reconciled profile's token too — otherwise the stale profile's + * bearer token would be sent to the reconciled host (review r3684524241). + * `Some` uses that token, `None` sends unauthenticated (the reconciled + * profile has no token — matching Go, which fails its token lookup and + * never attaches the stale one), `undefined` resolves from the service. + */ + readonly accessToken?: Option.Option>; /** * Set false where the Go twin fires no `TrackUpgradeSuggested` (vanity * check-availability), keeping telemetry 1:1. @@ -126,16 +146,18 @@ export const legacySuggestUpgrade = Effect.fnUntraced(function* (opts: { return; } - const tokenOpt = yield* resolveLegacyAccessToken; + const tokenOpt = opts.accessToken ?? (yield* resolveLegacyAccessToken); const authHeader: ( req: HttpClientRequest.HttpClientRequest, ) => HttpClientRequest.HttpClientRequest = Option.isSome(tokenOpt) ? HttpClientRequest.bearerToken(tokenOpt.value) : (req) => req; - const projectReq = HttpClientRequest.get( - `${cliConfig.apiUrl}/v1/projects/${opts.projectRef}`, - ).pipe(authHeader, HttpClientRequest.setHeader("User-Agent", cliConfig.userAgent)); + const apiUrl = opts.apiUrl ?? cliConfig.apiUrl; + const projectReq = HttpClientRequest.get(`${apiUrl}/v1/projects/${opts.projectRef}`).pipe( + authHeader, + HttpClientRequest.setHeader("User-Agent", cliConfig.userAgent), + ); const projectResp = yield* httpClient.execute(projectReq).pipe(Effect.option); if (projectResp._tag === "None" || projectResp.value.status !== 200) { return; @@ -149,9 +171,10 @@ export const legacySuggestUpgrade = Effect.fnUntraced(function* (opts: { return; } - const entReq = HttpClientRequest.get( - `${cliConfig.apiUrl}/v1/organizations/${orgSlug}/entitlements`, - ).pipe(authHeader, HttpClientRequest.setHeader("User-Agent", cliConfig.userAgent)); + const entReq = HttpClientRequest.get(`${apiUrl}/v1/organizations/${orgSlug}/entitlements`).pipe( + authHeader, + HttpClientRequest.setHeader("User-Agent", cliConfig.userAgent), + ); const entResp = yield* httpClient.execute(entReq).pipe(Effect.option); if (entResp._tag === "None" || entResp.value.status !== 200) { return; diff --git a/apps/cli/src/legacy/shared/legacy-upgrade-suggest.unit.test.ts b/apps/cli/src/legacy/shared/legacy-upgrade-suggest.unit.test.ts index 8b4246bd7c..ac088fe5f3 100644 --- a/apps/cli/src/legacy/shared/legacy-upgrade-suggest.unit.test.ts +++ b/apps/cli/src/legacy/shared/legacy-upgrade-suggest.unit.test.ts @@ -1,5 +1,5 @@ import { describe, expect, it } from "@effect/vitest"; -import { Effect } from "effect"; +import { Effect, Option, Redacted } from "effect"; import * as HttpClientRequest from "effect/unstable/http/HttpClientRequest"; import * as HttpClientResponse from "effect/unstable/http/HttpClientResponse"; @@ -288,6 +288,44 @@ describe("legacySuggestUpgrade", () => { }).pipe(Effect.provide(layer)); }); + it.live("a caller-provided reconciled token authenticates the fallback GETs", () => { + // Go resolves credentials for the process-wide reconciled CurrentProfile + // (`access_token.go:43`) — a reconciled caller passes its token with the + // URL so the stale profile's bearer token never follows the reconciled + // host (review r3684524241). + const { layer, api } = setup(); + return Effect.gen(function* () { + yield* legacySuggestUpgrade({ + projectRef: LEGACY_VALID_REF, + featureKey: "branching_limit", + statusCode: 402, + accessToken: Option.some(Redacted.make("sbp_reconciled_token")), + }); + expect(api.requests).toHaveLength(2); + for (const req of api.requests) { + expect(req.headers["authorization"]).toBe("Bearer sbp_reconciled_token"); + } + }).pipe(Effect.provide(layer)); + }); + + it.live("a reconciled profile with no token sends the fallback GETs unauthenticated", () => { + // `None` means the reconciled profile's own lookup found nothing — Go + // never falls back to the stale profile's token in that case. + const { layer, api } = setup(); + return Effect.gen(function* () { + yield* legacySuggestUpgrade({ + projectRef: LEGACY_VALID_REF, + featureKey: "branching_limit", + statusCode: 402, + accessToken: Option.none(), + }); + expect(api.requests).toHaveLength(2); + for (const req of api.requests) { + expect(req.headers["authorization"]).toBeUndefined(); + } + }).pipe(Effect.provide(layer)); + }); + it.live("no envelope and no featureKey is a no-op with zero API calls", () => { const { layer, out, analytics, api } = setup(); return Effect.gen(function* () { diff --git a/apps/cli/src/legacy/telemetry/legacy-linked-project-cache.layer.ts b/apps/cli/src/legacy/telemetry/legacy-linked-project-cache.layer.ts index 3083df568f..ab2538e3b7 100644 --- a/apps/cli/src/legacy/telemetry/legacy-linked-project-cache.layer.ts +++ b/apps/cli/src/legacy/telemetry/legacy-linked-project-cache.layer.ts @@ -54,20 +54,33 @@ export const legacyLinkedProjectCacheLayer = Layer.effect( const { stitch } = yield* LegacyIdentityStitch; return LegacyLinkedProjectCache.of({ - cache: (ref: string, workdir?: string) => + cache: ( + ref: string, + workdir?: string, + apiUrl?: string, + accessToken?: Option.Option>, + ) => Effect.gen(function* () { const cachePath = legacyTempPaths(path, workdir ?? cliConfig.workdir).linkedProjectCache; const exists = yield* fs.exists(cachePath).pipe(Effect.orElseSucceed(() => false)); if (exists) return; - // Resolve token: env wins over keyring/file lookup (Go-parity). - const tokenOpt = Option.isSome(cliConfig.accessToken) - ? cliConfig.accessToken - : yield* credentials.getAccessToken; + // Resolve token: an explicit reconciled-profile token wins outright + // (Some → use, None → the reconciled profile HAS no token, so skip + // like Go's failed lookup — never fall back to the stale profile's + // token, review r3684524241); otherwise env wins over keyring/file + // lookup (Go-parity). + const tokenOpt = + accessToken ?? + (Option.isSome(cliConfig.accessToken) + ? cliConfig.accessToken + : yield* credentials.getAccessToken); if (Option.isNone(tokenOpt)) return; const token = Redacted.value(tokenOpt.value); - const request = HttpClientRequest.get(`${cliConfig.apiUrl}/v1/projects/${ref}`).pipe( + const request = HttpClientRequest.get( + `${apiUrl ?? cliConfig.apiUrl}/v1/projects/${ref}`, + ).pipe( HttpClientRequest.setHeader("Authorization", `Bearer ${token}`), HttpClientRequest.setHeader("User-Agent", cliConfig.userAgent), ); diff --git a/apps/cli/src/legacy/telemetry/legacy-linked-project-cache.service.ts b/apps/cli/src/legacy/telemetry/legacy-linked-project-cache.service.ts index 92e3dceafc..3a7f83a9e7 100644 --- a/apps/cli/src/legacy/telemetry/legacy-linked-project-cache.service.ts +++ b/apps/cli/src/legacy/telemetry/legacy-linked-project-cache.service.ts @@ -1,4 +1,4 @@ -import type { Effect } from "effect"; +import type { Effect, Option, Redacted } from "effect"; import { Context } from "effect"; interface LegacyLinkedProjectCacheShape { @@ -15,8 +15,27 @@ interface LegacyLinkedProjectCacheShape { * Best-effort. Never fails the calling effect — auth errors, network errors, * and write errors are all swallowed (matches Go's `ensureProjectGroupsCached` * which logs to debug and returns). + * + * `apiUrl` overrides the Management API base URL of the cache-fill GET. + * Go's `ensureProjectGroupsCached` goes through `GetSupabase()` and the + * process-wide `CurrentProfile` — commands that reconcile a pflag-effective + * profile differing from the config layer's (sso add/update, PR #5974 + * round 7) pass that profile's URL. Defaults to `cliConfig.apiUrl`. + * + * `accessToken` complements `apiUrl`: Go resolves credentials for the same + * process-wide reconciled profile (`access_token.go:43`), so a reconciled + * caller passes the reconciled profile's token with the URL — the stale + * profile's bearer token must never be sent to the reconciled host (review + * r3684524241). `Some` uses that token, `None` skips the GET entirely + * (Go's token lookup fails before any request), `undefined` resolves from + * the config/credentials services. */ - readonly cache: (ref: string, workdir?: string) => Effect.Effect; + readonly cache: ( + ref: string, + workdir?: string, + apiUrl?: string, + accessToken?: Option.Option>, + ) => Effect.Effect; } export class LegacyLinkedProjectCache extends Context.Service< diff --git a/apps/cli/src/shared/cli/cobra-flag-groups.ts b/apps/cli/src/shared/cli/cobra-flag-groups.ts index bb3e20716a..8b2d6c0727 100644 --- a/apps/cli/src/shared/cli/cobra-flag-groups.ts +++ b/apps/cli/src/shared/cli/cobra-flag-groups.ts @@ -29,56 +29,369 @@ export function hasExplicitLongFlag( } /** - * Like `hasExplicitLongFlag`, but aware that a bare (`=`-less) occurrence of a - * *value-taking* flag consumes the very next argv token as its value — - * matching pflag's `parseLongArg` (`flag.go:1013-1031`), which takes the next - * raw arg unconditionally once a long flag needs a value, with no check that - * the token looks like another flag. + * Value-taking long flags registered persistently on the Go root command + * (`apps/cli-go/cmd/root.go:324-333`: `--workdir`, `--network-id`, + * `--profile`, `--output`, `--dns-resolver`, `--agent`), plus the TS-only + * `--output-format` global (`shared/cli/global-flags.ts`) which the TS + * parser accepts on any subcommand. pflag lets any of these consume the + * following argv token, so a pflag-faithful scan must know them or it will + * miscount positionals on perfectly normal invocations like + * `sso update --workdir . `. Keep in sync with `globalFlagsWithValues` + * in `shared/cli/run.ts`. + */ +export const PERSISTENT_VALUE_FLAG_NAMES: ReadonlySet = new Set([ + "workdir", + "network-id", + "profile", + "output", + "dns-resolver", + "agent", + "output-format", +]); + +/** + * Shorthands of the persistent value-taking flags above (`-o` → `--output`, + * `cmd/root.go:330`), mapped to their canonical long names. + */ +export const PERSISTENT_VALUE_FLAG_SHORTHANDS: ReadonlyMap = new Map([ + ["o", "output"], +]); + +export interface PflagArgvScanSpec { + /** + * Every value-taking (non-boolean) long flag reachable when this command + * parses: the command's own plus `PERSISTENT_VALUE_FLAG_NAMES`. Boolean + * flags never consume a token and must be omitted. + */ + readonly valueFlagNames: ReadonlySet; + /** + * Value-taking shorthand characters (`t` for `-t`) mapped to their + * canonical long names. Occurrences are recorded under the long name, + * matching pflag, whose `Visit` reports the canonical flag regardless of + * which form set it. + */ + readonly valueFlagShorthands?: ReadonlyMap; +} + +export interface PflagArgvScan { + /** Whether the command path was found in argv and the scan is scoped to it. */ + readonly anchored: boolean; + /** + * Every flag pflag would mark `Changed`, mapped to the raw value of each + * of its occurrences in argv order (shorthand occurrences under their + * canonical long name). + */ + readonly occurrences: ReadonlyMap>; + /** + * pflag-effective positional arguments: tokens not interpreted as flags + * and not consumed as a flag's value. Cobra's `ValidateArgs` (e.g. + * `ExactArgs(1)`) counts THESE, which can differ from what the Effect + * parser saw whenever pflag consumed a flag-shaped token as a value. + * Only populated when `anchored` — an unscoped scan cannot tell command + * path segments apart from operands. + */ + readonly positionals: ReadonlyArray; + /** + * Canonical long names of flags whose own token was consumed as another + * flag's value — pflag never parses them, so they stay unchanged even + * though the token is visibly present in argv. Covers both long tokens + * (`--type`, `--type=saml`) and mapped shorthand tokens (`-t`, `-t=saml`, + * `-tsaml`). Used to emulate cobra's `ValidateRequiredFlags` for flags the + * Effect parser believed were set. + */ + readonly consumedFlagNames: ReadonlySet; + /** + * Value-taking flags parsed BEFORE the command path completed — cobra's + * `Find`/`stripFlags` steps over them while routing to the subcommand, but + * pflag still parses them and marks them `Changed`, so a pre-path + * occurrence is the effective value when every post-path token of the same + * flag was consumed (e.g. `--profile A sso add --domains --profile`: + * pflag keeps A). Values in argv order. Boolean pre-path flags are not + * tracked — this exists for last-wins reconciliation of value-taking + * persistent flags (PR #5974 review round 10). + */ + readonly prePathOccurrences: ReadonlyMap>; + /** + * pflag's `ValueRequiredError` message when a bare value-taking flag is + * the final argv token — byte-exact per pflag `errors.go:75,78` + * (`flag needs an argument: --domains` / `flag needs an argument: 't' in + * -t`). pflag fails `ParseFlags` (cobra `command.go:919`) before + * `ValidateArgs`, every hook, `ValidateRequiredFlags`, and + * `ValidateFlagGroups`, so handlers must reject argv carrying this before + * any other check or side effect (binary-verified: `sso update a b + * --domains` reports the missing argument, not the arity violation). + */ + readonly missingValueError: string | undefined; +} + +/** + * Walks a shorthand cluster (`token.slice(1)`) the way pflag's + * `parseShortArg`/`parseSingleShortArg` does: characters before the first + * value-taking shorthand are boolean/unknown and consume nothing; the first + * value-taking shorthand either carries an inline value (`-o=json`, `-ojson`) + * or must consume the next argv token (`-o`). Returns `undefined` when no + * character maps to a value-taking flag. + */ +function clusterValueShorthand( + cluster: string, + valueFlagShorthands: ReadonlyMap, +): + | { readonly longName: string; readonly remaining: string; readonly inlineValue?: string } + | undefined { + let shorthands = cluster; + while (shorthands.length > 0) { + const longName = valueFlagShorthands.get(shorthands[0] ?? ""); + if (longName === undefined) { + // Boolean or unknown shorthand — consumes nothing. + shorthands = shorthands.slice(1); + continue; + } + if (shorthands.length > 2 && shorthands[1] === "=") { + return { longName, remaining: shorthands, inlineValue: shorthands.slice(2) }; // `-o=json` + } + if (shorthands.length > 1) { + return { longName, remaining: shorthands, inlineValue: shorthands.slice(1) }; // `-ojson` + } + return { longName, remaining: shorthands }; // `-o json` + } + return undefined; +} + +/** + * Scans raw argv the way pflag's `parseArgs`/`parseLongArg`/ + * `parseSingleShortArg` (`flag.go:1013-1031, 1080-1094`) would, returning + * every flag pflag would mark `Changed` after the command path, the + * pflag-effective positional arguments, and the flags whose own tokens got + * consumed as values. + * + * pflag-faithful rules: + * - `--name=value` records `value` (split on the first `=`) for any flag. + * - A bare `--name` where `name` is a *value-taking* flag + * (`spec.valueFlagNames`) unconditionally consumes the very next argv + * token as its value — even a flag-shaped token or `--`. pflag never + * checks that the consumed token "looks like a value", so + * `--metadata-file --metadata-url` is pflag's `metadata-file` flag being + * handed the (oddly named, but valid) string value `"--metadata-url"` — + * `metadata-url.Changed` stays `false`. The vendored Effect parser + * deliberately differs (`internal/parser.ts` only consumes `Value`-tagged + * tokens), which is why handlers must reconcile the values they act on + * against this scan instead of trusting the parsed options alone + * (CLI-1982). + * - A bare `--name` not in `valueFlagNames` (a boolean flag) records pflag's + * bool `NoOptDefVal` `"true"` (`flag.go:1017-1019`) and consumes nothing, + * while an inline-empty `--name=` records `""` (`flag.go:1014-1016`) — the + * two must stay distinguishable because pflag hands `""` to + * `strconv.ParseBool`, which rejects it (PR #5974 review round 5: + * `--skip-url-validation=false --skip-url-validation=` aborts Go's + * ParseFlags; a bare repeat ends true). + * - A shorthand cluster is walked per pflag's `parseSingleShortArg`: a + * value-taking shorthand takes `-t=v` / `-tv` inline or consumes the next + * token for `-t v`; other characters (booleans, `-h`) consume nothing. + * - A bare `--` that was not consumed as a value terminates flag parsing; + * every remaining token is positional (pflag `parseArgs`). + * - A bare value-taking flag as the very last token records nothing and + * reports pflag's `flag needs an argument` parse error via + * `missingValueError` — pflag aborts before the flag is ever Set. * - * Without this, scanning independently per flag name (as `hasExplicitLongFlag` - * does) can mistake a consumed value for a literal occurrence of a sibling - * mutex flag: `--metadata-file --metadata-url` is pflag's `metadata-file` - * flag being handed the (oddly named, but valid) string value - * `"--metadata-url"` — cobra never parses `--metadata-url` as its own flag, - * so `metadata-url.Changed` stays `false`. A naive scan sees both tokens and - * wrongly reports both as set. + * Anchoring mirrors cobra's `Find`/`stripFlags`: persistent flags (and the + * values they consume) may sit before or between command path segments — + * `sso --profile foo update ` routes to `sso update` — so the walk steps + * over flag tokens while matching path segments in order. When the path + * cannot be completed (a stray operand, a `--`, or argv ends), the scan + * falls back to an unscoped pass over the whole argv. * - * `valueFlagNames` must list every value-taking (non-boolean) flag declared - * on the command being scanned, so the scan knows which bare tokens consume a - * following value; boolean flags never consume one and must be omitted. This - * only covers flags local to the command — a global/inherited value-taking - * flag immediately preceding a mutex flag without `=` can still be misread - * the same way; closing that fully would mean teaching this scan about every - * flag reachable at parse time, not just the command's own, which is a - * bigger, cross-cutting change. + * Remaining gap, kept deliberately: the spec is written out per command + * rather than derived from the command tree, and unknown flags are treated + * as non-consuming. That is fail-open — argv carrying flags outside the + * spec is rejected by the Effect parser before any handler runs, so the + * scan can never invent a false positional (and with it a false arity + * error) for an invocation that actually reaches a handler. */ -export function hasExplicitValueFlag( +export function pflagArgvScan( rawArgs: ReadonlyArray, commandPath: ReadonlyArray, - valueFlagNames: ReadonlySet, - flagName: string, -): boolean { - const commandIndex = rawArgs.findIndex((_, index) => - commandPath.every((segment, offset) => rawArgs[index + offset] === segment), - ); - const scoped = commandIndex !== -1; - const tokens = scoped ? rawArgs.slice(commandIndex + commandPath.length) : rawArgs; + spec: PflagArgvScanSpec, +): PflagArgvScan { + const valueFlagNames = spec.valueFlagNames; + const valueFlagShorthands = spec.valueFlagShorthands ?? new Map(); + // Anchor: match the command path segments in argv order, stepping over + // flag tokens and the values they consume (cobra `stripFlags` consumes the + // next token for any bare value-taking flag while locating subcommands). + let segmentIndex = 0; + let cursor = 0; + const prePathOccurrences = new Map>(); + const recordPrePath = (name: string, value: string) => { + const existing = prePathOccurrences.get(name); + if (existing === undefined) { + prePathOccurrences.set(name, [value]); + } else { + existing.push(value); + } + }; + while (cursor < rawArgs.length && segmentIndex < commandPath.length) { + const token = rawArgs[cursor]; + if (token === undefined || token === "--") { + break; // `--` ends flag parsing — the path can no longer be completed. + } + if (token === commandPath[segmentIndex]) { + segmentIndex += 1; + cursor += 1; + continue; + } + if (!token.startsWith("-") || token === "-") { + break; // A stray operand — this argv does not route to the command. + } + if (token.startsWith("--")) { + const equalsIndex = token.indexOf("="); + if (equalsIndex !== -1) { + const name = token.slice(2, equalsIndex); + if (valueFlagNames.has(name)) { + recordPrePath(name, token.slice(equalsIndex + 1)); + } + cursor += 1; + continue; + } + const name = token.slice(2); + if (valueFlagNames.has(name)) { + const value = rawArgs[cursor + 1]; + if (value !== undefined) { + recordPrePath(name, value); + } + cursor += 2; + continue; + } + cursor += 1; + continue; + } + const hit = clusterValueShorthand(token.slice(1), valueFlagShorthands); + if (hit !== undefined) { + if (hit.inlineValue !== undefined) { + recordPrePath(hit.longName, hit.inlineValue); + cursor += 1; + } else { + const value = rawArgs[cursor + 1]; + if (value !== undefined) { + recordPrePath(hit.longName, value); + } + cursor += 2; + } + continue; + } + cursor += 1; + } + const anchored = segmentIndex === commandPath.length; + const tokens = anchored ? rawArgs.slice(cursor) : rawArgs; + // Like `positionals`, pre-path occurrences are only meaningful when the + // scan anchored — an unscoped scan re-walks the whole argv below, so + // keeping the partial walk's records would double-count them. + if (!anchored) { + prePathOccurrences.clear(); + } + + const occurrences = new Map>(); + const positionals: Array = []; + const consumedFlagNames = new Set(); + let missingValueError: string | undefined; + const record = (name: string, value: string) => { + const existing = occurrences.get(name); + if (existing === undefined) { + occurrences.set(name, [value]); + } else { + existing.push(value); + } + }; + // pflag's `--flag arg` / `-f arg` branches: the very next token is the + // value, unconditionally, so it can't be read as a flag (or positional) of + // its own. When that token is itself a flag pflag never got to parse, + // remember its canonical name. When there is no next token, pflag fails + // parsing (`errors.go:63-78`) — nothing is recorded because the flag never + // gets Set. Returns the index to resume scanning from. + const consumeNext = (name: string, index: number, missingMessage: string): number => { + const next = tokens[index + 1]; + if (next === undefined) { + missingValueError ??= missingMessage; + return index; + } + record(name, next); + if (next.startsWith("--") && next.length > 2) { + const equalsIndex = next.indexOf("="); + consumedFlagNames.add(next.slice(2, equalsIndex === -1 ? undefined : equalsIndex)); + } else if (next.startsWith("-") && !next.startsWith("--") && next !== "-") { + // A consumed shorthand cluster (`--domains -t saml`): pflag never + // parses `-t`, so `type` stays unchanged even though the Effect parser + // read it as a flag (CLI-1982, PR #5974 review round 3). + const hit = clusterValueShorthand(next.slice(1), valueFlagShorthands); + if (hit !== undefined) { + consumedFlagNames.add(hit.longName); + } + } + return index + 1; + }; for (let index = 0; index < tokens.length; index += 1) { const token = tokens[index]; - if (token === undefined || (scoped && token === "--")) { - return false; + if (token === undefined) { + break; } - if (token === `--${flagName}` || token.startsWith(`--${flagName}=`)) { - return true; + if (token === "--") { + // Only terminate when anchored to the command path — an unscoped scan + // cannot tell whether `--` belongs to this command's flags at all. + if (anchored) { + positionals.push(...tokens.slice(index + 1)); + break; + } + continue; + } + if (!token.startsWith("-") || token === "-") { + // pflag `parseArgs`: an empty token, a token without a leading dash, + // or a lone `-` is an operand. + if (anchored) { + positionals.push(token); + } + continue; + } + if (!token.startsWith("--")) { + // Shorthand cluster — pflag `parseShortArg` walks it one character at + // a time; the first value-taking shorthand ends the cluster. + const hit = clusterValueShorthand(token.slice(1), valueFlagShorthands); + if (hit !== undefined) { + if (hit.inlineValue !== undefined) { + record(hit.longName, hit.inlineValue); // `-o=json` / `-ojson` + } else { + // `-o json` — pflag `errors.go:75` quotes the shorthand character + // and the remaining cluster when the value is missing. + index = consumeNext( + hit.longName, + index, + `flag needs an argument: '${hit.remaining[0]}' in -${hit.remaining}`, + ); + } + } + continue; } - if (token.startsWith("--") && !token.includes("=") && valueFlagNames.has(token.slice(2))) { - // Bare occurrence of a value-taking flag — skip the token it consumes - // so it can't be mistaken for a literal occurrence of `flagName`. - index += 1; + const equalsIndex = token.indexOf("="); + if (equalsIndex !== -1) { + record(token.slice(2, equalsIndex), token.slice(equalsIndex + 1)); + continue; + } + const name = token.slice(2); + if (valueFlagNames.has(name)) { + index = consumeNext(name, index, `flag needs an argument: --${name}`); + } else { + // pflag's `NoOptDefVal` branch (`flag.go:1017-1019`): a bare boolean + // records the value pflag would Set — `"true"` — keeping it distinct + // from the `""` an inline-empty `--name=` records above. + record(name, "true"); } } - return false; + return { + anchored, + occurrences, + positionals, + consumedFlagNames, + prePathOccurrences, + missingValueError, + }; } /** diff --git a/apps/cli/src/shared/cli/cobra-flag-groups.unit.test.ts b/apps/cli/src/shared/cli/cobra-flag-groups.unit.test.ts index 2a410121da..b3af68a036 100644 --- a/apps/cli/src/shared/cli/cobra-flag-groups.unit.test.ts +++ b/apps/cli/src/shared/cli/cobra-flag-groups.unit.test.ts @@ -2,7 +2,9 @@ import { describe, expect, test } from "vitest"; import { cobraMutuallyExclusiveErrorMessage, hasExplicitLongFlag, - hasExplicitValueFlag, + PERSISTENT_VALUE_FLAG_NAMES, + PERSISTENT_VALUE_FLAG_SHORTHANDS, + pflagArgvScan, } from "./cobra-flag-groups.ts"; const COMMAND_PATH = ["functions", "deploy"] as const; @@ -48,85 +50,477 @@ describe("hasExplicitLongFlag", () => { }); }); -describe("hasExplicitValueFlag", () => { +describe("pflagArgvScan", () => { const SSO_UPDATE_PATH = ["sso", "update"] as const; - const VALUE_FLAGS = new Set(["metadata-file", "metadata-url", "domains", "add-domains"]); + const SPEC = { + valueFlagNames: new Set([ + "metadata-file", + "metadata-url", + "domains", + "add-domains", + ...PERSISTENT_VALUE_FLAG_NAMES, + ]), + valueFlagShorthands: PERSISTENT_VALUE_FLAG_SHORTHANDS, + }; - test("finds a bare flag after the command path", () => { - expect( - hasExplicitValueFlag( - ["sso", "update", "id", "--metadata-file", "foo.xml"], - SSO_UPDATE_PATH, - VALUE_FLAGS, - "metadata-file", - ), - ).toBe(true); + test("records value-taking persistent flags parsed BEFORE the command path", () => { + // cobra's Find/stripFlags steps over `--profile a.yml` while routing to + // `sso update`, but pflag parses it and marks it Changed — the pre-path + // occurrence is the effective value when later tokens are consumed + // (PR #5974 review round 10). + const scan = pflagArgvScan( + ["--profile", "a.yml", "sso", "update", "id", "--profile=b.yml"], + SSO_UPDATE_PATH, + SPEC, + ); + expect(scan.anchored).toBe(true); + expect(scan.prePathOccurrences.get("profile")).toEqual(["a.yml"]); + expect(scan.occurrences.get("profile")).toEqual(["b.yml"]); }); - test("finds a flag with an inline value", () => { - expect( - hasExplicitValueFlag( - ["sso", "update", "id", "--domains=a.com"], - SSO_UPDATE_PATH, - VALUE_FLAGS, - "domains", - ), - ).toBe(true); + test("records inline and repeated pre-path values in argv order", () => { + const scan = pflagArgvScan( + ["--profile=a.yml", "--profile", "b.yml", "sso", "update", "id"], + SSO_UPDATE_PATH, + SPEC, + ); + expect(scan.prePathOccurrences.get("profile")).toEqual(["a.yml", "b.yml"]); + expect(scan.occurrences.get("profile")).toBeUndefined(); }); - test("does not mistake a value-taking flag's consumed value for a sibling flag", () => { + test("records a bare flag's next token as its value", () => { + const { occurrences } = pflagArgvScan( + ["sso", "update", "id", "--metadata-file", "foo.xml"], + SSO_UPDATE_PATH, + SPEC, + ); + expect(occurrences.get("metadata-file")).toEqual(["foo.xml"]); + }); + + test("records an inline (`=`) value, split on the first `=`", () => { + const { occurrences } = pflagArgvScan( + ["sso", "update", "id", "--domains=a.com", "--metadata-file=a=b"], + SSO_UPDATE_PATH, + SPEC, + ); + expect(occurrences.get("domains")).toEqual(["a.com"]); + expect(occurrences.get("metadata-file")).toEqual(["a=b"]); + }); + + test("an explicit empty `--flag=` still counts as changed", () => { + const { occurrences } = pflagArgvScan( + ["sso", "update", "id", "--metadata-file="], + SSO_UPDATE_PATH, + SPEC, + ); + expect(occurrences.get("metadata-file")).toEqual([""]); + }); + + test("a consumed flag-shaped token becomes the value, not a sibling flag", () => { // pflag's `--flag arg` branch consumes the next token unconditionally // (`flag.go:1013-1031`), so `--metadata-file --metadata-url` gives // `metadata-file` the literal value `"--metadata-url"` and never parses // `--metadata-url` as its own flag. - const args = ["sso", "update", "id", "--metadata-file", "--metadata-url"]; - expect(hasExplicitValueFlag(args, SSO_UPDATE_PATH, VALUE_FLAGS, "metadata-file")).toBe(true); - expect(hasExplicitValueFlag(args, SSO_UPDATE_PATH, VALUE_FLAGS, "metadata-url")).toBe(false); + const scan = pflagArgvScan( + ["sso", "update", "id", "--metadata-file", "--metadata-url"], + SSO_UPDATE_PATH, + SPEC, + ); + expect(scan.occurrences.get("metadata-file")).toEqual(["--metadata-url"]); + expect(scan.occurrences.has("metadata-url")).toBe(false); + expect(scan.consumedFlagNames.has("metadata-url")).toBe(true); }); - test("does not mistake a value-taking flag's consumed value for a sibling flag, reversed", () => { - const args = ["sso", "update", "id", "--metadata-url", "--metadata-file"]; - expect(hasExplicitValueFlag(args, SSO_UPDATE_PATH, VALUE_FLAGS, "metadata-url")).toBe(true); - expect(hasExplicitValueFlag(args, SSO_UPDATE_PATH, VALUE_FLAGS, "metadata-file")).toBe(false); + test("a consumed flag-shaped token becomes the value, reversed order", () => { + const scan = pflagArgvScan( + ["sso", "update", "id", "--metadata-url", "--metadata-file"], + SSO_UPDATE_PATH, + SPEC, + ); + expect(scan.occurrences.get("metadata-url")).toEqual(["--metadata-file"]); + expect(scan.occurrences.has("metadata-file")).toBe(false); + expect(scan.consumedFlagNames.has("metadata-file")).toBe(true); }); - test("an inline (`=`) value is never treated as consuming the next token", () => { + test("an inline (`=`) value never consumes the next token", () => { // `--metadata-file=--metadata-url` is one token: metadata-file's value is // the literal string "--metadata-url", and no token is consumed after it. - const args = ["sso", "update", "id", "--metadata-file=--metadata-url", "--domains", "a.com"]; - expect(hasExplicitValueFlag(args, SSO_UPDATE_PATH, VALUE_FLAGS, "metadata-file")).toBe(true); - expect(hasExplicitValueFlag(args, SSO_UPDATE_PATH, VALUE_FLAGS, "metadata-url")).toBe(false); - expect(hasExplicitValueFlag(args, SSO_UPDATE_PATH, VALUE_FLAGS, "domains")).toBe(true); + const scan = pflagArgvScan( + ["sso", "update", "id", "--metadata-file=--metadata-url", "--domains", "a.com"], + SSO_UPDATE_PATH, + SPEC, + ); + expect(scan.occurrences.get("metadata-file")).toEqual(["--metadata-url"]); + expect(scan.occurrences.has("metadata-url")).toBe(false); + expect(scan.occurrences.get("domains")).toEqual(["a.com"]); + // The inline form consumed nothing — no flag token was swallowed. + expect(scan.consumedFlagNames.size).toBe(0); }); - test("a real, non-adjacent occurrence of both flags is still detected", () => { - const args = ["sso", "update", "id", "--metadata-file", "foo.xml", "--metadata-url", "url"]; - expect(hasExplicitValueFlag(args, SSO_UPDATE_PATH, VALUE_FLAGS, "metadata-file")).toBe(true); - expect(hasExplicitValueFlag(args, SSO_UPDATE_PATH, VALUE_FLAGS, "metadata-url")).toBe(true); + test("real, non-adjacent occurrences of both flags are both recorded", () => { + const { occurrences } = pflagArgvScan( + ["sso", "update", "id", "--metadata-file", "foo.xml", "--metadata-url", "url"], + SSO_UPDATE_PATH, + SPEC, + ); + expect(occurrences.get("metadata-file")).toEqual(["foo.xml"]); + expect(occurrences.get("metadata-url")).toEqual(["url"]); }); - test("returns false when the flag is absent", () => { - expect( - hasExplicitValueFlag(["sso", "update", "id"], SSO_UPDATE_PATH, VALUE_FLAGS, "domains"), - ).toBe(false); + test("repeated occurrences accumulate in argv order", () => { + const { occurrences } = pflagArgvScan( + ["sso", "update", "id", "--domains", "a.com", "--domains=b.com"], + SSO_UPDATE_PATH, + SPEC, + ); + expect(occurrences.get("domains")).toEqual(["a.com", "b.com"]); }); - test("stops scanning at a -- terminator", () => { - expect( - hasExplicitValueFlag( - ["sso", "update", "id", "--", "--domains"], + test("a bare boolean (non-value) flag records pflag's NoOptDefVal true without consuming", () => { + const scan = pflagArgvScan( + ["sso", "update", "id", "--skip-url-validation", "--metadata-url", "url"], + SSO_UPDATE_PATH, + SPEC, + ); + expect(scan.occurrences.get("skip-url-validation")).toEqual(["true"]); + expect(scan.occurrences.get("metadata-url")).toEqual(["url"]); + expect(scan.positionals).toEqual(["id"]); + }); + + test("an inline-empty boolean (`--flag=`) records the empty string, distinct from bare", () => { + // pflag `flag.go:1014-1019`: `--flag=` Sets `""` (which ParseBool later + // rejects) while a bare `--flag` Sets NoOptDefVal `"true"` — the scan + // must preserve that difference (PR #5974 review round 5). + const scan = pflagArgvScan( + ["sso", "update", "id", "--skip-url-validation=false", "--skip-url-validation="], + SSO_UPDATE_PATH, + SPEC, + ); + expect(scan.occurrences.get("skip-url-validation")).toEqual(["false", ""]); + }); + + test("returns an empty map when no flags are present", () => { + expect(pflagArgvScan(["sso", "update", "id"], SSO_UPDATE_PATH, SPEC).occurrences.size).toBe(0); + }); + + test("flags after a -- terminator are not recorded", () => { + const { occurrences } = pflagArgvScan( + ["sso", "update", "id", "--", "--domains"], + SSO_UPDATE_PATH, + SPEC, + ); + expect(occurrences.has("domains")).toBe(false); + }); + + test("a -- consumed as a bare value flag's value does not terminate the scan", () => { + const { occurrences } = pflagArgvScan( + ["sso", "update", "id", "--metadata-file", "--", "--domains", "a.com"], + SSO_UPDATE_PATH, + SPEC, + ); + expect(occurrences.get("metadata-file")).toEqual(["--"]); + expect(occurrences.get("domains")).toEqual(["a.com"]); + }); + + describe("missing value detection (pflag ValueRequiredError parity)", () => { + test("a bare value flag at the end of argv reports pflag's parse error", () => { + // pflag fails `ParseFlags` before anything else runs (`errors.go:78`), + // and the flag is never Set — so no occurrence is recorded either. + const scan = pflagArgvScan(["sso", "update", "id", "--metadata-file"], SSO_UPDATE_PATH, SPEC); + expect(scan.missingValueError).toBe("flag needs an argument: --metadata-file"); + expect(scan.occurrences.has("metadata-file")).toBe(false); + }); + + test("a bare value shorthand at the end of argv quotes the character", () => { + // pflag `errors.go:75`: `flag needs an argument: %q in -%s`. + const scan = pflagArgvScan(["sso", "update", "id", "-o"], SSO_UPDATE_PATH, SPEC); + expect(scan.missingValueError).toBe("flag needs an argument: 'o' in -o"); + expect(scan.occurrences.has("output")).toBe(false); + }); + + test("an inline empty value (`--flag=`) is not a missing value", () => { + const scan = pflagArgvScan( + ["sso", "update", "id", "--metadata-file="], SSO_UPDATE_PATH, - VALUE_FLAGS, - "domains", - ), - ).toBe(false); + SPEC, + ); + expect(scan.missingValueError).toBeUndefined(); + expect(scan.occurrences.get("metadata-file")).toEqual([""]); + }); + + test("a trailing boolean flag is not a missing value", () => { + const scan = pflagArgvScan( + ["sso", "update", "id", "--skip-url-validation"], + SSO_UPDATE_PATH, + SPEC, + ); + expect(scan.missingValueError).toBeUndefined(); + }); + + test("a value flag consuming a flag-shaped token is not a missing value", () => { + const scan = pflagArgvScan( + ["sso", "update", "id", "--domains", "--metadata-url"], + SSO_UPDATE_PATH, + SPEC, + ); + expect(scan.missingValueError).toBeUndefined(); + expect(scan.occurrences.get("domains")).toEqual(["--metadata-url"]); + }); }); - test("falls back to a bare scan when the command path is not found", () => { - expect(hasExplicitValueFlag(["--domains"], SSO_UPDATE_PATH, VALUE_FLAGS, "domains")).toBe(true); - expect(hasExplicitValueFlag(["--metadata-file"], SSO_UPDATE_PATH, VALUE_FLAGS, "domains")).toBe( - false, + test("ignores flags that appear before the command path", () => { + const { occurrences } = pflagArgvScan( + ["--domains", "a.com", "sso", "update", "id"], + SSO_UPDATE_PATH, + SPEC, ); + expect(occurrences.has("domains")).toBe(false); + }); + + test("falls back to a bare, unanchored scan when the command path is not found", () => { + const scan = pflagArgvScan(["--domains", "a.com"], SSO_UPDATE_PATH, SPEC); + expect(scan.anchored).toBe(false); + expect(scan.occurrences.get("domains")).toEqual(["a.com"]); + // An unscoped scan collects no positionals — it cannot tell command path + // segments apart from operands. + expect(scan.positionals).toEqual([]); + }); + + test("an unscoped scan does not treat -- as a terminator", () => { + const { occurrences } = pflagArgvScan(["--", "--domains", "a.com"], SSO_UPDATE_PATH, SPEC); + expect(occurrences.get("domains")).toEqual(["a.com"]); + }); + + describe("positional counting (cobra ValidateArgs parity)", () => { + test("a plain invocation has exactly the operands as positionals", () => { + const scan = pflagArgvScan( + ["sso", "update", "id", "--domains", "a.com"], + SSO_UPDATE_PATH, + SPEC, + ); + expect(scan.anchored).toBe(true); + expect(scan.positionals).toEqual(["id"]); + }); + + test("a consumed flag token shifts its parser-value into the positionals", () => { + // pflag hands `--metadata-url` to `--domains`; the URL the Effect + // parser read as metadata-url's value is a positional to pflag, so + // cobra's ExactArgs(1) sees 2 args (CLI-1982, PR #5974 review). + const scan = pflagArgvScan( + ["sso", "update", "--domains", "--metadata-url", "https://idp.example.com/m", "id"], + SSO_UPDATE_PATH, + SPEC, + ); + expect(scan.occurrences.get("domains")).toEqual(["--metadata-url"]); + expect(scan.occurrences.has("metadata-url")).toBe(false); + expect(scan.positionals).toEqual(["https://idp.example.com/m", "id"]); + }); + + test("a persistent global value flag consumes its value token", () => { + // `--workdir .` must not count `.` as a positional — pflag consumes it + // (root persistent flags, `cmd/root.go:324-333`). + const scan = pflagArgvScan( + ["sso", "update", "--workdir", ".", "id", "--profile", "staging"], + SSO_UPDATE_PATH, + SPEC, + ); + expect(scan.positionals).toEqual(["id"]); + }); + + test("a bare slice flag consumes a global flag token, orphaning its value", () => { + // Binary-verified Go behaviour: `--domains --profile staging ` + // arity-errors because `staging` becomes positional. + const scan = pflagArgvScan( + ["sso", "update", "--domains", "--profile", "staging", "id"], + SSO_UPDATE_PATH, + SPEC, + ); + expect(scan.occurrences.get("domains")).toEqual(["--profile"]); + expect(scan.consumedFlagNames.has("profile")).toBe(true); + expect(scan.positionals).toEqual(["staging", "id"]); + }); + + test("tokens after a live -- terminator are all positionals", () => { + const scan = pflagArgvScan( + ["sso", "update", "id", "--", "--domains", "x"], + SSO_UPDATE_PATH, + SPEC, + ); + expect(scan.positionals).toEqual(["id", "--domains", "x"]); + }); + + test("a lone - is a positional", () => { + const scan = pflagArgvScan(["sso", "update", "-"], SSO_UPDATE_PATH, SPEC); + expect(scan.positionals).toEqual(["-"]); + }); + }); + + describe("shorthand handling (pflag parseSingleShortArg parity)", () => { + const ADD_PATH = ["sso", "add"] as const; + const ADD_SPEC = { + valueFlagNames: new Set(["type", "domains", "metadata-url", ...PERSISTENT_VALUE_FLAG_NAMES]), + valueFlagShorthands: new Map([["t", "type"], ...PERSISTENT_VALUE_FLAG_SHORTHANDS]), + }; + + test("`-t saml` consumes the next token and records under the long name", () => { + const scan = pflagArgvScan(["sso", "add", "-t", "saml"], ADD_PATH, ADD_SPEC); + expect(scan.occurrences.get("type")).toEqual(["saml"]); + expect(scan.positionals).toEqual([]); + }); + + test("`-o json` consumes the next token via the persistent shorthand map", () => { + const scan = pflagArgvScan(["sso", "update", "-o", "json", "id"], SSO_UPDATE_PATH, SPEC); + expect(scan.occurrences.get("output")).toEqual(["json"]); + expect(scan.positionals).toEqual(["id"]); + }); + + test("`-o=json` and `-ojson` are self-contained", () => { + const eq = pflagArgvScan(["sso", "update", "-o=json", "id"], SSO_UPDATE_PATH, SPEC); + expect(eq.occurrences.get("output")).toEqual(["json"]); + expect(eq.positionals).toEqual(["id"]); + const glued = pflagArgvScan(["sso", "update", "-ojson", "id"], SSO_UPDATE_PATH, SPEC); + expect(glued.occurrences.get("output")).toEqual(["json"]); + expect(glued.positionals).toEqual(["id"]); + }); + + test("unknown/boolean shorthands consume nothing", () => { + const scan = pflagArgvScan(["sso", "update", "-h", "id"], SSO_UPDATE_PATH, SPEC); + expect(scan.occurrences.size).toBe(0); + expect(scan.positionals).toEqual(["id"]); + }); + }); + + describe("anchoring across interspersed flags (cobra Find/stripFlags parity)", () => { + test("a persistent value flag between group and leaf still anchors", () => { + // cobra routes `sso --profile foo update ` to `sso update` + // (`stripFlags` steps over the flag and its value while locating + // subcommands) — binary-verified; the arity re-count must not be + // skipped for such argv (PR #5974 review round 3). + const scan = pflagArgvScan( + ["sso", "--profile", "foo", "update", "id", "--domains", "a.com"], + SSO_UPDATE_PATH, + SPEC, + ); + expect(scan.anchored).toBe(true); + expect(scan.positionals).toEqual(["id"]); + expect(scan.occurrences.get("domains")).toEqual(["a.com"]); + }); + + test("an interspersed value flag consuming a path-named token still anchors", () => { + // `--profile update` hands the literal value "update" to --profile; + // the NEXT "update" is the real leaf segment. + const scan = pflagArgvScan( + ["sso", "--profile", "update", "update", "id"], + SSO_UPDATE_PATH, + SPEC, + ); + expect(scan.anchored).toBe(true); + expect(scan.positionals).toEqual(["id"]); + }); + + test("interspersed boolean and self-contained shorthand flags still anchor", () => { + const debug = pflagArgvScan(["sso", "--debug", "update", "id"], SSO_UPDATE_PATH, SPEC); + expect(debug.anchored).toBe(true); + expect(debug.positionals).toEqual(["id"]); + const glued = pflagArgvScan(["sso", "-ojson", "update", "id"], SSO_UPDATE_PATH, SPEC); + expect(glued.anchored).toBe(true); + expect(glued.positionals).toEqual(["id"]); + }); + + test("an interspersed value shorthand consumes its value token", () => { + const scan = pflagArgvScan(["sso", "-o", "json", "update", "id"], SSO_UPDATE_PATH, SPEC); + expect(scan.anchored).toBe(true); + expect(scan.positionals).toEqual(["id"]); + }); + + test("a leading value flag whose value collides with a path segment still anchors", () => { + const scan = pflagArgvScan( + ["--profile", "sso", "sso", "update", "id"], + SSO_UPDATE_PATH, + SPEC, + ); + expect(scan.anchored).toBe(true); + expect(scan.positionals).toEqual(["id"]); + }); + + test("a stray operand before the path fails the anchor open", () => { + // `sso foo update` never routes to `sso update` (cobra treats `foo` + // as an unknown subcommand), so the scan falls back to unscoped. + const scan = pflagArgvScan(["sso", "foo", "update", "id"], SSO_UPDATE_PATH, SPEC); + expect(scan.anchored).toBe(false); + expect(scan.positionals).toEqual([]); + }); + + test("a -- before the path completes fails the anchor open", () => { + const scan = pflagArgvScan(["sso", "--", "update", "id"], SSO_UPDATE_PATH, SPEC); + expect(scan.anchored).toBe(false); + }); + }); + + describe("consumed flag tracking (cobra ValidateRequiredFlags parity)", () => { + const ADD_PATH = ["sso", "add"] as const; + const ADD_SPEC = { + valueFlagNames: new Set(["type", "domains", ...PERSISTENT_VALUE_FLAG_NAMES]), + valueFlagShorthands: new Map([["t", "type"], ...PERSISTENT_VALUE_FLAG_SHORTHANDS]), + }; + + test("a consumed bare `--type` is tracked and not marked changed", () => { + const scan = pflagArgvScan(["sso", "add", "--domains", "--type", "saml"], ADD_PATH, ADD_SPEC); + expect(scan.occurrences.has("type")).toBe(false); + expect(scan.consumedFlagNames.has("type")).toBe(true); + }); + + test("a consumed `--type=saml` is tracked by its name before the `=`", () => { + const scan = pflagArgvScan(["sso", "add", "--domains", "--type=saml"], ADD_PATH, ADD_SPEC); + expect(scan.occurrences.has("type")).toBe(false); + expect(scan.consumedFlagNames.has("type")).toBe(true); + }); + + test("a consumed `-t` shorthand is tracked under its long name", () => { + // Binary-verified: `sso add --domains -t saml` fails Go's + // required-flag check — pflag hands `-t` to `--domains` and `type` + // stays unchanged, while `saml` becomes positional (PR #5974 review + // round 3). + const scan = pflagArgvScan(["sso", "add", "--domains", "-t", "saml"], ADD_PATH, ADD_SPEC); + expect(scan.occurrences.has("type")).toBe(false); + expect(scan.consumedFlagNames.has("type")).toBe(true); + expect(scan.occurrences.get("domains")).toEqual(["-t"]); + expect(scan.positionals).toEqual(["saml"]); + }); + + test("consumed `-t=saml` and `-tsaml` shorthand forms are tracked too", () => { + const inline = pflagArgvScan(["sso", "add", "--domains", "-t=saml"], ADD_PATH, ADD_SPEC); + expect(inline.occurrences.has("type")).toBe(false); + expect(inline.consumedFlagNames.has("type")).toBe(true); + const glued = pflagArgvScan(["sso", "add", "--domains", "-tsaml"], ADD_PATH, ADD_SPEC); + expect(glued.occurrences.has("type")).toBe(false); + expect(glued.consumedFlagNames.has("type")).toBe(true); + }); + + test("a consumed unmapped shorthand records no name", () => { + const scan = pflagArgvScan(["sso", "add", "--domains", "-h"], ADD_PATH, ADD_SPEC); + expect(scan.occurrences.get("domains")).toEqual(["-h"]); + expect(scan.consumedFlagNames.size).toBe(0); + }); + + test("a shorthand `-t` occurrence coexists with a consumed `--type` token", () => { + // pflag: `-t saml` sets type; `--domains` then swallows `--type`. The + // flag IS changed, so required-flag emulation must not fire. + const scan = pflagArgvScan( + ["sso", "add", "-t", "saml", "--domains", "--type", "saml"], + ADD_PATH, + ADD_SPEC, + ); + expect(scan.occurrences.get("type")).toEqual(["saml"]); + expect(scan.consumedFlagNames.has("type")).toBe(true); + }); + + test("a consumed `--` records no name", () => { + const scan = pflagArgvScan(["sso", "add", "--domains", "--", "x"], ADD_PATH, ADD_SPEC); + expect(scan.occurrences.get("domains")).toEqual(["--"]); + expect(scan.consumedFlagNames.size).toBe(0); + expect(scan.positionals).toEqual(["x"]); + }); }); }); diff --git a/apps/cli/tests/helpers/legacy-mocks.ts b/apps/cli/tests/helpers/legacy-mocks.ts index c3bbbfa1e3..1242bd1d44 100644 --- a/apps/cli/tests/helpers/legacy-mocks.ts +++ b/apps/cli/tests/helpers/legacy-mocks.ts @@ -317,14 +317,25 @@ export function mockLegacyLinkedProjectCacheTracked(): { readonly layer: Layer.Layer; readonly cached: boolean; readonly cachedRef: string | undefined; + readonly cachedApiUrl: string | undefined; + readonly cachedAccessToken: Option.Option> | undefined; } { let cached = false; let cachedRef: string | undefined; + let cachedApiUrl: string | undefined; + let cachedAccessToken: Option.Option> | undefined; const layer = Layer.succeed(LegacyLinkedProjectCache, { - cache: (ref: string) => + cache: ( + ref: string, + _workdir?: string, + apiUrl?: string, + accessToken?: Option.Option>, + ) => Effect.sync(() => { cached = true; cachedRef = ref; + cachedApiUrl = apiUrl; + cachedAccessToken = accessToken; }), }); return { @@ -335,6 +346,12 @@ export function mockLegacyLinkedProjectCacheTracked(): { get cachedRef() { return cachedRef; }, + get cachedApiUrl() { + return cachedApiUrl; + }, + get cachedAccessToken() { + return cachedAccessToken; + }, }; }