Skip to content

Commit 70103e1

Browse files
committed
sign CSRF with cookie, Login rate-limit key hardened against identifier-only lockout
1 parent fd013de commit 70103e1

6 files changed

Lines changed: 104 additions & 24 deletions

File tree

backend/src/__tests__/auth-enabled.integration.ts

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ describe("Auth Enabled Toggle Authorization", () => {
1111
const userAgent = "vitest-auth-enabled";
1212
let prisma: PrismaClient;
1313
let app: any;
14+
let agent: any;
1415
let csrfHeaderName: string;
1516
let csrfToken: string;
1617
let regularUserToken: string;
@@ -79,7 +80,7 @@ describe("Auth Enabled Toggle Authorization", () => {
7980
signOptions
8081
);
8182

82-
const agent = request.agent(app);
83+
agent = request.agent(app);
8384
const csrfRes = await agent
8485
.get("/csrf-token")
8586
.set("User-Agent", userAgent);
@@ -92,7 +93,7 @@ describe("Auth Enabled Toggle Authorization", () => {
9293
});
9394

9495
it("rejects unauthenticated auth-enabled toggle when auth is enabled", async () => {
95-
const response = await request(app)
96+
const response = await agent
9697
.post("/auth/auth-enabled")
9798
.set("User-Agent", userAgent)
9899
.set(csrfHeaderName, csrfToken)
@@ -102,7 +103,7 @@ describe("Auth Enabled Toggle Authorization", () => {
102103
});
103104

104105
it("rejects non-admin auth-enabled toggle", async () => {
105-
const response = await request(app)
106+
const response = await agent
106107
.post("/auth/auth-enabled")
107108
.set("User-Agent", userAgent)
108109
.set("Authorization", `Bearer ${regularUserToken}`)
@@ -120,7 +121,7 @@ describe("Auth Enabled Toggle Authorization", () => {
120121
expect(warmStatusResponse.status).toBe(200);
121122
expect(warmStatusResponse.body?.authEnabled).toBe(true);
122123

123-
const toggleResponse = await request(app)
124+
const toggleResponse = await agent
124125
.post("/auth/auth-enabled")
125126
.set("User-Agent", userAgent)
126127
.set("Authorization", `Bearer ${adminUserToken}`)

backend/src/__tests__/imports-compat.integration.ts

Lines changed: 12 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -267,6 +267,7 @@ describe("Import compatibility (legacy exports)", () => {
267267
const userAgent = "vitest-import-compat";
268268
let prisma: ReturnType<typeof getTestPrisma>;
269269
let app: any;
270+
let agent: any;
270271
let csrfHeaderName: string;
271272
let csrfToken: string;
272273

@@ -278,7 +279,8 @@ describe("Import compatibility (legacy exports)", () => {
278279
// Import the server AFTER DATABASE_URL is set by setupTestDb/getTestPrisma.
279280
({ app } = await import("../index"));
280281

281-
const csrfRes = await request(app).get("/csrf-token").set("User-Agent", userAgent);
282+
agent = request.agent(app);
283+
const csrfRes = await agent.get("/csrf-token").set("User-Agent", userAgent);
282284
csrfHeaderName = csrfRes.body.header;
283285
csrfToken = csrfRes.body.token;
284286
expect(typeof csrfHeaderName).toBe("string");
@@ -301,7 +303,7 @@ describe("Import compatibility (legacy exports)", () => {
301303
includeTrashDrawing: false,
302304
});
303305

304-
const res = await request(app)
306+
const res = await agent
305307
.post("/import/sqlite/legacy/verify")
306308
.set("User-Agent", userAgent)
307309
.set(csrfHeaderName, csrfToken)
@@ -323,7 +325,7 @@ describe("Import compatibility (legacy exports)", () => {
323325
includeTrashDrawing: true,
324326
});
325327

326-
const res = await request(app)
328+
const res = await agent
327329
.post("/import/sqlite/legacy")
328330
.set("User-Agent", userAgent)
329331
.set(csrfHeaderName, csrfToken)
@@ -359,7 +361,7 @@ describe("Import compatibility (legacy exports)", () => {
359361
includeTrashDrawing: false,
360362
});
361363

362-
const verify = await request(app)
364+
const verify = await agent
363365
.post("/import/sqlite/legacy/verify")
364366
.set("User-Agent", userAgent)
365367
.set(csrfHeaderName, csrfToken)
@@ -369,7 +371,7 @@ describe("Import compatibility (legacy exports)", () => {
369371
expect(verify.body.drawings).toBe(2);
370372
expect(verify.body.collections).toBe(1);
371373

372-
const res = await request(app)
374+
const res = await agent
373375
.post("/import/sqlite/legacy")
374376
.set("User-Agent", userAgent)
375377
.set(csrfHeaderName, csrfToken)
@@ -386,7 +388,7 @@ describe("Import compatibility (legacy exports)", () => {
386388
db.exec(`CREATE TABLE "NotDrawing" (id TEXT PRIMARY KEY NOT NULL);`);
387389
db.close();
388390

389-
const res = await request(app)
391+
const res = await agent
390392
.post("/import/sqlite/legacy/verify")
391393
.set("User-Agent", userAgent)
392394
.set(csrfHeaderName, csrfToken)
@@ -398,7 +400,7 @@ describe("Import compatibility (legacy exports)", () => {
398400

399401
it("rejects .excalidash verify when manifest has duplicate drawing IDs", async () => {
400402
const archive = await createExcalidashArchiveWithDuplicateDrawingIds();
401-
const res = await request(app)
403+
const res = await agent
402404
.post("/import/excalidash/verify")
403405
.set("User-Agent", userAgent)
404406
.set(csrfHeaderName, csrfToken)
@@ -410,7 +412,7 @@ describe("Import compatibility (legacy exports)", () => {
410412

411413
it("rejects .excalidash import when manifest has duplicate drawing IDs", async () => {
412414
const archive = await createExcalidashArchiveWithDuplicateDrawingIds();
413-
const res = await request(app)
415+
const res = await agent
414416
.post("/import/excalidash")
415417
.set("User-Agent", userAgent)
416418
.set(csrfHeaderName, csrfToken)
@@ -422,7 +424,7 @@ describe("Import compatibility (legacy exports)", () => {
422424

423425
it("rejects legacy verify when DB has duplicate drawing IDs", async () => {
424426
const legacyDb = createLegacySqliteDbWithDuplicateDrawingIds();
425-
const res = await request(app)
427+
const res = await agent
426428
.post("/import/sqlite/legacy/verify")
427429
.set("User-Agent", userAgent)
428430
.set(csrfHeaderName, csrfToken)
@@ -434,7 +436,7 @@ describe("Import compatibility (legacy exports)", () => {
434436

435437
it("rejects legacy import when DB has duplicate drawing IDs", async () => {
436438
const legacyDb = createLegacySqliteDbWithDuplicateDrawingIds();
437-
const res = await request(app)
439+
const res = await agent
438440
.post("/import/sqlite/legacy")
439441
.set("User-Agent", userAgent)
440442
.set(csrfHeaderName, csrfToken)

backend/src/auth.ts

Lines changed: 41 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -81,6 +81,7 @@ export const createAuthRouter = (deps: CreateAuthRouterDeps): express.Router =>
8181
let loginRateLimitConfig: LoginRateLimitConfig = { ...DEFAULT_LOGIN_RATE_LIMIT };
8282
let loginAttemptLimiter: ReturnType<typeof rateLimit> | null = null;
8383
let loginLimiterInitPromise: Promise<void> | null = null;
84+
let loginIdentifierKeyIndex = new Map<string, Set<string>>();
8485

8586
const parseLoginRateLimitConfig = (
8687
systemConfig: Awaited<ReturnType<typeof ensureSystemConfig>>
@@ -114,8 +115,31 @@ export const createAuthRouter = (deps: CreateAuthRouterDeps): express.Router =>
114115
return trimmed.length > 0 ? trimmed.slice(0, 255) : null;
115116
};
116117

118+
const resolveRateLimitIp = (req: Request): string =>
119+
(req.ip || req.connection.remoteAddress || "unknown").slice(0, 255);
120+
121+
const trackIdentifierRateLimitKey = (identifier: string, key: string): void => {
122+
if (!loginIdentifierKeyIndex.has(identifier) && loginIdentifierKeyIndex.size >= 5000) {
123+
const oldestIdentifier = loginIdentifierKeyIndex.keys().next().value;
124+
if (typeof oldestIdentifier === "string") {
125+
loginIdentifierKeyIndex.delete(oldestIdentifier);
126+
}
127+
}
128+
129+
const existing = loginIdentifierKeyIndex.get(identifier) ?? new Set<string>();
130+
if (existing.size >= 50) {
131+
const oldestKey = existing.values().next().value;
132+
if (typeof oldestKey === "string") {
133+
existing.delete(oldestKey);
134+
}
135+
}
136+
existing.add(key);
137+
loginIdentifierKeyIndex.set(identifier, existing);
138+
};
139+
117140
const buildLoginAttemptLimiter = (cfg: LoginRateLimitConfig) => {
118141
const store = new MemoryStore();
142+
loginIdentifierKeyIndex = new Map<string, Set<string>>();
119143
const limiter = rateLimit({
120144
windowMs: cfg.windowMs,
121145
max: cfg.max,
@@ -131,8 +155,12 @@ export const createAuthRouter = (deps: CreateAuthRouterDeps): express.Router =>
131155
store,
132156
keyGenerator: (req) => {
133157
const identifier = resolveAuthIdentifier(req as Request);
134-
if (identifier) return `login:${identifier}`;
135-
const ip = (req as Request).ip || "unknown";
158+
const ip = resolveRateLimitIp(req as Request);
159+
if (identifier) {
160+
const key = `login:${identifier}:ip:${ip}`;
161+
trackIdentifierRateLimitKey(identifier, key);
162+
return key;
163+
}
136164
return `login-ip:${ip}`;
137165
},
138166
});
@@ -171,9 +199,18 @@ export const createAuthRouter = (deps: CreateAuthRouterDeps): express.Router =>
171199

172200
const resetLoginAttemptKey = async (identifier: string): Promise<void> => {
173201
await ensureLoginAttemptLimiter();
174-
const key = `login:${identifier}`;
202+
const normalizedIdentifier = identifier.trim().toLowerCase();
203+
const keys = loginIdentifierKeyIndex.get(normalizedIdentifier);
175204
try {
176-
await loginAttemptLimiter?.resetKey(key);
205+
if (!keys || keys.size === 0) {
206+
// Backward-compatible fallback for pre-change key format.
207+
await loginAttemptLimiter?.resetKey(`login:${normalizedIdentifier}`);
208+
return;
209+
}
210+
for (const key of keys) {
211+
await loginAttemptLimiter?.resetKey(key);
212+
}
213+
loginIdentifierKeyIndex.delete(normalizedIdentifier);
177214
} catch (error) {
178215
if (process.env.NODE_ENV === "development") {
179216
console.debug("Rate limit reset skipped:", error);

backend/src/server/csrf.test.ts

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,41 @@
1+
import express from "express";
2+
import request from "supertest";
3+
import { describe, expect, it } from "vitest";
4+
import { registerCsrfProtection } from "./csrf";
5+
6+
describe("CSRF token issuance", () => {
7+
it("binds first-issued tokens to cookie client identity", async () => {
8+
const app = express();
9+
app.use(express.json());
10+
11+
registerCsrfProtection({
12+
app,
13+
isAllowedOrigin: () => true,
14+
maxRequestsPerWindow: 100,
15+
});
16+
17+
app.post("/drawings", (_req, res) => {
18+
res.status(200).json({ ok: true });
19+
});
20+
21+
const agent = request.agent(app);
22+
const csrfRes = await agent
23+
.get("/csrf-token")
24+
.set("User-Agent", "csrf-test-agent-a");
25+
26+
expect(csrfRes.status).toBe(200);
27+
const headerName = csrfRes.body.header as string;
28+
const token = csrfRes.body.token as string;
29+
expect(typeof headerName).toBe("string");
30+
expect(typeof token).toBe("string");
31+
32+
const postRes = await agent
33+
.post("/drawings")
34+
.set("User-Agent", "csrf-test-agent-b")
35+
.set(headerName, token)
36+
.send({ name: "test" });
37+
38+
expect(postRes.status).toBe(200);
39+
expect(postRes.body.ok).toBe(true);
40+
});
41+
});

backend/src/server/csrf.ts

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,6 @@ import {
1010
CSRF_CLIENT_COOKIE_NAME,
1111
getCsrfClientCookieValue,
1212
getCsrfValidationClientIds,
13-
getLegacyClientId,
1413
} from "../security/csrfClient";
1514

1615
const CSRF_CLIENT_COOKIE_MAX_AGE_SECONDS = 60 * 60 * 24 * 30; // 30 days
@@ -53,7 +52,7 @@ export const registerCsrfProtection = ({
5352
const getClientIdForTokenIssue = (
5453
req: express.Request,
5554
res: express.Response
56-
): { clientId: string; strategy: "cookie" | "legacy-bootstrap" } => {
55+
): { clientId: string; strategy: "cookie" } => {
5756
const existingCookieValue = getCsrfClientCookieValue(req);
5857
if (existingCookieValue) {
5958
return {
@@ -65,8 +64,8 @@ export const registerCsrfProtection = ({
6564
const generatedCookieValue = crypto.randomUUID().replace(/-/g, "");
6665
setCsrfClientCookie(req, res, generatedCookieValue);
6766
return {
68-
clientId: getLegacyClientId(req),
69-
strategy: "legacy-bootstrap",
67+
clientId: `cookie:${generatedCookieValue}`,
68+
strategy: "cookie",
7069
};
7170
};
7271

frontend/src/pages/Settings.tsx

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -76,8 +76,8 @@ export const Settings: React.FC = () => {
7676
);
7777

7878
if (response.data.authEnabled) {
79-
// Auth enabled -> prompt admin bootstrap via register.
80-
window.location.href = '/register';
79+
// Auth enabled -> bootstrap registration only when required.
80+
window.location.href = response.data.bootstrapRequired ? '/register' : '/login';
8181
return;
8282
}
8383

0 commit comments

Comments
 (0)