fix(cli): secrets set tolerates a malformed config.toml (Go parity)#5796
Conversation
## Current Behavior Go's `secrets set` (`internal/secrets/set/set.go:20-24`) swallows a malformed `supabase/config.toml` parse error to the debug logger and proceeds with empty `EdgeRuntime.Secrets` — env-file and positional-arg secrets still apply. `secrets set` has no `--linked`/`--local`/`--db-url` flag, so the root PreRun never loads the config first either; this is the only load. The TS port instead let the parse error propagate as a fatal `LegacySecretsConfigParseError`, aborting the whole command even when the user only wanted to set a secret via CLI arg or env-file. ## Expected Behavior - Catches `ProjectConfigParseError`, logs it to the debug logger, and continues with no config-declared secrets, matching Go. - The logged message is truncated before any blank-line-separated source snippet: `smol-toml`'s `TomlError` (and some schema-decode errors) embed a codeblock of the surrounding file lines in their message, which for this file's `[edge_runtime.secrets]` section can include literal secret values. Go's equivalent log line (`DecodeError.Error()`) is a short, content-free message — its verbose `.String()` with the snippet is never called by `set.go`. Truncating avoids echoing a user's own secret value into `--debug` output next to an unrelated syntax error. - Deletes the now-unused `LegacySecretsConfigParseError` (no remaining references). - Threads `LegacyDebugLogger` into the shared `legacyManagementApiRuntimeLayer` (used by many legacy commands) since the layer was already building the same instance internally for `cliConfig`/`httpClient`/`credentials` but never exposed it at the top level — the file's own doc comment describes this as the sanctioned way to add a new commonly-needed top-level service. Fixes CLI-1867
|
@codex review |
![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: 923815ff72
ℹ️ 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".
…ors (review: #5796) Go's viper+mapstructure decode (pkg/config/config.go:749) mutates the target struct field-by-field, so a type error on an unrelated key (e.g. analytics.port) doesn't stop EdgeRuntime.Secrets from landing on utils.Config — confirmed empirically against pkg/config. Effect Schema.decodeUnknownSync is all-or-nothing, so `secrets set` was silently discarding a valid [edge_runtime.secrets] section whenever any other field in config.toml failed schema decode. ProjectConfigParseError now carries the pre-decode document when the failure is a schema-decode error (not a raw parse error) so secrets set can re-decode just the edge_runtime subtree against the full schema and recover its secrets, without loosening decode semantics for other @supabase/config callers. A genuine parse failure still has no recoverable structure in either implementation.
|
@codex review |
1 similar comment
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 19dc72c044
ℹ️ 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".
… (review: #5796) Re-decoding the whole edge_runtime subtree still failed recovery when the schema error came from a sibling field in the same table (e.g. edge_runtime.inspector_port), discarding a valid [edge_runtime.secrets] block. Verified against pkg/config directly that Go's mapstructure decode tolerates this case and still populates EdgeRuntime.Secrets, so recoverEdgeRuntimeConfig now re-slices just edge_runtime.secrets instead of the whole edge_runtime table.
The previous commit's cast (`edgeRuntime as Record<string, unknown>`) violates this repo's no-`as`-casts policy; a local `isRecord` type predicate narrows `unknown` without it. Also fixes an oxfmt formatting drift in SIDE_EFFECTS.md left over from an earlier commit in this PR.
|
@codex review |
|
Codex Review: Didn't find any major issues. Hooray! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
Supabase CLI previewnpx --yes https://pkg.pr.new/supabase/cli/supabase@3153365d1d092a739b2d68b1a00fc9b76aaee6cbPreview package for commit |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a2b843d298
ℹ️ 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".
…only (review: #5796) Several callers of loadProjectConfig (gen types, next start, functions dev/serve/deploy) don't catch ProjectConfigParseError, so the attached document propagates uncaught. Only secrets set's recoverEdgeRuntimeConfig reads it, and only edge_runtime.secrets, so attaching the whole post-interpolation document (including literal edge_runtime.secrets values) is unnecessary blast radius for a TS-only telemetry surface with no Go equivalent to weigh against.
…5796) Regression test for a mixed valid/invalid edge_runtime.secrets map (GOOD = "ok" alongside BAD = 123). recoverEdgeRuntimeConfig's per-entry filtering (landed alongside the ProjectConfigParseError.document narrowing) now keeps GOOD instead of discarding the whole map, matching Go's mapstructure map decode which decodes each entry independently.
…review: #5796) loadProjectConfig resolves env(VAR) references against .env/.env.local before schema decode, so a malformed dotenv line fails with ProjectEnvParseError rather than ProjectConfigParseError, escaping the existing catchTag and aborting the command. Go's Load() (pkg/config/config.go:788-791) calls loadNestedEnv first too and swallows any resulting error the same way internal/secrets/set/set.go swallows a bad config.toml, so secrets set must stay non-fatal here as well and keep working from env-file/positional args.
jgoux
left a comment
jgoux
left a comment
There was a problem hiding this comment.
I found three remaining issues in the current head. The first two are Go-parity behavior gaps in secrets set; the third is a sensitive-data hardening issue in the config error shape.
…ocument (review: #5796) Several loadProjectConfig callers (gen types, next start, functions dev/serve/deploy) don't catch ProjectConfigParseError, so its document field can propagate uncaught. Wrap edge_runtime.secrets values in Redacted before attaching, matching the x-secret treatment secret() already gets elsewhere in this package, so an uncaught error can't carry a resolved secret in plaintext. secrets set's recoverEdgeRuntimeConfig/filterDecodableSecrets unwrap via Redacted.value before re-decoding.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4f58020207
ℹ️ 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".
…s (review: #5796) isRecord() treated arrays as records, so `[edge_runtime] secrets = [...]` fell through filterDecodableSecrets and Object.entries turned array indices into fabricated secret names (e.g. "0"). Go's mapstructure never sets WeaklyTypedInput, so a slice source for a map-typed field hits UnconvertibleTypeError and the whole field is left empty instead.
…eview: #5796) loadProjectConfig() omitted { projectRef: ref }, so a schema-decode recovery on `secrets set --project-ref <ref>` used the base [edge_runtime.secrets] instead of a matching [remotes.*] override. Go seeds Config.ProjectId before Load() and merges the override in loadFromFile before mapstructure ever runs, so the recovery must load against the same resolved ref. Also swallow DuplicateRemoteProjectIdError non-fatally, matching Go's LoadConfig catch-all.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5ff3823e87
ℹ️ 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".
…5796) Codex flagged that redactEdgeRuntimeSecrets only wrapped string secret values in Redacted before attaching them to ProjectConfigParseError.document, so a malformed entry with a non-string shape (e.g. a TOML array or inline table) still carried the secret in plaintext. Redacted.make accepts any value and always stringifies to <redacted:...>, so wrap every entry unconditionally instead of gating on typeof.
…ew: #5796) Go's DecryptSecretHookFunc (pkg/config/secret.go:98) never hashes an empty secret value, and ListSecrets (internal/secrets/set/set.go:48-52) only includes entries with a non-empty SHA256, so an empty config-declared secret is never sent and can't overwrite a same-named remote secret with an empty string. The TS merge loop only checked Redacted.isRedacted, which missed the empty-value case for both the happy-path config load and the malformed-config recovery path (both flow through the same loop). Require a non-empty value too so both of Go's zero-hash cases match.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: de9c5ffcf9
ℹ️ 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".
…rrors (review: #5796) `redactEdgeRuntimeSecrets` only wrapped per-entry values inside an `edge_runtime.secrets` table; a malformed `secrets` field that is itself non-object (e.g. `secrets = ["actual-secret"]`) fell through its early return and reached `ProjectConfigParseError.document` unredacted. Wrap the whole field in `Redacted` in that case too, and unwrap it in `secrets set`'s `recoverEdgeRuntimeConfig` before the `isRecord` check so the wrapper object itself isn't misread as a one-entry secrets map.
…rrors (review: #5796) `secrets set --debug` stringified the raw schema-decode error when a malformed `[edge_runtime.secrets]` entry failed decode, which embeds the rejected value inline (e.g. `Expected string, actual ["actual-secret"]`) with no blank-line separator for the existing truncation to catch. Go's pinned mapstructure decode-error types never include the rejected value, only type names, so use a fixed message for schema-decode failures and keep the truncation for genuine TOML syntax errors, where it already matches Go.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 75e09e27c2
ℹ️ 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".
Go's loadFromFile prints "Loading config override: [remotes.<name>]" to stderr unconditionally when a [remotes.*] block matches the resolved ref (pkg/config/config.go:605), before mapstructure decode ever runs. The ref-aware loadProjectConfig call added to secrets set mapped the loaded result straight down to .config, discarding appliedRemote and silently dropping the notice for remote-specific configs, unlike config push and the storage/seed handlers which already surface it.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7cbf5f303d
ℹ️ 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".
…crets set (review: #5796) Go's loadFromFile prints "Loading config override: [remotes.<name>]" to stderr before mapstructure decode runs, so the notice still fires even when the subsequent decode fails and secrets set recovers non-fatally. ProjectConfigParseError now carries appliedRemote through the failed decode so the catch path can surface it, matching the success path.
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Coly010
deleted the
columferry/cli-1867-secrets-set-tolerate-malformed-configtoml-instead-of
branch
2 participants
Current Behavior
Go's
secrets set(internal/secrets/set/set.go:20-24) swallows a malformedsupabase/config.tomlparse error to the debug logger and proceeds with emptyEdgeRuntime.Secrets— env-file and positional-arg secrets still apply.secrets sethas no--linked/--local/--db-urlflag, so the root PreRun never loads the config first either; this is the only load. The TS port instead let the parse error propagate as a fatalLegacySecretsConfigParseError, aborting the whole command even when the user only wanted to set a secret via CLI arg or env-file.Expected Behavior
ProjectConfigParseError, logs it to the debug logger, and continues with no config-declared secrets, matching Go.smol-toml'sTomlError(and some schema-decode errors) embed a codeblock of the surrounding file lines in their message, which for this file's[edge_runtime.secrets]section can include literal secret values. Go's equivalent log line (DecodeError.Error()) is a short, content-free message — its verbose.String()with the snippet is never called byset.go. Truncating avoids echoing a user's own secret value into--debugoutput next to an unrelated syntax error.LegacySecretsConfigParseError(no remaining references).LegacyDebugLoggerinto the sharedlegacyManagementApiRuntimeLayer(used by many legacy commands) since the layer was already building the same instance internally forcliConfig/httpClient/credentialsbut never exposed it at the top level — the file's own doc comment describes this as the sanctioned way to add a new commonly-needed top-level service.Fixes CLI-1867