fix(server): use ipKeyGenerator for IPv6 subnet normalisation

[azizur100389]

Closes #1360

Summary

• createRouteLimiter used a custom keyGenerator that referenced req.ip directly without calling ipKeyGenerator from express-rate-limit v8. This triggered ERR_ERL_KEY_GEN_IPV6 on startup when binding to 0.0.0.0, and let each full IPv6 address get its own counter — trivially bypassing the per-IP rate limit.
• Import ipKeyGenerator and pass the resolved IP through it so IPv6 addresses are collapsed to their /56 subnet. The existing req.ip → socket.remoteAddress → 'unknown' fallback chain is preserved.

Changes

File	What
gitnexus/src/server/validation.ts	Import ipKeyGenerator; wrap IP through it in keyGenerator
gitnexus/test/unit/rate-limit.test.ts	3 new assertions: construction-time regression guard, source-grep for import and call site

Test plan

• npx tsc --noEmit — clean
• npx vitest run test/unit/rate-limit.test.ts — 19/19 pass (was 16, +3 new)
• npx prettier --check on changed files — clean
• Pre-commit hook (eslint + prettier + typecheck) — clean

[vercel]

@azizur100389 is attempting to deploy a commit to the NexusCore Team on Vercel.

A member of the Team first needs to authorize it.

[magyargergo]

Nice find! Thank you!

[github-actions]

Claude finished @magyargergo's task in 4m 8s —— View job

---

PR #1374 Review — fix(server): use ipKeyGenerator for IPv6 subnet normalisation

Reviewed by: Senior Principal SWE · Security Reviewer · IPv6/Proxy Reviewer · Test Quality Reviewer · Elegance Reviewer

---

Review Bar for This PR

• createRouteLimiter() imports and calls ipKeyGenerator from express-rate-limit — raw req.ip must not be returned directly as the limiter key
• Fallback chain req.ip → req.socket?.remoteAddress → 'unknown' preserved; 'unknown' reachable only on abnormal/dropped connections
• ipKeyGenerator normalises IPv6 to /56 subnet via the library helper — no custom string logic
• trust proxy in api.ts remains correctly wired and compatible with the key generator
• Tests prove the construction-time regression and structurally guard against removing ipKeyGenerator; existing behavioural tests (429, headers, window reset, route wiring) still pass
• No hidden bidi/Unicode controls in executable code or tests
• Diff is strictly scoped: 2 files, no dependency or policy changes

---

Current PR State

Attribute	Value
Head SHA	751874b934a24a29771f62c7e7a10dc1ed4448b7
Status	Open, not draft
Commits	1
Changed files	2 (validation.ts +8/−2, rate-limit.test.ts +35/−0)
CI (8 checks)	✅ All green — PR Labeler ×2, Dependency Review, Gitleaks, PR Description Check, CodeQL, CI, Docker Build & Push
Typecheck	✅ PR body: npx tsc --noEmit clean; CodeQL pass confirms it
Rate-limit tests	✅ PR body: 19/19 (was 16, +3 new)
Issue #1360 linkage	✅ PR body + commit reference
Hidden Unicode	⚠️ Non-ASCII detected; see hygiene section below
Bidi controls	✅ None
Vercel	Irrelevant to this server/rate-limit fix
Code matches PR body	✅ Verified against actual diff

---

Understanding the Change

Why raw req.ip is unsafe for IPv6 with express-rate-limit v8: In v8, the library requires custom key generators to normalise IPv6 addresses via ipKeyGenerator. Without this, each full /128 IPv6 address gets its own counter — a client can trivially bypass rate limiting by rotating source addresses within the same /56 subnet assignment. The library signals this with ERR_ERL_KEY_GEN_IPV6.

What /56 normalisation means: ipKeyGenerator collapses all addresses from the same ISP-assigned /56 block to a single string key. Multiple clients behind a residential IPv6 prefix share one counter, which is the library's intended semantics.

Why the socket fallback exists: In some connection states (abruptly closed, reverse proxy strip) req.ip can be undefined. Without a fallback, ipKeyGenerator(undefined) would throw ERR_ERL_UNDEFINED_IP_ADDRESS, surfacing as an HTML 500 from Express's default handler. The fallback to req.socket?.remoteAddress and ultimately 'unknown' keeps the server alive.

Why trust proxy still matters: req.ip is derived from X-Forwarded-For only when trust-proxy is set. If incorrectly configured, req.ip returns the proxy's IP for all clients — collapsing all traffic into one counter and enabling per-origin bypass. The fix does not change this dependency; it correctly documents it in the JSDoc.

