diff --git a/src/review/lockfile-tamper.ts b/src/review/lockfile-tamper.ts index db68a0dac..ee33c6280 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 c1d9131b9..0274de81d 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(); + }); });