fix(cli): port gen bearer-jwt to native TypeScript (CLI-1961) - #6064
Merged
Coly010 merged 13 commits intoAug 7, 2026
Merged
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.
Contributor
Author
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Contributor
Supabase CLI previewnpx --yes https://pkg.pr.new/supabase/cli/supabase@6711642d7527d9e74bc3e4db65a115a44d00123fPreview 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.
Contributor
Author
|
@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.
Contributor
Author
|
@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.
Contributor
Author
|
@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".
…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.
Contributor
Author
|
@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".
…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".
…1-port-gen-bearer-jwt-to-native-typescript # Conflicts: # apps/cli/docs/go-cli-porting-status.md
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6711642d75
ℹ️ 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".
jgoux
approved these changes
Aug 7, 2026
Coly010
deleted the
columferry/cli-1961-port-gen-bearer-jwt-to-native-typescript
branch
August 7, 2026 14:58
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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