[RateLimiter] Remove ratelimiter to stop service from being unavailable

README.md

@@ -32,10 +32,8 @@ and keep a running balance of extra/minus hours.
 | `SESSION_SECRET`    | random   | Signs login cookies; without it everyone is logged out on restart. `npm run -s gen-secret` |
 | `REGISTRATION_CODE` | —        | If set, sign-up requires this invite code                                                  |
 
-Everything else is fixed in `backend/config.ts`: at most 100 accounts, 5 sign-ups per
-hour per IP, 10 failed logins per 15 min per IP, 30-day sessions. Client IPs are taken
-from `X-Forwarded-For` only when the connection comes from a private network (the
-ingress proxy).
+Everything else is fixed in `backend/config.ts`: at most 100 accounts, 30-day sessions.
+The app does no rate limiting of its own; that is left to the ingress (Traefik).
 
 Session cookies are always `Secure`; browsers accept that on `http://localhost`, but
 anywhere else the app must be served over HTTPS.

backend/app.ts

@@ -1,12 +1,11 @@
 import express from 'express';
 import fs from 'node:fs';
 import { isData, type Data } from '../shared/types';
-import { createSessionToken, hashPassword, RateLimiter, verifyPassword, verifySessionToken } from './auth';
+import { createSessionToken, hashPassword, verifyPassword, verifySessionToken } from './auth';
 import * as config from './config';
 import { UserStore, type User } from './store';
 
 const COOKIE = 'workh_session';
-const HOUR_MS = 3_600_000;
 
 export const USERNAME_RULE = /^[a-z0-9][a-z0-9._-]{2,31}$/;
 export const MIN_PASSWORD = 12;
