Pre-launch — Gluecron is in final validation. Public signups and git hosting for non-owner users open after launch review.
ccantynz/Gluecron.com
Commit1d3a220

fix(auth): logout did not revoke the session; 2FA had no rate limit

fix(auth): logout did not revoke the session; 2FA had no rate limit

Two defects in the auth path.

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 — a shared machine, a
   synced browser profile, a logged proxy — remained signed in after the user
   had explicitly signed out. Logging out must revoke the credential, not
   just forget it locally.

   Now deletes the row and invalidates the cache (softAuth/requireAuth read
   the cache BEFORE the database, so without that a deleted session keeps
   authenticating until the entry expires). The cookie is cleared even if the
   delete throws — leaving the user holding a live cookie because the DB
   hiccuped would be worse than the original bug.

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 arrives at the second factor holding a valid
   half-authenticated session and could grind a 6-digit TOTP (10^6) or a
   recovery code unthrottled.

   Adds a per-IP limit of 5/min — tighter than /login's 20 on purpose, since
   a real person types one code off their phone — plus an account-scoped
   lockout, which is the part an attacker cannot rotate IPs around. That
   reuses the existing evaluateLockout()/loginAttempts machinery the password
   step uses, so both factors share one threshold and one window rather than
   growing a second, divergent policy. The lockout is checked BEFORE any code
   is verified, and fails OPEN on a lookup error — locking every 2FA user out
   because one query failed would be a worse outage than the risk.

Both verified by reverting each fix and confirming the tests fail, then
restoring.

