From d7d91719b45d15845e93935112804341ff2e6a83 Mon Sep 17 00:00:00 2001 From: shin-core <153108882+shin-core@users.noreply.github.com> Date: Wed, 15 Jul 2026 17:11:15 +1000 Subject: [PATCH] fix(review): track lockfile entry nesting so a nested dependencies object can't hide tamper (#5837) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit scanPackageLockPatch tracked "which package-lock entry am I inside" with a single flat currentEntryKey/currentPackageName pointer. A real `node_modules/...` entry often contains its own nested `dependencies`/`devDependencies`/ `optionalDependencies` sub-object; hitting that nested container header reset the pointer to null and never restored it, so any `resolved`/`integrity`/`version` line for the SAME outer entry appearing after the nested object was silently skipped by the current-entry guard. Real npm output orders version/resolved/ integrity before dependencies, so legitimate diffs never tripped it — but an attacker could reorder an entry's keys to place `dependencies` first and evade the tamper detector entirely for that entry. Replace the flat pointer with a stack of brace scopes: each object-header line pushes a frame (an entry, or null for a container/root/other), each closing brace pops one, so closing a nested sub-object restores the enclosing entry as the current one. Entries are still keyed by their full `node_modules/...` path (#2692) and the legacy lockfileVersion-1 bare-name branch is unchanged; the public API and gate wiring are untouched — this is a parsing correction only. Adds a regression test where the entry's `dependencies` object precedes its resolved/integrity/version (reordered keys), and a second where two same-named entries each carry a nested dependencies object, confirming the #2692 keying and this fix compose. --- src/review/lockfile-tamper.ts | 31 +++++++++------ test/unit/lockfile-tamper.test.ts | 65 +++++++++++++++++++++++++++++++ 2 files changed, 85 insertions(+), 11 deletions(-) diff --git a/src/review/lockfile-tamper.ts b/src/review/lockfile-tamper.ts index cffa90f22b..1e5efc3727 100644 --- a/src/review/lockfile-tamper.ts +++ b/src/review/lockfile-tamper.ts @@ -113,8 +113,14 @@ function versionChanged(removed: string | undefined, added: string | undefined): * actively maintained repo's lockfile and is out of scope here. */ function scanPackageLockPatch(path: string, patch: string): LockfileTamperCandidate[] { const byEntry = new Map(); - let currentEntryKey: string | null = null; - let currentPackageName: string | null = null; + // A single flat currentEntryKey pointer loses the outer entry when a `node_modules/...` entry contains its own + // nested `dependencies`/`devDependencies`/`optionalDependencies` sub-object: the nested container header reset + // the pointer to null and never restored it, so any resolved/integrity/version line for the SAME outer entry + // appearing after the nested object was silently skipped — an attacker could reorder an entry's keys to put + // `dependencies` before `resolved`/`integrity` and evade the tamper check entirely (#5837). Track a stack of + // brace scopes instead: each object-header line pushes a frame (an entry, or null for a container/root/other), + // each closing brace pops one, so closing a nested sub-object restores the enclosing entry as the current one. + const scopeStack: Array<{ entryKey: string; packageName: string } | null> = []; let sawPackagesEntry = false; const entryFor = (entryKey: string, packageName: string): MutableCandidate => { @@ -140,23 +146,26 @@ function scanPackageLockPatch(path: string, patch: string): LockfileTamperCandid const key = objectHeader[1]!; const nodeModulesPackage = npmPackageFromNodeModulesPath(key); if (nodeModulesPackage) { - currentEntryKey = key; - currentPackageName = nodeModulesPackage; + scopeStack.push({ entryKey: key, packageName: nodeModulesPackage }); sawPackagesEntry = true; } else if (!sawPackagesEntry && !CONTAINER_KEYS.has(key)) { - currentEntryKey = key; - currentPackageName = key; + scopeStack.push({ entryKey: key, packageName: key }); } else { - currentEntryKey = null; - currentPackageName = null; + // A container/root/unrecognized header (incl. a nested dependencies sub-object) — not an entry itself, + // but still a brace scope, so push a null frame that a matching closing brace will pop back off. + scopeStack.push(null); } continue; } if (body === "}" || body.startsWith("},")) { - currentEntryKey = null; - currentPackageName = null; + scopeStack.pop(); } - if (!currentEntryKey || !currentPackageName || line.sign === " ") continue; + // The current entry is the innermost open scope IF that scope is an entry (we're directly in its body). A + // resolved/integrity/version line nested inside a sub-object (top frame null) is correctly not attributed. + const current = scopeStack.length > 0 ? scopeStack[scopeStack.length - 1] : null; + if (!current || line.sign === " ") continue; + const currentEntryKey = current.entryKey; + const currentPackageName = current.packageName; const resolvedMatch = /^"resolved"\s*:\s*"([^"]*)"/.exec(body); const integrityMatch = /^"integrity"\s*:\s*"([^"]*)"/.exec(body); diff --git a/test/unit/lockfile-tamper.test.ts b/test/unit/lockfile-tamper.test.ts index 7fc3dbc441..052c3520d5 100644 --- a/test/unit/lockfile-tamper.test.ts +++ b/test/unit/lockfile-tamper.test.ts @@ -95,6 +95,71 @@ describe("lockfileTamperRiskFinding", () => { expect(finding?.detail).toContain("lodash"); }); + // #5837: a nested dependencies sub-object inside an entry, appearing BEFORE the entry's own + // resolved/integrity/version lines (key order reversed from real npm output), must not make the parser lose + // track of the outer entry — otherwise a reordered-key tamper diff evades detection entirely. + it("still detects tamper when the entry's dependencies sub-object precedes its resolved/integrity/version (regression for #5837)", () => { + const lockPatch = [ + '@@ -100,12 +100,12 @@', + ' "node_modules/lodash": {', + ' "dependencies": {', + '- "helper": "^1.0.0"', + '+ "helper": "^1.0.1"', + ' },', + '- "version": "4.17.20",', + '- "resolved": "https://registry.npmjs.org/lodash/-/lodash-4.17.20.tgz",', + '- "integrity": "sha512-oldoldold=="', + '+ "version": "4.17.20",', + '+ "resolved": "https://registry.npmjs.org/lodash/-/lodash-4.17.20.tgz",', + '+ "integrity": "sha512-tamperedtampered=="', + ' },', + ].join("\n"); + // resolved/integrity changed but version did NOT — the classic tamper tell, here after a nested sub-object. + const finding = lockfileTamperRiskFinding([lockfilePatch(lockPatch)]); + expect(finding).not.toBeNull(); + expect(finding?.code).toBe("lockfile_tamper_risk"); + expect(finding?.detail).toContain("lodash"); + }); + + // #5837 + #2692: two distinct entries share the bare name "foo" (a nested duplicate under bar), each carrying + // its own dependencies sub-object. The full-path entry keying (#2692) and the nesting-depth fix must compose: + // the genuine bump on the top-level foo must NOT mask the unbumped resolved/integrity edit on the nested foo. + it("keeps two same-named entries independent when each also has a nested dependencies object (#5837 + #2692)", () => { + const lockPatch = [ + '@@ -200,20 +200,20 @@', + ' "node_modules/foo": {', + ' "dependencies": {', + '- "dep": "^1.0.0"', + '+ "dep": "^1.1.0"', + ' },', + '- "version": "1.0.0",', + '- "resolved": "https://registry.npmjs.org/foo/-/foo-1.0.0.tgz",', + '- "integrity": "sha512-fooold=="', + '+ "version": "1.1.0",', + '+ "resolved": "https://registry.npmjs.org/foo/-/foo-1.1.0.tgz",', + '+ "integrity": "sha512-foonew=="', + ' },', + ' "node_modules/bar/node_modules/foo": {', + ' "dependencies": {', + '- "dep": "^2.0.0"', + '+ "dep": "^2.0.1"', + ' },', + '- "version": "2.0.0",', + '- "resolved": "https://registry.npmjs.org/foo/-/foo-2.0.0.tgz",', + '- "integrity": "sha512-nestedold=="', + '+ "version": "2.0.0",', + '+ "resolved": "https://registry.npmjs.org/foo/-/foo-2.0.0.tgz",', + '+ "integrity": "sha512-nestedtampered=="', + ' },', + ].join("\n"); + // Top-level foo bumped legitimately (all three change); the nested foo's integrity changed with NO version + // bump — still flagged, because the two entries are keyed by their distinct full paths, not the shared name. + const finding = lockfileTamperRiskFinding([lockfilePatch(lockPatch)]); + expect(finding).not.toBeNull(); + expect(finding?.code).toBe("lockfile_tamper_risk"); + expect(finding?.detail).toContain("foo"); + }); + it("triggers on a resolved URL outside the npm registry, even with a version bump", () => { const lockPatch = [ '@@ -100,8 +100,8 @@',