diff --git a/packages/loopover-engine/src/review/safe-url.ts b/packages/loopover-engine/src/review/safe-url.ts index 93b7622165..4d400917bd 100644 --- a/packages/loopover-engine/src/review/safe-url.ts +++ b/packages/loopover-engine/src/review/safe-url.ts @@ -68,7 +68,12 @@ function ipv6IsPrivateOrLocal(host: string): boolean { const dotted = addr.match(/::ffff:(\d{1,3}\.\d{1,3}\.\d{1,3}\.\d{1,3})$/); /* v8 ignore next -- @preserve dotted ::ffff:N.N.N.N is normalized to hex by new URL() */ if (dotted) return ipv4IsPrivateOrLocal(dotted[1] as string); - const hex = addr.match(/^::ffff:([0-9a-f]{1,4}):([0-9a-f]{1,4})$/); + // Matches both the IPv4-mapped form (::ffff:7f00:1) and the older, `ffff:`-less IPv4-compatible + // form (::7f00:1, RFC 4291's deprecated ::/96) that `new URL()` normalizes the same bracket-free + // way: a literal `::127.0.0.1` or `::169.254.169.254` host reaches this branch with no "ffff" + // marker at all and was previously falling through to the final `return false` unchecked (SSRF + // bypass, #7777). + const hex = addr.match(/^::(?:ffff:)?([0-9a-f]{1,4}):([0-9a-f]{1,4})$/); if (hex) { const hi = parseInt(hex[1] as string, 16); const lo = parseInt(hex[2] as string, 16); diff --git a/src/review/content-lane/safe-url.ts b/src/review/content-lane/safe-url.ts index 93b7622165..4d400917bd 100644 --- a/src/review/content-lane/safe-url.ts +++ b/src/review/content-lane/safe-url.ts @@ -68,7 +68,12 @@ function ipv6IsPrivateOrLocal(host: string): boolean { const dotted = addr.match(/::ffff:(\d{1,3}\.\d{1,3}\.\d{1,3}\.\d{1,3})$/); /* v8 ignore next -- @preserve dotted ::ffff:N.N.N.N is normalized to hex by new URL() */ if (dotted) return ipv4IsPrivateOrLocal(dotted[1] as string); - const hex = addr.match(/^::ffff:([0-9a-f]{1,4}):([0-9a-f]{1,4})$/); + // Matches both the IPv4-mapped form (::ffff:7f00:1) and the older, `ffff:`-less IPv4-compatible + // form (::7f00:1, RFC 4291's deprecated ::/96) that `new URL()` normalizes the same bracket-free + // way: a literal `::127.0.0.1` or `::169.254.169.254` host reaches this branch with no "ffff" + // marker at all and was previously falling through to the final `return false` unchecked (SSRF + // bypass, #7777). + const hex = addr.match(/^::(?:ffff:)?([0-9a-f]{1,4}):([0-9a-f]{1,4})$/); if (hex) { const hi = parseInt(hex[1] as string, 16); const lo = parseInt(hex[2] as string, 16); diff --git a/src/review/lockfile-tamper.ts b/src/review/lockfile-tamper.ts index c7b90a284d..db68a0dac3 100644 --- a/src/review/lockfile-tamper.ts +++ b/src/review/lockfile-tamper.ts @@ -117,6 +117,21 @@ function scanPackageLockPatch(path: string, patch: string): LockfileTamperCandid let activeEntry: { entryKey: string; packageName: string } | null = null; let innerObjectDepth = 0; let sawPackagesEntry = false; + // True immediately after a header line is explicitly identified as NOT a real package entry (a + // container wrapper like "dependencies", or a bare key once we're already in node_modules/-keyed + // territory) -- suppresses the unattributed-entry fallback below for that block's own contents, so + // a deliberately-skipped key (see the "node_modules/" with nothing after the marker case) stays + // skipped rather than getting swept into a fallback bucket. Cleared by the next real header or a + // depth-0 close brace. + let insideRejectedBlock = false; + // Fallback bucket for a resolved/integrity/version change whose entry header isn't visible ANYWHERE + // in the diff -- git's default 3-line context doesn't guarantee an entry's opening-brace line + // survives when the changed field sits deeper than 3 lines into the entry (#7778). Without this, such + // 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. + let activeUnknownKey: string | null = null; + let unknownEntrySeq = 0; const entryFor = (entryKey: string, packageName: string): MutableCandidate => { const existing = byEntry.get(entryKey); @@ -144,14 +159,20 @@ function scanPackageLockPatch(path: string, patch: string): LockfileTamperCandid activeEntry = { entryKey: key, packageName: nodeModulesPackage }; innerObjectDepth = 0; sawPackagesEntry = true; + insideRejectedBlock = false; + activeUnknownKey = null; } else if (activeEntry) { innerObjectDepth++; } else if (!sawPackagesEntry && !CONTAINER_KEYS.has(key)) { activeEntry = { entryKey: key, packageName: key }; innerObjectDepth = 0; + insideRejectedBlock = false; + activeUnknownKey = null; } else { activeEntry = null; innerObjectDepth = 0; + insideRejectedBlock = true; + activeUnknownKey = null; } continue; } @@ -160,16 +181,27 @@ function scanPackageLockPatch(path: string, patch: string): LockfileTamperCandid innerObjectDepth--; } else { activeEntry = null; + insideRejectedBlock = false; + activeUnknownKey = null; } } - const currentEntryKey = activeEntry?.entryKey ?? null; - const currentPackageName = activeEntry?.packageName ?? 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); + let currentEntryKey = activeEntry?.entryKey ?? null; + let currentPackageName = activeEntry?.packageName ?? null; + if (!currentEntryKey && !insideRejectedBlock && line.sign !== " " && (resolvedMatch || integrityMatch || versionMatch)) { + if (!activeUnknownKey) { + unknownEntrySeq += 1; + activeUnknownKey = `${path}#unattributed-${unknownEntrySeq}`; + } + currentEntryKey = activeUnknownKey; + currentPackageName = "(unattributed lockfile entry)"; + } + if (!currentEntryKey || !currentPackageName || line.sign === " ") continue; + if (versionMatch) { const entry = entryFor(currentEntryKey, currentPackageName); // `line.sign` is guaranteed "+" or "-" here (never " ") by the `line.sign === " "` continue above -- a diff --git a/test/unit/content-lane-safe-url.test.ts b/test/unit/content-lane-safe-url.test.ts index 1d12d9752c..2a8049d9c4 100644 --- a/test/unit/content-lane-safe-url.test.ts +++ b/test/unit/content-lane-safe-url.test.ts @@ -126,6 +126,21 @@ describe("isSafeHttpUrl", () => { // Exercises ipv6IsPrivateOrLocal's final `return false` (not loopback/ULA/link-local/mapped). expect(isSafeHttpUrl("https://[2001:4860:4860::8888]")).toBe(true); }); + + it("rejects the ffff:-less IPv4-compatible IPv6 form pointing at loopback / cloud metadata (SSRF bypass, #7777)", () => { + // `new URL()` normalizes ::127.0.0.1 to hostname [::7f00:1] -- same bracket-free hex shape as the + // already-handled ::ffff:7f00:1 mapped form, just without the "ffff" marker. + expect(isSafeHttpUrl("https://[::127.0.0.1]")).toBe(false); + // ::169.254.169.254 -> [::a9fe:a9fe] -- the AWS/GCP/Azure cloud-metadata IP, the concrete exploit target. + expect(isSafeHttpUrl("https://[::169.254.169.254]")).toBe(false); + expect(isSafeEndpointUrl("wss://[::169.254.169.254]")).toBe(false); + }); + + it("accepts the ffff:-less IPv4-compatible IPv6 form when it points at a public IP", () => { + // ::8.8.8.8 -> [::808:808] -- exercises the new optional (?:ffff:)? group's non-present branch + // on the public side, matching the existing mapped-form public case just above. + expect(isSafeHttpUrl("https://[::8.8.8.8]")).toBe(true); + }); }); describe("isSafeEndpointUrl", () => { diff --git a/test/unit/lockfile-tamper.test.ts b/test/unit/lockfile-tamper.test.ts index 4227d9fd06..c1d9131b97 100644 --- a/test/unit/lockfile-tamper.test.ts +++ b/test/unit/lockfile-tamper.test.ts @@ -422,6 +422,89 @@ describe("lockfileTamperRiskFinding", () => { expect(finding?.detail).toContain("foo"); }); + // #7778: a resolved/integrity/version change can land in a hunk whose leading context (git's default + // 3 lines) doesn't reach back far enough to include its entry's own opening "node_modules/": { + // line -- before the fix, `activeEntry`/`currentEntryKey` stayed null for the whole hunk and the + // change was silently dropped rather than flagged. + it("still flags a changed integrity when the entry's own header line falls outside the diff's 3-line context window (#7778)", () => { + const lockPatch = [ + '@@ -50,10 +50,10 @@', + ' "version": "1.0.0",', + ' "resolved": "https://registry.npmjs.org/foo/-/foo-1.0.0.tgz",', + ' "license": "MIT",', + '- "integrity": "sha512-old=="', + '+ "integrity": "sha512-tampered=="', + ' "dependencies": {', + ' "bar": "^1.0.0"', + ' }', + ' }', + ].join("\n"); + // No `"node_modules/foo": {` header anywhere in this patch: it sits 4 lines above the changed + // integrity line, one line further back than git's default 3-line context window reaches. + const finding = lockfileTamperRiskFinding([lockfilePatch(lockPatch)]); + expect(finding).not.toBeNull(); + expect(finding?.code).toBe("lockfile_tamper_risk"); + expect(finding?.title).toContain("unattributed lockfile entry"); + }); + + it("does not flag an unrelated field change when no entry header is in view at all (#7778 fallback stays scoped to tracked fields)", () => { + const lockPatch = ['@@ -50,4 +50,4 @@', ' "license": "MIT",', '- "dev": true', '+ "dev": false', ' }'].join("\n"); + expect(lockfileTamperRiskFinding([lockfilePatch(lockPatch)])).toBeNull(); + }); + + it("does not flag a version-only change with no entry header in view (no resolved/integrity touched)", () => { + const lockPatch = [ + '@@ -50,4 +50,4 @@', + ' "resolved": "https://registry.npmjs.org/foo/-/foo-1.0.0.tgz",', + ' "license": "MIT",', + '- "version": "1.0.0",', + '+ "version": "1.0.1",', + ' }', + ].join("\n"); + // The unattributed fallback bucket is still created (a version line is a tracked field), but since + // resolved/integrity were never touched, resolvedOrIntegrityChanged stays false -- proves the + // fallback bucket tracks versionChanged correctly rather than always flagging once created. + expect(lockfileTamperRiskFinding([lockfilePatch(lockPatch)])).toBeNull(); + }); + + it("flags an off-registry resolved URL with no entry header in view", () => { + const lockPatch = [ + '@@ -50,4 +50,4 @@', + ' "license": "MIT",', + '- "resolved": "https://registry.npmjs.org/foo/-/foo-1.0.0.tgz",', + '+ "resolved": "https://evil.example.com/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("does not merge an unattributed change into a later, properly-attributed entry once a real header appears (#7778)", () => { + const lockPatch = [ + '@@ -50,13 +50,13 @@', + ' "license": "MIT",', + '- "integrity": "sha512-old-unattributed=="', + '+ "integrity": "sha512-new-unattributed=="', + ' },', + ' "node_modules/bar": {', + '- "version": "2.0.0",', + '- "resolved": "https://registry.npmjs.org/bar/-/bar-2.0.0.tgz",', + '- "integrity": "sha512-old-bar=="', + '+ "version": "2.1.0",', + '+ "resolved": "https://registry.npmjs.org/bar/-/bar-2.1.0.tgz",', + '+ "integrity": "sha512-new-bar=="', + ' },', + ].join("\n"); + const finding = lockfileTamperRiskFinding([lockfilePatch(lockPatch)]); + expect(finding).not.toBeNull(); + // "bar" is a legitimate, fully-bumped dependency and must not be swept into the unattributed + // bucket's tamper signal (nor let its own version bump mask the earlier unattributed one) -- + // only the truly unattributed integrity change should be flagged. + expect(finding?.detail).toContain("unattributed lockfile entry"); + expect(finding?.detail).not.toContain("bar"); + }); + it("tracks tamper signals inside a packages root wrapper and through optionalDependencies sub-objects", () => { const lockPatch = [ '@@ -1,12 +1,12 @@', diff --git a/test/unit/safe-url-engine.test.ts b/test/unit/safe-url-engine.test.ts index e1d3b8ed7b..68b35522ea 100644 --- a/test/unit/safe-url-engine.test.ts +++ b/test/unit/safe-url-engine.test.ts @@ -127,6 +127,21 @@ describe("isSafeHttpUrl", () => { // Exercises ipv6IsPrivateOrLocal's final `return false` (not loopback/ULA/link-local/mapped). expect(isSafeHttpUrl("https://[2001:4860:4860::8888]")).toBe(true); }); + + it("rejects the ffff:-less IPv4-compatible IPv6 form pointing at loopback / cloud metadata (SSRF bypass, #7777)", () => { + // `new URL()` normalizes ::127.0.0.1 to hostname [::7f00:1] -- same bracket-free hex shape as the + // already-handled ::ffff:7f00:1 mapped form, just without the "ffff" marker. + expect(isSafeHttpUrl("https://[::127.0.0.1]")).toBe(false); + // ::169.254.169.254 -> [::a9fe:a9fe] -- the AWS/GCP/Azure cloud-metadata IP, the concrete exploit target. + expect(isSafeHttpUrl("https://[::169.254.169.254]")).toBe(false); + expect(isSafeEndpointUrl("wss://[::169.254.169.254]")).toBe(false); + }); + + it("accepts the ffff:-less IPv4-compatible IPv6 form when it points at a public IP", () => { + // ::8.8.8.8 -> [::808:808] -- exercises the new optional (?:ffff:)? group's non-present branch + // on the public side, matching the existing mapped-form public case just above. + expect(isSafeHttpUrl("https://[::8.8.8.8]")).toBe(true); + }); }); describe("isSafeEndpointUrl", () => {