fix(security): always block the age key and PEM private keys even when secretScan=off

[chrisleekr]

Summary

Fixes the first half of #181: secretScan = "off" previously disabled all push-time secret detection — including the vault's own age secret key and PEM private keys. Committing the age key (even encrypted to recipients) hands every future reader the means to decrypt the entire vault, retroactively and irreversibly. That can never be safe, regardless of mode.

This adds an always-block catastrophic tier that fires in every mode:

• ALWAYS_BLOCK_PATTERNS (age-secret-key, private-key-pem) is derived from EMBEDDED_SECRET_PATTERNS by name, so each regex body stays defined once.
• scanForSecrets no longer early-returns [] for off — off now scans exactly the catastrophic tier, waiving only the ordinary API-token patterns.
• standard / strict selection is byte-for-byte unchanged.

Security hardening from review

A senior-review pass caught that the catastrophic tier was still bypassable via allowSecretValues: the allow-list exemption was applied to every pattern, and config set deliberately skips its own leak-scan for that key. So config set security.allowSecretValues '["AGE-SECRET-KEY-1…"]' would have landed the master key in plaintext agentsync.toml and silenced it at push. Closed two ways:

• scanForSecrets: catastrophic hits are never exemptible (catastrophic || !allow.includes(…)).
• config set: the allowSecretValues value is itself scanned for the catastrophic tier (off-mode scan) before being stored.

Tests

• catastrophic tier (age key + PEM) blocks in standard / strict / off
• allowSecretValues can not exempt a catastrophic value
• ordinary tokens still waived under off; ALWAYS_BLOCK_PATTERNS invariant
• config set security.allowSecretValues rejects an age key

Full suite: 980 pass / 0 fail.

Scope

First of two PRs for #181. This is the independent security-footgun fix. The redact mode (redact API keys and push, instead of blocking) plus placeholder-aware copy and the TUI surfacing follow in a second PR.

🤖 Generated with Claude Code

https://claude.ai/code/session_011L7s33UCjpQkreXW7amAw2

Summary by CodeRabbit

• Bug Fixes & Security

  • Enhanced secret scanning: catastrophic-tier secrets (vault age keys and PEM private keys) are now always blocked, regardless of the security.secretScan setting.

• Documentation

  • Clarified security.secretScan behavior: the "off" mode explicitly waives ordinary API-token patterns while maintaining protection against catastrophic-tier secrets in all modes.

[coderabbitai]

Warning

Review limit reached

@chrisleekr, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 40 minutes and 14 seconds. Learn how PR review limits work.

Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file).

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits.

🚦 How do rate limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: d4a4e66f-5f71-42d2-b85c-6e56c9681239

📥 Commits

Reviewing files that changed from the base of the PR and between e474501 and f0f9f16.

📒 Files selected for processing (4)

• docs/commands.md
• src/commands/__tests__/config.test.ts
• src/commands/config.ts
• src/core/__tests__/sanitizer.test.ts

📝 Walkthrough

Walkthrough

Introduces a "catastrophic tier" (age secret key + PEM private key) that scanForSecrets blocks across all SecretPolicy modes including off. Exports ALWAYS_BLOCK_PATTERNS from sanitizer.ts, prevents allow-list exemption for those patterns, and applies the same enforcement to security.allowSecretValues in performConfigSet. Updates schema comments, skills-walker comments, and user-facing documentation.

Changes

Catastrophic-tier secret enforcement

Layer / File(s)	Summary
ALWAYS_BLOCK_PATTERNS constant and scanForSecrets enforcement src/core/sanitizer.ts, src/core/__tests__/sanitizer.test.ts	Exports ALWAYS_BLOCK_PATTERNS (age-secret-key + private-key-pem) filtered from EMBEDDED_SECRET_PATTERNS. Removes the early-return for mode: "off" in scanForSecrets, replacing it with catastrophic-only pattern selection. Narrows allow-list exemption so catastrophic matches cannot be suppressed. Tests assert exact pattern membership, detection across standard/strict/off modes, and allow-list non-exemptibility for catastrophic patterns.
performConfigSet: catastrophic blocking for allowSecretValues src/commands/config.ts, src/commands/__tests__/config.test.ts	performConfigSet no longer skips scanForSecrets for SECRET_EXEMPT_KEY; it now calls it with mode: "off" and allow: [] so only catastrophic patterns are checked, returning invalid-value when any are found. New test confirms an AGE-SECRET-KEY-1-shaped value is rejected and the persisted allowlist remains empty.
Schema comments, skills-walker comments, and documentation src/config/schema.ts, src/agents/skills-walker.ts, docs/commands.md, docs/operations.md	Updates security.secretScan inline schema comments to describe the catastrophic-tier caveat for off. Adds comments in collectInteriorViolations stating bundle interiors are always scanned at standard. Revises docs/commands.md and docs/operations.md to document that off waives only ordinary API-token patterns while the catastrophic tier still blocks in every mode.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

• chrisleekr/agentsync#65: Touches the same collectInteriorViolations/scanForSecrets area in src/agents/skills-walker.ts for interior bundle scanning behavior.
• chrisleekr/agentsync#185: Introduces the config command and security schema surface that this PR modifies in performConfigSet and security.secretScan.

Suggested labels

bot:resolve

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately and concisely summarizes the main security fix: blocking the age key and PEM private keys in all modes, including when secretScan=off.
Description check	✅ Passed	The description provides comprehensive coverage of Summary, Changes, Test Evidence, and Risks sections, with clear explanations of the security issue, implementation approach, and test results.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 2

🤖 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 `@docs/commands.md`:
- Around line 379-380: The documentation for the `security.allowSecretValues`
configuration option on line 380 currently describes it as a blanket
literal-value exemption from secret detection, which is misleading given that
line 379 explicitly states catastrophic tier secrets (age keys, PEM private
keys) always block. Clarify the `security.allowSecretValues` description to
explicitly scope the exemption to ordinary-token detection patterns only, and
make it clear that catastrophic tier secrets cannot be allow-listed regardless
of this setting. This ensures the documentation accurately reflects that only
standard credential patterns are eligible for exemption through this option.

In `@src/commands/config.ts`:
- Around line 173-176: The scanForSecrets function is validating the rawValue
before parsing, but the actual persisted value is parseScalar(rawValue),
creating a gap where encoded secrets could bypass validation. Change both calls
to scanForSecrets (the one with the SECRET_EXEMPT_KEY condition and the else
case) to pass parseScalar(rawValue) instead of rawValue, ensuring the secret
scanning occurs on the decoded value that will actually be stored rather than
the raw encoded form.

🪄 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: ASSERTIVE

Plan: Pro

Run ID: 48f63da6-317b-4abe-915b-fdb6d2d7d634

📥 Commits

Reviewing files that changed from the base of the PR and between 1a07530 and e474501.

📒 Files selected for processing (8)

• docs/commands.md
• docs/operations.md
• src/agents/skills-walker.ts
• src/commands/__tests__/config.test.ts
• src/commands/config.ts
• src/config/schema.ts
• src/core/__tests__/sanitizer.test.ts
• src/core/sanitizer.ts