Fix #477: block interpreter-mediated writes in builder guard and validator sandbox - #488
Merged
Merged
Conversation
…#472) npm audit reported 5 high-severity advisories, all tracing to one root: a DoS/ReDoS bug in brace-expansion (GHSA-3jxr-9vmj-r5cp, GHSA-mh99-v99m-4gvg), reachable via eslint -> minimatch@3.1.5 -> brace-expansion@1.1.16. npm's only reported fix was eslint@10 (isSemVerMajor), which pulls in a new no-useless-assignment rule in @eslint/js's recommended config, flagging 15 pre-existing dead-store patterns across src/ -- too invasive for a security-only fix. Instead: add a root `overrides` entry (this repo already uses the same mechanism for esbuild/protobufjs/ws) forcing brace-expansion to the patched 5.0.8 line. Verified this reaches eslint's own tree (minimatch@3.1.5's brace-expansion now 5.0.8) while, as documented, leaving @earendil-works/pi-coding-agent's shrinkwrapped copy (5.0.6) untouched -- npm honors a shrinkwrap absolutely, so overrides can't reach it regardless. That shrinkwrapped copy is the only remaining high-severity finding, and it's exactly the node this gate's BUNDLED_ROOTS/ TOLERATED_BUNDLED_ADVISORIES allowlist already exists to tolerate -- added the second GHSA id to that allowlist (the gate is deliberately strict per-advisory-id, so a newly surfaced GHSA for an already-tolerated package still fails until consciously added). Verified: npm run lint unchanged (0 errors, same 4 pre-existing warnings -- confirms minimatch@3.1.5 works fine with the newer brace-expansion at runtime), npm run typecheck clean, gate logic simulated against real npm audit output now reports 0 blocking.
Qodo flagged four issues on the eslint/brace-expansion security fix: - no test coverage for the TOLERATED_BUNDLED_ADVISORIES allowlist change - brace-expansion@5.0.8 declares engines.node "20 || >=22", excluding the Node 18 CI lane - the ">=5.0.8" override is unbounded and can float to a future major - the script comment mis-attributed the version bump to an eslint 9->10 bump that never happened (the real mechanism is the root override) Fix: bound the override to an exact "5.0.8" pin; export the pure tolerate/block decision logic from security-audit.mjs (evaluateAdvisories, isFullyBundled, advisoryIds) behind a main-guard so it's unit-testable without shelling out to npm audit, and add tests/security-audit.test.js covering the tolerate/block/allowlist-miss/severity-threshold paths, plus a pinned-allowlist regression test; correct the stale comment to document the real mechanism and the accepted Node 18 engines trade-off (no brace-expansion release fixes both GHSAs while keeping Node 18 support; it's a devDependency-only path and no engine-strict is set, so this doesn't fail CI in practice). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…dator sandbox Two independent audits verified the same live bypass: node -e/python3 -c/ perl -e/etc. can write directly to protected .rstack/ governance state and source files, invisible to both the builder guard and the (supposedly stricter, deny-outright) validator sandbox — neither classifier modeled general-purpose interpreters as an arbitrary read/write/exec capability, only shell verbs and redirects. - destructive-actions.js: new isInterpreterExecCommand() detects inline-eval invocation (node -e/--eval/-p, python/python2/python3 -c, perl -e/-E, ruby -e, php -r, osascript -e, pwsh/powershell -Command/-EncodedCommand, deno eval) and bare-interpreter/piped-stdin invocation with no script argument (functionally identical to -e — reads its program from stdin). Deliberately does NOT try to parse the inline code itself (undecidable in general) — the mere ability to evaluate arbitrary code inline is the capability being classified, same as every other DESTRUCTIVE_CATEGORIES entry. A plain `node script.js` / `python3 manage.py runserver` (running a committed, on-disk file) stays allowed. Wired into classifyCommand as a new INTERPRETER_EXEC category (gateable via the existing per-task destructive-action:<taskId> approval — no new guard.js wiring needed, since approval-gating is category-agnostic) and into commandWritesFile (so the #373 BLOCKED-task gate can't be bypassed the same way). - validator-sandbox.js: reuses the exact same detector (one source of truth for the classification itself, even though builder/validator apply different policies to it) as a new interpreter-exec deny rule — no approval escape hatch, consistent with every other rule in that list. - Two smaller gaps found during the audit cross-validation, fixed alongside: `guard --explain` always exited 0 even for a `block` decision (a hook integration checking only the exit code would fail open on an explained block) — the exit code now matches the real decision in both the builder and validator-sandbox explain branches. `PROTECTED_CONFIG_PATTERN` matched only the two literal filenames `.claude/settings.json`/`settings.local.json` rather than the true `settings*.json` glob Claude Code actually supports (e.g. `settings.project.json`) — broadened to a real glob. - rstack-sdlc.ts: a delegated subprocess's env no longer inherits RSTACK_ALLOW_DESTRUCTIVE from the parent process — an operator's own scoped override was silently widening to every spawned sub-delegate. Adversarial test coverage added across all four surfaces: the classifier (harness-destructive-actions.test.js), the validator sandbox (harness-validator-sandbox.test.js), the guard CLI end-to-end including the #373 x #477 interaction (guard-cli.test.js), and the delegate env-scrub (harness-validator-sandbox-hook.test.js). 1636/1636 tests, typecheck clean, lint clean, security-audit clean. Fixes #477. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Strix Security Review1 open security finding on this PR:
5 resolved findings
Review summaryReviewed all security-relevant changes in the PR, including the new interpreter-exec classifier, validator sandbox reuse, guard exit-code handling, delegated environment scrubbing, and the security-audit allowlist logic. The previously reported glued Fixed the findings? re-run the review, or tag Updated for Reviewed by Strix |
Contributor
|
Warning Review limit reached
Next review available in: 50 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR classifies interpreter-based inline or stdin execution as destructive, blocks it in validator and guard flows, prevents delegated override inheritance, fixes explain-mode exit codes, and extracts security-audit evaluation helpers. It also adds a ChangesSecurity enforcement and audit
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Agent
participant GuardCLI
participant DestructiveClassifier
participant ValidatorSandbox
Agent->>GuardCLI: submit interpreter command
GuardCLI->>DestructiveClassifier: classify command
DestructiveClassifier-->>GuardCLI: interpreter-exec destructive result
GuardCLI->>ValidatorSandbox: evaluate guarded action
ValidatorSandbox-->>GuardCLI: blocking decision
GuardCLI-->>Agent: block verdict and exit code
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 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 |
PR Summary by QodoFix #477: block interpreter-mediated writes in builder guard and validator sandbox
AI Description
Diagram
High-Level Assessment
Files changed (12)
|
Code Review by Qodo
Context used✅ Compliance rules (platform):
300 rules✅ Skills:
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+108
to
+118
| if (tolerated.length) { | ||
| console.log('Tolerated advisories (vendored/bundled, unpatchable downstream — track upstream):'); | ||
| for (const t of tolerated) console.log(` - ${t.name} (${t.severity}) [${t.ids.join(', ')}]`); | ||
| } | ||
|
|
||
| if (blocking.length) { | ||
| console.error('\nBlocking high/critical advisories in our dependency tree:'); | ||
| for (const b of blocking) { | ||
| console.error(` - ${b.name} (${b.severity})`); | ||
| for (const n of b.nodes || []) console.error(` ${n}`); | ||
| } |
There was a problem hiding this comment.
2. Single-letter vars t/b 📜 Skill insight ⚙ Maintainability
In scripts/security-audit.mjs, the new loops use non-descriptive single-letter variables (t, b, n), reducing readability in security-gating logic and making future maintenance riskier.
Agent Prompt
## Issue description
Single-letter variable names (`t`, `b`, `n`) were introduced in new loops, which reduces clarity.
## Issue Context
These loops are part of security audit gating output; descriptive names help prevent mistakes during future changes.
## Fix Focus Areas
- scripts/security-audit.mjs[108-118]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+104
to
+107
| function main() { | ||
| const report = runAudit(); | ||
| const { blocking, tolerated } = evaluateAdvisories(report.vulnerabilities || {}); | ||
|
|
There was a problem hiding this comment.
3. Non-verb-noun main/adv 📜 Skill insight ⚙ Maintainability
New function names main() and adv() do not follow the required verb-noun naming pattern, making intent less clear (especially in security-audit gating code and its tests).
Agent Prompt
## Issue description
New function names `main` and `adv` do not follow the verb-noun naming convention.
## Issue Context
The checklist requires function names beginning with an action verb (e.g., `run...`, `make...`, `build...`, `evaluate...`).
## Fix Focus Areas
- scripts/security-audit.mjs[104-107]
- tests/security-audit.test.js[19-25]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+95
to
+137
| test('builder gates interpreter-mediated writes on the same audited approval as any other destructive category (#477)', async () => { | ||
| const { root } = seedProject(); | ||
| // The exact probes from the audit that previously classified as ALLOW. | ||
| const cases = [ | ||
| `node -e "require('fs').writeFileSync('.rstack/runs/x/tasks.json','[]')"`, | ||
| `python3 -c "open('.rstack/runs/x/tasks.json','w').write('[]')"`, | ||
| `node -e "require('fs').utimesSync('.rstack/runs/x/tasks.json.lock',new Date(),new Date())"`, | ||
| ]; | ||
| for (const command of cases) { | ||
| const blocked = await runGuard(['--command', command, '--task', 't1', '--project', root]); | ||
| assert.equal(blocked.code, 2, `expected block without approval: ${command}`); | ||
| assert.equal(blocked.verdict.category, 'interpreter-exec'); | ||
| assert.match(blocked.verdict.reason, /requires approval/); | ||
| } | ||
|
|
||
| const runId = 'run-interp-approved'; | ||
| const { root: approvedRoot } = seedProject({ runId, approvals: [approvedRecord('destructive-action:t1', runId)] }); | ||
| const allowed = await runGuard(['--command', cases[0], '--task', 't1', '--project', approvedRoot, '--run-id', runId]); | ||
| assert.equal(allowed.code, 0, 'the same per-task destructive-action approval unblocks interpreter-exec too'); | ||
| assert.equal(allowed.verdict.category, 'interpreter-exec'); | ||
| }); | ||
|
|
||
| test('validator/reviewer/security contexts deny interpreter-mediated writes outright, no approval escape hatch (#477)', async () => { | ||
| const denied = await runGuard(['--context', 'validator', '--command', `node -e "require('fs').writeFileSync('src/app.js','evil')"`], { | ||
| env: { RSTACK_ALLOW_DESTRUCTIVE: '1' }, // must NOT bypass the sandbox | ||
| }); | ||
| assert.equal(denied.code, 2); | ||
| assert.equal(denied.verdict.category, 'validator-sandbox'); | ||
| assert.match(denied.verdict.reason, /interpreter/i); | ||
|
|
||
| const bareInterpreter = await runGuard(['--context', 'reviewer', '--command', 'python3']); | ||
| assert.equal(bareInterpreter.code, 2, 'a bare interpreter with no script argument reads code from stdin'); | ||
| }); | ||
|
|
||
| test('a hard-BLOCKED task cannot bypass the attempt-budget gate via an interpreter one-liner (#373 x #477)', async () => { | ||
| const runId = 'run-blocked-interp'; | ||
| const { root } = seedProject({ runId }); | ||
| writeFileSync(join(root, '.rstack', 'runs', runId, 'tasks.json'), JSON.stringify([{ id: 't1', status: 'BLOCKED' }])); | ||
| const { code, verdict } = await runGuard(['--command', `node -e "require('fs').writeFileSync('x','y')"`, '--task', 't1', '--project', root, '--run-id', runId]); | ||
| assert.equal(code, 2); | ||
| assert.equal(verdict.category, 'attempt-budget-exhausted'); | ||
| }); | ||
|
|
There was a problem hiding this comment.
5. Regression tests added in-place 📜 Skill insight ⚙ Maintainability
This PR adds regression coverage by modifying several existing test files instead of introducing new dedicated regression test files. This violates the guideline to keep regression tests in separate new files to avoid coupling and churn in baseline test suites.
Agent Prompt
## Issue description
Regression test cases were added by editing existing test files, but the compliance guideline requires regression tests to be added as new test files (e.g., `*.regression-###.test.js`) without modifying existing test files.
## Issue Context
This PR adds multiple tests explicitly referencing `#477` / `#373`, which are regression-style protections, but they were appended into existing suites such as `tests/guard-cli.test.js`, `tests/harness-destructive-actions.test.js`, and `tests/harness-validator-sandbox.test.js`.
## Fix Focus Areas
- tests/guard-cli.test.js[95-137]
- tests/harness-destructive-actions.test.js[93-165]
- tests/harness-validator-sandbox.test.js[124-162]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
114
to
+118
| "overrides": { | ||
| "esbuild": ">=0.28.1", | ||
| "protobufjs": ">=7.6.4 <8", | ||
| "ws": ">=8.21.0" | ||
| "ws": ">=8.21.0", | ||
| "brace-expansion": "5.0.8" |
There was a problem hiding this comment.
7. Node engine mismatch risk 🐞 Bug ☼ Reliability
The PR forces brace-expansion to 5.0.8 via overrides, but the lockfile records that version as requiring Node 20 || >=22 while the repo and CI still claim/test Node 18 support; this will at least produce engine warnings on Node 18 and will hard-fail for any contributors/CI that run with engine-strict enabled. Since the lockfile also marks it as dev: true, this mostly impacts development/CI installs rather than runtime consumers, but it still conflicts with the project’s Node >=18 contract.
Agent Prompt
## Issue description
`package.json` now overrides `brace-expansion` to `5.0.8`, but `package-lock.json` shows that version declares `engines.node: "20 || >=22"` while the repo and CI still support/run Node 18. This creates install-time engine warnings on Node 18 and will fail in environments that enable `engine-strict`.
## Issue Context
- CI runs `npm ci` on Node 18/20/22.
- Root `package.json` claims `engines.node: >=18.0.0`.
- `brace-expansion@5.0.8` requires Node 20+ per lockfile.
## Fix Focus Areas
- package.json[114-118]
- package-lock.json[3569-3580]
- .github/workflows/ci.yml[11-29]
## Suggested fix options
Pick one consistent strategy:
1. **If Node 18 remains supported:** avoid forcing a Node-20-only `brace-expansion` (use a patched version compatible with Node 18, or adjust the dependency chain so the vulnerable version is eliminated without a Node 20-only bump).
2. **If Node 20+ is now the real support floor:** update `package.json` `engines.node` and drop Node 18 from CI matrix to match.
3. **If you accept warnings but want fewer surprises:** explicitly document that Node 18 installs will warn and may fail under `engine-strict`, and consider setting `engine-strict=false` in CI (if it could ever be toggled) to prevent accidental future breakage.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
![strix-security[bot]](https://avatars.githubusercontent.com/in/2749804?s=60&v=4){kind=small}
Comment on lines
+173
to
+178
| function interpreterVerbAndArgs(segment) { | ||
| const tokens = segment.trim().split(/\s+/).filter(Boolean).map(stripQuotes); | ||
| if (!tokens.length) return null; | ||
| const verb = tokens[0].toLowerCase().replace(/.*[/\\]/, ''); // basename of /usr/bin/node etc. | ||
| return INTERPRETER_VERBS.has(verb) ? { verb, args: tokens.slice(1) } : null; | ||
| } |
There was a problem hiding this comment.
Wrapped interpreter invocations still bypass the interpreter-exec guard
Resolved — no longer flagged as of 84f8397.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
tests/security-audit.test.js (1)
106-112: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant dynamic re-import.
TOLERATED_BUNDLED_ADVISORIEScould be added to the static import on line 15 instead of a separateawait import(...)here, since the module is already imported at module load time.♻️ Proposed simplification
-import { evaluateAdvisories, isFullyBundled, advisoryIds } from '../scripts/security-audit.mjs'; +import { evaluateAdvisories, isFullyBundled, advisoryIds, TOLERATED_BUNDLED_ADVISORIES } from '../scripts/security-audit.mjs';-test('the real TOLERATED_BUNDLED_ADVISORIES allowlist matches the documented brace-expansion GHSAs', async () => { - const { TOLERATED_BUNDLED_ADVISORIES } = await import('../scripts/security-audit.mjs'); - assert.deepEqual( +test('the real TOLERATED_BUNDLED_ADVISORIES allowlist matches the documented brace-expansion GHSAs', () => { + assert.deepEqual( [...TOLERATED_BUNDLED_ADVISORIES.get('brace-expansion')].sort(), ['GHSA-3jxr-9vmj-r5cp', 'GHSA-mh99-v99m-4gvg'].sort(), ); });🤖 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 `@tests/security-audit.test.js` around lines 106 - 112, Update the test to include TOLERATED_BUNDLED_ADVISORIES in the existing static import from scripts/security-audit.mjs, then remove the redundant await import inside the test while preserving the current allowlist assertion.
🤖 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 `@src/core/harness/destructive-actions.js`:
- Around line 185-211: Update interpreterHasEvalCapability so the no-argument
check runs before the special deno handling, causing bare deno to be classified
as stdin-fed code evaluation like other bare interpreters. Preserve deno’s
existing eval-only argument behavior for invocations that include arguments.
- Around line 155-177: Update interpreterVerbAndArgs to recognize
version-suffixed interpreter executable names, such as python3.11, ruby3.2, and
perl5.34, while preserving path basename normalization and existing exact
matches. Normalize or match the extracted verb against the supported
INTERPRETER_VERBS entries before returning the interpreter metadata, and keep
unsupported executables returning null.
- Around line 141-211: Extend isInterpreterExecCommand and its helper parsing so
supported shell wrappers and launchers cannot hide interpreter eval execution.
Recognize bash, sh, and zsh as inline/stdin-capable interpreters with -c
handling, and unwrap common prefixes such as env, nohup, xargs, and leading
VAR=value assignments before evaluating the nested command. Preserve existing
script-path behavior while ensuring wrapped node/python and equivalent
interpreter invocations are detected for both builder and validator callers.
---
Nitpick comments:
In `@tests/security-audit.test.js`:
- Around line 106-112: Update the test to include TOLERATED_BUNDLED_ADVISORIES
in the existing static import from scripts/security-audit.mjs, then remove the
redundant await import inside the test while preserving the current allowlist
assertion.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 57b3ee5e-a561-4cb8-8925-967a123bb52c
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (11)
package.jsonscripts/security-audit.mjssrc/commands/guard.jssrc/core/harness/destructive-actions.jssrc/core/harness/validator-sandbox.jssrc/integrations/pi/rstack-sdlc.tstests/guard-cli.test.jstests/harness-destructive-actions.test.jstests/harness-validator-sandbox-hook.test.jstests/harness-validator-sandbox.test.jstests/security-audit.test.js
…sses Three independent reviewers (CodeRabbit, Strix, qodo) confirmed the same class of bypass in isInterpreterExecCommand(): it only inspected the FIRST whitespace token of a segment, so wrapping the interpreter behind a launcher, a nested shell, or a version-suffixed binary evaded detection entirely — reproduced live against the extension by CodeRabbit (`bash -c "node -e ..."`) and Strix. Concretely closes: - Leading env-var assignments: `FOO=1 node -e ...` - Launcher/wrapper commands: `env`/`sudo`/`timeout`/`nohup`/`command`/ `nice`/`ionice`/`setsid`/`exec` node/python/etc, including stacked wrappers (`sudo env timeout 5 node -e ...`), each consuming its own flags/args before the real verb is found (bounded to 6 hops). - Nested shells: `bash`/`sh`/`zsh`/`dash`/`ksh` are now themselves INTERPRETER_VERBS with `-c` as their eval flag — a shell is a general-purpose interpreter too, so `bash -c "node -e ..."` is caught on the OUTER bash invocation regardless of what's nested inside. - Version-suffixed binaries: `python3.11`, `ruby3.2`, `perl5.34` (pyenv/ asdf/system-package-manager convention) now normalize to their base interpreter name. - Glued short flags: `-e'code'`, `-c"code"` (no space/`=` before the quoted value) are now recognized, scoped to <=2-char dash flags so a long `--flag` can't over-match as a prefix. - Bare `deno` (no args): previously fell through to `args[0] undefined !== 'eval'` and returned false, unlike every other bare interpreter in this file (which correctly treat "no args at all" as stdin-fed code) — now consistent. The validator sandbox reuses this exact detector, so every fix here closes the same gap in both the builder approval-gate and the deny-outright validator/reviewer/security path with one change. Also: added @param/@returns to the exported isInterpreterExecCommand JSDoc (qodo), and filled in docs/HARNESS.md's DESTRUCTIVE_CATEGORIES list — it was missing interpreter-exec (this PR's own new category) plus two pre-existing gaps (remote-exec, perm-destroy) found while fixing it. Adversarial test coverage added for every bypass form above (wrapper chains, nested shells, version suffixes, glued flags, bare deno) in both the classifier and validator-sandbox suites, plus a false-positive check confirming the fixes don't flag ordinary sudo/env/timeout/nohup usage. 1644/1644 tests, typecheck clean, lint clean. Skipped as out of scope for this PR (all originate from the already-open #473 branch merged in for CI, not from #477's own diff): security-audit.mjs variable-naming/console.log/verb-noun-naming findings, and the Node-18-vs- brace-expansion-5.0.8 engine note — belong to #473's review cycle. Skipped as not applicable: "regression tests should be new files" (does not match this codebase's actual, extensive convention of adding cases to existing suite files) and `any`-typing in the claim gate (matches this 3000+-line file's pre-existing, pervasive typing convention; a scoped retype was out of proportion to this fix). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
docs/HARNESS.md (1)
675-680: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDoc list omits
dash/ksh, which the implementation now also treats as interpreters.Minor completeness nit — the code's
INTERPRETER_EVAL_FLAGSincludesdashandkshalongsidebash/sh/zsh, but the doc's parenthetical list doesn't mention them.🤖 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 `@docs/HARNESS.md` around lines 675 - 680, Update the interpreter list in the `interpreter-exec` documentation entry to include `dash` and `ksh` alongside `bash`/`sh`/`zsh`, matching the implementations covered by `INTERPRETER_EVAL_FLAGS`; leave the surrounding inline/stdin and on-disk script behavior 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 `@src/core/harness/destructive-actions.js`:
- Around line 221-227: Update normalizeInterpreterVerb to include php in the
version-suffix normalization pattern, so binaries such as php7.4, php8.1,
php8.2, and php8.3 resolve to the base php verb while preserving existing
Python, Ruby, and Perl behavior.
- Around line 194-219: Update skipEnvAssignmentsAndWrappers so wrapper options
that consume a separate following value are skipped together with that value,
covering common sudo, nice, and ionice flags such as -u and -n. Preserve
existing assignment, flag, timeout-duration, hop-limit, and real-command
detection behavior so the walk reaches the interpreter verb and continues
failing closed through the shared classifier.
---
Nitpick comments:
In `@docs/HARNESS.md`:
- Around line 675-680: Update the interpreter list in the `interpreter-exec`
documentation entry to include `dash` and `ksh` alongside `bash`/`sh`/`zsh`,
matching the implementations covered by `INTERPRETER_EVAL_FLAGS`; leave the
surrounding inline/stdin and on-disk script behavior 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8d40525e-37ca-4e3a-9fbd-d7dea19b6b75
📒 Files selected for processing (4)
docs/HARNESS.mdsrc/core/harness/destructive-actions.jstests/harness-destructive-actions.test.jstests/harness-validator-sandbox.test.js
Owner
Author
|
@strix-security please re-run — the interpreter-detector bypass (wrappers, nested shells, version-suffixed binaries) reported in the previous review has been fixed. |
…rsion suffix A second Strix pass (after re-requesting review post-fix) confirmed the first round of wrapper/version-suffix fixes but found two residual gaps: 1. Wrapper flags that take their value as a SEPARATE following token (nice -n 10, ionice -c 3, sudo -u root, timeout --signal KILL 5) — the value token (`10`, `root`, `KILL`) was mistaken for the next command's verb and returned null, since the previous fix only knew how to skip tokens that themselves look like flags/assignments, not "a flag plus its separate value". Fixed with a per-wrapper WRAPPER_VALUE_FLAGS map naming which flags consume a following token, so `nice -n 10 node -e ...` now correctly walks past `-n 10` to find `node`. 2. Version-suffixed `php` binaries (`php7.4`, `php8.2` — distro package naming) weren't in the version-suffix normalization regex, which only covered python/ruby/perl. Added. Adversarial test coverage for both: every wrapper-with-value-flag combination Strix's report implied, plus a false-positive check that ordinary `nice`/`sudo`/`ionice` usage on non-interpreter commands stays unflagged. 1646/1646 tests, typecheck clean, lint clean. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Owner
Author
|
@strix-security please re-run — the two follow-up gaps (wrapper flags with separate-token values, php version-suffix) have been fixed. |
Comment on lines
+277
to
+287
| const lowerFlags = flagSet.map((f) => f.toLowerCase()); | ||
| return args.some((raw) => { | ||
| const lower = stripQuotes(raw).toLowerCase(); | ||
| if (lowerFlags.includes(lower)) return true; | ||
| if (lowerFlags.includes(lower.split('=')[0])) return true; // --flag=value | ||
| // A short single-dash flag glued directly to its value with no | ||
| // separating space or `=` (`-e'code'`, `-c"code"`). Scoped to <=2-char | ||
| // dash flags only, so this can't over-match a long --flag as a prefix | ||
| // of some unrelated longer token. | ||
| return lowerFlags.some((flag) => flag.length <= 2 && flag[0] === '-' && flag[1] !== '-' && lower.startsWith(flag) && lower.length > flag.length); | ||
| }); |
There was a problem hiding this comment.
Clustered shell short options still bypass the interpreter-exec guard
Resolved — no longer flagged as of 4e1cd88.
Third Strix pass confirmed both prior follow-up fixes and found one more gap: clustered single-dash short options — `bash -lc "node -e ..."` (login + command), `python3 -uc "..."` (unbuffered + command), `perl -pie '...'` (print-loop + in-place + eval) — bundle an eval-capable flag letter behind other single-char flags in the SAME token, which neither the exact-match nor the glued-prefix check recognized (`-lc` doesn't start with `-c`). Fixed: interpreterHasEvalCapability now also checks a bounded clustering pattern — a single-dash token of 1-4 lowercase letters where any letter matches a known single-CHARACTER eval flag (`-c`/`-e`/`-r`/`-p`; long `--flag` forms can't cluster, so they're excluded). The length bound keeps this a plausible short-flag cluster rather than a long GNU-style word that happens to contain the letter. Adversarial coverage for the exact reported forms plus their real, common non-eval clusters (`python3 -O`, `ruby -w`, `bash -lx`) staying unflagged. 1648/1648 tests, typecheck clean, lint clean. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Owner
Author
|
@strix-security please re-run — the clustered-short-option gap (bash -lc, python -uc, perl -pie) has been fixed. |
| // `10` in `-n 10` was mistaken for the next command's verb and the real | ||
| // interpreter one hop further in was never reached. | ||
| const WRAPPER_VALUE_FLAGS = Object.freeze({ | ||
| env: new Set(['-u', '--unset', '-c', '--chdir', '-s', '--split-string']), |
There was a problem hiding this comment.
Interpreter-exec guard still misses env -C and longer clustered short-option forms
env -C and longer clustered short-option formsResolved — no longer flagged as of 9b13559.
Comment on lines
+295
to
+298
| if (/^-[a-z]{1,4}$/.test(lower)) { | ||
| const clusteredLetters = lower.slice(1); | ||
| if (lowerFlags.some((flag) => flag.length === 2 && flag[0] === '-' && clusteredLetters.includes(flag[1]))) return true; | ||
| } |
There was a problem hiding this comment.
Inspect longer clustered short-option tokens for embedded eval flags
Suggested change
| if (/^-[a-z]{1,4}$/.test(lower)) { | |
| const clusteredLetters = lower.slice(1); | |
| if (lowerFlags.some((flag) => flag.length === 2 && flag[0] === '-' && clusteredLetters.includes(flag[1]))) return true; | |
| } | |
| if (/^-[a-z]{2,}$/.test(lower)) { | |
| const clusteredLetters = lower.slice(1); | |
| if (lowerFlags.some((flag) => flag.length === 2 && flag[0] === '-' && clusteredLetters.includes(flag[1]))) return true; | |
| } |
…unded cluster length (Strix 3rd follow-up) Two residual bypasses on the interpreter-exec detector: (1) env's real chdir flag is uppercase `-C` (distinct from `-c`), but the wrapper value-flags lookup compared tokens case-sensitively against a lowercase-only set, so `env -C /dir node -e ...` never consumed the directory argument and mistook it for the next command's verb, stopping short of the wrapped interpreter; matching is now case-insensitive. (2) the clustered-short-option regex was bounded to 4 letters, missing longer real clusters like `bash -lifc "..."`; widened to no upper bound (2+ letters) per Strix's suggested diff.
Owner
Author
|
@strix-security please re-run — fixed the env -C (uppercase chdir flag, case-insensitive matching now) and widened the clustered short-option regex to no upper bound per your suggested diff. |
Comment on lines
+205
to
+241
| const WRAPPER_VALUE_FLAGS = Object.freeze({ | ||
| env: new Set(['-u', '--unset', '-c', '--chdir', '-s', '--split-string']), | ||
| sudo: new Set(['-u', '--user', '-g', '--group', '-p', '--prompt', '-h', '--host', '-r', '--role', '-t', '--type']), | ||
| nice: new Set(['-n', '--adjustment']), | ||
| ionice: new Set(['-c', '--class', '-n', '--classdata', '-p', '--pid', '-t', '--ignore']), | ||
| timeout: new Set(['-s', '--signal', '-k', '--kill-after']), | ||
| }); | ||
|
|
||
| // Index into `tokens` of the real command verb, after skipping any leading | ||
| // `VAR=value` environment assignments and a bounded chain of launcher | ||
| // wrappers (each consuming its own flags/args, INCLUDING a separate value | ||
| // token for flags known to take one). Returns `tokens.length` when nothing | ||
| // but assignments/wrappers are present (no real command at all). | ||
| function skipEnvAssignmentsAndWrappers(tokens) { | ||
| let i = 0; | ||
| while (i < tokens.length && /^[A-Za-z_][A-Za-z0-9_]*=/.test(tokens[i])) i += 1; | ||
| for (let hop = 0; hop < MAX_WRAPPER_HOPS && i < tokens.length; hop += 1) { | ||
| const verb = tokens[i].toLowerCase().replace(/.*[/\\]/, ''); | ||
| if (!WRAPPER_VERBS.has(verb)) break; | ||
| i += 1; | ||
| const valueFlags = WRAPPER_VALUE_FLAGS[verb]; | ||
| while (i < tokens.length) { | ||
| const token = tokens[i]; | ||
| if (/^[A-Za-z_][A-Za-z0-9_]*=/.test(token)) { i += 1; continue; } | ||
| // Bare numeric duration for `timeout` (`timeout 5 node -e ...`). | ||
| if (verb === 'timeout' && /^[0-9]+(\.[0-9]+)?[smhd]?$/i.test(token)) { i += 1; continue; } | ||
| if (token.startsWith('-')) { | ||
| i += 1; | ||
| // A flag known to take a separate value token consumes the next | ||
| // token too, unless the value was already glued via `=`. Matched | ||
| // case-insensitively: GNU env's real chdir flag is `-C` (uppercase, | ||
| // distinct from `-c`) — an exact-case Set lookup silently missed it | ||
| // (Strix follow-up, 3rd pass), leaving `-C`'s directory argument | ||
| // mistaken for the next command's verb and the wrapped interpreter | ||
| // one hop further in never reached. | ||
| if (valueFlags?.has(token.toLowerCase()) && !token.includes('=') && i < tokens.length) i += 1; | ||
| continue; |
There was a problem hiding this comment.
env -S and env --split-string still bypass the interpreter-exec guard
env -S and env --split-string still bypass the interpreter-exec guardResolved — no longer flagged as of a1cec3d.
…mmand instead of skipping it (Strix 4th follow-up) GNU env's -S/--split-string re-parses its argument into the actual command line, unlike other value-taking wrapper flags whose value is opaque. Treating it like -u/-C and skipping the next token discarded the wrapped interpreter itself (`env -S 'node -e "1"'` went undetected). Naive whitespace tokenization already breaks the quoted payload into the same tokens the wrapped command would produce, so the fix consumes nothing extra after -S/--split-string and lets the walk continue straight into them.
Owner
Author
|
@strix-security please re-run — fixed env -S/--split-string: it re-parses its argument into the actual command line rather than taking an opaque value, so it no longer consumes an extra token (which was discarding the wrapped interpreter). |
Comment on lines
+239
to
+242
| if (token.startsWith('-')) { | ||
| const lowerToken = token.toLowerCase(); | ||
| i += 1; | ||
| if (verb === 'env' && ENV_SPLIT_STRING_FLAGS.has(lowerToken)) continue; |
There was a problem hiding this comment.
Glued env -S<cmd> still bypasses the interpreter-exec guard
env -S<cmd> still bypasses the interpreter-exec guardResolved — no longer flagged as of 775758a.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@src/core/harness/destructive-actions.js`:
- Around line 205-251: Update skipEnvAssignmentsAndWrappers to recognize GNU env
split-string options when the value is glued to -S or --split-string, including
quoted payloads and equals syntax. Extract and continue walking the embedded
command tokens rather than treating the entire option as an opaque flag, so
interpreter executions are detected by isInterpreterExecCommand and shared
classifiers fail closed as intended. Preserve existing handling for
separate-token split-string flags and other wrapper options.
In `@tests/harness-destructive-actions.test.js`:
- Around line 264-279: The env split-string detection must support glued -S and
--split-string= forms in addition to standalone tokens. Update the split-string
handling in isInterpreterExecCommand to recognize both glued variants while
preserving existing behavior, then add regression assertions for env -S'node -e
"1"' and env --split-string='python3 -c "1"' alongside the current tests.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e6296c58-e683-4ca8-bc95-c4578165d35f
📒 Files selected for processing (2)
src/core/harness/destructive-actions.jstests/harness-destructive-actions.test.js
Comment on lines
+205
to
+251
| const WRAPPER_VALUE_FLAGS = Object.freeze({ | ||
| env: new Set(['-u', '--unset', '-c', '--chdir']), | ||
| sudo: new Set(['-u', '--user', '-g', '--group', '-p', '--prompt', '-h', '--host', '-r', '--role', '-t', '--type']), | ||
| nice: new Set(['-n', '--adjustment']), | ||
| ionice: new Set(['-c', '--class', '-n', '--classdata', '-p', '--pid', '-t', '--ignore']), | ||
| timeout: new Set(['-s', '--signal', '-k', '--kill-after']), | ||
| }); | ||
| // GNU env's `-S`/`--split-string` re-parses its argument into the underlying | ||
| // command line — it is NOT an ordinary value-taking flag whose value can be | ||
| // skipped (Strix follow-up, 4th pass): `env -S 'node -e "1"'` still runs | ||
| // node. Naive whitespace tokenization already breaks the quoted payload into | ||
| // the SAME subsequent tokens the wrapped command would produce, so the fix | ||
| // is to consume nothing extra here and let the walk continue straight into | ||
| // them, rather than discarding the next token as if it were a plain value. | ||
| const ENV_SPLIT_STRING_FLAGS = new Set(['-s', '--split-string']); | ||
|
|
||
| // Index into `tokens` of the real command verb, after skipping any leading | ||
| // `VAR=value` environment assignments and a bounded chain of launcher | ||
| // wrappers (each consuming its own flags/args, INCLUDING a separate value | ||
| // token for flags known to take one). Returns `tokens.length` when nothing | ||
| // but assignments/wrappers are present (no real command at all). | ||
| function skipEnvAssignmentsAndWrappers(tokens) { | ||
| let i = 0; | ||
| while (i < tokens.length && /^[A-Za-z_][A-Za-z0-9_]*=/.test(tokens[i])) i += 1; | ||
| for (let hop = 0; hop < MAX_WRAPPER_HOPS && i < tokens.length; hop += 1) { | ||
| const verb = tokens[i].toLowerCase().replace(/.*[/\\]/, ''); | ||
| if (!WRAPPER_VERBS.has(verb)) break; | ||
| i += 1; | ||
| const valueFlags = WRAPPER_VALUE_FLAGS[verb]; | ||
| while (i < tokens.length) { | ||
| const token = tokens[i]; | ||
| if (/^[A-Za-z_][A-Za-z0-9_]*=/.test(token)) { i += 1; continue; } | ||
| // Bare numeric duration for `timeout` (`timeout 5 node -e ...`). | ||
| if (verb === 'timeout' && /^[0-9]+(\.[0-9]+)?[smhd]?$/i.test(token)) { i += 1; continue; } | ||
| if (token.startsWith('-')) { | ||
| const lowerToken = token.toLowerCase(); | ||
| i += 1; | ||
| if (verb === 'env' && ENV_SPLIT_STRING_FLAGS.has(lowerToken)) continue; | ||
| // A flag known to take a separate value token consumes the next | ||
| // token too, unless the value was already glued via `=`. Matched | ||
| // case-insensitively: GNU env's real chdir flag is `-C` (uppercase, | ||
| // distinct from `-c`) — an exact-case Set lookup silently missed it | ||
| // (Strix follow-up, 3rd pass), leaving `-C`'s directory argument | ||
| // mistaken for the next command's verb and the wrapped interpreter | ||
| // one hop further in never reached. | ||
| if (valueFlags?.has(lowerToken) && !token.includes('=') && i < tokens.length) i += 1; | ||
| continue; |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
env -S/--split-string glued to its value still bypasses the interpreter-exec guard.
ENV_SPLIT_STRING_FLAGS.has(lowerToken) (line 242) only matches when -S/--split-string is its own, separate token — exactly the case the code comment (and past Strix follow-up) already fixed. But GNU env also accepts the value glued directly to the flag: -S'cmd' (no space) or --split-string=cmd. Confirmed via GNU coreutils docs: env -vS'perl -T -w' and "Running env -Sstring splits the string into arguments based on unquoted spaces."
For a glued token such as -S'node -e "1"', lowerToken is -s'node (not -s), so the split-string branch never triggers. The token instead falls into the generic token.startsWith('-') branch, which silently consumes the entire glued token as an opaque boolean flag — the embedded node -e "1" is never extracted or inspected. Reproduction:
env -S'node -e "1"'
env --split-string='node -e "1"'
Both execute node -e "1" under real GNU env, yet isInterpreterExecCommand(...) returns false for both, so classifyCommand, commandWritesFile, and the validator's deny-outright path (which reuses this detector) all miss it.
🔒 Proposed fix: detect glued `-S`/`--split-string` forms
const ENV_SPLIT_STRING_FLAGS = new Set(['-s', '--split-string']);
+// Matches `-S` glued directly to its value (`-S'cmd'`, `-Scmd`) or the
+// long-option glued form (`--split-string=cmd`) — GNU env accepts both,
+// and neither is an exact match against ENV_SPLIT_STRING_FLAGS.
+const ENV_SPLIT_STRING_GLUED_RE = /^(?:-s.+|--split-string=.*)$/i;
function skipEnvAssignmentsAndWrappers(tokens) {
...
if (token.startsWith('-')) {
const lowerToken = token.toLowerCase();
i += 1;
- if (verb === 'env' && ENV_SPLIT_STRING_FLAGS.has(lowerToken)) continue;
+ if (verb === 'env' && (ENV_SPLIT_STRING_FLAGS.has(lowerToken) || ENV_SPLIT_STRING_GLUED_RE.test(token))) continue;
if (valueFlags?.has(lowerToken) && !token.includes('=') && i < tokens.length) i += 1;
continue;
}As per coding guidelines, "Destructive actions, approvals, protected-config writes, and validator restrictions must fail closed and use the shared classifier."
🤖 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 `@src/core/harness/destructive-actions.js` around lines 205 - 251, Update
skipEnvAssignmentsAndWrappers to recognize GNU env split-string options when the
value is glued to -S or --split-string, including quoted payloads and equals
syntax. Extract and continue walking the embedded command tokens rather than
treating the entire option as an opaque flag, so interpreter executions are
detected by isInterpreterExecCommand and shared classifiers fail closed as
intended. Preserve existing handling for separate-token split-string flags and
other wrapper options.
Source: Coding guidelines
Comment on lines
+264
to
+279
| // Strix follow-up, 4th pass: GNU env's `-S`/`--split-string` re-parses its | ||
| // argument into the underlying command line rather than taking an ordinary | ||
| // opaque value — treating it like `-u`/`-C` (skip one token) discarded the | ||
| // wrapped command itself instead of exposing it. | ||
| test('interpreter-exec: env -S / --split-string does not hide the wrapped interpreter (Strix 4th follow-up)', () => { | ||
| for (const cmd of [ | ||
| `env -S 'node -e "1"'`, | ||
| `env --split-string 'python3 -c "print(1)"'`, | ||
| ]) { | ||
| assert.equal(isInterpreterExecCommand(cmd), true, `expected interpreter-exec: ${cmd}`); | ||
| } | ||
| }); | ||
|
|
||
| test('interpreter-exec: env -S fix does not create a false positive when nothing evals', () => { | ||
| assert.equal(isInterpreterExecCommand(`env -S 'npm test'`), false); | ||
| }); |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Missing regression coverage for glued -S/--split-string forms.
Coverage here only exercises the space-separated form (env -S 'node -e "1"', env --split-string 'python3 -c "print(1)"'). It doesn't cover the glued forms (env -S'node -e "1"', env --split-string='python3 -c "1"'), which GNU env also accepts and which bypass the current detector (see companion comment on src/core/harness/destructive-actions.js lines 205-251).
src/core/harness/destructive-actions.js#L205-L251: extend the split-string check to also match glued-S/--split-string=forms, not just the exact standalone token.tests/harness-destructive-actions.test.js#L264-L279: add cases forenv -S'node -e "1"'andenv --split-string='python3 -c "1"'alongside the existing space-separated assertions.
🤖 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 `@tests/harness-destructive-actions.test.js` around lines 264 - 279, The env
split-string detection must support glued -S and --split-string= forms in
addition to standalone tokens. Update the split-string handling in
isInterpreterExecCommand to recognize both glued variants while preserving
existing behavior, then add regression assertions for env -S'node -e "1"' and
env --split-string='python3 -c "1"' alongside the current tests.
…he wrapped command (Strix 5th follow-up) The just-fixed env -S/--split-string handling only recognized the flag as a standalone token. GNU env also accepts the glued long form (--split-string=<cmd>), which left the wrapped command embedded inside that one token instead of exposed as the next token to walk into. Normalize the flag by splitting on `=` before matching, and when glued, substitute the payload (quote-stripped) in place of the flag token so the walk continues straight into the wrapped interpreter exactly like the space-separated form.
Owner
Author
|
@strix-security please re-run — fixed the glued env --split-string= form: it's now normalized on '=' before matching, and the payload is substituted in place so the walk continues into the wrapped interpreter. |
Owner
Author
|
@strix-security this finding appears stale — the glued |
| // detector (arbitrarily deep obfuscation is a losing regex battle — the | ||
| // approval-gate default for anything unrecognized as this class stays | ||
| // "allowed", same as before this module existed). | ||
| const WRAPPER_VERBS = new Set(['env', 'sudo', 'command', 'nohup', 'timeout', 'nice', 'ionice', 'setsid', 'exec']); |
There was a problem hiding this comment.
🟠 cmd /c wrapper bypasses the new interpreter-exec guard
Severity: HIGH · CWE-184
The new interpreter-exec classifier only unwraps a fixed set of launcher verbs and omits Windows cmd. As a result, commands such as cmd /c node -e "..." and cmd /c python3 -c "..." are treated as non-destructive even though they execute arbitrary inline code. Because validator-sandbox.js reuses the same detector, the bypass affects both builder approval gating and validator read-only enforcement.
Prompt to fix with AI
This is a security vulnerability found during a code review.
Vulnerability: `cmd /c` wrapper bypasses the new interpreter-exec guard
Severity: HIGH
CWE: CWE-184
The new interpreter-exec classifier only unwraps a fixed set of launcher verbs and omits Windows `cmd`. As a result, commands such as `cmd /c node -e "..."` and `cmd /c python3 -c "..."` are treated as non-destructive even though they execute arbitrary inline code. Because `validator-sandbox.js` reuses the same detector, the bypass affects both builder approval gating and validator read-only enforcement.
Location: src/core/harness/destructive-actions.js:194-194
Context: Wrapper allowlist omits Windows `cmd`, so `cmd /c ...` never unwraps to the nested interpreter
```
const WRAPPER_VERBS = new Set(['env', 'sudo', 'command', 'nohup', 'timeout', 'nice', 'ionice', 'setsid', 'exec']);
```
Location: src/core/harness/destructive-actions.js:284-290
Context: Interpreter classification depends entirely on the wrapper walker, so an unrecognized `cmd` wrapper yields a false negative
```
function interpreterVerbAndArgs(segment) {
const tokens = segment.trim().split(/\s+/).filter(Boolean).map(stripQuotes);
if (!tokens.length) return null;
const verbIndex = skipEnvAssignmentsAndWrappers(tokens);
if (verbIndex >= tokens.length) return null;
const verb = normalizeInterpreterVerb(tokens[verbIndex]);
return INTERPRETER_VERBS.has(verb) ? { verb, args: tokens.slice(verbIndex + 1) } : null;
}
```
Location: src/core/harness/validator-sandbox.js:74-87
Context: Validator enforcement inherits the same false negative because it reuses `isInterpreterExecCommand()` unchanged
```
Object.freeze({
// General-purpose interpreters (#477): `node -e "fs.writeFileSync(...)"`
// or a bare `python3` fed code over stdin/pipe is a write/read/exec
// primitive invisible to the shell-verb rules above — a validator is
// read-only, so ANY inline-eval or bare-stdin interpreter invocation is
// denied outright, same posture as every other rule in this list (no
// approval escape hatch). Uses a `test` predicate instead of a `pattern`
// regex because the decision needs tokenization, not just text matching;
// reuses the EXACT same detector destructive-actions.js gates the
// builder path with (#131: one source of truth for the classification
// itself, even though builder/validator apply different POLICIES to it).
id: 'interpreter-exec',
test: isInterpreterExecCommand,
reason: 'invokes a general-purpose interpreter with inline/stdin code evaluation (arbitrary read/write/exec capability)',
}),
```
How to fix:
Teach the wrapper walker to treat Windows `cmd` as a launcher wrapper and consume its execution flags before resolving the underlying verb. In practice, the detector should unwrap `cmd /c ...` and `cmd /k ...` the same way it already unwraps `env`, `sudo`, and other launchers, then re-run interpreter classification on the nested command. Add regression coverage for both `cmd /c node -e ...` and `cmd /c python3 -c ...` across `classifyCommand()`, `commandWritesFile()`, and validator-sandbox evaluation.
Please fix this vulnerability. If you propose a fix, make it concise and minimal.React 👍 / 👎 to tune Strix for this repo. A repo collaborator (or the PR author) can resolve this thread to dismiss the finding.
Comment on lines
+284
to
+290
| function interpreterVerbAndArgs(segment) { | ||
| const tokens = segment.trim().split(/\s+/).filter(Boolean).map(stripQuotes); | ||
| if (!tokens.length) return null; | ||
| const verbIndex = skipEnvAssignmentsAndWrappers(tokens); | ||
| if (verbIndex >= tokens.length) return null; | ||
| const verb = normalizeInterpreterVerb(tokens[verbIndex]); | ||
| return INTERPRETER_VERBS.has(verb) ? { verb, args: tokens.slice(verbIndex + 1) } : null; |
There was a problem hiding this comment.
Interpreter classification depends entirely on the wrapper walker, so an unrecognized cmd wrapper yields a false negative
Comment on lines
+74
to
+87
| Object.freeze({ | ||
| // General-purpose interpreters (#477): `node -e "fs.writeFileSync(...)"` | ||
| // or a bare `python3` fed code over stdin/pipe is a write/read/exec | ||
| // primitive invisible to the shell-verb rules above — a validator is | ||
| // read-only, so ANY inline-eval or bare-stdin interpreter invocation is | ||
| // denied outright, same posture as every other rule in this list (no | ||
| // approval escape hatch). Uses a `test` predicate instead of a `pattern` | ||
| // regex because the decision needs tokenization, not just text matching; | ||
| // reuses the EXACT same detector destructive-actions.js gates the | ||
| // builder path with (#131: one source of truth for the classification | ||
| // itself, even though builder/validator apply different POLICIES to it). | ||
| id: 'interpreter-exec', | ||
| test: isInterpreterExecCommand, | ||
| reason: 'invokes a general-purpose interpreter with inline/stdin code evaluation (arbitrary read/write/exec capability)', |
There was a problem hiding this comment.
Validator enforcement inherits the same false negative because it reuses isInterpreterExecCommand() unchanged
This was referenced Jul 31, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #477 (P0, Wave 0 of the #476 autonomy-hardening epic). Two independent audits verified the same live bypass:
node -e/python3 -c/perl -e/etc. write directly to protected.rstack/governance state and source files, invisible to both the builder guard and the (supposedly stricter, deny-outright) validator sandbox — neither classifier modeled general-purpose interpreters as an arbitrary read/write/exec capability, only shell verbs and redirects.destructive-actions.js: newisInterpreterExecCommand()detects inline-eval invocation (node -e/--eval/-p,python(2/3) -c,perl -e/-E,ruby -e,php -r,osascript -e,pwsh/powershell -Command/-EncodedCommand,deno eval) and bare-interpreter/piped-stdin invocation with no script argument (functionally identical to-e). Deliberately does not try to parse the inline code itself — the mere ability to evaluate arbitrary code inline is the capability being classified. A plainnode script.js/python3 manage.py runserver(a committed, on-disk file) stays allowed. Wired intoclassifyCommandas a newINTERPRETER_EXECcategory — gateable via the existing per-taskdestructive-action:<taskId>approval with no new guard.js wiring needed, since approval-gating is category-agnostic — and intocommandWritesFile(so the [P1][harness] Attempt/telemetry budgets are Pi-only — guard must enforce budgets on bridge/guard harnesses #373 BLOCKED-task gate can't be bypassed the same way).validator-sandbox.js: reuses the exact same detector (one source of truth for the classification, even though builder/validator apply different policies to it) as a new interpreter-exec deny rule — no approval escape hatch, consistent with every other rule.guard --explainalways exited0even for ablockdecision — a hook integration checking only the exit code would fail open on an explained block. Exit code now matches the real decision in both explain branches.PROTECTED_CONFIG_PATTERNmatched only the two literal filenames (settings.json/settings.local.json), not the true.claude/settings*.jsonglob Claude Code actually supports (e.g.settings.project.json). Broadened to a real glob.rstack-sdlc.ts: a delegated subprocess's env no longer inheritsRSTACK_ALLOW_DESTRUCTIVEfrom the parent process — an operator's own scoped override was silently widening to every spawned sub-delegate.Test plan
npx tsx --test tests/harness-destructive-actions.test.js— new adversarial corpus (every supported interpreter, bare/piped-stdin forms, the exact audit probes, safe-script-file non-flagging,commandWritesFile, glob broadening)npx tsx --test tests/harness-validator-sandbox.test.js— validator context denies the same probes outright, still allows running test scriptsnpx tsx --test tests/guard-cli.test.js— end-to-end CLI: builder approval-gates interpreter-exec, validator denies it (RSTACK_ALLOW_DESTRUCTIVE doesn't bypass), [P1][harness] Attempt/telemetry budgets are Pi-only — guard must enforce budgets on bridge/guard harnesses #373×[P0][Security] Block interpreter-mediated writes in builder guard and validator read-only sandbox #477 interaction,--explainexit-code fixnpx tsx --test tests/harness-validator-sandbox-hook.test.js— delegate env no longer leaksRSTACK_ALLOW_DESTRUCTIVEnpm test— 1636/1636npm run typecheck— cleannpm run lint— clean (pre-existing unrelated warnings only)npm run validate— cleannode scripts/security-audit.mjs— clean (branch includes the Fix Security baseline gate: override brace-expansion in eslint's dependency tree #473 brace-expansion fix so CI is green regardless of merge order)Scope notes (read before merge)
node <<EOF) is a known residual gap — the bare-interpreter check catches piped stdin with zero arguments, but a heredoc marker counts as an "argument" in the simple tokenizer, so it isn't caught. Not in the audits' concrete PoCs; flagged here for transparency rather than silently claimed as covered.python -m <module>(running a named, already-installed module) is intentionally not flagged — it takes no inline code argument, unlike-c, and is extremely common legitimate usage (python -m pytest,python -m http.server).🤖 Generated with Claude Code
Summary by CodeRabbit
--explain/sandbox exit codes to match blocking decisions and avoid fail-open behavior.