fix(overrides): emit >= security floors instead of exact pins - #135
Merged
Conversation
applyOverrides was writing exact version pins into pnpm.overrides / overrides / resolutions (and the npm direct-dependency path), which permanently blocks routine updates past a vulnerable version — 29/32 fleet projects were stuck this way on GHSA-r28c-9q8g-f849 via postcss. - applyOverrides now writes a caret floor (^8.5.26) for concrete targets via toFloorRange(); dist-tags and already-range values pass through unchanged. The npm direct-dependency write and the post-install verify/anyResolved checks are updated to match (floor satisfaction, not exact equality). - removeOverrideConflicts now removes an override only when the new target doesn't satisfy the existing range (semver.satisfies), instead of a plain string comparison that would have deleted the floor it just wrote. Floating dist-tags still force removal. - cleanStaleOverrides keeps skipping range-valued overrides (now documented as deliberate, not incidental) and replaces the hand-rolled parseInt version comparison with real semver comparison for the exact-pin case, fixing mishandling of prereleases and build metadata. Adds semver/@types/semver as real (non-phantom) dependencies, and src/lib/updaters/override.test.ts covering all three functions plus package.json indentation preservation.
Follow-up to 5491c8e per independent review: - Switch floors from caret to >= (owner decision): caret is inert below 1.0.0 (^0.0.3 expands to exactly one version, byte-identical to the exact pin this change removes) and always caps at the next major even above 1.0.0, so it can never admit a fix that ships in a later major. >= also matches this repo's own existing override convention (esbuild, qs, hono, ws). - removeOverrideConflicts / applyOverrides's post-install verification now compare against the range actually written (satisfiesWrittenOverride), not a bare "installed >= target" scalar, so a genuine mismatch (keyed override, workspace catalog, parent constraint) is still caught instead of rubber-stamped. - The anyResolved install-failure fallback no longer lets a pre-existing copy identical to fromVersion masquerade as a completed resolution when the install itself failed. - Dropped includePrerelease from the conflict-satisfaction check so it matches actual npm/pnpm resolver semantics. - Corrected toFloorRange's JSDoc claim about prerelease matching under >= (empirically re-verified rather than reasoned from the old caret-era wording). Adds tests for all of the above plus previously-untested edge cases (non-semver pinned values, wildcard/empty override values).
Follow-up to 3a604a8 per second review round, two owner decisions plus one more bug in the fromVersion guard: - Bare >= floors stay unbounded (owner decision), but the post-install verify block now reports what actually resolved, not just the target: adds resolvedVersion to UpdateResult/PatchHistoryEntry (additive, does not repurpose toVersion) and threads it through the output string, override_applied log, and patch-history entry so "requested >=8.5.26, resolved 10.1.0" is visible instead of a false audit trail recording only the target. - Adds an override_major_jump warning when the resolved major exceeds the targeted major (satisfies the floor, so success stays true). - Fixes the verification blind spot: the post-install check only ever looked at root node_modules, which is a silent no-op for any project using pnpm's default node-linker=isolated (transitive deps live in the .pnpm virtual store instead). Adds a store-aware lookup (root, then .pnpm store, then a pnpm list fallback for pnpm when both come up empty), checks every discovered copy against the floor rather than the first one found (catches the issue #80 false-clear class), and surfaces "could not verify" as a distinct state instead of silently treating an inconclusive probe as success. Extended the same store-aware lookup to the anyResolved install-failure fallback, leaving its batch-recovery shape untouched. - Fixes route.ts pushing the raw (often-absent) request fromVersion instead of the already-computed effectiveFromVersion, which left the previous round's stale-tree guard inert for any request that omits fromVersion. Adds 7 tests covering major-jump warnings, .pnpm-store resolution (including scoped packages and multiple coexisting versions), the pnpm list fallback, the inconclusive-verification state, and the anyResolved store-aware recovery path.
…ute lookup Follow-up to 3315dfd per round-3 review, F1-F5 plus minors: - F1/F2: resolvedVersion selection in applyOverrides's verify loop used probe.versions.sort(semver.rcompare) directly, which throws on any unparseable version field once a second copy exists (Array.sort never invokes the comparator for a single element, which is why single-copy cases were safe and multi-copy garbage wasn't). The throw landed in the loop's catch-all, silently discarding already-computed violations and reporting success with no warning at all -- worse than the pre-round-3 no-op. Restructured to compute violations first via the already-throw-safe findViolatingVersions, then pick resolvedVersion via new pickPrimaryVersion/worstVersion helpers that filter to parseable semver before sorting. When copies disagree, resolvedVersion now reports the worst (most vulnerable) violator, not the best-looking copy, so the structured field can't read as clean when a violation exists. - F3: installPackages' version-unchanged guard flagged failure whenever installed.version === fromVersion, with no check against the target -- benign once route.ts started passing a real, node_modules-read fromVersion (fixed last round) instead of the usually-absent raw request field. Narrowed the guard so "already at target" is not reported as a failure, since that's the guard's intended failure signal ("install exited 0 but nothing moved"), not a policy change. - F4: route.ts's effectiveFromVersion read was the same root-only node_modules blind spot fixed inside override.ts for the same class of (transitive, override-managed) package. Now reuses override.ts's exported findAllInstalledVersions + pickPrimaryVersion instead of a second, narrower inline lookup. - F5: added maxBuffer to the pnpm list CLI fallback, matching the convention already used elsewhere in this codebase, so large monorepos don't silently lose the fallback to the default 1MB cap. - Minors: prefix-collision test, route.ts results typed as UpdateResult[], workspace-root limitation documented on findPnpmStoreVersions. Adds src/lib/updaters/install.test.ts (new) plus more coverage in override.test.ts for the throw-path and multi-copy resolvedVersion cases.
…y one Follow-up to cafb0bd per round-4 review, B1 (a regression) and B2: - B1: route.ts's pre-install downgrade guard was calling pickPrimaryVersion(root, all) to read the currently-installed version, and pickPrimaryVersion prefers the HIGHEST copy when there's no root entry. On a pnpm isolated-linker project with no root copy and two store copies -- a vulnerable one and a clean one -- that fed the guard the clean copy, which read as "already past this fix" and refused the whole update before applyOverrides ever ran, leaving the vulnerable copy untouched. That's worse than before: previously the root-only read returned '', the guard was simply inert, and the request went through. A downgrade guard has to ask "is anything still below target", which needs the WORST case across every discovered copy, not a "primary" pick -- those are different questions that only shared a call site because pickPrimaryVersion was the only exported picker. Added pickLowestVersion, a distinctly named and purposed export, and pointed route.ts at it instead. pickPrimaryVersion keeps its original job (reporting "what resolved" once every copy is already confirmed to satisfy the floor) and is unchanged. - B2: pickPrimaryVersion/worstVersion filtered candidate versions with semver.valid(v, {loose:true}) but sorted with the bare semver.rcompare/compare function references, which run WITHOUT the loose flag when called as a .sort() comparator -- so a version loose parsing accepts but strict parsing rejects (e.g. a leading-zero segment) would pass the filter and then throw in the sort. Since resolvedVersion was being computed before the mismatch warning was assigned, that throw discarded an already-detected violation on its way to the outer catch-all -- silent success, no warning. Fixed by passing {loose:true} through to the sort comparator too, and, structurally, by moving the warning assignment and log call before version selection runs, so a future throw during selection can only ever cost the resolvedVersion field, never the warning itself. Adds two pinning tests in override.test.ts: one reproducing the exact multi-copy isolated-layout scenario end to end (proves the old picker would have refused, the new one doesn't, and the override is actually written), and one with a loose-valid/strict-invalid version alongside a normal one, both violating, confirming the mismatch warning still fires instead of going silent.
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
1 participant
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.
A fleet audit found 29 of 32 projects carrying exact postcss pins like
"postcss": "8.5.15". Eight were stuck on versions vulnerable to GHSA-r28c-9q8g-f849 — and an override forces a version, so no routine update could move them. The override was what held them there.This file is where those pins came from: it wrote
pkg.targetVersionverbatim, and its sibling logic assumed exact pins throughout.What changed
applyOverridesemits a>=floor for concrete versions, matching the convention already used by this repo's ownesbuild/qs/hono/wsoverrides. Dist-tags, git URLs,workspace:*and npm aliases pass through untouched — a caret or floor can't be prefixed to those.removeOverrideConflictsuses semver satisfaction instead of string equality. Without this the tool would have deleted the floors it had just written ("^8.5.23" !== "8.5.26"), logged asoverride_conflict_removed.cleanStaleOverridesstill skips range-valued overrides — deliberately, now documented. A working floor looks "stale" under an installed-is-newer rule, and removing it would strip security protection. Its exact-pin comparison moved from hand-rolledparseIntto semver.node_modules/<pkg>. These are transitive deps; under pnpm's default isolated linker they live in.pnpm/, so the check was a silent no-op on most of the fleet. It now scans the pnpm store, collects every copy, and flags a violation if any copy falls below the floor — the issue-False positive: fixViaOverride patches report success when nested copy is still vulnerable #80 nested-copy class.>=floor resolves tomaxSatisfying, so it can install a major beyond the target.resolvedVersionnow records what actually landed alongside what was requested, and a major jump gets its own warning.Why
>=and not a caretCaret is inert below 1.0.0:
^0.0.3expands to>=0.0.3 <0.0.4-0— one version, identical in effect to the exact pin this change exists to remove.^0.5.1caps at<0.6.0, and in0.xthe minor is the breaking axis.The trade is real:
>=has no ceiling, so an override can admit a major. That's why the verification and resolved-version reporting above aren't optional extras — they're what makes an unbounded floor safe to run unattended.Review history
Four review rounds. Three findings are worth calling out because they'd each have been shipped silently:
installed >= targetrather than "satisfies the range we wrote" — so a resolution outside the written range read as success.semverthrow on an unparseable version was swallowed by a catch-all, producingsuccess: truewith no warning and no log, while the violating versions sat in a local variable. Strictly worse than the no-op it replaced.8.4.31beside a clean8.5.30read as "already past this fix" and the update was refused — leaving the vulnerable copy in place, on exactly the multi-copy isolated layouts this work targets. A downgrade guard needs the lowest copy. Both behaviours are now pinned by tests.Verification
336 → 382 tests, 46 files.
tsc --noEmitclean,pnpm buildexit 0.src/lib/updaters/had no test file before this; it now has two.semverwas previously only a transitive dependency — it's now properly declared, so this doesn't introduce the phantom-dep class HexOps itself detects.Follow-ups, deliberately not included
overridePkgs.some(...)batch semantics (one package resolving marks the batch recovered),escalate/route.ts's duplicate hand-rolled version comparison, and wiringresolvedVersioninto the patch-history UI and CSV export.