---

Findings

[minor] Construction-time test comment overstates when the error fires

• Category: Test accuracy / regression guard reliability
• Files: gitnexus/test/unit/rate-limit.test.ts:83–90
• Issue: The test comment states "The validation fires at construction time (inside rateLimit())". However, ERR_ERL_KEY_GEN_IPV6 is thrown at request time — when the key generator returns a raw IPv6 address — not during rateLimit() construction. This means expect(() => createRouteLimiter()).not.toThrow() would have passed even before this fix (since construction never throws), making it a near-false-positive as a regression discriminator. The test does no harm and adds some signal, but its comment overstates its protective value.
• Why it matters: A reviewer or future maintainer might trust this test as proof that removing ipKeyGenerator would cause a construction failure, when in reality the breakage would occur at request time on an IPv6 client. The source-grep tests (lines 282–288) are the actual reliable regression guards here.
• Recommended fix: Update the comment to accurately describe when the error fires (request time, not construction), and acknowledge that the construction test is a canary for import issues while the source-grep tests are the structural guard. Alternatively, add a small integration test that exercises keyGenerator with a mock IPv6 req.ip and asserts the returned key differs from the raw address. Fix this →
• Blocks merge: No — the implementation is correct; this is a documentation/comment quality issue in the test.

[minor] No direct behavioural assertion on IPv6 key collapse

• Category: Test coverage
• Files: gitnexus/test/unit/rate-limit.test.ts
• Issue: There is no test that exercises the keyGenerator with a representative IPv6 address and asserts the returned key is the normalised subnet form rather than the raw address. The source-grep tests verify ipKeyGenerator is called, but do not prove the actual key produced for 2001:db8::1 differs from 2001:db8::2 within the same /56.
• Why it matters: If ipKeyGenerator were replaced with a no-op (e.g., (ip) => ip), the source-grep test for the call site would still pass. The behavioural gap is acceptable given that testing library internals is out of scope, but a single direct key-normalisation assertion would make the intent provably correct.
• Recommended fix: Consider extracting the keyGenerator option from a constructed limiter (express-rate-limit exposes it) and asserting keyGenerator({ip: '2001:db8:abcd:1200::1'}) === keyGenerator({ip: '2001:db8:abcd:12ff::1'}). Fix this →
• Blocks merge: No — source-grep plus construction test provide acceptable regression coverage for a two-file fix.

---

Implementation Assessment

The implementation is correct and minimal.

validation.ts line 22 — import:

import rateLimit, { type RateLimitRequestHandler, ipKeyGenerator } from 'express-rate-limit';

Clean single import; no duplicate or conflicting import remains. ipKeyGenerator is named-imported alongside the existing type import.

validation.ts lines 157–160 — keyGenerator:

keyGenerator: (req: Request) => {
  const ip = req.ip ?? req.socket?.remoteAddress;
  return ip ? ipKeyGenerator(ip) : 'unknown';
},

Fallback chain is correctly preserved: req.ip → socket.remoteAddress → 'unknown'. The 'unknown' branch is only reachable when both are falsy (abnormal/dropped connections). All normal requests produce an ipKeyGenerator-normalised key. Raw req.ip is never returned directly as the limiter key — the blocking condition is satisfied.

windowMs, limit, standardHeaders, legacyHeaders, passOnStoreError, and message are all unchanged. RouteLimiterOverrides is unchanged. The narrowly-scoped overrides surface (windowMs, limit) correctly prevents callers from disabling the limiter via skip.

Comments: Accurately explain IPv6 normalisation rationale, the ERR_ERL_UNDEFINED_IP_ADDRESS motivation for the socket fallback, and the trust-proxy dependency. No stale, narrated, or excessive comments.

---

IPv6 / Proxy Assessment

Scenario	Behaviour	Status
IPv6 client (routed)	Normalised to /56 subnet key via ipKeyGenerator	✅ Correct
IPv4 client	ipKeyGenerator is an identity for IPv4	✅ Correct
IPv4-mapped IPv6 (::ffff:1.2.3.4)	ipKeyGenerator handles this form	✅ Correct
Loopback (127.0.0.1, ::1)	trust proxy set to loopback — req.ip populated	✅ Correct
Docker / private network	trust proxy includes linklocal, uniquelocal	✅ Correct
Dropped/abruptly closed connection	'unknown' fallback — does not trigger ERR_ERL_UNDEFINED_IP_ADDRESS	✅ Correct
X-Forwarded-For spoofing	Unchanged risk; trust proxy is loopback, linklocal, uniquelocal only — reverse proxies in the private range are trusted	ℹ️ Acceptable for local-bound deployment

