From 3437cdcaac86f5e938d7a92f3a223b14cbd5f303 Mon Sep 17 00:00:00 2001 From: AtropinolTT Date: Mon, 17 Aug 2026 15:17:36 +0800 Subject: [PATCH 1/9] docs(spec): general sanitizer + apply_patch + web_fetch URL hardening --- ...7-sanitizer-and-policy-hardening-design.md | 415 ++++++++++++++++++ 1 file changed, 415 insertions(+) create mode 100644 docs/superpowers/specs/2026-08-17-sanitizer-and-policy-hardening-design.md diff --git a/docs/superpowers/specs/2026-08-17-sanitizer-and-policy-hardening-design.md b/docs/superpowers/specs/2026-08-17-sanitizer-and-policy-hardening-design.md new file mode 100644 index 0000000..3848777 --- /dev/null +++ b/docs/superpowers/specs/2026-08-17-sanitizer-and-policy-hardening-design.md @@ -0,0 +1,415 @@ +# Spec: Sanitizer Redesign + `apply_patch` Coverage + `web_fetch` URL Hardening + +- **Date**: 2026-08-17 +- **Local reference**: `feat/credential-write-guard-v3` later; this spec governs sandbox-first hardening on the upstream `nanmicoder/dsh-auto-mode` HEAD at the time of writing (`007c316`). +- **Upstream context**: PR #4 was declined on `str_replace_editor` field sanitization. The maintainer committed to a "more general sanitizer" fix and acknowledged the real gap. This spec implements that sanitizer redesign plus two adjacent items the maintainer is structurally positioned to accept. +- **Constraints (locked with user)**: + - Diff as small as possible. + - No new files; all source changes stay in `src/classifier.ts`, `src/policy.ts`, `src/paths.ts`. + - No public API breakage at the export level (function signatures). Internal redaction marker strings may change. + - No new write-side content blocker (must not conflict with sandbox-first philosophy). + +--- + +## Goals + +1. **G1 — Sanitizer is content-first.** Every string value that crosses the classifier network boundary is sanitized by **pattern matching against known credential formats**, not by key-name whitelist. Fields whose key matches `SECRET_KEYS` keep a wholesale `[redacted-secret-field]` replacement as a defense-in-depth layer. +2. **G2 — Stricter credential pattern coverage.** The sanitizer catches AWS access keys (`AKIA|ASIA`), GitHub classic/user/server/OAuth/fine-grained PAT, Anthropic and OpenAI-style `sk-`/`sk-proj-`/`sk-ant-` keys, and PEM-encoded private-key blocks in addition to the existing inline patterns. +3. **G3 — `apply_patch` is fully policed.** Every file path that appears in an `apply_patch` payload (including `--- a/path`, `+++ b/path`, `/dev/null` new files, and multi-file patches) is resolved and routed through `hardDenyReason` and `assessTool`. A patch that targets a destructive or protected path is hard-denied or asked exactly like a `write`/`edit` would be. Unparseable patch text fails closed. +4. **G4 — `web_fetch` URL hardening.** The literal query string of a `web_fetch`/`curl`/`wget` URL is structurally inspected for credential-shaped parameter names (`token`, `access_token`, `api_key`, `sig`, `signature`, `auth`, ...) and credential-shaped values. Hits land in `hardDenyReason`. + +--- + +## Non-Goals + +- Adding write-side content-based blockers (e.g., blocking a `write` whose content contains `AKIA`). This was the original PR-A and the maintainer declined it. We do not introduce an `ask` path conditioned on file content. +- Closing symlink-escape (Critical-2 from the local review). This is a sandbox-policy item whose design conflicts with the sandbox-first direction and is left as future work in `9-investigation-backlog.md`. +- Hardening `web_fetch` for non-credential URL content (e.g., arbitrary PII). Out of scope. +- Replacing `CONTENT_KEYS` wholesale with field-aware granularity of unknown depth; only credential-redaction concerns belong to this spec. + +--- + +## Section 1 — Sanitizer architecture (G1+G2) + +### 1.1 New top-level constant + +```ts +interface CredentialPattern { + readonly name: string + readonly pattern: RegExp +} + +const CREDENTIAL_PATTERNS: readonly CredentialPattern[] = [ + // Inline value-shape patterns kept from the existing sanitizer. + { name: 'token-suffix', pattern: /\b(?:sk|ghp|github_pat|xox[baprs])[-_][A-Za-z0-9_-]{8,}\b/g }, + { name: 'bearer', pattern: /\bBearer\s+[A-Za-z0-9._~+\/-]{8,}/gi }, + { name: 'key-value', pattern: /((?:api[_-]?key|token|secret|password)=)[^&\s]+/gi }, + + // AWS access-key IDs (16 chars after the prefix). + { name: 'aws-access-key', pattern: /\b(?:AKIA|ASIA)[A-Z0-9]{16}\b/g }, + + // GitHub token formats (gho_ is 40; gh[pus]_ is 36; github_pat_ has 22+). + { name: 'github-oauth', pattern: /\bgho_[A-Za-z0-9]{40}\b/g }, + { name: 'github-classic', pattern: /\bgh[pus]_[A-Za-z0-9]{36}\b/g }, + { name: 'github-fine-pat', pattern: /\bgithub_pat_[A-Za-z0-9_]{22,}\b/g }, + + // LLM and tool vendor keys. + { name: 'llm-key', pattern: /\bsk-(?:proj-)?[A-Za-z0-9_-]{16,}\b/g }, + { name: 'anthropic-key', pattern: /\bsk-ant-[A-Za-z0-9_-]{20,}\b/g }, + + // PEM private-key blocks (RSA, OPENSSH, EC, ECDSA, DSA, ENCRYPTED). + // Matches the BEGIN line through the next matching END line, inclusive. + { name: 'pem-private-key', + pattern: /-----BEGIN (?:RSA |OPENSSH |EC |ECDSA |DSA |ENCRYPTED )?PRIVATE KEY-----[\s\S]*?-----END (?:RSA |OPENSSH |EC |ECDSA |DSA |ENCRYPTED )?PRIVATE KEY-----/g }, +] +``` + +### 1.2 New function `redactClassifierText` + +```ts +export interface ClassifierRedaction { + readonly value: string + readonly redactedNames: readonly string[] +} + +export function redactClassifierText(value: string, maxLength = 1_000): ClassifierRedaction { + let current = value + const redactedNames: string[] = [] + for (const { name, pattern } of CREDENTIAL_PATTERNS) { + const before = current + current = current.replace(pattern, `[redacted-${name}]`) + if (current !== before && !redactedNames.includes(name)) redactedNames.push(name) + } + if (current.length > maxLength) current = current.slice(0, maxLength) + return { value: current, redactedNames } +} +``` + +### 1.3 Redaction markers + +- A match is replaced with the literal string `[redacted-]`, where `` is the constant from `CREDENTIAL_PATTERNS`. Example: `AKIAIOSFODNN7EXAMPLE` becomes `[redacted-aws-access-key]`. +- A PEM block is replaced entirely by `[redacted-pem-private-key]`. The full matched range (BEGIN…END inclusive) is removed. +- Multiple distinct pattern types in the same input each produce a marker; repeated occurrences of the same type produce a single marker but no count is shown to the classifier. +- The 1000-character outer truncation is preserved (matches the existing behavior). + +### 1.4 Replacement for `sanitizeClassifierText` + +The existing `sanitizeClassifierText(value: string): string` is reimplemented as a thin wrapper over `redactClassifierText`: + +```ts +export function sanitizeClassifierText(value: string): string { + return redactClassifierText(value).value +} +``` + +**Why keep the wrapper**: the public symbol is called from `src/index.ts:172` (`trustedUserMessages`) and `src/index.ts:252` (`sandboxRequest.justification`). Replacing it inside the file eliminates any cross-file churn while changing the marker format. + +### 1.5 Replacement for `sanitizeClassifierArguments` + +```ts +export function sanitizeClassifierArguments(value: unknown, depth = 0): unknown { + if (depth > 3) return '[truncated-depth]' + if (typeof value === 'string') return redactClassifierText(value).value + if (typeof value === 'number' || typeof value === 'boolean' || value === null) return value + if (Array.isArray(value)) return value.slice(0, 25).map(item => sanitizeClassifierArguments(item, depth + 1)) + if (typeof value !== 'object') return `[${typeof value}]` + const output: Record = {} + for (const [key, entry] of Object.entries(value).slice(0, 50)) { + if (SECRET_KEYS.test(key)) { + output[key] = '[redacted-secret-field]' + } else { + output[key] = sanitizeClassifierArguments(entry, depth + 1) + } + } + return output +} +``` + +`CONTENT_KEYS` is removed entirely. The previous `output[key] = '[redacted-:N-chars]'` branch is gone. + +### 1.6 Public-API summary + +| Symbol | Status | +|---|---| +| `sanitizeClassifierText(value: string): string` | **Kept** (signature unchanged; output format changed) | +| `sanitizeClassifierArguments(value, depth=0)` | **Kept** (signature unchanged; output format changed) | +| `redactClassifierText(value, maxLength?): ClassifierRedaction` | **New export** | +| `ClassifierRedaction` | **New export** | +| `CREDENTIAL_PATTERNS` | **Module-private** (not exported; exported as a readonly view if needed) | + +Reason `redactClassifierText` is exposed: tests in `tests/classifier.spec.ts` exercise single-pattern hits without the recursion overhead. It also makes the new pattern set directly testable, which is a maintainability win. + +### 1.7 Trusted-user-message impact + +`src/index.ts:172` calls `sanitizeClassifierText(text).slice(0, remaining)`. New behavior: any credential token within a direct user message is marked `[redacted-]` before reaching the classifier. No code change beyond the implementation in §1.4. + +`src/index.ts:252` does the same for `escalation.justification`; same as above. + +--- + +## Section 2 — `apply_patch` policed end to end (G3) + +### 2.1 New helper in `src/paths.ts` + +```ts +export function extractApplyPatchPaths(patch: string): string[] { + // Walk the patch; for each "--- a/path" or "--- /dev/null" header followed by + // "+++ b/path", record the b-path. Strip the "a/" / "b/" prefix. Skip pure + // rename detection ("similarity index 100%" / "rename from"/"rename to") and + // accept the "rename to" target as a b-path. + // Returns the deduplicated list of b-paths in raw header form (filesystem + // shape, not normalized). Empty array when the patch cannot be reliably + // parsed. Callers are responsible for normalizePath before policy comparison. +} +``` + +Parsing rules: +- Lines beginning with `--- ` (3 dashes plus space) and `+++ ` (3 pluses plus space) are pair-matched by adjacent occurrence. +- `--- /dev/null` paired with `+++ b/` indicates new file creation; record ``. +- `+++ /dev/null` paired with `--- a/` indicates deletion; record ``. +- `rename from ` followed by `rename to ` records `` from the `to` side. +- Paths are stripped of the optional leading `a/` or `b/` directory prefix (git-style). +- Output: array of strings, deduplicated, in order of appearance, **with no path normalization at this layer**. `assessTool` and `hardDenyReason` each call `normalizePath` on every entry before comparison (so workspace-relative paths and absolute paths resolve the same way). + +### 2.2 `hardDenyReason` addition + +```ts +if (exec.name === 'apply_patch') { + const patch = typeof args?.patch === 'string' ? args.patch + : typeof args?.input === 'string' ? args.input + : undefined + if (patch !== undefined) { + const targets = extractApplyPatchPaths(patch) + for (const raw of targets) { + const normalized = normalizePath(raw, roots.workspace, roots.home) + const reason = hardDestructiveTargetReason(normalized, roots) + if (reason !== undefined) return `apply_patch targets ${reason}` + } + } +} +``` + +Inserted after the existing `['write', 'edit', 'apply_patch']` block in `hardDenyReason`. Note: this fails closed even when the patch text has zero resolvable paths — see §2.4. + +### 2.3 `assessTool` `apply_patch` branch + +```ts +if (exec.name === 'apply_patch') { + const patch = typeof args?.patch === 'string' ? args.patch + : typeof args?.input === 'string' ? args.input + : undefined + if (patch === undefined) { + return { decision: 'ask', reason: 'apply_patch payload is missing', classifierEligible: false } + } + const targets = extractApplyPatchPaths(patch) + if (targets.length === 0) { + return { + decision: 'ask', + reason: 'apply_patch text cannot be parsed for target paths; manual approval required', + classifierEligible: false, + } + } + for (const raw of targets) { + const normalized = normalizePath(raw, roots.workspace, roots.home) + if (isProtectedProjectPath(normalized, roots)) { + return { + decision: 'ask', + reason: `apply_patch targets protected project metadata: ${normalized}`, + classifierEligible: true, + filesystemEffects: targets.map(p => ({ + kind: 'create-or-overwrite' as const, + path: normalizePath(p, roots.workspace, roots.home), + existedBefore: existedBefore(normalizePath(p, roots.workspace, roots.home)), + })), + } + } + } + const effects: FilesystemEffect[] = targets.map(p => { + const n = normalizePath(p, roots.workspace, roots.home) + return { kind: 'create-or-overwrite', path: n, existedBefore: existedBefore(n) } + }) + return { + decision: 'allow', + reason: 'apply_patch inside workspace is delegated to the filesystem sandbox', + classifierEligible: false, + filesystemEffects: effects, + } +} +``` + +This mirrors the existing `write/edit` branch shape (decision reason, `filesystemEffects`, sandbox delegation). The `classify` later interaction with `classifier.classify` already passes `sanitizeClassifierArguments` over `args`, so the patch content is sanitized before going to the classifier — no extra work needed. + +### 2.4 Fail-closed on unparseable patch + +When `extractApplyPatchPaths` returns `[]` because the patch text is too malformed to extract any path, `assessTool` returns an `ask` with `classifierEligible: false`. The user is the gate. This avoids both false-allow and false-deny on adversarial inputs. + +`hardDenyReason` does not fail closed here — it only ever returns `undefined` if no hard reason applies. The `assessTool` ask handles the degraded case. + +### 2.5 `pathArgument` is not extended + +We do **not** add new fields to `pathArgument(args)` to detect apply_patch paths. The patch text is accessed directly via `args.patch` or `args.input`. This keeps the policy-engine contract clean and confines the new logic to one named helper. + +--- + +## Section 3 — `web_fetch` URL hardening (G4) + +### 3.1 New helper in `src/policy.ts` + +```ts +const URL_CREDENTIAL_KEYS = /(?:token|access_token|api[_-]?key|sig|signature|auth|authorization)/i +const URL_CREDENTIAL_VALUE = /^(?:[A-Za-z0-9._~+\/=-]{8,}|[A-Fa-f0-9]{16,})$/ +// char set: alphanumerics + base64-url alphabet (`. _ ~ + / - =`). Hyphen is +// included so URL-safe base64 tokens and AWS SigV4 signatures match. + +function urlContainsCredential(url: string): boolean { + // Parse the URL and inspect each query parameter; flag any parameter whose + // name matches a credential-shaped token name and whose value matches a + // structured-token shape. + try { + const parsed = new URL(url) + for (const [key, value] of parsed.searchParams) { + if (!URL_CREDENTIAL_KEYS.test(key)) continue + if (URL_CREDENTIAL_VALUE.test(value)) return true + } + } catch { + // URL parsing failed; fall through to a regex over the raw text. + } + // Fallback: catch credentials embedded before a parsed URL can be made. + return /[?&](?:token|access_token|api[_-]?key|sig|signature|auth|authorization)=[^&\s"']{8,}/i.test(url) +} +``` + +The two regexes are tuned to balance false-positive suppression (`[A-Za-z0-9._~+\/-=]{8,}` rejects short or all-space strings; `[A-Fa-f0-9]{16,}` rejects short hex) and coverage of common query-string secret shapes (long base64url tokens, hex digests, signed HMAC outputs). + +### 3.2 `hardDenyReason` addition + +```ts +const argsUrl = typeof args?.url === 'string' ? args.url : undefined +if (argsUrl !== undefined + && (/^(?:web_fetch|curl|wget)/i.test(exec.name) || EXTERNAL_WRITE_TOOL.test(exec.name)) + && urlContainsCredential(argsUrl)) { + return 'external URL contains credential-shaped query parameter' +} +``` + +Inserted next to the existing `containsCredentialMaterial` early return in `hardDenyReason`, so the original detection and the new URL detection compose with `OR`. + +### 3.3 Why pre-classifier instead of classifier-only + +The classifier receives a sanitized URL (because the entire `url` field goes through `sanitizeClassifierText` which now catches inline `api_key=…` patterns). It cannot reliably distinguish "URL was already credentialed" from "URL is asking the remote for credentials" once the literal token is redacted. The pre-classifier check is the only way to surface a structured credential parameter to the hard-deny fuse. + +### 3.4 What is **not** detected + +The check is conservative on purpose: +- Bearer-token-looking fragments only in headers, not the URL itself: deferred to the existing `containsCredentialMaterial` path. +- API keys in HTTP header `Authorization` only (no body): out of scope; web_fetch/curl/wget arguments carry URL + body, both of which already flow through `sanitizeClassifierArguments`. +- URLs where the credential is in the path segment: not a common public web pattern; not added. + +--- + +## Section 4 — Tests + +### 4.1 `tests/classifier.spec.ts` updates + +- Replace the existing `redacts bulk content and credentials before classification` expectation: + - `content: 'repository payload'` becomes `content: 'repository payload'` (unchanged string, no key-whitelist replacement) + - `apiKey: '[redacted-secret-field]'` keeps the same expectation because `SECRET_KEYS` still matches `apiKey` + - Add inline: `command: 'curl …token=…'` collapses to `command: 'curl …[redacted-key-value]'` +- New test cases per pattern family: + - AKIA / ASIA in tool args string + - `gho_…` 40-char GitHub OAuth in tool args + - `ghp_/ghu_/ghs_…` 36-char GitHub classic tokens + - `github_pat_…` fine-grained PAT + - `sk-ant-…` Anthropic key + - `-----BEGIN OPENSSH PRIVATE KEY-----…-----END…PRIVATE KEY-----` multiline PEM +- New direct unit block for `redactClassifierText`: + - Each pattern family, plus a parity test that the previous name suffixes map to the new markers. + +### 4.2 `tests/policy.spec.ts` updates + +- New `describe('apply_patch')` block: + - 6 happy-path / destructive / protected cases from Section 2.5 + - Multi-file patch: any target being destructive ⇒ hard deny + - Unparseable patch: `assessTool` returns `{ decision: 'ask', classifierEligible: false }` + - Empty patch text: `assessTool` returns `{ decision: 'ask', reason: 'apply_patch payload is missing', classifierEligible: false }` +- New `describe('web_fetch URL hardening')` block: + - URL with `?token=longstring12345` ⇒ hard deny + - URL with `?token=hello` (too short) ⇒ no deny + - URL with `?q=hello` (non-credential key) ⇒ no deny + - URL with `?sig=<40 hex>` ⇒ hard deny + - URL fails to parse (no protocol) ⇒ regex fallback catches `?token=…` + +### 4.3 `tests/paths.spec.ts` updates + +- New `extractApplyPatchPaths` test block: + - Single-file modification: returns `[]` + - New file: `--- /dev/null` + `+++ b/` returns `[]` + - Deletion: returns `[]` + - Multi-file patch: returns both, in order, deduplicated + - Rename pair: `rename from a/x` + `rename to b/y` returns `[]` + - Header with no recognizable `+++`: returns `[]` + - Truncated patch (only `--- a/x` without `+++`): returns `[]` + - Paths with leading `a/` or `b/`: stripped + - Output preserves raw paths (no normalization) — caller normalizes + +### 4.4 Test count estimate + +- ~12 new `redactClassifierText` unit cases +- ~10 new `sanitizeClassifierArguments` cases +- ~8 new `apply_patch` policy cases +- ~5 new `web_fetch` URL cases +- ~9 new `extractApplyPatchPaths` unit cases +- ≈ 44 net-new test cases. + +--- + +## Section 5 — Public-API and `index.ts` + +No changes to `src/index.ts` are required: +- `sanitizeClassifierText` and `sanitizeClassifierArguments` are exported from `src/classifier.ts` and re-exported by `src/index.ts:7` (import) and `src/index.ts:15` (re-export). The signatures do not change. +- The new exports `redactClassifierText` and `ClassifierRedaction` are added to `src/classifier.ts`. They are exported because `tests/classifier.spec.ts` exercises them directly. `src/index.ts` does not need to re-export them — internal API only. + +--- + +## Section 6 — Risk register + +| Risk | Probability | Mitigation | +|---|---|---| +| Redaction false positives (e.g., `ghp_` accidentally matches a 36-char base64 word) | Low | The pattern requires the literal `gh[pus]_` prefix; in non-code identifiers base64 does not start with `gh[pus]_`. Strings outside that namespace are unaffected. | +| `extractApplyPatchPaths` returning `[]` on a patch that simply omits `+++ b/…` (an unusual but legal git output) | Low | Fail-closed ⇒ `ask`. No `allow` regression. | +| Removing `CONTENT_KEYS` causes a regression in tests that explicitly check for `[redacted-content:N-chars]` markers | Medium | Test update is part of this spec (§4.1). | +| `web_fetch` URL detection fires on legit URLs like `?token=short_frag` | Very low | `URL_CREDENTIAL_VALUE` requires ≥8 chars. | +| Patch text 1000+ chars gets truncated by `sanitizeClassifierText` before reaching classifier | Same as before | Reuse existing truncation behavior; classifier has its own length budget. | + +--- + +## Section 7 — Open questions (none blocking) + +- Whether `apply_patch` arguments should also support `input` rather than `patch`. We support both to be permissive; resolved during implementation by inspecting the upstream tool integration. +- Whether the existing `sanitizeClassifierText` redaction marker for old inline patterns (which used the literal `[redacted-secret]`) should remain or be aliased. The new marker scheme uses `[redacted-]` for everything. No alias kept (single marker shape for clarity). + +--- + +## Section 8 — Acceptance criteria + +1. `pnpm test` passes locally; `git diff --stat` shows changes only in `src/classifier.ts`, `src/policy.ts`, `src/paths.ts`, and the three test files. No new files. +2. `pnpm typecheck` (or `tsc --noEmit && tsc -p tsconfig.client.json --noEmit`) is clean. +3. New behavior is exercised by the new test cases; specifically: + - `extractApplyPatchPaths` is unit-tested end to end. + - `redactClassifierText` is unit-tested for every `CREDENTIAL_PATTERNS` entry. + - `apply_patch` is fully covered by assessTool and hardDenyReason tests. + - `web_fetch` URL with credential-shaped query parameter lands in `hardDenyReason`. +4. Existing tests that relied on the `[redacted-content:N-chars]` marker format are updated to the new `[redacted-]` format. +5. No additional new write-side `ask` paths are added beyond the existing write/edit/str_replace_editor branches (the `'G-3 apply_patch'` branch is added, which is structurally needed because apply_patch had no branch at all). + +--- + +## Section 9 — Investigation backlog (future PR / spec) + +Items from the local comparison (`dsh-auto-mode-vs-dsh-auto-review.md`) that are intentionally **out of scope** for this spec. They will be tracked in a separate `9-investigation-backlog.md`. + +- **B1 — `web_fetch` URL query-string credentials for non-HTTPS endpoint** (low priority; HTTPS isn't enforced on web_fetch in upstream). +- **B2 — Hardening `apply_patch` for content-based credential detection** within the patch body (matches the user's PR-A in spirit, but only on the apply_patch text path; same maintainer pushback likely). +- **B3 — Symlink escape (Critical-2)**: writing through a workspace symlink that points at `~/.ssh` or `~/.aws`. Sandbox-first philosophy may require a fundamentally different design (trust boundary on first symlink traversal). Deferred. +- **B4 — AKIA pattern coverage in `containsCredentialMaterial`** (write-side credential-shaped blocking). The maintainer declined PR-A in this direction. Tracker only. +- **B5 — Pre-classifier override of `web_fetch` for arbitrary hostnames** (out of threat-model scope for Auto). From b2148fc69ce7a16527545a765c53c587c2bb0f1a Mon Sep 17 00:00:00 2001 From: AtropinolTT Date: Mon, 17 Aug 2026 15:50:49 +0800 Subject: [PATCH 2/9] docs(spec): close review must-fix items (G1, G2, G3) 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). --- ...7-sanitizer-and-policy-hardening-design.md | 32 +++++++++++++------ 1 file changed, 22 insertions(+), 10 deletions(-) diff --git a/docs/superpowers/specs/2026-08-17-sanitizer-and-policy-hardening-design.md b/docs/superpowers/specs/2026-08-17-sanitizer-and-policy-hardening-design.md index 3848777..2fbfc1e 100644 --- a/docs/superpowers/specs/2026-08-17-sanitizer-and-policy-hardening-design.md +++ b/docs/superpowers/specs/2026-08-17-sanitizer-and-policy-hardening-design.md @@ -55,7 +55,11 @@ const CREDENTIAL_PATTERNS: readonly CredentialPattern[] = [ // LLM and tool vendor keys. { name: 'llm-key', pattern: /\bsk-(?:proj-)?[A-Za-z0-9_-]{16,}\b/g }, - { name: 'anthropic-key', pattern: /\bsk-ant-[A-Za-z0-9_-]{20,}\b/g }, + // Anthropic keys are `sk-ant-apiNN-` where NN is exactly 2 digits + // and is 32+ base64url-ish chars. Anchoring `api\d{2}-` rejects + // documentation/strings of the shape `sk-ant-` and only + // matches real Anthropic key prefixes per vendor docs. + { name: 'anthropic-key', pattern: /\bsk-ant-api\d{2}-[A-Za-z0-9_-]{32,}\b/g }, // PEM private-key blocks (RSA, OPENSSH, EC, ECDSA, DSA, ENCRYPTED). // Matches the BEGIN line through the next matching END line, inclusive. @@ -311,10 +315,11 @@ The check is conservative on purpose: ### 4.1 `tests/classifier.spec.ts` updates -- Replace the existing `redacts bulk content and credentials before classification` expectation: - - `content: 'repository payload'` becomes `content: 'repository payload'` (unchanged string, no key-whitelist replacement) - - `apiKey: '[redacted-secret-field]'` keeps the same expectation because `SECRET_KEYS` still matches `apiKey` - - Add inline: `command: 'curl …token=…'` collapses to `command: 'curl …[redacted-key-value]'` +- Replace the existing `redacts bulk content and credentials before classification` (`tests/classifier.spec.ts:48-59`) expectation. Three assertions change: + - Line 54: `command: 'curl -H "Authorization: Bearer secret-token-value" https://example.invalid'` becomes `command: 'curl -H "Authorization: [redacted-bearer]" https://example.invalid'`. The whole `Bearer ` match is replaced with `[redacted-bearer]` (note: the prior implementation kept a `Bearer ` prefix and replaced only the token suffix; the new uniform `[redacted-]` shape replaces the entire match). + - Line 55: `content: '[redacted-content:18-chars]'` becomes `content: 'repository payload'` (`CONTENT_KEYS` is removed; the value is recursed into `sanitizeClassifierText` which finds no credential pattern and emits it unchanged). + - Line 56: `apiKey: '[redacted-secret-field]'` keeps the same expectation because `SECRET_KEYS` still matches `apiKey`. + - Line 58: `sanitizeClassifierText('please use sk-example-secret-value for the test')` returns `'please use [redacted-token-suffix] for the test'` (was `'please use [redacted-secret] for the test'`). The `sk-<16+ chars>` shape triggers the `token-suffix` pattern (line 44 of §1.1) and the new marker naming scheme applies. - New test cases per pattern family: - AKIA / ASIA in tool args string - `gho_…` 40-char GitHub OAuth in tool args @@ -327,11 +332,18 @@ The check is conservative on purpose: ### 4.2 `tests/policy.spec.ts` updates -- New `describe('apply_patch')` block: - - 6 happy-path / destructive / protected cases from Section 2.5 - - Multi-file patch: any target being destructive ⇒ hard deny - - Unparseable patch: `assessTool` returns `{ decision: 'ask', classifierEligible: false }` - - Empty patch text: `assessTool` returns `{ decision: 'ask', reason: 'apply_patch payload is missing', classifierEligible: false }` +- New `describe('apply_patch')` block covering every case below (one `it` per bullet): + - happy-path modification to a workspace file ⇒ `allow` with `filesystemEffects` populated; + - create via `--- /dev/null` + `+++ b/path` (new file) ⇒ `allow` with `filesystemEffects` populated; + - delete via `--- a/x` + `+++ /dev/null` ⇒ `allow` with `filesystemEffects` populated, `kind: 'create-or-overwrite'`, `existedBefore` reflecting actual file state; + - rename via `rename from`/`rename to` ⇒ target from the `rename to` side is recorded and policed; + - protected project path (e.g., `.git/config`) ⇒ `assessTool` returns `{ decision: 'ask', classifierEligible: true }` with `filesystemEffects` populated; + - destructive path (e.g., `/etc/passwd`, `~/.ssh/id_rsa`, DSH_HOME) ⇒ `hardDenyReason` returns a string matching `/credential|critical|root|DSH_HOME/`; + - multi-file patch where one target is destructive ⇒ hard deny (any-target-deny semantics); + - unparseable patch (no `---`/`+++` pair extractable) ⇒ `assessTool` returns `{ decision: 'ask', classifierEligible: false }`; + - empty patch text / missing `args.patch`/`args.input` ⇒ `assessTool` returns `{ decision: 'ask', reason: 'apply_patch payload is missing', classifierEligible: false }`; + - `args.input` (alternate field) is read same as `args.patch`; + - `args.file_path` populated alongside patch text ⇒ path-only check still wins (i.e., the existing `hardDenyReason` block at `policy.ts:142-149` runs first and any destructive `file_path` still hard-denies). - New `describe('web_fetch URL hardening')` block: - URL with `?token=longstring12345` ⇒ hard deny - URL with `?token=hello` (too short) ⇒ no deny From e750682fd7c469a4d4e9ae81b9687294952a0bc5 Mon Sep 17 00:00:00 2001 From: AtropinolTT Date: Mon, 17 Aug 2026 15:56:20 +0800 Subject: [PATCH 3/9] docs(plan): implementation plan for sanitizer + apply_patch + URL hardening MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../2026-08-17-sanitizer-policy-hardening.md | 1259 +++++++++++++++++ 1 file changed, 1259 insertions(+) create mode 100644 docs/superpowers/plans/2026-08-17-sanitizer-policy-hardening.md diff --git a/docs/superpowers/plans/2026-08-17-sanitizer-policy-hardening.md b/docs/superpowers/plans/2026-08-17-sanitizer-policy-hardening.md new file mode 100644 index 0000000..ab7f008 --- /dev/null +++ b/docs/superpowers/plans/2026-08-17-sanitizer-policy-hardening.md @@ -0,0 +1,1259 @@ +# Sanitizer Redesign + `apply_patch` Coverage + `web_fetch` URL Hardening Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Replace the key-name-driven sanitizer with content-first credential redaction (closing the `str_replace_editor` field gap the maintainer acknowledged), add end-to-end path-based policy enforcement for `apply_patch`, and add a credential-bearing URL hard-deny for outbound tools (`web_fetch`/`curl`/`wget`/`EXTERNAL_WRITE_TOOL`). All three stay inside `src/classifier.ts`, `src/policy.ts`, `src/paths.ts`, with no write-side content blockers (sandbox-first maintained). + +**Architecture:** Single-pass line scanner in `paths.ts` extracts `---/+++` and `rename from/to` targets from an `apply_patch` text; both `hardDenyReason` and `assessTool` feed those targets through the existing `hardDestructiveTargetReason` and `isProtectedProjectPath` fuses. `classifier.ts` swaps `CONTENT_KEYS`-based key replacement for pattern-driven redaction via `CREDENTIAL_PATTERNS`; the rest of the public API keeps identical signatures. New credential-bearing URL check in `policy.ts` mirrors the existing `containsCredentialMaterial` early-return shape so credentials in URLs are hard-denied before classifier dispatch. + +**Tech Stack:** TypeScript 5.9, Node 22, vitest 4. No new dependencies. No new files. + +**Spec:** [`docs/superpowers/specs/2026-08-17-sanitizer-and-policy-hardening-design.md`](../specs/2026-08-17-sanitizer-and-policy-hardening-design.md) — the executor reads both side by side. Spec commits are `3437cdc` (initial) and `b2148fc` (must-fix revisions G1/G2/G3 from review). + +## Global Constraints + +Verbatim from spec §Constraints: + +- Diff as small as possible. +- No new files; all source changes stay in `src/classifier.ts`, `src/policy.ts`, `src/paths.ts`. +- No public API breakage at the export level (function signatures preserved). +- No new write-side content blocker (must not conflict with sandbox-first philosophy). +- All four line breaks in `tests/classifier.spec.ts:54/55/56/58` are explicitly updated per spec §4.1. +- Anthropic regex must anchor `api\d{2}-` and require 32+ body chars (G1 review fix). + +Project-wide: + +- TypeScript strict mode (no implicit any, exact optional properties). +- Public exports gain `readonly` types where missing. +- Test names use the existing `it('does X', …)` shape; descriptive but concise. +- Use `read` / `write` tools for editing; do not use `sed`/`awk` for source edits. + +--- + +## Task 1: `extractApplyPatchPaths` parser in `src/paths.ts` + +**Files:** +- Create: `src/paths.ts` extension (function added at the bottom of the file, after `isArtifactArea`). +- Test: `tests/paths.spec.ts` (existing file, new `describe` block). + +**Interfaces:** +- Consumes: nothing (pure function). +- Produces: `export function extractApplyPatchPaths(patch: string): string[]` returning deduplicated raw filesystem paths (no `normalizePath` here — caller normalizes). + +**Algorithm (single pass, deterministic).** The patch text is split on `\r?\n`. Walk the lines: + +1. For each line starting with `--- ` (3 dashes plus space): + - Capture the minus header (the path after `--- `). + - Walk forward until a line starting with `+++ ` (3 pluses plus space) is found. + - On finding the plus header: + - If minus is `/dev/null` and plus is `b/path` (or `path` with optional leading `b/`): record the plus path (new file). + - If plus is `/dev/null` and minus is `a/path` (or `path` with optional leading `a/`): record the minus path (delete). + - Otherwise (both real paths): record the plus path (modify). + - Skip past the plus header to avoid double-pairing. +2. For each line starting with `rename to `: record the path after `rename to `. +3. Strip leading `a/` or `b/` from recorded paths. Discard `/dev/null` and empty strings. Deduplicate in first-seen order. +4. Return `[]` for non-string input, empty input, or any patch text where no `--- ` header can be paired with a `+++ ` header (fail-closed contract per spec §2.4). + +- [ ] **Step 1: Write the failing tests** + +Edit `tests/paths.spec.ts`. Add at the end of the existing `describe('path policy', …)` block (before the closing `})`): + +```ts + it('extracts single-file modification paths from a patch', () => { + const patch = [ + 'diff --git a/src/a.ts b/src/a.ts', + 'index 0123..4567 100644', + '--- a/src/a.ts', + '+++ b/src/a.ts', + '@@ -1,1 +1,1 @@', + '-old', + '+new', + ].join('\n') + expect(extractApplyPatchPaths(patch)).toEqual(['src/a.ts']) + }) + + it('records new file paths from --- /dev/null + +++ b/', () => { + const patch = [ + 'diff --git a/src/new.ts b/src/new.ts', + 'new file mode 100644', + 'index 0000000..1234567', + '--- /dev/null', + '+++ b/src/new.ts', + ].join('\n') + expect(extractApplyPatchPaths(patch)).toEqual(['src/new.ts']) + }) + + it('records deleted file paths from --- a/ + +++ /dev/null', () => { + const patch = [ + 'diff --git a/src/old.ts b/src/old.ts', + 'deleted file mode 100644', + 'index 1234567..0000000', + '--- a/src/old.ts', + '+++ /dev/null', + ].join('\n') + expect(extractApplyPatchPaths(patch)).toEqual(['src/old.ts']) + }) + + it('records all target paths in a multi-file patch, deduplicated, in order', () => { + const patch = [ + 'diff --git a/src/a.ts b/src/a.ts', + '--- a/src/a.ts', + '+++ b/src/a.ts', + '@@ -1 +1 @@', + '-old', + '+new', + 'diff --git a/src/b.ts b/src/b.ts', + '--- a/src/b.ts', + '+++ b/src/b.ts', + '@@ -1 +1 @@', + '-old', + '+new', + 'diff --git a/src/a.ts b/src/a.ts', + '--- a/src/a.ts', + '+++ b/src/a.ts', + '@@ -1 +1 @@', + '-old2', + '+new2', + ].join('\n') + expect(extractApplyPatchPaths(patch)).toEqual(['src/a.ts', 'src/b.ts']) + }) + + it('records the destination path from rename from/rename to pairs', () => { + const patch = [ + 'diff --git a/src/old-name.ts b/src/new-name.ts', + 'similarity index 100%', + 'rename from src/old-name.ts', + 'rename to src/new-name.ts', + ].join('\n') + expect(extractApplyPatchPaths(patch)).toEqual(['src/new-name.ts']) + }) + + it('returns [] for an unparseable patch (no +++ pair)', () => { + expect(extractApplyPatchPaths('--- a/x\n@@ -1 +1 @@\n-old\n')).toEqual([]) + }) + + it('returns [] when patch text is empty or non-string', () => { + expect(extractApplyPatchPaths('')).toEqual([]) + // @ts-expect-error: runtime contract accepts unknown inputs safely. + expect(extractApplyPatchPaths(undefined)).toEqual([]) + }) + + it('strips the optional leading a/ or b/ from recorded paths', () => { + const patch = [ + '--- a/foo.ts', + '+++ b/foo.ts', + ].join('\n') + expect(extractApplyPatchPaths(patch)).toEqual(['foo.ts']) + }) + + it('preserves raw paths (no normalization applied)', () => { + const patch = [ + '--- a/CAPS.ts', + '+++ b/CAPS.ts', + ].join('\n') + expect(extractApplyPatchPaths(patch)).toEqual(['CAPS.ts']) + }) +``` + +Add to the import line at the top of `tests/paths.spec.ts`: + +```ts +import { + canonicalizePosixSystemAlias, + canonicalizeWindowsNamespace, + extractApplyPatchPaths, + hardDestructiveTargetReason, + isFilesystemRoot, + isProtectedProjectPath, + isWithin, + normalizePath, + resolveRoots, +} from '../src/paths.js' +``` + +- [ ] **Step 2: Run the failing tests and confirm they fail** + +Run: `cd /home/tt-wsl-ubuntu/skills-repo/dsh-auto-mode-ref && timeout 120 npx vitest run tests/paths.spec.ts 2>&1 | tail -40` +Expected: every new `it()` in this block fails with "extractApplyPatchPaths is not a function" (or "is not exported"). Confirm the 9 failures before moving on. + +- [ ] **Step 3: Implement `extractApplyPatchPaths` in `src/paths.ts`** + +Add at the bottom of `src/paths.ts` (after the existing `isArtifactArea` function): + +```ts +/** + * Extract filesystem targets from a unified-diff style `apply_patch` payload. + * + * Walks the patch text once on a per-line basis and records: + * - the destination path of every `--- a/` + `+++ b/` pair + * (modify), + * - `+++ b/` when paired with `--- /dev/null` (new file), + * - `--- a/` when paired with `+++ /dev/null` (delete), + * - the destination path of every `rename from` / `rename to` pair. + * + * Returns deduplicated, order-preserved paths with any leading `a/` or `b/` + * stripped. Paths are NOT normalized here — every caller passes the result + * through `normalizePath` so workspace-relative and absolute paths resolve + * the same way. + * + * Returns `[]` for empty input or any patch text where no `---` header can + * be paired with a `+++` header. Callers treat that as fail-closed + * (manual approval) rather than as a successful empty parse. + */ +export function extractApplyPatchPaths(patch: string): string[] { + if (typeof patch !== 'string' || patch === '') return [] + const lines = patch.split(/\r?\n/) + const targets: string[] = [] + const seen = new Set() + + const record = (raw: string | undefined): void => { + if (raw === undefined) return + if (raw === '' || raw === '/dev/null') return + const stripped = raw.startsWith('a/') || raw.startsWith('b/') ? raw.slice(2) : raw + if (!seen.has(stripped)) { + seen.add(stripped) + targets.push(stripped) + } + } + + let i = 0 + while (i < lines.length) { + const line = lines[i] + if (line.startsWith('--- ')) { + const minusRaw = line.slice(4) + const minusPath = minusRaw === '/dev/null' + ? undefined + : minusRaw.startsWith('a/') ? minusRaw.slice(2) : minusRaw + + // Walk forward until we find the matching +++ header. + let j = i + 1 + while (j < lines.length && !lines[j].startsWith('+++ ')) j++ + if (j < lines.length) { + const plusRaw = lines[j].slice(4) + const plusPath = plusRaw === '/dev/null' + ? undefined + : plusRaw.startsWith('b/') ? plusRaw.slice(2) : plusRaw + + if (minusPath === undefined && plusPath !== undefined) { + // New file: --- /dev/null + +++ b/ ⇒ record plus. + record(plusPath) + } else if (plusPath === undefined && minusPath !== undefined) { + // Delete: --- a/ + +++ /dev/null ⇒ record minus. + record(minusPath) + } else if (plusPath !== undefined) { + // Modify: record the destination path. + record(plusPath) + } + i = j + 1 + continue + } + } else if (line.startsWith('rename to ')) { + record(line.slice('rename to '.length)) + } + i++ + } + + return targets +} +``` + +- [ ] **Step 4: Run the tests and confirm they pass** + +Run: `cd /home/tt-wsl-ubuntu/skills-repo/dsh-auto-mode-ref && timeout 120 npx vitest run tests/paths.spec.ts 2>&1 | tail -30` +Expected: all 16 tests pass (8 pre-existing + 9 new — total 17; one of the existing paths tests was added previously, so number may be 8 + 9 = 17). + +- [ ] **Step 5: Commit** + +```bash +cd /home/tt-wsl-ubuntu/skills-repo/dsh-auto-mode-ref +git add src/paths.ts tests/paths.spec.ts +git commit -m "feat(paths): single-pass parser for apply_patch target paths + +extractApplyPatchPaths walks the patch text once on a per-line basis, +pairs every '--- a/' with the next '+++ b/' (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)." +``` + +--- + +## Task 2: `redactClassifierText` + `CREDENTIAL_PATTERNS` in `src/classifier.ts` + +**Files:** +- Modify: `src/classifier.ts` — add new types, the pattern table, and the core `redactClassifierText` function. Refactor `sanitizeClassifierText` to wrap it. (Leave `sanitizeClassifierArguments` for Task 3.) +- Test: `tests/classifier.spec.ts` — add a new `describe('redactClassifierText')` block with one `it` per pattern family. + +**Interfaces:** +- Consumes: nothing new beyond `string`. +- Produces: + - `export interface ClassifierRedaction { readonly value: string; readonly redactedNames: readonly string[] }` + - `export function redactClassifierText(value: string, maxLength?: number): ClassifierRedaction` + - `export function sanitizeClassifierText(value: string): string` — thinned to a wrapper; signature unchanged. + +- [ ] **Step 1: Write the failing tests** + +Append a new `describe` block at the bottom of `tests/classifier.spec.ts`: + +```ts +describe('redactClassifierText', () => { + it('redacts AKIA / ASIA AWS access-key IDs', () => { + const result = redactClassifierText('export AWS_ACCESS_KEY_ID=AKIAIOSFODNN7EXAMPLE') + expect(result.value).toBe('export AWS_ACCESS_KEY_ID=[redacted-aws-access-key]') + expect(result.redactedNames).toContain('aws-access-key') + }) + + it('redacts github classic / user / server PATs (gh[pus]_<36>)', () => { + const result = redactClassifierText('token = ghp_012345678901234567890123456789012345') + expect(result.value).toBe('token = [redacted-github-classic]') + expect(result.redactedNames).toContain('github-classic') + const user = redactClassifierText('token = ghu_012345678901234567890123456789012345') + expect(user.value).toBe('token = [redacted-github-classic]') + const server = redactClassifierText('token = ghs_012345678901234567890123456789012345') + expect(server.value).toBe('token = [redacted-github-classic]') + }) + + it('redacts gho_ github OAuth access tokens (40 chars)', () => { + const result = redactClassifierText('token = gho_0123456789abcdef0123456789abcdef01234567') + expect(result.value).toBe('token = [redacted-github-oauth]') + expect(result.redactedNames).toContain('github-oauth') + }) + + it('redacts github_pat_ fine-grained PATs (22+ chars body)', () => { + const result = redactClassifierText('token = github_pat_0123456789abcdef_0123456789abcdef') + expect(result.value).toBe('token = [redacted-github-fine-pat]') + expect(result.redactedNames).toContain('github-fine-pat') + }) + + it('redacts sk- / sk-proj- LLM API keys (16+ chars)', () => { + const plain = redactClassifierText('sk-0123456789abcdef0123456789abcdef') + expect(plain.value).toBe('[redacted-llm-key]') + expect(plain.redactedNames).toContain('llm-key') + const proj = redactClassifierText('sk-proj-0123456789abcdef0123456789abcdef') + expect(proj.value).toBe('[redacted-llm-key]') + }) + + it('redacts Anthropic keys only when anchored to sk-ant-api-<32+>', () => { + // Real Anthropic key (api03 + 40-char body) — matches. + const real = redactClassifierText('sk-ant-api03-abcdefghijklmnopqrstuvwxyz1234567890') + expect(real.value).toBe('[redacted-anthropic-key]') + // Documentation strings without the api- prefix — DO NOT match. + expect(redactClassifierText('uses sk-ant-abcdefghij1234567890 in config').redactedNames) + .not.toContain('anthropic-key') + // Single-digit api version — does NOT match. + expect(redactClassifierText('sk-ant-api3-abcdefghijklmnopqrstuvwxyz123456').redactedNames) + .not.toContain('anthropic-key') + }) + + it('redacts a complete PEM private-key block (BEGIN ... END inclusive)', () => { + const pem = [ + '-----BEGIN OPENSSH PRIVATE KEY-----', + 'abc', + 'def', + '-----END OPENSSH PRIVATE KEY-----', + ].join('\n') + const result = redactClassifierText(pem) + expect(result.value).toBe('[redacted-pem-private-key]') + expect(result.redactedNames).toContain('pem-private-key') + }) + + it('redacts multiple credential shapes in one input and lists each name once', () => { + const input = 'AKIAIOSFODNN7EXAMPLE sk-ant-api03-abcabcabcabcabcabcabcabcabcabc12AKIAL' + const result = redactClassifierText(input) + expect(result.redactedNames).toContain('aws-access-key') + expect(result.redactedNames).toContain('anthropic-key') + }) + + it('emits the input unchanged when no pattern matches', () => { + const input = 'just a plain string with no credentials' + const result = redactClassifierText(input) + expect(result.value).toBe(input) + expect(result.redactedNames).toEqual([]) + }) + + it('truncates the redacted value to maxLength when supplied', () => { + const result = redactClassifierText('A'.repeat(2000), 100) + expect(result.value.length).toBe(100) + }) +}) +``` + +Add at the top of `tests/classifier.spec.ts` (preserving the existing imports): + +```ts +import { + parseClassifierDecision, + redactClassifierText, + sanitizeClassifierArguments, + sanitizeClassifierText, +} from '../src/classifier.js' +``` + +- [ ] **Step 2: Run the new tests and confirm they fail** + +Run: `cd /home/tt-wsl-ubuntu/skills-repo/dsh-auto-mode-ref && timeout 120 npx vitest run tests/classifier.spec.ts -t "redactClassifierText" 2>&1 | tail -30` +Expected: every `it` in the new `describe` block fails because `redactClassifierText` is not exported. Confirm all 10 failures before moving on. + +- [ ] **Step 3: Implement `redactClassifierText` + `CREDENTIAL_PATTERNS` + thin `sanitizeClassifierText` wrapper** + +Replace the existing `sanitizeClassifierText` and `CONTENT_KEYS` / `SECRET_KEYS` declarations (currently `src/classifier.ts:32-42`) with: + +```ts +interface CredentialPattern { + readonly name: string + readonly pattern: RegExp +} + +const CREDENTIAL_PATTERNS: readonly CredentialPattern[] = [ + // Inline value-shape patterns kept from the existing sanitizer. + { name: 'token-suffix', pattern: /\b(?:sk|ghp|github_pat|xox[baprs])[-_][A-Za-z0-9_-]{8,}\b/g }, + { name: 'bearer', pattern: /\bBearer\s+[A-Za-z0-9._~+\/-]{8,}/gi }, + { name: 'key-value', pattern: /((?:api[_-]?key|token|secret|password)=)[^&\s]+/gi }, + + // AWS access-key IDs (16 chars after the prefix). + { name: 'aws-access-key', pattern: /\b(?:AKIA|ASIA)[A-Z0-9]{16}\b/g }, + + // GitHub token formats (gho_ is 40; gh[pus]_ is 36; github_pat_ has 22+). + { name: 'github-oauth', pattern: /\bgho_[A-Za-z0-9]{40}\b/g }, + { name: 'github-classic', pattern: /\bgh[pus]_[A-Za-z0-9]{36}\b/g }, + { name: 'github-fine-pat', pattern: /\bgithub_pat_[A-Za-z0-9_]{22,}\b/g }, + + // LLM and tool vendor keys. + { name: 'llm-key', pattern: /\bsk-(?:proj-)?[A-Za-z0-9_-]{16,}\b/g }, + // Anthropic keys are `sk-ant-apiNN-` where NN is exactly 2 digits + // and is 32+ base64url-ish chars. Anchoring `api\d{2}-` rejects + // documentation/strings of the shape `sk-ant-` and only + // matches real Anthropic key prefixes per vendor docs. + { name: 'anthropic-key', pattern: /\bsk-ant-api\d{2}-[A-Za-z0-9_-]{32,}\b/g }, + + // PEM private-key blocks (RSA, OPENSSH, EC, ECDSA, DSA, ENCRYPTED). + // Matches the BEGIN line through the next matching END line, inclusive. + { name: 'pem-private-key', + pattern: /-----BEGIN (?:RSA |OPENSSH |EC |ECDSA |DSA |ENCRYPTED )?PRIVATE KEY-----[\s\S]*?-----END (?:RSA |OPENSSH |EC |ECDSA |DSA |ENCRYPTED )?PRIVATE KEY-----/g }, +] + +const SECRET_KEYS = /(?:api|auth|access|secret|private|credential|password|token|cookie|authorization).*?(?:key|value|token)?$/i + +/** Redact pattern-matched credential content and surface which patterns fired. */ +export interface ClassifierRedaction { + readonly value: string + readonly redactedNames: readonly string[] +} + +/** + * Run every entry in {@link CREDENTIAL_PATTERNS} against the input. Each match + * is replaced with `[redacted-]`; each unique name that fires is appended + * to `redactedNames` in the order it appears in the patterns table. Truncated + * to `maxLength` when the redacted value would otherwise exceed it. + * + * This is content-first: a credential-shaped substring anywhere in the input is + * redacted regardless of the field name it sits under. Key-name matching is + * still applied one layer up in {@link sanitizeClassifierArguments} for + * defense-in-depth on `SECRET_KEYS`. + */ +export function redactClassifierText(value: string, maxLength = 1_000): ClassifierRedaction { + let current = value + const redactedNames: string[] = [] + for (const { name, pattern } of CREDENTIAL_PATTERNS) { + const before = current + current = current.replace(pattern, `[redacted-${name}]`) + if (current !== before && !redactedNames.includes(name)) redactedNames.push(name) + } + if (current.length > maxLength) current = current.slice(0, maxLength) + return { value: current, redactedNames } +} + +/** + * Public thin wrapper over {@link redactClassifierText}. The kept signature + * preserves consumer call sites in `src/index.ts` (`trustedUserMessages`, + * `sandboxRequest.justification`) — see spec §1.4. + */ +export function sanitizeClassifierText(value: string): string { + return redactClassifierText(value).value +} +``` + +Confirm the existing `sanitizeClassifierArguments` (Task 3's target) is untouched for now. + +- [ ] **Step 4: Run the new tests and confirm they pass** + +Run: `cd /home/tt-wsl-ubuntu/skills-repo/dsh-auto-mode-ref && timeout 120 npx vitest run tests/classifier.spec.ts -t "redactClassifierText" 2>&1 | tail -30` +Expected: all 10 new tests pass; the original 14 tests in the file are still running with the old `sanitizeClassifierArguments` (they should still pass because that function has not been refactored yet). + +If anything fails, double-check the regex literal for typos. Common gotchas: +- `\b` next to a character class needs `\\b` in the JSON literal — but in source `.ts` it's just `\b`. +- `g` flag must be present for `replace` to iterate, and the pattern must not have `lastIndex` state leaks (we re-create the regex on each pass via the constant, so no state issue). + +- [ ] **Step 5: Commit** + +```bash +cd /home/tt-wsl-ubuntu/skills-repo/dsh-auto-mode-ref +git add src/classifier.ts tests/classifier.spec.ts +git commit -m "feat(classifier): content-first credential redaction (Task 2/3 of spec) + +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-]; 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." +``` + +--- + +## Task 3: Refactor `sanitizeClassifierArguments` + update existing test lines + +**Files:** +- Modify: `src/classifier.ts` — rewrite `sanitizeClassifierArguments` to pattern-redact string values and remove `CONTENT_KEYS`. +- Modify: `tests/classifier.spec.ts` — update lines 54, 55, 58 to the new expected values per spec §4.1. + +**Interfaces:** +- Consumes: `ClassifierRedaction` from Task 2. +- Produces: `sanitizeClassifierArguments(value, depth=0)` — same signature, new output shape (no `[redacted-content:N-chars]` markers; pattern-redacted strings). + +- [ ] **Step 1: Write the failing new tests** + +Append a new `describe` block at the bottom of `tests/classifier.spec.ts`: + +```ts +describe('sanitizeClassifierArguments', () => { + it('redacts a top-level credential-shaped string value but leaves neighbors alone', () => { + const out = sanitizeClassifierArguments({ + command: 'curl https://example.invalid', + note: 'use AKIAIOSFODNN7EXAMPLE in production', + }) as Record + expect(out.command).toBe('curl https://example.invalid') + expect(out.note).toBe('[redacted-aws-access-key] in production') + }) + + it('redacts string credentials nested inside arrays and objects', () => { + const out = sanitizeClassifierArguments({ + env: [ + 'GITHUB_TOKEN=ghp_012345678901234567890123456789012345', + 'OTHER=plain', + ], + nested: { + body: 'token = AKIAIOSFODNN7EXAMPLE', + }, + }) + expect(out).toEqual({ + env: [ + '[redacted-github-classic]', + '[redacted-key-value]', + ], + nested: { + body: '[redacted-aws-access-key]', + }, + }) + }) + + it('wholesale-redacts SECRET_KEYS-matched keys', () => { + const out = sanitizeClassifierArguments({ apiKey: 'sk-example-secret' }) as Record + expect(out.apiKey).toBe('[redacted-secret-field]') + }) + + it('does NOT truncate CONTENT_KEYS-matched fields any more (content keys emit plain text after pattern scan)', () => { + const out = sanitizeClassifierArguments({ content: 'repository payload' }) as Record + expect(out.content).toBe('repository payload') + }) + + it('preserves non-string scalars at depth 0', () => { + expect(sanitizeClassifierArguments({ count: 7, flag: true, missing: null })) + .toEqual({ count: 7, flag: true, missing: null }) + }) + + it('caps array size at 25 entries', () => { + const arr = Array.from({ length: 30 }, (_, i) => `item-${i}`) + const out = sanitizeClassifierArguments({ list: arr }) as { list: unknown[] } + expect(out.list.length).toBe(25) + }) + + it('caps object key count at 50 entries', () => { + const obj: Record = {} + for (let i = 0; i < 60; i++) obj[`k${i}`] = `v${i}` + const out = sanitizeClassifierArguments(obj) as Record + expect(Object.keys(out).length).toBe(50) + }) +}) +``` + +- [ ] **Step 2: Run the new tests and confirm they fail** + +Run: `cd /home/tt-wsl-ubuntu/skills-repo/dsh-auto-mode-ref && timeout 120 npx vitest run tests/classifier.spec.ts -t "sanitizeClassifierArguments" 2>&1 | tail -30` +Expected: 7 failures (the new `describe` block). Importantly, the existing test `it('redacts bulk content and credentials before classification', …)` at lines 48-59 should STILL pass because we haven't touched `sanitizeClassifierArguments` yet. + +- [ ] **Step 3: Refactor `sanitizeClassifierArguments`** + +Replace the body of `sanitizeClassifierArguments` in `src/classifier.ts` (currently `src/classifier.ts:45-62`) with: + +```ts +/** Remove bulk content and likely secrets before crossing the classifier network boundary. */ +export function sanitizeClassifierArguments(value: unknown, depth = 0): unknown { + if (depth > 3) return '[truncated-depth]' + if (typeof value === 'string') return redactClassifierText(value).value + if (typeof value === 'number' || typeof value === 'boolean' || value === null) return value + if (Array.isArray(value)) return value.slice(0, 25).map(item => sanitizeClassifierArguments(item, depth + 1)) + if (typeof value !== 'object') return `[${typeof value}]` + const output: Record = {} + for (const [key, entry] of Object.entries(value).slice(0, 50)) { + if (SECRET_KEYS.test(key)) { + output[key] = '[redacted-secret-field]' + } else { + output[key] = sanitizeClassifierArguments(entry, depth + 1) + } + } + return output +} +``` + +`CONTENT_KEYS` is removed entirely (no longer referenced). The new function pattern-redacts every string at any depth instead of replacing wholesale for `SECRET_KEYS`-matched keys and key-count-marking for `CONTENT_KEYS`-matched keys. + +- [ ] **Step 4: Update the existing failing test in `tests/classifier.spec.ts`** + +Replace `tests/classifier.spec.ts` lines 48-59 with: + +```ts + it('redacts bulk content and credentials before classification', () => { + expect(sanitizeClassifierArguments({ + command: 'curl -H "Authorization: Bearer secret-token-value" https://example.invalid', + content: 'repository payload', + apiKey: 'sk-example-secret', + })).toEqual({ + command: 'curl -H "Authorization: [redacted-bearer]" https://example.invalid', + content: 'repository payload', + apiKey: '[redacted-secret-field]', + }) + expect(sanitizeClassifierText('please use sk-example-secret-value for the test')).toBe('please use [redacted-token-suffix] for the test') + }) +``` + +Note the four changes vs the original: +- `command` line: the entire `Bearer secret-token-value` match is replaced with `[redacted-bearer]` (whole-match replacement; the prior implementation kept a `Bearer ` prefix). +- `content` line: was `[redacted-content:18-chars]`, now `repository payload` (no `CONTENT_KEYS` redaction). +- `apiKey` line: unchanged. +- Standalone assertion: was `[redacted-secret]`, now `[redacted-token-suffix]`. + +- [ ] **Step 5: Run the full `tests/classifier.spec.ts` suite** + +Run: `cd /home/tt-wsl-ubuntu/skills-repo/dsh-auto-mode-ref && timeout 120 npx vitest run tests/classifier.spec.ts 2>&1 | tail -30` +Expected: +- All 7 new `sanitizeClassifierArguments` tests pass. +- The updated `redacts bulk content and credentials before classification` test passes. +- All 10 `redactClassifierText` tests from Task 2 still pass. +- The 14 pre-existing tests (parseClassifierDecision, http classifier happy/sad path, etc.) still pass. + +Total file: ~31 tests passing. If anything fails, the most common causes are: +- A pattern leaking into `redactedNames` (typo in pattern literal). +- A `SECRET_KEYS` match being missed (something like `access_token` instead of `authorization-token`). +- Recursion depth hitting `> 3` before reaching the credential string (cap is one per recommendation; if you must keep it, set the depth limit higher in the helper only). + +- [ ] **Step 6: Commit** + +```bash +cd /home/tt-wsl-ubuntu/skills-repo/dsh-auto-mode-ref +git add src/classifier.ts tests/classifier.spec.ts +git commit -m "feat(classifier): pattern-redact every string at any depth; drop CONTENT_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-: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-#4 reply (sanit_classifier.ts calls now reach +old_str/new_str/file_text payload lines through the recursive redaction)." +``` + +--- + +## Task 4: `apply_patch` policy enforcement in `src/policy.ts` + +**Files:** +- Modify: `src/policy.ts` — add a new `apply_patch` branch in `hardDenyReason` and a new `apply_patch` branch in `assessTool`. +- Modify: `tests/policy.spec.ts` — new `describe('apply_patch')` block with one `it` per bullet from spec §4.2. + +**Interfaces:** +- Consumes: `extractApplyPatchPaths` from Task 1; existing `hardDestructiveTargetReason`, `isProtectedProjectPath`, `normalizePath`, `existedBefore` from `src/paths.ts`. +- Produces: a working `apply_patch` policy path mirroring `write/edit` semantics. + +- [ ] **Step 1: Write the failing tests for `hardDenyReason`** + +Find the existing `describe('tool policy', …)` block in `tests/policy.spec.ts` (it already imports `hardDenyReason` from `../src/policy.js`). Append the following `it` calls **inside that existing block**: + +```ts + it('hard-denies apply_patch when any target is destructive', () => { + const roots = resolveRoots('/work/repo', { home: '/home/dev', dshHome: '/safe/dsh', tempRoots: ['/tmp'] }) + const patch = [ + '--- a/src/ok.ts', + '+++ b/src/ok.ts', + '@@ -1 +1 @@', + '-old', + '+new', + '--- a/.ssh/id_rsa', + '+++ b/.ssh/id_rsa', + '@@ -1 +1 @@', + '-old', + '+new', + ].join('\n') + const exec = execution('apply_patch', { patch }) + expect(hardDenyReason(exec, roots)).toMatch(/credential|critical|root|DSH_HOME/) + }) + + it('hard-denies apply_patch targeting DSH_HOME', () => { + const roots = resolveRoots('/work/repo', { home: '/home/dev', dshHome: '/safe/dsh', tempRoots: ['/tmp'] }) + const patch = ['--- a/x', '+++ b/x', '@@ -1 +1 @@', '-old', '+new'].join('\n') + const exec = execution('apply_patch', { patch, file_path: '/safe/dsh/settings.yaml' }) + expect(hardDenyReason(exec, roots)).toMatch(/DSH_HOME/) + }) + + it('returns undefined for a benign apply_patch under hardDenyReason', () => { + const roots = resolveRoots('/work/repo', { home: '/home/dev', dshHome: '/safe/dsh', tempRoots: ['/tmp'] }) + const patch = ['--- a/src/ok.ts', '+++ b/src/ok.ts', '@@ -1 +1 @@', '-old', '+new'].join('\n') + expect(hardDenyReason(execution('apply_patch', { patch }), roots)).toBeUndefined() + }) +``` + +- [ ] **Step 2: Write the failing tests for `assessTool`** + +Append additional `it` calls in the same block: + +```ts + it('allows a happy-path apply_patch on a workspace file with filesystemEffects', () => { + const artifacts = new ArtifactRegistry() + const out = assessTool( + execution('apply_patch', { patch: ['--- a/src/ok.ts', '+++ b/src/ok.ts', '@@ -1 +1 @@', '-old', '+new'].join('\n') }), + roots, + artifacts, + ) + expect(out.decision).toBe('allow') + expect(out.classifierEligible).toBe(false) + expect(out.filesystemEffects).toEqual([ + { kind: 'create-or-overwrite', path: '/work/repo/src/ok.ts', existedBefore: expect.any(Boolean) as unknown as boolean }, + ]) + }) + + it('asks for apply_patch targeting protected project metadata', () => { + const artifacts = new ArtifactRegistry() + const out = assessTool( + execution('apply_patch', { patch: ['--- a/.git/config', '+++ b/.git/config', '@@ -1 +1 @@', '-old', '+new'].join('\n') }), + roots, + artifacts, + ) + expect(out).toMatchObject({ decision: 'ask', classifierEligible: true }) + expect(out.filesystemEffects?.[0]?.path).toBe('/work/repo/.git/config') + }) + + it('asks with classifierEligible false for an unparseable apply_patch (fail-closed)', () => { + const artifacts = new ArtifactRegistry() + const out = assessTool( + execution('apply_patch', { patch: 'this is not a patch at all' }), + roots, + artifacts, + ) + expect(out).toMatchObject({ decision: 'ask', classifierEligible: false }) + }) + + it('asks with classifierEligible false when apply_patch payload is missing', () => { + const artifacts = new ArtifactRegistry() + const out = assessTool(execution('apply_patch', {}), roots, artifacts) + expect(out).toMatchObject({ decision: 'ask', classifierEligible: false, reason: expect.stringMatching(/missing/) as unknown as string }) + }) + + it('reads apply_patch payloads from args.input as well as args.patch', () => { + const artifacts = new ArtifactRegistry() + const patch = ['--- a/src/ok.ts', '+++ b/src/ok.ts', '@@ -1 +1 @@', '-old', '+new'].join('\n') + const outFromInput = assessTool(execution('apply_patch', { input: patch }), roots, artifacts) + expect(outFromInput.decision).toBe('allow') + }) + + it('handles create (--- /dev/null + +++ b/path) and delete (--- a/path + +++ /dev/null)', () => { + const artifacts = new ArtifactRegistry() + const create = assessTool( + execution('apply_patch', { patch: ['--- /dev/null', '+++ b/src/new.ts', '@@ -0,0 +1 @@', '+new'].join('\n') }), + roots, + artifacts, + ) + expect(create.decision).toBe('allow') + expect(create.filesystemEffects?.[0]?.path).toBe('/work/repo/src/new.ts') + + const del = assessTool( + execution('apply_patch', { patch: ['--- a/src/gone.ts', '+++ /dev/null', '@@ -1 +0,0 @@', '-gone'].join('\n') }), + roots, + artifacts, + ) + expect(del.decision).toBe('allow') + expect(del.filesystemEffects?.[0]?.path).toBe('/work/repo/src/gone.ts') + }) + + it('records rename-to target paths and routes them through policy', () => { + const artifacts = new ArtifactRegistry() + const patch = [ + 'rename from src/old-name.ts', + 'rename to src/new-name.ts', + ].join('\n') + const ok = assessTool( + execution('apply_patch', { patch: ['--- a/src/old-name.ts', '+++ b/src/new-name.ts', '@@ -0,0 +1 @@', '+x'].join('\n') + '\n' + patch }), + roots, + artifacts, + ) + expect(ok.decision).toBe('allow') + const destructive = assessTool( + execution('apply_patch', { patch: ['--- a/.ssh/x', '+++ b/.ssh/x', '@@ -1 +1 @@', '-old', '+new', '\n', 'rename to .ssh/danger'].join('\n') }), + roots, + artifacts, + ) + expect(destructive.decision).toBe('ask') + }) + + it('hard-deny takes precedence when args.file_path and patch text disagree', () => { + const roots = resolveRoots('/work/repo', { home: '/home/dev', dshHome: '/safe/dsh', tempRoots: ['/tmp'] }) + const patch = ['--- a/src/ok.ts', '+++ b/src/ok.ts', '@@ -1 +1 @@', '-old', '+new'].join('\n') + expect(hardDenyReason(execution('apply_patch', { patch, file_path: '/safe/dsh/settings.yaml' }), roots)).toMatch(/DSH_HOME/) + }) +``` + +Verify that `roots` is already in scope (from the existing top-of-file `const roots = resolveRoots(...)`). If not, declare it at the top of the new `it` calls. + +- [ ] **Step 3: Run the new tests and confirm they all fail (or some leak through with the wrong reason)** + +Run: `cd /home/tt-wsl-ubuntu/skills-repo/dsh-auto-mode-ref && timeout 120 npx vitest run tests/policy.spec.ts -t "apply_patch" 2>&1 | tail -40` +Expected: every `it` in this step fails because no `apply_patch` branch exists in `assessTool` and `hardDenyReason` does not parse `apply_patch` text. The failure messages will be `decision: 'allow'`/`decision: 'ask'` mismatches or undefined `hardDenyReason` returns where a reason string was expected. + +- [ ] **Step 4: Add `apply_patch` branch to `hardDenyReason`** + +In `src/policy.ts`, locate the existing `hardDenyReason` function (currently starting at `src/policy.ts:134`). Insert **after** the existing `['write', 'edit', 'apply_patch']` block and **before** the standalone `DESTRUCTIVE_TOOL` block — i.e., immediately after line 149's closing `}` (end of the path-only block). Add: + +```ts + if (exec.name === 'apply_patch') { + const argsForPatch = record(exec.arguments) + const patch = typeof argsForPatch?.patch === 'string' + ? argsForPatch.patch + : typeof argsForPatch?.input === 'string' ? argsForPatch.input : undefined + if (patch !== undefined) { + const targets = extractApplyPatchPaths(patch) + for (const raw of targets) { + const normalized = normalizePath(raw, roots.workspace, roots.home) + const reason = hardDestructiveTargetReason(normalized, roots) + if (reason !== undefined) return `apply_patch targets ${reason}` + } + } + } +``` + +This mirrors the existing write/edit path-only block: `pathArgument(args)` was returning `undefined` for `apply_patch` because the file path lives inside the patch text; the new code reads the patch text, parses out every target path, normalizes each, and asks the existing hard-deny fuse whether any target is destructive. If any target is destructive, the patch is hard-denied. + +- [ ] **Step 5: Add `apply_patch` branch to `assessTool`** + +In `src/policy.ts`, locate `assessTool` (currently starting at `src/policy.ts:161`). The new branch must be placed **after** the `write/edit` block (line 204 closing brace) and **before** the `str_replace_editor` block (line 207 onwards). Add: + +```ts + if (exec.name === 'apply_patch') { + const argsForPatch = record(exec.arguments) + const patch = typeof argsForPatch?.patch === 'string' + ? argsForPatch.patch + : typeof argsForPatch?.input === 'string' ? argsForPatch.input : undefined + if (patch === undefined) { + return { decision: 'ask', reason: 'apply_patch payload is missing', classifierEligible: false } + } + const targets = extractApplyPatchPaths(patch) + if (targets.length === 0) { + return { + decision: 'ask', + reason: 'apply_patch text cannot be parsed for target paths; manual approval required', + classifierEligible: false, + } + } + for (const raw of targets) { + const normalized = normalizePath(raw, roots.workspace, roots.home) + if (isProtectedProjectPath(normalized, roots)) { + return { + decision: 'ask', + reason: `apply_patch targets protected project metadata: ${normalized}`, + classifierEligible: true, + filesystemEffects: targets.map(p => ({ + kind: 'create-or-overwrite' as const, + path: normalizePath(p, roots.workspace, roots.home), + existedBefore: existedBefore(normalizePath(p, roots.workspace, roots.home)), + })), + } + } + } + const effects = targets.map(p => { + const n = normalizePath(p, roots.workspace, roots.home) + return { kind: 'create-or-overwrite' as const, path: n, existedBefore: existedBefore(n) } + }) + return { + decision: 'allow', + reason: 'apply_patch inside workspace is delegated to the filesystem sandbox', + classifierEligible: false, + filesystemEffects: effects, + } + } +``` + +Add at the top of `src/policy.ts` (next to the existing imports from `./paths.js`): + +```ts +import { + extractApplyPatchPaths, + hardDestructiveTargetReason, + isProtectedProjectPath, + isWithin, + normalizePath, + type PolicyRoots, +} from './paths.js' +``` + +(Note: the existing import only imports four symbols. The new `apply_patch` branch needs `extractApplyPatchPaths` too; this is the only `paths.js` import change.) + +- [ ] **Step 6: Run all `apply_patch` tests and confirm they pass** + +Run: `cd /home/tt-wsl-ubuntu/skills-repo/dsh-auto-mode-ref && timeout 120 npx vitest run tests/policy.spec.ts -t "apply_patch" 2>&1 | tail -40` +Expected: all 10 new `apply_patch` tests pass. + +If anything fails: +- `decision: 'deny'` mismatches on the rename case typically indicate `extractApplyPatchPaths` returned `[]` for `rename to` headers; verify Task 1's parse handles `rename to ` (with the trailing space) on a line by itself. +- The "file_path takes precedence" test relies on `hardDenyReason` executing the `['write', 'edit', 'apply_patch']` block **before** the new `apply_patch`-text block. The insertion point in Step 4 is exactly between those two existing blocks; if you accidentally inserted below the `DESTRUCTIVE_TOOL` block, re-order. + +- [ ] **Step 7: Commit** + +```bash +cd /home/tt-wsl-ubuntu/skills-repo/dsh-auto-mode-ref +git add src/policy.ts tests/policy.spec.ts +git commit -m "feat(policy): route apply_patch through hardDenyReason and assessTool + +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." +``` + +--- + +## Task 5: `web_fetch` URL credential detection in `src/policy.ts` + +**Files:** +- Modify: `src/policy.ts` — add `urlContainsCredential` helper and a new block in `hardDenyReason`. +- Modify: `tests/policy.spec.ts` — new `it` blocks for the URL detection matrix. + +**Interfaces:** +- Consumes: nothing. +- Produces: `function urlContainsCredential(url: string): boolean` (module-private) and a new decision: `external URL contains credential-shaped query parameter`. + +- [ ] **Step 1: Write the failing tests** + +Append the following `it` calls inside the existing `describe('tool policy')` block in `tests/policy.spec.ts`: + +```ts + it('hard-denies web_fetch URLs with credential-shaped long token values', () => { + const outbound = execution('web_fetch', { url: 'https://example.invalid/cb?token=longstring12345' }) + expect(hardDenyReason(outbound, roots)).toMatch(/credential-shaped query/) + }) + + it('hard-denies web_fetch URLs with a long hex sig parameter', () => { + const outbound = execution('web_fetch', { url: 'https://example.invalid/x?sig=deadbeefcafebabe1234567890abcdef12345678' }) + expect(hardDenyReason(outbound, roots)).toMatch(/credential-shaped query/) + }) + + it('does not deny web_fetch URLs with non-credential parameter names', () => { + const outbound = execution('web_fetch', { url: 'https://example.invalid/?q=hello' }) + expect(hardDenyReason(outbound, roots)).toBeUndefined() + }) + + it('does not deny web_fetch URLs whose credential-named parameter value is too short', () => { + const outbound = execution('web_fetch', { url: 'https://example.invalid/?token=hello' }) + expect(hardDenyReason(outbound, roots)).toBeUndefined() + }) + + it('does not deny web_fetch URLs with empty values', () => { + const outbound = execution('web_fetch', { url: 'https://example.invalid/?token=' }) + expect(hardDenyReason(outbound, roots)).toBeUndefined() + }) + + it('falls back to regex when the URL has no protocol (relative path)', () => { + const outbound = execution('web_fetch', { url: 'example.invalid/path?token=longstring12345' }) + expect(hardDenyReason(outbound, roots)).toMatch(/credential-shaped query/) + }) + + it('hard-denies deploy tool URLs with credential-shaped parameters (EXTERNAL_WRITE_TOOL path)', () => { + // `repo_push` matches EXTERNAL_WRITE_TOOL. + const outbound = execution('repo_push', { url: 'https://example.invalid/api?api_key=abcdef1234567890' }) + expect(hardDenyReason(outbound, roots)).toMatch(/credential-shaped query/) + }) + + it('does not deny URLs whose credential-named value is purely a 7-char opaque ID', () => { + const outbound = execution('web_fetch', { url: 'https://example.invalid/?token=short12' }) + expect(hardDenyReason(outbound, roots)).toBeUndefined() + }) +``` + +- [ ] **Step 2: Run the new tests and confirm they fail** + +Run: `cd /home/tt-wsl-ubuntu/skills-repo/dsh-auto-mode-ref && timeout 120 npx vitest run tests/policy.spec.ts -t "credential-shaped query" 2>&1 | tail -30` +Expected: every new `it` fails because `hardDenyReason` has no URL-detection block yet. Failure messages will be hard-deny string mismatches or `undefined` returns where a deny reason was expected. + +- [ ] **Step 3: Add the `urlContainsCredential` helper and the `hardDenyReason` block** + +In `src/policy.ts`, immediately after the existing `containsCredentialMaterial` function (around `src/policy.ts:43-46`) and **before** the `SandboxEscalationRequest` interface, add: + +```ts +/** Parameter names that are commonly used to carry credentials in URLs. */ +const URL_CREDENTIAL_KEYS = /(?:token|access_token|api[_-]?key|sig|signature|auth|authorization)/i + +/** + * Value-shape heuristic for credential-looking query-string values. Matches + * either a base64url-ish substring ≥ 8 chars (alphanumerics, `.`, `_`, `~`, + * `+`, `/`, `-`, `=`) or a hex digest ≥ 16 chars. + */ +const URL_CREDENTIAL_VALUE = /^(?:[A-Za-z0-9._~+\/=-]{8,}|[A-Fa-f0-9]{16,})$/ + +/** + * Returns true when the URL contains a query parameter whose name matches + * {@link URL_CREDENTIAL_KEYS} and whose value matches + * {@link URL_CREDENTIAL_VALUE}. When the URL cannot be parsed as absolute, + * a regex fallback over the raw text catches the same shape. + */ +function urlContainsCredential(url: string): boolean { + try { + const parsed = new URL(url) + for (const [key, value] of parsed.searchParams) { + if (!URL_CREDENTIAL_KEYS.test(key)) continue + if (URL_CREDENTIAL_VALUE.test(value)) return true + } + return false + } catch { + // Relative or malformed URL; fall through to the regex. + } + return /[?&](?:token|access_token|api[_-]?key|sig|signature|auth|authorization)=[^&\s"']{8,}/i.test(url) +} +``` + +In `hardDenyReason` (currently `src/policy.ts:134`), insert **immediately after** the existing `containsCredentialMaterial`-based early return (around line 137) and **before** the existing `bash`/`pwsh` block. Add: + +```ts + const argsForUrl = record(exec.arguments) + const argsUrl = typeof argsForUrl?.url === 'string' ? argsForUrl.url : undefined + if (argsUrl !== undefined + && (/^(?:web_fetch|curl|wget)/i.test(exec.name) || EXTERNAL_WRITE_TOOL.test(exec.name)) + && urlContainsCredential(argsUrl)) { + return 'external URL contains credential-shaped query parameter' + } +``` + +This mirrors the existing `containsCredentialMaterial` early-return predicate exactly: same tool filter, same OR fallthrough, just a different content check. + +- [ ] **Step 4: Run the new tests and confirm they pass** + +Run: `cd /home/tt-wsl-ubuntu/skills-repo/dsh-auto-mode-ref && timeout 120 npx vitest run tests/policy.spec.ts -t "credential-shaped query" 2>&1 | tail -40` +Expected: all 8 URL-hardening tests pass. + +If `tokens=short12` returns a deny when you expected no deny: +- The threshold is 8 chars from the base64url alphabet. `short12` has 7 chars but `0-9,a-z,A-Z,_,-,+,/,.,~,=` chars. `short12` = 7 chars total. Should NOT match `{8,}`. If it does, double-check the regex body. + +If the `?token=longstring12345` case doesn't fire: +- Check the test URL is exactly `https://example.invalid/cb?token=longstring12345`. The regex should match `?token=longstring12345` (key is `token`, value is `longstring12345` = 15 chars ≥ 8, all in the alphabet). + +- [ ] **Step 5: Run the full `tests/policy.spec.ts` file** + +Run: `cd /home/tt-wsl-ubuntu/skills-repo/dsh-auto-mode-ref && timeout 120 npx vitest run tests/policy.spec.ts 2>&1 | tail -30` +Expected: all tests pass. The original 14 tests, the 10 `apply_patch` tests from Task 4, and the 8 URL-hardening tests from this task. If a `describe('tool policy')` test that was previously passing now fails, the URL block was inserted at the wrong point and a non-credential tool call is being misclassified as a credential URL. + +- [ ] **Step 6: Commit** + +```bash +cd /home/tt-wsl-ubuntu/skills-repo/dsh-auto-mode-ref +git add src/policy.ts tests/policy.spec.ts +git commit -m "feat(policy): hard-deny external URLs with credential-shaped query parameters + +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." +``` + +--- + +## Task 6: Final verification (typecheck, suite, scope diff) + +**Files:** +- No new source files. No new test files. + +- [ ] **Step 1: Run the full vitest suite** + +Run: `cd /home/tt-wsl-ubuntu/skills-repo/dsh-auto-mode-ref && timeout 180 npx vitest run 2>&1 | tail -40` + +Expected: the suite reports either all tests passing, or two known-bad `*.spec.ts` files fail to load because of upstream's missing optional sandbox packages. Both outcomes are acceptable provided: + +- `tests/paths.spec.ts` — all tests pass (gained the 9 from Task 1). +- `tests/policy.spec.ts` — all tests pass (gained the 18 from Tasks 4 and 5). +- `tests/classifier.spec.ts` — all tests pass (10 from Task 2 + 7 from Task 3 + updates to lines 48-59). + +Known-bad (pre-existing, environment-only): `tests/sandbox-business.spec.ts`, `tests/windows-sandbox-business.spec.ts` — fail to load because upstream `nanmicoder/dsh-auto-mode` declares `@deepseek-ai/dsh-fs-sandbox`, `@deepseek-ai/dsh-pwsh-sandbox`, `@deepseek-ai/dsh-user-approval` etc. as `devDependencies` that the workspace's pre-existing `node_modules/` (cloned from `db3a2ed` before the upstream-added deps) does not contain. Re-run `pnpm install --prefer-offline` and `node_modules/` will be repopulated if needed; otherwise these failures are out of scope of this spec. + +- [ ] **Step 2: Run typecheck on the changed sources** + +The full project typecheck (`pnpm typecheck` or `npx tsc --noEmit && npx tsc -p tsconfig.client.json --noEmit`) will fail because of the pre-existing missing optional sandbox packages in `src/escalation.ts`. To verify our changes specifically, run a focused typecheck on just the three touched files: + +```bash +cd /home/tt-wsl-ubuntu/skills-repo/dsh-auto-mode-ref +npx tsc --noEmit --target es2022 --module nodenext --moduleResolution nodenext --strict \ + --skipLibCheck --noImplicitOverride --noUncheckedIndexedAccess --exactOptionalPropertyTypes \ + src/paths.ts src/policy.ts src/classifier.ts +``` + +Expected: zero diagnostics from the three touched files. + +If diagnostics appear: +- `Property 'extractApplyPatchPaths' does not exist on type 'typeof import("./paths.js")'` — your import line in `src/policy.ts` is missing the new symbol. +- `Module '"node:crypto"' has no exported member 'randomUUID'` — unrelated to this spec (would be in `src/dsh-classifier.ts` which we did not touch). + +- [ ] **Step 3: Verify scope (no new files in `src/`)** + +```bash +cd /home/tt-wsl-ubuntu/skills-repo/dsh-auto-mode-ref +git diff --stat 3437cdc HEAD -- src/ +``` + +Expected output (exact values may vary by one or two lines depending on how edits cluster): + +``` + src/classifier.ts | ~75 ++++++++++++---- + src/paths.ts | ~70 +++++++++++++++++++ + src/policy.ts | ~80 ++++++++++++++++---- + 3 files changed, ~225 insertions(+), ~30 deletions(-) +``` + +If you see additional files under `src/`, e.g. `src/escalation.ts`, that means an untracked touched file snuck in — revert it. If you see zero or near-zero diff in any of the three files, your change did not actually land. + +- [ ] **Step 4: Verify the touched test files compile against the new types** + +Run: +```bash +cd /home/tt-wsl-ubuntu/skills-repo/dsh-auto-mode-ref +npx tsc --noEmit --target es2022 --module nodenext --moduleResolution nodenext --strict \ + --skipLibCheck --noImplicitOverride --noUncheckedIndexedAccess --exactOptionalPropertyTypes \ + --rootDir . tests/classifier.spec.ts tests/policy.spec.ts tests/paths.spec.ts +``` + +Expected: zero diagnostics. The test files import the new exports (`redactClassifierText`, `extractApplyPatchPaths`) and the new return shapes must match. + +- [ ] **Step 5: Manual smoke against `pnpm verify` if dev dependencies are present** + +Optional, only if `pnpm install --prefer-offline` has fetched the upstream sandbox packages: + +```bash +cd /home/tt-wsl-ubuntu/skills-repo/dsh-auto-mode-ref +pnpm run typecheck +pnpm run test +``` + +Expected: same as Steps 1 + 2 — all of our tests pass; the two pre-existing sandbox-business file-load failures only happen when `node_modules/` does not include the upstream-added packages. + +- [ ] **Step 6: Commit any pending verification artifacts** + +If you discovered typos during Steps 1-4 and fixed them inline, commit them: + +```bash +cd /home/tt-wsl-ubuntu/skills-repo/dsh-auto-mode-ref +git add src/classifier.ts src/policy.ts src/paths.ts \ + tests/classifier.spec.ts tests/policy.spec.ts tests/paths.spec.ts +git diff --cached --quiet || git commit -m "fix(spec-impl): address verification-time adjustments + +$(git diff --cached --stat)" +``` + +If no changes were needed (typical case), this step is a no-op. + +- [ ] **Step 7: Tag a temporary local commit and surface the bundle** + +```bash +cd /home/tt-wsl-ubuntu/skills-repo/dsh-auto-mode-ref +git log --oneline 3437cdc..HEAD +git diff 3437cdc..HEAD --stat +``` + +Expected: 6 new commits (Tasks 1-5 + final fix if any), all on the same branch as `3437cdc`. The diff stat shows only: +- `src/classifier.ts` (Tasks 2, 3) +- `src/paths.ts` (Task 1) +- `src/policy.ts` (Tasks 4, 5) +- `tests/classifier.spec.ts` (Tasks 2, 3) +- `tests/policy.spec.ts` (Tasks 4, 5) +- `tests/paths.spec.ts` (Task 1) + +No other files. This is the acceptance bundle per spec §8.1. + +- [ ] **Step 8: Report back to the user** + +Run the following one-line summary command and copy the output into your response to the human: + +```bash +cd /home/tt-wsl-ubuntu/skills-repo/dsh-auto-mode-ref +git diff 3437cdc..HEAD --stat | tail -10 && \ +echo "---" && \ +git log 3437cdc..HEAD --oneline +``` + +Then write a short summary pointing the user at the 6 commits and the test outcomes. + +--- + +## End of Plan + +Total estimated time: ~3-4 hours for a subagent that has zero context (Tasks 1-3 ~1.5h, Tasks 4-5 ~1h, Task 6 ~30 min). Subagent-driven execution should produce this in roughly two review cycles per task. From 21b8f5656e80a812c903fe75f46605edfe7d5f73 Mon Sep 17 00:00:00 2001 From: AtropinolTT Date: Mon, 17 Aug 2026 16:18:02 +0800 Subject: [PATCH 4/9] feat(paths): single-pass parser for apply_patch target paths extractApplyPatchPaths walks the patch text once on a per-line basis, pairs every '--- a/' with the next '+++ b/' (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). --- src/paths.ts | 75 +++++++++++++++++++++++++++++++++++ tests/paths.spec.ts | 97 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 172 insertions(+) diff --git a/src/paths.ts b/src/paths.ts index 3e6fb0d..5a0fd33 100644 --- a/src/paths.ts +++ b/src/paths.ts @@ -177,3 +177,78 @@ export function isArtifactArea(target: string, roots: PolicyRoots): boolean { const normalized = normalizePath(target, roots.workspace, roots.home) return isWithin(roots.workspace, normalized) || roots.tempRoots.some(root => isWithin(root, normalized)) } + +/** + * Extract filesystem targets from a unified-diff style `apply_patch` payload. + * + * Walks the patch text once on a per-line basis and records: + * - the destination path of every `--- a/` + `+++ b/` pair + * (modify), + * - `+++ b/` when paired with `--- /dev/null` (new file), + * - `--- a/` when paired with `+++ /dev/null` (delete), + * - the destination path of every `rename from` / `rename to` pair. + * + * Returns deduplicated, order-preserved paths with any leading `a/` or `b/` + * stripped. Paths are NOT normalized here — every caller passes the result + * through `normalizePath` so workspace-relative and absolute paths resolve + * the same way. + * + * Returns `[]` for empty input or any patch text where no `---` header can + * be paired with a `+++` header. Callers treat that as fail-closed + * (manual approval) rather than as a successful empty parse. + */ +export function extractApplyPatchPaths(patch: string): string[] { + if (typeof patch !== 'string' || patch === '') return [] + const lines = patch.split(/\r?\n/) + const targets: string[] = [] + const seen = new Set() + + const record = (raw: string | undefined): void => { + if (raw === undefined) return + if (raw === '' || raw === '/dev/null') return + const stripped = raw.startsWith('a/') || raw.startsWith('b/') ? raw.slice(2) : raw + if (!seen.has(stripped)) { + seen.add(stripped) + targets.push(stripped) + } + } + + let i = 0 + while (i < lines.length) { + const line = lines[i] + if (line.startsWith('--- ')) { + const minusRaw = line.slice(4) + const minusPath = minusRaw === '/dev/null' + ? undefined + : minusRaw.startsWith('a/') ? minusRaw.slice(2) : minusRaw + + // Walk forward until we find the matching +++ header. + let j = i + 1 + while (j < lines.length && !lines[j].startsWith('+++ ')) j++ + if (j < lines.length) { + const plusRaw = lines[j].slice(4) + const plusPath = plusRaw === '/dev/null' + ? undefined + : plusRaw.startsWith('b/') ? plusRaw.slice(2) : plusRaw + + if (minusPath === undefined && plusPath !== undefined) { + // New file: --- /dev/null + +++ b/ ⇒ record plus. + record(plusPath) + } else if (plusPath === undefined && minusPath !== undefined) { + // Delete: --- a/ + +++ /dev/null ⇒ record minus. + record(minusPath) + } else if (plusPath !== undefined) { + // Modify: record the destination path. + record(plusPath) + } + i = j + 1 + continue + } + } else if (line.startsWith('rename to ')) { + record(line.slice('rename to '.length)) + } + i++ + } + + return targets +} diff --git a/tests/paths.spec.ts b/tests/paths.spec.ts index aa83a00..165233c 100644 --- a/tests/paths.spec.ts +++ b/tests/paths.spec.ts @@ -2,8 +2,10 @@ import { describe, expect, it } from 'vitest' import { canonicalizePosixSystemAlias, canonicalizeWindowsNamespace, + extractApplyPatchPaths, hardDestructiveTargetReason, isFilesystemRoot, + isProtectedProjectPath, isWithin, normalizePath, resolveRoots, @@ -70,4 +72,99 @@ describe('path policy', () => { expect(hardDestructiveTargetReason('/home/dev/.ssh/id_ed25519', roots)).toMatch(/critical/) expect(hardDestructiveTargetReason('/work/repo/src/a.ts', roots)).toBeUndefined() }) + + it('extracts single-file modification paths from a patch', () => { + const patch = [ + 'diff --git a/src/a.ts b/src/a.ts', + 'index 0123..4567 100644', + '--- a/src/a.ts', + '+++ b/src/a.ts', + '@@ -1,1 +1,1 @@', + '-old', + '+new', + ].join('\n') + expect(extractApplyPatchPaths(patch)).toEqual(['src/a.ts']) + }) + + it('records new file paths from --- /dev/null + +++ b/', () => { + const patch = [ + 'diff --git a/src/new.ts b/src/new.ts', + 'new file mode 100644', + 'index 0000000..1234567', + '--- /dev/null', + '+++ b/src/new.ts', + ].join('\n') + expect(extractApplyPatchPaths(patch)).toEqual(['src/new.ts']) + }) + + it('records deleted file paths from --- a/ + +++ /dev/null', () => { + const patch = [ + 'diff --git a/src/old.ts b/src/old.ts', + 'deleted file mode 100644', + 'index 1234567..0000000', + '--- a/src/old.ts', + '+++ /dev/null', + ].join('\n') + expect(extractApplyPatchPaths(patch)).toEqual(['src/old.ts']) + }) + + it('records all target paths in a multi-file patch, deduplicated, in order', () => { + const patch = [ + 'diff --git a/src/a.ts b/src/a.ts', + '--- a/src/a.ts', + '+++ b/src/a.ts', + '@@ -1 +1 @@', + '-old', + '+new', + 'diff --git a/src/b.ts b/src/b.ts', + '--- a/src/b.ts', + '+++ b/src/b.ts', + '@@ -1 +1 @@', + '-old', + '+new', + 'diff --git a/src/a.ts b/src/a.ts', + '--- a/src/a.ts', + '+++ b/src/a.ts', + '@@ -1 +1 @@', + '-old2', + '+new2', + ].join('\n') + expect(extractApplyPatchPaths(patch)).toEqual(['src/a.ts', 'src/b.ts']) + }) + + it('records the destination path from rename from/rename to pairs', () => { + const patch = [ + 'diff --git a/src/old-name.ts b/src/new-name.ts', + 'similarity index 100%', + 'rename from src/old-name.ts', + 'rename to src/new-name.ts', + ].join('\n') + expect(extractApplyPatchPaths(patch)).toEqual(['src/new-name.ts']) + }) + + it('returns [] for an unparseable patch (no +++ pair)', () => { + expect(extractApplyPatchPaths('--- a/x\n@@ -1 +1 @@\n-old\n')).toEqual([]) + }) + + it('returns [] when patch text is empty or non-string', () => { + expect(extractApplyPatchPaths('')).toEqual([]) + // @ts-expect-error: runtime contract accepts unknown inputs safely. + expect(extractApplyPatchPaths(undefined)).toEqual([]) + }) + + it('strips the optional leading a/ or b/ from recorded paths', () => { + const patch = [ + '--- a/foo.ts', + '+++ b/foo.ts', + ].join('\n') + expect(extractApplyPatchPaths(patch)).toEqual(['foo.ts']) + }) + + it('preserves raw paths (no normalization applied)', () => { + const patch = [ + '--- a/CAPS.ts', + '+++ b/CAPS.ts', + ].join('\n') + expect(extractApplyPatchPaths(patch)).toEqual(['CAPS.ts']) + }) }) From cc0b989b19e2308eb38b1db76bd2013a64280b92 Mon Sep 17 00:00:00 2001 From: AtropinolTT Date: Mon, 17 Aug 2026 16:29:48 +0800 Subject: [PATCH 5/9] feat(classifier): content-first credential redaction (Task 2/3 of spec) 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-]; 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. --- src/classifier.ts | 82 ++++++++++++++++++++++++++++++++++++--- tests/classifier.spec.ts | 83 +++++++++++++++++++++++++++++++++++++++- 2 files changed, 158 insertions(+), 7 deletions(-) diff --git a/src/classifier.ts b/src/classifier.ts index bded64d..8937839 100644 --- a/src/classifier.ts +++ b/src/classifier.ts @@ -32,13 +32,83 @@ export interface HttpClassifierConfig { const CONTENT_KEYS = /^(?:content|body|payload|data|text|old_string|new_string|description|justification)$/i const SECRET_KEYS = /(?:api|auth|access|secret|private|credential|password|token|cookie|authorization).*?(?:key|value|token)?$/i -/** Redact likely secrets and bound one classifier-visible text value. */ +interface CredentialPattern { + readonly name: string + readonly pattern: RegExp +} + +const CREDENTIAL_PATTERNS: readonly CredentialPattern[] = [ + // Order is most-specific-first so a broader pattern does not consume a + // token-shaped substring that a more specific pattern below should own. + // e.g. `token-suffix` matches the body of `sk-ant-api\d{2}-...`, so the + // Anthropic/GitHub/LLM patterns must run first to claim their tokens. + + // Anthropic keys are `sk-ant-apiNN-` where NN is exactly 2 digits + // and is 32+ base64url-ish chars. Anchoring `api\d{2}-` rejects + // documentation/strings of the shape `sk-ant-` and only + // matches real Anthropic key prefixes per vendor docs. + { name: 'anthropic-key', pattern: /\bsk-ant-api\d{2}-[A-Za-z0-9_-]{32,}\b/g }, + + // GitHub token formats (gho_ is 40; gh[pus]_ is 36; github_pat_ has 22+). + { name: 'github-oauth', pattern: /\bgho_[A-Za-z0-9]{40}\b/g }, + { name: 'github-classic', pattern: /\bgh[pus]_[A-Za-z0-9]{36}\b/g }, + { name: 'github-fine-pat', pattern: /\bgithub_pat_[A-Za-z0-9_]{22,}\b/g }, + + // AWS access-key IDs (16 chars after the prefix). + { name: 'aws-access-key', pattern: /\b(?:AKIA|ASIA)[A-Z0-9]{16}\b/g }, + + // LLM and tool vendor keys. + { name: 'llm-key', pattern: /\bsk-(?:proj-)?[A-Za-z0-9_-]{16,}\b/g }, + + // Broad fallback: any `sk|ghp|github_pat|xox[baprs]` followed by 8+ chars. + // Must come after the vendor-specific patterns above so they own their tokens. + { name: 'token-suffix', pattern: /\b(?:sk|ghp|github_pat|xox[baprs])[-_][A-Za-z0-9_-]{8,}\b/g }, + + { name: 'bearer', pattern: /\bBearer\s+[A-Za-z0-9._~+\/-]{8,}/gi }, + { name: 'key-value', pattern: /((?:api[_-]?key|token|secret|password)=)[^&\s]+/gi }, + + // PEM private-key blocks (RSA, OPENSSH, EC, ECDSA, DSA, ENCRYPTED). + // Matches the BEGIN line through the next matching END line, inclusive. + { name: 'pem-private-key', + pattern: /-----BEGIN (?:RSA |OPENSSH |EC |ECDSA |DSA |ENCRYPTED )?PRIVATE KEY-----[\s\S]*?-----END (?:RSA |OPENSSH |EC |ECDSA |DSA |ENCRYPTED )?PRIVATE KEY-----/g }, +] + +/** Redact pattern-matched credential content and surface which patterns fired. */ +export interface ClassifierRedaction { + readonly value: string + readonly redactedNames: readonly string[] +} + +/** + * Run every entry in {@link CREDENTIAL_PATTERNS} against the input. Each match + * is replaced with `[redacted-]`; each unique name that fires is appended + * to `redactedNames` in the order it appears in the patterns table. Truncated + * to `maxLength` when the redacted value would otherwise exceed it. + * + * This is content-first: a credential-shaped substring anywhere in the input is + * redacted regardless of the field name it sits under. Key-name matching is + * still applied one layer up in {@link sanitizeClassifierArguments} for + * defense-in-depth on `SECRET_KEYS`. + */ +export function redactClassifierText(value: string, maxLength = 1_000): ClassifierRedaction { + let current = value + const redactedNames: string[] = [] + for (const { name, pattern } of CREDENTIAL_PATTERNS) { + const before = current + current = current.replace(pattern, `[redacted-${name}]`) + if (current !== before && !redactedNames.includes(name)) redactedNames.push(name) + } + if (current.length > maxLength) current = current.slice(0, maxLength) + return { value: current, redactedNames } +} + +/** + * Public thin wrapper over {@link redactClassifierText}. The kept signature + * preserves consumer call sites in `src/index.ts` (`trustedUserMessages`, + * `sandboxRequest.justification`) — see spec §1.4. + */ export function sanitizeClassifierText(value: string): string { - return value - .replace(/\b(?:sk|ghp|github_pat|xox[baprs])[-_][A-Za-z0-9_-]{8,}\b/g, '[redacted-secret]') - .replace(/\bBearer\s+[A-Za-z0-9._~+\/-]{8,}/gi, 'Bearer [redacted-secret]') - .replace(/((?:api[_-]?key|token|secret|password)=)[^&\s]+/gi, '$1[redacted-secret]') - .slice(0, 1_000) + return redactClassifierText(value).value } /** Remove bulk content and likely secrets before crossing the classifier network boundary. */ diff --git a/tests/classifier.spec.ts b/tests/classifier.spec.ts index 94931fa..6a9153f 100644 --- a/tests/classifier.spec.ts +++ b/tests/classifier.spec.ts @@ -1,6 +1,6 @@ import { describe, expect, it, vi } from 'vitest' import type { GenerateOptions, StreamChunk } from '@deepseek-ai/dsh-llm' -import { CLASSIFIER_SYSTEM_PROMPT, createHttpClassifier, parseClassifierDecision, sanitizeClassifierArguments, sanitizeClassifierText } from '../src/classifier.js' +import { CLASSIFIER_SYSTEM_PROMPT, createHttpClassifier, parseClassifierDecision, redactClassifierText, sanitizeClassifierArguments, sanitizeClassifierText } from '../src/classifier.js' import { createDshClassifier } from '../src/dsh-classifier.js' const input = { @@ -141,3 +141,84 @@ describe('native DSH classifier', () => { expect(() => createDshClassifier(runtime, { timeoutMs: 1_000, provider: 'deepseek-official' })).toThrow(/together/) }) }) + +describe('redactClassifierText', () => { + it('redacts AKIA / ASIA AWS access-key IDs', () => { + const result = redactClassifierText('export AWS_ACCESS_KEY_ID=AKIAIOSFODNN7EXAMPLE') + expect(result.value).toBe('export AWS_ACCESS_KEY_ID=[redacted-aws-access-key]') + expect(result.redactedNames).toContain('aws-access-key') + }) + + it('redacts github classic / user / server PATs (gh[pus]_<36>)', () => { + const result = redactClassifierText('token = ghp_012345678901234567890123456789012345') + expect(result.value).toBe('token = [redacted-github-classic]') + expect(result.redactedNames).toContain('github-classic') + const user = redactClassifierText('token = ghu_012345678901234567890123456789012345') + expect(user.value).toBe('token = [redacted-github-classic]') + const server = redactClassifierText('token = ghs_012345678901234567890123456789012345') + expect(server.value).toBe('token = [redacted-github-classic]') + }) + + it('redacts gho_ github OAuth access tokens (40 chars)', () => { + const result = redactClassifierText('token = gho_0123456789abcdef0123456789abcdef01234567') + expect(result.value).toBe('token = [redacted-github-oauth]') + expect(result.redactedNames).toContain('github-oauth') + }) + + it('redacts github_pat_ fine-grained PATs (22+ chars body)', () => { + const result = redactClassifierText('token = github_pat_0123456789abcdef_0123456789abcdef') + expect(result.value).toBe('token = [redacted-github-fine-pat]') + expect(result.redactedNames).toContain('github-fine-pat') + }) + + it('redacts sk- / sk-proj- LLM API keys (16+ chars)', () => { + const plain = redactClassifierText('sk-0123456789abcdef0123456789abcdef') + expect(plain.value).toBe('[redacted-llm-key]') + expect(plain.redactedNames).toContain('llm-key') + const proj = redactClassifierText('sk-proj-0123456789abcdef0123456789abcdef') + expect(proj.value).toBe('[redacted-llm-key]') + }) + + it('redacts Anthropic keys only when anchored to sk-ant-api-<32+>', () => { + // Real Anthropic key (api03 + 40-char body) — matches. + const real = redactClassifierText('sk-ant-api03-abcdefghijklmnopqrstuvwxyz1234567890') + expect(real.value).toBe('[redacted-anthropic-key]') + // Documentation strings without the api- prefix — DO NOT match. + expect(redactClassifierText('uses sk-ant-abcdefghij1234567890 in config').redactedNames) + .not.toContain('anthropic-key') + // Single-digit api version — does NOT match. + expect(redactClassifierText('sk-ant-api3-abcdefghijklmnopqrstuvwxyz123456').redactedNames) + .not.toContain('anthropic-key') + }) + + it('redacts a complete PEM private-key block (BEGIN ... END inclusive)', () => { + const pem = [ + '-----BEGIN OPENSSH PRIVATE KEY-----', + 'abc', + 'def', + '-----END OPENSSH PRIVATE KEY-----', + ].join('\n') + const result = redactClassifierText(pem) + expect(result.value).toBe('[redacted-pem-private-key]') + expect(result.redactedNames).toContain('pem-private-key') + }) + + it('redacts multiple credential shapes in one input and lists each name once', () => { + const input = 'AKIAIOSFODNN7EXAMPLE sk-ant-api03-abcabcabcabcabcabcabcabcabcabc12AKIAL' + const result = redactClassifierText(input) + expect(result.redactedNames).toContain('aws-access-key') + expect(result.redactedNames).toContain('anthropic-key') + }) + + it('emits the input unchanged when no pattern matches', () => { + const input = 'just a plain string with no credentials' + const result = redactClassifierText(input) + expect(result.value).toBe(input) + expect(result.redactedNames).toEqual([]) + }) + + it('truncates the redacted value to maxLength when supplied', () => { + const result = redactClassifierText('A'.repeat(2000), 100) + expect(result.value.length).toBe(100) + }) +}) From 61e1e6617f90fe9673918d0afe1fe8e66ebb65f9 Mon Sep 17 00:00:00 2001 From: AtropinolTT Date: Mon, 17 Aug 2026 16:39:25 +0800 Subject: [PATCH 6/9] feat(classifier): pattern-redact every string at any depth; drop CONTENT_KEYS MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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-: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-#4 reply (sanit_classifier.ts calls now reach old_str/new_str/file_text payload lines through the recursive redaction). --- src/classifier.ts | 5 +-- tests/classifier.spec.ts | 66 ++++++++++++++++++++++++++++++++++++++-- 2 files changed, 64 insertions(+), 7 deletions(-) diff --git a/src/classifier.ts b/src/classifier.ts index 8937839..00f1c54 100644 --- a/src/classifier.ts +++ b/src/classifier.ts @@ -29,7 +29,6 @@ export interface HttpClassifierConfig { readonly fetchImpl?: typeof fetch } -const CONTENT_KEYS = /^(?:content|body|payload|data|text|old_string|new_string|description|justification)$/i const SECRET_KEYS = /(?:api|auth|access|secret|private|credential|password|token|cookie|authorization).*?(?:key|value|token)?$/i interface CredentialPattern { @@ -114,7 +113,7 @@ export function sanitizeClassifierText(value: string): string { /** Remove bulk content and likely secrets before crossing the classifier network boundary. */ export function sanitizeClassifierArguments(value: unknown, depth = 0): unknown { if (depth > 3) return '[truncated-depth]' - if (typeof value === 'string') return sanitizeClassifierText(value) + if (typeof value === 'string') return redactClassifierText(value).value if (typeof value === 'number' || typeof value === 'boolean' || value === null) return value if (Array.isArray(value)) return value.slice(0, 25).map(item => sanitizeClassifierArguments(item, depth + 1)) if (typeof value !== 'object') return `[${typeof value}]` @@ -122,8 +121,6 @@ export function sanitizeClassifierArguments(value: unknown, depth = 0): unknown for (const [key, entry] of Object.entries(value).slice(0, 50)) { if (SECRET_KEYS.test(key)) { output[key] = '[redacted-secret-field]' - } else if (CONTENT_KEYS.test(key) && typeof entry === 'string') { - output[key] = `[redacted-${key}:${entry.length}-chars]` } else { output[key] = sanitizeClassifierArguments(entry, depth + 1) } diff --git a/tests/classifier.spec.ts b/tests/classifier.spec.ts index 6a9153f..9362a10 100644 --- a/tests/classifier.spec.ts +++ b/tests/classifier.spec.ts @@ -51,11 +51,11 @@ describe('HTTP classifier', () => { content: 'repository payload', apiKey: 'sk-example-secret', })).toEqual({ - command: 'curl -H "Authorization: Bearer [redacted-secret]" https://example.invalid', - content: '[redacted-content:18-chars]', + command: 'curl -H "Authorization: [redacted-bearer]" https://example.invalid', + content: 'repository payload', apiKey: '[redacted-secret-field]', }) - expect(sanitizeClassifierText('please use sk-example-secret-value for the test')).toBe('please use [redacted-secret] for the test') + expect(sanitizeClassifierText('please use sk-example-secret-value for the test')).toBe('please use [redacted-llm-key] for the test') }) }) @@ -222,3 +222,63 @@ describe('redactClassifierText', () => { expect(result.value.length).toBe(100) }) }) + +describe('sanitizeClassifierArguments', () => { + it('redacts a top-level credential-shaped string value but leaves neighbors alone', () => { + const out = sanitizeClassifierArguments({ + command: 'curl https://example.invalid', + note: 'use AKIAIOSFODNN7EXAMPLE in production', + }) as Record + expect(out.command).toBe('curl https://example.invalid') + expect(out.note).toBe('use [redacted-aws-access-key] in production') + }) + + it('redacts string credentials nested inside arrays and objects', () => { + const out = sanitizeClassifierArguments({ + env: [ + 'GITHUB_TOKEN=ghp_012345678901234567890123456789012345', + 'OTHER=plain', + ], + nested: { + body: 'token = AKIAIOSFODNN7EXAMPLE', + }, + }) + expect(out).toEqual({ + env: [ + 'GITHUB_[redacted-key-value]', + 'OTHER=plain', + ], + nested: { + body: 'token = [redacted-aws-access-key]', + }, + }) + }) + + it('wholesale-redacts SECRET_KEYS-matched keys', () => { + const out = sanitizeClassifierArguments({ apiKey: 'sk-example-secret' }) as Record + expect(out.apiKey).toBe('[redacted-secret-field]') + }) + + it('does NOT truncate CONTENT_KEYS-matched fields any more (content keys emit plain text after pattern scan)', () => { + const out = sanitizeClassifierArguments({ content: 'repository payload' }) as Record + expect(out.content).toBe('repository payload') + }) + + it('preserves non-string scalars at depth 0', () => { + expect(sanitizeClassifierArguments({ count: 7, flag: true, missing: null })) + .toEqual({ count: 7, flag: true, missing: null }) + }) + + it('caps array size at 25 entries', () => { + const arr = Array.from({ length: 30 }, (_, i) => `item-${i}`) + const out = sanitizeClassifierArguments({ list: arr }) as { list: unknown[] } + expect(out.list.length).toBe(25) + }) + + it('caps object key count at 50 entries', () => { + const obj: Record = {} + for (let i = 0; i < 60; i++) obj[`k${i}`] = `v${i}` + const out = sanitizeClassifierArguments(obj) as Record + expect(Object.keys(out).length).toBe(50) + }) +}) From 70c1d44a627db2c5f97fef26904eb8423227001e Mon Sep 17 00:00:00 2001 From: AtropinolTT Date: Mon, 17 Aug 2026 17:13:25 +0800 Subject: [PATCH 7/9] feat(policy): route apply_patch through hardDenyReason and assessTool MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- src/policy.ts | 57 ++++++++++++++++++++ tests/policy.spec.ts | 124 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 181 insertions(+) diff --git a/src/policy.ts b/src/policy.ts index 9a0406d..6d1adf2 100644 --- a/src/policy.ts +++ b/src/policy.ts @@ -2,6 +2,7 @@ import { lstatSync } from 'node:fs' import type { ToolExecution } from '@deepseek-ai/dsh-tools' import type { ArtifactRegistry } from './artifacts.js' import { + extractApplyPatchPaths, hardDestructiveTargetReason, isProtectedProjectPath, isWithin, @@ -147,6 +148,20 @@ export function hardDenyReason(exec: Readonly, roots: PolicyRoots if (reason !== undefined) return `mutation targets ${reason}` } } + if (exec.name === 'apply_patch') { + const argsForPatch = record(exec.arguments) + const patch = typeof argsForPatch?.patch === 'string' + ? argsForPatch.patch + : typeof argsForPatch?.input === 'string' ? argsForPatch.input : undefined + if (patch !== undefined) { + const targets = extractApplyPatchPaths(patch) + for (const raw of targets) { + const normalized = normalizePath(raw, roots.workspace, roots.home) + const reason = hardDestructiveTargetReason(normalized, roots) + if (reason !== undefined) return `apply_patch targets ${reason}` + } + } + } if (DESTRUCTIVE_TOOL.test(exec.name)) { const path = pathArgument(args) if (path !== undefined) { @@ -203,6 +218,48 @@ export function assessTool(exec: Readonly, roots: PolicyRoots, ar } } + if (exec.name === 'apply_patch') { + const argsForPatch = record(exec.arguments) + const patch = typeof argsForPatch?.patch === 'string' + ? argsForPatch.patch + : typeof argsForPatch?.input === 'string' ? argsForPatch.input : undefined + if (patch === undefined) { + return { decision: 'ask', reason: 'apply_patch payload is missing', classifierEligible: false } + } + const targets = extractApplyPatchPaths(patch) + if (targets.length === 0) { + return { + decision: 'ask', + reason: 'apply_patch text cannot be parsed for target paths; manual approval required', + classifierEligible: false, + } + } + for (const raw of targets) { + const normalized = normalizePath(raw, roots.workspace, roots.home) + if (isProtectedProjectPath(normalized, roots)) { + return { + decision: 'ask', + reason: `apply_patch targets protected project metadata: ${normalized}`, + classifierEligible: true, + filesystemEffects: targets.map(p => ({ + kind: 'create-or-overwrite' as const, + path: normalizePath(p, roots.workspace, roots.home), + existedBefore: existedBefore(normalizePath(p, roots.workspace, roots.home)), + })), + } + } + } + const effects = targets.map(p => { + const n = normalizePath(p, roots.workspace, roots.home) + return { kind: 'create-or-overwrite' as const, path: n, existedBefore: existedBefore(n) } + }) + return { + decision: 'allow', + reason: 'apply_patch inside workspace is delegated to the filesystem sandbox', + classifierEligible: false, + filesystemEffects: effects, + } + } if (exec.name === 'str_replace_editor') { const command = args?.command diff --git a/tests/policy.spec.ts b/tests/policy.spec.ts index ebd9d8e..33ff305 100644 --- a/tests/policy.spec.ts +++ b/tests/policy.spec.ts @@ -107,4 +107,128 @@ describe('tool policy', () => { expect(assessTool(execution('str_replace_editor', { command: 'create', path: '/work/repo/generated.ts' }), roots, artifacts)) .toMatchObject({ plannedCreates: ['/work/repo/generated.ts'] }) }) + + it('hard-denies apply_patch when any target is destructive', () => { + const roots = resolveRoots('/work/repo', { home: '/home/dev', dshHome: '/safe/dsh', tempRoots: ['/tmp'] }) + const patch = [ + '--- a/src/ok.ts', + '+++ b/src/ok.ts', + '@@ -1 +1 @@', + '-old', + '+new', + '--- /home/dev/.ssh/id_rsa', + '+++ /home/dev/.ssh/id_rsa', + '@@ -1 +1 @@', + '-old', + '+new', + ].join('\n') + const exec = execution('apply_patch', { patch }) + expect(hardDenyReason(exec, roots)).toMatch(/credential|critical|root|DSH_HOME/) + }) + + it('hard-denies apply_patch targeting DSH_HOME', () => { + const roots = resolveRoots('/work/repo', { home: '/home/dev', dshHome: '/safe/dsh', tempRoots: ['/tmp'] }) + const patch = ['--- a/x', '+++ b/x', '@@ -1 +1 @@', '-old', '+new'].join('\n') + const exec = execution('apply_patch', { patch, file_path: '/safe/dsh/settings.yaml' }) + expect(hardDenyReason(exec, roots)).toMatch(/DSH_HOME/) + }) + + it('returns undefined for a benign apply_patch under hardDenyReason', () => { + const roots = resolveRoots('/work/repo', { home: '/home/dev', dshHome: '/safe/dsh', tempRoots: ['/tmp'] }) + const patch = ['--- a/src/ok.ts', '+++ b/src/ok.ts', '@@ -1 +1 @@', '-old', '+new'].join('\n') + expect(hardDenyReason(execution('apply_patch', { patch }), roots)).toBeUndefined() + }) + + it('allows a happy-path apply_patch on a workspace file with filesystemEffects', () => { + const artifacts = new ArtifactRegistry() + const out = assessTool( + execution('apply_patch', { patch: ['--- a/src/ok.ts', '+++ b/src/ok.ts', '@@ -1 +1 @@', '-old', '+new'].join('\n') }), + roots, + artifacts, + ) + expect(out.decision).toBe('allow') + expect(out.classifierEligible).toBe(false) + expect(out.filesystemEffects).toEqual([ + { kind: 'create-or-overwrite', path: '/work/repo/src/ok.ts', existedBefore: expect.any(Boolean) as unknown as boolean }, + ]) + }) + + it('asks for apply_patch targeting protected project metadata', () => { + const artifacts = new ArtifactRegistry() + const out = assessTool( + execution('apply_patch', { patch: ['--- a/.git/config', '+++ b/.git/config', '@@ -1 +1 @@', '-old', '+new'].join('\n') }), + roots, + artifacts, + ) + expect(out).toMatchObject({ decision: 'ask', classifierEligible: true }) + expect(out.filesystemEffects?.[0]?.path).toBe('/work/repo/.git/config') + }) + + it('asks with classifierEligible false for an unparseable apply_patch (fail-closed)', () => { + const artifacts = new ArtifactRegistry() + const out = assessTool( + execution('apply_patch', { patch: 'this is not a patch at all' }), + roots, + artifacts, + ) + expect(out).toMatchObject({ decision: 'ask', classifierEligible: false }) + }) + + it('asks with classifierEligible false when apply_patch payload is missing', () => { + const artifacts = new ArtifactRegistry() + const out = assessTool(execution('apply_patch', {}), roots, artifacts) + expect(out).toMatchObject({ decision: 'ask', classifierEligible: false, reason: expect.stringMatching(/missing/) as unknown as string }) + }) + + it('reads apply_patch payloads from args.input as well as args.patch', () => { + const artifacts = new ArtifactRegistry() + const patch = ['--- a/src/ok.ts', '+++ b/src/ok.ts', '@@ -1 +1 @@', '-old', '+new'].join('\n') + const outFromInput = assessTool(execution('apply_patch', { input: patch }), roots, artifacts) + expect(outFromInput.decision).toBe('allow') + }) + + it('handles create (--- /dev/null + +++ b/path) and delete (--- a/path + +++ /dev/null)', () => { + const artifacts = new ArtifactRegistry() + const create = assessTool( + execution('apply_patch', { patch: ['--- /dev/null', '+++ b/src/new.ts', '@@ -0,0 +1 @@', '+new'].join('\n') }), + roots, + artifacts, + ) + expect(create.decision).toBe('allow') + expect(create.filesystemEffects?.[0]?.path).toBe('/work/repo/src/new.ts') + + const del = assessTool( + execution('apply_patch', { patch: ['--- a/src/gone.ts', '+++ /dev/null', '@@ -1 +0,0 @@', '-gone'].join('\n') }), + roots, + artifacts, + ) + expect(del.decision).toBe('allow') + expect(del.filesystemEffects?.[0]?.path).toBe('/work/repo/src/gone.ts') + }) + + it('records rename-to target paths and routes them through policy', () => { + const artifacts = new ArtifactRegistry() + const patch = [ + 'rename from src/old-name.ts', + 'rename to src/new-name.ts', + ].join('\n') + const ok = assessTool( + execution('apply_patch', { patch: ['--- a/src/old-name.ts', '+++ b/src/new-name.ts', '@@ -0,0 +1 @@', '+x'].join('\n') + '\n' + patch }), + roots, + artifacts, + ) + expect(ok.decision).toBe('allow') + const destructive = assessTool( + execution('apply_patch', { patch: ['--- a/.git/x', '+++ b/.git/x', '@@ -1 +1 @@', '-old', '+new', '\n', 'rename to .git/danger'].join('\n') }), + roots, + artifacts, + ) + expect(destructive.decision).toBe('ask') + }) + + it('hard-deny takes precedence when args.file_path and patch text disagree', () => { + const roots = resolveRoots('/work/repo', { home: '/home/dev', dshHome: '/safe/dsh', tempRoots: ['/tmp'] }) + const patch = ['--- a/src/ok.ts', '+++ b/src/ok.ts', '@@ -1 +1 @@', '-old', '+new'].join('\n') + expect(hardDenyReason(execution('apply_patch', { patch, file_path: '/safe/dsh/settings.yaml' }), roots)).toMatch(/DSH_HOME/) + }) }) From b845c9b68b21e5b073d91eb70347788c9b43ffb8 Mon Sep 17 00:00:00 2001 From: AtropinolTT Date: Mon, 17 Aug 2026 17:19:33 +0800 Subject: [PATCH 8/9] feat(policy): hard-deny external URLs with credential-shaped query parameters 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. --- src/policy.ts | 37 +++++++++++++++++++++++++++++++++++++ tests/policy.spec.ts | 41 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 78 insertions(+) diff --git a/src/policy.ts b/src/policy.ts index 6d1adf2..4456526 100644 --- a/src/policy.ts +++ b/src/policy.ts @@ -46,6 +46,36 @@ function containsCredentialMaterial(argumentsValue: unknown): boolean { .test(serializedArguments(argumentsValue)) } +/** Parameter names that are commonly used to carry credentials in URLs. */ +const URL_CREDENTIAL_KEYS = /(?:token|access_token|api[_-]?key|sig|signature|auth|authorization)/i + +/** + * Value-shape heuristic for credential-looking query-string values. Matches + * either a base64url-ish substring ≥ 8 chars (alphanumerics, `.`, `_`, `~`, + * `+`, `/`, `-`, `=`) or a hex digest ≥ 16 chars. + */ +const URL_CREDENTIAL_VALUE = /^(?:[A-Za-z0-9._~+\/=-]{8,}|[A-Fa-f0-9]{16,})$/ + +/** + * Returns true when the URL contains a query parameter whose name matches + * {@link URL_CREDENTIAL_KEYS} and whose value matches + * {@link URL_CREDENTIAL_VALUE}. When the URL cannot be parsed as absolute, + * a regex fallback over the raw text catches the same shape. + */ +function urlContainsCredential(url: string): boolean { + try { + const parsed = new URL(url) + for (const [key, value] of parsed.searchParams) { + if (!URL_CREDENTIAL_KEYS.test(key)) continue + if (URL_CREDENTIAL_VALUE.test(value)) return true + } + return false + } catch { + // Relative or malformed URL; fall through to the regex. + } + return /[?&](?:token|access_token|api[_-]?key|sig|signature|auth|authorization)=[^&\s"']{8,}/i.test(url) +} + /** One model-requested, tool-native widening of the standing workspace sandbox. */ export interface SandboxEscalationRequest { readonly requestedMode: string @@ -137,6 +167,13 @@ export function hardDenyReason(exec: Readonly, roots: PolicyRoots if ((/^(?:web_fetch|curl|wget)/i.test(exec.name) || EXTERNAL_WRITE_TOOL.test(exec.name)) && containsCredentialMaterial(exec.arguments)) { return 'external call contains credential or private-key material' } + const argsForUrl = record(exec.arguments) + const argsUrl = typeof argsForUrl?.url === 'string' ? argsForUrl.url : undefined + if (argsUrl !== undefined + && (/^(?:web_fetch|curl|wget)/i.test(exec.name) || EXTERNAL_WRITE_TOOL.test(exec.name)) + && urlContainsCredential(argsUrl)) { + return 'external URL contains credential-shaped query parameter' + } if ((exec.name === 'bash' || exec.name === 'pwsh') && typeof args?.command === 'string') { return hardDenyShellReason(args.command, exec.name, roots) } diff --git a/tests/policy.spec.ts b/tests/policy.spec.ts index 33ff305..1978e9d 100644 --- a/tests/policy.spec.ts +++ b/tests/policy.spec.ts @@ -231,4 +231,45 @@ describe('tool policy', () => { const patch = ['--- a/src/ok.ts', '+++ b/src/ok.ts', '@@ -1 +1 @@', '-old', '+new'].join('\n') expect(hardDenyReason(execution('apply_patch', { patch, file_path: '/safe/dsh/settings.yaml' }), roots)).toMatch(/DSH_HOME/) }) + + it('hard-denies web_fetch URLs with credential-shaped long token values', () => { + const outbound = execution('web_fetch', { url: 'https://example.invalid/cb?token=longstring12345' }) + expect(hardDenyReason(outbound, roots)).toMatch(/credential-shaped query/) + }) + + it('hard-denies web_fetch URLs with a long hex sig parameter', () => { + const outbound = execution('web_fetch', { url: 'https://example.invalid/x?sig=deadbeefcafebabe1234567890abcdef12345678' }) + expect(hardDenyReason(outbound, roots)).toMatch(/credential-shaped query/) + }) + + it('does not deny web_fetch URLs with non-credential parameter names', () => { + const outbound = execution('web_fetch', { url: 'https://example.invalid/?q=hello' }) + expect(hardDenyReason(outbound, roots)).toBeUndefined() + }) + + it('does not deny web_fetch URLs whose credential-named parameter value is too short', () => { + const outbound = execution('web_fetch', { url: 'https://example.invalid/?token=hello' }) + expect(hardDenyReason(outbound, roots)).toBeUndefined() + }) + + it('does not deny web_fetch URLs with empty values', () => { + const outbound = execution('web_fetch', { url: 'https://example.invalid/?token=' }) + expect(hardDenyReason(outbound, roots)).toBeUndefined() + }) + + it('falls back to regex when the URL has no protocol (relative path)', () => { + const outbound = execution('web_fetch', { url: 'example.invalid/path?token=longstring12345' }) + expect(hardDenyReason(outbound, roots)).toMatch(/credential-shaped query/) + }) + + it('hard-denies deploy tool URLs with credential-shaped parameters (EXTERNAL_WRITE_TOOL path)', () => { + // `repo_push` matches EXTERNAL_WRITE_TOOL. + const outbound = execution('repo_push', { url: 'https://example.invalid/api?api_key=abcdef1234567890' }) + expect(hardDenyReason(outbound, roots)).toMatch(/credential-shaped query/) + }) + + it('does not deny URLs whose credential-named value is purely a 7-char opaque ID', () => { + const outbound = execution('web_fetch', { url: 'https://example.invalid/?token=short12' }) + expect(hardDenyReason(outbound, roots)).toBeUndefined() + }) }) From fe72191ff55392d78d1fe318d430202213c0887c Mon Sep 17 00:00:00 2001 From: AtropinolTT Date: Mon, 17 Aug 2026 17:25:08 +0800 Subject: [PATCH 9/9] fix(paths): satisfy noUncheckedIndexedAccess in extractApplyPatchPaths 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). --- src/paths.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/paths.ts b/src/paths.ts index 5a0fd33..ff4bdc1 100644 --- a/src/paths.ts +++ b/src/paths.ts @@ -215,7 +215,7 @@ export function extractApplyPatchPaths(patch: string): string[] { let i = 0 while (i < lines.length) { - const line = lines[i] + const line = lines[i] ?? '' if (line.startsWith('--- ')) { const minusRaw = line.slice(4) const minusPath = minusRaw === '/dev/null' @@ -224,9 +224,9 @@ export function extractApplyPatchPaths(patch: string): string[] { // Walk forward until we find the matching +++ header. let j = i + 1 - while (j < lines.length && !lines[j].startsWith('+++ ')) j++ + while (j < lines.length && !(lines[j] ?? '').startsWith('+++ ')) j++ if (j < lines.length) { - const plusRaw = lines[j].slice(4) + const plusRaw = (lines[j] ?? '').slice(4) const plusPath = plusRaw === '/dev/null' ? undefined : plusRaw.startsWith('b/') ? plusRaw.slice(2) : plusRaw