Fix Security baseline gate: override brace-expansion in eslint's dependency tree#473
Open
richard-devbot wants to merge 1 commit into
Open
Fix Security baseline gate: override brace-expansion in eslint's dependency tree#473richard-devbot wants to merge 1 commit into
richard-devbot wants to merge 1 commit 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
|
Warning Review limit reached
Next review available in: 18 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 ignored due to path filters (1)
📒 Files selected for processing (2)
✨ Finishing Touches🧪 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
+31
to
42
| // - brace-expansion GHSA-3jxr-9vmj-r5cp + GHSA-mh99-v99m-4gvg (both DoS): | ||
| // pinned 5.0.6 by pi's shrinkwrap, both advisories confined to that one | ||
| // bundled node (verified: eslint's own brace-expansion moved to the | ||
| // patched 5.0.8 after #472's eslint 9->10 bump, so only the shrinkwrapped | ||
| // copy remains on the vulnerable <=5.0.7 line); every non-shrinkwrapped | ||
| // path in our tree is on the fixed line. | ||
| // - protobufjs / ws: previously tolerated by name while their advisories were | ||
| // live in the bundled subtree; currently dormant, so no ids are listed — if | ||
| // one refires, the gate fails loudly and the specific GHSA gets re-added. | ||
| const TOLERATED_BUNDLED_ADVISORIES = new Map([ | ||
| ['brace-expansion', new Set(['GHSA-3jxr-9vmj-r5cp'])], | ||
| ['brace-expansion', new Set(['GHSA-3jxr-9vmj-r5cp', 'GHSA-mh99-v99m-4gvg'])], | ||
| ]); |
There was a problem hiding this comment.
1. security-audit.mjs change lacks tests 📘 Rule violation ▣ Testability
This PR changes the Security baseline gate behavior by expanding TOLERATED_BUNDLED_ADVISORIES to accept an additional GHSA ID, but no automated tests were added/updated in the same changeset to cover this new allowlist behavior. Without tests, future refactors may unintentionally broaden/narrow the gate logic and break CI or weaken security controls silently.
Agent Prompt
## Issue description
The PR updates `scripts/security-audit.mjs` to tolerate an additional `brace-expansion` advisory ID, which changes CI gate behavior, but there is no corresponding test change in this PR to lock in the intended behavior.
## Issue Context
The `security-baseline` CI job runs `node scripts/security-audit.mjs`, so changes to this script are executable behavior changes and should be covered by automated tests.
## Fix Focus Areas
- scripts/security-audit.mjs[31-42]
- tests/security-audit.test.js[1-200]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
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
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.
3. Unbounded override range 🐞 Bug ⚙ Maintainability
The override uses ">=5.0.8", which allows future lockfile regenerations to select newer (including potentially breaking) major versions of brace-expansion. It also forces minimatch@3.1.5 to run against brace-expansion 5.x even though minimatch declares a 1.x range, creating an ongoing transitive-compatibility risk.
Agent Prompt
## Issue description
The override `"brace-expansion": ">=5.0.8"` is open-ended and can float to future major releases when the lockfile is regenerated. It also overrides `minimatch@3.1.5`’s declared dependency range (`^1.1.7`), so the repo is relying on compatibility that npm is no longer enforcing.
## Issue Context
Right now the lockfile pins `brace-expansion` to `5.0.8`, but the manifest range increases the chance of unexpected breakage on routine dependency maintenance.
## Fix Focus Areas
- package.json[114-119]
- package-lock.json[4994-5005]
## What to change
- Prefer pinning to an exact version (`"5.0.8"`) or a bounded range (`"^5.0.8"`).
- Consider scoping the override to only the eslint/minimatch subtree (npm overrides support nested overrides) so you don’t globally replace every consumer’s `brace-expansion` with a potentially incompatible major.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+31
to
+36
| // - brace-expansion GHSA-3jxr-9vmj-r5cp + GHSA-mh99-v99m-4gvg (both DoS): | ||
| // pinned 5.0.6 by pi's shrinkwrap, both advisories confined to that one | ||
| // bundled node (verified: eslint's own brace-expansion moved to the | ||
| // patched 5.0.8 after #472's eslint 9->10 bump, so only the shrinkwrapped | ||
| // copy remains on the vulnerable <=5.0.7 line); every non-shrinkwrapped | ||
| // path in our tree is on the fixed line. |
There was a problem hiding this comment.
4. Incorrect audit script comment 🐞 Bug ⚙ Maintainability
The updated comment claims eslint’s brace-expansion moved to 5.0.8 due to an “eslint 9->10 bump”, but this PR keeps eslint at ^9 and makes the change via npm overrides. This mismatch can mislead future debugging of the security baseline gate.
Agent Prompt
## Issue description
The comment in `scripts/security-audit.mjs` attributes the brace-expansion change to an ESLint 9→10 bump, but the repo is still on ESLint `^9.0.0` and the actual mechanism is a root `overrides` entry.
## Issue Context
This is documentation-only, but it directly affects maintainers’ ability to reason about why the vulnerability is (or isn’t) present.
## Fix Focus Areas
- scripts/security-audit.mjs[31-36]
- package.json[101-106]
- package.json[114-119]
## What to change
Rewrite the comment to say the non-shrinkwrapped tree is fixed due to the root `overrides` forcing `brace-expansion@5.0.8`, while the shrinkwrapped `@earendil-works/pi-coding-agent` copy remains pinned and tolerated by advisory id.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
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)