chore(asv3): security hardening pass (WP-9)#405
Conversation
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
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
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: `<a :href="...">` and `<a v-bind:href="...">`
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
The existing rule body already catches `homedir() + '.claude'` concatenation through `concatenatesClaudeDir` (PR #348 era). Add five explicit positive fixtures spelling out the WP-9 brief's exact patterns so a future regression in the rule body breaks the test rather than silently slipping past the audit. Also fills in `loop-state.md` with the full four-track iteration log and audit summary. https://claude.ai/code/session_01UWDtzLuFJU3QmQmLrXCxWj
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7e9e71a7a7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // 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)) { |
There was a problem hiding this comment.
Rejecting any binaryPath that contains [;&|$<>\n\r]turns legitimate install paths into hard launch failures even thoughSubprocessLifecycle.spawn()invokeschild_process.spawn()withoutshell: true(so these characters are not interpreted as shell syntax). This means users whose absolute path includes characters like&or$(valid on both POSIX and Windows filesystems) will always getCLI_LAUNCH_FAILED` despite having a real Claude binary.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in f655e1c. Valid catch — SubprocessLifecycle.spawn() calls child_process.spawn() without shell: true, so the metacharacter regex was overreach: those bytes are passed to the kernel as opaque path content, never shell-interpreted. Removed the check; legitimate install paths with &/$/; no longer hard-fail.
Kept the other gates (empty, absolute-only, forbidden interpreter basenames, claude(-code)?(.exe|.cmd|.bat)? allow-list) — those still defend against the actual threat model.
Test surface swapped: dropped the 9-case "rejects shell metacharacters" describe; added a 5-case "accepts legitimate path segments containing shell metacharacters" describe that locks in the new behaviour (/Users/me/Apps & Tools/claude, /home/user/$WORK/claude, C:\Program Files (x86)\Anthropic\claude.exe, etc.). Updated the file header with a "What we do not reject" section + the rationale, so a future reader doesn't re-add the regex.
Pre-PR gate green: 1929/1929 tests, typecheck, lint (0 errors), npm audit 0 vulns, build + build:web + docs:api succeed.
Generated by Claude Code
…Codex P2) Codex P2 review on PR #405 flagged the regex rejecting any `binaryPath` containing `[;&|`$<>\n\r]` as overreach: `SubprocessLifecycle.spawn()` invokes `child_process.spawn()` without `shell: true`, so those bytes are not shell-interpreted — they're opaque path bytes the kernel hands straight to `execve`/`CreateProcess`. Users whose absolute path includes `&` (e.g. `/Users/me/Apps & Tools/claude`) or `$` (e.g. an Anthropic install under a directory with a literal `$WORK` segment) were getting hard `CLI_LAUNCH_FAILED` despite holding a real Claude binary. - assertSpawnable.ts: removed the metacharacter regex + error branch. Kept all the other gates (empty, absolute-only, forbidden interpreter basenames, claude(-code)?(.exe|.cmd|.bat)? allow-list). Updated the header comment with a "What we **do not** reject" section explaining the Codex rationale so the next reader doesn't re-add the regex. - assertSpawnable.test.ts: replaced the 9-case "rejects shell metacharacters" describe with a 5-case "accepts legitimate path segments containing shell metacharacters (Codex P2 round-1)" describe, locking in that `&`, `$`, `;`, `` ` ``, and Windows parenthesised path segments all pass when the basename is in the claude allow-list. Pre-PR gate green: 1929/1929 tests, typecheck, lint (0 errors), `npm audit` 0 vulns, build + build:web + docs:api succeed. https://claude.ai/code/session_01UWDtzLuFJU3QmQmLrXCxWj
2 participants
Summary
Closes the four concrete security gaps surfaced by the v2 attack-surface walk for the agent-sidepanel-v3 stack. Each gap maps to a single track / commit:
test(asv3): WP-9 Track 1): closes the 0% / 58% coverage holes onLocalStorageSecretStoreandMockSecretStore. Adds round-trip persistence assertions,available: falsedegraded-branch assertions, a no-secret-in-logs invariant on everyconsole.*channel, and pinsSECRET_ID_ANTHROPICto the/^[a-z0-9-]+$/regex required by Obsidian'sApp.secretStoragevalidator.assertSpawnabledefense-in-depth (feat(asv3): WP-9 Track 2): newsrc/infrastructure/obsidian/assertSpawnable.tsrejects empty input, shell metacharacters, relative paths, known shells/interpreters (sh/bash/zsh/cmd.exe/powershell.exe/…), and any basename that isn'tclaude(-code)?(.exe|.cmd|.bat)?. Wired into both subprocess entry points (queryStreamvia_spawnChild,runStructuredviarunSubprocessStructured) BEFORElifecycle.spawn. Guard failure surfaces asClaudeCliError{CLI_LAUNCH_FAILED}with aSPAWN_GUARD_FAILED:technical-message prefix.no-unsafe-anchor-hrefESLint rule (feat(asv3): WP-9 Track 3): manual audit of every.vuefile undersrc/ui/components/{agent,chat}/— onlyMarkdownBlock.vue(WP-4 territory) composes anchor URLs, and it already routes throughsafeHref. Neweslint-rules/no-unsafe-anchor-href.cjsrule covers<a :href="...">template bindings,.href =assignments,window.open(...), andsetAttribute('href', ...). WARN severity, scoped to the audit surface — preventive only.no-claude-home-readsinvariant pinning (test(asv3): WP-9 Track 4): the existing rule body already catcheshomedir() + '.claude'viaconcatenatesClaudeDir(PR PR-ASM-5: ESLint rule + integration tests + release polish #348). Adds five explicit positive fixtures locking in the WP-9 brief's exact patterns so a future regression in the rule body breaks the test rather than slipping silently past the audit.Brief:
specs/agent-sidepanel-v3/wp-09-security-hardening/brief.md. Iteration log:loop-state.mdin the same folder.Test plan
npm audit --audit-level=high --omit=dev— 0 vulnerabilitiesnpm run typecheck— cleannpm run lint— 0 errors (24 pre-existing warnings; the new rule produces 0 warnings against current code)npm run lint:rules— both project-local rules pass their RuleTester + linter-shape assertionsnpm run test— 152 files / 1933 tests pass (21 new secret-store assertions + 49 newassertSpawnableassertions)npm run build— plugin bundle 2.9 MBnpm run build:web— standalone UI 270 KBnpm run docs:api— generated (one pre-existing TypeDoc broken-link warning, unrelated)https://claude.ai/code/session_01UWDtzLuFJU3QmQmLrXCxWj
Generated by Claude Code