feat(cli): validate sensitive fields require env() references#110
Closed
tonychang04 wants to merge 10 commits into
Closed
feat(cli): validate sensitive fields require env() references#110tonychang04 wants to merge 10 commits into
tonychang04 wants to merge 10 commits into
Conversation
WalkthroughAdds insforge.toml management to the CLI: parse/serialize TOML, validate schema and secrets, compute and format diffs, and register ChangesConfig File Management and CLI Commands
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
tonychang04
force-pushed
the
feat/insforge-toml-mvp
branch
from
d8d34a6 to
0fb69b3
Compare
tonychang04
force-pushed
the
feat/insforge-toml-secrets-validation
branch
from
44496e6 to
150ef30
Compare
Sensitive fields in insforge.toml (OAuth client_secret, SMTP password, S3 secret key, etc.) MUST be env(NAME) references — never literal values. Same convention as Vercel/Fly.io/Supabase, makes the file unconditionally safe to commit to git. parseEnvRef() extracts the secret name from a well-formed env(NAME) reference (NAME must be UPPER_SNAKE_CASE). validateSensitiveString() throws ConfigValidationError with an actionable message naming the exact 'insforge secrets add' command when a literal is pasted in place of an env() ref. Foundation laid but not yet wired — first consumer will be [email.smtp] or [auth.providers.<built_in>], whichever ships next. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
tonychang04
force-pushed
the
feat/insforge-toml-secrets-validation
branch
from
9d7329b to
8f05013
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/commands/config/apply.ts`:
- Around line 65-67: The call to applyChange(result.changes) passes an unused
file parameter; remove the unused parameter from applyChange's signature and all
its call sites (including the loop that does await applyChange(change, file))
or, if you intend to reserve it, rename the parameter to _file in the
applyChange function and the call sites to suppress linter warnings; update
applyChange's definition and every place that calls applyChange to match the new
parameter list (or underscore name) so the codebase compiles cleanly.
- Around line 42-46: The command currently prints two separate JSON payloads
when the --json flag is used (the JSON print of result at the
console.log(JSON.stringify(result...)) and a second JSON output later), which
breaks single-document parsers; update the apply command so that when the json
flag is true it emits exactly one JSON document containing the final output
(merge or replace whatever intermediate JSON payloads into the single canonical
result object) and skip any additional console.log(JSON.stringify(...)) or
formatPlan(...) calls that would print a second document; specifically change
the logic around the json check in apply.ts (the
console.log(JSON.stringify(result...)) and the later JSON print near the code
that currently calls formatPlan(result)) so only one JSON.stringify(result) is
executed and non-JSON formatted output (formatPlan) is used only when json is
false.
In `@src/commands/config/export.ts`:
- Around line 42-45: The current construction of const config: InsforgeConfig
always emits auth.allowed_redirect_urls as an empty array when
raw.auth?.allowedRedirectUrls is missing; change the config assembly so
allowed_redirect_urls is only set when raw.auth?.allowedRedirectUrls is defined
(i.e. do not synthesize an empty array). Update the config/auth object creation
in export.ts (the const config and auth block) to conditionally include
allowed_redirect_urls (using a conditional spread or guard on
raw.auth?.allowedRedirectUrls !== undefined) so the field is omitted when the
source metadata omits it.
In `@src/lib/config-schema.ts`:
- Around line 18-20: The constructor for ConfigValidationError currently formats
the message as `config.${path}: ${message}` which yields `config.: ...` when
path is empty; update the constructor in the ConfigValidationError class (the
constructor(public readonly path: string, message: string)) to conditionally
format the prefix: use `config: ${message}` when path is empty or whitespace,
otherwise use `config.<path>: ${message}` so the CLI output never shows a stray
dot for root paths.
In `@src/lib/config-toml.ts`:
- Around line 8-10: The catch block that throws new Error("TOML parse error:
...") is losing the original parse exception; update the throw to include the
caught error as the ES2022 error cause (e.g., throw new Error(`TOML parse error:
${(err as Error).message}`, { cause: err })); locate the catch in
src/lib/config-toml.ts where the TOML parse is handled and add the { cause: err
} option so the original exception is preserved.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2652fb45-1427-4173-a391-fc9f547ac23d
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (15)
package.jsonsrc/commands/config/apply.tssrc/commands/config/export.tssrc/commands/config/index.tssrc/commands/config/plan.tssrc/index.tssrc/lib/config-diff.test.tssrc/lib/config-diff.tssrc/lib/config-format.test.tssrc/lib/config-format.tssrc/lib/config-schema.tssrc/lib/config-secrets.test.tssrc/lib/config-secrets.tssrc/lib/config-toml.test.tssrc/lib/config-toml.ts
Comment on lines
+42
to
+46
| if (json) { | ||
| console.log(JSON.stringify(result, null, 2)); | ||
| } else { | ||
| console.log(formatPlan(result)); | ||
| } |
There was a problem hiding this comment.
--json currently emits two separate JSON documents.
At Line 43 and Line 70, the command prints two top-level JSON payloads in a single run when changes are applied. That breaks parsers expecting one JSON document per invocation.
Suggested fix
- if (json) {
- console.log(JSON.stringify(result, null, 2));
- } else {
+ if (!json) {
console.log(formatPlan(result));
}
@@
- if (json) {
- console.log(JSON.stringify({ applied: true, changes: result.changes }));
+ if (json) {
+ console.log(JSON.stringify({ plan: result, applied: true }, null, 2));
} else {Also applies to: 69-71
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/commands/config/apply.ts` around lines 42 - 46, The command currently
prints two separate JSON payloads when the --json flag is used (the JSON print
of result at the console.log(JSON.stringify(result...)) and a second JSON output
later), which breaks single-document parsers; update the apply command so that
when the json flag is true it emits exactly one JSON document containing the
final output (merge or replace whatever intermediate JSON payloads into the
single canonical result object) and skip any additional
console.log(JSON.stringify(...)) or formatPlan(...) calls that would print a
second document; specifically change the logic around the json check in apply.ts
(the console.log(JSON.stringify(result...)) and the later JSON print near the
code that currently calls formatPlan(result)) so only one JSON.stringify(result)
is executed and non-JSON formatted output (formatPlan) is used only when json is
false.
Comment on lines
+65
to
+67
| for (const change of result.changes) { | ||
| await applyChange(change, file); | ||
| } |
There was a problem hiding this comment.
Remove or underscore the unused file parameter.
Line 87 introduces an unused argument that already shows up in CI lint warnings. Either remove it from applyChange and its call site, or rename to _file if intentionally reserved.
Suggested fix
- for (const change of result.changes) {
- await applyChange(change, file);
- }
+ for (const change of result.changes) {
+ await applyChange(change);
+ }
@@
-async function applyChange(
- change: DiffChange,
- file: InsforgeConfig,
-): Promise<void> {
+async function applyChange(change: DiffChange): Promise<void> {Also applies to: 85-88
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/commands/config/apply.ts` around lines 65 - 67, The call to
applyChange(result.changes) passes an unused file parameter; remove the unused
parameter from applyChange's signature and all its call sites (including the
loop that does await applyChange(change, file)) or, if you intend to reserve it,
rename the parameter to _file in the applyChange function and the call sites to
suppress linter warnings; update applyChange's definition and every place that
calls applyChange to match the new parameter list (or underscore name) so the
codebase compiles cleanly.
Comment on lines
+42
to
+45
| const config: InsforgeConfig = { | ||
| auth: { | ||
| allowed_redirect_urls: raw.auth?.allowedRedirectUrls ?? [], | ||
| }, |
There was a problem hiding this comment.
Don’t synthesize allowed_redirect_urls: [] when metadata omits the field.
Line 44 conflates “missing” with “explicit empty,” which can produce unintended destructive diffs on apply.
Suggested patch
- const config: InsforgeConfig = {
- auth: {
- allowed_redirect_urls: raw.auth?.allowedRedirectUrls ?? [],
- },
- };
+ const redirects = raw.auth?.allowedRedirectUrls;
+ const config: InsforgeConfig =
+ redirects === undefined
+ ? {}
+ : {
+ auth: { allowed_redirect_urls: redirects },
+ };📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const config: InsforgeConfig = { | |
| auth: { | |
| allowed_redirect_urls: raw.auth?.allowedRedirectUrls ?? [], | |
| }, | |
| const redirects = raw.auth?.allowedRedirectUrls; | |
| const config: InsforgeConfig = | |
| redirects === undefined | |
| ? {} | |
| : { | |
| auth: { allowed_redirect_urls: redirects }, | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/commands/config/export.ts` around lines 42 - 45, The current construction
of const config: InsforgeConfig always emits auth.allowed_redirect_urls as an
empty array when raw.auth?.allowedRedirectUrls is missing; change the config
assembly so allowed_redirect_urls is only set when raw.auth?.allowedRedirectUrls
is defined (i.e. do not synthesize an empty array). Update the config/auth
object creation in export.ts (the const config and auth block) to conditionally
include allowed_redirect_urls (using a conditional spread or guard on
raw.auth?.allowedRedirectUrls !== undefined) so the field is omitted when the
source metadata omits it.
Comment on lines
+18
to
+20
| constructor(public readonly path: string, message: string) { | ||
| super(`config.${path}: ${message}`); | ||
| this.name = 'ConfigValidationError'; |
There was a problem hiding this comment.
Fix root-path error formatting (config.: ...).
At Line 30, an empty path renders as config.: ..., which is confusing in CLI output.
Suggested patch
export class ConfigValidationError extends Error {
constructor(public readonly path: string, message: string) {
- super(`config.${path}: ${message}`);
+ super(path ? `config.${path}: ${message}` : `config: ${message}`);
this.name = 'ConfigValidationError';
}
}Also applies to: 30-30
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/config-schema.ts` around lines 18 - 20, The constructor for
ConfigValidationError currently formats the message as `config.${path}:
${message}` which yields `config.: ...` when path is empty; update the
constructor in the ConfigValidationError class (the constructor(public readonly
path: string, message: string)) to conditionally format the prefix: use `config:
${message}` when path is empty or whitespace, otherwise use `config.<path>:
${message}` so the CLI output never shows a stray dot for root paths.
Comment on lines
+8
to
+10
| } catch (err) { | ||
| throw new Error(`TOML parse error: ${(err as Error).message}`); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify tsconfig target/lib compatibility for ErrorOptions ({ cause })
fd -i 'tsconfig.*\.json' -x sh -c 'echo "## $1"; rg -n "\"target\"|\"lib\"" "$1"' sh {}Repository: InsForge/CLI
Length of output: 126
🏁 Script executed:
cat -n src/lib/config-toml.ts | head -20Repository: InsForge/CLI
Length of output: 839
Preserve the original TOML parse exception as cause.
Line 9 currently drops the caught error context and fails the preserve-caught-error CI check. Error chaining via { cause } is supported by your ES2022 target.
Suggested patch
try {
parsed = smolToml.parse(input);
} catch (err) {
- throw new Error(`TOML parse error: ${(err as Error).message}`);
+ const cause = err instanceof Error ? err : new Error(String(err));
+ throw new Error(`TOML parse error: ${cause.message}`, { cause });
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| } catch (err) { | |
| throw new Error(`TOML parse error: ${(err as Error).message}`); | |
| } | |
| try { | |
| parsed = smolToml.parse(input); | |
| } catch (err) { | |
| const cause = err instanceof Error ? err : new Error(String(err)); | |
| throw new Error(`TOML parse error: ${cause.message}`, { cause }); | |
| } |
🧰 Tools
🪛 ESLint
[error] 9-9: There is no cause attached to the symptom error being thrown.
(preserve-caught-error)
🪛 GitHub Actions: CI / 0_Lint & Build.txt
[error] 9-9: ESLint preserve-caught-error: There is no cause attached to the symptom error being thrown
🪛 GitHub Actions: CI / Lint & Build
[error] 9-9: There is no cause attached to the symptom error being thrown preserve-caught-error
🪛 GitHub Check: Lint & Build
[failure] 9-9:
There is no cause attached to the symptom error being thrown
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/config-toml.ts` around lines 8 - 10, The catch block that throws new
Error("TOML parse error: ...") is losing the original parse exception; update
the throw to include the caught error as the ES2022 error cause (e.g., throw new
Error(`TOML parse error: ${(err as Error).message}`, { cause: err })); locate
the catch in src/lib/config-toml.ts where the TOML parse is handled and add the
{ cause: err } option so the original exception is preserved.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/lib/config-toml.ts (1)
8-10:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAttach the original parse exception as
causeon Line 9.Line 9 rethrows without
{ cause }, which both loses root error context and failspreserve-caught-errorin CI.Suggested patch
try { parsed = smolToml.parse(input); } catch (err) { - throw new Error(`TOML parse error: ${(err as Error).message}`); + const cause = err instanceof Error ? err : new Error(String(err)); + throw new Error(`TOML parse error: ${cause.message}`, { cause }); }#!/bin/bash # Verify the throw site and TS target/lib compatibility for ErrorOptions.cause. cat -n src/lib/config-toml.ts | sed -n '1,24p' fd -i 'tsconfig*.json' -x sh -c 'echo "## $1"; rg -n "\"target\"|\"lib\"" "$1"' sh {}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/config-toml.ts` around lines 8 - 10, Update the rethrow in the catch block in src/lib/config-toml.ts to attach the original exception as the Error.cause by changing the throw to: throw new Error(`TOML parse error: ${(err as Error).message}`, { cause: err }); and ensure your TS compilation target/libs support ErrorOptions.cause (adjust tsconfig "target" / "lib" if necessary).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@src/lib/config-toml.ts`:
- Around line 8-10: Update the rethrow in the catch block in
src/lib/config-toml.ts to attach the original exception as the Error.cause by
changing the throw to: throw new Error(`TOML parse error: ${(err as
Error).message}`, { cause: err }); and ensure your TS compilation target/libs
support ErrorOptions.cause (adjust tsconfig "target" / "lib" if necessary).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d4330d5e-e9b4-4edc-bf0f-9807451ac316
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (15)
package.jsonsrc/commands/config/apply.tssrc/commands/config/export.tssrc/commands/config/index.tssrc/commands/config/plan.tssrc/index.tssrc/lib/config-diff.test.tssrc/lib/config-diff.tssrc/lib/config-format.test.tssrc/lib/config-format.tssrc/lib/config-schema.tssrc/lib/config-secrets.test.tssrc/lib/config-secrets.tssrc/lib/config-toml.test.tssrc/lib/config-toml.ts
✅ Files skipped from review due to trivial changes (3)
- package.json
- src/lib/config-schema.ts
- src/lib/config-toml.test.ts
🚧 Files skipped from review as they are similar to previous changes (11)
- src/lib/config-diff.test.ts
- src/lib/config-format.test.ts
- src/commands/config/apply.ts
- src/lib/config-diff.ts
- src/commands/config/plan.ts
- src/index.ts
- src/commands/config/export.ts
- src/lib/config-secrets.test.ts
- src/lib/config-format.ts
- src/commands/config/index.ts
- src/lib/config-secrets.ts
Contributor
Author
|
Consolidated into #109 (the env() validation commit is now part of the unified config-as-code PR for combined review/merge). |
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
1 participant
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.
Summary
parseEnvRef()andvalidateSensitiveString()insrc/lib/config-secrets.tsclient_secret, SMTP password, S3 secret key, etc.) MUST beenv(NAME)references — never literal valuesinsforge.tomlunconditionally safe to commit to git[email.smtp]or[auth.providers.<built_in>]Why this is a separate branch
The MVP scope on
feat/insforge-toml-mvp(PR #109) is[auth] allowed_redirect_urlsonly — zero sensitive fields, so the validator wouldn't be exercised. This branch lands the foundation so the next sensitive section to ship has the validator ready to register.Test plan
src/lib/config-secrets.test.ts— all passsrc/libtest suite (18 tests acrossconfig-toml,config-diff,config-format,config-secrets) passesvalidateConfig()when first sensitive field section ships🤖 Generated with Claude Code
Summary by cubic
Adds insforge.toml support to the CLI with
config export,plan, andapplyfor[auth] allowed_redirect_urls. Sets upenv(NAME)validation for sensitive fields to keep configs safe to commit.insforge configsubcommands:exportwrites live config toinsforge.toml;planshows a readable diff;applyupdates via PUT/api/auth/configwith confirm and--dry-run/--auto-approve.smol-toml, with deterministic formatting.diffConfig()+formatPlan()with default-keep semantics for absent fields.parseEnvRef()andvalidateSensitiveString()enforceenv(NAME)(UPPER_SNAKE_CASE); wiring will happen when SMTP/OAuth sections land.Written for commit 8f05013. Summary will update on new commits.
Summary by CodeRabbit