fix: secure authentication request identity

Replace Better Auth's default trust of a caller-controlled X-Forwarded-For with a platform-owned client-address resolution boundary: direct mode (the safe default) trusts only the real socket peer, and an explicit trusted-proxy mode requires a header name, a concrete trusted-proxy list, and only honors a forwarded chain from an actual trusted peer. This closes a rate-limit bypass where rotating a spoofed header let a caller choose a fresh bucket on every request. Public sign-up is also disabled by default and requires an explicit typed opt-in, using Better Auth's own disableSignUp gate so no email-existence leak or second signup path is introduced. Co-Authored-By: Claude Sonnet 5 <[email protected]>
2026-07-26 12:20:51 -05:00
parent 5e09330001
commit cafd513019
22 changed files with 1772 additions and 38 deletions
@@ -18,6 +18,7 @@ import { mkdir, mkdtemp, rm, writeFile } from "node:fs/promises";
 import { tmpdir } from "node:os";
 import { Client, type QueryResultRow } from "pg";
 import { afterAll, describe, expect, it, vi } from "vitest";
+import type { ClientAddressConfigInput } from "../src/client-address.js";
 import { createApiRuntimeConfig } from "../src/config.js";
 import { startApi, type RunningApi } from "../src/server.js";

@@ -325,16 +326,30 @@ function apiOrigin(running: RunningApi): string {
  return `http://127.0.0.1:${address.port}`;
 }

