feat(supply-chain): resolve npm dependencies through the lockfile - #344
feat(supply-chain): resolve npm dependencies through the lockfile#344Mark2Mac wants to merge 1 commit into
Conversation
08ab40d to
79771bf
Compare
rng1995
left a comment
There was a problem hiding this comment.
[Automated SkillSpector Review]
Approved. The lockfile parser covers npm lockfile v1 and v2/v3 layouts, preserves distinct installed versions, resolves manifest ranges only from the shallowest direct install, keeps npm normalization separate from PyPI, and avoids quadratic line lookup. Exact-head verification passed all 270 supply-chain pattern tests plus Ruff lint and format checks.
SC4 read package.json and stopped there. For npm that is the smaller half: the manifest lists direct dependencies, usually as a range, and the versions actually installed -- direct and transitive -- are in package-lock.json, which was never read. Python has no such gap; uv.lock and poetry.lock are already read and preferred. Read package-lock.json and npm-shrinkwrap.json the same way: resolve manifest ranges to the version on disk, and scan the lockfile itself so transitive dependencies are covered. Both layouts are handled -- lockfileVersion 2/3 keyed by install path, and version 1 with nested dependencies. Three details are load-bearing: - Deduplication is by name and version, not by name. npm installs the same package at several versions routinely, nesting the ones it cannot hoist; keeping one entry per name drops the others silently, and a dropped copy is as installed as the one kept. - The npm and PyPI version maps are separate. semver, packaging and requests all exist in both ecosystems, so one shared map would answer a Python question with an npm version. Name normalization differs for the same reason: PyPI folds _ into -, npm does not, and string_decoder and string-decoder are two real packages. - Line numbers come from one indexing pass. Searching per package is quadratic, which cost 6.3s on a 5000-entry lockfile; lockfiles that size are ordinary. Signed-off-by: Marco Macrì <Mark2Mac@users.noreply.github.com>
79771bf to
33447d3
Compare
|
Rebased onto The conflict was with #357, which added SC8 to the same module. It was confined to the import block — #357 adds The approved content is unchanged. Verified after the rebase, on the exact head:
SC8 and this PR do not interact: SC8 flags shipped bytecode during discovery, the lockfile resolution runs over dependency manifests. |
|
Field evidence from a package installed today, which sharpens one claim in the PR body.
The correction. The PR body says the name-and-version deduplication removes a class of miss "without changing any finding count", because on the corpus I had measured, it didn't. On this package it does: For completeness on severity: none of the 18 are installed on this machine (no |
The gap
SC4 reads
package.jsonand stops there. For npm that is the smaller half of the problem:"^4.18.0"). After fix(supply-chain): SC4 must not claim a vulnerability it did not verify (#318) #319 a range correctly resolves to no version, so there is nothing to ask OSV about — honest, but silent.package-lock.json. That file is never read.Python does not have this gap: #263 already reads
uv.lockandpoetry.lockand prefers those versions. npm had no lockfile support at all.The change
Read npm lockfiles —
package-lock.jsonandnpm-shrinkwrap.json— and use them the same way:"commander": "^11.0.0"with a lockfile is11.1.0. Note it is not11.0.0, which is what the old caret-stripping produced and what a reader would guess.uv.lockalready gets.Both layouts are handled:
lockfileVersion2/3 (keyed by install path, including nestednode_modules/a/node_modules/b) and version 1 (nesteddependencies). The root entry is the project itself and is skipped; aliased installs use the declaredname.Three details that are load-bearing
Each of these is a silent-wrong-answer trap, so each has a test that fails without it.
Deduplication is by name and version. npm installs the same package at several versions routinely, nesting the copies it cannot hoist. Keying on the name alone keeps whichever entry came first and drops the rest — and the dropped copy is on disk, so a vulnerable one would simply go unreported. On the corpus below, 12 packages are installed at two versions each; none of those 24 versions happens to carry an advisory, so this changed no finding here. It removes a class of miss, not a count.
The ecosystem maps are kept separate.
semver,packagingandrequestsall exist in both PyPI and npm. One shared map would answer a Python question with an npm version. Name normalization differs for the same reason:_normalize_package_namefolds_into-, which is right for PyPI and wrong for npm, wherestring_decoderandstring-decoderare two different real packages.Manifest ranges resolve to the direct install. A range in
package.jsonnames a direct dependency, so it resolves to the top-level copy — not to a nested one that exists only to satisfy some other package's constraint.Cost
Looking a line number up per package is quadratic: both the search and the offset-to-line conversion restart at the top of the file each time. One indexing pass instead:
5000-entry lockfiles are ordinary, so the quadratic version was not a theoretical concern. A regression test asserts 2000 packages parse in under a second.
Measured
5 lockfiles found in Claude Code plugins on a real machine, 458 installed packages:
Each was re-verified by querying OSV directly for that exact version, not just trusting the analyzer:
@modelcontextprotocol/sdk==1.20.1undici==7.25.0ws==8.19.0path-to-regexp==8.3.0js-yaml==3.14.2,picomatch==2.3.1,brace-expansion==1.1.12None of these appear in any
package.json. They are exactly the dependencies a manifest-only scan cannot see, and the first row is the kind an agent-security scanner should not be missing.Volume stays sane: 458 packages produced 11 findings, not hundreds — the exact-version match is what keeps it quiet.
Scope
yarn.lockandpnpm-lock.yamlare deliberately not here. They are different formats with their own parsing problems, and bundling them would make this change harder to review than it needs to be. The seam they would plug into —_npm_lock_entriesreturning(name, version, line, depth)— is the same one this PR adds.Tests
Both lockfile layouts, aliased installs, the skipped root entry, malformed JSON, ecosystem separation, npm name normalization, exact-pin-beats-lockfile precedence, multi-version installs, direct-vs-nested resolution, and the parse-cost regression. Plus two analyzer-level ones:
LOWunverifiable to a version-matchedHIGHonce the lockfile is present — the case fix(supply-chain): SC4 must not claim a vulnerability it did not verify (#318) #319 deliberately made quiet, now made verifiable instead.Full suite green (1751 passed),
ruff checkandruff format --checkclean at 0.15.19.Relationship to #323
Independent, and measured rather than assumed: merging #323 into this branch is a clean merge, and the combined tree passes. Either can land first.
They do compound, though. The tests here build manifests with
json.dumps(..., indent=2)on purpose, because a compactpackage.jsonstill yields zero dependencies onmain— the line-oriented scan never enters the dependency section, which is what #323 fixes. Until that lands, range resolution has nothing to resolve on a one-line manifest. The lockfile scan, which is where all 11 findings above come from, is unaffected either way.