From 9a75c0de69663e183370677d414d9229011b33bc Mon Sep 17 00:00:00 2001 From: tgolob <34978067+tgolob@users.noreply.github.com> Date: Fri, 21 Aug 2026 23:40:21 -0400 Subject: [PATCH] fix(npm): derive direct dependencies from root lock entry --- docs/inventory-sources.md | 7 +++ internal/ecosystem/npm/npm.go | 50 +++++++++++++----- internal/ecosystem/npm/npm_test.go | 82 +++++++++++++++++++++++++++++- 3 files changed, 126 insertions(+), 13 deletions(-) diff --git a/docs/inventory-sources.md b/docs/inventory-sources.md index 1e64555..0dd886c 100644 --- a/docs/inventory-sources.md +++ b/docs/inventory-sources.md @@ -96,6 +96,13 @@ URL and integrity hash in the lockfile are read for parsing but are intentionally not emitted in v0.1's slim schema (see [`internal/model/model.go`](../internal/model/model.go)). +For lockfile versions 2 and 3, `direct_dependency` is derived from the +root package descriptor at `packages[""]`, not from physical +`node_modules` depth, because npm may hoist transitive packages to the +top level. Hidden `node_modules/.package-lock.json` files omit that root +descriptor, so records sourced only from a hidden lockfile leave +`direct_dependency` absent rather than guessing. + References: - npm `package-lock.json` format: diff --git a/internal/ecosystem/npm/npm.go b/internal/ecosystem/npm/npm.go index 832bec8..cdc9952 100644 --- a/internal/ecosystem/npm/npm.go +++ b/internal/ecosystem/npm/npm.go @@ -38,12 +38,16 @@ type lockfile struct { } type lockEntry struct { - Version string `json:"version"` - Name string `json:"name"` - Dev bool `json:"dev"` - Optional bool `json:"optional"` - Link bool `json:"link"` - Scripts map[string]string `json:"scripts"` + Version string `json:"version"` + Name string `json:"name"` + Dev bool `json:"dev"` + Optional bool `json:"optional"` + Link bool `json:"link"` + Scripts map[string]string `json:"scripts"` + Dependencies map[string]string `json:"dependencies"` + DevDependencies map[string]string `json:"devDependencies"` + OptionalDependencies map[string]string `json:"optionalDependencies"` + PeerDependencies map[string]string `json:"peerDependencies"` } type lockDepV1 struct { @@ -136,6 +140,8 @@ func (s *Scanner) ScanLockfile(path string, base model.Record) error { switch { case len(lf.Packages) > 0: // lockfileVersion 2 or 3 + root, hasRoot := lf.Packages[""] + directNames := root.directDependencyNames() keys := make([]string, 0, len(lf.Packages)) for k := range lf.Packages { keys = append(keys, k) @@ -155,7 +161,6 @@ func (s *Scanner) ScanLockfile(path string, base model.Record) error { if name == "" || entry.Version == "" { continue } - direct := isDirectFromKey(key) scripts := scriptKeys(entry.Scripts) r := base r.Ecosystem = Ecosystem @@ -166,8 +171,12 @@ func (s *Scanner) ScanLockfile(path string, base model.Record) error { r.PackageManager = pm r.SourceType = "npm-lockfile" r.SourceFile = path - d := direct - r.DirectDependency = &d + if hasRoot { + installName := nameFromPackagesKey(key, "") + _, declaredAtRoot := directNames[installName] + direct := isTopLevelPackageKey(key) && declaredAtRoot + r.DirectDependency = &direct + } r.HasLifecycleScripts = len(scripts) > 0 r.LifecycleScripts = scripts r.InstallScope = installScope(entry.Dev) @@ -290,9 +299,26 @@ func nameFromPackagesKey(key, explicit string) string { return tail } -// isDirectFromKey: a top-level dep has exactly one "node_modules/" segment. -func isDirectFromKey(key string) bool { - return strings.Count(key, "node_modules/") == 1 +// isTopLevelPackageKey reports whether key is physically installed directly +// under the root node_modules. This is necessary but not sufficient for a +// direct dependency because npm also hoists transitive packages there. +func isTopLevelPackageKey(key string) bool { + return strings.HasPrefix(key, "node_modules/") && strings.Count(key, "node_modules/") == 1 +} + +func (e lockEntry) directDependencyNames() map[string]struct{} { + out := make(map[string]struct{}) + for _, dependencies := range []map[string]string{ + e.Dependencies, + e.DevDependencies, + e.OptionalDependencies, + e.PeerDependencies, + } { + for name := range dependencies { + out[name] = struct{}{} + } + } + return out } func scriptKeys(m map[string]string) []string { diff --git a/internal/ecosystem/npm/npm_test.go b/internal/ecosystem/npm/npm_test.go index 1de09e2..897f465 100644 --- a/internal/ecosystem/npm/npm_test.go +++ b/internal/ecosystem/npm/npm_test.go @@ -39,7 +39,12 @@ func TestScanLockfileV3ScopedAndUnscoped(t *testing.T) { "version": "1.0.0", "lockfileVersion": 3, "packages": { - "": { "name": "demo", "version": "1.0.0" }, + "": { + "name": "demo", + "version": "1.0.0", + "dependencies": { "lodash": "^4.17.21" }, + "devDependencies": { "@tanstack/query-core": "^5.0.0" } + }, "node_modules/lodash": { "version": "4.17.21", "resolved": "https://registry.npmjs.org/lodash/-/lodash-4.17.21.tgz", @@ -95,6 +100,81 @@ func TestScanLockfileV3ScopedAndUnscoped(t *testing.T) { } } +func TestScanLockfileV3DirectnessUsesRootDeclarations(t *testing.T) { + dir := t.TempDir() + lock := filepath.Join(dir, "package-lock.json") + writeFile(t, lock, `{ + "name": "demo", + "version": "1.0.0", + "lockfileVersion": 3, + "packages": { + "": { + "name": "demo", + "version": "1.0.0", + "dependencies": { "is-odd": "^3.0.1" } + }, + "node_modules/is-number": { "version": "6.0.0" }, + "node_modules/is-odd": { "version": "3.0.1" } + } +}`) + + s, got, _ := newCollector() + if err := s.ScanLockfile(lock, model.Record{}); err != nil { + t.Fatalf("ScanLockfile: %v", err) + } + records := map[string]model.Record{} + for _, r := range *got { + records[r.PackageName] = r + } + if r := records["is-odd"]; r.DirectDependency == nil || !*r.DirectDependency { + t.Errorf("is-odd should be direct: %+v", r) + } + if r := records["is-number"]; r.DirectDependency == nil || *r.DirectDependency { + t.Errorf("hoisted is-number should be transitive: %+v", r) + } +} + +func TestScanHiddenLockfileLeavesDirectnessUnknown(t *testing.T) { + dir := t.TempDir() + lock := filepath.Join(dir, "node_modules", ".package-lock.json") + writeFile(t, lock, `{ + "lockfileVersion": 3, + "packages": { + "node_modules/is-number": { "version": "6.0.0" }, + "node_modules/is-odd": { "version": "3.0.1" } + } +}`) + + s, got, _ := newCollector() + if err := s.ScanLockfile(lock, model.Record{}); err != nil { + t.Fatalf("ScanLockfile: %v", err) + } + if len(*got) != 2 { + t.Fatalf("got %d records, want 2", len(*got)) + } + for _, r := range *got { + if r.DirectDependency != nil { + t.Errorf("%s direct_dependency = %v, want unknown", r.PackageName, *r.DirectDependency) + } + } +} + +func TestIsTopLevelPackageKey(t *testing.T) { + tests := map[string]bool{ + "node_modules/lodash": true, + "node_modules/@scope/pkg": true, + "node_modules/a/node_modules/b": false, + "packages/worker/node_modules/lodash": false, + "packages/worker/node_modules/@scope/pkg": false, + "packages/worker/node_modules/a/node_modules/b": false, + } + for key, want := range tests { + if got := isTopLevelPackageKey(key); got != want { + t.Errorf("isTopLevelPackageKey(%q) = %v, want %v", key, got, want) + } + } +} + func TestScanLockfileV1(t *testing.T) { dir := t.TempDir() lock := filepath.Join(dir, "npm-shrinkwrap.json")