Skip to content

fix(review): track lockfile entry nesting so a nested dependencies object can't hide tamper (#5837) #6050

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
shin-core wants to merge 1 commit into from
Closed

fix(review): track lockfile entry nesting so a nested dependencies object can't hide tamper (#5837) #6050

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 20 additions & 11 deletions src/review/lockfile-tamper.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<string, MutableCandidate>();
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 => {
Expand All @@ -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);
Expand Down
65 changes: 65 additions & 0 deletions test/unit/lockfile-tamper.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 @@',
Expand Down