[pyramation]

Summary

Bug. graphql/server mounted request-ip's middleware (which sets req.clientIp from the leftmost X-Forwarded-For entry) and set Express trust proxy to an always-true predicate. auth.ts and graphile.ts then put req.clientIp into the transaction as jwt.claims.ip_address. Any caller could therefore send X-Forwarded-For: 1.2.3.4 and pick — and rotate per request — the address every per-IP limit and audit row in the database is keyed on. Admission control and the refusal recorder already did this correctly with clientIpFrom(req, hops) (rightmost-minus-trusted-hops, never the leftmost), so the server had two notions of "the client's address".

Fix. One trusted client address per request, resolved once and read everywhere:

// @constructive-io/express-context (new client-ip.ts)
clientIpMiddleware(trustedProxyHops)   // req.clientIp = clientIpFrom(req, hops)
clientIpOf(req)                        // req.clientIp ?? socket peer — never the header

// graphql/server/src/server.ts
- trustProxy(app, opts.server.trustProxy);   // app.set('trust proxy', () => true)
- app.use(requestIp.mw());
+ app.set('trust proxy', hops > 0 ? hops : false);
+ app.use(clientIpMiddleware(hops));          // hops = opts.server.trustedProxyHops ?? 0

• auth.ts / graphile.ts / buildPgSettings keep reading req.clientIp, which now carries the trusted address.
• createAdmissionControlMiddleware() and recordRefusal() read clientIpOf(req) instead of re-deriving hops themselves; their trustedProxyHops options and the req.app.get('trust proxy') ? 1 : 0 guess are gone.
• Config: ServerOptions.trustProxy: boolean → ServerOptions.trustedProxyHops: number (default 0), env SERVER_TRUST_PROXY → TRUSTED_PROXY_HOPS, parsed by @pgpmjs/env and so by @constructive-io/graphql-env's getEnvOptions. The raw process.env.TRUSTED_PROXY_HOPS reader in express-context is removed.
• request-ip / @types/request-ip dropped (graphql/server, and an unused declaration in graphile-settings). pnpm-lock.yaml only loses those entries.
• No leftmost-XFF fallback anywhere: 0 hops ⇒ socket peer, XFF ignored; a header shorter than the hop count or malformed ⇒ socket peer.

Deployment

TRUSTED_PROXY_HOPS must equal the number of X-Forwarded-For-appending proxies you control between the client and this server — proxies that discard the caller-supplied header and append the peer address they themselves saw. 0 (default) trusts only the TCP peer; one reverse proxy/ingress directly in front of the server is 1. Counting higher than the deployment really has lets a direct caller choose its own address, so when in doubt count lower. The edge must also preserve the client source address all the way to the first trusted proxy (no SNAT in between), otherwise the trusted entry is an internal hop's address and every per-IP limit collapses onto it. Deployments that set SERVER_TRUST_PROXY=true behind a single proxy need TRUSTED_PROXY_HOPS=1.

Nothing inside this repo adds a hop: the GraphQL server is served directly by Express, with no proxy process in front of it.

Tests

• graphql/server-test/__tests__/client-ip.integration.test.ts — real server via getConnections, a fixture query requestClientIp that returns current_setting('jwt.claims.ip_address'): behind 1 hop a forged leftmost entry yields the proxy-appended address, cannot be rotated, absent/malformed header falls back to the socket peer; with 0 hops XFF is ignored entirely.
• packages/express-context/__tests__/client-ip.test.ts — req.clientIp for 0 hops, 1 hop (forged leftmost, rotation), absent/malformed/short chain, non-positive hop count.
• admission-control.test.ts updated: keys on req.clientIp, falls back to the socket peer (never the header) when the middleware did not run.

Before (main a94434095 + these tests, local run):

FAIL __tests__/client-ip.integration.test.ts
  ✕ is the address the proxy appended, not the one the caller forged
      Expected: "203.0.113.7"   Received: "1.2.3.4"
  ✕ cannot be rotated per request by rewriting the leftmost entry
      Expected ["203.0.113.7"]  Received ["5.5.5.5", "6.6.6.6", "7.7.7.7"]
  ✕ is the socket peer and ignores X-Forwarded-For entirely
      Expected: "127.0.0.1"     Received: "1.2.3.4"
Tests: 3 failed, 2 passed, 5 total

FAIL packages/express-context/__tests__/client-ip.test.ts
  TS2307: Cannot find module '../src/client-ip'

After (this branch):

PASS __tests__/client-ip.integration.test.ts   Tests: 5 passed
packages/express-context                         Tests: 134 passed
graphql/server src/middleware + src/refusals     Tests: 143 passed
pgpm/env                                         Tests: 26 passed, 1 snapshot
graphql/env                                      Tests: 53 passed, 1 snapshot

