Skip to content

Commit 0ff0b8f

Browse files
JSONboredloopover-orb[bot]
JSONbored
and
loopover-orb[bot]
authored
fix(review): close SSRF and lockfile-tamper detection gaps (#7826)
* fix(review): recognize IPv4-compatible IPv6 hosts in the SSRF guard (#7777) ipv6IsPrivateOrLocal in safe-url.ts recognized the IPv4-mapped IPv6 form (::ffff:a.b.c.d, normalized by new URL() to ::ffff:7f00:1) but not the older, ffff:-less IPv4-compatible form (::a.b.c.d, normalized the same bracket-free way to ::7f00:1). A URL like https://[::169.254.169.254] (cloud metadata) or https://[::127.0.0.1] (loopback) passed the guard as a public host. Generalize the existing hex-pair-to-IPv4 conversion to treat "ffff:" as optional, so both encodings are checked the same way. Applied identically to the byte-identical engine twin (packages/loopover-engine/src/review/safe-url.ts) to keep engine-parity:drift-check passing. Closes #7777 * fix(review): flag lockfile tamper changes with no entry header in view (#7778) scanPackageLockPatch tracked which package-lock entry a line belonged to only by watching for that entry's own opening "node_modules/<pkg>": { line in the diff. git's default 3-line context doesn't guarantee that line survives when a changed resolved/integrity/version field sits deeper into the entry -- when it doesn't, currentEntryKey stayed null for the whole hunk and the change was silently dropped instead of flagged. Add a fallback "unattributed entry" bucket for a tracked-field change with no known active entry, gated by a new insideRejectedBlock flag so the existing deliberate-skip case (a malformed "node_modules/" key with nothing after the marker) still behaves exactly as before. Closes #7778 --------- Co-authored-by: loopover-orb[bot] <296761690+loopover-orb[bot]@users.noreply.github.com>
1 parent 9d95c96 commit 0ff0b8f

6 files changed

Lines changed: 160 additions & 5 deletions

File tree

‎packages/loopover-engine/src/review/safe-url.ts‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -68,7 +68,12 @@ function ipv6IsPrivateOrLocal(host: string): boolean {
6868
const dotted = addr.match(/::ffff:(\d{1,3}\.\d{1,3}\.\d{1,3}\.\d{1,3})$/);
6969
/* v8 ignore next -- @preserve dotted ::ffff:N.N.N.N is normalized to hex by new URL() */
7070
if (dotted) return ipv4IsPrivateOrLocal(dotted[1] as string);
71-
const hex = addr.match(/^::ffff:([0-9a-f]{1,4}):([0-9a-f]{1,4})$/);
71+
// Matches both the IPv4-mapped form (::ffff:7f00:1) and the older, `ffff:`-less IPv4-compatible
72+
// form (::7f00:1, RFC 4291's deprecated ::/96) that `new URL()` normalizes the same bracket-free
73+
// way: a literal `::127.0.0.1` or `::169.254.169.254` host reaches this branch with no "ffff"
74+
// marker at all and was previously falling through to the final `return false` unchecked (SSRF
75+
// bypass, #7777).
76+
const hex = addr.match(/^::(?:ffff:)?([0-9a-f]{1,4}):([0-9a-f]{1,4})$/);
7277
if (hex) {
7378
const hi = parseInt(hex[1] as string, 16);
7479
const lo = parseInt(hex[2] as string, 16);

‎src/review/content-lane/safe-url.ts‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -68,7 +68,12 @@ function ipv6IsPrivateOrLocal(host: string): boolean {
6868
const dotted = addr.match(/::ffff:(\d{1,3}\.\d{1,3}\.\d{1,3}\.\d{1,3})$/);
6969
/* v8 ignore next -- @preserve dotted ::ffff:N.N.N.N is normalized to hex by new URL() */
7070
if (dotted) return ipv4IsPrivateOrLocal(dotted[1] as string);
71-
const hex = addr.match(/^::ffff:([0-9a-f]{1,4}):([0-9a-f]{1,4})$/);
71+
// Matches both the IPv4-mapped form (::ffff:7f00:1) and the older, `ffff:`-less IPv4-compatible
72+
// form (::7f00:1, RFC 4291's deprecated ::/96) that `new URL()` normalizes the same bracket-free
73+
// way: a literal `::127.0.0.1` or `::169.254.169.254` host reaches this branch with no "ffff"
74+
// marker at all and was previously falling through to the final `return false` unchecked (SSRF
75+
// bypass, #7777).
76+
const hex = addr.match(/^::(?:ffff:)?([0-9a-f]{1,4}):([0-9a-f]{1,4})$/);
7277
if (hex) {
7378
const hi = parseInt(hex[1] as string, 16);
7479
const lo = parseInt(hex[2] as string, 16);

‎src/review/lockfile-tamper.ts‎

Lines changed: 35 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -117,6 +117,21 @@ function scanPackageLockPatch(path: string, patch: string): LockfileTamperCandid
117117
let activeEntry: { entryKey: string; packageName: string } | null = null;
118118
let innerObjectDepth = 0;
119119
let sawPackagesEntry = false;
120+
// True immediately after a header line is explicitly identified as NOT a real package entry (a
121+
// container wrapper like "dependencies", or a bare key once we're already in node_modules/-keyed
122+
// territory) -- suppresses the unattributed-entry fallback below for that block's own contents, so
123+
// a deliberately-skipped key (see the "node_modules/" with nothing after the marker case) stays
124+
// skipped rather than getting swept into a fallback bucket. Cleared by the next real header or a
125+
// depth-0 close brace.
126+
let insideRejectedBlock = false;
127+
// Fallback bucket for a resolved/integrity/version change whose entry header isn't visible ANYWHERE
128+
// in the diff -- git's default 3-line context doesn't guarantee an entry's opening-brace line
129+
// survives when the changed field sits deeper than 3 lines into the entry (#7778). Without this, such
130+
// a change was silently dropped: `currentEntryKey` stayed null for the whole hunk, so the
131+
// `!currentEntryKey ... continue` guard below skipped it -- a tampered field could evade detection
132+
// entirely just by having enough unchanged sibling fields ahead of it in its entry.
133+
let activeUnknownKey: string | null = null;
134+
let unknownEntrySeq = 0;
120135

121136
const entryFor = (entryKey: string, packageName: string): MutableCandidate => {
122137
const existing = byEntry.get(entryKey);
@@ -144,14 +159,20 @@ function scanPackageLockPatch(path: string, patch: string): LockfileTamperCandid
144159
activeEntry = { entryKey: key, packageName: nodeModulesPackage };
145160
innerObjectDepth = 0;
146161
sawPackagesEntry = true;
162+
insideRejectedBlock = false;
163+
activeUnknownKey = null;
147164
} else if (activeEntry) {
148165
innerObjectDepth++;
149166
} else if (!sawPackagesEntry && !CONTAINER_KEYS.has(key)) {
150167
activeEntry = { entryKey: key, packageName: key };
151168
innerObjectDepth = 0;
169+
insideRejectedBlock = false;
170+
activeUnknownKey = null;
152171
} else {
153172
activeEntry = null;
154173
innerObjectDepth = 0;
174+
insideRejectedBlock = true;
175+
activeUnknownKey = null;
155176
}
156177
continue;
157178
}
@@ -160,16 +181,27 @@ function scanPackageLockPatch(path: string, patch: string): LockfileTamperCandid
160181
innerObjectDepth--;
161182
} else {
162183
activeEntry = null;
184+
insideRejectedBlock = false;
185+
activeUnknownKey = null;
163186
}
164187
}
165-
const currentEntryKey = activeEntry?.entryKey ?? null;
166-
const currentPackageName = activeEntry?.packageName ?? null;
167-
if (!currentEntryKey || !currentPackageName || line.sign === " ") continue;
168188

169189
const resolvedMatch = /^"resolved"\s*:\s*"([^"]*)"/.exec(body);
170190
const integrityMatch = /^"integrity"\s*:\s*"([^"]*)"/.exec(body);
171191
const versionMatch = /^"version"\s*:\s*"([^"]*)"/.exec(body);
172192