The test file documents a trap worth remembering: a naive block-comment
stripper is unsafe on this codebase, because route paths contain a
comment-opener sequence. Stripping app.tsx that way silently deleted the very
rate-limit registrations being asserted on and reported correct code as
broken. Comment-stripping is only needed for absence assertions; presence
assertions read raw source.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ccantynz-alt committed on July 28, 2026Parent: 4f4cb3a
3 files changed+241−01d3a220924d4976d97b82edce9f2eed056f980e5
3 changed files+241−0
Addedsrc/__tests__/auth-logout-2fa.test.ts+135−0View fileUnifiedSplit
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
25import { describe, expect, it } from "bun:test";
26import { 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.
39const stripLineComments = (s: string) => s.replace(/^\s*\/\/.*$/gm, "");
40
41const AUTH = stripLineComments(readFileSync("src/routes/auth.tsx", "utf8"));
42const APP = readFileSync("src/app.tsx", "utf8");
43
44describe("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
78describe("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
94describe("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});
Modifiedsrc/app.tsx+7−0View fileUnifiedSplit
375375app.use("/forgot-password", rateLimit(5, 60_000, "forgot-password"));
376376// BLOCK Q2 — throttle magic-link sign-in for the same reason.
377377app.use("/login/magic", rateLimit(5, 60_000, "magic-link"));
378// The second factor was the one auth endpoint with NO limit, which made it
379// the cheapest thing on the platform to attack: an attacker who already has
380// the password reaches /login/2fa holding a valid half-authenticated session
381// and can grind a 6-digit TOTP (10^6) or a recovery code unthrottled. Tighter
382// than /login deliberately — a real person types one code from their phone,
383// occasionally mistypes it, and never needs ten attempts a minute.
384app.use("/login/2fa", rateLimit(5, 60_000, "login-2fa"));
378385
379386// CSRF protection — set token on all requests, validate on mutations
380387app.use("*", csrfToken);
Modifiedsrc/routes/auth.tsx+99−0View fileUnifiedSplit
3737 getGithubOauthConfig,
3838 getGoogleOauthConfig,
3939} from "../lib/sso";
40import { sessionCache } from "../lib/cache";
4041import { Layout } from "../views/layout";
4142import { SignInV2 } from "../views/signin-v2";
4243import {
676677 return c.redirect(redirect);
677678 }
678679
680 // Account-scoped lockout, checked BEFORE any code is verified so a locked
681 // account costs an attacker nothing to discover and gains them nothing.
682 // Reuses the same evaluateLockout()/loginAttempts machinery the password
683 // step uses, so both factors share one threshold and one window.
684 try {
685 const [u] = await db
686 .select({ email: users.email })
687 .from(users)
688 .where(eq(users.id, session.userId))
689 .limit(1);
690 if (u?.email) {
691 const since = new Date(Date.now() - LOGIN_FAIL_WINDOW_MS);
692 const [agg] = await db
693 .select({
694 failures: sql<number>`count(*)::int`,
695 newest: sql<string | null>`max(${loginAttempts.createdAt})`,
696 })
697 .from(loginAttempts)
698 .where(
699 and(
700 eq(loginAttempts.email, u.email.toLowerCase()),
701 eq(loginAttempts.success, false),
702 gte(loginAttempts.createdAt, since)
703 )
704 );
705 const state = evaluateLockout({
706 failureCount: Number(agg?.failures ?? 0),
707 newestFailureAt: agg?.newest ? new Date(agg.newest) : null,
708 });
709 if (state.locked) {
710 return c.redirect(
711 `/login/2fa?error=${encodeURIComponent(
712 `Too many attempts. Try again in ${retryAfterMinutes(state)} minutes.`
713 )}&redirect=${encodeURIComponent(redirect)}`
714 );
715 }
716 }
717 } catch (err) {
718 // Fail OPEN on a lockout-lookup error: the per-IP rate limit still
719 // applies, and locking every 2FA user out because one query failed
720 // would be a worse outage than the risk it mitigates.
721 console.error("[auth] 2fa lockout check:", err);
722 }
723
679724 // Try TOTP code first.
680725 const isSix = /^\d{6}$/.test(code);
681726 let ok = false;
706751 }
707752
708753 if (!ok) {
754 // Record the failure and lock out after the same threshold the password
755 // step uses. The per-IP rate limit added in app.tsx throttles a single
756 // source; this is the part an attacker cannot rotate around, because it
757 // keys on the ACCOUNT. Without it, someone who already has the password
758 // holds a valid half-authenticated session and can grind a 6-digit TOTP
759 // from as many addresses as they like.
760 try {
761 const [u] = await db
762 .select({ email: users.email })
763 .from(users)
764 .where(eq(users.id, session.userId))
765 .limit(1);
766 if (u?.email) {
767 await db.insert(loginAttempts).values({
768 email: u.email.toLowerCase(),
769 success: false,
770 // Same derivation the password step uses — `ip` is NOT NULL, and
771 // "unknown" keeps an attempt behind a proxy that strips headers
772 // countable rather than silently dropping the row.
773 ip:
774 c.req.header("x-forwarded-for")?.split(",")[0]?.trim() ||
775 c.req.header("x-real-ip") ||
776 "unknown",
777 });
778 }
779 } catch (err) {
780 // A failed audit write must not become a free retry, but it also
781 // must not 500 the sign-in — log and fall through to the refusal.
782 console.error("[auth] 2fa failure record:", err);
783 }
709784 return c.redirect(
710785 `/login/2fa?error=Invalid+code&redirect=${encodeURIComponent(redirect)}`
711786 );
730805});
731806
732807auth.get("/logout", async (c) => {
808 // Dropping the cookie only clears the CLIENT's copy. The `sessions` row
809 // survived its full 30-day expiry and `sessionCache` kept serving the user
810 // for the cache TTL, so anyone holding a copy of the token — a shared
811 // machine, a synced browser profile, a logged proxy — stayed signed in
812 // after the user had explicitly signed out. Logging out has to revoke the
813 // credential, not just forget it.
814 const token = getCookie(c, "session");
815 if (token) {
816 try {
817 await db.delete(sessions).where(eq(sessions.token, token));
818 } catch (err) {
819 // Never leave the user with a live cookie because the delete failed —
820 // fall through and still clear it, and log so this is visible.
821 console.error("[logout] session row delete failed:", err);
822 }
823 // softAuth/requireAuth read this cache before the DB, so without an
824 // explicit invalidation the deleted session keeps authenticating until
825 // the entry expires on its own.
826 try {
827 sessionCache.invalidate(token);
828 } catch {
829 /* cache is best-effort */
830 }
831 }
733832 deleteCookie(c, "session", { path: "/" });
734833 return c.redirect("/");
735834});
736835