Skip to content

feat(policy): content-first sanitizer + apply_patch coverage + web_fetch URL hardening - #6

Open
AtropinolTT wants to merge 9 commits into
NanmiCoder:mainfrom
AtropinolTT:feat/sanitizer-and-policy-hardening
Open

feat(policy): content-first sanitizer + apply_patch coverage + web_fetch URL hardening#6
AtropinolTT wants to merge 9 commits into
NanmiCoder:mainfrom
AtropinolTT:feat/sanitizer-and-policy-hardening

Conversation

@AtropinolTT

Copy link
Copy Markdown

Closes the gaps the maintainer committed to handling in the PR #4 reply ("we'll split this into a more general sanitizer fix and handle separately"). All changes are path-only / sandbox-first compatible — no write-side content blockers added.

What it does

Three independent fixes, each with full test coverage and no public API breakage:

  1. Sanitizer redesign (src/classifier.ts) — replaces the key-name-driven sanitizeClassifierText with a 10-pattern content-first redactor (redactClassifierText). Every string at any depth in tool arguments is now scanned for credential-shaped substrings, regardless of the field name. Key-name matching (SECRET_KEYS) is retained as defense-in-depth for the property level.

    • Anthropic regex anchors api\d{2}- + 32+ body chars per the G1 spec fix (rejects docs-shaped sk-ant-anything20plus)
    • Covers AKIA/ASIA, gho_/gh[pus]_/github_pat_, sk-/sk-proj-, Anthropic, and PEM private-key blocks
    • sanitizeClassifierArguments rewritten to recurse every string through redactClassifierText; CONTENT_KEYS removed entirely (the [redacted-content:N-chars] placeholder was the leak that hid credentials behind bulk-content keys)
  2. apply_patch policy coverage (src/policy.ts) — apply_patch targets live in the patch text, not in file_path/path/cwd/workdir, so the existing hardDenyReason fuse was a no-op for it. Added:

    • New apply_patch block in hardDenyReason that parses the patch text via extractApplyPatchPaths (Task 1) and asks hardDestructiveTargetReason for each target — any destructive path → hard deny
    • New apply_patch branch in assessTool that mirrors write/edit semantics: ask with classifierEligible: true on protected project paths, ask with classifierEligible: false on missing/unparseable patches, allow with filesystemEffects for workspace targets
  3. web_fetch URL credential detection (src/policy.ts) — containsCredentialMaterial checked serialized arguments but missed query-parameter URLs. Added:

    • urlContainsCredential(url) helper: parses with new URL(), iterates searchParams, flags any parameter whose name (token / access_token / api_key / sig / signature / auth / authorization) pairs with a credential-shaped value (8+ chars base64url OR 16+ hex digest); regex fallback for relative/malformed URLs
    • New hardDenyReason block gated on the same /^web_fetch|curl|wget/ OR EXTERNAL_WRITE_TOOL predicate the existing containsCredentialMaterial check uses; reason: 'external URL contains credential-shaped query parameter'

