Blame · Line-by-line history

`auth-logout-2fa.test.ts`

Each line is annotated with the commit that last touched it. Click any SHA to jump to that commit and see the surrounding change.

auth-logout-2fa.test.tsBlame135 lines · 1 contributor

Normal view Raw

1d3a220ccantynz-alt	1	/**
	2	* Logging out must revoke the session, and the second factor must be
	3	* rate-limited.
	4	*
	5	* Two defects, both verified by reading the code:
	6	*
	7	* 1. GET /logout called deleteCookie() and nothing else. The `sessions` row
	8	* survived its full 30-day expiry and `sessionCache` kept serving the
	9	* user for its TTL, so anyone holding a copy of the token — shared
	10	* machine, synced browser profile, logged proxy — stayed signed in after
	11	* the user had explicitly signed out. Logging out has to revoke the
	12	* credential, not just forget it locally.
	13	*
	14	* 2. /login/2fa was the ONE auth endpoint with no rate limit. app.tsx
	15	* throttles /login, /register, /forgot-password and /login/magic. An
	16	* attacker who already has the password reaches the second factor holding
	17	* a valid half-authenticated session and could grind a 6-digit TOTP
	18	* (10^6) or a recovery code unthrottled.
	19	*
	20	* Assertions run against comment-stripped source: the explanations above and
	21	* in the implementation necessarily contain the strings being asserted, and
	22	* that has falsely tripped four separate assertions earlier in this session.
	23	*/
	24
	25	import { describe, expect, it } from "bun:test";
	26	import { readFileSync } from "fs";
	27
	28	// Strip comments — but ONLY line comments.
	29	//
	30	// A naive block-comment regex is unsafe on this codebase. Route paths such as
	31	// "/api" followed by a slash-star wildcard contain a comment-opener sequence,
	32	// so a block-comment pattern treats them as the start of a comment and eats
	33	// everything up to the next closer. In app.tsx that silently deleted the
	34	// rate-limit registrations this file asserts on, and reported a correct
	35	// implementation as broken.
	36	//
	37	// Stripping is only needed for ABSENCE assertions, where an explanatory
	38	// comment would falsely match. Presence assertions read raw source.
	39	const stripLineComments = (s: string) => s.replace(/^\s\/\/.$/gm, "");
	40
	41	const AUTH = stripLineComments(readFileSync("src/routes/auth.tsx", "utf8"));
	42	const APP = readFileSync("src/app.tsx", "utf8");
	43
	44	describe("logout revokes the session", () => {
	45	const handler = AUTH.slice(
	46	AUTH.indexOf('auth.get("/logout"'),
	47	AUTH.indexOf('auth.post("/api/auth/register"')
	48	);
	49
	50	it("has a handler to inspect", () => {
	51	// Anchored slice; a wrong anchor yields "" and every assertion below
	52	// would pass vacuously.
	53	expect(handler.length).toBeGreaterThan(100);
	54	});
	55
	56	it("deletes the session row, not just the cookie", () => {
	57	expect(handler).toContain("db.delete(sessions)");
	58	expect(handler).toContain("eq(sessions.token, token)");
	59	});
	60
	61	it("invalidates the session cache", () => {
	62	// softAuth/requireAuth read sessionCache BEFORE the database, so a
	63	// deleted row keeps authenticating until the entry expires on its own.
	64	expect(handler).toContain("sessionCache.invalidate(token)");
	65	});
	66
	67	it("still clears the cookie even if the row delete throws", () => {
	68	// Leaving the user holding a live cookie because the DB hiccuped would
	69	// be worse than the original bug.
	70	const deleteIdx = handler.indexOf("db.delete(sessions)");
	71	const cookieIdx = handler.indexOf("deleteCookie(c");
	72	expect(deleteIdx).toBeGreaterThan(-1);
	73	expect(cookieIdx).toBeGreaterThan(deleteIdx);
	74	expect(handler).toContain("catch");
	75	});
	76	});
	77
	78	describe("the second factor is rate limited", () => {
	79	it("app.tsx throttles /login/2fa", () => {
	80	expect(APP).toMatch(/app\.use\("\/login\/2fa", rateLimit\(/);
	81	});
	82
	83	it("is at least as tight as the password step", () => {
	84	const m = APP.match(/app\.use\("\/login\/2fa", rateLimit\((\d+),/);
	85	const login = APP.match(/app\.use\("\/login", rateLimit\((\d+),/);
	86	expect(m).not.toBeNull();
	87	expect(login).not.toBeNull();
	88	// A human types one code off their phone; ten a minute is already
	89	// generous, and /login allows 20.
	90	expect(Number(m![1])).toBeLessThanOrEqual(Number(login![1]));
	91	});
	92	});
	93
	94	describe("2FA failures are counted per account, not just per IP", () => {
	95	const handler = AUTH.slice(
	96	AUTH.indexOf('auth.post("/login/2fa"'),
	97	AUTH.indexOf('auth.get("/logout"')
	98	);
	99
	100	it("has a handler to inspect", () => {
	101	expect(handler.length).toBeGreaterThan(200);
	102	});
	103
	104	it("records a failed attempt", () => {
	105	// The per-IP limit throttles one source; this is the part an attacker
	106	// cannot rotate around, because it keys on the account.
	107	expect(handler).toContain("insert(loginAttempts)");
	108	expect(handler).toContain("success: false");
	109	});
	110
	111	it("checks the lockout before verifying any code", () => {
	112	const lockIdx = handler.indexOf("evaluateLockout");
	113	const verifyIdx = handler.indexOf("verifyTotpCode");
	114	expect(lockIdx).toBeGreaterThan(-1);
	115	expect(verifyIdx).toBeGreaterThan(-1);
	116	// A locked account should cost nothing to discover and gain nothing.
	117	expect(lockIdx).toBeLessThan(verifyIdx);
	118	});
	119
	120	it("reuses the password step's window and threshold", () => {
	121	// One threshold across both factors rather than a second, divergent one.
	122	expect(handler).toContain("LOGIN_FAIL_WINDOW_MS");
	123	expect(handler).toContain("evaluateLockout");
	124	});
	125
	126	it("fails OPEN if the lockout lookup errors", () => {
	127	// Locking every 2FA user out because one query failed would be a worse
	128	// outage than the risk it mitigates; the per-IP limit still applies.
	129	const lockBlock = handler.slice(
	130	handler.indexOf("evaluateLockout"),
	131	handler.indexOf("verifyTotpCode")
	132	);
	133	expect(lockBlock).toContain("catch");
	134	});
	135	});