diff --git a/README.md b/README.md index 288bf51..3070e62 100644 --- a/README.md +++ b/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. diff --git a/backend/app.ts b/backend/app.ts index 0c8409c..c304880 100644 --- a/backend/app.ts +++ b/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 = {}) { @@ -45,18 +41,12 @@ export function createApp(overrides: Partial = {}) { 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 = {}) { 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 = {}) { }); 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 = {}) { } 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 = {}) { 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 = {}) { // ---------- 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 = {}) { 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 = {}) { 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; } diff --git a/backend/auth.ts b/backend/auth.ts index 6a837a4..8cf46b0 100644 --- a/backend/auth.ts +++ b/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(); - - 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); - } -} diff --git a/backend/config.ts b/backend/config.ts index 1e9b4f7..a835afd 100644 --- a/backend/config.ts +++ b/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'; diff --git a/e2e/auth.spec.ts b/e2e/auth.spec.ts index 576ddf5..c0d6d49 100644 --- a/e2e/auth.spec.ts +++ b/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'); diff --git a/e2e/fixtures.ts b/e2e/fixtures.ts index b782564..11f6c14 100644 --- a/e2e/fixtures.ts +++ b/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 { }; } -/** 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); diff --git a/tests/backend/app.test.ts b/tests/backend/app.test.ts index 694cdd3..fa721ed 100644 --- a/tests/backend/app.test.ts +++ b/tests/backend/app.test.ts @@ -20,8 +20,6 @@ async function start(overrides: Partial = {}) { 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)); diff --git a/tests/backend/auth.test.ts b/tests/backend/auth.test.ts index a2e78be..7cc9835 100644 --- a/tests/backend/auth.test.ts +++ b/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); - }); -});