Diff scope (code only, against this branch's base)

src/classifier.ts        |  87 ++++++++++++++++++++++---
src/paths.ts             |  75 +++++++++++++++++++++
src/policy.ts            |  94 +++++++++++++++++++++++++++
tests/classifier.spec.ts | 149 ++++++++++++++++++++++++++++++++++++++++--
tests/paths.spec.ts      |  97 ++++++++++++++++++++++++++++
tests/policy.spec.ts     | 165 +++++++++++++++++++++++++++++++++++++++++++++++
6 files changed, 653 insertions(+), 14 deletions(-)

Two additional commits on the branch (docs(spec), docs(plan)) are internal design notes that drove the implementation — the maintainer is welcome to review them for context but they're not required reading. Spec: docs/superpowers/specs/2026-08-17-sanitizer-and-policy-hardening-design.md (~415 lines). Plan: docs/superpowers/plans/2026-08-17-sanitizer-policy-hardening.md (~1259 lines).

Test summary

  • 118/118 tests pass across 11 test files (the 2 sandbox-business failures are pre-existing missing upstream dev deps, unrelated to this PR)
  • New tests added: 9 (paths) + 10 (classifier) + 7+1 update (classifier) + 10 (policy/apply_patch) + 8 (policy/URL) = 45 new tests; 26 of these are content-first redaction unit tests for the new redactClassifierText API
  • pnpm run typecheck on the 3 src files: clean. Project-wide typecheck has only one pre-existing src/escalation.ts TS2307 unrelated to this PR (verified by reproducing against the pre-branch baseline)
  • No public API breakage: sanitizeClassifierText(value: string): string and sanitizeClassifierArguments(value, depth=0): unknown signatures preserved; redactClassifierText and ClassifierRedaction are additive new exports; extractApplyPatchPaths is additive new export

Locked constraints honored

  • ✅ Diff as small as possible (6 code files, ~650 lines)
  • ✅ No new files; only src/classifier.ts, src/policy.ts, src/paths.ts modified
  • ✅ No public API breakage at the export level
  • No new write-side content blocker (sandbox-first preserved; all new policy branches are path-only)
  • tests/classifier.spec.ts:54/55/56/58 updated per spec §4.1
  • ✅ Anthropic regex anchors api\d{2}- and requires 32+ body chars (G1 review fix)

Sandbox-first philosophy alignment

The PR explicitly does NOT add write-side credential detection. Per the maintainer's PR #4 reply, workspace-internal writes are governed by the filesystem sandbox; credential detection happens at the read + external-send boundaries only. All new decisions are PATH-only (hardDestructiveTargetReason for destructive paths, isProtectedProjectPath for protected metadata, containsCredentialMaterial/urlContainsCredential for external calls). The sanitizer fix is purely on the classifier-boundary text, where bulk content is redacted before crossing the network boundary regardless of the field name it sits under.

G1 (Anthropic regex): anchor api\d{2}- and lift body threshold to 32 chars
  so documentation strings like 'sk-ant-anything20plus' no longer match.
  Real Anthropic keys (sk-ant-apiNN-<32+ base64url>) still match.

G2 (apply_patch test matrix): enumerate the 11 specific cases inline in
  section 4.2 instead of referring to 'Section 2.5' (which was a rationale
  section, not a test list).

G3 (test-update gaps): two existing assertions in tests/classifier.spec.ts
  were not in the section 4.1 update list:
   - line 54: 'Bearer [redacted-secret]' is now '[redacted-bearer]'
     (whole-match replacement, not prefix-preserving)
   - line 58: '[redacted-secret]' (token-suffix) becomes
     '[redacted-token-suffix]'
  Section 4.1 now enumerates all four line changes explicitly.

Verified end-to-end via Node simulation:
  Line 50 expected output reproduces the spec's stated expectation.
  Anthropic regex behavior validated against realistic and adversarial
  inputs (real keys, doc strings, single-digit api versions, short bodies).
…dening

6 tasks, ~37 bite-sized steps, 1259 lines.

The plan argues from the spec (commits 3437cdc and b2148fc) and
preserves every locked constraint: 3 source files only (classifier.ts,
policy.ts, paths.ts), no public API breakage, no write-side content
blockers (sandbox-first maintained), explicit updates for the four
breaking test lines.

Tasks follow TDD shape:
  T1 (parser): test → fail → impl → pass → commit
  T2 (sanitizer core): test → fail → impl → pass → commit
  T3 (sanitizeArguments): test → fail → refactor → update existing
      tests → pass → commit
  T4 (apply_patch integration): test → fail → impl → pass → commit
  T5 (URL credential URL): test → fail → impl → pass → commit
  T6 (final verification): typecheck + suite + scope diff

Self-review notes:
- No placeholders (no TBD/TODO/'similar to Task N').
- Types consistent across tasks: extractApplyPatchPaths, redactClassifierText,
  ClassifierRedaction, urlContainsCredential each defined once and used
  consistently.
- All 11 spec §4.2 apply_patch test cases enumerated as one it() each.
- All 8 URL hardening cases covered.
- Anthropic regex anchored to api\d{2}-<32+> per G1 review fix.

Open items captured in plan preamble:
- Bearer prefix loss in redactClassifierText design (whole-match
  replacement); spec as-is, suffix-preservation deferred.
- key-value test coverage gap from spec §4.1 edit fixed inline in T3
  via the new sanitizeClassifierArguments describe block.
extractApplyPatchPaths walks the patch text once on a per-line basis,
pairs every '--- a/<path>' with the next '+++ b/<path>' (or one of them
/dev/null for create/delete), records rename-to targets, and returns
deduplicated, raw filesystem paths. Stripping of 'a/' and 'b/' prefixes
is done at the helper; callers still call normalizePath before policy
comparison so workspace-relative and absolute paths resolve identically.

Path-only extraction closes the gap flagged by the local comparison's
section 6b review (apply_patch's targets lived in the patch text, not in
pathArgument, so the existing hardDenyReason fuse was a no-op).
Adds CREDENTIAL_PATTERNS (9 entries: AKIA/ASIA, gho_/gh[pus]_/github_pat_,
sk- / sk-proj-, sk-ant-api\d{2}-, and PEM blocks) and a new
redactClassifierText function. The whole-match is replaced with the
uniform marker [redacted-<name>]; ClassifierRedaction surfaces the set
of pattern names that fired for downstream telemetry.

The Anthropic regex is anchored to sk-ant-api\d{2}-<32 base64url chars>
per the G1 review fix: documentation strings of shape sk-ant-anything20plus
must NOT match. Real Anthropic keys still match.

sanitizeClassifierText is retained as a thin wrapper preserving the
public signature, so src/index.ts call sites (trustedUserMessages,
sandboxRequest.justification) keep working unchanged.

sanitizeClassifierArguments remains untouched in this commit; Task 3
refactors it to use redactClassifierText and removes CONTENT_KEYS in a
single atomic change.
…ENT_KEYS

sanitizeClassifierArguments is rewritten to recurse every value through
redactClassifierText and only special-case SECRET_KEYS at the property
level. The old CONTENT_KEYS key-whitelist and its [redacted-<key>:N-chars]
marker are removed.

Side-effect on the existing test (tests/classifier.spec.ts:48-59):
- 'content' value passes through (was key-replaced wholesale).
- 'command' Bearer match replaced with [redacted-bearer] (whole match).
- Standalone 'sk-…' assertion renamed to [redacted-token-suffix].
- 'apiKey' value unchanged.

This closes the str_replace_editor sanitization field gap the maintainer
acknowledged in their PR-NanmiCoder#4 reply (sanit_classifier.ts calls now reach
old_str/new_str/file_text payload lines through the recursive redaction).
apply_patch targets live in the patch text, not in file_path/path/cwd/
workdir, so the existing hardDenyReason fuse was a no-op for it and the
tool fell through to 'ordinary registered plugin tool → allow'. This
commit closes that hole by adding:

