security: rate-limit IP-keyed, fail-closed on empty key (#37)
Rate-limiter now accepts string | string[] so callers can key on multiple buckets simultaneously. If any bucket is exhausted the request is denied, which lets login/TOTP/reset-password throttle on BOTH user identifier and source IP without either becoming a bypass. Fail-closed: empty/whitespace-only keys now deny by default instead of silently allowing unbounded attempts (was CWE-307 gap). Degraded-fallback divisor reduced from /10 to /2 — the old aggressive clamp forced-logged-out legitimate users during brief Redis outages; /2 still meaningfully slows distributed brute-force. Callers updated: - auth.ts (login): both email: and ip: buckets - auth router requestPasswordReset: email + IP - auth router resetPassword: IP before lookup, email-reset after - invite router getInvite/acceptInvite: IP - user-self-service verifyTotp: userId + IP TRPCContext now carries clientIp; web tRPC route extracts it from X-Forwarded-For / X-Real-IP. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
This commit is contained in:
@@ -103,9 +103,9 @@ describe("rate limiter", () => {
|
||||
}));
|
||||
|
||||
const { createRateLimiter } = await import("../middleware/rate-limit.js");
|
||||
// Degraded fallback uses max(1, floor(maxRequests/10)), so with
|
||||
// maxRequests=20 the degraded limit is 2.
|
||||
const limiter = createRateLimiter(60_000, 20, {
|
||||
// Degraded fallback uses max(1, floor(maxRequests/2)), so with
|
||||
// maxRequests=4 the degraded limit is 2 attempts within the window.
|
||||
const limiter = createRateLimiter(60_000, 4, {
|
||||
backend: "redis",
|
||||
redisUrl: "redis://test",
|
||||
name: "redis-fallback-test",
|
||||
@@ -120,4 +120,39 @@ describe("rate limiter", () => {
|
||||
expect(third.allowed).toBe(false);
|
||||
expect(third.remaining).toBe(0);
|
||||
});
|
||||
|
||||
it("denies by default when called with an empty key (fail-closed)", async () => {
|
||||
const { createRateLimiter } = await import("../middleware/rate-limit.js");
|
||||
const limiter = createRateLimiter(60_000, 5, { backend: "memory", name: "empty-key-test" });
|
||||
|
||||
const empty = await limiter("");
|
||||
const whitespace = await limiter(" ");
|
||||
const emptyArray = await limiter([]);
|
||||
const allEmpty = await limiter(["", " "]);
|
||||
|
||||
expect(empty.allowed).toBe(false);
|
||||
expect(whitespace.allowed).toBe(false);
|
||||
expect(emptyArray.allowed).toBe(false);
|
||||
expect(allEmpty.allowed).toBe(false);
|
||||
});
|
||||
|
||||
it("denies if any key in a multi-key call is over its limit", async () => {
|
||||
const { createRateLimiter } = await import("../middleware/rate-limit.js");
|
||||
const limiter = createRateLimiter(60_000, 2, { backend: "memory", name: "multi-key-test" });
|
||||
|
||||
// Exhaust the "email:a" bucket alone
|
||||
await limiter("email:a");
|
||||
await limiter("email:a");
|
||||
const emailExhausted = await limiter("email:a");
|
||||
expect(emailExhausted.allowed).toBe(false);
|
||||
|
||||
// A call keyed on both email:a AND ip:x must deny because email:a is
|
||||
// exhausted, even though ip:x is fresh.
|
||||
const combined = await limiter(["email:a", "ip:x"]);
|
||||
expect(combined.allowed).toBe(false);
|
||||
|
||||
// A fresh bucket pair still succeeds.
|
||||
const freshPair = await limiter(["email:b", "ip:y"]);
|
||||
expect(freshPair.allowed).toBe(true);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -90,15 +90,16 @@ function makeSelfServiceCtx(dbOverrides: Record<string, unknown> = {}) {
|
||||
};
|
||||
}
|
||||
|
||||
function makePublicCtx(dbOverrides: Record<string, unknown> = {}) {
|
||||
function makePublicCtx(overrides: Record<string, unknown> = {}) {
|
||||
return {
|
||||
db: {
|
||||
user: {
|
||||
findUnique: vi.fn(),
|
||||
update: vi.fn().mockResolvedValue({}),
|
||||
...((dbOverrides.user as object | undefined) ?? {}),
|
||||
...((overrides.user as object | undefined) ?? {}),
|
||||
},
|
||||
},
|
||||
clientIp: (overrides.clientIp as string | null | undefined) ?? null,
|
||||
};
|
||||
}
|
||||
|
||||
@@ -277,14 +278,27 @@ describe("verifyTotp", () => {
|
||||
expect(ctx.db.user.findUnique).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("calls the rate limiter with the userId as key", async () => {
|
||||
it("calls the rate limiter with both userId and client IP as keys", async () => {
|
||||
totpValidateMock.mockReturnValue(0);
|
||||
const ctx = makePublicCtx({
|
||||
user: { findUnique: vi.fn().mockResolvedValue(mfaUser) },
|
||||
clientIp: "198.51.100.7",
|
||||
});
|
||||
await verifyTotp(ctx as Parameters<typeof verifyTotp>[0], {
|
||||
userId: "user_1",
|
||||
token: "123456",
|
||||
});
|
||||
expect(totpRateLimiterMock).toHaveBeenCalledWith(["user:user_1", "ip:198.51.100.7"]);
|
||||
});
|
||||
|
||||
it("falls back to userId-only keying when no client IP is available", async () => {
|
||||
totpValidateMock.mockReturnValue(0);
|
||||
const ctx = makePublicCtx({ user: { findUnique: vi.fn().mockResolvedValue(mfaUser) } });
|
||||
await verifyTotp(ctx as Parameters<typeof verifyTotp>[0], {
|
||||
userId: "user_1",
|
||||
token: "123456",
|
||||
});
|
||||
expect(totpRateLimiterMock).toHaveBeenCalledWith("user_1");
|
||||
expect(totpRateLimiterMock).toHaveBeenCalledWith(["user:user_1"]);
|
||||
});
|
||||
});
|
||||
|
||||
|
||||
Reference in New Issue
Block a user