From ddec44667da310308e47c1436fde0392e1a872a8 Mon Sep 17 00:00:00 2001 From: Chris Lee Date: Sat, 20 Jun 2026 22:17:57 +1000 Subject: [PATCH 1/2] feat(security): add redact secretScan mode with placeholder-aware copy `secretScan = "redact"` replaces an ordinary API token in structured config (.claude.json, mcp.json, Codex config.toml, hook settings) with a $AGENTSYNC_REDACTED_ placeholder and pushes, instead of aborting. A secret in prose still aborts; the catastrophic tier (age key, PEM) still blocks in every mode. On copy, mergePreservingSecrets (new src/core/secret-merge.ts) merges incoming over the local file so a placeholder never overwrites a real local value, local-only entries survive, and array elements (e.g. MCP args) merge element-wise. Wired into all four redacting adapters: claude, codex, cursor, vscode. The cursor/vscode apply paths previously overwrote wholesale. TUI Config tab: redact joins the cycle, a "what this means" panel explains the selected mode, and switching to off prompts a y/n confirm (blocking modal). Closes #181. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_011L7s33UCjpQkreXW7amAw2 --- CLAUDE.md | 16 +++- docs/architecture.md | 2 +- docs/commands.md | 4 +- docs/operations.md | 19 +++- src/agents/_utils.ts | 25 +++++ src/agents/claude/__tests__/index.test.ts | 27 ++++++ src/agents/claude/index.ts | 11 ++- src/agents/codex/__tests__/index.test.ts | 17 ++++ src/agents/codex/index.ts | 8 +- src/agents/cursor/__tests__/index.test.ts | 14 +++ src/agents/cursor/index.ts | 22 ++++- src/agents/vscode/__tests__/index.test.ts | 14 +++ src/agents/vscode/index.ts | 22 ++++- src/commands/__tests__/push.test.ts | 20 ++++ src/commands/tui/__tests__/config-tab.test.ts | 62 +++++++++++- src/commands/tui/app.ts | 12 +++ src/commands/tui/state.ts | 4 + src/commands/tui/tabs/config.ts | 96 +++++++++++++++++-- src/config/schema.ts | 10 +- src/core/__tests__/sanitizer.test.ts | 44 +++++++++ src/core/__tests__/secret-merge.test.ts | 67 +++++++++++++ src/core/sanitizer.ts | 66 +++++++++++-- src/core/secret-merge.ts | 92 ++++++++++++++++++ 23 files changed, 632 insertions(+), 42 deletions(-) create mode 100644 src/core/__tests__/secret-merge.test.ts create mode 100644 src/core/secret-merge.ts diff --git a/CLAUDE.md b/CLAUDE.md index 69331ea..8f9432e 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -58,10 +58,18 @@ bun run check:act # run CI workflow locally via nektos/act ## Conventions and gotchas -- **Encryption is non-negotiable**: every artifact written to the vault must - go through `src/core/encryptor.ts`. `src/core/sanitizer.ts` enforces - hard never-sync patterns and aborts the push when literal secrets are - detected — do not loosen these without a documented reason. +- **Encryption is non-negotiable; the secret policy is tiered**: every + artifact written to the vault goes through `src/core/encryptor.ts`. + `src/core/sanitizer.ts` enforces hard never-sync patterns plus a + **catastrophic tier** (`ALWAYS_BLOCK_PATTERNS` — the vault's own age key and + PEM private keys) that aborts the push in EVERY `secretScan` mode (`off` and + `redact` included) and is never exemptible via `allowSecretValues`. Ordinary + API tokens follow the mode: `standard`/`strict` abort; `redact` replaces them + in structured config with a `$AGENTSYNC_REDACTED_` placeholder and + pushes (a secret in prose still aborts); `off` waives them. On the apply side, + redact placeholders are reconciled by `mergePreservingSecrets` + (`src/core/secret-merge.ts`) — a placeholder never overwrites a real local + value. Do not loosen these without a documented reason. - **Per-machine vault layout (v2)**: every artifact lives under `machines///…`, composed only through `machineVaultRoot` in `src/config/paths.ts` (never hardcode the `machines/` segment). Each machine diff --git a/docs/architecture.md b/docs/architecture.md index f66f773..4b7aef9 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -228,7 +228,7 @@ Daemon installation paths per OS, log locations, and the configuration table liv Three places own the security contract: - **Encryptor**: the only path that generates age identities, derives recipients, and encrypts content. Plaintext never leaves this layer for any artefact going to disk or to the network. -- **Sanitiser**: the only place that decides what is safe to encrypt. Never-sync paths are hard-coded rules. Literal-secret detection is a **known-credential-format** guard (vendor key prefixes, AWS/GitHub/GitLab/Slack/Google tokens, age identities, PEM private keys, and JWTs in strict mode) — not a general secret scanner: a plain password or bespoke token with no recognised shape is not caught, so encryption, not the scan, is the real protection. The scan's job is to keep well-known credentials out of git history. Its breadth and the base64 redactor are tuned through the `[security]` config, resolved by `securityToPolicy`; the never-sync rules are not configurable. +- **Sanitiser**: the only place that decides what is safe to encrypt. Never-sync paths are hard-coded rules. Literal-secret detection is a **known-credential-format** guard (vendor key prefixes, AWS/GitHub/GitLab/Slack/Google tokens, age identities, PEM private keys, and JWTs in strict mode) — not a general secret scanner: a plain password or bespoke token with no recognised shape is not caught, so encryption, not the scan, is the real protection. The scan's job is to keep well-known credentials out of git history. Its breadth and the base64 redactor are tuned through the `[security]` config, resolved by `securityToPolicy`; the never-sync rules are not configurable. A **catastrophic tier** (the vault's own age key and PEM private keys) blocks the push in every mode and cannot be allow-listed. `secretScan = redact` swaps an ordinary token in structured config for a `$AGENTSYNC_REDACTED_` placeholder and pushes; on apply, `mergePreservingSecrets` keeps a real local value rather than overwrite it with the placeholder. - **Tar bundler**: exists because some agent assets are directory-shaped. The tar is built in memory before encryption so an intermediate plaintext archive never lands on disk. Private keys stay on disk in the local runtime directory (`~/.config/agentsync/key.txt` by default on Unix, with restrictive permissions). They are never committed and never logged. diff --git a/docs/commands.md b/docs/commands.md index 947e416..f794827 100644 --- a/docs/commands.md +++ b/docs/commands.md @@ -78,7 +78,7 @@ agentsync tui # explicit alias, same behaviour | 3 Machines | The vault's `machines//` namespaces. Move with `↑`/`↓`; `enter` copies the selected machine's config to this machine (the same `performCopy` core as the CLI; never touches the vault). | | 4 Migrate | From / To / Type form (To and Type are multi-select with sub-cursor). Preview is mandatory before Apply enables. | | 5 Activity | Session-only ring buffer of TUI actions. | -| 6 Config | View and change vault config (agents enabled, `sync.*`, `claudePlugins.*`, `security.*`) with `↑`/`↓` to move, `space` to toggle a boolean, `←`/`→` to cycle an enum or adjust a number. Writes go through the same [`config`](#config) core (reconcile + commit + push). Also lists the recipients who can decrypt the vault, read-only. | +| 6 Config | View and change vault config (agents enabled, `sync.*`, `claudePlugins.*`, `security.*`) with `↑`/`↓` to move, `space` to toggle a boolean, `←`/`→` to cycle an enum or adjust a number. Cycling `security.secretScan` shows a one-line explainer of the selected mode; choosing `off` first prompts a `y`/`n` confirm because it pushes live secrets. Writes go through the same [`config`](#config) core (reconcile + commit + push). Also lists the recipients who can decrypt the vault, read-only. | **Global keys** (any tab): @@ -376,7 +376,7 @@ agentsync config set security.allowSecretValues '["AKIA-not-a-real-key"]' | `sync.debounceMs` | integer 50–10000 | Daemon quiet-window before an auto-push. | | `sync.autoPush` | boolean | Whether the daemon auto-pushes on change. | | `claudePlugins.syncPlugins` | boolean | Record the Claude plugin reinstall manifest on push. | -| `security.secretScan` | `standard`\|`strict`\|`off` | Push-time secret-scan mode. `standard` = built-in credential patterns; `strict` also flags JWTs; `off` waives the ordinary API-token patterns (the catastrophic tier — age key, PEM private keys — still blocks in every mode). | +| `security.secretScan` | `standard`\|`strict`\|`redact`\|`off` | Push-time secret-scan mode. `standard` = built-in credential patterns (abort on hit); `strict` also flags JWTs; `redact` replaces ordinary tokens in structured config with a `$AGENTSYNC_REDACTED_` placeholder and pushes (`copy` then preserves a real local value over the placeholder); `off` waives the ordinary patterns. The catastrophic tier (age key, PEM) still blocks in every mode. | | `security.allowSecretValues` | string[] (JSON) | Literal values exempt from ordinary-token detection and base64 redaction. Catastrophic-tier values (age key, PEM private keys) are never exemptible. | | `security.redactBase64Values` | boolean | When `true` (default), redact long base64-looking JSON values; set `false` if a config legitimately stores base64 that must round-trip. | diff --git a/docs/operations.md b/docs/operations.md index a89f4c3..36cac80 100644 --- a/docs/operations.md +++ b/docs/operations.md @@ -345,11 +345,24 @@ What it does **not** catch: a plain password, a bespoke or internal API token, a `agentsync config set security.secretScan `: -- `standard` (default) — the built-in credential patterns above, minus JWTs. +- `standard` (default) — the built-in credential patterns above, minus JWTs. A literal token aborts the push. - `strict` — adds JWT detection. Use when no legitimate JWT appears in your config. -- `off` — waives the ordinary API-token patterns; those values ride into the (encrypted) vault unflagged. The **catastrophic tier still blocks in every mode, `off` included**: the vault's own age key (`AGE-SECRET-KEY-1…`) and PEM private keys can never be pushed — no encryption makes it safe to commit the key that decrypts the vault itself. **Skill-bundle interiors are still scanned at `standard`** as a fail-safe. +- `redact` — instead of aborting, replace an ordinary API token inside a structured config file (`.claude.json`, `mcp.json`, Codex `config.toml`, hook settings) with a `$AGENTSYNC_REDACTED_` placeholder and push. A secret in **prose** (a markdown body, a skill README) has no structured field to replace, so it still aborts — remove it. See "The redact contract" below. +- `off` — waives the ordinary API-token patterns; those values ride into the (encrypted) vault unflagged. Encryption is then the *only* protection: every recipient and any lost device key can read them. -`agentsync config set security.allowSecretValues '[""]'` exempts a specific value the scanner false-positives on (and exempts it from base64 redaction). `agentsync config set security.redactBase64Values false` stops AgentSync replacing long base64-looking JSON values with a placeholder, for configs that legitimately store such values. See [config](commands.md#config). +In **every** mode — `redact` and `off` included — the **catastrophic tier still blocks**: the vault's own age key (`AGE-SECRET-KEY-1…`) and PEM private keys can never be pushed, redacted, or allow-listed, because no encryption makes it safe to commit the key that decrypts the vault itself. **Skill-bundle interiors are always scanned at `standard`** as a fail-safe. + +`agentsync config set security.allowSecretValues '[""]'` exempts a specific value the scanner false-positives on (and exempts it from base64 redaction) — for ordinary tokens only; a catastrophic-tier literal is refused even here. `agentsync config set security.redactBase64Values false` stops AgentSync replacing long base64-looking JSON values with a placeholder, for configs that legitimately store such values. See [config](commands.md#config). + +#### The redact contract + +`redact` mode is a round trip, and the second half lives on the **apply** side (`agentsync copy`): + +1. **Push** replaces the token with `$AGENTSYNC_REDACTED_` — a shell-env-var-shaped placeholder — and ships that. The real secret never enters the vault. +2. **Copy** onto another machine merges the incoming config: a placeholder **never overwrites a real local value**, and local-only entries (e.g. an MCP server not in the vault) are preserved. So a machine that already holds the key keeps it. +3. On a **fresh** machine with no local value, the placeholder lands in the config as-is. AgentSync does **not** expand it — replace `$AGENTSYNC_REDACTED_` with the real secret on that machine (paste the value, or point the field at your own secret manager / the agent's native `${VAR}` env syntax). The literal `$AGENTSYNC_REDACTED_…` is your signal that a value is required. + +This is the right default when a vault has more than one recipient (a teammate's key, or another of your own devices): encryption alone lets every recipient read every secret, so keeping the token out of the vault entirely is the only way to scope it to the machine that owns it. ### Daemon is not running diff --git a/src/agents/_utils.ts b/src/agents/_utils.ts index 7dc6737..8bf19fd 100644 --- a/src/agents/_utils.ts +++ b/src/agents/_utils.ts @@ -102,6 +102,31 @@ export function setJsoncTopLevelKey(raw: string, key: string, value: unknown): s return applyEdits(raw, edits); } +/** + * Read a single top-level key from a JSONC document, tolerating comments and + * trailing commas. Returns undefined when the document is empty, malformed, a + * non-object root, or the key is absent — callers treat "no local value" the + * same as a missing file, so a best-effort parse is the right contract here. + */ +export function getJsoncTopLevelKey(raw: string, key: string): unknown { + const parsed = parse(raw, [], { allowTrailingComma: true }); + return parsed && typeof parsed === "object" && !Array.isArray(parsed) + ? (parsed as Record)[key] + : undefined; +} + +/** + * Parse a whole JSONC document into a plain object, tolerating comments and + * trailing commas. Returns undefined for an empty, malformed, or non-object + * root — callers treat that as "no local value to merge against". + */ +export function parseJsoncObject(raw: string): Record | undefined { + const parsed = parse(raw, [], { allowTrailingComma: true }); + return parsed && typeof parsed === "object" && !Array.isArray(parsed) + ? (parsed as Record) + : undefined; +} + /** * Build a `SnapshotArtifact` from a `RedactionResult`. * Using this helper keeps the artifact shape consistent across agents. diff --git a/src/agents/claude/__tests__/index.test.ts b/src/agents/claude/__tests__/index.test.ts index 9e29aeb..d3404f8 100644 --- a/src/agents/claude/__tests__/index.test.ts +++ b/src/agents/claude/__tests__/index.test.ts @@ -318,6 +318,33 @@ describe("apply* functions", () => { expect((parsed.mcpServers as Record).srv).toBeDefined(); }); + test("applyClaudeMcp keeps a local secret when the vault ships a placeholder", async () => { + await writeFile( + testClaudePaths.mcpJson, + JSON.stringify({ + mcpServers: { + foo: { command: "x", env: { TOKEN: "sk-real-local" } }, + localOnly: { command: "z" }, + }, + }), + "utf8", + ); + // `redact` mode ships a $AGENTSYNC_REDACTED_ placeholder for foo.env.TOKEN. + await claudeModule.applyClaudeMcp( + JSON.stringify({ + mcpServers: { foo: { command: "x", env: { TOKEN: "$AGENTSYNC_REDACTED_TOKEN" } } }, + }), + ); + const parsed = JSON.parse(await Bun.file(testClaudePaths.mcpJson).text()) as Record< + string, + { foo: { env: { TOKEN: string } }; localOnly?: unknown } + >; + const mcp = parsed.mcpServers; + expect(mcp.foo.env.TOKEN).toBe("sk-real-local"); // local secret preserved + expect(mcp.localOnly).toBeDefined(); // local-only server survives + expect(JSON.stringify(parsed)).not.toContain("AGENTSYNC_REDACTED"); + }); + test("applyClaudeCommand writes a command file", async () => { await claudeModule.applyClaudeCommand("review.md", "# Code review command"); const content = await Bun.file(join(testClaudePaths.commandsDir, "review.md")).text(); diff --git a/src/agents/claude/index.ts b/src/agents/claude/index.ts index a721329..c565c69 100644 --- a/src/agents/claude/index.ts +++ b/src/agents/claude/index.ts @@ -3,6 +3,7 @@ import { AgentPaths } from "../../config/paths"; import type { AgentSyncConfig } from "../../config/schema"; import { denormalizeFromVault } from "../../core/path-portability"; import { securityToPolicy } from "../../core/sanitizer"; +import { mergePreservingSecrets } from "../../core/secret-merge"; import { type ApplyPlan, defineFileArtifact, @@ -14,6 +15,7 @@ import { collectMarkdownDir, collectSingleFile } from "../_snapshot"; import { atomicWrite, collect, + getJsoncTopLevelKey, readIfExists, type SnapshotArtifact, type SnapshotResult, @@ -155,10 +157,15 @@ export async function applyClaudeMcp(claudeJsonContent: string): Promise { const incoming = JSON.parse(claudeJsonContent) as Record; // ~/.claude.json is large and JSONC-tolerant. Edit `mcpServers` in place so // the rest of Claude's config (and any trailing comma) is left untouched. - const mcpServers = denormalizeFromVault(incoming.mcpServers ?? {}, home); + const incomingMcp = denormalizeFromVault(incoming.mcpServers ?? {}, home); + // Merge onto the local mcpServers: a redacted placeholder (`redact` mode) + // must not overwrite a real local key, and local-only servers survive (copy + // is additive). With no local file this is just the incoming tree. + const existingMcp = existingRaw ? getJsoncTopLevelKey(existingRaw, "mcpServers") : undefined; + const { merged } = mergePreservingSecrets(existingMcp ?? {}, incomingMcp); await atomicWrite( AgentPaths.claude.mcpJson, - setJsoncTopLevelKey(existingRaw ?? "", "mcpServers", mcpServers), + setJsoncTopLevelKey(existingRaw ?? "", "mcpServers", merged), ); } diff --git a/src/agents/codex/__tests__/index.test.ts b/src/agents/codex/__tests__/index.test.ts index 36f87a6..931d528 100644 --- a/src/agents/codex/__tests__/index.test.ts +++ b/src/agents/codex/__tests__/index.test.ts @@ -232,6 +232,23 @@ describe("apply* functions", () => { expect(content).toContain("local_only"); }); + test("applyCodexConfig keeps a local secret when the vault ships a placeholder", async () => { + await writeFile( + testCodexPaths.configToml, + 'model = "gpt-4"\n\n[mcp.foo]\napi_key = "sk-real-local-secret"\n', + "utf8", + ); + // `redact` mode ships a $AGENTSYNC_REDACTED_ placeholder instead of the key. + await codexModule.applyCodexConfig( + 'model = "o3"\n\n[mcp.foo]\napi_key = "$AGENTSYNC_REDACTED_API_KEY"\n', + ); + + const content = await Bun.file(testCodexPaths.configToml).text(); + expect(content).toContain("o3"); // incoming non-secret wins + expect(content).toContain("sk-real-local-secret"); // local secret preserved + expect(content).not.toContain("AGENTSYNC_REDACTED"); // placeholder not written + }); + test("applyCodexRule writes a rule file", async () => { await codexModule.applyCodexRule("testing.md", "## Testing rules"); const content = await Bun.file(join(testCodexPaths.rulesDir, "testing.md")).text(); diff --git a/src/agents/codex/index.ts b/src/agents/codex/index.ts index d9c0c04..35fa1e6 100644 --- a/src/agents/codex/index.ts +++ b/src/agents/codex/index.ts @@ -10,6 +10,7 @@ import { type SecretPolicy, securityToPolicy, } from "../../core/sanitizer"; +import { mergePreservingSecrets } from "../../core/secret-merge"; import { type ApplyPlan, defineFileArtifact, @@ -144,9 +145,10 @@ export async function applyCodexConfig(content: string): Promise { return; } - // Shallow-merge at top level: incoming keys win, local-only keys survive. - const merged: TOML.JsonMap = { ...existing, ...incoming }; - await atomicWrite(AgentPaths.codex.configToml, TOML.stringify(merged)); + // Deep, placeholder-aware merge: a redacted placeholder (`redact` mode) must + // not overwrite a real local value, and nested local-only keys survive. + const { merged } = mergePreservingSecrets(existing, incoming); + await atomicWrite(AgentPaths.codex.configToml, TOML.stringify(merged as TOML.JsonMap)); } /** Restore one Codex rule markdown file from the vault. */ diff --git a/src/agents/cursor/__tests__/index.test.ts b/src/agents/cursor/__tests__/index.test.ts index ebc71d1..635df6a 100644 --- a/src/agents/cursor/__tests__/index.test.ts +++ b/src/agents/cursor/__tests__/index.test.ts @@ -274,6 +274,20 @@ describe("cursor apply functions", () => { expect(await Bun.file(testCursorPaths.mcpGlobal).text()).toBe(content); }); + test("applyCursorMcp keeps a local secret when the vault ships a placeholder", async () => { + const { applyCursorMcp } = cursorModule; + await Bun.write( + testCursorPaths.mcpGlobal, + JSON.stringify({ mcpServers: { foo: { env: { KEY: "sk-real-local" } } } }), + ); + await applyCursorMcp( + `${JSON.stringify({ mcpServers: { foo: { env: { KEY: "$AGENTSYNC_REDACTED_KEY" } } } }, null, 2)}\n`, + ); + const written = await Bun.file(testCursorPaths.mcpGlobal).text(); + expect(written).toContain("sk-real-local"); // local secret preserved + expect(written).not.toContain("AGENTSYNC_REDACTED"); + }); + test("applyCursorCommand writes named command file under commandsDir", async () => { const { applyCursorCommand } = cursorModule; await applyCursorCommand("my-cmd.md", "# My Cmd\nDo things."); diff --git a/src/agents/cursor/index.ts b/src/agents/cursor/index.ts index 66715e6..d317d73 100644 --- a/src/agents/cursor/index.ts +++ b/src/agents/cursor/index.ts @@ -5,6 +5,7 @@ import { AgentPaths } from "../../config/paths"; import type { AgentSyncConfig } from "../../config/schema"; import { denormalizeStringFromVault, normalizeStringForVault } from "../../core/path-portability"; import { sanitizeAndNormalizeJson, securityToPolicy } from "../../core/sanitizer"; +import { mergePreservingSecrets } from "../../core/secret-merge"; import { type ApplyPlan, defineFileArtifact, @@ -16,6 +17,7 @@ import { collectMarkdownDir } from "../_snapshot"; import { atomicWrite, collect, + parseJsoncObject, readIfExists, type SnapshotArtifact, type SnapshotResult, @@ -131,10 +133,22 @@ export async function applyCursorRules(rulesContent: string): Promise { * file is immediately usable by Cursor without a separate post-process. */ export async function applyCursorMcp(mcpJsonContent: string): Promise { - await atomicWrite( - AgentPaths.cursor.mcpGlobal, - denormalizeStringFromVault(mcpJsonContent, homedir()), - ); + const restored = denormalizeStringFromVault(mcpJsonContent, homedir()); + const existingRaw = await readIfExists(AgentPaths.cursor.mcpGlobal); + if (existingRaw === null) { + await atomicWrite(AgentPaths.cursor.mcpGlobal, restored); + return; + } + // Merge so a redacted placeholder (`redact` mode) never overwrites a real + // local key and local-only servers survive. On any parse failure, fall back + // to the restored content rather than lose the sync. + try { + const incoming = JSON.parse(restored); + const { merged } = mergePreservingSecrets(parseJsoncObject(existingRaw) ?? {}, incoming); + await atomicWrite(AgentPaths.cursor.mcpGlobal, `${JSON.stringify(merged, null, 2)}\n`); + } catch { + await atomicWrite(AgentPaths.cursor.mcpGlobal, restored); + } } /** diff --git a/src/agents/vscode/__tests__/index.test.ts b/src/agents/vscode/__tests__/index.test.ts index fd0e05e..f739afd 100644 --- a/src/agents/vscode/__tests__/index.test.ts +++ b/src/agents/vscode/__tests__/index.test.ts @@ -142,6 +142,20 @@ describe("applyVsCodeMcp", () => { await applyVsCodeMcp(content); expect(await Bun.file(testVsCodePaths.mcpJson).text()).toBe(content); }); + + test("keeps a local secret when the vault ships a placeholder", async () => { + const { applyVsCodeMcp } = vsCodeModule; + await Bun.write( + testVsCodePaths.mcpJson, + JSON.stringify({ servers: { foo: { env: { KEY: "sk-real-local" } } } }), + ); + await applyVsCodeMcp( + `${JSON.stringify({ servers: { foo: { env: { KEY: "$AGENTSYNC_REDACTED_KEY" } } } }, null, 2)}\n`, + ); + const written = await Bun.file(testVsCodePaths.mcpJson).text(); + expect(written).toContain("sk-real-local"); // local secret preserved + expect(written).not.toContain("AGENTSYNC_REDACTED"); + }); }); // ── dryRun vault apply ───────────────────────────────────────────────── diff --git a/src/agents/vscode/index.ts b/src/agents/vscode/index.ts index 0ddbc49..f4bddc3 100644 --- a/src/agents/vscode/index.ts +++ b/src/agents/vscode/index.ts @@ -3,10 +3,12 @@ import { AgentPaths } from "../../config/paths"; import type { AgentSyncConfig } from "../../config/schema"; import { denormalizeStringFromVault } from "../../core/path-portability"; import { sanitizeAndNormalizeJson, securityToPolicy } from "../../core/sanitizer"; +import { mergePreservingSecrets } from "../../core/secret-merge"; import { type ApplyPlan, defineFileArtifact, makeApplyVault } from "../_apply"; import { atomicWrite, collect, + parseJsoncObject, readIfExists, type SnapshotArtifact, type SnapshotResult, @@ -34,10 +36,22 @@ export async function snapshotVsCode(config?: AgentSyncConfig): Promise { - await atomicWrite( - AgentPaths.vscode.mcpJson, - denormalizeStringFromVault(mcpJsonContent, homedir()), - ); + const restored = denormalizeStringFromVault(mcpJsonContent, homedir()); + const existingRaw = await readIfExists(AgentPaths.vscode.mcpJson); + if (existingRaw === null) { + await atomicWrite(AgentPaths.vscode.mcpJson, restored); + return; + } + // Merge so a redacted placeholder (`redact` mode) never overwrites a real + // local key and local-only servers survive. Fall back to the restored + // content on any parse failure rather than lose the sync. + try { + const incoming = JSON.parse(restored); + const { merged } = mergePreservingSecrets(parseJsoncObject(existingRaw) ?? {}, incoming); + await atomicWrite(AgentPaths.vscode.mcpJson, `${JSON.stringify(merged, null, 2)}\n`); + } catch { + await atomicWrite(AgentPaths.vscode.mcpJson, restored); + } } // ─── Apply (pull side) ──────────────────────────────────────────────────────── diff --git a/src/commands/__tests__/push.test.ts b/src/commands/__tests__/push.test.ts index 147b5a6..c83da09 100644 --- a/src/commands/__tests__/push.test.ts +++ b/src/commands/__tests__/push.test.ts @@ -426,6 +426,26 @@ describe("performPush — literal secret embedded in markdown body", () => { expect(result.pushed).toBeGreaterThan(0); }); + test("redact mode still aborts on a secret in a prose body (nothing to redact)", async () => { + // redact only rewrites structured JSON/TOML values; a markdown body has no + // field to replace, so the secret-leak boundary must still abort the push. + mkdirSync(mutableCopilotPaths.promptsDir, { recursive: true }); + const promptPath = join(mutableCopilotPaths.promptsDir, "leaky.prompt.md"); + const fakeKey = `sk-ant-api03-${"A".repeat(48)}`; + writeFileSync(promptPath, `# Demo prompt\n\nMy API key is ${fakeKey}\n`, "utf8"); + + const configPath = resolveConfigPath(machine.vaultDir); + const config = await loadConfig(configPath); + config.security.secretScan = "redact"; + await writeConfig(configPath, config); + runGit(["commit", "-am", "config: redact"], machine.vaultDir); + runGit(["push", "origin", "main"], machine.vaultDir); + + const result = await pushMod.performPush({ agent: "copilot" }); + expect(result.fatal).toBe(true); + expect(result.errors.some((e) => e.includes("Detected literal secret"))).toBe(true); + }); + test("strict mode flags a JWT that standard mode lets through", async () => { mkdirSync(mutableCopilotPaths.promptsDir, { recursive: true }); const promptPath = join(mutableCopilotPaths.promptsDir, "jwt.prompt.md"); diff --git a/src/commands/tui/__tests__/config-tab.test.ts b/src/commands/tui/__tests__/config-tab.test.ts index 7eba480..15e2827 100644 --- a/src/commands/tui/__tests__/config-tab.test.ts +++ b/src/commands/tui/__tests__/config-tab.test.ts @@ -17,7 +17,7 @@ import { } from "../../../test-helpers/fixtures"; import { type ConfigRow, createInitialState } from "../state"; import { createStore, type Store } from "../store"; -import { ensureConfigLoaded, onConfigKey } from "../tabs/config"; +import { ensureConfigLoaded, onConfigKey, secretScanExplainer } from "../tabs/config"; function key(name: string): KeyEvent { return { name, sequence: name, ctrl: false, meta: false, shift: false } as unknown as KeyEvent; @@ -42,7 +42,7 @@ describe("onConfigKey — navigation", () => { key: "security.secretScan", value: "standard", kind: "enum", - options: ["standard", "strict", "off"], + options: ["standard", "strict", "redact", "off"], }, ]; @@ -75,6 +75,25 @@ describe("onConfigKey — navigation", () => { }); }); +describe("secretScanExplainer", () => { + test("returns a distinct non-empty line for each mode", () => { + const modes = ["standard", "strict", "redact", "off"]; + const lines = modes.map((m) => secretScanExplainer(m)); + for (const line of lines) expect(line.length).toBeGreaterThan(0); + expect(new Set(lines).size).toBe(modes.length); // all distinct + }); + + test("does not claim env-var auto-expansion for redact (placeholder is literal)", () => { + const redact = secretScanExplainer("redact").toLowerCase(); + expect(redact).toContain("$agentsync_redacted_"); + expect(redact).not.toContain("env var"); + }); + + test("returns empty string for an unknown mode", () => { + expect(secretScanExplainer("nope")).toBe(""); + }); +}); + describe("Config tab against a real vault", () => { let tmpDir: string; let machine: TestMachineFixture; @@ -167,14 +186,49 @@ describe("Config tab against a real vault", () => { expect(config.sync.debounceMs).toBe(350); }); - test("left cycles an enum backwards and wraps to the last option", async () => { + test("left wraps to off but gates the apply behind a y/n confirm", async () => { const store = createStore(createInitialState()); await loadAndFocus(store, "security.secretScan"); // standard (index 0) + // left wraps standard -> off, the dangerous transition: it opens the + // confirm modal instead of applying immediately. expect(onConfigKey(key("left"), store)).toBe(true); + expect(store.getState().config.pendingSecretScan).toBe("off"); + expect(store.getState().config.lastResult).toBeNull(); // not applied yet + + // y confirms and persists. + expect(onConfigKey(key("y"), store)).toBe(true); await waitFor(() => store.getState().config.lastResult !== null); + expect(store.getState().config.pendingSecretScan).toBeNull(); + const config = await loadConfig(resolveConfigPath(machine.vaultDir)); + expect(config.security.secretScan).toBe("off"); // wrapped to last, confirmed + }); + test("n cancels the off confirm and leaves the value unchanged", async () => { + const store = createStore(createInitialState()); + await loadAndFocus(store, "security.secretScan"); // standard + expect(onConfigKey(key("left"), store)).toBe(true); // -> off, modal opens + expect(store.getState().config.pendingSecretScan).toBe("off"); + expect(onConfigKey(key("n"), store)).toBe(true); // cancel + expect(store.getState().config.pendingSecretScan).toBeNull(); + + const config = await loadConfig(resolveConfigPath(machine.vaultDir)); + expect(config.security.secretScan).toBe("standard"); // untouched + }); + + test("cycling to redact applies immediately without a confirm", async () => { + const store = createStore(createInitialState()); + await loadAndFocus(store, "security.secretScan"); // standard -> strict -> redact + onConfigKey(key("right"), store); // strict + await waitFor(() => store.getState().config.lastResult !== null); + onConfigKey(key("right"), store); // redact + await waitFor( + () => + String(store.getState().config.rows.find((r) => r.key === "security.secretScan")?.value) === + "redact", + ); + expect(store.getState().config.pendingSecretScan).toBeNull(); const config = await loadConfig(resolveConfigPath(machine.vaultDir)); - expect(config.security.secretScan).toBe("off"); // wrapped to last + expect(config.security.secretScan).toBe("redact"); }); test("ensureConfigLoaded surfaces an error (not a crash) on an un-init vault", async () => { diff --git a/src/commands/tui/app.ts b/src/commands/tui/app.ts index 1900f51..46a3997 100644 --- a/src/commands/tui/app.ts +++ b/src/commands/tui/app.ts @@ -249,6 +249,18 @@ function handleKey(key: KeyEvent, ctx: AppContext, quit: () => void): void { return; } + // The Config tab's "switch secretScan → off?" confirm is a blocking modal, + // like the help overlay: route every key to the tab so a global `p` (push), + // number, or tab key cannot fire behind it. The tab handler answers y/n/esc + // and swallows the rest. + if ( + ctx.store.getState().activeTab === "config" && + ctx.store.getState().config.pendingSecretScan !== null + ) { + delegateTabKey(key, ctx); + return; + } + // Show the action before running it: flash the pressed key in the bars and // surface its label, so a keypress is visibly acknowledged even when the // handler that follows does slow work. diff --git a/src/commands/tui/state.ts b/src/commands/tui/state.ts index 18ccbb1..a1659e9 100644 --- a/src/commands/tui/state.ts +++ b/src/commands/tui/state.ts @@ -238,6 +238,9 @@ export interface ConfigSlice { error: string | null; /** Result of the most recent set, kept visible until the next one. */ lastResult: { ok: boolean; message: string } | null; + /** Pending `secretScan` value awaiting y/n confirmation (only the dangerous + * → `off` transition gates here). Null when no confirmation is open. */ + pendingSecretScan: string | null; } export interface AppState { @@ -315,6 +318,7 @@ export function createInitialState(): AppState { cursor: 0, error: null, lastResult: null, + pendingSecretScan: null, }, inFlight: {}, opSeq: 0, diff --git a/src/commands/tui/tabs/config.ts b/src/commands/tui/tabs/config.ts index 2e5da13..9cbb7c0 100644 --- a/src/commands/tui/tabs/config.ts +++ b/src/commands/tui/tabs/config.ts @@ -10,7 +10,23 @@ import type { Store } from "../store"; // Sections the Config tab lets you edit. Mirrors SETTABLE_PREFIXES in // commands/config.ts; remote/version/recipients are not editable here. const SETTABLE_PREFIXES = ["agents.", "sync.", "claudePlugins.", "security."]; -const SECRET_SCAN_OPTIONS = ["standard", "strict", "off"] as const; +const SECRET_SCAN_OPTIONS = ["standard", "strict", "redact", "off"] as const; + +/** One-line consequence of each secretScan mode, shown in the explainer panel. */ +export function secretScanExplainer(value: unknown): string { + switch (String(value)) { + case "standard": + return "Built-in credential patterns. A literal key or token blocks the push — you remove it and retry. Nothing secret-shaped enters the vault."; + case "strict": + return "standard + JWT detection. Higher false-positive rate; blocks the push on any match."; + case "redact": + return "API keys in JSON/TOML config are replaced with a $AGENTSYNC_REDACTED_ placeholder and pushed. copy keeps each machine's own real value; a fresh machine shows the placeholder to fill in. Secrets in prose still block."; + case "off": + return "No redaction — API keys are encrypted and pushed as-is. Protection is encryption ALONE: every recipient and any lost device key can read them."; + default: + return ""; + } +} const DEBOUNCE_STEP = 50; const DEBOUNCE_MIN = 50; const DEBOUNCE_MAX = 10_000; @@ -123,6 +139,23 @@ export function onConfigKey(key: KeyEvent, store: Store): boolean { const c = store.getState().config; if (c.phase !== "ready" || c.rows.length === 0) return false; + // The → off confirmation modal is blocking: it consumes every key until the + // user answers y (apply) or n/esc (cancel). + if (c.pendingSecretScan !== null) { + if (key.name === "y") { + const target = c.pendingSecretScan; + store.dispatch((d) => { + d.config.pendingSecretScan = null; + }); + setConfig(store, "security.secretScan", target); + } else if (key.name === "n" || key.name === "escape") { + store.dispatch((d) => { + d.config.pendingSecretScan = null; + }); + } + return true; + } + if (key.name === "down") { store.dispatch((d) => { d.config.cursor = Math.min(d.config.cursor + 1, d.config.rows.length - 1); @@ -150,7 +183,17 @@ export function onConfigKey(key: KeyEvent, store: Store): boolean { const found = row.options.indexOf(String(row.value)); const idx = found === -1 ? (dir === 1 ? -1 : 0) : found; const next = row.options[(idx + dir + row.options.length) % row.options.length]; - if (next !== undefined && next !== row.value) setConfig(store, row.key, next); + if (next !== undefined && next !== row.value) { + // The → off transition pushes live secrets into the vault, so it gates + // behind a y/n confirm. Every other value applies immediately. + if (row.key === "security.secretScan" && next === "off") { + store.dispatch((d) => { + d.config.pendingSecretScan = next; + }); + } else { + setConfig(store, row.key, next); + } + } return true; } if ((key.name === "left" || key.name === "right") && row.kind === "number") { @@ -226,6 +269,46 @@ export function renderConfig(renderer: CliRenderer, host: BoxRenderable, state: listBox.add(new TextRenderable(renderer, { content: body, fg: "#d8dee9", bg: "#11151a" })); wrapper.add(listBox); + const cursorRow = c.phase === "ready" ? c.rows[c.cursor] : undefined; + + // Context panel: the dangerous → off confirm, or a "what this means" line for + // the secretScan mode under the cursor. Nothing for ordinary rows. + if (c.pendingSecretScan !== null) { + const confirm = new BoxRenderable(renderer, { + width: "100%", + border: true, + borderColor: "#bf616a", + borderStyle: "single", + title: " Switch secretScan → off? ", + backgroundColor: "#11151a", + }); + confirm.add( + new TextRenderable(renderer, { + content: `\n ${secretScanExplainer("off")}\n Age keys & PEM private keys are still refused.\n\n [y] confirm [n] cancel`, + fg: "#e5c07b", + bg: "#11151a", + }), + ); + wrapper.add(confirm); + } else if (cursorRow?.key === "security.secretScan") { + const explain = new BoxRenderable(renderer, { + width: "100%", + border: true, + borderColor: "#3b4252", + borderStyle: "single", + title: " What this means ", + backgroundColor: "#11151a", + }); + explain.add( + new TextRenderable(renderer, { + content: `\n ${secretScanExplainer(cursorRow.value)}`, + fg: "#a9b3c0", + bg: "#11151a", + }), + ); + wrapper.add(explain); + } + // Recipients — who can decrypt the vault (read-only; `key list`). const recipientsBody = c.recipients.length > 0 @@ -247,10 +330,11 @@ export function renderConfig(renderer: CliRenderer, host: BoxRenderable, state: ); wrapper.add(recipientsBox); - const cursorRow = c.phase === "ready" ? c.rows[c.cursor] : undefined; - const hint = c.lastResult - ? ` last: ${c.lastResult.ok ? "✓" : "✗"} ${c.lastResult.message}` - : ` ↑↓ move • ${editHintFor(cursorRow)} • changes reconcile + push to the vault`; + const hint = c.pendingSecretScan + ? " press y to confirm switching to off, or n to cancel" + : c.lastResult + ? ` last: ${c.lastResult.ok ? "✓" : "✗"} ${c.lastResult.message}` + : ` ↑↓ move • ${editHintFor(cursorRow)} • changes reconcile + push to the vault`; wrapper.add( new TextRenderable(renderer, { height: 2, diff --git a/src/config/schema.ts b/src/config/schema.ts index 33a80c4..f9e1134 100644 --- a/src/config/schema.ts +++ b/src/config/schema.ts @@ -96,10 +96,14 @@ export const AgentSyncConfigSchema = z.object({ // How the push-time secret scan behaves: // standard — the built-in high-precision credential patterns (default) // strict — standard plus JWT detection + // redact — replace ordinary API tokens in structured config with a + // $AGENTSYNC_REDACTED_ placeholder and push, instead + // of aborting. Secrets in prose (markdown) still abort. // off — waive the ordinary API-token patterns (values ride in - // encrypted). The catastrophic tier (the vault's own age - // key, PEM private keys) still blocks the push in EVERY mode. - secretScan: z.enum(["standard", "strict", "off"]).default("standard"), + // encrypted). + // The catastrophic tier (the vault's own age key, PEM private keys) still + // blocks the push in EVERY mode, redact and off included. + secretScan: z.enum(["standard", "strict", "redact", "off"]).default("standard"), // Literal values to exempt from secret detection AND base64 redaction. // The escape hatch for a legitimate high-entropy config value that the // scanner/redactor would otherwise flag or silently replace. diff --git a/src/core/__tests__/sanitizer.test.ts b/src/core/__tests__/sanitizer.test.ts index e90df1b..520b3f7 100644 --- a/src/core/__tests__/sanitizer.test.ts +++ b/src/core/__tests__/sanitizer.test.ts @@ -339,3 +339,47 @@ describe("secret policy", () => { expect(out.tok).toContain("REDACTED"); // a real key prefix still redacts }); }); + +describe("redact mode", () => { + const redact: SecretPolicy = { mode: "redact", allow: [], redactBase64: true }; + + test("replaces an ordinary token with a placeholder and a NON-fatal notice", () => { + const out = redactSecretLiterals({ env: { TOKEN: `ghp_${"a".repeat(36)}` } }, "mcp", redact); + const value = out.value as { env: { TOKEN: string } }; + expect(value.env.TOKEN).toBe("$AGENTSYNC_REDACTED_TOKEN"); + // "Redacted", not "Detected" — push.ts only aborts on the "Detected" prefix. + expect(out.warnings).toEqual(["Redacted literal secret for field TOKEN"]); + expect(out.warnings.some((w) => w.startsWith("Detected literal secret"))).toBe(false); + }); + + test("covers embedded-only shapes the whole-value redactor misses (Anthropic)", () => { + const antKey = `sk-ant-api03-${"A".repeat(50)}`; + const out = redactSecretLiterals({ env: { K: antKey } }, "mcp", redact); + expect((out.value as { env: { K: string } }).env.K).toBe("$AGENTSYNC_REDACTED_K"); + }); + + test("leaves a catastrophic value untouched so the central scan still blocks it", () => { + const ageKey = `AGE-SECRET-KEY-1${"A".repeat(58)}`; + const out = redactSecretLiterals({ id: ageKey }, "mcp", redact); + expect((out.value as { id: string }).id).toBe(ageKey); // not redacted + expect(out.warnings).toHaveLength(0); + // The plaintext heading for encryption still carries the age key, so the + // central scanForSecrets aborts the push. + expect(scanForSecrets(JSON.stringify(out.value), "/tmp/x", redact)).toEqual([ + "Detected literal secret (age-secret-key) in /tmp/x", + ]); + }); + + test("honours the allow-list for ordinary tokens but never for catastrophic", () => { + const tok = `ghp_${"a".repeat(36)}`; + const ageKey = `AGE-SECRET-KEY-1${"A".repeat(58)}`; + const policy: SecretPolicy = { mode: "redact", allow: [tok, ageKey], redactBase64: true }; + const out = redactSecretLiterals({ a: tok, b: ageKey }, "root", policy); + const v = out.value as { a: string; b: string }; + expect(v.a).toBe(tok); // allow-listed ordinary: left as-is + expect(v.b).toBe(ageKey); // catastrophic: redactor leaves it for the scan to block + expect(scanForSecrets(ageKey, "/tmp/x", policy).some((w) => w.includes("age-secret-key"))).toBe( + true, + ); + }); +}); diff --git a/src/core/__tests__/secret-merge.test.ts b/src/core/__tests__/secret-merge.test.ts new file mode 100644 index 0000000..1014d20 --- /dev/null +++ b/src/core/__tests__/secret-merge.test.ts @@ -0,0 +1,67 @@ +import { describe, expect, test } from "bun:test"; +import { mergePreservingSecrets } from "../secret-merge"; + +const PH = "$AGENTSYNC_REDACTED_KEY"; + +describe("mergePreservingSecrets", () => { + test("an incoming placeholder never overwrites a real local value", () => { + const { merged, placeholders } = mergePreservingSecrets( + { srv: { env: { KEY: "sk-real-local" } } }, + { srv: { env: { KEY: PH } } }, + ); + expect((merged as { srv: { env: { KEY: string } } }).srv.env.KEY).toBe("sk-real-local"); + expect(placeholders).toEqual([]); // nothing left needing a value + }); + + test("a placeholder with no local value is written and reported", () => { + const { merged, placeholders } = mergePreservingSecrets({}, { srv: { env: { KEY: PH } } }); + expect((merged as { srv: { env: { KEY: string } } }).srv.env.KEY).toBe(PH); + expect(placeholders).toEqual(["srv.env.KEY"]); + }); + + test("local-only keys survive the merge (copy is additive)", () => { + const { merged } = mergePreservingSecrets( + { localOnly: { command: "z" }, shared: 1 }, + { shared: 2 }, + ); + const m = merged as { localOnly: unknown; shared: number }; + expect(m.localOnly).toEqual({ command: "z" }); + expect(m.shared).toBe(2); // incoming wins for shared keys + }); + + test("an incoming real value overwrites the local one (normal sync)", () => { + const { merged } = mergePreservingSecrets({ k: "old" }, { k: "new" }); + expect((merged as { k: string }).k).toBe("new"); + }); + + test("a local placeholder counts as no value, so the incoming placeholder is taken", () => { + const { merged, placeholders } = mergePreservingSecrets({ k: PH }, { k: PH }); + expect((merged as { k: string }).k).toBe(PH); + expect(placeholders).toEqual(["k"]); + }); + + test("a placeholder inside an array preserves the local element (e.g. MCP args)", () => { + // MCP args commonly carry a token, e.g. ["--token", "sk-real"]. The snapshot + // side redacts the element, so apply must merge element-wise. + const { merged, placeholders } = mergePreservingSecrets( + { srv: { args: ["--token", "sk-real-local"] } }, + { srv: { args: ["--token", PH] } }, + ); + expect((merged as { srv: { args: string[] } }).srv.args).toEqual(["--token", "sk-real-local"]); + expect(placeholders).toEqual([]); + }); + + test("non-secret array elements take the incoming value", () => { + const { merged } = mergePreservingSecrets( + { args: ["--old", "keep-me"] }, + { args: ["--new", "x"] }, + ); + expect((merged as { args: string[] }).args).toEqual(["--new", "x"]); + }); + + test("a prototype-polluting key from vault content is ignored", () => { + const { merged } = mergePreservingSecrets({}, JSON.parse('{"__proto__":{"x":1},"ok":1}')); + expect(({} as Record).x).toBeUndefined(); // no pollution + expect((merged as { ok: number }).ok).toBe(1); + }); +}); diff --git a/src/core/sanitizer.ts b/src/core/sanitizer.ts index b23d3a4..0551053 100644 --- a/src/core/sanitizer.ts +++ b/src/core/sanitizer.ts @@ -123,6 +123,12 @@ const ALWAYS_BLOCK_NAMES: ReadonlySet = new Set(["age-secret-key", "priv export const ALWAYS_BLOCK_PATTERNS: ReadonlyArray<{ name: string; pattern: RegExp }> = EMBEDDED_SECRET_PATTERNS.filter((p) => ALWAYS_BLOCK_NAMES.has(p.name)); +// The ordinary (API-token) embedded patterns: everything EMBEDDED scans except +// the catastrophic tier. `redact` mode replaces a structured-config value +// matching one of these with a placeholder, rather than aborting the push. +const ORDINARY_EMBEDDED_PATTERNS: ReadonlyArray<{ name: string; pattern: RegExp }> = + EMBEDDED_SECRET_PATTERNS.filter((p) => !ALWAYS_BLOCK_NAMES.has(p.name)); + // Additional patterns scanned only in `strict` mode. A JWT legitimately appears // in API examples and docs, so its higher false-positive rate is opt-in rather // than aborting every push that mentions one. @@ -136,11 +142,12 @@ export const STRICT_SECRET_PATTERNS: ReadonlyArray<{ name: string; pattern: RegE /** Secret-handling policy resolved from the vault's [security] config section. */ export interface SecretPolicy { /** - * `standard` (built-in patterns), `strict` (+ JWT), or `off` (waive the - * ordinary API-token patterns). The catastrophic tier (age key, PEM) blocks - * in every mode, `off` included. + * `standard` (built-in patterns, abort on hit), `strict` (+ JWT), `redact` + * (replace ordinary tokens in structured config with a placeholder and push), + * or `off` (waive the ordinary API-token patterns). The catastrophic tier + * (age key, PEM) blocks in every mode — `redact` and `off` included. */ - mode: "standard" | "strict" | "off"; + mode: "standard" | "strict" | "redact" | "off"; /** Literal values exempt from both detection and redaction. */ allow: readonly string[]; /** When false, the generic base64 whole-value redaction is skipped. */ @@ -160,7 +167,7 @@ export const DEFAULT_SECRET_POLICY: SecretPolicy = Object.freeze({ /** Resolve a {@link SecretPolicy} from the optional [security] config section. */ export function securityToPolicy(security?: { - secretScan?: "standard" | "strict" | "off"; + secretScan?: "standard" | "strict" | "redact" | "off"; allowSecretValues?: readonly string[]; redactBase64Values?: boolean; }): SecretPolicy { @@ -177,6 +184,15 @@ export interface RedactionResult { warnings: string[]; } +/** + * Prefix of the placeholder that `redact` mode writes in place of a secret. + * Shaped like a shell env-var reference (`$NAME`) for readability, but AgentSync + * does NOT expand it — on a fresh machine the user replaces it with the real + * value. `mergePreservingSecrets` keys off this prefix to avoid overwriting a + * real local value with a placeholder on `copy`. + */ +export const REDACTION_PLACEHOLDER_PREFIX = "$AGENTSYNC_REDACTED_"; + /** * Returns true when a file path matches any entry in NEVER_SYNC_PATTERNS. * NEVER_SYNC_PATTERNS is the authoritative list — add new patterns there and @@ -195,6 +211,31 @@ function looksLikeSecretLiteral(value: string, policy: SecretPolicy): boolean { return patterns.some((pattern) => pattern.test(value)); } +/** + * Classify a structured-config string value for `redact` mode. + * + * - `catastrophic` — an age key or PEM private key. Checked FIRST and never + * exemptible: redact mode leaves it untouched so the central `scanForSecrets` + * blocks the push (a placeholder would silently let the master key through). + * - `ordinary` — an API token (whole-value shape, or an embedded credential + * prefix appearing in the value). These are replaced with a placeholder. + * - `none` — nothing to redact. Includes allow-listed ordinary values. + * + * Ordinary coverage is the union of `looksLikeSecretLiteral`'s whole-value + * shapes and the ordinary embedded prefixes, so every ordinary token the + * scanner would later flag is redacted first (no surprise abort post-redaction). + */ +function classifyForRedact( + value: string, + policy: SecretPolicy, +): "catastrophic" | "ordinary" | "none" { + if (ALWAYS_BLOCK_PATTERNS.some(({ pattern }) => pattern.test(value))) return "catastrophic"; + if (policy.allow.includes(value)) return "none"; + if (looksLikeSecretLiteral(value, policy)) return "ordinary"; + if (ORDINARY_EMBEDDED_PATTERNS.some(({ pattern }) => pattern.test(value))) return "ordinary"; + return "none"; +} + /** * Scan arbitrary text for secrets embedded as substrings. Returns warnings * prefixed `Detected literal secret` so the Phase-1 abort in @@ -260,9 +301,22 @@ export function redactSecretLiterals( } if (typeof input === "string") { + if (policy.mode === "redact") { + // Replace ordinary tokens with a placeholder and emit a NON-fatal notice + // (prefix "Redacted", not "Detected", so the Phase-1 abort in push.ts + // skips it and Phase-2 surfaces it as a warning). A catastrophic value is + // left untouched so the central scanForSecrets still blocks the push. + if (classifyForRedact(input, policy) === "ordinary") { + return { + value: `${REDACTION_PLACEHOLDER_PREFIX}${fieldName.toUpperCase()}`, + warnings: [`Redacted literal secret for field ${fieldName}`], + }; + } + return { value: input, warnings: [] }; + } if (looksLikeSecretLiteral(input, policy)) { return { - value: `$AGENTSYNC_REDACTED_${fieldName.toUpperCase()}`, + value: `${REDACTION_PLACEHOLDER_PREFIX}${fieldName.toUpperCase()}`, warnings: [`Detected literal secret for field ${fieldName}`], }; } diff --git a/src/core/secret-merge.ts b/src/core/secret-merge.ts new file mode 100644 index 0000000..8f42415 --- /dev/null +++ b/src/core/secret-merge.ts @@ -0,0 +1,92 @@ +import { REDACTION_PLACEHOLDER_PREFIX } from "./sanitizer"; + +/** + * src/core/secret-merge.ts + * + * Placeholder-aware merge for the apply (copy) side. `redact` mode ships a + * `$AGENTSYNC_REDACTED_` placeholder into the vault instead of a real + * token (see `redactSecretLiterals`). When that placeholder is applied onto a + * machine that already holds the real secret, a wholesale overwrite would + * replace the working key with the placeholder and break the local config. + * + * `mergePreservingSecrets` overlays `incoming` onto `existing` with one rule: + * an incoming placeholder never clobbers a real local value. It also keeps + * local-only keys (copy is additive — a key absent from the vault stays), which + * incidentally fixes the prior wholesale-replace that dropped local-only MCP + * servers. + */ + +export interface SecretMergeResult { + /** The merged tree, ready to serialise back to disk. */ + merged: unknown; + /** Dotted field paths where a placeholder was written because no local value + * existed — the destination must still replace these with the real value. + * Informational: the apply callers currently ignore it (there is no + * apply-side warning channel yet); the placeholder itself is self-documenting + * on disk. Kept so a future copy-summary can surface "these need a value". */ + placeholders: string[]; +} + +function isPlainObject(value: unknown): value is Record { + return typeof value === "object" && value !== null && !Array.isArray(value); +} + +function isPlaceholder(value: unknown): boolean { + return typeof value === "string" && value.startsWith(REDACTION_PLACEHOLDER_PREFIX); +} + +function mergeNode( + existing: unknown, + incoming: unknown, + path: string, + placeholders: string[], +): unknown { + // Incoming placeholder: keep a real local value rather than overwrite it. Only + // fall through to writing the placeholder when there is no local value to + // preserve (a local placeholder counts as "no value"). + if (isPlaceholder(incoming)) { + if (existing !== undefined && existing !== null && !isPlaceholder(existing)) { + return existing; + } + placeholders.push(path || "(root)"); + return incoming; + } + + // Incoming object: deep-merge so nested secrets are preserved per-leaf and + // local-only keys survive (additive). When the local side is not an object + // (missing, or a type change) we still walk incoming over an empty base so + // nested placeholders are recorded — the merged output is just incoming. + if (isPlainObject(incoming)) { + const base = isPlainObject(existing) ? existing : {}; + const out: Record = { ...base }; + for (const [key, value] of Object.entries(incoming)) { + // Never assign through a prototype-polluting key from vault content. + if (key === "__proto__" || key === "constructor" || key === "prototype") continue; + out[key] = mergeNode(base[key], value, path ? `${path}.${key}` : key, placeholders); + } + return out; + } + + // Arrays: merge element-wise so a placeholder at index i preserves the local + // element. The snapshot side redacts inside arrays too (an MCP `args` array + // can carry a token, e.g. ["--token", "sk-…"]), so without this an incoming + // placeholder element would clobber the real local value — the exact failure + // the object branch prevents, one level down. + if (Array.isArray(incoming)) { + const base = Array.isArray(existing) ? existing : []; + return incoming.map((value, i) => mergeNode(base[i], value, `${path}[${i}]`, placeholders)); + } + + // Primitives and type changes: incoming wins (the synced value). + return incoming; +} + +/** + * Merge `incoming` (from the vault) onto `existing` (local disk), preserving any + * real local value an incoming redaction placeholder would otherwise overwrite. + */ +export function mergePreservingSecrets(existing: unknown, incoming: unknown): SecretMergeResult { + const placeholders: string[] = []; + const merged = mergeNode(existing, incoming, "", placeholders); + return { merged, placeholders }; +} From e9cf630475dcf662ddde165393f6492e39643698 Mon Sep 17 00:00:00 2001 From: Chris Lee Date: Sat, 20 Jun 2026 22:29:45 +1000 Subject: [PATCH 2/2] fix(apply): reject malformed JSONC instead of merging a half-parsed tree Address CodeRabbit review on PR #191: getJsoncTopLevelKey and parseJsoncObject passed an empty errors array to jsonc-parser, which is fault-tolerant and returns a partially-recovered object on malformed input. A corrupt local config could then become a bad merge base on copy. Now check errors.length === 0 (matching setJsoncTopLevelKey) and return undefined on any parse error, so a malformed local file is treated as "no value". Adds regression tests. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_011L7s33UCjpQkreXW7amAw2 --- src/agents/__tests__/_utils.test.ts | 22 +++++++++++++++++++++- src/agents/_utils.ts | 16 ++++++++++++---- 2 files changed, 33 insertions(+), 5 deletions(-) diff --git a/src/agents/__tests__/_utils.test.ts b/src/agents/__tests__/_utils.test.ts index a09e8e9..71cc170 100644 --- a/src/agents/__tests__/_utils.test.ts +++ b/src/agents/__tests__/_utils.test.ts @@ -2,7 +2,14 @@ import { afterEach, beforeEach, describe, expect, test } from "bun:test"; import { rm, writeFile } from "node:fs/promises"; import { join } from "node:path"; import { createTmpDir } from "../../test-helpers/fixtures"; -import { atomicWrite, collect, readIfExists, setJsoncTopLevelKey } from "../_utils"; +import { + atomicWrite, + collect, + getJsoncTopLevelKey, + parseJsoncObject, + readIfExists, + setJsoncTopLevelKey, +} from "../_utils"; // _utils helpers @@ -143,4 +150,17 @@ describe("agents/_utils", () => { const out = setJsoncTopLevelKey("{not valid json", "rules", "x"); expect(JSON.parse(out)).toEqual({ rules: "x" }); }); + + test("parseJsoncObject reads a valid object but rejects malformed input", () => { + expect(parseJsoncObject('{"a": 1, /* c */ "b": 2,}')).toEqual({ a: 1, b: 2 }); + // jsonc-parser would partially recover this; we reject it so a corrupt local + // file is never used as a merge base. + expect(parseJsoncObject("{not valid json")).toBeUndefined(); + expect(parseJsoncObject("[1,2,3]")).toBeUndefined(); // non-object root + }); + + test("getJsoncTopLevelKey reads a key but rejects malformed input", () => { + expect(getJsoncTopLevelKey('{"mcpServers": {"x": 1}}', "mcpServers")).toEqual({ x: 1 }); + expect(getJsoncTopLevelKey("{not valid json", "mcpServers")).toBeUndefined(); + }); }); diff --git a/src/agents/_utils.ts b/src/agents/_utils.ts index 8bf19fd..8315b31 100644 --- a/src/agents/_utils.ts +++ b/src/agents/_utils.ts @@ -109,8 +109,13 @@ export function setJsoncTopLevelKey(raw: string, key: string, value: unknown): s * same as a missing file, so a best-effort parse is the right contract here. */ export function getJsoncTopLevelKey(raw: string, key: string): unknown { - const parsed = parse(raw, [], { allowTrailingComma: true }); - return parsed && typeof parsed === "object" && !Array.isArray(parsed) + // jsonc-parser is fault-tolerant: it returns a partially-recovered object on + // malformed input. Reject anything with parse errors (matching + // setJsoncTopLevelKey) so a corrupt local file is treated as "no value" + // rather than merged from a half-parsed tree. + const errors: ParseError[] = []; + const parsed = parse(raw, errors, { allowTrailingComma: true }); + return errors.length === 0 && parsed && typeof parsed === "object" && !Array.isArray(parsed) ? (parsed as Record)[key] : undefined; } @@ -121,8 +126,11 @@ export function getJsoncTopLevelKey(raw: string, key: string): unknown { * root — callers treat that as "no local value to merge against". */ export function parseJsoncObject(raw: string): Record | undefined { - const parsed = parse(raw, [], { allowTrailingComma: true }); - return parsed && typeof parsed === "object" && !Array.isArray(parsed) + // Reject on parse errors (see getJsoncTopLevelKey): a half-parsed object must + // not become a merge base, or apply could drop or corrupt local config. + const errors: ParseError[] = []; + const parsed = parse(raw, errors, { allowTrailingComma: true }); + return errors.length === 0 && parsed && typeof parsed === "object" && !Array.isArray(parsed) ? (parsed as Record) : undefined; }