fix(cli): port gen bearer-jwt to native TypeScript (CLI-1961) - #6064
fix(cli): port gen bearer-jwt to native TypeScript (CLI-1961)#6064Coly010 wants to merge 12 commits into
Conversation
Replaces the Go-proxy wrapper with a fully local, native handler that resolves a signing JWK (built-in default key, or [auth].signing_keys_path via stdin/kid prompt or an interactive TTY picker) and signs a Go-jwt.MapClaims-shaped bearer token, byte-matching Go's claim computation, key-selection prompts, and asymmetric-signing error family. Hoists the shared [auth].signing_keys_path config/file loading out of gen signing-key into gen.signing-keys-config.ts, and refactors legacy-go-jwt.ts's asymmetric signer into a generic legacySignJwtWithJwk so both gen signing-key's fixed anon/service_role claims and bearer-jwt's arbitrary MapClaims-shaped claims share one Go-parity validation/signing path. Along the way, corrects two pre-existing legacy-go-jwt.unit.test.ts expectations that encoded a kty-vs-algorithm cross-check Go does not actually perform (verified against the real Go binary).
…1961) Fixes several Go-parity and security divergences flagged by independent review of the native TypeScript port: --sub "" must still set is_anonymous, a stdin JWK of null must be rejected (not fall back to the default signing key), alg is validated against Go's RS256/ES256 allowlist at JWK-decode time (both the stdin and signing-keys-file paths), --exp now rejects invalid calendar dates (e.g. Feb 30) the way Go's time.Parse does, and the TTY key picker no longer crashes on a zero-key signing_keys_path file. Also fixes a sub-second --valid-for truncation-order bug, adds --exp whitespace trimming, tightens the malformed-JSON --payload error text without repeated JSON.parse retries, renames StoredSigningKeyJwk to LegacyStoredSigningKeyJwk for the mandatory legacy/ export prefix, and hoists the generic Go-JSON kind-name helper to legacy-go-json.ts.
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Supabase CLI previewnpx --yes https://pkg.pr.new/supabase/cli/supabase@f676173df73ce0023c9e855c20c48cd950bad7c2Preview package for commit |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a7f45f813f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
… gen bearer-jwt --role (ci) Go marks --role required (cmd/gen.go:175) but cobra's ValidateRequiredFlags runs AFTER PersistentPreRunE (cobra@v1.10.2/command.go:985,1007) — which is where Go's telemetry service is constructed and later flushed to telemetry.json. The native port instead rejected a missing --role during Effect CLI's own argument parsing, before the handler (and its telemetry wrapper) ever ran, and normalize-error.ts's MissingOption case renders the message with an "Error: " prefix that cobra's real (SilenceErrors: true) output never has. Verified against the real binary via the e2e parity harness: Go writes telemetry.json and a bare "required flag(s) \"role\" not set" line on this exact failure; the TS port wrote neither. Makes --role optional at parse time and enforces it in the handler instead (same established pattern as vanity-subdomains activate's --desired-subdomain), so the failure now flushes telemetry and matches Go's stderr byte-for-byte. Also carries this command's own integration-test additions for the review fixes landing in the following commits of this push (signing-key ancestor/ config.json resolution, the TTY picker's stderr routing).
…er config.json (review)
Go's Config.Load("") (pkg/config/utils.go:43-48) resolves ONLY
<workdir>/supabase/config.toml — no ancestor climb once cliConfig.workdir is
already resolved (an explicit --workdir pointing at a subdirectory below
another project's root gets no climb either: ChangeWorkDir only calls
getProjectRoot's climb when --workdir/SUPABASE_WORKDIR is unset,
internal/utils/misc.go:246-249) and no JSON project-config fallback.
legacyResolveSigningKeysConfigPaths (shared by gen bearer-jwt and gen
signing-key) called loadProjectConfig without { tomlOnly: true, search:
false } — the exact pair legacy-local-project-context.ts already
establishes for this same Config.Load-parity reason.
Verified against the real binary: from a workdir with no config.toml of its
own but an ancestor with a configured signing_keys_path, Go falls back to
the unconfigured-default branch (prompts for a raw JWK) while the TS port
picked up the ancestor's signing_keys_path and prompted for a kid instead.
Fixes the shared helper's loadProjectConfig call and updates gen
signing-key's own sibling integration test, which was asserting the old
JSON-preferring (non-Go-parity) display path for a stray config.json with
no config.toml present.
Codex review finding (chatgpt-codex-connector), CLI-1961.
…derr (review) Go's own interactive picker always writes to stderr (internal/utils/prompt.go's PromptChoice: tea.WithOutput(os.Stderr), "Interactive prompts should always be written to stderr") — but clack's select()/log.info() default to process.stdout (verified directly against the installed @clack/prompts source: no output override at either call site). gen bearer-jwt's own stdout IS the signed-token payload even in text mode, so an interactive user with a configured signing_keys_path would get the picker UI and the "Selected key ID: ..." line mixed into their stdout-captured token. Verified empirically: a minimal probe against the real textOutputLayer showed output.info's bytes land on stdout, not stderr, contradicting a stale comment elsewhere in this codebase that assumed otherwise. Adds an opt-in `stream?: "stdout" | "stderr"` to Output.promptSelect's behavior (defaults to "stdout", so every other of this command's ~10 existing callers is unaffected) and uses it — plus output.raw(..., "stderr") instead of output.info for the "Selected key ID:" line — in bearer-jwt's own signing-key resolver. Codex review finding (chatgpt-codex-connector), CLI-1961.
…ull claims (review) Go's time.Parse(time.RFC3339, ...) rejects an out-of-range zone offset (e.g. --exp 2030-01-01T00:00:00+99:99: "time zone offset minute out of range" — verified directly against the Go standard library), but legacyParseBearerJwtExp's calendar check never looked at the offset at all: the regexp matched, isValidRfc3339Calendar passed (it only checks the LOCAL time components), and Date.parse silently returned NaN. NaN then flowed into exp/iat, JSON.stringify(NaN) === "null", and the command signed a token with null exp/iat claims instead of failing — a security-relevant divergence, since it defeats expiration entirely rather than erroring loudly. Go's own range check (time/format.go:1267-1278) is intentionally lenient in one direction: "> rather than >=, as some people do write offsets of 24 hours or 60 minutes" — so +24:00 parses successfully in Go while +99:99 does not. A naive `if (!Number.isFinite(Date.parse(...))) throw` fallback would get this backwards (Date.parse itself rejects +24:00, which Go accepts), so this reimplements Go's t.addSec(-zoneOffset) arithmetic directly: the local wall-clock components (already validated), interpreted as UTC, minus the signed offset in seconds — byte-verified against the real binary for both the accept and reject boundaries. Codex review finding (chatgpt-codex-connector), CLI-1961.
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 38ecf1dd7a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…lds, and unblock the bearer-jwt TTY picker under json output (review)
Four confirmed Codex review findings on gen bearer-jwt, each verified against
the real apps/cli-go binary/stdlib:
- bearer-jwt.flags.ts / bearer-jwt.claims.ts: fractional --exp seconds were
dropped during parsing before the iat = exp - validFor arithmetic ran. Go's
time.Parse preserves fractional seconds even for RFC3339 (a documented
parse-only extension) and only truncates the final exp/iat via
jwt.NewNumericDate. Preserve the fraction through legacyParseBearerJwtExp
and floor exp/iat only at the end in legacyBuildBearerJwtClaims.
- bearer-jwt.claims.ts: an out-of-range JSON number in --payload (e.g.
{"extra":1e309}) was accepted by JSON.parse as Infinity and later
serialized as null, silently changing a custom claim. Go's json.Unmarshal
into jwt.MapClaims rejects it outright. Scan for the first non-finite
number literal once the payload's top-level shape is confirmed to be an
object (matching Go's exact priority over the existing array/scalar
type-mismatch check).
- bearer-jwt.signing-key.ts: malformed optional JWK fields (key_ops with a
non-string element, ext as a non-bool, etc.) were silently dropped instead
of failing the way Go's json.Unmarshal into config.JWK does. Generalized
to every field this normalizer reads, wrapped with each call site's own
Go-matching error prefix (Branch A's "failed to parse JWK: %w", the
signing_keys_path file's "failed to decode signing keys: failed to parse
response body: %w").
- bearer-jwt.signing-key.ts: the TTY key picker aborted with
NonInteractiveError under --output-format=json/stream-json, even though Go
has no such flag and always prompts on a real TTY, and this command's
stdout is the raw token unconditionally in every format. Try the ambient
Output first (preserving every existing mock-driven test), and only retry
through a real textOutputLayer instance on NonInteractiveError.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dfa0595b05
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
… and array JWK entries (review)
Five Codex review findings against Go parity, each verified empirically
against the Go standard library / real binary:
- bearer-jwt.flags.ts / bearer-jwt.claims.ts: `legacyParseBearerJwtExp`
collapsed an epoch-scale whole-second count and a nanosecond fraction
into a single float, which silently rounds a near-second fraction (e.g.
`.999999999`) UP into the next second — a full second later than Go's
`jwt.NewNumericDate`, which truncates DOWN. Now returns an exact
`{ wholeSeconds, nanos }` pair, combined via the new
`legacyAddSecondsAndFloor` using integer nanosecond arithmetic.
- bearer-jwt.claims.ts / bearer-jwt.handler.ts: when `--exp` is omitted,
`exp` was computed from an already-floored `now`, shortening the token's
lifetime by up to a second whenever `--valid-for` has a sub-second
component. `nowInstant` is now built unfloored from `Date.now()` and
combined the same exact way.
- bearer-jwt.flags.ts: the `--exp` regexp only accepted `.` before
fractional digits; Go's `time.Parse(time.RFC3339, ...)` also accepts `,`.
- bearer-jwt.flags.ts: `Date.UTC`/`new Date(...)` apply JS's legacy
two-digit-year remapping to any year in `[0, 99]` (`0001` silently became
`1901`), even though Go parses that year literally. Builds the instant via
`setUTCFullYear`/`setUTCHours` instead, which has no such special case.
- gen.signing-keys-config.ts: `isRecord`'s `typeof value === "object"` check
also matched JSON arrays, so a `signing_keys_path` entry like `[]` passed
as a "record" and a later valid key could still be selected — Go's decode
straight into `[]config.JWK` rejects any array-shaped element outright.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f0457f5752
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ); | ||
| } | ||
| for (const item of decoded) { | ||
| if (!isRecord(item)) { |
There was a problem hiding this comment.
When a configured signing_keys_path contains a null entry before otherwise valid keys, for example [null, {"kid":"k2", ...valid key...}], Go's json.Unmarshal into []config.JWK treats the null as a zero-value JWK and still lets a non-TTY user select k2. This shape check rejects the whole file as "expected a JSON array of objects", so the native command aborts even though the Go CLI can sign with the later key; allow null entries to normalize to the same zero-value JWK instead of failing the decode.
AGENTS.md reference: apps/cli/AGENTS.md:L247-L257
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@/private/tmp/claude-501/-Users-colum-supabase-cli-wt/097f7fc1-7107-4a36-b258-967a2bfeef74/scratchpad/replies/reply2.md
There was a problem hiding this comment.
Rejected — verified empirically against the real apps/cli-go binary that this doesn't happen.
With signing_keys_path decoding to [null, {valid k2 key}] and auth.enabled = true, Go's Config.Validate runs generateAPIKeys() (the anon/service_role auto-generation) before gen bearer-jwt's own getSigningKey ever runs — and generateAPIKeys always signs with SigningKeys[0], which is the null-decoded zero-value JWK. That fails the whole command at config-load time, before the kid-selection prompt is ever reached:
$ echo 'k2' | ./supabase gen bearer-jwt --role anon
failed to convert JWK to private key: unsupported key type:
exit 1
So there's no code path where a non-TTY user reaches, let alone selects, k2 — the finding's premise doesn't hold. (Side note for completeness: the current TS shape-check does fail with different text than Go's real error — "expected a JSON array of objects" vs Go's "failed to convert JWK to private key: unsupported key type: " — but both paths still reject the command with exit 1, so there's no user-observable "TS rejects where Go accepts" regression here. Reproducing Go's exact text for this case would mean porting generateAPIKeys's SigningKeys[0] auto-key-generation into the gen family's config loader, which is explicitly out of scope for legacy-config-validate.ts's shared Config.Validate port today — happy to open a follow-up if that precision is wanted, but not folding it into this thread's fix.)
Leaving this open rather than resolving, per the rejected-thread convention.
…meric claim order, and gate signing-key read on auth.enabled (review) - legacy-go-jwt.ts: pre-validate EC/RSA JWK numeric fields as raw unpadded base64url before importing, matching Go's base64.RawURLEncoding rejection of padding (verified against the real binary). - legacy-go-jwt.ts: serialize the JWT header with encodeGoJsonCompact instead of JSON.stringify, matching Go's HTML-escaping json.Marshal. - legacy-go-json.ts / legacy-go-output.encoders.ts: sortKeysDeep now builds a Map instead of a plain object, since plain objects silently reorder integer-like string keys into numeric order on enumeration, undoing Go's lexicographic jwt.MapClaims key sort for numeric-looking claim names. - signing-key.handler.ts: gate the signing-keys file read on auth.enabled, matching gen bearer-jwt's existing gate and Go's real (if surprising) behavior verified against the binary: with auth disabled, Go never reads the configured file at all, so --append clobbers real file content with the default key instead of failing on a malformed file. - legacy-go-duration.ts: accept the Greek-mu (U+03BC) microsecond spelling alongside us/µs, matching Go's time.ParseDuration unitMap. One Codex review finding (null entries in a signing_keys_path array) was rejected: verified against the real binary that Go's config generateAPIKeys step fails outright on a null-first-entry array during config load, before key selection is ever reached — the claimed "still lets a non-tty user select k2" behavior does not occur.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7b1803a568
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…path env() from Go's full dotenv cascade (review) - assertNoMalformedDuplicateJwkField (gen.signing-keys-config.ts) re-validates a JWK object's raw source text for a known field repeated with an earlier, Go-rejecting occurrence that JSON.parse alone would silently discard (kept only the last, valid one) before any type check ever ran — closing the gap for both the pasted-JWK Branch A path and signing_keys_path file entries. Verified against the real binary that Go's encoding/json still errors on the first bad occurrence even when a later duplicate is individually valid, and that config.Algorithm's TextUnmarshaler-backed allowlist check behaves differently (an earlier failing occurrence stops Go from ever attempting a later one) than a plain scalar field's type check (which Go still attempts). - legacyResolveSigningKeysConfigPaths now resolves a Go-accurate ProjectEnvironment (matching legacyLoadLocalProjectContext's established stop/status pattern) before loading config.toml, so signing_keys_path = "env(KEYS_PATH)" resolves against Go's full loadNestedEnv cascade (.env.<SUPABASE_ENV>[.local] plus the project-root directory), not just @supabase/config's narrower default supabase/.env(.local) load. Fixes CLI-1961 Codex review findings on bearer-jwt.signing-key.ts:180 and gen.signing-keys-config.ts:74.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d7365fffac
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…tive zero, and MinInt64 durations (review) - gen.signing-keys-config.ts: a null signing_keys_path array element now normalizes to a zero-value JWK regardless of position, matching Go's json.Unmarshal; only rejecting the whole file broke Go-valid files where the first key is valid (generateAPIKeys succeeds, non-TTY kid match still resolves). - gen.signing-keys-config.ts: legacyReadSigningKeysFile now parses only the first JSON value in signing_keys_path, matching fetcher.ParseJSON's single json.Decoder.Decode call, which ignores trailing bytes instead of requiring the whole file to be exactly one JSON value. - gen.signing-keys-config.ts: a null key_ops element now decodes to "" (Go's []string zero value) instead of a type-mismatch error; GenerateAsymmetricJWT never reads key_ops so this never affected signing in Go either way. - legacy-go-json.ts: the shared JSON walker now preserves negative zero's sign (encoding/json marshals float64 -0 as "-0"; JSON.stringify(-0) prints "0"), fixing a payload/signature divergence for gen bearer-jwt --payload. - legacy-go-output.encoders.ts: sortKeysDeep now sorts object keys with Go's byte/code-point order (via legacy-go-struct-output.encoders.ts's goStringCompare, now exported) instead of JS's UTF-16 code-unit order, which diverges once an astral character meets a high-BMP one. - legacy-go-duration.ts: legacyParseGoDuration now mirrors time.ParseDuration's exact two-tier overflow bound (uint64 accumulator allowed up to 1<<63, stricter 1<<63-1 ceiling only for non-negative results), so the real math.MinInt64 duration parses instead of being rejected one bound early. Verified each against the real apps/cli-go binary/Go stdlib (scratch probes and, for the signing-keys-path cases, the compiled binary directly).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0fab9271f6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| const payloadJson = legacyEncodeBearerJwtClaims(claims); | ||
| const token = yield* Effect.try({ | ||
| try: () => legacySignJwtWithJwk(jwk, payloadJson), |
There was a problem hiding this comment.
When the selected EC JWK has valid base64url x/y coordinates that do not correspond to its private d value, this new native signing path can still mint a token because Node accepts that JWK for signing. The Go CLI reaches crypto/ecdsa through GenerateAsymmetricJWT and rejects the inconsistent public point before returning a token, so this can produce JWTs that fail against the JWKS public key instead of surfacing the bad key like the legacy binary. Validate or derive the EC public point before calling the signer.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Rejected — verified empirically against the real Go standard library and golang-jwt/jwt v5.3.1 that Go does not perform this validation either, so there's no parity gap here.
jwkToECDSAPrivateKey (apps/cli-go/pkg/config/apikeys.go:171-203) builds the key directly from the JWK's fields with zero cross-check:
return &ecdsa.PrivateKey{
PublicKey: ecdsa.PublicKey{Curve: elliptic.P256(), X: x, Y: y},
D: d,
}, nilSigning then goes through golang-jwt/jwt/v5's SigningMethodECDSA.Sign (ecdsa.go:94-124), which calls ecdsa.Sign(rand.Reader, ecdsaKey, hash) — that only touches ecdsaKey.D and the curve; X/Y are never read during signing. I reproduced the exact struct construction Go uses and confirmed this directly:
mismatched := &ecdsa.PrivateKey{
PublicKey: ecdsa.PublicKey{Curve: elliptic.P256(), X: priv2.X, Y: priv2.Y}, // wrong public point
D: priv1.D, // real private scalar
}
sig, err := ecdsa.SignASN1(rand.Reader, mismatched, hash[:])
// Go ACCEPTED mismatched x/y + d, signature len: 71
// Signature verifies against CLAIMED (mismatched) public x/y? false
// Signature verifies against TRUE public key (derived from D)? trueGo signs successfully and produces a token that verifies against the public point actually implied by D, not the JWK's claimed (mismatched) x/y — exactly the same behavior Node's crypto.createPrivateKey({format:"jwk"}) + createSign exhibits (verified the same way: Node also signs, and the signature only verifies against the true public point derived from d, not the claimed mismatched x/y). I also checked Go 1.26's newer FIPS-validated path (crypto/internal/fips140/ecdsa.NewPrivateKey) — it validates that Q (built from X/Y) is a valid point on the curve, but never checks Q == D·G.
So this isn't a TS-vs-Go divergence — it's a footgun Go's own gen bearer-jwt already has. Adding cross-validation here would make the native port stricter than the CLI it's supposed to match, which is the opposite of parity. Given that a genuinely mismatched d/x/y JWK is an extremely unusual thing for a real key file to contain (vs. this PR's earlier confirmed bugs, which were all reachable from ordinary, syntactically-normal input), and the behavior matches Go byte-for-byte, I'm not porting extra validation Go itself doesn't have.
…WK fields (review) - legacy-go-duration.ts: mirror Go's time.ParseDuration float64-mediated fraction conversion instead of an exact BigInt division, so a long fractional --valid-for (e.g. 18+ nines) rounds up to match Go instead of truncating down, which previously shifted the final floored iat/exp by a full second. - gen.signing-keys-config.ts / bearer-jwt.signing-key.ts: resolve JWK fields case-insensitively (resolveJwkFieldValue), matching encoding/json's case-insensitive struct-tag matching, for both readOptionalString/Array/ Boolean and the early alg-allowlist pre-checks; extend the existing duplicate-field detection to group case-variant spellings together too.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f676173df7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const configuredPath = loaded.config.auth.signing_keys_path; | ||
| if (configuredPath === undefined || configuredPath.length === 0) { |
There was a problem hiding this comment.
When supabase/config.toml omits signing_keys_path but SUPABASE_AUTH_SIGNING_KEYS_PATH=./keys.json is set, including from Go's dotenv cascade, Go's Viper AutomaticEnv populates Auth.SigningKeysPath before Config.Validate, so gen bearer-jwt and gen signing-key --append read/write that file. This lookup only uses the decoded TOML value and takes the absent branch, so the command falls back to stdin/default signing or prints a standalone key instead of using the env-configured key file; merge the direct SUPABASE_ override before deciding the path is absent.
AGENTS.md reference: apps/cli/AGENTS.md:L247-L257
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Rejected — re-verified from scratch per the request to not assume this is the same as the round-1 rejection just because it sounds similar, and it is genuinely the same underlying behavior, confirmed with a higher-fidelity probe this time.
Round 1 rejected the same claim (direct SUPABASE_AUTH_SIGNING_KEYS_PATH override when signing_keys_path is omitted from config.toml) using the compiled binary's prompt text as the signal. This time I went one level deeper and called the real pkg/config.Load/Config.Validate code path in-process (the exact code flags.LoadConfig → bearerjwt.Run/signingkeys.Run invoke) and inspected the resulting struct field directly, so there's no reliance on prompt-text interpretation:
cfg := config.NewConfig(config.WithHostname("127.0.0.1"))
err := cfg.Load("", &rootFS{fsys: afero.NewOsFs()}) // cwd = project with
// [auth] enabled=true, site_url set,
// NO signing_keys_pathWith SUPABASE_AUTH_SIGNING_KEYS_PATH=/abs/path/to/keys.json (a valid, existing JWK array file) set and signing_keys_path omitted from config.toml:
Load err: <nil>
Auth.SigningKeysPath = ""
Auth.SigningKeys = [{... the built-in default key, kid: b81269f1-... ...}]
Auth.SigningKeysPath stays empty and Auth.SigningKeys is populated with Go's own generated default key — the env var is not merged in. As a positive control (to rule out the probe itself silently ignoring env vars), the same harness against SUPABASE_DB_PORT=19999 — a field that is present in config.toml (db.port = 54322) — correctly returns Db.Port = 19999. So env overrides do work through this pipeline for keys already present somewhere in viper's data; auth.signing_keys_path specifically is not one of them when it's absent from both the user's config.toml and Go's own default template (where it's commented out: pkg/config/templates/config.toml:169).
I did trace a plausible mechanism for why the finding's premise seemed reasonable — loadFromFile's viper instance uses viper.ExperimentalBindStruct() + AutomaticEnv(), and ExperimentalBindStruct does add every destination-struct field path (not just already-set keys) to the key list Unmarshal/UnmarshalExact resolves via Get(), and Get()'s precedence checks env before the config file tier. On paper that reads like it should surface the env var for an otherwise-untouched field. The probe above shows that isn't what happens in practice; I won't guess at the exact reason decodeStructKeys doesn't add this particular key (or why Get still misses it) without further evidence, but the observed behavior itself is unambiguous with a real, valid key file behind the env var and a real config-load run: gen bearer-jwt/gen signing-key --append in Go do not pick it up when signing_keys_path is omitted, so legacyResolveSigningKeysConfigPaths reading only the decoded TOML value is correct as-is.
Leaving this open rather than resolving, per the rejected-thread convention.
| const loaded = yield* loadProjectConfig(cwd, { | ||
| projectEnv: projectEnv !== null ? { ...projectEnv, values: projectEnvValues } : undefined, | ||
| goViperCompat: true, |
There was a problem hiding this comment.
When the project config is syntactically valid but fails Go's Config.Validate, such as [db] major_version = 16 or auth.site_url = "", Go's flags.LoadConfig aborts before any signing-key prompt, JWT signing, or key-file overwrite. This resolver stops at loadProjectConfig, whose schema accepts many values that Go later rejects, so gen bearer-jwt can mint a token and gen signing-key can rewrite key files in projects the Go CLI would fail fast; route this load through the existing Go-validation path before returning signing-key paths.
AGENTS.md reference: apps/cli/AGENTS.md:L241-L243
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Escalating again, consistent with the round-3 reasoning on this exact gap (raised there for bearer-jwt.handler.ts:73, "Run Go config validation before signing") — not fixing it differently here, since this is the same underlying gap surfacing from a second call site in the same command family, not a new one.
Recap of that reasoning, re-checked against the current tree: legacyValidateResolvedConfig (legacy-config-validate.ts) still has exactly two call sites — the db/migration loader (legacy-db-config.toml-read.ts:1711) and the status/stop resolver (legacy-local-config-values.ts:2877) — and LegacyConfigValidationInput still requires db, storageBucketNames, functionSlugs, edgeRuntimeDenoVersion, analytics, and experimental unconditionally. legacyResolveSigningKeysConfigPaths (this file) only ever resolves auth.signing_keys_path/auth.enabled, so wiring it into the shared validator today would mean either inventing a partial/divergent LegacyConfigValidationInput for the gen family, or resolving a pile of unrelated config (storage buckets, functions, analytics, experimental) that gen bearer-jwt/gen signing-key have no other reason to load — exactly the divergence-from-the-"one home"-pattern problem round 3 flagged. Nothing about that architecture has changed since: same two call sites, same required fields, no partial-validator variant has been introduced anywhere else in the tree to point to instead.
Worth noting apps/cli/CLAUDE.md now has a dedicated "Config.Validate parity has one home" section formalizing exactly this decision ("do not add per-command reimplementations of these checks... When a Go validation branch or message changes, change it there.") — so this isn't just a one-off judgment call being re-litigated per PR, it's the documented policy this file's own gap should eventually be closed against, via a dedicated cross-command follow-up (the same "commands that call loadProjectConfig but skip full Config.Validate" family covering gen bearer-jwt, gen signing-key, config push, secrets set, and functions new), not a fix folded into this PR.
Leaving this open rather than resolving, per the escalated-thread convention.
1 participant
What changed
Ports
gen bearer-jwt(a Phase-0 Go-proxy) to native TypeScript. Go's implementation (apps/cli-go/internal/gen/bearerjwt/bearerjwt.go,cmd/gen.go) is fully local — no Docker, no network: load config, resolve a signing key from[auth].signing_keys(with interactive JWK/kid selection prompts), build claims, sign.Key parity detail: Go's real claims object is a
jwt.MapClaims(a Go map), so JSON serializes keys alphabetically, not insertion order — unlike the pre-existinglegacyGenerateAsymmetricGoJwthelper (struct-shaped, insertion-order). This required a dedicated map-shaped claims encoder rather than reusing the existing struct-shaped signer as-is; both are now documented and kept deliberately distinct to avoid a future caller mixing them up.Also fixes a validation-order bug in the pre-existing shared
legacy-go-jwt.ts(extracting a newlegacySignJwtWithJwk): Go checks key-type/curve first (wrapped infailed to convert JWK to private key: ...), then algorithm (unwrapped), and has no explicit cross-check between kty and algorithm — a mismatch is only caught when the underlying JWT library itself fails to sign (failed to sign JWT: key is of invalid type: ...). Two pre-existing unit tests that asserted the wrong (Go-divergent) behavior are corrected as part of this fix. This is a genuine prerequisite for the port (both commands share this signing path), not new-code-only — flagging explicitly since the commit doesn't otherwise signal that shipped error text for the pre-existinggen signing-key-adjacent path changed.Hoisted
apps/cli/src/legacy/commands/gen/gen.signing-keys-config.ts, shared betweengen bearer-jwtand the pre-existinggen signing-keycommand (both in the samegenfamily, per this repo's hoist-to-family-root rule).Why
Part of the M9 "Go removal" milestone.
Review notes
gen bearer-jwtmints signed JWTs, so this got an unusually thorough pass: the go-parity-auditor and engineer-reviewer both built and executed the real Go binary with probe inputs to verify claims empirically rather than reading source alone. That surfaced (and this PR's follow-up commit fixes) several real correctness/security gaps found only by execution:nullwas silently falling back to a default, non-secret signing key where Go actually refuses.algwasn't validated against Go'sRS256/ES256allowlist at decode time, letting anHS256key reach the signing step instead of being rejected earlier, matching Go.--sub ""(explicitly empty, as opposed to omitted) was incorrectly suppressingis_anonymous— Go's own check treats an empty string the same as absent.--expaccepted invalid calendar dates (e.g. Feb 30) thatDate.parsesilently rolled over instead of rejecting, unlike Go'stime.Parse.TypeErrorinstead of Go'suser aborted.Also fixed: a missing
Legacy-prefix convention violation, a misplaced generic JSON-parity helper, sub-second--valid-fortruncation ordering (verified backwards vs. Go),--expwhitespace trimming (Go's pflag trims, this didn't), and a stale test assertion.One architectural suggestion — reusing
legacy-config-validate.ts's existing signing-keys helpers instead of the new hoisted module — is deliberately deferred: the go-parity-auditor's own code-executing pass did not find a live behavioral bug from the current shape, and this PR is already large; noting it here so it isn't lost.Fixes CLI-1961