Harden four things the audit turned up

**The login rate limiter could be sidestepped.** X-Forwarded-For is a list each
hop appends to, and nginx's $proxy_add_x_forwarded_for appends ours — so a
client sending "X-Forwarded-For: 1.2.3.4" arrives as "1.2.3.4, <their real
address>". Reading the leftmost entry, as we did, handed the caller a
rate-limit key they could change per request: unlimited password guessing
against a deployment that looks correctly configured. Read from the right
instead, skip hops that are themselves trusted proxies, and believe the header
only when the peer is one (loopback and the private ranges by default,
TRUSTED_PROXIES to be explicit).

**The upload cap was a suggestion.** It read content-length, which a chunked
request simply omits. Count the bytes through a stream, as the image proxy
already does.

**App password secrets were drawn with a modulo.** 256 is not a multiple of 33,
so the first 25 characters of the alphabet came up on 8 byte values and the
last 8 on only 7. Rejection sampling instead. The test weighs the whole tail of
the alphabet rather than single characters, because a 7/8 skew is invisible
per character against the noise — and it does fail when the bias is put back.

**Upstream headers were relayed wholesale.** Anything the mail server set —
cookies, auth challenges, CORS grants — landed on our origin, where it means
something else. Allowlist what is actually wanted.

server/src/account.ts

@@ -371,16 +371,25 @@ async function assertCurrentPassword(ctx: Ctx, current: string, otpCode?: string
   if (!res.ok) throw new UpstreamError(`Could not verify the current password (${res.status})`, 502);
 }
 