-async function startMigratedApi(
+interface StartConfiguredApiOptions {
+  readonly clientAddress?: ClientAddressConfigInput;
+  readonly modulesDirectoryOverride?: string;
+  /** Defaults to `true`: almost every existing scenario in this suite signs accounts up over HTTP
+   * and needs it enabled. Pass `false` explicitly for the dedicated signup-gating scenarios. */
+  readonly publicSignupEnabled?: boolean;
+  /** Defaults to a generous override so this suite's own request volume never trips Better Auth's
+   * strict built-in sign-in/sign-up rule. The dedicated rate-limit/client-address scenarios pass
+   * `undefined` explicitly to exercise the real, unmodified production rule instead. */
+  readonly rateLimitOverride?: { readonly max: number; readonly window: number } | undefined;
+}
+
+async function startConfiguredApi(
  url: string,
-  modulesDirectoryOverride?: string,
+  options: StartConfiguredApiOptions = {},
 ): Promise<RunningApi> {
  const config = createApiRuntimeConfig(
    {
+      ...(options.clientAddress === undefined ? {} : { clientAddress: options.clientAddress }),
      databaseUrl: url,
      host: "127.0.0.1",
      identity: {
        baseUrl: identityBaseUrl,
+        publicSignupEnabled: options.publicSignupEnabled ?? true,
        secret: "integration-test-secret-0123456789abcdefgh",
        trustedOrigins: [identityBaseUrl, dashboardOrigin],
      },
@@ -342,13 +357,26 @@ async function startMigratedApi(
    },
    { environment: "production" },
  );
+  return startApi(config, {
+    ...("rateLimitOverride" in options
+      ? { ...(options.rateLimitOverride ? { rateLimitOverride: options.rateLimitOverride } : {}) }
+      : { rateLimitOverride: { max: 1000, window: 1 } }),
+    ...(options.modulesDirectoryOverride === undefined
+      ? {}
+      : { modulesDirectoryOverride: options.modulesDirectoryOverride }),
+  });
+}
+
+async function startMigratedApi(
+  url: string,
+  modulesDirectoryOverride?: string,
+): Promise<RunningApi> {
  // Narrow, explicit test-only overrides: production composition (server.ts's `main`) never sets
  // either. Better Auth's built-in sign-in/sign-up rate limit (3 requests / 10s) would otherwise
  // reject this suite's many sequential requests; `modulesDirectoryOverride` lets a scenario point
  // permission composition at a real, disposable module fixture instead of this repository's own
  // (empty) `modules/` directory.
-  return startApi(config, {
-    rateLimitOverride: { max: 1000, window: 1 },
+  return startConfiguredApi(url, {
    ...(modulesDirectoryOverride === undefined ? {} : { modulesDirectoryOverride }),
  });
 }
@@ -456,6 +484,69 @@ function rawPostRequest(
  });
 }

+interface RawSignInAttemptOptions {
+  /** Originates the outbound connection from this local address — real, distinct loopback
+   * addresses (127.0.0.2, 127.0.0.3, ...) are all independently routable on the loopback interface,
+   * so this proves genuinely different real socket peers, not merely different header claims. */
+  readonly localAddress?: string;
+  /** A raw, unvalidated `X-Forwarded-For` header value to send — deliberately not parsed or
+   * validated here, since these tests are proving how the *server* handles it. */
+  readonly forwardedFor?: string;
+}
+
+interface RawResponse {
+  readonly body: string;
+  readonly headers: Record<string, string | string[] | undefined>;
+  readonly status: number;
+}
+
+/**
+ * A raw `node:http` sign-in attempt against a deliberately wrong password. Better Auth's rate
+ * limiter keys and counts every `/sign-in*` request before ever validating credentials, so this
+ * never needs a real account and always resolves to either `401` (accepted by the limiter, wrong
+ * credentials) or `429` (rejected by the limiter) — the only two outcomes these tests distinguish.
+ * Raw `node:http`, not `fetch()`, for the same Fetch-Metadata reason `rawPostRequest` documents.
+ */
+function rawSignInAttempt(
+  origin: string,
+  options: RawSignInAttemptOptions = {},
+): Promise<RawResponse> {
+  return new Promise((resolve, reject) => {
+    const url = new URL("/api/auth/sign-in/email", origin);
+    const payload = JSON.stringify({
+      email: "[email protected]",
+      password: "definitely-the-wrong-password",
+    });
+    const req = httpRequest(
+      url,
+      {
+        headers: {
+          "content-length": Buffer.byteLength(payload),
+          "content-type": "application/json",
+          ...(options.forwardedFor === undefined
+            ? {}
+            : { "x-forwarded-for": options.forwardedFor }),
+        },
+        ...(options.localAddress === undefined ? {} : { localAddress: options.localAddress }),
+        method: "POST",
+      },
+      (res) => {
+        const chunks: Buffer[] = [];
+        res.on("data", (chunk) => chunks.push(chunk as Buffer));
+        res.on("end", () => {
+          resolve({
+            body: Buffer.concat(chunks).toString("utf8"),
+            headers: res.headers,
+            status: res.statusCode ?? 0,
+          });
+        });
+      },
+    );
+    req.on("error", reject);
+    req.end(payload);
+  });
+}
+
 describe.sequential("live API — signup, sign-in, principal, sign-out", () => {
  let database: { name: string; url: string };
  let running: RunningApi;
@@ -798,6 +889,7 @@ describe.sequential("live API — startup and migration guarantees", () => {
          host: "127.0.0.1",
          identity: {
            baseUrl: identityBaseUrl,
+            publicSignupEnabled: true,
            secret: "integration-test-secret-0123456789abcdefgh",
            trustedOrigins: [identityBaseUrl],
          },
@@ -1528,3 +1620,312 @@ describe.sequential("live API — installed module permission end-to-end", () =>
    await dropDatabase(database.name);
  });
 });
+
+describe.sequential("live API — public signup is disabled by default", () => {
+  it("rejects sign-up with a stable sanitized 400 when publicSignupEnabled is not set, creating no user row", async () => {
+    const database = await createDisposableDatabase("signup_disabled");
+    await applyCoreMigrations(database.url);
+    const running = await startConfiguredApi(database.url, { publicSignupEnabled: false });
+    try {
+      const origin = apiOrigin(running);
+      const response = await fetch(`${origin}/api/auth/sign-up/email`, {
+        body: JSON.stringify({
+          email: "[email protected]",
+          name: "Blocked",
+          password: "correct horse battery staple",
+        }),
+        headers: { "content-type": "application/json", origin: dashboardOrigin },
+        method: "POST",
+      });
+      const text = await response.text();
+
+      expect(response.status).toBe(400);
+      expect(response.headers.get("cache-control")).toBe("no-store");
+      expect(text.toLowerCase()).not.toContain("stack");
+      expect(text).not.toContain(database.url);
+
+      const admin = await openAdministrativeClient(database.url);
+      try {
+        const rows = await safeQuery<{ count: string }>(
+          admin,
+          'select count(*)::text as count from "ekklesia_core_user"',
+        );
+        expect(rows[0]?.count).toBe("0");
+      } finally {
+        await safeEnd(admin);
+      }
+    } finally {
+      await running.shutdown().catch(() => undefined);
+      await dropDatabase(database.name);
+    }
+  });
+
+  it("gives an identical rejection whether or not the requested email is already registered", async () => {
+    const database = await createDisposableDatabase("signup_disabled_enum");
+    await applyCoreMigrations(database.url);
+    const existingEmail = "[email protected]";
+
+    // Briefly enable signup on this same database to seed one real, pre-existing account, matching
+    // how a production database might have been seeded before signup was later disabled.
+    const seedRunning = await startConfiguredApi(database.url, { publicSignupEnabled: true });
+    try {
+      const seedSignUp = await signUp(
+        apiOrigin(seedRunning),
+        existingEmail,
+        "correct horse battery",
+      );
+      expect(seedSignUp.response.status).toBe(200);
+    } finally {
+      await seedRunning.shutdown().catch(() => undefined);
+    }
+
+    const running = await startConfiguredApi(database.url, { publicSignupEnabled: false });
+    try {
+      const origin = apiOrigin(running);
+      const signUpAttempt = (email: string) =>
+        fetch(`${origin}/api/auth/sign-up/email`, {
+          body: JSON.stringify({ email, name: "X", password: "correct horse battery staple" }),
+          headers: { "content-type": "application/json", origin: dashboardOrigin },
+          method: "POST",
+        });
+
+      const existingResponse = await signUpAttempt(existingEmail);
+      const newResponse = await signUpAttempt("[email protected]");
+
+      expect(existingResponse.status).toBe(400);
+      expect(newResponse.status).toBe(400);
+      expect(existingResponse.status).toBe(newResponse.status);
+      expect(await existingResponse.text()).toBe(await newResponse.text());
+    } finally {
+      await running.shutdown().catch(() => undefined);
+      await dropDatabase(database.name);
+    }
+  });
+
+  it("keeps sign-in and existing-session behavior fully functional for a pre-existing account while signup stays disabled", async () => {
+    const database = await createDisposableDatabase("signup_disabled_signin");
+    await applyCoreMigrations(database.url);
+    const email = "[email protected]";
+    const password = "correct horse battery staple";
+
+    const seedRunning = await startConfiguredApi(database.url, { publicSignupEnabled: true });
+    try {
+      const seedSignUp = await signUp(apiOrigin(seedRunning), email, password);
+      expect(seedSignUp.response.status).toBe(200);
+    } finally {
+      await seedRunning.shutdown().catch(() => undefined);
+    }
+
+    const running = await startConfiguredApi(database.url, { publicSignupEnabled: false });
+    try {
+      const origin = apiOrigin(running);
+      const signInResult = await signIn(origin, email, password);
+      expect(signInResult.response.status).toBe(200);
+      expect(signInResult.cookiePair).toBeDefined();
+
+      const { body } = await fetchPrincipal(origin, { cookie: signInResult.cookiePair as string });
+      expect(body).toMatchObject({ kind: "authenticated" });
+
+      const blockedSignUp = await fetch(`${origin}/api/auth/sign-up/email`, {
+        body: JSON.stringify({ email: "[email protected]", name: "X", password }),
+        headers: { "content-type": "application/json", origin: dashboardOrigin },
+        method: "POST",
+      });
+      expect(blockedSignUp.status).toBe(400);
+    } finally {
+      await running.shutdown().catch(() => undefined);
+      await dropDatabase(database.name);
+    }
+  });
+});
+
+describe.sequential("live API — client-address resolution and auth rate-limit identity", () => {
+  let database: { name: string; url: string };
+  let directModeApi: RunningApi;
+  let directOrigin: string;
+
+  it("starts a direct-mode API (the safe default) against a freshly migrated database, with Better Auth's real unmodified rate-limit rule", async () => {
+    database = await createDisposableDatabase("client_address");
+    await applyCoreMigrations(database.url);
+    directModeApi = await startConfiguredApi(database.url, { rateLimitOverride: undefined });
+    directOrigin = apiOrigin(directModeApi);
+    expect(directModeApi.server.listening).toBe(true);
+  });
+
+  it("reproduces the original vulnerability's exact shape and proves it is closed: rotating spoofed X-Forwarded-For values never bypass the limiter in direct mode", async () => {
+    // Before this pass, Better Auth trusted a caller-controlled X-Forwarded-For directly: each
+    // distinct spoofed value looked like an unrelated client, so a caller could send an unlimited
+    // number of sign-in attempts merely by rotating the header on every request. Direct mode now
+    // resolves identity from the real socket peer only — the header is never consulted.
+    const peer = "127.0.0.21";
+    const first = await rawSignInAttempt(directOrigin, {
+      forwardedFor: "1.1.1.1",
+      localAddress: peer,
+    });
+    const second = await rawSignInAttempt(directOrigin, {
+      forwardedFor: "2.2.2.2",
+      localAddress: peer,
+    });
+    const third = await rawSignInAttempt(directOrigin, {
+      forwardedFor: "3.3.3.3",
+      localAddress: peer,
+    });
+    expect([first.status, second.status, third.status]).toEqual([401, 401, 401]);
+
+    // The fabricated value on this fourth request is new *again* — under the pre-fix behavior this
+    // would still look like a brand-new, never-before-seen client and would succeed. It must not.
+    const fourth = await rawSignInAttempt(directOrigin, {
+      forwardedFor: "4.4.4.4",
+      localAddress: peer,
+    });
+    expect(fourth.status).toBe(429);
+    // The limiter's own existing response semantics are preserved unmodified.
+    expect(fourth.headers["x-retry-after"]).toBeDefined();
+    expect(JSON.parse(fourth.body)).toMatchObject({ message: expect.any(String) });
+
+    // A request from the identical real peer with *no* forwarded header at all is equally blocked
+    // — proving the header was never what determined the bucket, in either direction.
+    const fifth = await rawSignInAttempt(directOrigin, { localAddress: peer });
+    expect(fifth.status).toBe(429);
+  });
+
+  it("gives distinct real peer addresses independent rate-limit buckets", async () => {
+    const peerA = "127.0.0.22";
+    const peerB = "127.0.0.23";
+    expect((await rawSignInAttempt(directOrigin, { localAddress: peerA })).status).toBe(401);
+    expect((await rawSignInAttempt(directOrigin, { localAddress: peerA })).status).toBe(401);
+    expect((await rawSignInAttempt(directOrigin, { localAddress: peerA })).status).toBe(401);
+    expect((await rawSignInAttempt(directOrigin, { localAddress: peerA })).status).toBe(429);
+
+    // An unrelated real peer's budget is completely untouched by peerA's exhaustion.
+    expect((await rawSignInAttempt(directOrigin, { localAddress: peerB })).status).toBe(401);
+  });
+
+  it("shuts down the direct-mode API", async () => {
+    await directModeApi.shutdown();
+  });
+
+  let proxyModeApi: RunningApi;
+  let proxyOrigin: string;
+  const trustedProxyPeer = "127.0.0.24";
+
+  it("starts a second API in explicit trusted-proxy mode against the same migrated database", async () => {
+    proxyModeApi = await startConfiguredApi(database.url, {
+      clientAddress: {
+        header: "x-forwarded-for",
+        mode: "trusted-proxy",
+        trustedProxies: [trustedProxyPeer],
+      },
+      rateLimitOverride: undefined,
+    });
+    proxyOrigin = apiOrigin(proxyModeApi);
+  });
+
+  it("honors a forwarded address only when it comes from the explicitly trusted proxy peer", async () => {
+    expect(
+      (
+        await rawSignInAttempt(proxyOrigin, {
+          forwardedFor: "203.0.113.50",
+          localAddress: trustedProxyPeer,
+        })
+      ).status,
+    ).toBe(401);
+    expect(
+      (
+        await rawSignInAttempt(proxyOrigin, {
+          forwardedFor: "203.0.113.50",
+          localAddress: trustedProxyPeer,
+        })
+      ).status,
+    ).toBe(401);
+    expect(
+      (
+        await rawSignInAttempt(proxyOrigin, {
+          forwardedFor: "203.0.113.50",
+          localAddress: trustedProxyPeer,
+        })
+      ).status,
+    ).toBe(401);
+    expect(
+      (
+        await rawSignInAttempt(proxyOrigin, {
+          forwardedFor: "203.0.113.50",
+          localAddress: trustedProxyPeer,
+        })
+      ).status,
+    ).toBe(429);
+
+    // A *different* forwarded client from the same trusted proxy gets its own independent bucket.
+    expect(
+      (
+        await rawSignInAttempt(proxyOrigin, {
+          forwardedFor: "203.0.113.51",
+          localAddress: trustedProxyPeer,
+        })
+      ).status,
+    ).toBe(401);
+  });
+
+  it("ignores a forwarded address claimed by an untrusted peer, using that peer's own real address instead", async () => {
+    // 203.0.113.50 is already exhausted above when legitimately forwarded by the trusted proxy —
+    // an untrusted peer merely claiming to *be* that address must not inherit its exhausted bucket,
+    // and must not be able to choose an arbitrary bucket of its own by forging the header either.
+    const untrustedPeer = "127.0.0.25";
+    const response = await rawSignInAttempt(proxyOrigin, {
+      forwardedFor: "203.0.113.50",
+      localAddress: untrustedPeer,
+    });
+    expect(response.status).toBe(401);
+  });
+
+  it("walks a chained forwarded header right-to-left, skipping the trusted-proxy hop to find the genuine client", async () => {
+    const chained = `203.0.113.60, ${trustedProxyPeer}`;
+    expect(
+      (
+        await rawSignInAttempt(proxyOrigin, {
+          forwardedFor: chained,
+          localAddress: trustedProxyPeer,
+        })
+      ).status,
+    ).toBe(401);
+    expect(
+      (
+        await rawSignInAttempt(proxyOrigin, {
+          forwardedFor: chained,
+          localAddress: trustedProxyPeer,
+        })
+      ).status,
+    ).toBe(401);
+    expect(
+      (
+        await rawSignInAttempt(proxyOrigin, {
+          forwardedFor: chained,
+          localAddress: trustedProxyPeer,
+        })
+      ).status,
+    ).toBe(401);
+    expect(
+      (
+        await rawSignInAttempt(proxyOrigin, {
+          forwardedFor: chained,
+          localAddress: trustedProxyPeer,
+        })
+      ).status,
+    ).toBe(429);
+  });
+
+  it("fails safely with a sanitized 503 on a malformed forwarded chain from a trusted proxy, never falling back to a shared bucket", async () => {
+    const response = await rawSignInAttempt(proxyOrigin, {
+      forwardedFor: "not-an-ip",
+      localAddress: trustedProxyPeer,
+    });
+    expect(response.status).toBe(503);
+    expect(JSON.parse(response.body)).toEqual({ code: "CLIENT_ADDRESS_UNRESOLVED" });
+    expect(response.headers["cache-control"]).toBe("no-store");
+  });
+
+  it("shuts down the trusted-proxy-mode API and drops the shared database", async () => {
+    await proxyModeApi.shutdown();
+    await dropDatabase(database.name);
+  });
+});