Fix npm overrides of git deps being refused (#490) - #491
Conversation
Assisted-by: Claude Code:claude-opus-5-5
When a dependency declares another package from git, a URL or file:, and the project's package.json "overrides" pins it back to a registry version, npm installs the registry release. socket-patch still treated that copy as installed from git: hosted scan skipped it and exited 0 with the package unpatched, vendored scan failed, and vex refused to attest a correctly patched install. The check that decides which lock entries come from the registry now reads the root package.json overrides (top-level rules, rules nested under the dependent or its ancestors, "." values and $name refs). An override that clearly sends the edge to a registry spec makes the copy patchable again in hosted mode, vendored mode and vex. Anything less clear keeps the old, cautious behavior. Fixes #490 Assisted-by: Claude Code:claude-opus-5-5
871c293 to
fbaea5b
Compare
The hosted capstone suite gains the #490 project shape: a local package depends on left-pad from git and the project's overrides pin it to the registry release. The test checks that scan redirects it, that a fresh npm ci installs the patched bytes, that vex verifies them, and that rollback restores the lock. It skips on npm older than 8.3, which has no overrides. Refs #490 Assisted-by: Claude Code:claude-opus-5-5
|
BugBot review Generated by Claude Code |
npm 9.0.0 can't install the #490 project at all: it dies with "Invalid comparator: github:..." when an override covers a git spec. The case doesn't exist on that npm, so the e2e returns instead of failing CI's pinned 9.0.0 leg. npm 9.9 and later run it fully. Refs #490 Assisted-by: Claude Code:claude-opus-5-5
|
BugBot review Generated by Claude Code |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Two override rules scoped to different ancestors at the same nesting depth tied, and the one later in key order won. npm uses the rule of the closest ancestor, so a farther registry rule could clear an edge that a closer git or file: rule keeps on that source. socket-patch could then patch or attest a copy npm still installs from the spec. Rules are now ranked by how close their ancestor is to the dependent, then by depth. Equally ranked rules that disagree count as unclear, so the copy stays reported as unpatched. Refs #490 Assisted-by: Claude Code:claude-opus-5-5
|
BugBot review Generated by Claude Code |
Mikola Lysenko (mikolalysenko)
added
the
Ready for review
|
Burn-down agent: ready for review at
Generated by Claude Code |
|
Reviewed
Validation: 17 |
Two override shapes still let socket-patch treat a git or URL dependency as a registry install while npm ci kept fetching the original spec: - A "*" or empty override is a no-op in npm, which keeps the dependent's raw spec, but socket-patch read "*" as a registry range and patched the entry. - A rule nested under a range selector such as "pkga@^1" was ignored, so a broader top-level registry rule cleared the entry even though npm uses the narrower rule. A winning "*", empty or nested-only rule now leaves the raw spec in effect. A range selector that may apply, or a target selector other than the edge's own spec, makes the result unclear, so the copy stays reported as unpatched. Refs #490 Assisted-by: Claude Code:claude-opus-5-5
The previous commit used bool::then with a closure where then_some is enough, which fails the CI clippy gate (-D warnings). Refs #490 Assisted-by: Claude Code:claude-opus-5-5
|
[agent] Both findings confirmed and fixed in 5dff221. fa440ae is a follow-up clippy fix.
The two classification tests fail on c657128 and pass now. Locally: 369 related core tests pass (npm_origin, npm_lock, VEX npm discovery, redirect), and Generated by Claude Code |
|
BugBot review Generated by Claude Code |
|
Follow-up review of P1: build-qualified ancestor selectors can still bypass the source guard ( At this exact head, the three independent public-rewriter probes produced 2 passed, 1 failed, confirming that remaining case. The same fixture was independently checked with npm 10.9.4 / Node 24.21.0: fresh-cache The 20 |
An override scoped to "pkga@1.0.0+build.1" applies to an installed pkga 1.0.0, because semver ignores build metadata. socket-patch compared the selector to the version as raw text, skipped the rule, and let a broader registry override patch a dependency that npm ci still fetches from its URL. Exact-version selectors now compare without build metadata: equal versions apply the nested rules, and different ones (prereleases included) still can't. Refs #490 Assisted-by: Claude Code:claude-opus-5-5
|
[agent] Confirmed and fixed in 3ef00ef. An exact-version enclosing selector is now compared to the ancestor's version under semver equality, with build metadata stripped on both sides.
Generated by Claude Code |
|
BugBot review Generated by Claude Code |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 3ef00ef. Configure here.
|
[agent] CI note on 3ef00ef: I don't believe this failure comes from this PR:
I'll re-run the failed job once when the workflow finishes; GitHub refuses a re-run while it's still running. If it fails again, I'll treat it as real and investigate. Generated by Claude Code |
|
Second-pass verification of I reran all three original independent public-rewriter probes at this exact head: 3 passed (wildcard no-op, uncertain range, and build-qualified ancestor). No further code change was needed. This supersedes my earlier changes-needed recommendation; this PR had already merged before this verification. |
Main now reads the root package.json for the npm lock rewriter as advisory input (#490/#491). Beside a berry yarn.lock the manifest is still read strictly, since the berry pin writes it; otherwise the advisory read applies. The berry-only memory selection rule is dropped because main always fetches package.json. Assisted-by: Claude Code:claude-opus-5-5
3 participants
LLM Description written by Claude Code:claude-opus-5-5
Fixes #490
Summary
A dependency can declare a package from git, a URL or
file:. When the project'soverridespin that package back to a registry version, npm installs the registry release. Since #345, socket-patch still treated that copy as coming from git:scanskipped it withredirect_npm_non_registry_entry_skipped, exited 0 and left the package unpatched;scanfailed withvendor_lock_entry_not_rewritable;vexrefused to attest an install that was correctly patched.All three now treat the overridden copy as the registry install it is. They patch it and attest it.
Root cause
vendor::npm_origin::npm_non_registry_entriesdecides which lock entries come from the registry. The hosted rewriter, the vendored backend and VEX discovery all share it. It flagged an entry whenever some dependent's raw spec for that name wasn't a registry spec, and it never looked atoverrides. npm doesn't recordoverridesin the lock (checked with npm 10: the root""entry has nooverrides, whileresolvedis the registry tarball andpkga's entry keepsgithub:…), so the lock alone can't tell this case apart.Changes
core
vendor/npm_origin.rs: newNpmOverrides, parsed from the rootpackage.json(overridesplus the root dependency specs, so$namereferences resolve).npm_non_registry_entries(lock, &overrides)drops a non-registry edge only when an override that clearly applies to it gives a registry spec. Applicable rules are:Exact parent selectors are compared under semver equality, so build metadata is ignored and
pkga@1.0.0+build.1matches an installed1.0.0. Values may be strings,"."values or$namereferences. As in npm, the rule scoped to the closest ancestor wins, then the most deeply nested one. A winning*, empty or nested-only rule is a no-op that leaves the raw spec in effect (npm'sedge.js). The answer is unclear, and no override clears the source check, when:x.y.z[-pre][+build](pkga@^1,1.x,=1.0.0, …) may apply and its subtree has a rule for the dependency;$namereference can't be resolved.The npm hosted and vendored modes rewire git-sourced lock entries, so npm ci silently installs the unpatched git bytes while scan and VEX report success #326 behavior covers everything unclear, so a doubtful copy stays reported as UNPATCHED and is never attested.
Hosted: the engine reads
package.jsonas advisory input when an npm candidate meets an npm lock. A symlinked or unreadable in-memory entry is left out, never refused. The in-memory selector fetches it (EXTRA_TEXT_FILES). The rewriter never writes it.Vendored (
vendor/npm_lock.rs):vendor_npmand the download-plan preflight read it once and pass it to the scan, the sibling-lock scan and the rewire.VEX (
vex/discover/npm.rs): lockfile discovery reads it quietly (no diagnostic, no recognition) next to the lock.CHANGELOG.md: added to the existing npm hosted and vendored modes rewire git-sourced lock entries, so npm ci silently installs the unpatched git bytes while scan and VEX report success #326 entry.The npm/pypi/gem wrappers only dispatch to the binary, so they need no change.
Tests
vendor::npm_origin::tests::issue_490_*(registry overrides of every supported shape, fresh and already-redirected locks; innermost wins; nested/scoped ancestors)issue_490_overrides_that_do_not_clearly_apply_keep_the_edgeissue_490_the_closest_ancestor_rule_wins_whatever_the_key_orderissue_490_equally_close_rules_that_disagree_keep_the_edge*/ empty overrides are no-ops (review P1)issue_490_wildcard_and_empty_overrides_leave_the_raw_specissue_490_range_selectors_that_may_apply_keep_the_edgeissue_490_exact_selectors_compare_without_build_metadata*,pkga@^1,pkga@1.0.0+build.1)patch::redirect::tests::issue_490_unclear_overrides_leave_a_url_dependency_unredirectedpatch::redirect::tests::issue_490_a_git_edge_overridden_to_the_registry_is_redirectedhosted::engine::tests::issue_490_the_root_manifest_overrides_reach_the_npm_rewritervendor::npm_lock::tests::issue_490_a_git_edge_overridden_to_the_registry_is_vendoredvendor_lock_entry_not_rewritable)vex::discover::npm::tests::issue_490_a_git_edge_overridden_to_the_registry_is_attested(hosted + vendored wiring)e2e_redirect_npm_build::npm_redirect_overridden_git_dependency_installs_patched_bytesThe existing #326 tests (
npm_non_registry_entries_are_skipped_with_loud_warning,non_registry_only_instances_refuse_and_write_nothing,entries_npm_installs_from_a_non_registry_spec_are_not_attested, the goldenindexed_npm_lock_rewrite_matches_golden) still pass unchanged.Real-npm e2e. The new test reuses the hosted capstone fixture with a real
npm installof the #490 project shape. Each run checks four things:scan --mode hosted --vexreportsredirected: 1, writes the lock pin and emits one in-run VEX statement;npm ciinstalls the patched bytes byte for byte;vexattests them;rollbackrestores the lock.CI's npm compatibility matrix runs it end to end on npm 9.9.4, 10.9.9, 11.20.0, 12.0.0 and 12.1.0. It is N/A, not a skip (
skip()fails the pinned REQUIRED legs), in two cases:overrides;Invalid comparator: github:…when an override covers a git spec.CI on 3ef00ef. All 11 workflows pass: CI, npm, pnpm, Bun, vlt, Poetry, PDM, Pipenv, Go, Composer and the GHA audit. Bugbot found no issues on this commit. The first attempt of Poetry 1.1.15 failed one case (
direct vendored,rescanIdempotent). This PR doesn't touch Poetry code, the case passed on fa440ae and onmain, and the one re-run passed.Local runs (on 3ef00ef):
cargo clippy --workspace --all-features -- -D warnings: exit 0.npm_origin,npm_lock, VEX npm discovery, redirect).cargo test --workspace --all-features --no-fail-fastpassed except 12 chmod/read-only "write failure" tests, which can't fail under uid 0 in this sandbox (the same set as other branches; see Fix berry mode takeover reverting before gates (#468, #369) #470).🤖 Generated with Claude Code
https://claude.ai/code/session_01NLJYcEVmFmVgtyWFgtLjez