api.ts:647: app.set('trust proxy', 'loopback, linklocal, uniquelocal') — still correctly set, unchanged by this PR. The trust proxy setting and ipKeyGenerator are complementary: trust proxy determines what req.ip is; ipKeyGenerator determines the rate-limit key derived from it.

'unknown' fallback collapse risk: Only reachable on genuinely abnormal connections (socket closed before Express reads the address). Normal traffic cannot hit this path under the current trust-proxy configuration. Not a concern.

req.socket?.remoteAddress passed to ipKeyGenerator: Socket remoteAddress is always a valid IP string or undefined. The optional chain ensures ipKeyGenerator is only called with a string. No format-safety concern.

---

Test Assessment

Construction-time regression test (rate-limit.test.ts:88–90):

it('does not throw ERR_ERL_KEY_GEN_IPV6 on construction (#1360)', () => {
  expect(() => createRouteLimiter()).not.toThrow();
});

Provides a canary for import errors (e.g., removing the ipKeyGenerator import would cause a ReferenceError at construction). However, as noted in Findings, ERR_ERL_KEY_GEN_IPV6 fires at request time, not construction — so this test would pass even before the fix. Its value is as a "does not import-error" guard, not a full IPv6-bypass regression guard.

Source-grep tests (rate-limit.test.ts:282–288):

expect(validationSource).toMatch(/import.*ipKeyGenerator.*from\s+'express-rate-limit'/);
expect(validationSource).toMatch(/ipKeyGenerator\(ip\)/);

These will fail if ipKeyGenerator is removed from the import or the call site — they are the actual reliable regression guards for this PR. The regexes are tight enough to catch obvious removal but forgiving of whitespace. Acceptable for a two-file targeted fix.

Existing behavioural tests — all 16 pre-existing tests remain correct and unchanged:

• 429 fires after limit ✅
• Draft-7 RateLimit header emitted (no legacy X-RateLimit-*) ✅
• JSON error shape { error: "Too many..." } ✅
• Retry-After header present ✅
• Window resets after windowMs ✅
• Production-default permits 60 req/min ✅
• 6 production route wiring checks (including SPA_FALLBACK_REGEX and trust proxy) ✅

Tests are deterministic, loopback-only (no external network), cross-platform (OS tmpdir, no POSIX-isms).

---

Hidden Unicode / Hygiene Assessment

Bidi controls (U+202A–U+202E, U+2066–U+2069): ✅ None found. No bidirectional override or isolate characters in either changed file. GitHub's Unicode warning is a false alarm here.

Non-ASCII characters: Found in comments only — all are U+2014 em dashes (—) used in JSDoc prose (req.ip → socket fallback, normalised to their /56 subnet — without this…). These appear throughout the pre-existing file as a house style choice. They are not in executable code, test logic, regex patterns, or import statements. No invisible spaces, zero-width joiners, or hidden controls detected.

Scope: Exactly 2 files changed. No package.json, package-lock.json, AGENTS.md, CLAUDE.md, generated files, debug logging, or unrelated churn. express-rate-limit version unchanged at ^8.4.1.

---

Elegance Assessment

This is an exemplary minimal fix. The diff is 10 effective lines: 1 import line modified, 1 keyGenerator one-liner expanded to 3 lines with the ipKeyGenerator wrapper, and 3 comment lines added to the JSDoc. The approach is exactly correct — use the library's own helper rather than custom string manipulation. No over-engineering, no new abstractions, no speculative features. Comments explain the why (IPv6 bypass, trust-proxy dependency, ERR_ERL_UNDEFINED_IP_ADDRESS prevention) without narrating the what. The RouteLimiterOverrides narrowing surface is unchanged and correctly serves its purpose.

---

Final Verdict

production-ready with minor follow-ups

The core fix is correct: ipKeyGenerator is imported and called on every resolved IP, raw req.ip is never returned directly as a limiter key, the fallback chain is preserved, and ERR_ERL_UNDEFINED_IP_ADDRESS cannot fire on normal traffic. CI is fully green across all 8 checks including CodeQL. The two follow-up items — correcting the construction-test comment and optionally adding a direct IPv6 key-collapse assertion — are polish, not correctness blockers. The non-ASCII characters flagged by GitHub are benign em dashes in comments only, with zero bidi controls present. This PR is safe to merge as-is; the minor follow-ups can be addressed in a subsequent commit or as part of the next test-coverage pass.

---

· Branch: fix/ipv6-rate-limit-keygen