Merge pull request #240 from Coffey-Labs/fix/upstream-outage-lockout

Do not spend login attempts on an outage nobody caused

server/src/app.test.ts

@@ -107,3 +107,40 @@ test("only a PDF blob may be framed, and only by us", async () => {
   assert.equal(securityHeadersFor("image/png", true), "DENY");
   assert.equal(securityHeadersFor("text/html", true), "DENY");
 });
+
+/*
+ * #239: retrying through an outage must not lock somebody out of the recovery.
+ *
+ * STALWART_URL at the top of this file is 127.0.0.1:1 — nothing listens there,
+ * so every sign-in here is the outage case. Before the fix, the eleventh of
+ * these came back 429 and stayed 429 for fifteen minutes, outliving whatever
+ * had actually been wrong.
+ */
+test("an unreachable upstream does not spend login attempts", async () => {
+  const app = createApp();
+  const login = () =>
+    app.request("/api/auth/login", {
+      method: "POST",
+      headers: { "content-type": "application/json", "x-requested-with": "ihasmail" },
+      body: JSON.stringify({ username: "[email protected]", password: "hunter2" }),
+    });
+
+  // Comfortably past LOGIN_RATE_LIMIT, which defaults to 10.
+  for (let i = 0; i < 25; i++) {
+    const res = await login();
+    assert.notEqual(res.status, 429, `attempt ${i + 1} was rate limited`);
+    assert.ok(res.status === 502 || res.status === 504, `attempt ${i + 1} said ${res.status}`);
+  }
+});
+
+test("an unreachable upstream says it is not the password", async () => {
+  const app = createApp();
+  const res = await app.request("/api/auth/login", {
+    method: "POST",
+    headers: { "content-type": "application/json", "x-requested-with": "ihasmail" },
+    body: JSON.stringify({ username: "[email protected]", password: "hunter2" }),
+  });
+  const body = (await res.json()) as { error: string; message: string };
+  assert.notEqual(body.error, "invalid_credentials");
+  assert.match(body.message, /not a problem with your password/i);
+});

server/src/app.ts

@@ -37,6 +37,20 @@ type Env = { Variables: { session: LiveSession } };
 
 export const sessions: SessionBackend = new SessionStore(config.sessionFile);
 const loginLimiter = new RateLimiter(config.loginRateLimit, 15 * 60_000);
+/*
+ * The backstop that is never refunded.
+ *
+ * `loginLimiter` guards password guessing and gives its attempts back when the
+ * upstream never judged the password (#239) -- otherwise retrying through an
+ * outage locks somebody out until after it has ended. But "not counted" cannot
+ * mean "unlimited": each attempt still costs ihasmail an outbound connection
+ * that may sit there until `UPSTREAM_TIMEOUT`, so a flood during an outage is
+ * the one moment the endpoint is cheapest to abuse.
+ *
+ * Hence a second ceiling, per address, twenty times looser and refunded never.
+ * A person retrying an outage will not come near it; something hammering will.
+ */
+const loginFloodLimiter = new RateLimiter(config.loginRateLimit * 20, 15 * 60_000);
 /**
  * Credential changes verify the current password upstream, and Stalwart's
  * fail2ban counts those failures against the *caller's* IP — which for a proxy
@@ -149,10 +163,10 @@ function upstreamFailure(c: Context, err: unknown) {
   }
   const name = (err as Error)?.name ?? "";
   if (name === "TimeoutError" || name === "AbortError") {
-    return c.json({ error: "upstream_timeout", message: "The mail server did not respond in time" }, 504);
+    return c.json({ error: "upstream_timeout", message: "The mail server did not respond in time. This is not a problem with your password." }, 504);
   }
   console.error("[ihasmail] upstream failure:", err);
-  return c.json({ error: "upstream_error", message: "Could not reach the mail server" }, 502);
+  return c.json({ error: "upstream_error", message: "Could not reach the mail server. This is not a problem with your password." }, 502);
 }
 
 /**
@@ -196,7 +210,24 @@ export function createApp(basePath = config.basePath): Hono<Env> {
     if (!username || !password) return c.json({ error: "missing_credentials" }, 400);
     if (username.length > 320 || password.length > 1024) return c.json({ error: "bad_request" }, 400);
 
+    /*
+     * Three checks, answering different questions.
+     *
+     * `limitKey` is this username from this address, and `ip` is any username
+     * from it -- both guard guessing, and both are given back when the upstream
+     * never got as far as judging the password. Refunding only the first would
+     * not fix #239: ten retries through an outage would still spend the address
+     * budget, and behind one office NAT that budget belongs to the whole
+     * building.
+     *
+     * The flood ceiling is the one that is never refunded, and it is the reason
+     * the other two safely can be.
+     */
     const limitKey = `${ip}|${username.toLowerCase()}`;
