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