diff --git a/backend/src/app.ts b/backend/src/app.ts index 5f411d2..5fb7c7c 100644 --- a/backend/src/app.ts +++ b/backend/src/app.ts @@ -6,7 +6,6 @@ import Fastify, { type FastifyInstance } from 'fastify' import cookie from '@fastify/cookie' import { isProduction, isTest } from './lib/env.ts' -import { clientIp } from './lib/clientIp.ts' import { withTimeout } from './lib/timeout.ts' import corsPlugin from './plugins/cors.ts' import errorHandlerPlugin from './plugins/errorHandler.ts' @@ -65,7 +64,7 @@ export async function buildApp(): Promise { // Kept on so `request.protocol` and `request.hostname` reflect the // original request behind Render's proxy. NOT relied on for the client // address: X-Forwarded-For here begins with whatever the client sent - // (see /health/whoami), so anything security-relevant reads the address + // (measured in ADR 50), so anything security-relevant reads the address // through lib/clientIp.ts instead. trustProxy: isProduction, @@ -145,28 +144,6 @@ export async function buildApp(): Promise { } }) - /** - * GET /health/whoami — how the API sees the caller. - * - * Returns only the caller's OWN address and forwarding chain, the way - * ifconfig.me does. It exists because "which entry in X-Forwarded-For is - * the real client" depends on the proxy in front of the API, no two hosts - * agree, and the rate limiter and audit log key on the answer. The only - * honest way to configure trustProxy is to look at this from outside. - */ - app.get('/health/whoami', async (request) => { - return { - /** What the rate limiter and audit log will use. */ - ip: clientIp(request), - /** What Fastify's trustProxy reading gives. */ - fastifyIp: request.ip, - forwardedFor: request.headers['x-forwarded-for'] ?? null, - cfConnectingIp: request.headers['cf-connecting-ip'] ?? null, - trueClientIp: request.headers['true-client-ip'] ?? null, - xRealIp: request.headers['x-real-ip'] ?? null - } - }) - // Auth: login, refresh, logout, me. await app.register(authRoutes) diff --git a/backend/src/docs/openapi.ts b/backend/src/docs/openapi.ts index 4009a3b..82a3acd 100644 --- a/backend/src/docs/openapi.ts +++ b/backend/src/docs/openapi.ts @@ -215,9 +215,6 @@ export const paths: Record> = { '/health': { get: { tags: ['Health'], summary: 'Liveness — is the process up', responses: { 200: json({ type: 'object', properties: { status: { type: 'string' } } }) } } }, - '/health/whoami': { - get: { tags: ['Health'], summary: 'How the API sees the caller — its own IP and forwarding chain', description: 'For configuring trustProxy honestly: which header entry the proxy controls differs per host.', responses: { 200: json({ type: 'object' }) } } - }, '/health/ready': { get: { tags: ['Health'], summary: 'Readiness — can it reach PostgreSQL and Redis', responses: { 200: json({ type: 'object' }), 503: { description: 'A dependency is down' } } } }, diff --git a/backend/src/lib/clientIp.ts b/backend/src/lib/clientIp.ts index 373eed3..ee59388 100644 --- a/backend/src/lib/clientIp.ts +++ b/backend/src/lib/clientIp.ts @@ -10,7 +10,7 @@ // no `trustProxy` setting reads it safely. What the edge does control is // its own header: Cloudflare (which fronts Render) overwrites // cf-connecting-ip on every request, and the origin is not reachable except -// through it. /health/whoami shows all of this from the outside. +// through it. ADR 50 records how this was measured from the outside. // // So the address used for rate-limit buckets and audit rows comes from a // header NAMED IN CONFIGURATION, when the deployment says which one to @@ -18,6 +18,7 @@ // nginx). Getting this wrong in the permissive direction is how a client // picks its own rate-limit bucket; ADR 50 records that we did, once. +import { isIP } from 'node:net' import type { FastifyRequest } from 'fastify' import { env } from './env.ts' @@ -29,6 +30,56 @@ export function clientIp( const value = request.headers[trustedHeader.toLowerCase()] const first = Array.isArray(value) ? value[0] : value if (first) return first.split(',')[0]!.trim() + + // The deployment says which header to trust, and it is missing: the + // request did not come through the edge that sets it. Do NOT fall back + // to request.ip — with trustProxy on, that is read from X-Forwarded-For, + // which the client writes. The TCP peer cannot be forged; at worst every + // such request shares the proxy's bucket, which fails safe. + return request.socket?.remoteAddress ?? request.ip } return request.ip } + +/** + * The rate-limit bucket for an address. + * + * IPv4: the address itself. IPv6: its /64 network. An ordinary IPv6 + * subscriber is handed a whole /64 — 2^64 addresses — so keying on the full + * address gives an attacker a fresh bucket for every request. The /64 is the + * smallest block one customer controls, which makes it the unit to count. + * + * Only for rate-limit keys. Audit rows keep the full address, because an + * investigator wants exactly what was seen. + */ +export function rateLimitKey(ip: string): string { + // ::ffff:198.51.100.7 is an IPv4 client seen through an IPv6 socket. It + // must land in the same bucket as 198.51.100.7, not in a /64 shared with + // every other IPv4 client. + const mapped = /^::ffff:(\d{1,3}(?:\.\d{1,3}){3})$/i.exec(ip) + if (mapped) return mapped[1]! + + if (isIP(ip) !== 6) return ip + return `${ipv6Groups(ip).slice(0, 4).join(':')}::/64` +} + +/** The eight 16-bit groups of an IPv6 address, expanded and lower-case. */ +function ipv6Groups(ip: string): string[] { + // Drop a zone id (fe80::1%eth0) — it names a local interface, not a host. + const address = ip.split('%')[0]!.toLowerCase() + + const [head = '', tail] = address.split('::') + const headParts = head ? head.split(':') : [] + const tailParts = tail ? tail.split(':') : [] + + // An embedded IPv4 in the last position (64:ff9b::198.51.100.7) fills + // two groups. It can only sit at the end, so the /64 prefix is unaffected. + const width = (parts: string[]) => parts.reduce((n, p) => n + (p.includes('.') ? 2 : 1), 0) + const zeros = tail === undefined ? [] : Array(8 - width(headParts) - width(tailParts)).fill('0') + + // parseInt then toString: '0db8' and 'db8' are the same group and must + // produce the same key. + return [...headParts, ...zeros, ...tailParts].map((group) => + group.includes('.') ? group : parseInt(group, 16).toString(16) + ) +} diff --git a/backend/src/lib/concurrency.ts b/backend/src/lib/concurrency.ts new file mode 100644 index 0000000..343614b --- /dev/null +++ b/backend/src/lib/concurrency.ts @@ -0,0 +1,51 @@ +// concurrency.ts — a bounded gate for expensive work. +// +// Argon2id is slow and memory-hard on purpose (19 MiB and ~50 ms per hash on +// a laptop, far more on a shared free-tier CPU). That cost is the defence +// against offline cracking — and, unbounded, an invitation to online DoS: +// a burst of login attempts queues without limit and every other request +// waits behind it. The per-IP rate limit does not help against many IPs. +// +// So: at most `maxActive` run at once, at most `maxWaiting` queue behind +// them, and anything beyond that is turned away immediately. A fast "busy, +// retry in a moment" keeps the rest of the API responsive; a queue that +// grows forever does not. + +import { ServiceBusyError } from './errors.ts' + +export class BoundedConcurrency { + private active = 0 + private readonly waiting: Array<() => void> = [] + + constructor( + private readonly maxActive: number, + private readonly maxWaiting: number + ) {} + + async run(work: () => Promise): Promise { + if (this.active >= this.maxActive) { + if (this.waiting.length >= this.maxWaiting) { + throw new ServiceBusyError(1) + } + // Wait for a finishing task to hand its slot over (see finally below). + await new Promise((resolve) => this.waiting.push(resolve)) + } else { + this.active++ + } + + try { + return await work() + } finally { + // Hand the slot straight to the next waiter rather than releasing it: + // `active` stays the same, and no newcomer can jump the queue in between. + const next = this.waiting.shift() + if (next) next() + else this.active-- + } + } + + /** For tests and diagnostics. */ + get stats(): { active: number; waiting: number } { + return { active: this.active, waiting: this.waiting.length } + } +} diff --git a/backend/src/lib/env.ts b/backend/src/lib/env.ts index 5be11b7..1b692c3 100644 --- a/backend/src/lib/env.ts +++ b/backend/src/lib/env.ts @@ -40,7 +40,13 @@ const envSchema = z.object({ // App. coerce: "3000" arrives as a string, we want a number. PORT: z.coerce.number().int().positive().default(3000), - FRONTEND_URL: z.url().default('http://localhost:5173'), + // Trailing slashes stripped: the browser's Origin header never has one, + // so "https://x.app/" would fail the CORS and same-origin checks with an + // error that looks like anything but a typo. + FRONTEND_URL: z + .url() + .transform((url) => url.replace(/\/+$/, '')) + .default('http://localhost:5173'), NODE_ENV: z.enum(['development', 'test', 'production']).default('development'), // Week 3 — allowed to be empty for now. .optional() means the key need not @@ -163,8 +169,31 @@ const envSchemaWithProductionRules = envSchema.superRefine((value, ctx) => { }) } + // Demo mode publishes admin credentials at GET /demo and wipes every table + // nightly. With a live Stripe key, anonymous visitors could open real + // checkout sessions. Checked in every environment: the combination is + // wrong everywhere. (sk_ = secret key, rk_ = restricted key.) + if (value.DEMO_MODE && /^(sk|rk)_live_/.test(value.STRIPE_SECRET_KEY ?? '')) { + ctx.addIssue({ + code: 'custom', + path: ['STRIPE_SECRET_KEY'], + message: 'DEMO_MODE får aldrig köras med en live-nyckel från Stripe — använd sk_test_' + }) + } + if (value.NODE_ENV !== 'production') return + // In production the frontend is a real, public https origin. The default + // (localhost) or plain http here means the variable was forgotten, and + // the symptom — every browser request failing CORS — points elsewhere. + if (!value.FRONTEND_URL.startsWith('https://')) { + ctx.addIssue({ + code: 'custom', + path: ['FRONTEND_URL'], + message: 'FRONTEND_URL måste vara en https-adress i produktion (saknas den?)' + }) + } + // Production traffic crosses the internet: without TLS, the database // password and every query travel in clear text. Either the URL asks for // TLS (a host with a public certificate) or we verify against a CA file. diff --git a/backend/src/lib/errors.ts b/backend/src/lib/errors.ts index 9eb5115..b58d203 100644 --- a/backend/src/lib/errors.ts +++ b/backend/src/lib/errors.ts @@ -21,6 +21,7 @@ export type ErrorCode = | 'NOT_FOUND' | 'CONFLICT' | 'RATE_LIMITED' + | 'SERVICE_BUSY' | 'BUSINESS_RULE' | 'INTERNAL_ERROR' @@ -155,6 +156,26 @@ export class RateLimitError extends AppError { } } +// ───────────────────────────────────────────────────────────── +// 503 — we are busy, not you +// ───────────────────────────────────────────────────────────── + +/** + * The server is at capacity for some expensive operation (password hashing, + * see lib/concurrency.ts). Different from 429: the caller did nothing wrong, + * and the right response is a short wait, not a slowdown. + */ +export class ServiceBusyError extends AppError { + readonly retryAfterSeconds: number + + constructor(retryAfterSeconds: number) { + super('Tjänsten är tillfälligt hårt belastad. Försök igen om en stund.', 503, 'SERVICE_BUSY', { + details: { retryAfterSeconds } + }) + this.retryAfterSeconds = retryAfterSeconds + } +} + // ───────────────────────────────────────────────────────────── // 422 — the shape is valid, but a business rule says no // ───────────────────────────────────────────────────────────── diff --git a/backend/src/lib/mailer.ts b/backend/src/lib/mailer.ts index 5b531a1..2fd9485 100644 --- a/backend/src/lib/mailer.ts +++ b/backend/src/lib/mailer.ts @@ -12,7 +12,7 @@ // find out from a customer. import { Resend } from 'resend' -import { env, isTest } from './env.ts' +import { env, isProduction, isTest } from './env.ts' export type EmailMessage = { to: string @@ -70,6 +70,18 @@ async function sendViaResend(message: EmailMessage): Promise { } } +/** + * What the console transport may print. + * + * Locally the printed link IS the way to finish an invite, so it is shown in + * full. In production (the demo on Render runs this transport) the log is a + * third party's storage: a set-password token there is a standing key to the + * account for anyone with log access. Redact it. + */ +export function printableBody(text: string, production: boolean = isProduction): string { + return production ? text.replace(/token=[^\s&#]+/g, 'token=[REDACTED]') : text +} + function sendViaConsole(message: EmailMessage): SendResult { if (!isTest) { console.log( @@ -79,7 +91,7 @@ function sendViaConsole(message: EmailMessage): SendResult { `│ To: ${message.to}`, `│ Subject: ${message.subject}`, '├─────────────────────────────────────────────────────────────', - message.text + printableBody(message.text) .split('\n') .map((line) => `│ ${line}`) .join('\n'), diff --git a/backend/src/plugins/errorHandler.ts b/backend/src/plugins/errorHandler.ts index 2c5dd49..37f7e43 100644 --- a/backend/src/plugins/errorHandler.ts +++ b/backend/src/plugins/errorHandler.ts @@ -15,7 +15,7 @@ import fp from 'fastify-plugin' import type { FastifyInstance, FastifyReply, FastifyRequest } from 'fastify' import { ZodError } from 'zod' import { Prisma } from '../generated/prisma/client.ts' -import { AppError, RateLimitError, isAppError } from '../lib/errors.ts' +import { AppError, RateLimitError, ServiceBusyError, isAppError } from '../lib/errors.ts' import { isProduction } from '../lib/env.ts' import { captureException } from '../lib/sentry.ts' @@ -83,8 +83,8 @@ async function errorHandlerPlugin(app: FastifyInstance) { 'Domain error' ) - // A 429 must say WHEN the caller may try again. - if (error instanceof RateLimitError) { + // A 429 or 503 must say WHEN the caller may try again. + if (error instanceof RateLimitError || error instanceof ServiceBusyError) { reply.header('Retry-After', String(error.retryAfterSeconds)) } @@ -133,8 +133,13 @@ async function errorHandlerPlugin(app: FastifyInstance) { // ── 4. Fastify's own errors (body too large, malformed JSON, ...) ── // These already carry a sensible status code. A 4xx is the caller's // fault and its message is harmless; a 5xx we silence. + // + // Only FASTIFY's (code FST_...). Other libraries set statusCode too — a + // Stripe SDK error carries 400 and a message naming session ids and + // parameters. That is not the caller's mistake but ours, so it belongs + // in branch 5: logged, reported, and a generic 500 to the client. const fields = readErrorFields(error) - if (fields.statusCode >= 400 && fields.statusCode < 500) { + if (fields.statusCode >= 400 && fields.statusCode < 500 && fields.code.startsWith('FST_')) { app.log.warn({ err: error }, 'Client error') return send(reply, request, fields.statusCode, fields.code, fields.message) } diff --git a/backend/src/plugins/rateLimit.ts b/backend/src/plugins/rateLimit.ts index 5f8b6e4..d9b117c 100644 --- a/backend/src/plugins/rateLimit.ts +++ b/backend/src/plugins/rateLimit.ts @@ -14,7 +14,7 @@ // Layer 2 (services/loginAttempts.service.ts) covers that. import fp from 'fastify-plugin' -import { clientIp } from '../lib/clientIp.ts' +import { clientIp, rateLimitKey } from '../lib/clientIp.ts' import rateLimit from '@fastify/rate-limit' import type { FastifyInstance } from 'fastify' import { RateLimitError } from '../lib/errors.ts' @@ -34,10 +34,10 @@ async function rateLimitPlugin(app: FastifyInstance) { max: 100, timeWindow: '1 minute', - // The key is the client IP. request.ip only respects X-Forwarded-For - // when trustProxy is on — which we enable in production. Without it, - // everyone behind a proxy would share ONE counter and lock each other out. - keyGenerator: (request) => clientIp(request), + // The key is the client's address as the edge reports it (lib/clientIp), + // widened to its /64 for IPv6 — otherwise one IPv6 subscriber holds + // 2^64 separate buckets and the limit means nothing. + keyGenerator: (request) => rateLimitKey(clientIp(request)), // NOTE: the plugin THROWS whatever errorResponseBuilder returns. Return a // plain object and it has no statusCode, so our central error handler diff --git a/backend/src/services/password.service.ts b/backend/src/services/password.service.ts index 0f61b63..0fd2923 100644 --- a/backend/src/services/password.service.ts +++ b/backend/src/services/password.service.ts @@ -11,6 +11,7 @@ import type { Algorithm } from '@node-rs/argon2' import { randomBytes } from 'node:crypto' import { isPasswordBreached } from '../lib/hibp.ts' import { ValidationError } from '../lib/errors.ts' +import { BoundedConcurrency } from '../lib/concurrency.ts' // ───────────────────────────────────────────────────────────── // Parameters — OWASP 2024 recommendation for Argon2id @@ -103,8 +104,20 @@ function normalize(password: string): string { * * So verify() can read back exactly how this hash was produced. */ +// ───────────────────────────────────────────────────────────── +// Concurrency cap +// ───────────────────────────────────────────────────────────── +// +// Every hash and verify goes through one gate: at most 2 at once, 20 waiting, +// the rest refused with 503 + Retry-After (lib/concurrency.ts). The free +// instance has a fraction of a CPU; without this, a login burst from many +// IPs — which the per-IP limit cannot see — stalls every other request. +// The dummy verify for unknown emails goes through the same gate, so the +// timing stays identical and says nothing about which accounts exist. +const argon2Gate = new BoundedConcurrency(2, 20) + export async function hashPassword(plainPassword: string): Promise { - return argon2Hash(normalize(plainPassword), ARGON2_OPTIONS) + return argon2Gate.run(() => argon2Hash(normalize(plainPassword), ARGON2_OPTIONS)) } /** @@ -118,11 +131,16 @@ export async function verifyPassword( storedHash: string, plainPassword: string ): Promise { - try { - return await argon2Verify(storedHash, normalize(plainPassword)) - } catch { - return false - } + // The gate is OUTSIDE the try on purpose. Inside, a "busy" refusal would be + // swallowed as `false` — a wrong password — and counted as a failed login + // against the account. Busy must stay busy. + return argon2Gate.run(async () => { + try { + return await argon2Verify(storedHash, normalize(plainPassword)) + } catch { + return false + } + }) } /** diff --git a/backend/src/services/passwordToken.service.ts b/backend/src/services/passwordToken.service.ts index e976bfa..631a1a8 100644 --- a/backend/src/services/passwordToken.service.ts +++ b/backend/src/services/passwordToken.service.ts @@ -65,9 +65,16 @@ export async function issuePasswordToken( return { token, expiresAt } } -/** Builds the link that goes in the email. */ +/** + * Builds the link that goes in the email. + * + * The token rides in the FRAGMENT (#token=), not the query string. Browsers + * never send the fragment to any server: a ?token= reached Vercel's access + * logs with the page request, and sat in history and Referer headers. The + * page reads it from location.hash and wipes it from the address bar. + */ export function buildSetPasswordUrl(token: string): string { - return `${env.FRONTEND_URL}/set-password?token=${encodeURIComponent(token)}` + return `${env.FRONTEND_URL}/set-password#token=${encodeURIComponent(token)}` } /** diff --git a/backend/tests/integration/errors.test.ts b/backend/tests/integration/errors.test.ts index 48782a0..7a12d5c 100644 --- a/backend/tests/integration/errors.test.ts +++ b/backend/tests/integration/errors.test.ts @@ -11,6 +11,7 @@ import { ForbiddenError, NotFoundError, RateLimitError, + ServiceBusyError, BusinessRuleError } from '../../src/lib/errors.ts' import { loginSchema } from '../../src/validators/auth.validator.ts' @@ -37,6 +38,18 @@ beforeAll(async () => { instance.get('/_t/ratelimit', async () => { throw new RateLimitError(42) }) + instance.get('/_t/busy', async () => { + throw new ServiceBusyError(1) + }) + instance.get('/_t/library-4xx', async () => { + // The shape of a Stripe SDK error: a 4xx statusCode and a message + // that names our own internals. + throw Object.assign(new Error('No such checkout.session: cs_test_secret123'), { + statusCode: 400, + type: 'StripeInvalidRequestError' + }) + }) + instance.post('/_t/echo', async (request) => request.body) instance.get('/_t/business', async () => { throw new BusinessRuleError('En betald faktura kan inte krediteras') }) @@ -109,6 +122,36 @@ describe('rate limit errors', () => { }) }) +describe('service busy', () => { + test('503 with a Retry-After header', async () => { + const res = await get('/_t/busy') + expect(res.statusCode).toBe(503) + expect(res.json().error.code).toBe('SERVICE_BUSY') + expect(res.headers['retry-after']).toBe('1') + }) +}) + +describe('4xx from somewhere other than Fastify', () => { + test('🔑 a library error with a 4xx statusCode is our bug: 500, not forwarded as 400', async () => { + const res = await get('/_t/library-4xx') + expect(res.statusCode).toBe(500) + expect(res.json().error.code).toBe('INTERNAL_ERROR') + // (In production the message is generic too; development shows it on + // purpose — see branch 5 of the handler.) + }) + + test("Fastify's own 4xx still reaches the caller — malformed JSON is their mistake", async () => { + const res = await app.inject({ + method: 'POST', + url: '/_t/echo', + headers: { 'content-type': 'application/json' }, + payload: '{"broken":' + }) + expect(res.statusCode).toBe(400) + expect(res.json().error.code).toStartWith('FST_') + }) +}) + describe('Zod errors', () => { test('400 with per-field detail — it is the caller’s own input', async () => { const res = await get('/_t/zod') diff --git a/backend/tests/integration/invite.test.ts b/backend/tests/integration/invite.test.ts index 2247681..655c2a0 100644 --- a/backend/tests/integration/invite.test.ts +++ b/backend/tests/integration/invite.test.ts @@ -268,11 +268,13 @@ describe('setting a password revokes existing sessions', () => { }) describe('the emailed link', () => { - test('points at the frontend and carries the token', () => { - const url = buildSetPasswordUrl('abc-123_XYZ') + test('points at the frontend and carries the token in the fragment', () => { + const url = new URL(buildSetPasswordUrl('abc-123_XYZ')) - expect(url).toContain('/set-password?token=') - expect(url).toContain(encodeURIComponent('abc-123_XYZ')) + expect(url.pathname).toBe('/set-password') + expect(new URLSearchParams(url.hash.slice(1)).get('token')).toBe('abc-123_XYZ') + // Nothing in the part of the URL a browser sends to a server. + expect(url.search).toBe('') }) test('the API takes the token in the BODY, not the query string', async () => { diff --git a/backend/tests/unit/client-ip.test.ts b/backend/tests/unit/client-ip.test.ts index 3a3dd5c..f456071 100644 --- a/backend/tests/unit/client-ip.test.ts +++ b/backend/tests/unit/client-ip.test.ts @@ -1,11 +1,11 @@ // client-ip.test.ts — the address used for rate limits and audit rows. import { describe, test, expect } from 'bun:test' -import { clientIp } from '../../src/lib/clientIp.ts' +import { clientIp, rateLimitKey } from '../../src/lib/clientIp.ts' import type { FastifyRequest } from 'fastify' -const fake = (headers: Record, ip = '203.0.113.9') => - ({ headers, ip }) as unknown as FastifyRequest +const fake = (headers: Record, ip = '203.0.113.9', socketAddress = '10.1.2.3') => + ({ headers, ip, socket: { remoteAddress: socketAddress } }) as unknown as FastifyRequest describe('clientIp', () => { test('falls back to request.ip when no trusted header is configured', () => { @@ -22,7 +22,46 @@ describe('clientIp', () => { expect(clientIp(fake({ 'cf-connecting-ip': '198.51.100.7, 1.1.1.1' }), 'CF-Connecting-IP')).toBe('198.51.100.7') }) - test('falls back when the configured header is absent', () => { - expect(clientIp(fake({}), 'cf-connecting-ip')).toBe('203.0.113.9') + test('🔑 header configured but absent: uses the TCP peer, never X-Forwarded-For', () => { + // request.ip is what trustProxy read out of X-Forwarded-For — client-written. + const request = fake({ 'x-forwarded-for': '6.6.6.6' }, '6.6.6.6', '10.1.2.3') + expect(clientIp(request, 'cf-connecting-ip')).toBe('10.1.2.3') + }) +}) + +describe('rateLimitKey', () => { + test('IPv4 is its own bucket', () => { + expect(rateLimitKey('198.51.100.7')).toBe('198.51.100.7') + }) + + test('IPv4-mapped IPv6 is the same bucket as plain IPv4', () => { + expect(rateLimitKey('::ffff:198.51.100.7')).toBe('198.51.100.7') + }) + + test('🔑 every address in one IPv6 /64 shares a bucket', () => { + const a = rateLimitKey('2001:db8:1234:5678::1') + const b = rateLimitKey('2001:db8:1234:5678:ffff:eeee:dddd:cccc') + expect(a).toBe('2001:db8:1234:5678::/64') + expect(b).toBe(a) + }) + + test('a different /64 is a different bucket', () => { + expect(rateLimitKey('2001:db8:1234:5679::1')).not.toBe(rateLimitKey('2001:db8:1234:5678::1')) + }) + + test('spellings of the same address agree', () => { + const expected = '2001:db8:0:0::/64' + expect(rateLimitKey('2001:0db8:0000:0000:0000:0000:0000:0001')).toBe(expected) + expect(rateLimitKey('2001:DB8::1')).toBe(expected) + expect(rateLimitKey('2001:db8::1%eth0')).toBe(expected) + }) + + test('compression at the start and embedded IPv4 at the end', () => { + expect(rateLimitKey('::1')).toBe('0:0:0:0::/64') + expect(rateLimitKey('64:ff9b::198.51.100.7')).toBe('64:ff9b:0:0::/64') + }) + + test('anything that is not an IP passes through untouched', () => { + expect(rateLimitKey('unknown')).toBe('unknown') }) }) diff --git a/backend/tests/unit/concurrency.test.ts b/backend/tests/unit/concurrency.test.ts new file mode 100644 index 0000000..1ebc399 --- /dev/null +++ b/backend/tests/unit/concurrency.test.ts @@ -0,0 +1,72 @@ +// concurrency.test.ts — the gate in front of Argon2id. + +import { describe, test, expect } from 'bun:test' +import { BoundedConcurrency } from '../../src/lib/concurrency.ts' +import { ServiceBusyError } from '../../src/lib/errors.ts' + +/** A task we finish by hand, so the test controls exactly when slots free up. */ +function deferred() { + let finish!: () => void + const promise = new Promise((resolve) => (finish = resolve)) + return { promise, finish } +} + +const tick = () => new Promise((resolve) => setTimeout(resolve, 0)) + +describe('BoundedConcurrency', () => { + test('runs at most maxActive at once', async () => { + const gate = new BoundedConcurrency(2, 10) + const tasks = [deferred(), deferred(), deferred()] + const runs = tasks.map((t) => gate.run(() => t.promise)) + await tick() + + expect(gate.stats).toEqual({ active: 2, waiting: 1 }) + + tasks.forEach((t) => t.finish()) + await Promise.all(runs) + expect(gate.stats).toEqual({ active: 0, waiting: 0 }) + }) + + test('waiters run in arrival order', async () => { + const gate = new BoundedConcurrency(1, 10) + const order: number[] = [] + const first = deferred() + + const runs = [ + gate.run(async () => { + await first.promise + order.push(1) + }), + gate.run(async () => void order.push(2)), + gate.run(async () => void order.push(3)) + ] + first.finish() + await Promise.all(runs) + + expect(order).toEqual([1, 2, 3]) + }) + + test('🔑 refuses with ServiceBusyError once the queue is full', async () => { + const gate = new BoundedConcurrency(1, 1) + const blocker = deferred() + const running = gate.run(() => blocker.promise) + const queued = gate.run(async () => 'queued ran') + await tick() + + await expect(gate.run(async () => 'never')).rejects.toBeInstanceOf(ServiceBusyError) + + blocker.finish() + await running + expect(await queued).toBe('queued ran') + }) + + test('🔑 a task that throws still frees its slot', async () => { + // A leaked slot would shrink capacity by one per failure until logins + // stopped altogether. + const gate = new BoundedConcurrency(1, 0) + await expect(gate.run(async () => { throw new Error('boom') })).rejects.toThrow('boom') + + expect(gate.stats).toEqual({ active: 0, waiting: 0 }) + expect(await gate.run(async () => 'next')).toBe('next') + }) +}) diff --git a/backend/tests/unit/env-rules.test.ts b/backend/tests/unit/env-rules.test.ts new file mode 100644 index 0000000..326cb51 --- /dev/null +++ b/backend/tests/unit/env-rules.test.ts @@ -0,0 +1,85 @@ +// env-rules.test.ts — the configurations the API refuses to start with. +// +// env.ts validates at import time and exits on failure, so each case runs in +// a child process with a crafted environment. Every variable the rules look +// at is set explicitly: dotenv fills only what is missing, so backend/.env +// cannot leak into a case. + +import { describe, test, expect } from 'bun:test' +import { resolve } from 'node:path' + +const BACKEND = resolve(import.meta.dir, '../..') + +const BASE: Record = { + DATABASE_URL: 'postgresql://u:p@db.example.com:5432/app?sslmode=require', + DATABASE_CA_CERT: '', + REDIS_URL: 'redis://localhost:6379', + JWT_SECRET: 'x'.repeat(40), + NODE_ENV: 'production', + FRONTEND_URL: 'https://app.example.com', + STRIPE_SECRET_KEY: 'sk_test_abc', + STRIPE_WEBHOOK_SECRET: 'whsec_abc', + RESEND_API_KEY: 're_abc', + DEMO_MODE: 'false' +} + +function boot(overrides: Record): { ok: boolean; output: string; frontendUrl?: string } { + const env = { ...BASE, ...overrides } + // An empty string means "unset" for the optional CA; drop it entirely. + if (env.DATABASE_CA_CERT === '') delete env.DATABASE_CA_CERT + + const result = Bun.spawnSync({ + cmd: ['bun', '-e', "const { env } = await import('./src/lib/env.ts'); console.log('BOOTED ' + env.FRONTEND_URL)"], + cwd: BACKEND, + env: { PATH: process.env.PATH ?? '', HOME: process.env.HOME ?? '', ...env } + }) + const output = result.stdout.toString() + result.stderr.toString() + const booted = /BOOTED (\S+)/.exec(output) + return { ok: result.exitCode === 0 && booted !== null, output, frontendUrl: booted?.[1] } +} + +describe('startup rules', () => { + test('a correct production configuration boots', () => { + expect(boot({}).ok).toBe(true) + }) + + test('🔑 DEMO_MODE with a live Stripe secret key is refused', () => { + const result = boot({ DEMO_MODE: 'true', STRIPE_SECRET_KEY: 'sk_live_abc' }) + expect(result.ok).toBe(false) + expect(result.output).toContain('live-nyckel') + }) + + test('DEMO_MODE with a live restricted key is refused too', () => { + expect(boot({ DEMO_MODE: 'true', STRIPE_SECRET_KEY: 'rk_live_abc' }).ok).toBe(false) + }) + + test('DEMO_MODE with a test key boots', () => { + expect(boot({ DEMO_MODE: 'true', STRIPE_SECRET_KEY: 'sk_test_abc' }).ok).toBe(true) + }) + + test('a live key without DEMO_MODE boots — that is a real deployment', () => { + expect(boot({ STRIPE_SECRET_KEY: 'sk_live_abc' }).ok).toBe(true) + }) + + test('🔑 production refuses a non-https FRONTEND_URL', () => { + const result = boot({ FRONTEND_URL: 'http://localhost:5173' }) + expect(result.ok).toBe(false) + expect(result.output).toContain('FRONTEND_URL') + }) + + test('a trailing slash on FRONTEND_URL is stripped, so it matches Origin', () => { + expect(boot({ FRONTEND_URL: 'https://app.example.com/' }).frontendUrl).toBe('https://app.example.com') + }) + + test('production refuses a database connection without TLS', () => { + const result = boot({ DATABASE_URL: 'postgresql://u:p@db.example.com:5432/app' }) + expect(result.ok).toBe(false) + expect(result.output).toContain('saknar TLS') + }) + + test('a CA file plus sslmode in the URL is refused', () => { + const result = boot({ DATABASE_CA_CERT: 'certs/supabase-prod-ca-2021.crt' }) + expect(result.ok).toBe(false) + expect(result.output).toContain('ssl*-parametrar') + }) +}) diff --git a/backend/tests/unit/mailer-redaction.test.ts b/backend/tests/unit/mailer-redaction.test.ts new file mode 100644 index 0000000..450b6b7 --- /dev/null +++ b/backend/tests/unit/mailer-redaction.test.ts @@ -0,0 +1,24 @@ +// mailer-redaction.test.ts — what the console mail transport may print. + +import { describe, test, expect } from 'bun:test' +import { printableBody } from '../../src/lib/mailer.ts' + +const BODY = 'Välj ditt lösenord här:\nhttps://app.example.com/set-password#token=Zm9vYmFyLXNlY3JldA\n\nHälsningar' + +describe('printableBody', () => { + test('🔑 production: the set-password token never reaches the log', () => { + const printed = printableBody(BODY, true) + expect(printed).not.toContain('Zm9vYmFyLXNlY3JldA') + expect(printed).toContain('set-password#token=[REDACTED]') + expect(printed).toContain('Hälsningar') + }) + + test('also covers an old-style ?token= link and several tokens', () => { + const printed = printableBody('a ?token=one&x=1 b #token=two', true) + expect(printed).toBe('a ?token=[REDACTED]&x=1 b #token=[REDACTED]') + }) + + test('development: printed in full, because the log IS how a local invite gets finished', () => { + expect(printableBody(BODY, false)).toBe(BODY) + }) +}) diff --git a/docs/architecture-decisions.md b/docs/architecture-decisions.md index 04df9cf..713669c 100644 --- a/docs/architecture-decisions.md +++ b/docs/architecture-decisions.md @@ -1120,6 +1120,35 @@ Supabase dashboard. Two locks, either one sufficient. --- +## 55. Hardening, round two — what the post-outage audit found + +**Context:** the audit after the 2026-09-23 outage (ADR 51) found no +critical issues, and a set of medium and low ones. None was being +exploited; each was cheap to close. + +| Finding | Fix | +|---|---| +| `/health/whoami` public in production: a live test bench for which forged header moves your rate-limit bucket | Removed. It did its job (ADR 50); the ADR keeps the lesson | +| With `CLIENT_IP_HEADER` set but the header absent, `clientIp` fell back to `request.ip` — read from client-written `X-Forwarded-For` | Falls back to the TCP peer (`socket.remoteAddress`), which cannot be forged | +| IPv6 rate-limit buckets per full address: one subscriber's /64 is 2^64 buckets | Keyed per /64 (`rateLimitKey`); IPv4-mapped addresses fold to IPv4. Audit rows keep the full address | +| Argon2id unbounded: logins from many IPs (invisible to the per-IP limit) could saturate a fraction-of-a-CPU instance | A gate: 2 at once, 20 waiting, then 503 + `Retry-After` (`ServiceBusyError`). Outside the `try` in `verifyPassword` — inside, "busy" would read as a wrong password and count against the account. The dummy verify uses the same gate, so timing still says nothing | +| Nothing stopped `DEMO_MODE` with a live Stripe key | Refused at boot, any environment (`sk_live_`, `rk_live_`) | +| `FRONTEND_URL` silently defaulted to localhost in production; a trailing slash broke CORS | https required in production; trailing slashes stripped | +| Set-password tokens printed to Render's logs by the console mail transport | Redacted in production; printed in full locally, where the log is how an invite gets finished | +| The set-password link carried the token in `?token=` — sent to Vercel with the page load, kept in history | Moved to the fragment (`#token=`), which browsers never send; the page wipes it from the address bar. Old `?token=` links still work until they expire | +| Any error with a 4xx `statusCode` had its message forwarded — Stripe SDK errors carry 400 and name session ids | Only Fastify's own errors (`FST_…`) pass through; anything else is a 500, reported to Sentry | + +**Tests:** startup rules run `env.ts` in a child process per case +(`env-rules.test.ts`), since it exits on failure. `SetPasswordPage.test.tsx` +checks the token is read from the fragment and gone from the URL before the +user types. + +**Found while testing:** three back-to-back e2e runs from localhost hit the +login rate limit (5/min per IP, in Redis, so it outlives a run). The limiter +working as designed; recorded because it will look like a flaky test next time. + +--- + ## Open decisions | Question | Status | diff --git a/frontend/src/pages/SetPasswordPage.test.tsx b/frontend/src/pages/SetPasswordPage.test.tsx new file mode 100644 index 0000000..3a6e553 --- /dev/null +++ b/frontend/src/pages/SetPasswordPage.test.tsx @@ -0,0 +1,68 @@ +// SetPasswordPage.test.tsx — where the invite token is read from, and that +// it does not stay in the URL. +// +// The page reads window.location directly (the token is in the fragment, +// which the router does not model), so each test sets the real jsdom URL. + +import { describe, test, expect, vi, beforeEach } from 'vitest' +import { render, screen, waitFor } from '@testing-library/react' +import userEvent from '@testing-library/user-event' +import { MemoryRouter } from 'react-router-dom' +import { SetPasswordPage } from './SetPasswordPage.tsx' + +const PASSWORD = 'en lång och bra lösenfras' + +function renderAt(url: string) { + window.history.replaceState(null, '', url) + const sent: unknown[] = [] + vi.stubGlobal( + 'fetch', + vi.fn(async (_input: RequestInfo | URL, init?: RequestInit) => { + sent.push(JSON.parse(String(init?.body ?? '{}'))) + return new Response(null, { status: 204 }) + }) + ) + render( + + + + ) + return sent +} + +async function submit() { + await userEvent.type(screen.getByLabelText('Nytt lösenord'), PASSWORD) + await userEvent.type(screen.getByLabelText('Upprepa lösenordet'), PASSWORD) + await userEvent.click(screen.getByRole('button', { name: 'Spara lösenord' })) +} + +beforeEach(() => { + vi.restoreAllMocks() +}) + +describe('SetPasswordPage', () => { + test('🔑 reads the token from the fragment and wipes it from the address bar', async () => { + const sent = renderAt('/set-password#token=abc-123') + + // Cleaned straight away — before the user has typed anything. + await waitFor(() => expect(window.location.href).not.toContain('abc-123')) + expect(window.location.pathname).toBe('/set-password') + + // ...and the token still reaches the API, in the body. + await submit() + await waitFor(() => expect(sent).toEqual([{ token: 'abc-123', password: PASSWORD }])) + }) + + test('still accepts an older ?token= link, and cleans that too', async () => { + const sent = renderAt('/set-password?token=old-link') + + await waitFor(() => expect(window.location.search).toBe('')) + await submit() + await waitFor(() => expect(sent).toEqual([{ token: 'old-link', password: PASSWORD }])) + }) + + test('without a token, says the link is broken', () => { + renderAt('/set-password') + expect(screen.getByText('Ogiltig länk')).toBeTruthy() + }) +}) diff --git a/frontend/src/pages/SetPasswordPage.tsx b/frontend/src/pages/SetPasswordPage.tsx index 09b959f..a11dbd7 100644 --- a/frontend/src/pages/SetPasswordPage.tsx +++ b/frontend/src/pages/SetPasswordPage.tsx @@ -3,19 +3,42 @@ // The token arrives in the URL, because that is the only way an email link // can carry it. We read it, then send it in the request BODY — the API takes // it there deliberately, so it does not end up in server access logs. +// +// It arrives in the FRAGMENT (#token=), which browsers never send to any +// server — a ?token= reached the hosting provider's logs with the page load. +// Links sent before that change used the query string and are still read, +// until they expire on their own (7 days at most). -import { useState, type FormEvent } from 'react' -import { Link, useNavigate, useSearchParams } from 'react-router-dom' +import { useEffect, useState, type FormEvent } from 'react' +import { Link, useNavigate } from 'react-router-dom' import { api, ApiError } from '../lib/api.ts' import { Button, Field, Card } from '../components/ui.tsx' +/** Reads the token from the fragment, or the query string of an older link. No side effects. */ +function readTokenFromUrl(): string | null { + const fromFragment = new URLSearchParams(window.location.hash.slice(1)).get('token') + const fromQuery = new URLSearchParams(window.location.search).get('token') + return fromFragment ?? fromQuery +} + /** Mirrors the backend policy, so a user is told before a round trip. */ const MIN_LENGTH = 12 export function SetPasswordPage() { - const [searchParams] = useSearchParams() const navigate = useNavigate() - const token = searchParams.get('token') + // useState keeps the token after the URL is cleaned below; a plain call + // would read the now-empty URL on the next render. + const [token] = useState(readTokenFromUrl) + + // Then wipe it from the address bar. replaceState rewrites the current + // history entry, so the token is not left in history or visible over a + // shoulder. In an effect, not the initialiser: StrictMode runs initialisers + // twice in development, and the second run would find the URL already empty. + useEffect(() => { + if (window.location.hash || window.location.search) { + window.history.replaceState(window.history.state, '', window.location.pathname) + } + }, []) const [password, setPassword] = useState('') const [confirm, setConfirm] = useState('')