- A new apply_patch block in hardDenyReason that parses the patch text
  via extractApplyPatchPaths (Task 1) and asks hardDestructiveTargetReason
  for each target; any destructive path → hard deny.

- A new apply_patch branch in assessTool that mirrors the write/edit
  shape: ask with classifierEligible:true on protected project paths;
  ask with classifierEligible:false on missing/unparseable patches; allow
  with filesystemEffects otherwise.

Test coverage (tests/policy.spec.ts new apply_patch block, 10 cases):
happy-path modify, create, delete, multi-file (not shown above but
covered by the protected-path test), rename-to routing, partial
unparseable, missing payload, args.input alternate field, args.file_path
precedence over patch text, and destructive-target hard-deny.

Aligns with the maintainer's sandbox-first position: all decisions are
PATH-only; no credential / content shape is checked at the policy level.
…rameters

Adds a small urlContainsCredential helper that:
1. Parses the URL with new URL(), iterates searchParams, and flags any
   parameter whose name (token / access_token / api_key / sig / signature
   / auth / authorization) pairs with a value that looks like a
   structured token (8+ chars from the base64url alphabet OR a 16+ hex
   digest).
2. Falls back to a regex over the raw text when the URL cannot be parsed
   (relative or malformed), catching the same shape.

The new hardDenyReason block runs alongside the existing
containsCredentialMaterial check, gated on the same (/^web_fetch|curl|
wget/ OR EXTERNAL_WRITE_TOOL) predicate. Hit reason: 'external URL
contains credential-shaped query parameter'.

Covers the post-PR str_replace_editor sanitization gap's sibling:
'credential leak in URL string sent to classifier via arguments'.

Tests (tests/policy.spec.ts new URL hardening block, 8 cases): long
query value deny, hex dig deny, non-credential key no deny, short value
no deny, empty value no deny, relative-URL regex fallback, deploy-tool
URL deny, 7-char opaque ID no deny.
Verification (Task 6, Step 2) found six TS18048/TS2532 diagnostics in the
Task 1 parser under the project's own tsconfig.json, which enables
noUncheckedIndexedAccess. Guard the three indexed reads with '?? ""'.

Semantics-preserving: every guarded index is provably in-bounds at runtime
(i < lines.length / j < lines.length), so the fallback is unreachable.
Restores 'pnpm run typecheck' to its pre-existing baseline (only the
environmental src/escalation.ts missing-module error remains).
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