Lint clean on the touched packages; pnpm --filter ... build green for @pgpmjs/types, @pgpmjs/env, @pgpmjs/server-utils, @constructive-io/express-context, @constructive-io/graphql-server, graphql-server-test.

Link to Devin session: https://app.devin.ai/sessions/959661f0d69640789bd6b38550d379b7
Open in Devin Desktop: https://app.devin.ai/desktop/session/959661f0d69640789bd6b38550d379b7?variant=devin
Requested by: @pyramation

[devin-ai-integration]

I'll fix CI failures and address comments from users with write access. I'll skip comments containing "(aside)".

• Disable automatic comment, CI, and merge conflict monitoring

[tenki-reviewer]

Review complete. 🟠 1 high · 🟡 1 medium

📍 Findings outside the diff (2) — 🟠 1 high · 🟡 1 medium — defects on lines GitHub can't attach comments to

🟠 High — Aborted queued requests permanently leak concurrency slots · admission-control.ts:96–105 · unchanged line

// graphql/server/src/middleware/admission-control.ts
96	    let lease: AdmissionLease;
97	    try {
98	      lease = await concurrency.acquire(databaseId, {
99	        limit: protection.maxConcurrentRequests,
100	        queueWaitMs: protection.maxQueueWaitMs
101	      });
102	    } catch (e) {
103	      next(e);
104	      return;
105	    }

The release listeners are attached only after await concurrency.acquire(...) resolves (graphql/server/src/middleware/admission-control.ts:136-137), but acquire can wait up to maxQueueWaitMs inside the queue (packages/express-context/src/admission.ts:126). A client that hangs up while queued emits close on the response during that await — before any listener exists — so the event is missed; once the slot is later granted to this dead request, neither close (already fired) nor finish (the response is destroyed and may never complete) runs, and lease.release() is never called. Because the slot is transferred rather than freed on release, one aborted queued request permanently consumes one of the tenant's maxConcurrentRequests per replica, and a client that retries on abort drains the tenant's budget one leaked slot at a time until process restart — the exact failure the close listener comment at lines 131-135 claims to prevent.

---

🟡 Medium — Update the stale graphql-env merge snapshot · merge.test.ts.snap:84 · file not in diff

// graphql/env/__tests__/__snapshots__/merge.test.ts.snap
84	    "trustProxy": false,

The PR renamed server.trustProxy to server.trustedProxyHops in pgpmDefaults (pgpm/types/src/pgpm.ts:381) and updated pgpm/env's own snapshot, but graphql/env/__tests__/__snapshots__/merge.test.ts.snap:84 still expects "trustProxy": false in the server block. Since @constructive-io/graphql-env re-merges the @pgpmjs/env core options (graphql/env/src/merge.ts:29), the merged result now contains trustedProxyHops: 0 instead, so the snapshot test fails CI and only this snapshot was missed in the rename.

🧹 Nitpicks (1) — 🟢 1 low

• 🟢 Silent removal of SERVER_TRUST_PROXY without deprecation signal (env.ts:127) — The PR replaces SERVER_TRUST_PROXY (boolean proxy trust) with TRUSTED_PROXY_HOPS and drops the old env var and the server.trustProxy option with no alias or deprecation warning (pgpm/env/src/env.ts:127).

---

The change introduces a new client-IP middleware in packages/express-context that derives the client address by walking X-Forwarded-For right-to-left for a configurable number of trusted proxy hops, and rewires the GraphQL server's admission control to use that IP as the rate-limit key instead of the previous req.ip attribution.

Files	Change
packages/express-context/src/client-ip.ts, admission.ts, index.ts	New clientIpOf/clientIpFrom helpers and clientIpMiddleware, plus updated admission options
graphql/server/src/server.ts, middleware/admission-control.ts, refusals/recorder.ts	Mounts the middleware, sets Express trust proxy from hops, and keys the limiter on the resolved client IP
pgpm/env/src/env.ts, pgpm/types/src/pgpm.ts	Replaces SERVER_TRUST_PROXY with the parsed TRUSTED_PROXY_HOPS env option
packages/express-context/src/pg-settings.ts	Forwards the client IP into pgSettings as jwt.claims.ip_address
graphql/server-test/*, packages/express-context/__tests__/client-ip.test.ts	New unit and integration tests plus a client-ip seed fixture
graphql/env/*	Snapshot updates for the new default

One high-severity issue (concurrency-slot leak on aborted queued requests) and a stale graphql-env snapshot need attention before merge.

_{Reviewed commit: d225a53}