fix(review): close SSRF and lockfile-tamper detection gaps

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);

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);

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

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", () => {

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/<pkg>": {
+  // 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 @@',

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", () => {