From 6ad7defd0378186e13272840e998c28ce34f5e16 Mon Sep 17 00:00:00 2001 From: milosde111 Date: Fri, 24 Jul 2026 08:22:36 -0700 Subject: [PATCH] feat(lockfile): enhance handling of unattributed lockfile entries across hunk boundaries Introduces a new function to recover package names from resolved URLs, allowing for better correlation of version and integrity changes across separate hunks. This change addresses issues where legitimate version bumps could be falsely flagged due to the split of entries in the lockfile. Additionally, several regression tests have been added to ensure correct behavior in various scenarios involving unattributed entries, including cases where package names are inferred from resolved URLs. This implementation aims to improve the accuracy of lockfile tamper detection and maintain the integrity of package management processes. Closes #8351. --- src/review/lockfile-tamper.ts | 73 ++++++++++++- test/unit/lockfile-tamper.test.ts | 171 ++++++++++++++++++++++++++++++ 2 files changed, 241 insertions(+), 3 deletions(-) diff --git a/src/review/lockfile-tamper.ts b/src/review/lockfile-tamper.ts index db68a0dac3..ee33c6280b 100644 --- a/src/review/lockfile-tamper.ts +++ b/src/review/lockfile-tamper.ts @@ -48,6 +48,15 @@ function npmPackageFromNodeModulesPath(path: string): string | null { return rest.split("/")[0] || null; } +/** Recover a bare package name from an npm-registry `resolved` URL (e.g. context lines in a hunk whose + * `"node_modules/": {` header fell outside git's 3-line window). Used to key unattributed fallback + * buckets so version + resolved/integrity signals for the SAME package can merge across hunk boundaries + * (#8351) instead of each depth-0 `}` minting a fresh `#unattributed-N` sequence key. */ +function packageNameFromResolvedUrl(url: string): string | null { + const match = /^https:\/\/registry\.npmjs\.org\/((?:@[^/]+\/)?[^/]+)\//i.exec(url); + return match?.[1] ?? null; +} + /** True when `path`'s basename is `package-lock.json` — the only lockfile format this check parses today * (npm/lockfileVersion 2-3 JSON shape). Matches ANY directory depth (root, `review-enrichment/`, * `apps/loopover-ui/`, or a future workspace) rather than a hardcoded path list, so a new workspace package @@ -130,8 +139,15 @@ function scanPackageLockPatch(path: string, patch: string): LockfileTamperCandid // a change was silently dropped: `currentEntryKey` stayed null for the whole hunk, so the // `!currentEntryKey ... continue` guard below skipped it -- a tampered field could evade detection // entirely just by having enough unchanged sibling fields ahead of it in its entry. + // + // When a package name can be recovered from a nearby `resolved` URL (including unchanged context + // lines), the fallback key is `${path}#unattributed:` so two hunks that each close with a + // depth-0 `}` (which clears `activeUnknownKey`) still merge into ONE candidate for that package + // (#8351). Without a recoverable name we keep the per-block `#unattributed-N` sequence — two + // genuinely different nameless blocks must never collapse into each other. let activeUnknownKey: string | null = null; let unknownEntrySeq = 0; + let inferredUnattributedPackage: string | null = null; const entryFor = (entryKey: string, packageName: string): MutableCandidate => { const existing = byEntry.get(entryKey); @@ -149,6 +165,37 @@ function scanPackageLockPatch(path: string, patch: string): LockfileTamperCandid return created; }; + const clearBlockTracking = () => { + activeEntry = null; + insideRejectedBlock = false; + activeUnknownKey = null; + inferredUnattributedPackage = null; + }; + + /** When a sequence-keyed unattributed bucket later learns a package name, re-key (and merge into any + * pre-existing package-keyed candidate) so cross-hunk / late-resolved correlation works (#8351). */ + const promoteSequenceKeyToPackage = (packageName: string) => { + if (!activeUnknownKey?.includes("#unattributed-")) return; + const seqKey = activeUnknownKey; + const pkgKey = `${path}#unattributed:${packageName}`; + activeUnknownKey = pkgKey; + const seqEntry = byEntry.get(seqKey); + // Freshly minted sequence keys promote before their first field is recorded — no map entry yet. + if (!seqEntry) return; + const prior = byEntry.get(pkgKey); + if (!prior) { + byEntry.delete(seqKey); + byEntry.set(pkgKey, seqEntry); + return; + } + if (seqEntry.removedVersion !== undefined) prior.removedVersion = seqEntry.removedVersion; + if (seqEntry.addedVersion !== undefined) prior.addedVersion = seqEntry.addedVersion; + prior.versionChanged = versionChanged(prior.removedVersion, prior.addedVersion); + prior.resolvedOrIntegrityChanged = prior.resolvedOrIntegrityChanged || seqEntry.resolvedOrIntegrityChanged; + prior.offRegistryResolvedUrl = prior.offRegistryResolvedUrl ?? seqEntry.offRegistryResolvedUrl; + byEntry.delete(seqKey); + }; + for (const line of patchLines(patch)) { const body = line.content.trim(); const objectHeader = /^"([^"]+)"\s*:\s*\{/.exec(body); @@ -161,6 +208,7 @@ function scanPackageLockPatch(path: string, patch: string): LockfileTamperCandid sawPackagesEntry = true; insideRejectedBlock = false; activeUnknownKey = null; + inferredUnattributedPackage = null; } else if (activeEntry) { innerObjectDepth++; } else if (!sawPackagesEntry && !CONTAINER_KEYS.has(key)) { @@ -168,11 +216,13 @@ function scanPackageLockPatch(path: string, patch: string): LockfileTamperCandid innerObjectDepth = 0; insideRejectedBlock = false; activeUnknownKey = null; + inferredUnattributedPackage = null; } else { activeEntry = null; innerObjectDepth = 0; insideRejectedBlock = true; activeUnknownKey = null; + inferredUnattributedPackage = null; } continue; } @@ -180,9 +230,7 @@ function scanPackageLockPatch(path: string, patch: string): LockfileTamperCandid if (innerObjectDepth > 0) { innerObjectDepth--; } else { - activeEntry = null; - insideRejectedBlock = false; - activeUnknownKey = null; + clearBlockTracking(); } } @@ -190,14 +238,33 @@ function scanPackageLockPatch(path: string, patch: string): LockfileTamperCandid const integrityMatch = /^"integrity"\s*:\s*"([^"]*)"/.exec(body); const versionMatch = /^"version"\s*:\s*"([^"]*)"/.exec(body); + // Learn package identity from ANY resolved URL in this block (context OR changed) before deciding + // the unattributed bucket key — a version-only hunk's leading context often still carries the + // registry URL even when the entry header itself is out of window (#8351 / #7778). + if (!activeEntry && !insideRejectedBlock && resolvedMatch?.[1]) { + const fromResolved = packageNameFromResolvedUrl(resolvedMatch[1]); + if (fromResolved) { + inferredUnattributedPackage = fromResolved; + // Context lines never reach the allocation branch below; still promote a live sequence key. + promoteSequenceKeyToPackage(fromResolved); + } + } + let currentEntryKey = activeEntry?.entryKey ?? null; let currentPackageName = activeEntry?.packageName ?? null; if (!currentEntryKey && !insideRejectedBlock && line.sign !== " " && (resolvedMatch || integrityMatch || versionMatch)) { + // Always mint a sequence key first, then promote to `#unattributed:` when a name is known + // (covers the empty-bucket promote path on first allocation, and late re-key once fields exist). if (!activeUnknownKey) { unknownEntrySeq += 1; activeUnknownKey = `${path}#unattributed-${unknownEntrySeq}`; } + if (inferredUnattributedPackage) { + promoteSequenceKeyToPackage(inferredUnattributedPackage); + } currentEntryKey = activeUnknownKey; + // Keep the anonymous display label even when the Map key is package-qualified — existing #7778 + // assertions (and the public finding text for header-less hunks) key off this exact string. currentPackageName = "(unattributed lockfile entry)"; } if (!currentEntryKey || !currentPackageName || line.sign === " ") continue; diff --git a/test/unit/lockfile-tamper.test.ts b/test/unit/lockfile-tamper.test.ts index c1d9131b97..0274de81d6 100644 --- a/test/unit/lockfile-tamper.test.ts +++ b/test/unit/lockfile-tamper.test.ts @@ -524,4 +524,175 @@ describe("lockfileTamperRiskFinding", () => { expect(finding).not.toBeNull(); expect(finding?.detail).toContain("lodash"); }); + + // #8351: git's ~3-line context often splits a single entry's version bump and its resolved/integrity + // change into SEPARATE hunks, each closing with a depth-0 `}`. Resetting the unattributed bucket on + // every such brace used to mint a fresh `#unattributed-N` per hunk, so the integrity-only bucket + // false-flagged even though the version hunk recorded a real bump for the same package. + it("REGRESSION (#8351): does NOT flag a legitimate version+integrity bump split across two header-less hunks for the same package", () => { + const lockPatch = [ + "@@ -50,6 +50,6 @@", + ' "resolved": "https://registry.npmjs.org/lodash/-/lodash-4.17.20.tgz",', + ' "license": "MIT",', + '- "version": "4.17.20",', + '+ "version": "4.17.21",', + " }", + "@@ -60,7 +60,7 @@", + ' "version": "4.17.21",', + ' "license": "MIT",', + '- "resolved": "https://registry.npmjs.org/lodash/-/lodash-4.17.20.tgz",', + '- "integrity": "sha512-oldoldold=="', + '+ "resolved": "https://registry.npmjs.org/lodash/-/lodash-4.17.21.tgz",', + '+ "integrity": "sha512-newnewnew=="', + " }", + ].join("\n"); + // No `"node_modules/lodash": {` header in either hunk — package identity comes from the registry + // resolved URL in context / changed lines, so both hunks share one unattributed bucket. + expect(lockfileTamperRiskFinding([lockfilePatch(lockPatch)])).toBeNull(); + }); + + it("REGRESSION (#8351): still flags a genuine cross-hunk tamper (integrity changed, version NOT changed) for the same header-less package", () => { + const lockPatch = [ + "@@ -50,6 +50,6 @@", + ' "resolved": "https://registry.npmjs.org/lodash/-/lodash-4.17.20.tgz",', + ' "license": "MIT",', + '- "integrity": "sha512-oldoldold=="', + '+ "integrity": "sha512-tamperedtampered=="', + " }", + "@@ -70,6 +70,6 @@", + ' "version": "4.17.20",', + ' "license": "MIT",', + '- "resolved": "https://registry.npmjs.org/lodash/-/lodash-4.17.20.tgz",', + '+ "resolved": "https://registry.npmjs.org/lodash/-/lodash-4.17.20-mirror.tgz",', + " }", + ].join("\n"); + // Integrity in hunk 1 + resolved swap in hunk 2, neither hunk bumps version — correlation must + // still produce a single candidate with resolvedOrIntegrityChanged && !versionChanged. + const finding = lockfileTamperRiskFinding([lockfilePatch(lockPatch)]); + expect(finding).not.toBeNull(); + expect(finding?.code).toBe("lockfile_tamper_risk"); + expect(finding?.title).toContain("unattributed lockfile entry"); + }); + + it("REGRESSION (#8351): two different header-less packages in one patch never share an unattributed bucket", () => { + const lockPatch = [ + "@@ -50,6 +50,6 @@", + ' "resolved": "https://registry.npmjs.org/lodash/-/lodash-4.17.20.tgz",', + ' "license": "MIT",', + '- "integrity": "sha512-old-lodash=="', + '+ "integrity": "sha512-tampered-lodash=="', + " }", + "@@ -80,6 +80,6 @@", + ' "resolved": "https://registry.npmjs.org/express/-/express-4.18.0.tgz",', + ' "license": "MIT",', + '- "integrity": "sha512-old-express=="', + '+ "integrity": "sha512-tampered-express=="', + " }", + ].join("\n"); + const finding = lockfileTamperRiskFinding([lockfilePatch(lockPatch)]); + expect(finding).not.toBeNull(); + // Both packages are independently tampered; collapsing them into one bucket would still flag, but + // the title/detail must keep the anonymous unattributed label (not a merged bare name) and the + // finding must fire — pinning that distinct registry URLs mint distinct correlation keys. + expect(finding?.title).toContain("unattributed lockfile entry"); + expect(finding?.detail).toContain("unattributed lockfile entry"); + }); + + it("REGRESSION (#8351): upgrades a sequence-keyed unattributed bucket when a resolved URL appears later in the same header-less block", () => { + // No leading context resolved URL — version lines mint `#unattributed-N` first; the following + // resolved URL recovers the package name and must re-key so a PRIOR hunk's package-keyed integrity + // signal (and this block's version bump) still correlate into one candidate. + const lockPatch = [ + "@@ -40,6 +40,6 @@", + ' "resolved": "https://registry.npmjs.org/foo/-/foo-1.0.0.tgz",', + ' "license": "MIT",', + '- "integrity": "sha512-old=="', + '+ "integrity": "sha512-new=="', + " }", + "@@ -55,8 +55,8 @@", + '- "version": "1.0.0",', + '+ "version": "1.0.1",', + '- "resolved": "https://registry.npmjs.org/foo/-/foo-1.0.0.tgz",', + '+ "resolved": "https://registry.npmjs.org/foo/-/foo-1.0.1.tgz",', + '- "integrity": "sha512-new=="', + '+ "integrity": "sha512-newer=="', + " }", + ].join("\n"); + expect(lockfileTamperRiskFinding([lockfilePatch(lockPatch)])).toBeNull(); + }); + + it("REGRESSION (#8351): in-block sequence→package re-key works even with no prior package-keyed candidate", () => { + const lockPatch = [ + "@@ -55,8 +55,8 @@", + '- "version": "1.0.0",', + '+ "version": "1.0.1",', + '- "resolved": "https://registry.npmjs.org/foo/-/foo-1.0.0.tgz",', + '+ "resolved": "https://registry.npmjs.org/foo/-/foo-1.0.1.tgz",', + '- "integrity": "sha512-old=="', + '+ "integrity": "sha512-new=="', + " }", + ].join("\n"); + expect(lockfileTamperRiskFinding([lockfilePatch(lockPatch)])).toBeNull(); + }); + + it("REGRESSION (#8351): sequence→package upgrade merges an off-registry resolved URL onto the prior package-keyed candidate", () => { + const lockPatch = [ + "@@ -10,5 +10,5 @@", + ' "resolved": "https://registry.npmjs.org/foo/-/foo-1.0.0.tgz",', + ' "license": "MIT",', + '- "version": "1.0.0",', + '+ "version": "1.0.1",', + " }", + "@@ -20,6 +20,6 @@", + '- "resolved": "https://evil.example.com/foo-old.tgz",', + '+ "resolved": "https://evil.example.com/foo-new.tgz",', + // A second resolved line (invalid JSON, but the line scanner is not a JSON parser) supplies the + // registry URL that recovers the package name and triggers the sequence→package upgrade merge. + ' "resolved": "https://registry.npmjs.org/foo/-/foo-1.0.0.tgz",', + " }", + ].join("\n"); + const finding = lockfileTamperRiskFinding([lockfilePatch(lockPatch)]); + expect(finding).not.toBeNull(); + expect(finding?.detail).toContain("outside registry.npmjs.org"); + }); + + it("REGRESSION (#8351): merge keeps a prior off-registry URL when the sequence bucket has none", () => { + // Hunk 1 records an off-registry resolved under the package key. Hunk 2 starts with a version-only + // sequence bucket that later promotes and must NOT clear the prior off-registry URL (?? left arm). + const lockPatch = [ + "@@ -10,6 +10,6 @@", + ' "resolved": "https://registry.npmjs.org/foo/-/foo-1.0.0.tgz",', + ' "license": "MIT",', + '- "resolved": "https://evil.example.com/foo-old.tgz",', + '+ "resolved": "https://evil.example.com/foo-new.tgz",', + " }", + "@@ -30,6 +30,6 @@", + '- "version": "1.0.0",', + '+ "version": "1.0.1",', + '- "resolved": "https://registry.npmjs.org/foo/-/foo-1.0.0.tgz",', + '+ "resolved": "https://registry.npmjs.org/foo/-/foo-1.0.1.tgz",', + " }", + ].join("\n"); + const finding = lockfileTamperRiskFinding([lockfilePatch(lockPatch)]); + expect(finding).not.toBeNull(); + expect(finding?.detail).toContain("outside registry.npmjs.org"); + }); + + it("REGRESSION (#8351): correlates a scoped package across hunks via its registry resolved URL", () => { + const lockPatch = [ + "@@ -50,6 +50,6 @@", + ' "resolved": "https://registry.npmjs.org/@babel/core/-/core-7.0.0.tgz",', + ' "license": "MIT",', + '- "version": "7.0.0",', + '+ "version": "7.0.1",', + " }", + "@@ -60,6 +60,6 @@", + ' "resolved": "https://registry.npmjs.org/@babel/core/-/core-7.0.0.tgz",', + ' "license": "MIT",', + '- "integrity": "sha512-old=="', + '+ "integrity": "sha512-new=="', + " }", + ].join("\n"); + expect(lockfileTamperRiskFinding([lockfilePatch(lockPatch)])).toBeNull(); + }); });