+    if (!loginFloodLimiter.check(ip)) {
+      c.header("Retry-After", String(loginFloodLimiter.retryAfterSeconds(ip)));
+      return c.json({ error: "rate_limited", message: "Too many login attempts. Please wait and try again." }, 429);
+    }
     if (!loginLimiter.check(limitKey) || !loginLimiter.check(ip)) {
       c.header("Retry-After", String(loginLimiter.retryAfterSeconds(limitKey)));
       return c.json({ error: "rate_limited", message: "Too many login attempts. Please wait and try again." }, 429);
@@ -212,6 +243,10 @@ export function createApp(basePath = config.basePath): Hono<Env> {
       // locale and self-service credentials each fail in their own way with
       // nothing to connect them. The credentials were good, so say so.
       if (!hasStalwartRegistry(upstream)) {
+        // The credentials were accepted; only the server is too old. Not an
+        // attempt worth counting against them.
+        loginLimiter.refund(limitKey);
+        loginLimiter.refund(ip);
         return c.json(
           {
             error: "unsupported_server",
@@ -259,6 +294,16 @@ export function createApp(basePath = config.basePath): Hono<Env> {
           401,
         );
       }
+      /*
+       * A 401 is a judgement about the password and stays counted. Anything
+       * else -- refused, timed out, DNS, TLS -- is the upstream failing to
+       * answer, which says nothing about the credentials and must not spend
+       * somebody's attempts while they wait for it to come back (#239).
+       */
+      if (!(err instanceof UpstreamError && err.status === 401)) {
+        loginLimiter.refund(limitKey);
+        loginLimiter.refund(ip);
+      }
       return upstreamFailure(c, err);
     }
   });

server/src/ratelimit.test.ts

@@ -0,0 +1,67 @@
+import { test } from "node:test";
+import assert from "node:assert/strict";
+import { RateLimiter } from "./ratelimit.js";
+
+/**
+ * The limiter's job is to slow down password guessing. #239 is about the
+ * attempts it takes for outcomes that were never a guess: ihasmail runs apart
+ * from Stalwart, so an upstream that refuses a connection is ordinary, and
+ * retrying through one used to spend the window and lock somebody out until
+ * after the cause had gone.
+ */
+
+test("check allows up to the limit and then refuses", () => {
+  const rl = new RateLimiter(3, 60_000);
+  assert.equal(rl.check("k"), true);
+  assert.equal(rl.check("k"), true);
+  assert.equal(rl.check("k"), true);
+  assert.equal(rl.check("k"), false);
+});
+
+test("refund gives back exactly one attempt", () => {
+  const rl = new RateLimiter(2, 60_000);
+  rl.check("k");
+  rl.check("k");
+  assert.equal(rl.check("k"), false, "spent");
+  rl.refund("k");
+  assert.equal(rl.check("k"), true, "one back");
+  assert.equal(rl.check("k"), false, "and only one");
+});
+
+test("refunding every attempt leaves the key spending nothing", () => {
+  // The outage case: every try refunded, so a person retrying through it is
+  // not locked out when the server returns.
+  const rl = new RateLimiter(2, 60_000);
+  for (let i = 0; i < 20; i++) {
+    assert.equal(rl.check("k"), true, `attempt ${i} allowed`);
+    rl.refund("k");
+  }
+});
+
+test("a run of real failures still adds up around a refunded one", () => {
+  // Refund takes one attempt back, not the key's whole history -- an outage in
+  // the middle of somebody guessing must not clear what they spent before it.
+  const rl = new RateLimiter(3, 60_000);
+  rl.check("k");            // a wrong password
+  rl.check("k");            // another
+  rl.check("k"); rl.refund("k"); // an outage, given back
+  assert.equal(rl.check("k"), true, "third real attempt");
+  assert.equal(rl.check("k"), false, "and now spent");
+});
+
+test("refunding a key that never spent anything is harmless", () => {
+  const rl = new RateLimiter(1, 60_000);
+  rl.refund("never-seen");
+  assert.equal(rl.check("never-seen"), true);
+});
+
+test("reset clears the key, refund does not", () => {
+  const rl = new RateLimiter(2, 60_000);
+  rl.check("k");
+  rl.check("k");
+  rl.refund("k");
+  assert.equal(rl.check("k"), true);
+  assert.equal(rl.check("k"), false);
+  rl.reset("k");
+  assert.equal(rl.check("k"), true, "reset is the successful-sign-in case");
+});

server/src/ratelimit.ts

@@ -23,6 +23,28 @@ export class RateLimiter {
     return true;
   }
 
+  /**
+   * Give back the attempt `check` just took.
+   *
+   * For an outcome that says nothing about whether the credentials were right.
+   * ihasmail runs in its own container, usually on its own host, so an upstream
+   * that never answered is an ordinary Tuesday rather than an attack -- and the
+   * limiter exists to slow down password guessing, which a server that refused
+   * the connection has not told us anything about. Without this, retrying
+   * through a thirty-second outage spends the window and locks somebody out
+   * until well after the cause has gone (#239).
+   *
+   * Refunds one attempt rather than clearing the key, so a run of real failures
+   * with an outage in the middle still adds up.
+   */
+  refund(key: string): void {
+    const arr = this.hits.get(key);
+    if (!arr?.length) return;
+    arr.pop();
+    if (arr.length) this.hits.set(key, arr);
+    else this.hits.delete(key);
+  }
+
   reset(key: string): void {
     this.hits.delete(key);
   }