config: executable and key-bearing settings must never live in the tracked layer

john commented

2026-08-13 15:44:27 +00:00

Owner

Three config keys are executable or key-bearing, and all three live in .planning/config.json, which is tracked and travels with a clone. Clone a repo, run Cadence, and that repo's config decides what runs on your machine and where your provider key is read from.

key	what a hostile repo gets
`workflow.test_command`	a command Cadence runs (`cad-executor-contract:28,34`, `workflows/task.md:46`, `workflows/coverage.md:34,63`, `references/execute-parallel.md:28`)
`workflow.lint_command`	same, run by an executor before it commits
`review.key_file`	redirects which env file the provider key is read from

The key_file arm is the nastiest of the three. review-provider.mjs providersEnvPath(override) takes the value straight through, expands ~, and reads it, and --key-file <review.key_file, only if set> is passed from repo config at four call sites (references/review-triggers.md:197, workflows/decision-review.md:82, references/consult.md:43, workflows/config-review.md:27). A cloned repo can point it at a key file committed inside that same repo, and your review payload - your plan text, your diffs, your source - is then sent to the attacker's provider account. Silent, and it looks like a working review.

Bounded honestly: that arm needs a cross-model provider configured, and the default review.reviewers is ["claude-subagent"], so nothing leaves the machine on a default install. An env-set key also still beats the file. It fires on the configured path, which is the path a real user is on.

Decision: these never reach the tracked layer at all

Not a confirm-on-first-use prompt. If a value is an injection risk, the fix is that it is never committed in the first place, rather than asking the user to approve it after it has already arrived. A prompt puts a security decision in front of someone who has no way to judge it, on every fresh clone, which is exactly the prompt people learn to click through.

These keys become global-layer-only. A repo layer that sets one is IGNORED, with a warning naming the key and the file it came from.

The mechanism already exists and is inert

config.schema.json already carries a src field. Today it has exactly one value across 34 keys ("src": "repo"), the other keys have none, and NO code reads it - route.mjs:48 only mentions it in a comment about the retired waiver path. It is documentation that documents nothing.

Make it load-bearing. Add "src": "global" for the three keys above, have the merge honor it, and self-verify can then check in both directions the way it already does for routing cells. That reuses existing vocabulary instead of inventing a second config home.

Constraints

A repo layer setting a global-only key warns, never silently drops. Silent would hide both an attack and an honest mistake. mergeLayers already has warnings[] and callers already surface it.
cmdDetectCommands is unaffected. planning.mjs:2001-2065 emits fixed literals and never interpolates repo content, so the detected arm keeps working with no config key set at all. That is the migration path for the common case: most users never needed the key.
/cad-config writes these to the global layer. The write face stays in one place; only its destination changes for these keys.
Migration. An existing repo config carrying one of these keys warns on next read and points at the global equivalent. It does not silently move it, and it does not error out.

Consistency

Same root fact as #114: a tracked .planning/config.json is attacker-controlled input, not the user's own settings. Both fixes should land together and be tested against the same hostile-repo fixture.

Three config keys are executable or key-bearing, and all three live in `.planning/config.json`, which is tracked and travels with a clone. Clone a repo, run Cadence, and that repo's config decides what runs on your machine and where your provider key is read from. | key | what a hostile repo gets | |---|---| | `workflow.test_command` | a command Cadence runs (`cad-executor-contract:28,34`, `workflows/task.md:46`, `workflows/coverage.md:34,63`, `references/execute-parallel.md:28`) | | `workflow.lint_command` | same, run by an executor before it commits | | `review.key_file` | redirects which env file the provider key is read from | The `key_file` arm is the nastiest of the three. `review-provider.mjs providersEnvPath(override)` takes the value straight through, expands `~`, and reads it, and `--key-file <review.key_file, only if set>` is passed from repo config at four call sites (`references/review-triggers.md:197`, `workflows/decision-review.md:82`, `references/consult.md:43`, `workflows/config-review.md:27`). A cloned repo can point it at a key file committed inside that same repo, and your review payload - your plan text, your diffs, your source - is then sent to the attacker's provider account. Silent, and it looks like a working review. Bounded honestly: that arm needs a cross-model provider configured, and the default `review.reviewers` is `["claude-subagent"]`, so nothing leaves the machine on a default install. An env-set key also still beats the file. It fires on the configured path, which is the path a real user is on. ## Decision: these never reach the tracked layer at all Not a confirm-on-first-use prompt. If a value is an injection risk, the fix is that it is never committed in the first place, rather than asking the user to approve it after it has already arrived. A prompt puts a security decision in front of someone who has no way to judge it, on every fresh clone, which is exactly the prompt people learn to click through. These keys become global-layer-only. A repo layer that sets one is IGNORED, with a warning naming the key and the file it came from. ## The mechanism already exists and is inert `config.schema.json` already carries a `src` field. Today it has exactly one value across 34 keys (`"src": "repo"`), the other keys have none, and NO code reads it - `route.mjs:48` only mentions it in a comment about the retired waiver path. It is documentation that documents nothing. Make it load-bearing. Add `"src": "global"` for the three keys above, have the merge honor it, and self-verify can then check in both directions the way it already does for routing cells. That reuses existing vocabulary instead of inventing a second config home. ## Constraints - **A repo layer setting a global-only key warns, never silently drops.** Silent would hide both an attack and an honest mistake. `mergeLayers` already has `warnings[]` and callers already surface it. - **`cmdDetectCommands` is unaffected.** `planning.mjs:2001-2065` emits fixed literals and never interpolates repo content, so the detected arm keeps working with no config key set at all. That is the migration path for the common case: most users never needed the key. - **`/cad-config` writes these to the global layer.** The write face stays in one place; only its destination changes for these keys. - **Migration.** An existing repo config carrying one of these keys warns on next read and points at the global equivalent. It does not silently move it, and it does not error out. ## Consistency Same root fact as #114: a tracked `.planning/config.json` is attacker-controlled input, not the user's own settings. Both fixes should land together and be tested against the same hostile-repo fixture.

