fix(builder): prove eviction in tests, strip XFF port suffixes, document the fallback collapse edges

packages/builder/src/auth.ts

@@ -16,6 +16,24 @@ export interface AuthConfig {
 
 export const ANONYMOUS_PREFIX = 'anon:';
 
+/**
+ * Strips a trailing port suffix from a proxy-supplied address. The trusted proxy
+ * SHOULD emit a bare address, but a proxy that includes the ephemeral source port
+ * (e.g. "$remote_addr:$remote_port"-style config) would otherwise fragment one
+ * client across many rate-limit buckets - ephemeral ports rotate per connection,
+ * which defeats both per-IP limiting and the registry cap. Bracketed IPv6
+ * ("[::1]:8080", or bare "[::1]" with no port) is unwrapped; unbracketed IPv6 is
+ * returned unchanged, since its own colons are address, not a port delimiter, and
+ * there is no way to tell them apart without the brackets.
+ */
+function stripPort(addr: string): string {
+  const bracketed = /^\[(.+)\](?::\d+)?$/.exec(addr);
+  if (bracketed) return bracketed[1];
+  const ipv4WithPort = /^(\d{1,3}(?:\.\d{1,3}){3}):\d+$/.exec(addr);
+  if (ipv4WithPort) return ipv4WithPort[1];
+  return addr;
+}
+
 /**
  * Client address for rate-limit identity when auth is disabled.
  *
@@ -23,19 +41,41 @@ export const ANONYMOUS_PREFIX = 'anon:';
  * APPENDS the true peer address to X-Forwarded-For. The RIGHTMOST entry is therefore
  * the only trustworthy one; everything left of it is client-supplied and forgeable.
  * Direct access (self-hosting without a proxy) has no XFF header and falls back to the
- * socket's remote address.
+ * socket's remote address. The trusted proxy SHOULD emit that rightmost entry as a bare
+ * address with no port; a port suffix is stripped defensively (see stripPort) rather than
+ * trusted to be absent.
+ *
+ * Two fallback-tier edges collapse distinct callers into one bucket by design, not by
+ * bug - both are accepted degraded modes of this fallback tier, and both are visible
+ * in logs by their bucket name rather than silently misattributed:
+ * - In the proxied topology, the socket-fallback path sees the PROXY's own loopback
+ *   address, never the original caller's - the proxy is the direct TCP peer. Any
+ *   request that reaches the builder with no XFF header at all (a proxy
+ *   misconfiguration, a health check, direct loopback access) lands in one
+ *   ordinary-looking `anon:127.0.0.1` bucket shared by everything that arrived that way.
+ * - A request with neither an XFF header nor a readable socket address collapses to
+ *   the shared `anon:unknown` bucket.
  */
 export function clientIpOf(req: {
   headers: Record<string, string | string[] | undefined>;
   socket?: { remoteAddress?: string };
 }): string {
+  // Node joins duplicate X-Forwarded-For header instances into one comma-joined
+  // string by default (only set-cookie is ever really an array), so this array
+  // branch is defensive - it matches the general IncomingHttpHeaders shape rather
+  // than an observed runtime case.
   const raw = req.headers['x-forwarded-for'];
   const headerValue = Array.isArray(raw) ? raw[raw.length - 1] : raw;
   if (headerValue) {
     const entries = headerValue.split(',');
-    const rightmost = entries[entries.length - 1]?.trim();
-    if (rightmost) return rightmost.replace(/^::ffff:/i, '');
+    // split() always returns at least one element, so this index is never out of
+    // range.
+    const rightmost = entries[entries.length - 1].trim();
+    if (rightmost) return stripPort(rightmost).replace(/^::ffff:/i, '');
   }
+  // socket.remoteAddress is kernel-supplied and never carries a port (Node exposes
+  // remotePort separately), so no stripPort pass is needed here - only the
+  // proxy-controlled XFF value can carry one.
   const remote = req.socket?.remoteAddress;
   if (remote) return remote.replace(/^::ffff:/i, '');
   return 'unknown';

packages/builder/test/auth.test.ts

@@ -341,6 +341,18 @@ describe('clientIpOf', () => {
   test('an empty rightmost entry falls back rather than yielding an empty identity', () => {
     assert.equal(clientIpOf(mk('203.0.113.50,', '10.0.0.1')), '10.0.0.1');
   });
+
+  test('strips a trailing port suffix from IPv4 and bracketed IPv6; unbracketed IPv6 is left intact', () => {
+    // The trusted proxy SHOULD emit a bare address, but a port suffix is stripped
+    // defensively so a proxy that includes the ephemeral source port doesn't
+    // fragment one client across many rate-limit buckets.
+    assert.equal(clientIpOf(mk('203.0.113.50:8080')), '203.0.113.50');
+    assert.equal(clientIpOf(mk('[2001:db8::1]:8080')), '2001:db8::1');
+    assert.equal(clientIpOf(mk('[2001:db8::1]')), '2001:db8::1');
+    // Unbracketed IPv6 has no unambiguous port delimiter - its own colons must
+    // never be mistaken for one.
+    assert.equal(clientIpOf(mk('2001:db8::1')), '2001:db8::1');
+  });
 });
 
 describe('resolveIdentity with auth disabled (per-IP anonymous buckets)', () => {

packages/builder/test/rate-limiter.test.ts

@@ -121,15 +121,29 @@ describe('limiter registry cap', () => {
   test('evicts the least-recently-used identity at the cap, and refresh-on-access protects active callers', () => {
     resetLimiters();
     const first = limiterFor('keeper');
-    for (let i = 0; i < MAX_TRACKED_IDENTITIES - 1; i++) limiterFor(`filler-${i}`);
+    // Captured at the moment filler-0 is created, so it can be compared against
+    // (not just type-checked against) whatever limiterFor('filler-0') returns
+    // after the overflow below - see the comment on that assertion for why a
+    // same-object check is the load-bearing part of this test.
+    const originalFillerZero = limiterFor('filler-0');
+    for (let i = 1; i < MAX_TRACKED_IDENTITIES - 1; i++) limiterFor(`filler-${i}`);
     // registry is now exactly at the cap; 'keeper' is the oldest by insertion.
     assert.equal(limiterFor('keeper'), first, 'accessing keeper must refresh its recency, not evict it');
     limiterFor('overflow-1'); // evicts filler-0 (now the oldest), NOT keeper
     assert.equal(limiterFor('keeper'), first, 'keeper must survive the overflow eviction');
-    // filler-0 was evicted: asking for it again yields a FRESH instance (budget reset - the
-    // documented memory-bound trade).
-    const refilled = limiterFor('filler-0');
-    assert.ok(refilled instanceof RateLimiter);
+    // filler-0 was evicted: asking for it again must yield a FRESH instance (budget
+    // reset - the documented memory-bound trade), not the object captured above.
+    // An `instanceof RateLimiter` check alone cannot tell "evicted, this is a new
+    // one" apart from "never evicted, this is still the same one" - limiterFor's
+    // return type already guarantees instanceof, so that check passes whether or
+    // not eviction actually ran. Proven by mutation test: deleting the cap-eviction
+    // block in limiterFor left the old assertion green (see task-1-report.md for
+    // the recorded failure output of THIS assertion against that mutation).
+    assert.notEqual(
+      limiterFor('filler-0'),
+      originalFillerZero,
+      'filler-0 must be a FRESH instance - eviction really happened and its budget reset'
+    );
     resetLimiters();
   });
 });