Fix Security baseline gate: override brace-expansion in eslint's dependency tree - #473
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.
Strix Security ReviewNo security issues found. Updated for Reviewed by Strix |
Contributor
📝 WalkthroughWalkthroughThe pull request adds a ChangesSecurity audit baseline
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 CI security baseline by overriding brace-expansion in ESLint tree
AI Description
Diagram
High-Level Assessment
Files changed (3)
|
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
3569
to
3580
| "node_modules/brace-expansion": { | ||
| "version": "1.1.16", | ||
| "resolved": "https://registry.npmjs.org/brace-expansion/-/brace-expansion-1.1.16.tgz", | ||
| "integrity": "sha512-IDw48K2/2kRkg9LdJxurvq3lV3aBgq0REY89duEqFRthjlPdXHKMj7EnQOXVckxzgisinf3nHfrcE2FufFLXMw==", | ||
| "version": "5.0.8", | ||
| "resolved": "https://registry.npmjs.org/brace-expansion/-/brace-expansion-5.0.8.tgz", | ||
| "integrity": "sha512-JZyDyq3D4AUifKTPOB7DELf6XsB3WdPuNxCtob1vFXPsSXhdAiHBWJ/tJ8HAc9aH84BK+5JFZLNkJKx3G9kzQg==", | ||
| "dev": true, | ||
| "license": "MIT", | ||
| "dependencies": { | ||
| "balanced-match": "^1.0.0", | ||
| "concat-map": "0.0.1" | ||
| "balanced-match": "^4.0.2" | ||
| }, | ||
| "engines": { | ||
| "node": "20 || >=22" | ||
| } |
There was a problem hiding this comment.
2. Node version mismatch 🐞 Bug ☼ Reliability
The override pulls in brace-expansion@5.0.8 which declares Node "20 || >=22", but this repo declares Node ">=18" and CI still runs a Node 18.x lane. This can cause install-time engine failures (in stricter environments) or make the Node 18 lane brittle when the overridden dependency is exercised (e.g., lint tooling).
Agent Prompt
## Issue description
`brace-expansion@5.0.8` is now forced into the root dependency tree, but it declares `engines.node = "20 || >=22"`, conflicting with this repo’s declared `engines.node >=18` and the CI matrix that still runs Node 18.
## Issue Context
Even if npm only warns by default, this becomes a hard failure in environments that enforce engines (or if the dependency starts using Node 20-only APIs).
## Fix Focus Areas
- package.json[41-43]
- package.json[114-119]
- .github/workflows/ci.yml[13-17]
- package-lock.json[3569-3580]
## What to change
Choose one:
1) If Node 18 is truly supported: use a patched `brace-expansion` version that supports Node 18, or scope the override so Node 18 paths don’t require Node 20-only packages.
2) If Node 18 is *not* supported in practice: update `package.json` engines and remove Node 18 from the CI matrix so the declared support matches reality.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
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>
![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: 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 `@scripts/security-audit.mjs`:
- Around line 48-52: Update advisoryIds to fail closed for malformed advisory
objects: retain valid URL-derived IDs, but produce a non-tolerated result for
any object entry lacking a usable url instead of filtering it out. Ensure
callers of advisoryIds, including the additional referenced path, block the
report when such an unidentified entry is present.
In `@tests/security-audit.test.js`:
- Around line 59-72: Extend the security tests near the existing bundled
advisory case with a mixed-GHSA scenario for brace-expansion: include both the
allowlisted GHSA-3jxr-9vmj-r5cp and a new, non-allowlisted ID, then call
evaluateAdvisories with the same bundled configuration and assert the advisory
is blocking rather than tolerated. Keep the existing single-new-ID regression
intact.
🪄 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: 8921db52-b91e-4e5f-adcd-b0207efd2798
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
package.jsonscripts/security-audit.mjstests/security-audit.test.js
Comment on lines
+48
to
52
| export function advisoryIds(info) { | ||
| return (info.via ?? []) | ||
| .filter((entry) => typeof entry === 'object' && entry?.url) | ||
| .map((entry) => String(entry.url).split('/').pop()); | ||
| } |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Fail closed when an advisory entry cannot be identified.
advisoryIds drops object entries without url. A bundled report containing one allowlisted GHSA plus an unparseable advisory object is therefore tolerated. Treat malformed object entries as non-tolerated and block the advisory instead of filtering them out. As per coding guidelines, “validator restrictions must fail closed.”
Also applies to: 90-94
🤖 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 `@scripts/security-audit.mjs` around lines 48 - 52, Update advisoryIds to fail
closed for malformed advisory objects: retain valid URL-derived IDs, but produce
a non-tolerated result for any object entry lacking a usable url instead of
filtering it out. Ensure callers of advisoryIds, including the additional
referenced path, block the report when such an unidentified entry is present.
Source: Coding guidelines
Comment on lines
+59
to
+72
| test('blocks a bundled advisory whose id is NOT on the allowlist — a new GHSA forces a conscious decision', () => { | ||
| const vulns = { | ||
| 'brace-expansion': adv({ | ||
| ids: ['GHSA-brand-new-id'], | ||
| nodes: [`${BUNDLED_ROOT}brace-expansion`], | ||
| }), | ||
| }; | ||
| const { blocking, tolerated } = evaluateAdvisories(vulns, { | ||
| toleratedBundledAdvisories: new Map([['brace-expansion', new Set(['GHSA-3jxr-9vmj-r5cp'])]]), | ||
| bundledRoots: [BUNDLED_ROOT], | ||
| }); | ||
| assert.equal(tolerated.length, 0); | ||
| assert.equal(blocking.length, 1); | ||
| }); |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Add a mixed-GHSA mutation regression.
Add a bundled brace-expansion case with one allowlisted ID and one new ID, asserting it blocks. The current suite would not catch an every→some mutation in evaluateAdvisories. As per coding guidelines, tests must include “mutation-oriented tests for security.”
🤖 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 59 - 72, Extend the security tests
near the existing bundled advisory case with a mixed-GHSA scenario for
brace-expansion: include both the allowlisted GHSA-3jxr-9vmj-r5cp and a new,
non-allowlisted ID, then call evaluateAdvisories with the same bundled
configuration and assert the advisory is blocking rather than tolerated. Keep
the existing single-new-ID regression intact.
Source: Coding guidelines
This was referenced Jul 30, 2026
richard-devbot
added a commit
that referenced
this pull request
Jul 30, 2026
…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>
richard-devbot
added a commit
that referenced
this pull request
Jul 30, 2026
…sors Two reviewers (qodo, Strix) found real correctness bugs in the claim gate's #479 checks, all stemming from the same root cause: checking only ONE item (via .find()) when the invariant actually requires checking EVERY item. 1. (qodo, Action required) Active-stage check used .find() to grab a single IN_PROGRESS task and authorize the candidate against just that one. With overlapping parallel_groups (e.g. [A,B] and [A,C]), a candidate authorized against A (found first) could be wrongly allowed even though a DIFFERENT active task B was not authorized against it. Fixed: .filter() to get every active task, refuse if ANY of them is unauthorized with the candidate. 2. (qodo, Action required) Predecessor check only ever looked at the FIRST unmet predecessor (findUnmetPredecessorTask), and if that one happened to be exempt via parallel-group authorization, a second, non-exempt unmet predecessor elsewhere in the plan was never even checked. Fixed: new findUnmetPredecessorTasks (plural) returns every unmet predecessor; the claim gate finds the first one that ISN'T exempt, and refuses if any such predecessor exists. The singular function is now a thin wrapper (`[0]`) kept for its existing narrow callers/tests. 3. (Strix, HIGH, CWE-862) The predecessor exemption only checked parallel_groups CONFIG authorization, never whether the predecessor was actually running. Strix reproduced this live: validate an early stage into NEEDS_CONTEXT (its parallel_groups sibling is still configured as authorized), then call sdlc_build_next directly for that sibling — it claimed anyway, bypassing the runner's intended stop-for-human-context behavior. Fixed: exemption now requires `predecessor.status === "IN_PROGRESS"` in addition to authorization — a predecessor merely CONFIGURED into a group but not actually running (NEEDS_CONTEXT, BLOCKED, FAIL) is never exempt. Authorization only ever widens what may run alongside a genuinely active peer; it never excuses a predecessor that isn't running at all. Also (qodo DRY finding): extracted the repeated "no task granted" return object into one `refusal()` helper shared by the three return sites (plain no-task, active-stage block, dependency block) instead of duplicating the same seven-field literal three times. Bonus fix found while writing the adversarial tests below: the dependency_unmet tool response's `details` was missing `predecessor_status` (present in the emitted event, but not in what the caller sees) — added. Test coverage: findUnmetPredecessorTasks (plural) unit tests, plus three new end-to-end tests reproducing each finding exactly — overlapping active-task authorization, multiple unmet predecessors with only one exempt, and the Strix NEEDS_CONTEXT bypass. 1645/1645 tests (1 known, unrelated, pre-existing temp-dir-cleanup flake in dashboard-env-write.test.js that passes in isolation), typecheck clean, lint clean. Skipped as out of scope for this PR (originate from the already-open #473 branch merged in for CI): security-audit.mjs advisoryIds naming, console.log usage. Skipped as not applicable given this file's pre-existing, pervasive convention: `any` typing in the claim gate. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
richard-devbot
added a commit
that referenced
this pull request
Jul 30, 2026
…re signal Strix (HIGH, CWE-807) and qodo both found the same real gap in the infra-failure heuristic: it inferred "the command never ran" from "stdout is empty", but (a) a genuine test failure that only writes to stderr also has empty stdout — a false positive that could silently downgrade a real FAIL to a WARN — and (b) since the command runs via `sh -c` INSIDE the container, repository-controlled test code could deliberately print one of the known Docker/Podman error phrases to stderr and exit non-zero to suppress its own real failure (the untrusted code influencing a trust decision about itself). Fixed with an unspoofable signal instead of an inference: buildSandboxArgv now wraps the untrusted command so a TRUSTED start sentinel (`__rstack_sandbox_started__`) prints to stdout before the command ever runs. looksLikeInfraFailure now checks for the sentinel's presence in the RAW (untruncated) stdout buffer — not "is stdout empty" — so: - a real failure that writes only to stderr still shows the sentinel on stdout and correctly stays FAIL; - untrusted code has no way to make the sentinel ABSENT, since the trusted wrapper's printf already ran and was already captured before the untrusted command got control — the only lever left to the untrusted code is printing the SAME sentinel too, which is harmless (still proves the command started, still a real FAIL either way). The sentinel check runs on the full buffer before tail() truncates it, so a long-output command whose sentinel would otherwise fall out of the 8KB tail window is still correctly recognized as started. Also (qodo, real bug): validateSandboxConfig compared execution_policy values case-sensitively while sandbox.js's resolveExecutionPolicy normalizes via trim().toLowerCase() before comparing — an operator's "Required" would validate as invalid even though it's accepted and applied correctly at runtime. Added a matching normalizeExecutionPolicy helper used by both the top-level and per-stage checks. Also (qodo, minor): extracted the hardcoded `300` stderr-truncation length in the infra-failure evidence string into a named INFRA_FAILURE_EVIDENCE_MAX_CHARS constant. Test coverage: updated the pinned argv test for the new wrapped command format, two new tests proving a genuine stderr-only failure and a deliberately daemon-phrase-mimicking real failure both stay FAIL, a dedicated spoofing-attempt regression test, and case/whitespace insensitivity coverage for the config validator matching the resolver. 1654/1654 tests, typecheck clean, lint clean. Skipped as out of scope for this PR (originate from the already-open #473 branch merged in for CI): PII-in-owner-comment finding (the attribution line is this codebase's established, intentional convention, naming the real project owner — not accidental PII), console.log usage in security-audit.mjs. Skipped as not applicable given this file's pre-existing convention: `any` typing on runValidationExecution. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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
Security baselinejob fails onmainright now, independent of any PR.npm auditreports 5 high-severity advisories, all tracing to one root: a DoS/ReDoS bug inbrace-expansion(GHSA-3jxr-9vmj-r5cp, GHSA-mh99-v99m-4gvg), reachable viaeslint -> minimatch@3.1.5 -> brace-expansion@1.1.16.npm audit's only reported fix waseslint@10(isSemVerMajor: true) — tried it, but@eslint/js's ESLint-10 recommended config adds a newno-useless-assignmentrule that flags 15 pre-existing dead-store patterns acrosssrc/. Too invasive for what's really a transitive-dependency-only issue (eslintis a devDependency, never installed bynpm install rstack-agents).overridesentry (same mechanism this repo already uses foresbuild/protobufjs/ws) forcingbrace-expansionto the patched5.0.8line. Confirmed this reaches eslint's own tree while — as the script's own comments document — leaving@earendil-works/pi-coding-agent's shrinkwrapped copy (5.0.6) untouched (npm honors a shrinkwrap absolutely; overrides can't reach it).security-audit.mjs'sBUNDLED_ROOTS/TOLERATED_BUNDLED_ADVISORIESallowlist already exists to tolerate. Added the second GHSA id to that allowlist — the gate is deliberately strict per-advisory-id (a newly-surfaced GHSA for an already-tolerated package still fails until consciously added), so this is a real, intentional decision, not a silent widening.Test plan
npm run lint— unchanged (0 errors, same 4 pre-existing warnings), confirmingminimatch@3.1.5works fine against the newerbrace-expansionat runtimenpm run typecheck— 0 errorsnpm audit --json— only the already-tolerated bundledpi-coding-agentnode remains; simulated the gate's own logic against the real audit output and confirmed 0 blockingnpx/npminspawn()without a shell — a known, unrelated environment limitation that also affects an unmodifiedmaincheckout identically; CI runs on Linux where this doesn't apply, same as the last two PRs)Summary by CodeRabbit
Security
brace-expansionadvisory as tolerated.Tests