-/** A legacy app password a person can read off a screen and type. */
-function readableSecret(): string {
+/**
+ * A legacy app password a person can read off a screen and type.
+ *
+ * Drawn by rejection sampling. Plain `% alphabet.length` would favour the
+ * first 25 characters, because 256 is not a multiple of 33: each of those
+ * would come up on 8 byte values and the remaining 8 on only 7.
+ */
+export function readableSecret(): string {
   const alphabet = "abcdefghijkmnopqrstuvwxyz23456789"; // no l/1/0 lookalikes
-  const bytes = randomBytes(20);
-  let out = "";
-  for (let i = 0; i < 20; i++) {
-    if (i > 0 && i % 5 === 0) out += "-";
-    out += alphabet[bytes[i]! % alphabet.length];
+  const limit = 256 - (256 % alphabet.length);
+  const chars: string[] = [];
+  while (chars.length < 20) {
+    for (const b of randomBytes(32)) {
+      if (b >= limit) continue; // the tail that would skew the alphabet
+      chars.push(alphabet[b % alphabet.length]!);
+      if (chars.length === 20) break;
+    }
   }
-  return out;
+  return (chars.join("").match(/.{5}/g) ?? []).join("-");
 }
 
 export { MASKED };

server/src/app.ts

@@ -5,6 +5,7 @@ import { getConnInfo } from "@hono/node-server/conninfo";
 import { config } from "./config.js";
 import { SessionStore, type LiveSession } from "./sessions.js";
 import { RateLimiter } from "./ratelimit.js";
+import { resolveClientIp } from "./clientip.js";
 import {
   type AccountInfo,
   UpstreamError,
@@ -57,17 +58,13 @@ const HOP_BY_HOP = new Set([
 ]);
 
 export function clientIp(c: Context): string {
-  if (config.trustProxy) {
-    const xff = c.req.header("x-forwarded-for");
-    if (xff) return xff.split(",")[0]!.trim();
-    const realIp = c.req.header("x-real-ip");
-    if (realIp) return realIp.trim();
-  }
+  let peer = "unknown";
   try {
-    return getConnInfo(c).remote.address ?? "unknown";
+    peer = getConnInfo(c).remote.address ?? "unknown";
   } catch {
-    return "unknown";
+    /* no socket information available */
   }
+  return resolveClientIp(peer, { forwardedFor: c.req.header("x-forwarded-for"), realIp: c.req.header("x-real-ip") }, config);
 }
 
 function isSecureRequest(c: Context): boolean {
@@ -450,6 +447,9 @@ export function createApp(): Hono<Env> {
     const accountId = c.req.param("accountId");
     const len = Number(c.req.header("content-length") ?? "0");
     if (len > config.maxUploadBytes) return c.json({ error: "too_large" }, 413);
+    // content-length is absent on a chunked request, so the header alone is a
+    // suggestion; count the bytes as they go past.
+    const body = c.req.raw.body ? c.req.raw.body.pipeThrough(byteCap(config.maxUploadBytes)) : null;
     try {
       const upstream = await getUpstreamSession(session.id, session.authorization);
       const url = absoluteUpstream(expandTemplate(upstream.uploadUrl, { accountId }));
@@ -460,7 +460,7 @@ export function createApp(): Hono<Env> {
           "content-type": c.req.header("content-type") ?? "application/octet-stream",
           accept: "application/json",
         },
-        body: c.req.raw.body,
+        body,
         duplex: "half",
         signal: AbortSignal.timeout(Math.max(config.upstreamTimeout, 5 * 60_000)),
       });
@@ -550,6 +550,18 @@ export function createApp(): Hono<Env> {
   return app;
 }
 
+/** Fail a stream that runs past `max` bytes, whatever its headers claimed. */
+function byteCap(max: number): TransformStream<Uint8Array, Uint8Array> {
+  let total = 0;
+  return new TransformStream<Uint8Array, Uint8Array>({
+    transform(chunk, controller) {
+      total += chunk.byteLength;
+      if (total > max) controller.error(new Error("upload too large"));
+      else controller.enqueue(chunk);
+    },
+  });
+}
+
 async function readJson<T>(c: Context): Promise<T | null> {
   try {
     return (await c.req.json()) as T;
@@ -582,10 +594,17 @@ function sessionExtras(session: LiveSession, info: AccountInfo = { locale: null,
   };
 }
 
+/**
+ * Headers worth relaying from the mail server. An allowlist rather than a
+ * denylist: everything else it might set — cookies, auth challenges, CORS
+ * grants — would be landing on *our* origin, where it means something else.
+ */
+const PASSTHROUGH_HEADERS = new Set(["content-type", "content-disposition", "content-language", "etag", "last-modified", "retry-after"]);
+
 function passthrough(res: Response): Response {
   const headers = new Headers();
   res.headers.forEach((v, k) => {
-    if (!HOP_BY_HOP.has(k.toLowerCase())) headers.set(k, v);
+    if (PASSTHROUGH_HEADERS.has(k.toLowerCase())) headers.set(k, v);
   });
   if (!headers.has("content-type")) headers.set("content-type", "application/json");
   headers.set("Cache-Control", "no-store");

server/src/clientip.test.ts

@@ -0,0 +1,94 @@
+import { test } from "node:test";
+import assert from "node:assert/strict";
+import { inRange, isTrustedProxy, resolveClientIp } from "./clientip.js";
+
+/**
+ * The rate limiter keys on whatever this returns, so anything a client can
+ * choose is a way to sidestep it. nginx's `$proxy_add_x_forwarded_for`
+ * *appends*, so a client sending `X-Forwarded-For: 1.2.3.4` reaches us as
+ * "1.2.3.4, <their real address>" — reading the leftmost entry hands them a
+ * key they can change per request.
+ */
+
+const cfg = { trustProxy: true, trustedProxies: [] as string[] };
+const direct = { trustProxy: false, trustedProxies: [] as string[] };
+
+test("CIDR matching covers both families and single addresses", () => {
+  assert.equal(inRange("10.1.2.3", "10.0.0.0/8"), true);
+  assert.equal(inRange("11.1.2.3", "10.0.0.0/8"), false);
+  assert.equal(inRange("172.16.5.4", "172.16.0.0/12"), true);
+  assert.equal(inRange("172.32.5.4", "172.16.0.0/12"), false);
+  assert.equal(inRange("127.0.0.1", "127.0.0.1"), true, "a bare address is a /32");
+  assert.equal(inRange("::1", "::1/128"), true);
+  assert.equal(inRange("fd00::5", "fc00::/7"), true);
+  assert.equal(inRange("2001:db8::1", "fc00::/7"), false);
+  assert.equal(inRange("10.1.2.3", "not-a-range"), false);
+  assert.equal(inRange("10.1.2.3", "::1/128"), false, "families do not cross");
+});
+
+test("loopback and private peers are trusted by default", () => {
+  for (const p of ["127.0.0.1", "::1", "10.0.0.5", "172.17.0.1", "192.168.1.9", "fd00::2"]) {
+    assert.equal(isTrustedProxy(p, cfg), true, p);
+  }
+  for (const p of ["8.8.8.8", "2001:db8::1"]) {
+    assert.equal(isTrustedProxy(p, cfg), false, p);
+  }
+});
+
+test("the real client is taken from the right, not the left", () => {
+  // What nginx produces when the client sent a forged header of their own.
+  const ip = resolveClientIp("172.17.0.1", { forwardedFor: "1.2.3.4, 203.0.113.9" }, cfg);
+  assert.equal(ip, "203.0.113.9", "the entry our own proxy observed");
+});
+
+test("a forged chain cannot move the rate-limit key", () => {
+  const forged = ["9.9.9.9", "8.8.8.8, 7.7.7.7", "203.0.113.1, 203.0.113.2, 203.0.113.3"];
+  const seen = forged.map((f) => resolveClientIp("127.0.0.1", { forwardedFor: `${f}, 198.51.100.7` }, cfg));
+  assert.deepEqual(seen, ["198.51.100.7", "198.51.100.7", "198.51.100.7"], "always the same real client");
+});
+
+test("hops we run ourselves are skipped over", () => {
+  // client → our edge proxy → our app proxy → us
+  const ip = resolveClientIp("127.0.0.1", { forwardedFor: "198.51.100.7, 10.0.0.2, 10.0.0.3" }, cfg);
+  assert.equal(ip, "198.51.100.7");
+});
+
+test("a peer we do not run is believed only about itself", () => {
+  const ip = resolveClientIp("8.8.8.8", { forwardedFor: "1.2.3.4" }, cfg);
+  assert.equal(ip, "8.8.8.8", "an untrusted peer cannot name its own client");
+});
+
+test("forwarding headers are ignored entirely when the proxy is not trusted", () => {
+  assert.equal(resolveClientIp("203.0.113.5", { forwardedFor: "1.2.3.4", realIp: "5.6.7.8" }, direct), "203.0.113.5");
+});
+
+test("X-Real-IP is a fallback, never an override", () => {
+  assert.equal(resolveClientIp("127.0.0.1", { realIp: "198.51.100.7" }, cfg), "198.51.100.7");
+  assert.equal(
+    resolveClientIp("127.0.0.1", { forwardedFor: "198.51.100.7", realIp: "1.2.3.4" }, cfg),
+    "198.51.100.7",
+    "the chain wins where there is one",
+  );
+});
+
+test("junk in the chain is discarded rather than used as a key", () => {
+  assert.equal(resolveClientIp("127.0.0.1", { forwardedFor: "not-an-ip, 198.51.100.7" }, cfg), "198.51.100.7");
+  assert.equal(resolveClientIp("127.0.0.1", { forwardedFor: "not-an-ip" }, cfg), "127.0.0.1", "falls back to the peer");
+  assert.equal(resolveClientIp("127.0.0.1", { forwardedFor: "" }, cfg), "127.0.0.1");
+});
+
+test("bracketed and IPv4-mapped forms are normalised", () => {
+  assert.equal(resolveClientIp("::1", { forwardedFor: "[2001:db8::5]" }, cfg), "2001:db8::5");
+  assert.equal(resolveClientIp("::1", { forwardedFor: "::ffff:198.51.100.7" }, cfg), "198.51.100.7");
+});
+
+test("an explicit trusted list replaces the defaults", () => {
+  const only = { trustProxy: true, trustedProxies: ["203.0.113.0/24"] };
+  assert.equal(resolveClientIp("203.0.113.9", { forwardedFor: "198.51.100.7" }, only), "198.51.100.7");
+  // Loopback is no longer trusted once a list is given.
+  assert.equal(resolveClientIp("127.0.0.1", { forwardedFor: "198.51.100.7" }, only), "127.0.0.1");
+});
+
+test("a chain of nothing but our own proxies still yields an address", () => {
+  assert.equal(resolveClientIp("127.0.0.1", { forwardedFor: "10.0.0.2, 10.0.0.3" }, cfg), "10.0.0.2");
+});

server/src/clientip.ts

@@ -0,0 +1,99 @@
+import { isIP } from "node:net";
+
+/**
+ * Work out who is really talking to us, for rate limiting and session records.
+ *
+ * `X-Forwarded-For` is a list that each hop appends to, so the entry nearest
+ * the right is the one our own proxy observed and the entries to its left were
+ * supplied by whoever came before — including the client. nginx's
+ * `$proxy_add_x_forwarded_for` appends, so a client sending
+ * `X-Forwarded-For: 1.2.3.4` arrives as `1.2.3.4, <their real address>`:
+ * reading the leftmost entry hands an attacker a rate-limit key they can
+ * change at will. Read from the right instead, skipping hops we run ourselves,
+ * and only believe the header at all when the peer is a proxy we trust.
+ */
+
+/** Peers whose forwarding headers are believed when none are configured. */
+const DEFAULT_TRUSTED = ["127.0.0.0/8", "::1/128", "10.0.0.0/8", "172.16.0.0/12", "192.168.0.0/16", "fc00::/7"];
+
+export interface TrustConfig {
+  trustProxy: boolean;
+  /** CIDRs or bare addresses; empty means DEFAULT_TRUSTED. */
+  trustedProxies: string[];
+}
+
+function toBits(addr: string): { value: bigint; width: number } | null {
+  const v = isIP(addr);
+  if (v === 4) {
+    const parts = addr.split(".").map(Number);
+    if (parts.length !== 4 || parts.some((n) => !Number.isInteger(n) || n < 0 || n > 255)) return null;
+    return { value: parts.reduce((acc, n) => (acc << 8n) | BigInt(n), 0n), width: 32 };
+  }
+  if (v === 6) {
+    // Expand "::" and any embedded IPv4 tail into eight 16-bit groups.
+    let text = addr;
+    const tail = /:(\d+\.\d+\.\d+\.\d+)$/.exec(text);
+    if (tail) {
+      const b = tail[1]!.split(".").map(Number);
+      text = `${text.slice(0, tail.index)}:${((b[0]! << 8) | b[1]!).toString(16)}:${((b[2]! << 8) | b[3]!).toString(16)}`;
+    }
+    const [head, rest] = text.split("::");
+    const left = head ? head.split(":").filter(Boolean) : [];
+    const right = rest !== undefined ? (rest ? rest.split(":").filter(Boolean) : []) : null;
+    const groups = right === null ? left : [...left, ...Array<string>(8 - left.length - right.length).fill("0"), ...right];
+    if (groups.length !== 8) return null;
+    let value = 0n;
+    for (const g of groups) {
+      const n = parseInt(g, 16);
+      if (!Number.isInteger(n) || n < 0 || n > 0xffff) return null;
+      value = (value << 16n) | BigInt(n);
+    }
+    return { value, width: 128 };
+  }
+  return null;
+}
+
+/** Is `addr` inside `range`, which may be a CIDR or a single address? */
+export function inRange(addr: string, range: string): boolean {
+  const [net, bitsText] = range.trim().split("/");
+  const a = toBits(addr);
+  const n = toBits(net ?? "");
+  if (!a || !n || a.width !== n.width) return false;
+  const bits = bitsText === undefined ? n.width : Number(bitsText);
+  if (!Number.isInteger(bits) || bits < 0 || bits > n.width) return false;
+  if (bits === 0) return true;
+  const shift = BigInt(n.width - bits);
+  return a.value >> shift === n.value >> shift;
+}
+
+export function isTrustedProxy(addr: string, cfg: TrustConfig): boolean {
+  const ranges = cfg.trustedProxies.length ? cfg.trustedProxies : DEFAULT_TRUSTED;
+  return ranges.some((r) => inRange(addr, r));
+}
+
+export interface ForwardHeaders {
+  forwardedFor?: string;
+  realIp?: string;
+}
+
+/**
+ * The client address to attribute a request to. `peer` is the socket address,
+ * which is the only part nobody downstream can forge.
+ */
+export function resolveClientIp(peer: string, headers: ForwardHeaders, cfg: TrustConfig): string {
+  if (!cfg.trustProxy || !peer || peer === "unknown") return peer || "unknown";
+  // A peer we do not run is not allowed to tell us who its client is.
+  if (!isTrustedProxy(peer, cfg)) return peer;
+  const chain = (headers.forwardedFor ?? "")
+    .split(",")
+    .map((s) => s.trim().replace(/^\[|\]$/g, "").replace(/^::ffff:(?=\d+\.\d+\.\d+\.\d+$)/i, ""))
+    .filter((s) => isIP(s) !== 0);
+  // Rightmost first: the last hop we trust is ours, anything left of the first
+  // untrusted entry was written by someone we have no reason to believe.
+  for (let i = chain.length - 1; i >= 0; i--) {
+    if (!isTrustedProxy(chain[i]!, cfg)) return chain[i]!;
+  }
+  if (chain.length) return chain[0]!;
+  const real = headers.realIp?.trim();
+  return real && isIP(real) !== 0 ? real : peer;
+}

server/src/config.ts

@@ -65,6 +65,13 @@ export const config = {
   stalwartUrl,
   appSecret,
   trustProxy: bool("TRUST_PROXY", true),
+  /**
+   * Peers whose X-Forwarded-* headers are believed. Empty falls back to
+   * loopback and the private ranges, which covers the usual reverse proxy on
+   * the same host or Docker network. A peer outside this is attributed by its
+   * socket address whatever it claims.
+   */
+  trustedProxies: (process.env.TRUSTED_PROXIES ?? "").split(",").map((s) => s.trim()).filter(Boolean),
   /** "auto" = Secure when the request arrived over https; "1"/"0" to force. */
   secureCookies: (process.env.SECURE_COOKIES ?? "auto").toLowerCase(),
   sessionTtl: int("SESSION_TTL", 12 * 60 * 60),

server/src/sessions.test.ts

@@ -63,3 +63,37 @@ test("normalizes Stalwart account locales to BCP-47 tags", () => {
   assert.equal(normalizeLocale({ locale: "de_DE" }), null);
   assert.equal(normalizeLocale("../etc/passwd"), null);
 });
+
+test("generated app passwords are unbiased and long enough", async () => {
+  const { readableSecret } = await import("./account.js");
+  const alphabet = "abcdefghijkmnopqrstuvwxyz23456789";
+  const counts = new Map<string, number>();
+  let samples = 0;
+  for (let i = 0; i < 2000; i++) {
+    const secret = readableSecret();
+    assert.match(secret, /^[a-z2-9]{5}-[a-z2-9]{5}-[a-z2-9]{5}-[a-z2-9]{5}$/, secret);
+    for (const ch of secret.replace(/-/g, "")) {
+      counts.set(ch, (counts.get(ch) ?? 0) + 1);
+      samples++;
+    }
+  }
+  assert.equal(samples, 2000 * 20);
+
+  /*
+   * `% 33` over a byte maps 25 characters onto 8 values each and the last 8
+   * onto 7, so the digits — the tail of the alphabet — would come up about
+   * 7/8 as often as they should. Testing each character on its own cannot see
+   * a skew that size against the noise, so weigh the whole tail at once:
+   * uniform puts 8/33 of the draw there, the biased version 7/8 of that, and
+   * over 40,000 draws the two are more than four standard deviations apart.
+   */
+  const tail = alphabet.slice(25); // "23456789"
+  const tailSeen = [...tail].reduce((n, ch) => n + (counts.get(ch) ?? 0), 0);
+  const p = tail.length / alphabet.length;
+  const expected = samples * p;
+  const sigma = Math.sqrt(samples * p * (1 - p));
+  assert.ok(
+    Math.abs(tailSeen - expected) < 4 * sigma,
+    `digits appeared ${tailSeen} times, expected ~${Math.round(expected)} (sigma ${sigma.toFixed(1)}) - modulo bias?`,
+  );
+});