@@ -31,9 +30,6 @@ export interface AppOptions {
   sessionTtlMs: number;
   registrationCode: string;
   maxUsers: number;
-  registrationsPerIpPerHour: number;
-  loginMaxFailures: number;
-  loginWindowMs: number;
 }
 
 export function createApp(overrides: Partial<AppOptions> = {}) {
@@ -45,18 +41,12 @@ export function createApp(overrides: Partial<AppOptions> = {}) {
     sessionTtlMs: config.SESSION_TTL_MS,
     registrationCode: config.REGISTRATION_CODE,
     maxUsers: config.MAX_USERS,
-    registrationsPerIpPerHour: config.REGISTRATIONS_PER_IP_PER_HOUR,
-    loginMaxFailures: config.LOGIN_MAX_FAILURES,
-    loginWindowMs: config.LOGIN_WINDOW_MS,
     ...overrides,
   };
   const store = new UserStore(o.dataDir);
   const app = express();
   app.disable('x-powered-by');
-  app.set('trust proxy', config.TRUST_PROXY);
 
-  const loginLimiter = new RateLimiter(o.loginMaxFailures, o.loginWindowMs);
-  const signupLimiter = new RateLimiter(o.registrationsPerIpPerHour, HOUR_MS);
   // Logins for unknown usernames still run scrypt against this, so they take as long as real ones.
   const dummyHash = hashPassword('dummy password for timing');
 
@@ -116,16 +106,6 @@ export function createApp(overrides: Partial<AppOptions> = {}) {
     return { username: username.trim().toLowerCase(), password };
   };
 
-  const tooMany = (res: express.Response, limiter: RateLimiter, key: string, what: string) => {
-    const wait = limiter.retryAfter(key);
-    if (wait === 0) return false;
-    res
-      .set('Retry-After', String(wait))
-      .status(429)
-      .json({ error: `Too many ${what}. Try again later.` });
-    return true;
-  };
-
   // ---------- Registration ----------
 
   app.get('/api/registration', (_req, res) => {
@@ -133,9 +113,6 @@ export function createApp(overrides: Partial<AppOptions> = {}) {
   });
 
   app.post('/api/register', async (req, res) => {
-    const ip = req.ip ?? 'unknown';
-    if (tooMany(res, signupLimiter, ip, 'new accounts from your address')) return;
-
     const creds = credentials(req.body);
     if (!creds) {
       res.status(400).json({ error: 'username and password are required' });
@@ -153,7 +130,6 @@ export function createApp(overrides: Partial<AppOptions> = {}) {
     }
     const { inviteCode } = (req.body ?? {}) as { inviteCode?: unknown };
     if (o.registrationCode !== '' && inviteCode !== o.registrationCode) {
-      signupLimiter.hit(ip); // guessing invite codes counts against the sign-up limit
       res.status(403).json({ error: 'Invalid invite code' });
       return;
     }
@@ -182,7 +158,6 @@ export function createApp(overrides: Partial<AppOptions> = {}) {
       fs.renameSync(o.legacyDataFile, `${o.legacyDataFile}.migrated`);
       console.log(`WorkH: moved ${o.legacyDataFile} into the new account "${user.username}"`);
     }
-    signupLimiter.hit(ip);
     startSession(res, user);
     res.status(201).json({ username: user.username });
   });
@@ -190,9 +165,6 @@ export function createApp(overrides: Partial<AppOptions> = {}) {
   // ---------- Sessions ----------
 
   app.post('/api/login', async (req, res) => {
-    const ip = req.ip ?? 'unknown';
-    if (tooMany(res, loginLimiter, ip, 'failed logins')) return;
-
     const creds = credentials(req.body);
     if (!creds) {
       res.status(400).json({ error: 'username and password are required' });
@@ -201,11 +173,9 @@ export function createApp(overrides: Partial<AppOptions> = {}) {
     const user = store.findByName(creds.username);
     const ok = await verifyPassword(creds.password, user?.passwordHash ?? (await dummyHash));
     if (!user || !ok) {
-      loginLimiter.hit(ip);
       res.status(401).json({ error: 'Wrong username or password' });
       return;
     }
-    loginLimiter.reset(ip);
     startSession(res, user);
     res.json({ username: user.username });
   });
@@ -232,11 +202,8 @@ export function createApp(overrides: Partial<AppOptions> = {}) {
 
   app.delete('/api/account', requireAuth, async (req, res) => {
     const { user } = req as AuthedRequest;
-    const ip = req.ip ?? 'unknown';
-    if (tooMany(res, loginLimiter, ip, 'failed attempts')) return;
     const { password } = (req.body ?? {}) as { password?: unknown };
     if (typeof password !== 'string' || !(await verifyPassword(password, user.passwordHash))) {
-      loginLimiter.hit(ip);
       res.status(403).json({ error: 'Wrong password' });
       return;
     }

backend/auth.ts

@@ -71,46 +71,3 @@ export function verifySessionToken(token: string, secret: string, now = Date.now
     return null;
   }
 }
-
-// ---------- Rate limiting ----------
-
-/**
- * Counts events (failed logins, sign-ups) per key (client IP) in a sliding window. After
- * `maxHits` events the key is blocked until its oldest event leaves the window.
- */
-export class RateLimiter {
-  private hits = new Map<string, number[]>();
-
-  constructor(
-    private maxHits: number,
-    private windowMs: number,
-    private now: () => number = Date.now,
-  ) {}
-
-  private recent(key: string): number[] {
-    const cutoff = this.now() - this.windowMs;
-    const list = (this.hits.get(key) ?? []).filter((t) => t > cutoff);
-    if (list.length) this.hits.set(key, list);
-    else this.hits.delete(key);
-    return list;
-  }
-
-  /** Seconds until the key may try again; 0 if it is not blocked. */
-  retryAfter(key: string): number {
-    const list = this.recent(key);
-    if (list.length < this.maxHits) return 0;
-    return Math.max(1, Math.ceil((list[0] + this.windowMs - this.now()) / 1000));
-  }
-
-  hit(key: string): void {
-    this.hits.set(key, [...this.recent(key), this.now()]);
-    // Keep memory bounded if someone sprays many source IPs.
-    if (this.hits.size > 10_000) {
-      for (const k of this.hits.keys()) this.recent(k);
-    }
-  }
-
-  reset(key: string): void {
-    this.hits.delete(key);
-  }
-}

backend/config.ts

@@ -21,8 +21,3 @@ export const DIST_DIR = path.join(ROOT, 'dist');
 export const LEGACY_DATA_FILE = path.join(ROOT, 'data.json');
 export const SESSION_TTL_MS = 30 * 86_400_000;
 export const MAX_USERS = 100;
-export const REGISTRATIONS_PER_IP_PER_HOUR = 5;
-export const LOGIN_MAX_FAILURES = 10;
-export const LOGIN_WINDOW_MS = 15 * 60_000;
-/** Trust X-Forwarded-For only from private-network peers, i.e. the ingress proxy. */
-export const TRUST_PROXY = 'loopback, linklocal, uniquelocal';

e2e/auth.spec.ts

@@ -106,18 +106,6 @@ test.describe('log in / log out', () => {
     await expect(page.getByLabel('Password')).toHaveValue('');
   });
 
-  test('ten wrong passwords lock the client out, even with the right one', async ({ page, user }) => {
-    await page.request.post('/api/logout', { data: {} });
-    for (let i = 0; i < 10; i++) {
-      await page.request.post('/api/login', { data: { username: user.username, password: 'wrong guess' } });
-    }
-    await page.goto('/');
-    await page.getByLabel('Username').fill(user.username);
-    await page.getByLabel('Password').fill(user.password);
-    await page.getByRole('button', { name: 'Log in' }).click();
-    await expect(page.getByRole('alert')).toContainText('Too many failed logins');
-  });
-
   test('logging out right after typing still saves the change', async ({ page, user }) => {
     await page.goto('/');
     await dayInput(page, MONDAY).fill('7:15');

e2e/fixtures.ts

@@ -17,15 +17,6 @@ export function uniqueName(prefix = 'u'): string {
   return `${prefix}${process.pid.toString(36)}${(counter++).toString(36)}${Math.random().toString(36).slice(2, 7)}`;
 }
 
-/**
- * A made-up client address. The server trusts X-Forwarded-For from loopback (like it trusts
- * Traefik in the cluster), so each test is a separate client for the per-IP rate limits.
- */
-export function randomIp(): string {
-  const b = () => Math.floor(Math.random() * 254) + 1;
-  return `10.${b()}.${b()}.${b()}`;
-}
-
 // Accounts created by the running test; deleted afterwards so the servers never reach
 // their account limit (MAX_USERS), however often the suite runs.
 const created: { creds: Creds; baseURL: string }[] = [];
@@ -45,7 +36,7 @@ export async function registerViaApi(request: APIRequestContext, creds: Creds) {
 // test's trace recording.
 async function deleteCreated() {
   for (const { creds, baseURL } of created.splice(0)) {
-    const headers = { 'Content-Type': 'application/json', 'X-Forwarded-For': randomIp() };
+    const headers = { 'Content-Type': 'application/json' };
     const login = await fetch(`${baseURL}/api/login`, { method: 'POST', headers, body: JSON.stringify(creds) });
     if (!login.ok) continue; // already deleted by the test, or never finished signing up
     const cookie = login.headers.get('set-cookie')!.split(';')[0];
@@ -73,9 +64,9 @@ export function baseData(overrides: Partial<Data> = {}): Data {
   };
 }
 
-/** New browser context = a different device/person, with its own IP and frozen clock. */
+/** New browser context = a different device/person, with its own frozen clock. */
 export async function newClient(browser: Browser, baseURL = 'http://localhost:3100') {
-  const context = await browser.newContext({ baseURL, extraHTTPHeaders: { 'X-Forwarded-For': randomIp() } });
+  const context = await browser.newContext({ baseURL });
   const page = await context.newPage();
   await page.clock.setFixedTime(NOW);
   return { context, page };
@@ -106,9 +97,6 @@ export const test = base.extend<{ user: Creds; cleanup: void }>({
     },
     { auto: true },
   ],
-  extraHTTPHeaders: async ({}, use) => {
-    await use({ 'X-Forwarded-For': randomIp() });
-  },
   page: async ({ page }, use) => {
     await page.clock.setFixedTime(NOW);
     await use(page);

tests/backend/app.test.ts

@@ -20,8 +20,6 @@ async function start(overrides: Partial<AppOptions> = {}) {
     legacyDataFile: path.join(tmpDir, 'data.json'),
     distDir: path.join(tmpDir, 'dist'),
     sessionSecret: 's'.repeat(32),
-    loginMaxFailures: 3,
-    registrationsPerIpPerHour: 100,
     ...overrides,
   }).listen(0, '127.0.0.1');
   await new Promise((resolve) => server.once('listening', resolve));
@@ -172,15 +170,6 @@ describe('registration', () => {
     expect((await (await fetch(`${base}/api/registration`)).json()).open).toBe(false);
   });
 
-  it('limits sign-ups per IP address', async () => {
-    await restart({ registrationsPerIpPerHour: 2 });
-    await newUser('one');
-    await newUser('two');
-    const r = await register('three');
-    expect(r.status).toBe(429);
-    expect(Number(r.headers.get('retry-after'))).toBeGreaterThan(0);
-  });
-
   it('gives hours from the old single-user data.json to the first account only', async () => {
     fs.writeFileSync(path.join(tmpDir, 'data.json'), JSON.stringify(withEntry(123)));
     const first = await newUser('first');
@@ -217,23 +206,6 @@ describe('login', () => {
     expect((await fetch(`${base}/api/login`, json('POST', { username: 'aaro' }))).status).toBe(400);
   });
 
-  it('rate-limits repeated failures, even with the right password', async () => {
-    for (let i = 0; i < 3; i++) expect((await login('aaro', 'wrong password!')).status).toBe(401);
-    expect((await login('aaro')).status).toBe(429);
-  });
-
-  it('rate-limits per client IP from X-Forwarded-For set by a private-network proxy', async () => {
-    const asClient = (ip: string, password: string) =>
-      fetch(`${base}/api/login`, {
-        ...json('POST', { username: 'aaro', password }),
-        headers: { 'Content-Type': 'application/json', 'X-Forwarded-For': ip },
-      });
-    for (let i = 0; i < 3; i++) expect((await asClient('203.0.113.1', 'wrong password!')).status).toBe(401);
-    expect((await asClient('203.0.113.1', PASSWORD)).status).toBe(429);
-    // Another client behind the same proxy is not affected.
-    expect((await asClient('203.0.113.2', PASSWORD)).status).toBe(200);
-  });
-
   it('logs out by clearing the cookie', async () => {
     const cookie = cookieOf(await login('aaro'));
     const r = await fetch(`${base}/api/logout`, json('POST', {}, cookie));
@@ -356,12 +328,6 @@ describe('deleting an account', () => {
     expect((await getData(cookie)).status).toBe(200);
   });
 
-  it('rate-limits password guesses', async () => {
-    const cookie = await newUser();
-    for (let i = 0; i < 3; i++) expect((await deleteAccount(cookie, 'wrong password!')).status).toBe(403);
-    expect((await deleteAccount(cookie)).status).toBe(429);
-  });
-
   it('removes the account, its data and its sessions; the name becomes free', async () => {
     const cookie = await newUser('aaro');
     await putData(cookie, withEntry(450));

tests/backend/auth.test.ts

@@ -3,7 +3,6 @@ import {
   createSessionToken,
   hashPassword,
   isPasswordHash,
-  RateLimiter,
   verifyPassword,
   verifySessionToken,
 } from '../../backend/auth';
@@ -58,28 +57,3 @@ describe('session tokens', () => {
     for (const t of ['', 'abc', 'a.b.c', '.', 'e30.']) expect(verifySessionToken(t, SECRET, now)).toBeNull();
   });
 });
-
-describe('RateLimiter', () => {
-  it('blocks after too many hits and unblocks when the window passes', () => {
-    let t = 0;
-    const limiter = new RateLimiter(3, 60_000, () => t);
-    for (let i = 0; i < 3; i++) {
-      expect(limiter.retryAfter('ip')).toBe(0);
-      limiter.hit('ip');
-      t += 1000;
-    }
-    expect(limiter.retryAfter('ip')).toBe(57); // first hit at t=0 leaves the window at 60 s
-    expect(limiter.retryAfter('other-ip')).toBe(0);
-    t = 60_001;
-    expect(limiter.retryAfter('ip')).toBe(0);
-  });
-
-  it('resets', () => {
-    const limiter = new RateLimiter(2, 60_000, () => 0);
-    limiter.hit('ip');
-    limiter.hit('ip');
-    expect(limiter.retryAfter('ip')).toBeGreaterThan(0);
-    limiter.reset('ip');
-    expect(limiter.retryAfter('ip')).toBe(0);
-  });
-});