fix(npm): honor consuming repo overrides in closure probe (v2.12.0) - #19
Merged
Conversation
safedeps check resolved a package's *published* dependency closure via a throwaway probe with an empty package.json, ignoring the consuming repo's npm `overrides`. A transitive the repo had already pinned to a patched version (the canonical remediation) was still false-flagged — e.g. exceljs -> uuid@8.3.2 (GHSA-w5hq-g745-h8pq) stayed blocked even with `overrides: { "uuid@8.3.2": "^11.1.1" }` in place. Add safedeps_npm_repo_overrides_json: discover the consuming repo's overrides (SAFEDEPS_NPM_OVERRIDES_JSON env, else nearest package.json up from $PWD bounded by the git root) and inject them into the closure probe so npm resolves transitives the way the real install will. Only concrete version pins are honored — $-ref strings and objects mentioning $ are dropped (no meaning in the standalone probe). This cannot hide a real vuln: the probe still resolves each override to a concrete version and OSV is queried for that exact version. Minor bump 2.9.1 -> 2.10.0; smoke covers concrete-pin/$-ref-drop/no-override/env.
… dir
The overrides walk-up broke out on `[[ -d "${dir}/.git" ]]`, but a git
worktree root carries a `.git` FILE, not a directory. Inside any worktree
the walk therefore stepped straight past the project root and picked up an
ancestor's `overrides` -- which are not the overrides the real install in
that worktree will use.
The Yarn project-context walk-up in the same file already got this right
and says so in a comment; this is the same test, so the two boundaries now
agree.
Regression test simulates a worktree root with a `.git` file and an
ancestor carrying `overrides`, and asserts the ancestor's pins do not leak
in. Mutation-verified: reverting to `-d` makes the new test fail with
`got: {"leaked":"9.9.9"}`.
Honoring `overrides` made the probe closure a function of the consuming
project, but the approval key stayed global. An approval earned in a repo
that pinned a vulnerable transitive to a patched version therefore
satisfied the same check in a repo that had not, whose real install
resolves the vulnerable version. Reproduced: repo A with
`overrides: {minimist: 1.2.8}` approves mkdirp@0.5.1, then repo B with no
overrides returns already_approved while resolving minimist@0.0.8.
A published-package approval may be global only because it is
project-independent. Once overrides apply, it is not, so it is now keyed
the way the Yarn project closure already was, reusing that machinery
rather than adding a parallel contract:
- closure.sh gains `safedeps_npm_overrides_context`, emitting a
`npm-overrides-probe` context with project_root, the override set, its
canonical (key-sorted) hash, and a context_hash over both.
- ledger.sh requires those fields for the new type and rejects an empty
override set, matching how it gates the Yarn contexts.
- bin/safedeps wires the context at both approval sites and derives the
same hash for the cache lookup, so a scoped approval still hits.
- the pre-guard derives the same key, so an approved install is not
blocked by a key the guard could not reproduce.
Repos with no overrides get no context and keep the ordinary global
approval. Failing to apply overrides to the probe manifest is now logged
instead of silently dropping them; it only makes the check stricter, but
an unexplained denial is not observable.
Tests: approval-key scoping and order-independence, ledger provenance
gating, and a hermetic e2e that stubs npm so the resolved closure depends
on the probe manifest — asserting patched=clean, still-vulnerable=deny,
no-overrides=no leak, same-set=already_approved. Both mutation-verified.
Docs: English SSoT + Korean mirrors describe the behavior, the
concrete-pins-only limit, and the approval scope.
Both mktemp failures in safedeps_npm_resolve_spec_closure returned without removing what was already allocated: the new overrides-source file dropped tmp_dir and the project context, and the pre-existing tmp_dir line dropped the project context. Every other early return in this function already cleans up; these now match.
The guard derives the approval key independently of the check. If the two ever disagree, an approved install is denied at the gate — a functional regression the existing tests would not catch, since they only exercised the check side.
…t root The probe resolves from an empty manifest, so its closure depends only on the override set — the project root is in the key deliberately, not by accident. Record the tradeoff (a redundant resolve for a second repo with identical overrides, in exchange for an auditable entry and parity with the Yarn context key) so it does not read as an oversight.
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.
Revived from a stale local branch (originally authored 2026-06-29, rebased onto
maintoday). It was sitting unmerged with a version collision; nothing inmainsupersedes it.Problem
safedeps checkresolves a package's published dependency closure through a throwaway probe with an emptypackage.json, which ignores the consuming repo's npmoverrides. A transitive that the repo had already pinned to a patched version -- the canonical remediation -- was still false-flagged.Concrete case:
exceljs->uuid@8.3.2(GHSA-w5hq-g745-h8pq) stayed blocked even withoverrides: { "uuid@8.3.2": "^11.1.1" }in place.Approach
safedeps_npm_repo_overrides_jsondiscovers the consuming repo'soverridesand injects them into the closure probe, so npm resolves transitives the way the real install will.SAFEDEPS_NPM_OVERRIDES_JSONenv (explicit JSON, for tests and non-cwd callers), else the nearestpackage.jsonwalking up from$PWD, bounded by the git root.$-ref strings ("$react") and objects mentioning$are dropped -- they have no meaning in a standalone probe and would break resolution.This cannot hide a real vulnerability. The probe still resolves each override to a concrete version and OSV is queried for that exact version -- the version that will actually be installed. An override pointing at a still-vulnerable version is flagged like any other.
Relationship to v2.10/v2.11
This is the npm-
overridesanalogue of the Yarn-resolutionswork, and the two do not interact.safedeps_npm_resolve_spec_closuretakes the Yarn project path first; when a Yarn context exists it approves, materializes, or fails closed and returns -- it never falls through to the published probe. Overrides injection lives only on the published-probe path, reached when there is no Yarn project context. The v2.11 no-published-fallback invariant is untouched.Rebase notes
2.10.0, whichmainhas since used for Yarn resolution-aware check. Rebased to 2.12.0 (package.json==bin/safedepsSAFEDEPS_VERSION).lib/npm/closure.shconflicted because both branches added functions at the same insertion point. Both sides kept;safedeps_npm_write_sourceandsafedeps_npm_repo_overrides_jsoneach retain their own closing brace.assets/demo.tapefix for thealex-extensions->agent-extensionsrename, whichmainstill lacks.Verification
npm test(smoke + e2e) green, includingnpm closure honors repo overrides (concrete pins kept, $-refs dropped),npm closure with no repo overrides is unchanged, andnpm closure override env source precedenceTMPDIR(the cross-platform trap that bit PR feat(check): materialize absent Yarn candidates in an isolated mirror (v2.11.0) #18)bash -non all touched scripts,git diff --check, version SSoT checkNot done here
Docs are not updated for this feature, and this is hard-floor code that has not been through a validator loop. Review before merge.