193+
let currentEntryKey = activeEntry?.entryKey ?? null;
194+
let currentPackageName = activeEntry?.packageName ?? null;
195+
if (!currentEntryKey && !insideRejectedBlock && line.sign !== " " && (resolvedMatch || integrityMatch || versionMatch)) {
196+
if (!activeUnknownKey) {
197+
unknownEntrySeq += 1;
198+
activeUnknownKey = `${path}#unattributed-${unknownEntrySeq}`;
199+
}
200+
currentEntryKey = activeUnknownKey;
201+
currentPackageName = "(unattributed lockfile entry)";
202+
}
203+
if (!currentEntryKey || !currentPackageName || line.sign === " ") continue;
204+
173205
if (versionMatch) {
174206
const entry = entryFor(currentEntryKey, currentPackageName);
175207
// `line.sign` is guaranteed "+" or "-" here (never " ") by the `line.sign === " "` continue above -- a

‎test/unit/content-lane-safe-url.test.ts‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -126,6 +126,21 @@ describe("isSafeHttpUrl", () => {
126126
// Exercises ipv6IsPrivateOrLocal's final `return false` (not loopback/ULA/link-local/mapped).
127127
expect(isSafeHttpUrl("https://[2001:4860:4860::8888]")).toBe(true);
128128
});
129+
130+
it("rejects the ffff:-less IPv4-compatible IPv6 form pointing at loopback / cloud metadata (SSRF bypass, #7777)", () => {
131+
// `new URL()` normalizes ::127.0.0.1 to hostname [::7f00:1] -- same bracket-free hex shape as the
132+
// already-handled ::ffff:7f00:1 mapped form, just without the "ffff" marker.
133+
expect(isSafeHttpUrl("https://[::127.0.0.1]")).toBe(false);
134+
// ::169.254.169.254 -> [::a9fe:a9fe] -- the AWS/GCP/Azure cloud-metadata IP, the concrete exploit target.
135+
expect(isSafeHttpUrl("https://[::169.254.169.254]")).toBe(false);
136+
expect(isSafeEndpointUrl("wss://[::169.254.169.254]")).toBe(false);
137+
});
138+
139+
it("accepts the ffff:-less IPv4-compatible IPv6 form when it points at a public IP", () => {
140+
// ::8.8.8.8 -> [::808:808] -- exercises the new optional (?:ffff:)? group's non-present branch
141+
// on the public side, matching the existing mapped-form public case just above.
142+
expect(isSafeHttpUrl("https://[::8.8.8.8]")).toBe(true);
143+
});
129144
});
130145

