diff --git a/server/src/app.test.ts b/server/src/app.test.ts index 6699cde..393af76 100644 --- a/server/src/app.test.ts +++ b/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: "someone@example.com", 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: "someone-else@example.com", 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); +}); diff --git a/server/src/app.ts b/server/src/app.ts index 13db7f7..47808d5 100644 --- a/server/src/app.ts +++ b/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 { 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 { // 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 { 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); } }); diff --git a/server/src/ratelimit.test.ts b/server/src/ratelimit.test.ts new file mode 100644 index 0000000..ba663d1 --- /dev/null +++ b/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"); +}); diff --git a/server/src/ratelimit.ts b/server/src/ratelimit.ts index a419f81..96e2e70 100644 --- a/server/src/ratelimit.ts +++ b/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); }