Skip to content

fix(core): resolve 6 deferred security/correctness findings#12

Merged
c-1k merged 1 commit into
masterfrom
ship/deferred-findings
Mar 31, 2026
Merged

fix(core): resolve 6 deferred security/correctness findings#12
c-1k merged 1 commit into
masterfrom
ship/deferred-findings

Conversation

@c-1k

@c-1k c-1k commented Mar 31, 2026

Copy link
Copy Markdown
Contributor

Summary

Fixes all 6 deferred findings from the code review and security review of #10 (governAction). These are pre-existing issues in the interceptCall() LLM path that apply to both old and new governance paths.

Critical

  • DEFAULT_RULES dead codeDEFAULT_RULES (block-zero-budget, block-budget-exhausted, warn-high-cost) now used as fallback when no policy file exists OR when the file is empty/corrupt. Previously, policyRules defaulted to [], meaning zero budget enforcement in dry-run mode.

High

  • PII in warn mode logged verbatim — Audit writes now redact PII in warn/redact mode before writing to the append-only chain. Applied to success paths, failure paths (error strings that may echo request content), and action params.
  • Secret pattern detection — PII scanner extended with: API keys (OpenAI sk-, Anthropic sk-ant-, Stripe sk_live_/sk_test_), AWS keys (AKIA), GitHub tokens (ghp_/gho_/ghs_/ghr_/github_pat_), Bearer tokens, JWTs, PEM private keys, npm tokens, and generic secrets (Slack xox, SendGrid SG., GitLab glpat-, long hex).
  • TOCTOU budget commit gapinFlightHoldTotal and budgetSpent mutations now re-acquire the async mutex, preventing transient budget miscalculation under concurrent calls. Applied to all 5 budget mutation sites (streaming success, streaming error, non-streaming success, non-streaming failure, governActionImpl).

Medium

  • Proxy settlement failure not audited — Proxy settle catch blocks now emit settlement_ambiguous audit events (matching the TigerBeetle path). Applied to both interceptCall and governActionImpl.
  • Synthetic auditHash misleading — Degraded audit receipts now return "AUDIT_DEGRADED" sentinel instead of a synthetic SHA-256 hash. Applied to all 4 receipt construction sites including the streaming estimated receipt.

Also includes (from internal review remediation)

  • redactPII() utility for deep-cloning data with PII replaced by [REDACTED:<type>] placeholders
  • Error strings in llm_call_failed and stream_partial_delivery audit events now redacted when PII mode is active
  • isGenericSecret no longer double-fires with isApiKey on sk- prefixed keys

Test plan

  • npx vitest run — 1146/1155 pass (9 pre-existing @clack/prompts CLI failures)
  • Verify DEFAULT_RULES fallback: trust client with budget=1 and no policy file denies second call
  • Verify PII redaction: audit events contain [REDACTED:email] not raw emails in warn mode
  • Verify secret detection: sk-, AKIA, ghp_, Bearer, JWT, PEM all detected
  • Verify proxy settle audit: settlement_ambiguous event emitted when proxy settle fails
  • Verify AUDIT_DEGRADED sentinel: receipt.auditHash === "AUDIT_DEGRADED" when audit writer throws
  • Verify no double-fire: sk- key tagged as api_key only, not generic_secret

Files changed (6 files, +785/-54)

File Changes
packages/core/src/govern.ts DEFAULT_RULES fallback, TOCTOU mutex, proxy audit events, AUDIT_DEGRADED sentinel, PII redaction in audit writes
packages/core/src/policy/pii.ts 7 new secret patterns, redactPII() function, JWT/PEM/npm detection
packages/core/tests/govern/govern.test.ts DEFAULT_RULES + AUDIT_DEGRADED tests
packages/core/tests/govern/failure-modes.test.ts Proxy settlement audit test
packages/core/tests/govern/action-governance.test.ts PII redaction in audit tests (warn, redact, off modes)
packages/core/tests/policy/pii.test.ts 28 new tests for secret patterns, redactPII, Stripe, JWT, PEM, npm, no-double-fire

🤖 Generated with Claude Code

Critical: DEFAULT_RULES now used as fallback when no policy file exists
(or policy file is empty/corrupt), ensuring budget enforcement in all
modes including dry-run.

High: PII detection extended with secret patterns (API keys incl. Stripe
sk_, AWS AKIA, GitHub tokens, Bearer tokens, JWTs, PEM private keys,
npm tokens, generic secrets). Audit writes now redact PII in warn/redact
mode — both success and failure paths, including error strings that may
echo request content.

High: TOCTOU budget commit gap fixed — inFlightHoldTotal and budgetSpent
mutations now re-acquire the async mutex, preventing transient budget
miscalculation under concurrent calls.

Medium: Proxy settlement failures now emit settlement_ambiguous audit
events (matching the TigerBeetle path). Degraded audit receipts return
"AUDIT_DEGRADED" sentinel instead of a misleading synthetic hash.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@c-1k

c-1k commented Mar 31, 2026

Copy link
Copy Markdown
Contributor Author

Codex Review (gpt-5.3-codex)

No issues found. PR is clean.

@c-1k

c-1k commented Mar 31, 2026

Copy link
Copy Markdown
Contributor Author

Codex Remediation Summary

# Finding Severity Classification Action Commit
No issues found

Deliberations: 0 of 2 used
Result: Codex review clean — no findings to triage

@c-1k c-1k marked this pull request as ready for review March 31, 2026 02:13
@c-1k

c-1k commented Mar 31, 2026

Copy link
Copy Markdown
Contributor Author

CI Results (Final)

Step Status Duration
Typecheck ✅ pass 19s
Lint ✅ pass 16s
Tests + Coverage ✅ pass 27s

Commit: 939505f

@c-1k

c-1k commented Mar 31, 2026

Copy link
Copy Markdown
Contributor Author

Ship Pipeline Complete

Branch: ship/deferred-findings
Pipeline stages: 0-15 ✅

Audit Trail

  • Internal code review: 4 findings, 4 fixed (CR-1: streaming AUDIT_DEGRADED, CR-3: empty policy file fallback, CR-4: isGenericSecret overlap, SR-1: error string PII leak)
  • Internal security review: 8 findings — 5 fixed, 3 deferred (pre-existing: DLQ HMAC key, ReDoS guard, verify chain hash advancement)
  • Codex findings: 0 found — clean review
  • Deliberations: 0 of 2 used
  • CI: ✅ pass (typecheck, lint, tests)

Deferred from internal reviews (pre-existing, not in scope)

  • DLQ HMAC key derived from predictable path (pre-existing in chain.ts/engine.ts)
  • NESTED_QUANTIFIER_RE misses alternation-based ReDoS (pre-existing in gate.ts)
  • verifyChain advances expectedPreviousHash using stored hash on mismatch (pre-existing in verify.ts)

Summary

  • 6 deferred security/correctness findings resolved in a single PR
  • 28 new tests added across 4 test files
  • PII scanner extended from 5 to 12 pattern types
  • Budget enforcement now guaranteed in all modes (dry-run, proxy, production)

@c-1k c-1k merged commit e09da58 into master Mar 31, 2026
6 checks passed
@c-1k c-1k deleted the ship/deferred-findings branch March 31, 2026 02:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant