diff --git a/src/review/lockfile-tamper.ts b/src/review/lockfile-tamper.ts index a049cffbbb..e178198ff3 100644 --- a/src/review/lockfile-tamper.ts +++ b/src/review/lockfile-tamper.ts @@ -1,16 +1,32 @@ // Lockfile-tamper-risk gate check (#2563). Deterministic scan of a changed `package-lock.json` (or another -// `*.lock` file) diff for the classic supply-chain tell: a `resolved`/`integrity` value changed WITHOUT the -// corresponding `package.json` dependency version changing, or a `resolved` URL that points outside the public -// npm registry. Distinct from the OSV.dev CVE analyzer (review-enrichment/src/analyzers/lockfile-drift.ts) — -// that flags KNOWN-CVE versions; this flags tamper/integrity-substitution regardless of whether the substituted +// `*.lock` file) diff for the classic supply-chain tell: a `resolved`/`integrity` value changed WITHOUT that +// SAME package-lock entry's own `"version"` field genuinely changing, or a `resolved` URL that points outside +// the public npm registry. Distinct from the OSV.dev CVE analyzer (review-enrichment/src/analyzers/lockfile-drift.ts) +// — that flags KNOWN-CVE versions; this flags tamper/integrity-substitution regardless of whether the substituted // version has a published CVE. Config-driven, off by default (see rules/advisory.ts isConfiguredGateBlocker + // signals/focus-manifest.ts gate.lockfileIntegrity) — this module only PRODUCES the finding; it never decides // whether the finding blocks. +// +// Why compare against the lockfile entry's OWN version rather than package.json (see #2563 gate-review +// follow-up on #2676): every package-lock.json entry — direct AND transitive — carries its own version/ +// resolved/integrity trio, and a genuine `npm install`/`npm update` always bumps all three together for any +// entry it touches. package.json, by contrast, only lists DIRECT dependencies, so the vast majority of lockfile +// entries (transitive dependencies) never appear there at all; treating "package.json didn't change" as a +// tamper signal made every ordinary transitive bump misfire. A hand-edited resolved/integrity pointing at +// malicious content while its OWN declared version is left unchanged (to look unremarkable) is a more specific, +// self-contained tell that doesn't require cross-referencing a different file. import type { AdvisoryFinding, PullRequestFileRecord } from "../types"; const NPM_REGISTRY_HOST_RE = /^https:\/\/registry\.npmjs\.org\//i; +// Only a `resolved` value that IS an http(s) URL can be judged against the registry-host allowlist. An npm +// workspace's own local packages (e.g. this repo's `packages/gittensory-mcp`, `apps/gittensory-ui` — see +// `"link": true` entries in package-lock.json) have a `resolved` field that's a RELATIVE FILESYSTEM PATH, not a +// URL at all (e.g. `"packages/gittensory-mcp"`). Such a value was never resolved FROM a registry, so it can't be +// "off-registry" — it must be exempted rather than flagged just because it fails the npmjs.org prefix check. +const HTTP_URL_RE = /^https?:\/\//i; + // Package-lock "packages" entries are keyed either `"node_modules/"` (lockfileVersion 2/3) or a bare // `""` (lockfileVersion 1 "dependencies" tree, and yarn/pnpm equivalents keep a similar bare-name header). // Root ("": {...}) and pure container headers ("packages": {...}, "dependencies": {...}) are never package @@ -57,17 +73,64 @@ type LockfileTamperCandidate = { package: string; /** True when a `resolved`/`integrity` value changed for this package block in the diff. */ resolvedOrIntegrityChanged: boolean; - /** A `+resolved` URL seen for this package block that does not point at registry.npmjs.org, or null. */ + /** True when THIS SAME package block's own `"version"` line was added and/or removed with a different value + * somewhere in the diff (see versionChanged() below for the exact rule). */ + versionChanged: boolean; + /** A `+resolved` URL seen for this package block that does not point at registry.npmjs.org, or null. Only + * ever set for values that ARE http(s) URLs — see HTTP_URL_RE. */ offRegistryResolvedUrl: string | null; }; -/** Parse one `package-lock.json` unified-diff patch for per-package resolved/integrity changes. Heuristic - * line-based scan (mirrors review-enrichment's lockfile-drift parser), not a full JSON parse — good enough to - * flag suspicious hunks without needing the complete (potentially huge) lockfile tree in memory. */ +type MutableCandidate = LockfileTamperCandidate & { + removedVersion: string | undefined; + addedVersion: string | undefined; +}; + +/** True when the `"version"` value for a package block genuinely changed within the diff: an added-only or + * removed-only version line, or an added+removed pair with different values. Mirrors the same add/remove + * reconciliation package.json dependency-range diffing used (before this fix, that was the ONLY signal this + * module had) — applied here to the lockfile entry's own version field instead. */ +function versionChanged(removed: string | undefined, added: string | undefined): boolean { + return removed !== added; +} + +/** Parse one `package-lock.json` unified-diff patch for per-package resolved/integrity/version changes. + * Heuristic line-based scan (mirrors review-enrichment's lockfile-drift parser), not a full JSON parse — good + * enough to flag suspicious hunks without needing the complete (potentially huge) lockfile tree in memory. + * + * Keyed by the FULL lockfile-entry path (e.g. `node_modules/bar/node_modules/foo`), not the bare package name + * (see #2563 gate-review follow-up on #2692): a package can appear as MULTIPLE distinct lockfileVersion 2/3 + * entries under different nesting paths when different dependents require incompatible versions of it. Keying + * by bare name merged those distinct entries into one shared record, so a genuine version bump on one entry + * could mask an unbumped resolved/integrity edit on a DIFFERENT entry of the same package -- the full + * `node_modules/...` path IS unique per entry (npm packages a duplicate copy under a distinct nested path + * precisely because two entries with the same bare name coexist), so using it as the map key keeps every + * entry's own signal independent. The bare package name is still recorded separately for display. The legacy + * lockfileVersion 1 "dependencies" tree (bare, non-path keys — see npmPackageFromNodeModulesPath's fallback) + * has no such embedded full path and keeps its pre-existing bare-name keying; that format predates any + * actively maintained repo's lockfile and is out of scope here. */ function scanPackageLockPatch(path: string, patch: string): LockfileTamperCandidate[] { - const byPackage = new Map(); - let currentPackage: string | null = null; + const byEntry = new Map(); + let currentEntryKey: string | null = null; + let currentPackageName: string | null = null; let sawPackagesEntry = false; + + const entryFor = (entryKey: string, packageName: string): MutableCandidate => { + const existing = byEntry.get(entryKey); + if (existing) return existing; + const created: MutableCandidate = { + file: path, + package: packageName, + resolvedOrIntegrityChanged: false, + versionChanged: false, + offRegistryResolvedUrl: null, + removedVersion: undefined, + addedVersion: undefined, + }; + byEntry.set(entryKey, created); + return created; + }; + for (const line of patchLines(patch)) { const body = line.content.trim(); const objectHeader = /^"([^"]+)"\s*:\s*\{/.exec(body); @@ -75,79 +138,61 @@ function scanPackageLockPatch(path: string, patch: string): LockfileTamperCandid const key = objectHeader[1]!; const nodeModulesPackage = npmPackageFromNodeModulesPath(key); if (nodeModulesPackage) { - currentPackage = nodeModulesPackage; + currentEntryKey = key; + currentPackageName = nodeModulesPackage; sawPackagesEntry = true; } else if (!sawPackagesEntry && !CONTAINER_KEYS.has(key)) { - currentPackage = key; + currentEntryKey = key; + currentPackageName = key; } else { - currentPackage = null; + currentEntryKey = null; + currentPackageName = null; } continue; } - if (body === "}" || body.startsWith("},")) currentPackage = null; - if (!currentPackage || line.sign === " ") continue; + if (body === "}" || body.startsWith("},")) { + currentEntryKey = null; + currentPackageName = null; + } + if (!currentEntryKey || !currentPackageName || line.sign === " ") continue; const resolvedMatch = /^"resolved"\s*:\s*"([^"]*)"/.exec(body); const integrityMatch = /^"integrity"\s*:\s*"([^"]*)"/.exec(body); + const versionMatch = /^"version"\s*:\s*"([^"]*)"/.exec(body); + + if (versionMatch) { + const entry = entryFor(currentEntryKey, currentPackageName); + // `line.sign` is guaranteed "+" or "-" here (never " ") by the `line.sign === " "` continue above -- a + // context ("unchanged") "version" line never reaches this branch, so it can never masquerade as removed. + if (line.sign === "+") entry.addedVersion = versionMatch[1]; + else entry.removedVersion = versionMatch[1]; + entry.versionChanged = versionChanged(entry.removedVersion, entry.addedVersion); + continue; + } + if (!resolvedMatch && !integrityMatch) continue; - const entry = - byPackage.get(currentPackage) ?? - ({ file: path, package: currentPackage, resolvedOrIntegrityChanged: false, offRegistryResolvedUrl: null } satisfies LockfileTamperCandidate); + const entry = entryFor(currentEntryKey, currentPackageName); entry.resolvedOrIntegrityChanged = true; - if (resolvedMatch && line.sign === "+" && resolvedMatch[1] && !NPM_REGISTRY_HOST_RE.test(resolvedMatch[1])) { + if (resolvedMatch && line.sign === "+" && resolvedMatch[1] && HTTP_URL_RE.test(resolvedMatch[1]) && !NPM_REGISTRY_HOST_RE.test(resolvedMatch[1])) { entry.offRegistryResolvedUrl = resolvedMatch[1]; } - byPackage.set(currentPackage, entry); - } - return [...byPackage.values()]; -} - -// `"": ""` inside a package.json dependency block, e.g. `"lodash": "^4.17.21",`. Line-based, not a -// full JSON parse — the same heuristic review-enrichment's dependency-scan.ts uses for the same shape. -const PACKAGE_JSON_DEP_RE = /^"([^"]+)"\s*:\s*"([^"]+)"/; - -/** Package names whose declared `package.json` version range CHANGED somewhere in this PR's diff (across every - * changed `package.json`, any dependency block) — a `+`/`-` pair with different range strings for the same key - * counts as changed; a line present on only one side (add/remove of the dependency entirely) also counts. */ -function packagesWithManifestVersionChange(files: PullRequestFileRecord[]): Set { - const changed = new Set(); - for (const file of files) { - if (file.path.replace(/\\/g, "/").toLowerCase().split("/").pop() !== "package.json") continue; - const patch = typeof file.payload?.patch === "string" ? file.payload.patch : ""; - if (!patch) continue; - const removedVersions = new Map(); - const addedVersions = new Map(); - for (const line of patchLines(patch)) { - if (line.sign === " ") continue; - const match = PACKAGE_JSON_DEP_RE.exec(line.content.trim()); - if (!match) continue; - const [, name, range] = match as unknown as [string, string, string]; - (line.sign === "+" ? addedVersions : removedVersions).set(name, range); - } - for (const [name, addedRange] of addedVersions) { - const removedRange = removedVersions.get(name); - if (removedRange === undefined || removedRange !== addedRange) changed.add(name); - } - for (const name of removedVersions.keys()) { - if (!addedVersions.has(name)) changed.add(name); - } } - return changed; + return [...byEntry.values()]; } const MAX_FLAGGED_PACKAGES_IN_TITLE = 3; /** * Scan every changed `package-lock.json` in the PR for a tamper-risk hunk: a `resolved`/`integrity` value - * changed WITHOUT the same package's version changing in a changed `package.json`, or a `resolved` URL outside - * `registry.npmjs.org`. Returns ONE `lockfile_tamper_risk` advisory finding on any hit, else null. Callers gate - * this on the repo's `lockfileIntegrityGateMode` (default `off` — see rules/advisory.ts) before invoking it. + * changed for a lockfile entry WITHOUT that same entry's own `"version"` field genuinely changing in the diff, + * or a `resolved` URL outside `registry.npmjs.org`. Returns ONE `lockfile_tamper_risk` advisory finding on any + * hit, else null. Callers gate this on the repo's `lockfileIntegrityGateMode` (default `off` — see + * rules/advisory.ts) before invoking it. */ export function lockfileTamperRiskFinding(files: PullRequestFileRecord[]): AdvisoryFinding | null { const lockfiles = files.filter((file) => isNpmLockfilePath(file.path)); if (lockfiles.length === 0) return null; - const bumpedPackages = packagesWithManifestVersionChange(files); const flagged: { file: string; package: string; reason: "off_registry" | "unbumped_resolved" }[] = []; for (const file of lockfiles) { @@ -156,7 +201,7 @@ export function lockfileTamperRiskFinding(files: PullRequestFileRecord[]): Advis for (const candidate of scanPackageLockPatch(file.path, patch)) { if (candidate.offRegistryResolvedUrl) { flagged.push({ file: candidate.file, package: candidate.package, reason: "off_registry" }); - } else if (candidate.resolvedOrIntegrityChanged && !bumpedPackages.has(candidate.package)) { + } else if (candidate.resolvedOrIntegrityChanged && !candidate.versionChanged) { flagged.push({ file: candidate.file, package: candidate.package, reason: "unbumped_resolved" }); } } diff --git a/test/unit/lockfile-tamper.test.ts b/test/unit/lockfile-tamper.test.ts index b5b9a49892..c1d6d765e8 100644 --- a/test/unit/lockfile-tamper.test.ts +++ b/test/unit/lockfile-tamper.test.ts @@ -218,13 +218,17 @@ describe("lockfileTamperRiskFinding", () => { expect(finding?.detail).toContain("lodash"); }); - it("counts a fully removed manifest dependency as a version change (no matching add)", () => { + it("still flags a changed integrity with no version bump even when the package is ALSO being dropped from package.json (#2563 gate-review follow-up)", () => { + // The tamper signal is now self-contained to the lockfile entry's OWN version field (see the module header + // comment on why this replaced the package.json cross-reference) — a package.json removal is irrelevant to + // it. If lodash is genuinely being dropped, its lockfile entry should be REMOVED too, not silently have its + // integrity swapped while the entry stays present with an unchanged version; that is exactly the suspicious + // shape this check exists to catch. const lockPatch = ['@@ -1,4 +1,4 @@', ' "node_modules/lodash": {', '- "integrity": "sha512-oldoldold=="', '+ "integrity": "sha512-tamperedtampered=="', ' },'].join("\n"); - // lodash is removed from package.json entirely (no corresponding "+" line) — still counts as a version - // change for tamper-risk purposes (the dependency's presence itself changed), so lodash is NOT flagged. const manifestDiff = ['@@ -10,4 +10,3 @@', ' "dependencies": {', '- "lodash": "^4.17.20",', ' "express": "^4.18.0"'].join("\n"); const finding = lockfileTamperRiskFinding([lockfilePatch(lockPatch), manifestPatch(manifestDiff)]); - expect(finding).toBeNull(); + expect(finding).not.toBeNull(); + expect(finding?.detail).toContain("lodash"); }); it("collapses multiple flagged packages into one finding, capping the title list and reporting the overflow count", () => { @@ -238,4 +242,112 @@ describe("lockfileTamperRiskFinding", () => { expect(finding?.detail).toContain("alpha"); expect(finding?.detail).toContain("echo"); }); + + // #2563 gate-review follow-up: the original package.json-cross-reference signal could never see a + // TRANSITIVE dependency (never listed in any package.json), so it misfired on every ordinary transitive + // bump -- the vast majority of any real lockfile diff. The fix compares against the SAME entry's own + // "version" line instead, which a genuine npm install/update always bumps alongside resolved/integrity. + it("does NOT flag a transitive dependency bump (own version line changes, no package.json anywhere in the diff)", () => { + const lockPatch = [ + '@@ -40,6 +40,6 @@', + ' "node_modules/send": {', + '- "version": "0.18.0",', + '- "resolved": "https://registry.npmjs.org/send/-/send-0.18.0.tgz",', + '- "integrity": "sha512-oldoldold=="', + '+ "version": "0.19.0",', + '+ "resolved": "https://registry.npmjs.org/send/-/send-0.19.0.tgz",', + '+ "integrity": "sha512-newnewnew=="', + " },", + ].join("\n"); + // No package.json in this diff at all -- "send" is a transitive dependency of a direct dependency, never + // listed in any manifest, exactly the majority-case shape a real `npm update` produces. + const finding = lockfileTamperRiskFinding([lockfilePatch(lockPatch)]); + expect(finding).toBeNull(); + }); + + // #2563 gate-review follow-up: an npm workspace's own local packages have a `resolved` field that is a + // relative filesystem path, not a URL -- the old exact-registry-prefix check misclassified any such value + // as off-registry the moment it changed (i.e. on every routine workspace-member version bump). + it("does NOT flag a workspace-local package's relative-path resolved value as off-registry", () => { + const lockPatch = [ + "@@ -2,7 +2,7 @@", + ' "packages/gittensory-mcp": {', + '- "version": "0.6.0",', + '- "resolved": "packages/gittensory-mcp",', + " \"link\": true", + '+ "version": "0.7.0",', + '+ "resolved": "packages/gittensory-mcp",', + " \"link\": true", + " },", + ].join("\n"); + const finding = lockfileTamperRiskFinding([lockfilePatch(lockPatch)]); + expect(finding).toBeNull(); + }); + + // REGRESSION (#2563 gate-review follow-up on #2692): two DISTINCT lockfileVersion 2/3 entries can share the + // same bare package name (npm nests a second copy under a dependent's own node_modules when versions + // conflict) -- keying candidates by bare name merged them into one shared record, so a legitimate bump on + // ONE entry masked an unbumped, tampered resolved/integrity edit on the OTHER. Keying by the full entry path + // fixes this; both orderings are tested since the old bug's masking depended on which block was processed last. + it("does NOT let a legitimate bump on one nested copy of a package mask a tampered edit on ANOTHER nested copy of the SAME package name", () => { + const lockPatch = [ + '@@ -1,16 +1,16 @@', + ' "node_modules/foo": {', + '- "version": "1.0.0",', + '- "resolved": "https://registry.npmjs.org/foo/-/foo-1.0.0.tgz",', + '- "integrity": "sha512-old1=="', + '+ "version": "1.1.0",', + '+ "resolved": "https://registry.npmjs.org/foo/-/foo-1.1.0.tgz",', + '+ "integrity": "sha512-new1=="', + ' },', + ' "node_modules/bar/node_modules/foo": {', + ' "version": "2.0.0",', + '- "resolved": "https://registry.npmjs.org/foo/-/foo-2.0.0.tgz",', + '- "integrity": "sha512-old2=="', + '+ "resolved": "https://registry.npmjs.org/foo/-/foo-2.0.0.tgz",', + '+ "integrity": "sha512-tampered2=="', + ' },', + ].join("\n"); + const finding = lockfileTamperRiskFinding([lockfilePatch(lockPatch)]); + expect(finding).not.toBeNull(); + }); + + it("still catches the masking scenario in the REVERSE order (tampered entry processed before the legitimately-bumped one)", () => { + const lockPatch = [ + '@@ -1,16 +1,16 @@', + ' "node_modules/bar/node_modules/foo": {', + ' "version": "2.0.0",', + '- "resolved": "https://registry.npmjs.org/foo/-/foo-2.0.0.tgz",', + '- "integrity": "sha512-old2=="', + '+ "resolved": "https://registry.npmjs.org/foo/-/foo-2.0.0.tgz",', + '+ "integrity": "sha512-tampered2=="', + ' },', + ' "node_modules/foo": {', + '- "version": "1.0.0",', + '- "resolved": "https://registry.npmjs.org/foo/-/foo-1.0.0.tgz",', + '- "integrity": "sha512-old1=="', + '+ "version": "1.1.0",', + '+ "resolved": "https://registry.npmjs.org/foo/-/foo-1.1.0.tgz",', + '+ "integrity": "sha512-new1=="', + ' },', + ].join("\n"); + const finding = lockfileTamperRiskFinding([lockfilePatch(lockPatch)]); + expect(finding).not.toBeNull(); + }); + + it("still flags a genuine off-registry resolved URL (an http(s) URL outside registry.npmjs.org)", () => { + const lockPatch = [ + '@@ -10,4 +10,4 @@', + ' "node_modules/evil-pkg": {', + '- "version": "1.0.0",', + '- "resolved": "https://registry.npmjs.org/evil-pkg/-/evil-pkg-1.0.0.tgz",', + '+ "version": "1.0.0",', + '+ "resolved": "https://attacker.example.com/evil-pkg-1.0.0.tgz",', + " },", + ].join("\n"); + const finding = lockfileTamperRiskFinding([lockfilePatch(lockPatch)]); + expect(finding).not.toBeNull(); + expect(finding?.detail).toContain("evil-pkg"); + expect(finding?.detail).toContain("outside registry.npmjs.org"); + }); });