fix(security): patch minimatch ReDoS vulnerability#8
Conversation
…e 28 lint errors Force all transitive minimatch instances to >=10.2.1 via npm overrides, fixing O(4^N) ReDoS with repeated wildcards. Also fix every pre-existing lint error: replace Array<T> syntax, remove as-any casts, add missing return types, eliminate non-null assertions, convert static-only class to const object.
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughThe PR implements security patching for a ReDoS vulnerability via minimatch override, refactors OrchestrationFSM from class-based to functional approach, standardizes TypeScript Array syntax across the codebase, strengthens type safety through explicit return types and safer property access, and documents policy guidelines for issue ownership. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/validation/validatePatchOps.ts (1)
8-13: CJS interop pattern is correct; consider makingAjvPluginaccept optional second args.The
default ?? moduleresolution pattern is correct and well-commented. One minor typing note: bothajv-formats@3andajv-errors@3accept an optional secondoptionsargument, and both returnAjvrather thanvoid. The currentAjvPlugintype silently drops those constraints, but since the return value is discarded and no options are passed, it is functionally harmless.🔎 More precise type (optional)
-type AjvPlugin = (ajv: InstanceType<typeof Ajv>) => void; +type AjvPlugin = (ajv: InstanceType<typeof Ajv>, options?: Record<string, unknown>) => InstanceType<typeof Ajv> | void;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/validation/validatePatchOps.ts` around lines 8 - 13, The Ajv plugin type is too narrow: update the AjvPlugin typedef used by addFormats and ajvErrors so it accepts an optional second options argument and returns the Ajv instance instead of void (e.g., change AjvPlugin to (ajv: InstanceType<typeof Ajv>, options?: unknown) => InstanceType<typeof Ajv>), then ensure addFormats and ajvErrors keep their existing CJS default-resolution logic but are typed with the new AjvPlugin signature.package.json (1)
30-32: Consider tightening the override range to^10.2.1.GHSA-3ppc-4f35-3m26 is confirmed valid: minimatch < 10.2.1 is vulnerable and 10.2.1 is the first patched release. The
>=10.2.1range correctly addresses the CVE, but it is unbounded across future major versions — if minimatch@11 ships with a breaking API, the nextnpm installcould silently force all six transitive consumers (eslint, glob, etc.) onto an incompatible major, breaking the build without any change to this file.🔒 Proposed tighter range
"overrides": { - "minimatch": ">=10.2.1" + "minimatch": "^10.2.1" },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` around lines 30 - 32, The override for minimatch currently uses an unbounded range ">=10.2.1", which prevents installing vulnerable versions but may allow future breaking major versions; update the "overrides" entry for "minimatch" to use a caret-constrained range like "^10.2.1" so you ensure at least the patched release while preventing inadvertent jumps to a new major; modify the package.json overrides object (the "overrides" key and the "minimatch" value) accordingly and run a fresh install to verify dependency resolution.src/validation/signPatchFixture.ts (1)
10-11:sha512helper and polyfill are copy-pasted fromcrypto.ts.Lines 10–11 are identical to
crypto.tslines 8–11. Sincesha512is a private (non-exported) symbol incrypto.ts,signPatchFixture.tscan't import it and must redeclare. Exportingsha512(or a dedicatedsetupEd25519Polyfill()helper) fromcrypto.tswould remove the duplication and ensure there's a single canonical Node.js SHA-512 shim for the entire module.♻️ Proposed approach
In
src/validation/crypto.ts, export the helper:-const sha512 = (msg: Uint8Array): Uint8Array => new Uint8Array(createHash("sha512").update(msg).digest()); +export const sha512 = (msg: Uint8Array): Uint8Array => new Uint8Array(createHash("sha512").update(msg).digest());In
src/validation/signPatchFixture.ts, import instead of redefining:-import { - canonicalize, - prefixedBlake3, - buildUnsignedPayloadForDigest -} from "./crypto.js"; +import { + sha512, + canonicalize, + prefixedBlake3, + buildUnsignedPayloadForDigest +} from "./crypto.js"; -// Polyfill sha512 for `@noble/ed25519` (v3 requires manual hash setup) -const sha512 = (msg: Uint8Array): Uint8Array => new Uint8Array(createHash("sha512").update(msg).digest()); -ed.hashes.sha512 = sha512;This also eliminates the redundant
import { createHash } from "node:crypto"in this file.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/validation/signPatchFixture.ts` around lines 10 - 11, The sha512 helper (const sha512 and assignment ed.hashes.sha512) is duplicated; move the canonical implementation into the existing crypto module by exporting either the sha512 function or a setupEd25519Polyfill() that assigns ed.hashes.sha512, then in signPatchFixture import that exported symbol instead of redefining sha512 and remove the redundant createHash import; update signPatchFixture to use the imported sha512 (or call setupEd25519Polyfill) so ed.hashes.sha512 is provided from the single exported implementation.src/domain/services/OrchestrationFSM.ts (1)
19-22:data as Jsoncast silently accepts non-serializable values.
Record<string, unknown>values could includeundefined,bigint, functions, or symbols, whichcanonicalizewill mishandle (e.g., skip or throw). The cast bypasses TypeScript's type checking. The current sole call site ({ state: nextState, runId: context.runId }) is safe, but the signature is misleadingly permissive.♻️ Tighten the signature to the actual requirement
-function computeDigest(data: Record<string, unknown>): string { - const canonical = canonicalize(data as Json); +function computeDigest(data: Record<string, Json>): string { + const canonical = canonicalize(data); return prefixedBlake3(canonical); }This surfaces the type constraint at the call site rather than silently swallowing it at runtime.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/domain/services/OrchestrationFSM.ts` around lines 19 - 22, The computeDigest function accepts a too-permissive Record<string, unknown> and casts it to Json, which can mask non-serializable values; change computeDigest's parameter type to the stricter Json (or a Serializables type matching canonicalize's input) so callers must pass only JSON-serializable data, update its signature (function computeDigest(data: Json): string) and adjust the current call site that passes { state: nextState, runId: context.runId } if necessary to ensure both properties are Json-serializable; keep the body using canonicalize(data) and prefixedBlake3(canonical) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/domain/services/OrchestrationFSM.ts`:
- Line 44: The eventId generation ignores the injected deterministic clock by
calling new Date().toISOString() directly; instead reuse the already-resolved
now (which respects context.clock) when building audit.eventId to keep
audit.timestamp and the date fragment consistent; locate the eventId
construction (symbol: eventId) and replace the direct new Date() usage with a
formatted date derived from now (the variable computed from context.clock) so
tests remain deterministic.
---
Nitpick comments:
In `@package.json`:
- Around line 30-32: The override for minimatch currently uses an unbounded
range ">=10.2.1", which prevents installing vulnerable versions but may allow
future breaking major versions; update the "overrides" entry for "minimatch" to
use a caret-constrained range like "^10.2.1" so you ensure at least the patched
release while preventing inadvertent jumps to a new major; modify the
package.json overrides object (the "overrides" key and the "minimatch" value)
accordingly and run a fresh install to verify dependency resolution.
In `@src/domain/services/OrchestrationFSM.ts`:
- Around line 19-22: The computeDigest function accepts a too-permissive
Record<string, unknown> and casts it to Json, which can mask non-serializable
values; change computeDigest's parameter type to the stricter Json (or a
Serializables type matching canonicalize's input) so callers must pass only
JSON-serializable data, update its signature (function computeDigest(data:
Json): string) and adjust the current call site that passes { state: nextState,
runId: context.runId } if necessary to ensure both properties are
Json-serializable; keep the body using canonicalize(data) and
prefixedBlake3(canonical) unchanged.
In `@src/validation/signPatchFixture.ts`:
- Around line 10-11: The sha512 helper (const sha512 and assignment
ed.hashes.sha512) is duplicated; move the canonical implementation into the
existing crypto module by exporting either the sha512 function or a
setupEd25519Polyfill() that assigns ed.hashes.sha512, then in signPatchFixture
import that exported symbol instead of redefining sha512 and remove the
redundant createHash import; update signPatchFixture to use the imported sha512
(or call setupEd25519Polyfill) so ed.hashes.sha512 is provided from the single
exported implementation.
In `@src/validation/validatePatchOps.ts`:
- Around line 8-13: The Ajv plugin type is too narrow: update the AjvPlugin
typedef used by addFormats and ajvErrors so it accepts an optional second
options argument and returns the Ajv instance instead of void (e.g., change
AjvPlugin to (ajv: InstanceType<typeof Ajv>, options?: unknown) =>
InstanceType<typeof Ajv>), then ensure addFormats and ajvErrors keep their
existing CJS default-resolution logic but are typed with the new AjvPlugin
signature.
- Bug: eventId now uses injected clock (`now`) instead of `new Date()`, fixing test determinism when context.clock is provided - Nit: minimatch override tightened from `>=10.2.1` to `^10.2.1` - Nit: AjvPlugin type accepts optional options arg, returns Ajv instance - Nit: sha512 polyfill exported from crypto.ts, imported in signPatchFixture (eliminates duplication and redundant createHash import) - Nit: computeDigest parameter narrowed to Record<string, Json>
|
@coderabbitai review please 🥕 |
|
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CHANGELOG.md`:
- Line 5: Update the release header "## [1.0.0-alpha.5] - Milestone 5: WARP
Dashboard TUI Overhaul" to follow Keep-a-Changelog format by replacing the
milestone subtitle with an ISO date (e.g., "## [1.0.0-alpha.5] - 2026-02-20") or
"Unreleased", and move "Milestone 5: WARP Dashboard TUI Overhaul" to the next
line as a descriptive subheading or paragraph so the header matches the other
entries like "## [1.0.0-alpha.4] - 2026-02-17".
---
Duplicate comments:
In `@src/domain/services/OrchestrationFSM.ts`:
- Line 44: eventId generation now correctly uses the resolved now from
context.clock making the date fragment deterministic for frozen-time tests;
leave the template string that builds eventId (the expression using
now.slice(0,10).replace(...) and
Math.random().toString(36).slice(2,8).toUpperCase()) as-is and ensure the
variable now is sourced from context.clock before this line (check the
assignment to now and its usage in eventId).
Fixes: - DepAnalysis: transitiveCount fallback `?? direct` → `?? 0` (#10) Wrong fallback inflated blocker scores for tasks with DONE dependents. - DashboardApp: drawer render guard `> 0` → `> 4` to prevent negative content widths during early animation frames (#4) - DashboardApp: add onComplete to drawer tween for exact snap (#7) - DashboardApp: remove dead `focus-panel` from ViewAction union (#5) - dashboard-view: campaign DAG falls back to declaration order when sortedCampaignIds is empty after filtering (#8) - my-stuff-drawer: guard pw < 10 returns empty string (#1) - my-stuff-drawer: use true pendingReview.length for label (#2) - check-graph-algorithms.sh: quote $SCAN_DIRS (#16) Tests: - Fix drawer test to assert on drawer-unique content (#13) - Add [ / ] view cycling tests with wraparound (#14) - Add 6 renderMyStuffDrawer unit tests (agent scope, empty width, submissions filtering, activity feed, pending count label) (#3) Total: 729 tests (up from 721)
1 participant
Summary
*wildcards causingO(4^N)backtrackingoverridesinpackage.jsonto force all 6 transitive minimatch instances to>=10.2.1(resolves to 10.2.2)CLAUDE.mdChanges
minimatch >=10.2.1override inpackage.json;package-lock.jsonupdatedArray<T>→T[]in 5 fileshashes.sha512polyfill,loadKeyringnarrowing, ajv CJS interopvalidatePatchOps.tsandcrypto.tscrypto.ts,signPatchFixture.ts,validatePatchOps.tsOrchestrationFSMconverted from static class toconstobjectTest plan
npm run build— TypeScript compiles cleannpm run lint— zero errors, zero warningsnpm run test:local— 186/186 tests passnpm auditshows no minimatch vulnerabilitiesnpm ls minimatchconfirms all instances at 10.2.2Summary by CodeRabbit
Security
Documentation
Refactor
Bug Fixes