john added this to the v3.2.0 milestone

2026-08-13 15:44:27 +00:00

john added the

needs-decision

security

labels

2026-08-13 15:44:27 +00:00

john commented

2026-08-13 15:46:49 +00:00

Author

Owner

Decided: option 1, confirm on first sight per repo

workflow.test_command and workflow.lint_command stay in .planning/config.json. On the first run in a given repo, show the command and ask once, then remember the answer keyed by repo path so it is a single prompt on a fresh clone and silent thereafter.

Rejected: option 2 (per-machine trust record) splits config across two homes and complicates /cad-config for a case the prompt already covers. Option 3 (document and accept) puts the burden on the user least likely to read it.

Constraints the implementation has to respect

The trust record is per-machine and NOT in .planning/. Storing it in the tracked tree would let a cloned repo pre-approve its own command, which is the whole defect. It belongs beside the global config, keyed by repo path.
Key the record on the command TEXT, not just the repo. If the config's command changes after approval, that is a new command and it asks again. Otherwise a repo approved once can swap the command on a later pull and inherit the old approval.
Unattended runs must not auto-approve. Under git.auto_close there is no user to ask, so an unapproved command is a refusal with a clear reason, never a silent yes. Same posture as #116's halt gate: absence of an answer is not an answer.
cmdDetectCommands stays as it is. planning.mjs:2001-2065 emits fixed literals and never interpolates repo content, so a detected command is already trustworthy. Only a command that came out of a config layer needs the prompt.

Consistent with #114: both treat a tracked .planning/config.json as attacker-controlled input rather than as the user's own settings.

## Decided: option 1, confirm on first sight per repo `workflow.test_command` and `workflow.lint_command` stay in `.planning/config.json`. On the first run in a given repo, show the command and ask once, then remember the answer keyed by repo path so it is a single prompt on a fresh clone and silent thereafter. Rejected: option 2 (per-machine trust record) splits config across two homes and complicates `/cad-config` for a case the prompt already covers. Option 3 (document and accept) puts the burden on the user least likely to read it. ## Constraints the implementation has to respect - **The trust record is per-machine and NOT in `.planning/`.** Storing it in the tracked tree would let a cloned repo pre-approve its own command, which is the whole defect. It belongs beside the global config, keyed by repo path. - **Key the record on the command TEXT, not just the repo.** If the config's command changes after approval, that is a new command and it asks again. Otherwise a repo approved once can swap the command on a later pull and inherit the old approval. - **Unattended runs must not auto-approve.** Under `git.auto_close` there is no user to ask, so an unapproved command is a refusal with a clear reason, never a silent yes. Same posture as #116's halt gate: absence of an answer is not an answer. - **`cmdDetectCommands` stays as it is.** `planning.mjs:2001-2065` emits fixed literals and never interpolates repo content, so a detected command is already trustworthy. Only a command that came out of a config layer needs the prompt. Consistent with #114: both treat a tracked `.planning/config.json` as attacker-controlled input rather than as the user's own settings.

john added

bug

and removed

needs-decision

labels

2026-08-13 15:46:49 +00:00

john changed title from ~~workflow.test_command: a cloned repo chooses what runs on your machine~~ to config: executable and key-bearing settings must never live in the tracked layer

2026-08-13 15:48:31 +00:00

john commented

2026-08-13 15:48:31 +00:00

Author

Owner

Superseding the previous comment. Option 1 (confirm on first sight) is withdrawn. The right fix is that these values never reach a tracked file at all - approving an injection risk after it has arrived is the wrong shape, and it trains people to click through a security prompt on every fresh clone. Scope also grew: review.key_file belongs in this class and is the worst of the three, because it can redirect your review payload to an attacker's provider account. Issue body rewritten.

**Superseding the previous comment.** Option 1 (confirm on first sight) is withdrawn. The right fix is that these values never reach a tracked file at all - approving an injection risk after it has arrived is the wrong shape, and it trains people to click through a security prompt on every fresh clone. Scope also grew: `review.key_file` belongs in this class and is the worst of the three, because it can redirect your review payload to an attacker's provider account. Issue body rewritten.

john closed this issue

2026-08-14 17:01:41 +00:00

john commented

2026-08-14 17:01:59 +00:00

Author

Owner

Fixed in v3.2.0 (PR #124). CFG-02, phase 1. workflow.test_command, workflow.lint_command and review.key_file are global-layer only now. A repo layer that sets one is ignored and warns, naming the key and the file it came from, and a repo layer holding a non-object ancestor is stripped and warned too. The src field in config.schema.json carries the scope and self-verify checks it in both directions.

Verified through the phase UAT walk. The milestone audit traced 12/12 requirements with 0 broken and 36/36 acceptance criteria covered.

Fixed in v3.2.0 (PR #124). CFG-02, phase 1. workflow.test_command, workflow.lint_command and review.key_file are global-layer only now. A repo layer that sets one is ignored and warns, naming the key and the file it came from, and a repo layer holding a non-object ancestor is stripped and warned too. The src field in config.schema.json carries the scope and self-verify checks it in both directions. Verified through the phase UAT walk. The milestone audit traced 12/12 requirements with 0 broken and 36/36 acceptance criteria covered.

Rows
Columns

config: executable and key-bearing settings must never live in the tracked layer #119

Decision: these never reach the tracked layer at all

The mechanism already exists and is inert

Constraints

Consistency

Decided: option 1, confirm on first sight per repo

Constraints the implementation has to respect