131146
describe("isSafeEndpointUrl", () => {

‎test/unit/lockfile-tamper.test.ts‎

Lines changed: 83 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -422,6 +422,89 @@ describe("lockfileTamperRiskFinding", () => {
422422
expect(finding?.detail).toContain("foo");
423423
});
424424

425+
// #7778: a resolved/integrity/version change can land in a hunk whose leading context (git's default
426+
// 3 lines) doesn't reach back far enough to include its entry's own opening "node_modules/<pkg>": {
427+
// line -- before the fix, `activeEntry`/`currentEntryKey` stayed null for the whole hunk and the
428+
// change was silently dropped rather than flagged.
429+
it("still flags a changed integrity when the entry's own header line falls outside the diff's 3-line context window (#7778)", () => {
430+
const lockPatch = [
431+
'@@ -50,10 +50,10 @@',
432+
' "version": "1.0.0",',
433+
' "resolved": "https://registry.npmjs.org/foo/-/foo-1.0.0.tgz",',
434+
' "license": "MIT",',
435+
'- "integrity": "sha512-old=="',
436+
'+ "integrity": "sha512-tampered=="',
437+
' "dependencies": {',
438+
' "bar": "^1.0.0"',
439+
' }',
440+
' }',
441+
].join("\n");
442+
// No `"node_modules/foo": {` header anywhere in this patch: it sits 4 lines above the changed
443+
// integrity line, one line further back than git's default 3-line context window reaches.
444+
const finding = lockfileTamperRiskFinding([lockfilePatch(lockPatch)]);
445+
expect(finding).not.toBeNull();
446+
expect(finding?.code).toBe("lockfile_tamper_risk");
447+
expect(finding?.title).toContain("unattributed lockfile entry");
448+
});
449+
450+
it("does not flag an unrelated field change when no entry header is in view at all (#7778 fallback stays scoped to tracked fields)", () => {
451+
const lockPatch = ['@@ -50,4 +50,4 @@', ' "license": "MIT",', '- "dev": true', '+ "dev": false', ' }'].join("\n");
452+
expect(lockfileTamperRiskFinding([lockfilePatch(lockPatch)])).toBeNull();
453+
});
454+
455+
it("does not flag a version-only change with no entry header in view (no resolved/integrity touched)", () => {
456+
const lockPatch = [
457+
'@@ -50,4 +50,4 @@',
458+
' "resolved": "https://registry.npmjs.org/foo/-/foo-1.0.0.tgz",',
459+
' "license": "MIT",',
460+
'- "version": "1.0.0",',
461+
'+ "version": "1.0.1",',
462+
' }',
463+
].join("\n");
464+
// The unattributed fallback bucket is still created (a version line is a tracked field), but since
465+
// resolved/integrity were never touched, resolvedOrIntegrityChanged stays false -- proves the
466+
// fallback bucket tracks versionChanged correctly rather than always flagging once created.
467+
expect(lockfileTamperRiskFinding([lockfilePatch(lockPatch)])).toBeNull();
468+
});
469+
470+
it("flags an off-registry resolved URL with no entry header in view", () => {
471+
const lockPatch = [
472+
'@@ -50,4 +50,4 @@',
473+
' "license": "MIT",',
474+
'- "resolved": "https://registry.npmjs.org/foo/-/foo-1.0.0.tgz",',
475+
'+ "resolved": "https://evil.example.com/foo-1.0.0.tgz",',
476+
' }',
477+
].join("\n");
478+
const finding = lockfileTamperRiskFinding([lockfilePatch(lockPatch)]);
479+
expect(finding).not.toBeNull();
480+
expect(finding?.detail).toContain("outside registry.npmjs.org");
481+
});
482+
483+
it("does not merge an unattributed change into a later, properly-attributed entry once a real header appears (#7778)", () => {
484+
const lockPatch = [
485+
'@@ -50,13 +50,13 @@',
486+
' "license": "MIT",',
487+
'- "integrity": "sha512-old-unattributed=="',
488+
'+ "integrity": "sha512-new-unattributed=="',
489+
' },',
490+
' "node_modules/bar": {',
491+
'- "version": "2.0.0",',
492+
'- "resolved": "https://registry.npmjs.org/bar/-/bar-2.0.0.tgz",',
493+
'- "integrity": "sha512-old-bar=="',
494+
'+ "version": "2.1.0",',
495+
'+ "resolved": "https://registry.npmjs.org/bar/-/bar-2.1.0.tgz",',
496+
'+ "integrity": "sha512-new-bar=="',
497+
' },',
498+
].join("\n");
499+
const finding = lockfileTamperRiskFinding([lockfilePatch(lockPatch)]);
500+
expect(finding).not.toBeNull();
501+
// "bar" is a legitimate, fully-bumped dependency and must not be swept into the unattributed
502+
// bucket's tamper signal (nor let its own version bump mask the earlier unattributed one) --
503+
// only the truly unattributed integrity change should be flagged.
504+
expect(finding?.detail).toContain("unattributed lockfile entry");
505+
expect(finding?.detail).not.toContain("bar");
506+
});
507+
425508
it("tracks tamper signals inside a packages root wrapper and through optionalDependencies sub-objects", () => {
426509
const lockPatch = [
427510
'@@ -1,12 +1,12 @@',

‎test/unit/safe-url-engine.test.ts‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -127,6 +127,21 @@ describe("isSafeHttpUrl", () => {
127127
// Exercises ipv6IsPrivateOrLocal's final `return false` (not loopback/ULA/link-local/mapped).
128128
expect(isSafeHttpUrl("https://[2001:4860:4860::8888]")).toBe(true);
129129
});
130+
131+
it("rejects the ffff:-less IPv4-compatible IPv6 form pointing at loopback / cloud metadata (SSRF bypass, #7777)", () => {
132+
// `new URL()` normalizes ::127.0.0.1 to hostname [::7f00:1] -- same bracket-free hex shape as the
133+
// already-handled ::ffff:7f00:1 mapped form, just without the "ffff" marker.
134+
expect(isSafeHttpUrl("https://[::127.0.0.1]")).toBe(false);
135+
// ::169.254.169.254 -> [::a9fe:a9fe] -- the AWS/GCP/Azure cloud-metadata IP, the concrete exploit target.
136+
expect(isSafeHttpUrl("https://[::169.254.169.254]")).toBe(false);
137+
expect(isSafeEndpointUrl("wss://[::169.254.169.254]")).toBe(false);
138+
});
139+
140+
it("accepts the ffff:-less IPv4-compatible IPv6 form when it points at a public IP", () => {
141+
// ::8.8.8.8 -> [::808:808] -- exercises the new optional (?:ffff:)? group's non-present branch
142+
// on the public side, matching the existing mapped-form public case just above.
143+
expect(isSafeHttpUrl("https://[::8.8.8.8]")).toBe(true);
144+
});
130145
});
131146

132147
describe("isSafeEndpointUrl", () => {

0 commit comments

Comments
 (0)