From b66759a1ab2f807ef051a1aaa3cfdcb9965ca39f Mon Sep 17 00:00:00 2001 From: Danijel Martinek Date: Fri, 10 Jul 2026 17:28:56 +0200 Subject: [PATCH] fix(auth): derive clientIp server-side, drop it from sign-in input MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit clientIp was part of the public signInInputSchema, so any client could spoof its own rate-limit bucket or dodge IP throttling entirely (audit finding B2). The schema no longer carries it (strict parsing rejects it with BAD_REQUEST); instead the web-next tRPC fetch adapter derives it in createTrpcContext from x-forwarded-for (first hop) / x-real-ip — trust caveat documented — and the router threads ctx.clientIp to the controller as a second, server-only argument typed outside the input schema (SignInRequestContext). Co-Authored-By: Claude Fable 5 --- .../web-next/src/app/api/trpc/[trpc]/route.ts | 5 +- .../application/use-cases/sign-in.use-case.ts | 16 +++++- .../auth/src/integrations/api/router.test.ts | 41 +++++++++++++++- packages/auth/src/integrations/api/router.ts | 21 ++++++-- .../controllers/sign-in.controller.test.ts | 40 +++++++++++++++ .../controllers/sign-in.controller.ts | 17 +++++-- packages/core-shared/src/trpc/context.test.ts | 49 +++++++++++++++++++ packages/core-shared/src/trpc/context.ts | 32 +++++++++++- 8 files changed, 207 insertions(+), 14 deletions(-) create mode 100644 packages/core-shared/src/trpc/context.test.ts diff --git a/apps/web-next/src/app/api/trpc/[trpc]/route.ts b/apps/web-next/src/app/api/trpc/[trpc]/route.ts index 94971f1..09bc884 100644 --- a/apps/web-next/src/app/api/trpc/[trpc]/route.ts +++ b/apps/web-next/src/app/api/trpc/[trpc]/route.ts @@ -1,12 +1,15 @@ import { fetchRequestHandler } from "@trpc/server/adapters/fetch"; import { appRouter } from "@repo/core-api"; +import { createTrpcContext } from "@repo/core-shared/trpc/context"; const handler = async (req: Request) => { return fetchRequestHandler({ endpoint: "/api/trpc", req, router: appRouter, - createContext: () => ({}), + // Threads server-derived fields (clientIp from proxy headers — see the + // trust caveat in core-shared/trpc/context.ts) into every procedure (B2). + createContext: () => createTrpcContext(req), }); }; diff --git a/packages/auth/src/application/use-cases/sign-in.use-case.ts b/packages/auth/src/application/use-cases/sign-in.use-case.ts index bf8ef72..dadb7b1 100644 --- a/packages/auth/src/application/use-cases/sign-in.use-case.ts +++ b/packages/auth/src/application/use-cases/sign-in.use-case.ts @@ -11,15 +11,27 @@ import type { IUsersRepository } from "../repositories/users.repository.interfac import type { IAuthenticationService } from "../services/authentication.service.interface"; // ── Input ──────────────────────────────────────────────────────────────── +// `.strict()` + no clientIp field: a client submitting clientIp is rejected +// at the procedure boundary (audit finding B2). export const signInInputSchema = z .object({ username: z.string().min(3).max(31), password: z.string().min(6).max(255), - clientIp: z.string().optional(), }) .strict(); export type SignInInput = z.infer; +/** + * Server-derived per-request context, typed OUTSIDE the public input schema + * so it can never be client-supplied (audit finding B2). The tRPC adapter + * derives `clientIp` from trusted proxy headers and the controller threads + * it through; `undefined` means "no proxy header present" and falls into a + * shared bucket. + */ +export type SignInRequestContext = { + clientIp?: string; +}; + // ── Output ─────────────────────────────────────────────────────────────── export const signInOutputSchema = z.object({ session: sessionSchema, @@ -36,7 +48,7 @@ export const signInUseCase = authenticationService: IAuthenticationService, rateLimit: IRateLimit, ) => - async (input: SignInInput): Promise => { + async (input: SignInInput & SignInRequestContext): Promise => { const { allowed: ipAllowed } = await rateLimit.consume( "ip", `signIn:ip:${input.clientIp ?? ""}`, diff --git a/packages/auth/src/integrations/api/router.test.ts b/packages/auth/src/integrations/api/router.test.ts index ad9af3a..745f297 100644 --- a/packages/auth/src/integrations/api/router.test.ts +++ b/packages/auth/src/integrations/api/router.test.ts @@ -1,4 +1,4 @@ -import { describe, it, expect, beforeEach } from "vitest"; +import { describe, it, expect, beforeEach, vi } from "vitest"; import { TRPCError } from "@trpc/server"; import { authRouter } from "@/integrations/api/router"; @@ -27,6 +27,45 @@ describe("authRouter", () => { }); expect(result.name).toBe("session"); }); + + it("rejects a client-supplied clientIp at the procedure boundary (B2)", async () => { + const caller = authRouter.createCaller({}); + try { + await caller.signIn({ + username: "alice", + password: "password_alice", + clientIp: "6.6.6.6", + } as never); + throw new Error("expected throw"); + } catch (e) { + expect(e).toBeInstanceOf(TRPCError); + expect((e as TRPCError).code).toBe("BAD_REQUEST"); + } + }); + + it("threads ctx.clientIp (server-derived) into the controller (B2)", async () => { + const original = authContainer.get(AUTH_SYMBOLS.ISignInController); + authContainer.unbind(AUTH_SYMBOLS.ISignInController); + const spy = vi.fn(async () => ({ + name: "session", + value: "tok", + attributes: {}, + })); + authContainer.bind(AUTH_SYMBOLS.ISignInController).toConstantValue(spy); + try { + const caller = authRouter.createCaller({ clientIp: "203.0.113.7" }); + await caller.signIn({ username: "alice", password: "password_alice" }); + expect(spy).toHaveBeenCalledWith( + { username: "alice", password: "password_alice" }, + { clientIp: "203.0.113.7" }, + ); + } finally { + authContainer.unbind(AUTH_SYMBOLS.ISignInController); + authContainer + .bind(AUTH_SYMBOLS.ISignInController) + .toConstantValue(original); + } + }); }); describe("authRouter error mapping", () => { diff --git a/packages/auth/src/integrations/api/router.ts b/packages/auth/src/integrations/api/router.ts index 5ebf5e2..aca7d8e 100644 --- a/packages/auth/src/integrations/api/router.ts +++ b/packages/auth/src/integrations/api/router.ts @@ -14,18 +14,29 @@ import type { ISignOutController } from "../../interface-adapters/controllers/si import { authProcedure } from "./procedures"; export const authRouter = router({ - signIn: authProcedure.input(signInInputSchema).mutation(({ input }) => { - const ctrl = authContainer.get(AUTH_SYMBOLS.ISignInController); - return ctrl(input); + signIn: authProcedure.input(signInInputSchema).mutation(({ input, ctx }) => { + const ctrl = authContainer.get( + AUTH_SYMBOLS.ISignInController, + ); + // clientIp is derived server-side by the adapter's createContext (from + // trusted proxy headers) — never from the client payload; the strict + // input schema rejects a client-supplied clientIp outright (B2). Same + // ctx-cast pattern as the dsr router until the shared t is context-typed. + const { clientIp } = ctx as { clientIp?: string }; + return ctrl(input, { clientIp }); }), signUp: authProcedure.input(signUpInputSchema).mutation(({ input }) => { - const ctrl = authContainer.get(AUTH_SYMBOLS.ISignUpController); + const ctrl = authContainer.get( + AUTH_SYMBOLS.ISignUpController, + ); return ctrl(input); }), signOut: authProcedure.input(signOutInputSchema).mutation(({ input }) => { - const ctrl = authContainer.get(AUTH_SYMBOLS.ISignOutController); + const ctrl = authContainer.get( + AUTH_SYMBOLS.ISignOutController, + ); return ctrl(input); }), }); diff --git a/packages/auth/src/interface-adapters/controllers/sign-in.controller.test.ts b/packages/auth/src/interface-adapters/controllers/sign-in.controller.test.ts index f98c20f..0148805 100644 --- a/packages/auth/src/interface-adapters/controllers/sign-in.controller.test.ts +++ b/packages/auth/src/interface-adapters/controllers/sign-in.controller.test.ts @@ -6,6 +6,7 @@ import { MockAuthenticationService } from "@/infrastructure/services/authenticat import { InputParseError } from "@/entities/errors/common"; import { userFactory } from "@/__factories__/user.factory"; import { NoopRateLimit } from "@repo/core-shared/rate-limit"; +import { RecordingRateLimit } from "@repo/core-testing/rate-limit"; describe("signInController", () => { it("returns a cookie on successful sign-in", async () => { @@ -28,6 +29,45 @@ describe("signInController", () => { expect(result.value).toBeTruthy(); }); + it("threads the server-derived clientIp into the use case (B2)", async () => { + const users = new MockUsersRepository([]); + const auth = new MockAuthenticationService(users); + const rl = new RecordingRateLimit(); + await users.createUser( + userFactory.build({ + username: "alice", + passwordHash: "hashed_testpassword", + }), + ); + + const controller = signInController(signInUseCase(users, auth, rl)); + await controller( + { username: "alice", password: "testpassword" }, + { clientIp: "203.0.113.7" }, + ); + + expect(rl.consumeCalls[0]).toMatchObject({ + budgetName: "ip", + key: "signIn:ip:203.0.113.7", + }); + }); + + it("rejects clientIp inside the client payload (strict schema, B2)", async () => { + const users = new MockUsersRepository([]); + const auth = new MockAuthenticationService(users); + const controller = signInController( + signInUseCase(users, auth, new NoopRateLimit()), + ); + + await expect( + controller({ + username: "alice", + password: "testpassword", + clientIp: "6.6.6.6", + }), + ).rejects.toBeInstanceOf(InputParseError); + }); + it("throws InputParseError on invalid input", async () => { const users = new MockUsersRepository([]); const auth = new MockAuthenticationService(users); diff --git a/packages/auth/src/interface-adapters/controllers/sign-in.controller.ts b/packages/auth/src/interface-adapters/controllers/sign-in.controller.ts index 88f555e..e25fffa 100644 --- a/packages/auth/src/interface-adapters/controllers/sign-in.controller.ts +++ b/packages/auth/src/interface-adapters/controllers/sign-in.controller.ts @@ -3,6 +3,7 @@ import { signInInputSchema, type ISignInUseCase, type SignInOutput, + type SignInRequestContext, } from "../../application/use-cases/sign-in.use-case"; function presenter(value: SignInOutput) { @@ -13,11 +14,21 @@ export type ISignInController = ReturnType; export const signInController = (signInUseCase: ISignInUseCase) => - async (input: unknown): Promise> => { + async ( + input: unknown, + // Server-derived, never part of the client-facing input schema (B2): + // the tRPC adapter builds it from trusted proxy headers. + requestContext?: SignInRequestContext, + ): Promise> => { const parsed = signInInputSchema.safeParse(input); if (!parsed.success) { - throw new InputParseError("Invalid sign-in input", { cause: parsed.error }); + throw new InputParseError("Invalid sign-in input", { + cause: parsed.error, + }); } - const result = await signInUseCase(parsed.data); + const result = await signInUseCase({ + ...parsed.data, + clientIp: requestContext?.clientIp, + }); return presenter(result); }; diff --git a/packages/core-shared/src/trpc/context.test.ts b/packages/core-shared/src/trpc/context.test.ts new file mode 100644 index 0000000..57f37e6 --- /dev/null +++ b/packages/core-shared/src/trpc/context.test.ts @@ -0,0 +1,49 @@ +import { describe, it, expect } from "vitest"; +import { clientIpFromHeaders, createTrpcContext } from "@/trpc/context"; + +describe("clientIpFromHeaders", () => { + it("takes the first x-forwarded-for hop", () => { + const headers = new Headers({ + "x-forwarded-for": "203.0.113.7, 10.0.0.1, 10.0.0.2", + }); + expect(clientIpFromHeaders(headers)).toBe("203.0.113.7"); + }); + + it("trims whitespace around the first hop", () => { + const headers = new Headers({ + "x-forwarded-for": " 203.0.113.7 , 10.0.0.1", + }); + expect(clientIpFromHeaders(headers)).toBe("203.0.113.7"); + }); + + it("falls back to x-real-ip when x-forwarded-for is absent", () => { + const headers = new Headers({ "x-real-ip": "198.51.100.4" }); + expect(clientIpFromHeaders(headers)).toBe("198.51.100.4"); + }); + + it("returns undefined when neither header is present", () => { + expect(clientIpFromHeaders(new Headers())).toBeUndefined(); + }); + + it("returns undefined for empty header values", () => { + const headers = new Headers({ "x-forwarded-for": " ", "x-real-ip": "" }); + expect(clientIpFromHeaders(headers)).toBeUndefined(); + }); +}); + +describe("createTrpcContext", () => { + it("attaches the derived clientIp from the request", async () => { + const req = new Request("https://example.test/api/trpc", { + headers: { "x-forwarded-for": "203.0.113.7" }, + }); + await expect(createTrpcContext(req)).resolves.toEqual({ + clientIp: "203.0.113.7", + }); + }); + + it("yields an undefined clientIp without a request", async () => { + await expect(createTrpcContext()).resolves.toEqual({ + clientIp: undefined, + }); + }); +}); diff --git a/packages/core-shared/src/trpc/context.ts b/packages/core-shared/src/trpc/context.ts index d38a5cb..ff3a779 100644 --- a/packages/core-shared/src/trpc/context.ts +++ b/packages/core-shared/src/trpc/context.ts @@ -1,5 +1,33 @@ -export async function createTrpcContext() { - return {}; +/** + * Derive the client IP from reverse-proxy headers. + * + * TRUST CAVEAT (audit finding B2): `x-forwarded-for` and `x-real-ip` are + * ordinary request headers. They are only trustworthy when the app runs + * behind a proxy/load balancer that overwrites (or verifiably appends to) + * them on every request. Exposed directly to the internet, a client can + * spoof them; deployments that need a hard guarantee must read the socket + * address at their edge and strip inbound copies of these headers. + * + * We take the FIRST `x-forwarded-for` entry — the client as reported by the + * first (trusted) hop — falling back to `x-real-ip`. + */ +export function clientIpFromHeaders(headers: Headers): string | undefined { + const forwarded = headers.get("x-forwarded-for"); + const firstHop = forwarded?.split(",")[0]?.trim(); + if (firstHop) return firstHop; + const realIp = headers.get("x-real-ip")?.trim(); + return realIp || undefined; +} + +/** + * Build the per-request tRPC context. Pass the adapter's incoming fetch + * `Request` so server-derived fields (currently `clientIp`) are attached — + * procedures must never trust client-supplied equivalents (B2). + */ +export async function createTrpcContext(req?: Request) { + return { + clientIp: req ? clientIpFromHeaders(req.headers) : undefined, + }; } export type TrpcContext = Awaited>;