Fix Security baseline gate: override brace-expansion in eslint's dependency tree - #473
Open
richard-devbot wants to merge 2 commits into
Open
Fix Security baseline gate: override brace-expansion in eslint's dependency tree#473richard-devbot wants to merge 2 commits into
richard-devbot wants to merge 2 commits into
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 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