feat: per-account login lockout, mail index, resend captcha, i18n scoping
Gitea Actions Runner Test / test-job (push) Successful in 2s
CI / check (push) Successful in 28s
CI / tests-integration (push) Successful in 1m42s
CI / tests-unit (push) Failing after 1m45s
CI / tests-ui (push) Successful in 2m29s
CI / preflight (push) Skipped
CI / deploy (push) Skipped
Gitea Actions Runner Test / test-job (push) Successful in 2s
CI / check (push) Successful in 28s
CI / tests-integration (push) Successful in 1m42s
CI / tests-unit (push) Failing after 1m45s
CI / tests-ui (push) Successful in 2m29s
CI / preflight (push) Skipped
CI / deploy (push) Skipped
Closes the four HIGH/MEDIUM items left open after the previous pass. Login lockout - The only login limits were keyed on the client IP, so a distributed attempt could grind on one account indefinitely. Added a per-account lockout with a budget of 8 failures per 15 minutes. - The bucket is keyed on the RESOLVED account id, not on the submitted string: users may sign in with either username or e-mail and neither the lookup nor the input normaliser folds case, so an input-keyed bucket would hand out a fresh budget per spelling of the same account. - precheckLogin and NextAuth's authorize share the bucket, so the pre-check cannot be used to buy extra attempts and a client that skips it entirely is still bounded. Both check the lockout BEFORE verifying the password: the success path clears the counter, which would otherwise walk a locked account straight back in on the right password. - A successful login clears the failures, which needs two new primitives in rate-limit.ts: peekRateLimit (read-only, does not consume a unit) and clearRateLimit. - Fixed a latent inconsistency while doing so: the in-process bucket capped its counter at the limit while Redis' INCR kept climbing, so the two backends disagreed about how far over the limit a key was. Both now track the true count. Mail lookup index - Added an index on users.mail (0035). Password reset, e-mail verification and the resend cooldown all resolve a single account from a submitted address and were full table scans of `users`. Deliberately non-unique: legacy rows can hold the same address more than once, so a unique index would fail to apply. Resend captcha - /verify's resend form triggers real outbound mail and was reachable with only a cooldown. It now runs the configured captcha before the account lookup and before any send. Client message payload - The root layout serialised the whole catalogue into every page. pages.admin and admin are ~177 KB of the ~235 KB and are unreachable from the public route group, so that layout now installs its own provider with the staff namespaces removed. Nested providers replace rather than merge, which is why this has to live in the segment layout. /admin, /mod, /client and /admin-next keep the full set; a guard test fails if a public page ever references a staff namespace.
This commit is contained in:
1 parent
6cc45d7413
commit
179484642f
41 files changed
+863
-42
No files matched your search
+17
-1
@@ -11,6 +11,11 @@ import {
|
||||
runDummyHashCheck,
|
||||
verifyLoginPassword,
|
||||
} from "@/lib/auth/login-core";
|
||||
import {
|
||||
clearLoginLockout,
|
||||
isLoginLocked,
|
||||
recordLoginFailure,
|
||||
} from "@/lib/auth/login-lockout";
|
||||
|
||||
export { invalidateLoginCache };
|
||||
|
||||
@@ -59,8 +64,19 @@ export const { handlers, signOut, auth } = NextAuth({
|
||||
return null;
|
||||
}
|
||||
|
||||
// Same per-account lockout the pre-check uses (same key and budget), so
|
||||
// the two paths cannot be used to buy extra attempts, and a client that
|
||||
// skips the pre-check is still bounded. Checked before the password is
|
||||
// verified — clearing on success would unlock a locked account.
|
||||
if (await isLoginLocked(user.id)) return null;
|
||||
|
||||
const res = await verifyLoginPassword(user, password);
|
||||
if (!res.valid) return null;
|
||||
if (!res.valid) {
|
||||
await recordLoginFailure(user.id);
|
||||
return null;
|
||||
}
|
||||
|
||||
await clearLoginLockout(user.id);
|
||||
|
||||
if (await isEmailUnverified(user)) {
|
||||
return null;
|
||||
|
||||
@@ -0,0 +1,69 @@
|
||||
// @ts-nocheck
|
||||
import { beforeEach, describe, expect, it, vi } from "vitest";
|
||||
|
||||
vi.mock("@/lib/redis", () => ({ redis: null }));
|
||||
|
||||
// In-process buckets survive across the module boundary, so every suite uses a
|
||||
// distinct account id to keep the windows independent.
|
||||
import {
|
||||
clearLoginLockout,
|
||||
isLoginLocked,
|
||||
LOGIN_MAX_ATTEMPTS,
|
||||
recordLoginFailure,
|
||||
} from "./login-lockout";
|
||||
|
||||
beforeEach(() => {
|
||||
vi.restoreAllMocks();
|
||||
});
|
||||
|
||||
describe("login lockout", () => {
|
||||
it("starts unlocked", async () => {
|
||||
expect(await isLoginLocked(1)).toBe(false);
|
||||
});
|
||||
|
||||
it("stays unlocked until the attempt budget is exhausted", async () => {
|
||||
for (let i = 0; i < LOGIN_MAX_ATTEMPTS; i++) {
|
||||
expect(await recordLoginFailure(2)).toBe(false);
|
||||
expect(await isLoginLocked(2)).toBe(false);
|
||||
}
|
||||
});
|
||||
|
||||
it("locks on the attempt that exceeds the budget", async () => {
|
||||
for (let i = 0; i < LOGIN_MAX_ATTEMPTS; i++) {
|
||||
await recordLoginFailure(3);
|
||||
}
|
||||
expect(await recordLoginFailure(3)).toBe(true);
|
||||
expect(await isLoginLocked(3)).toBe(true);
|
||||
});
|
||||
|
||||
it("clears on a successful authentication", async () => {
|
||||
for (let i = 0; i < LOGIN_MAX_ATTEMPTS; i++) {
|
||||
await recordLoginFailure(4);
|
||||
}
|
||||
await recordLoginFailure(4);
|
||||
expect(await isLoginLocked(4)).toBe(true);
|
||||
|
||||
await clearLoginLockout(4);
|
||||
|
||||
expect(await isLoginLocked(4)).toBe(false);
|
||||
expect(await recordLoginFailure(4)).toBe(false);
|
||||
});
|
||||
|
||||
it("does not consume an attempt just to check the lock", async () => {
|
||||
for (let i = 0; i < 40; i++) {
|
||||
await isLoginLocked(5);
|
||||
}
|
||||
await recordLoginFailure(5);
|
||||
// Still one failure recorded, not forty.
|
||||
expect(await isLoginLocked(5)).toBe(false);
|
||||
});
|
||||
|
||||
it("keeps buckets per account", async () => {
|
||||
for (let i = 0; i < LOGIN_MAX_ATTEMPTS; i++) {
|
||||
await recordLoginFailure(6);
|
||||
}
|
||||
await recordLoginFailure(6);
|
||||
expect(await isLoginLocked(6)).toBe(true);
|
||||
expect(await isLoginLocked(7)).toBe(false);
|
||||
});
|
||||
});
|
||||
@@ -0,0 +1,53 @@
|
||||
import { clearRateLimit, peekRateLimit, rateLimit } from "@/lib/rate-limit";
|
||||
|
||||
/**
|
||||
* Per-account login lockout.
|
||||
*
|
||||
* The pre-existing limits were keyed on the client IP, so a distributed attack
|
||||
* — or simply a botnet — could grind on a single account indefinitely. This one
|
||||
* is keyed on the resolved account id, which is what actually needs protecting.
|
||||
*
|
||||
* Keying on the id (rather than the submitted string) is deliberate: users may
|
||||
* sign in with either their username or their e-mail, and neither `getLoginUser`
|
||||
* nor `normalizeLoginInput` fold case. Keying on the input would hand an
|
||||
* attacker a fresh budget for every spelling of the same account.
|
||||
*/
|
||||
|
||||
/** Failed attempts tolerated before an account locks. */
|
||||
export const LOGIN_MAX_ATTEMPTS = 8;
|
||||
/** How long a failure stays on the record. */
|
||||
export const LOGIN_LOCKOUT_WINDOW_MS = 15 * 60_000;
|
||||
|
||||
function lockoutKey(userId: number): string {
|
||||
return `login-fail:${userId}`;
|
||||
}
|
||||
|
||||
/**
|
||||
* Whether this account is currently locked. Read-only: the caller must not
|
||||
* have to consume an attempt just to find out.
|
||||
*/
|
||||
export async function isLoginLocked(userId: number): Promise<boolean> {
|
||||
return !(
|
||||
await peekRateLimit(
|
||||
lockoutKey(userId),
|
||||
LOGIN_MAX_ATTEMPTS,
|
||||
LOGIN_LOCKOUT_WINDOW_MS,
|
||||
)
|
||||
).ok;
|
||||
}
|
||||
|
||||
/** Record a failed attempt. Resolves true when this failure trips the lockout. */
|
||||
export async function recordLoginFailure(userId: number): Promise<boolean> {
|
||||
return !(
|
||||
await rateLimit(
|
||||
lockoutKey(userId),
|
||||
LOGIN_MAX_ATTEMPTS,
|
||||
LOGIN_LOCKOUT_WINDOW_MS,
|
||||
)
|
||||
).ok;
|
||||
}
|
||||
|
||||
/** Forget the account's failures after a successful authentication. */
|
||||
export async function clearLoginLockout(userId: number): Promise<void> {
|
||||
await clearRateLimit(lockoutKey(userId));
|
||||
}
|
||||
@@ -0,0 +1,113 @@
|
||||
// @ts-nocheck
|
||||
import { readdirSync, readFileSync, statSync } from "node:fs";
|
||||
import { join } from "node:path";
|
||||
import { describe, expect, it } from "vitest";
|
||||
|
||||
/**
|
||||
* The public route group installs a reduced catalogue (see
|
||||
* `publicClientMessages`). Anything a public page asks for must therefore live
|
||||
* outside the staff-only namespaces — a reference to one of them would silently
|
||||
* fall back to the raw key, so this is enforced here rather than discovered in
|
||||
* production.
|
||||
*/
|
||||
|
||||
const SITE_DIR = join(process.cwd(), "src/app/(site)");
|
||||
const STAFF_NAMESPACES = ['useTranslations("admin', 'getTranslations("admin'];
|
||||
|
||||
function sourceFiles(dir: string): string[] {
|
||||
const out: string[] = [];
|
||||
for (const entry of readdirSync(dir)) {
|
||||
const full = join(dir, entry);
|
||||
if (statSync(full).isDirectory()) {
|
||||
out.push(...sourceFiles(full));
|
||||
} else if (/\.(ts|tsx)$/.test(entry)) {
|
||||
out.push(full);
|
||||
}
|
||||
}
|
||||
return out;
|
||||
}
|
||||
|
||||
describe("public client message scope", () => {
|
||||
it("the site route group has pages to check", () => {
|
||||
expect(sourceFiles(SITE_DIR).length).toBeGreaterThan(10);
|
||||
});
|
||||
|
||||
it("no public page reads a staff-only namespace", () => {
|
||||
const offenders: string[] = [];
|
||||
for (const file of sourceFiles(SITE_DIR)) {
|
||||
const src = readFileSync(file, "utf8");
|
||||
for (const ns of STAFF_NAMESPACES) {
|
||||
// `pages.admin.*` is the second form; `admin.*` the first.
|
||||
for (const needle of [
|
||||
`${ns})`,
|
||||
`${ns}.`,
|
||||
ns.replace("(admin", "(pages.admin"),
|
||||
]) {
|
||||
if (src.includes(needle)) {
|
||||
offenders.push(`${file.replace(process.cwd(), "")}: ${needle}`);
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
expect(offenders).toEqual([]);
|
||||
});
|
||||
});
|
||||
|
||||
describe("publicClientMessages", () => {
|
||||
async function load() {
|
||||
const mod = await import("@/lib/i18n-client-messages");
|
||||
return mod.publicClientMessages;
|
||||
}
|
||||
|
||||
it("strips the staff-only namespaces", async () => {
|
||||
const publicClientMessages = await load();
|
||||
const result = publicClientMessages({
|
||||
admin: { title: "x" },
|
||||
nav: { home: "Home" },
|
||||
pages: {
|
||||
admin: { dashboard: "y" },
|
||||
home: { title: "z" },
|
||||
},
|
||||
});
|
||||
expect(result.admin).toBeUndefined();
|
||||
expect(result.nav).toEqual({ home: "Home" });
|
||||
expect(result.pages.admin).toBeUndefined();
|
||||
expect(result.pages.home).toEqual({ title: "z" });
|
||||
});
|
||||
|
||||
it("keeps every non-admin top-level namespace", async () => {
|
||||
const publicClientMessages = await load();
|
||||
const en = JSON.parse(
|
||||
readFileSync(join(process.cwd(), "src/messages/en.json"), "utf8"),
|
||||
);
|
||||
const result = publicClientMessages(en);
|
||||
for (const key of Object.keys(en)) {
|
||||
if (key === "admin") continue;
|
||||
expect(result[key]).toBeDefined();
|
||||
}
|
||||
expect(result.pages.admin).toBeUndefined();
|
||||
});
|
||||
|
||||
it("actually shrinks the catalogue", async () => {
|
||||
const publicClientMessages = await load();
|
||||
const en = JSON.parse(
|
||||
readFileSync(join(process.cwd(), "src/messages/en.json"), "utf8"),
|
||||
);
|
||||
const full = JSON.stringify(en).length;
|
||||
const scoped = JSON.stringify(publicClientMessages(en)).length;
|
||||
// The staff-tool namespaces dominate the file; without them the payload
|
||||
// should drop by well over half.
|
||||
expect(scoped).toBeLessThan(full * 0.5);
|
||||
});
|
||||
|
||||
it("does not mutate the input", async () => {
|
||||
const publicClientMessages = await load();
|
||||
const en = JSON.parse(
|
||||
readFileSync(join(process.cwd(), "src/messages/en.json"), "utf8"),
|
||||
);
|
||||
const before = JSON.stringify(en).length;
|
||||
publicClientMessages(en);
|
||||
expect(JSON.stringify(en).length).toBe(before);
|
||||
expect(en.pages.admin).toBeDefined();
|
||||
});
|
||||
});
|
||||
@@ -0,0 +1,53 @@
|
||||
import type { AbstractIntlMessages } from "next-intl";
|
||||
|
||||
/**
|
||||
* Client-side message scoping.
|
||||
*
|
||||
* `getMessages()` hands back the whole catalogue, and the root layout feeds all
|
||||
* of it to `NextIntlClientProvider`, which serialises every message into the RSC
|
||||
* payload of every page. The catalogue is ~235 KB of which `pages.admin` and
|
||||
* `admin` together are ~177 KB — and no public page ever reads either.
|
||||
*
|
||||
* Nested `NextIntlClientProvider`s REPLACE the parent's messages rather than
|
||||
* merging them (see use-intl's IntlProvider: `messages ?? parent.messages`), so
|
||||
* a segment cannot ask for "just my part". Instead the public layout installs a
|
||||
* provider seeded with the public subset, which shrinks the payload for every
|
||||
* public page while leaving /admin, /mod, /client and /admin-next on the full set.
|
||||
*/
|
||||
|
||||
/** Top-level namespaces plus `pages.*` sub-namespaces reserved for staff tools. */
|
||||
const ADMIN_ONLY_NAMESPACES = new Set(["admin"]);
|
||||
const ADMIN_ONLY_PAGE_NAMESPACES = new Set(["admin"]);
|
||||
|
||||
/**
|
||||
* Drop the staff-tool namespaces from a full catalogue.
|
||||
*
|
||||
* A denylist rather than an allowlist on purpose: a new public page should work
|
||||
* immediately rather than silently 500 on a missing namespace, and the only
|
||||
* namespaces that must never reach a public client are the two admin ones. A
|
||||
* guard test asserts that no component under `app/(site)` references them.
|
||||
*/
|
||||
export function publicClientMessages(
|
||||
messages: AbstractIntlMessages,
|
||||
): AbstractIntlMessages {
|
||||
const result: AbstractIntlMessages = {};
|
||||
|
||||
for (const [key, value] of Object.entries(messages)) {
|
||||
if (ADMIN_ONLY_NAMESPACES.has(key)) continue;
|
||||
if (key !== "pages") {
|
||||
result[key] = value;
|
||||
continue;
|
||||
}
|
||||
|
||||
const pages: AbstractIntlMessages = {};
|
||||
for (const [pageKey, pageValue] of Object.entries(
|
||||
value as AbstractIntlMessages,
|
||||
)) {
|
||||
if (ADMIN_ONLY_PAGE_NAMESPACES.has(pageKey)) continue;
|
||||
pages[pageKey] = pageValue;
|
||||
}
|
||||
result.pages = pages;
|
||||
}
|
||||
|
||||
return result;
|
||||
}
|
||||
@@ -5,7 +5,7 @@ vi.mock("@/lib/redis", () => ({
|
||||
redis: null,
|
||||
}));
|
||||
|
||||
import { rateLimit } from "./rate-limit";
|
||||
import { clearRateLimit, peekRateLimit, rateLimit } from "./rate-limit";
|
||||
|
||||
beforeEach(() => {
|
||||
vi.restoreAllMocks();
|
||||
@@ -46,3 +46,63 @@ describe("rateLimit (in-memory fallback)", () => {
|
||||
expect(a2.ok).toBe(false);
|
||||
});
|
||||
});
|
||||
|
||||
describe("peekRateLimit", () => {
|
||||
it("reports a fresh bucket as available without consuming a unit", async () => {
|
||||
const key = `peek:fresh:${Date.now()}`;
|
||||
const first = await peekRateLimit(key, 2, 60_000);
|
||||
expect(first.ok).toBe(true);
|
||||
expect(first.retryAfter).toBe(0);
|
||||
});
|
||||
|
||||
it("does not consume a unit", async () => {
|
||||
const key = `peek:noconsume:${Date.now()}`;
|
||||
await peekRateLimit(key, 2, 60_000);
|
||||
await peekRateLimit(key, 2, 60_000);
|
||||
await peekRateLimit(key, 2, 60_000);
|
||||
// Three peeks, budget of two: still allowed, and count is still 0.
|
||||
expect((await peekRateLimit(key, 2, 60_000)).ok).toBe(true);
|
||||
expect((await rateLimit(key, 2, 60_000)).ok).toBe(true);
|
||||
expect((await rateLimit(key, 2, 60_000)).ok).toBe(true);
|
||||
expect((await rateLimit(key, 2, 60_000)).ok).toBe(false);
|
||||
});
|
||||
|
||||
it("reports a bucket that is already over the limit", async () => {
|
||||
const key = `peek:over:${Date.now()}`;
|
||||
await rateLimit(key, 1, 60_000);
|
||||
expect((await peekRateLimit(key, 1, 60_000)).ok).toBe(true);
|
||||
await rateLimit(key, 1, 60_000);
|
||||
const res = await peekRateLimit(key, 1, 60_000);
|
||||
expect(res.ok).toBe(false);
|
||||
expect(res.retryAfter).toBeGreaterThan(0);
|
||||
});
|
||||
|
||||
it("reports an expired bucket as available again", async () => {
|
||||
const key = `peek:expired:${Date.now()}`;
|
||||
await rateLimit(key, 1, 50);
|
||||
await rateLimit(key, 1, 50);
|
||||
expect((await peekRateLimit(key, 1, 50)).ok).toBe(false);
|
||||
await new Promise((r) => setTimeout(r, 60));
|
||||
expect((await peekRateLimit(key, 1, 50)).ok).toBe(true);
|
||||
});
|
||||
});
|
||||
|
||||
describe("clearRateLimit", () => {
|
||||
it("empties the bucket so the limit is fully available again", async () => {
|
||||
const key = `clear:basic:${Date.now()}`;
|
||||
expect((await rateLimit(key, 2, 60_000)).ok).toBe(true);
|
||||
expect((await rateLimit(key, 2, 60_000)).ok).toBe(true);
|
||||
expect((await rateLimit(key, 2, 60_000)).ok).toBe(false);
|
||||
|
||||
await clearRateLimit(key);
|
||||
|
||||
expect((await peekRateLimit(key, 2, 60_000)).ok).toBe(true);
|
||||
expect((await rateLimit(key, 2, 60_000)).ok).toBe(true);
|
||||
});
|
||||
|
||||
it("is a no-op on an unknown key", async () => {
|
||||
await expect(
|
||||
clearRateLimit(`clear:missing:${Date.now()}`),
|
||||
).resolves.toBeUndefined();
|
||||
});
|
||||
});
|
||||
+66
-2
@@ -85,10 +85,14 @@ export async function rateLimit(
|
||||
|
||||
if (!bucket || now >= bucket.resetAt) {
|
||||
buckets.set(windowKey, { count: 1, resetAt: now + windowMs });
|
||||
return { ok: true, retryAfter: 0 };
|
||||
return { ok: limit >= 1, retryAfter: 0 };
|
||||
}
|
||||
|
||||
// The counter keeps climbing past the limit, matching Redis' INCR. Capping
|
||||
// it here would make the two backends disagree about how far over the limit
|
||||
// a key is, which breaks read-only consumers such as the login lockout.
|
||||
const newCount = bucket.count + 1;
|
||||
bucket.count = newCount;
|
||||
if (newCount > limit) {
|
||||
return {
|
||||
ok: false,
|
||||
@@ -96,10 +100,70 @@ export async function rateLimit(
|
||||
};
|
||||
}
|
||||
|
||||
bucket.count = newCount;
|
||||
return { ok: true, retryAfter: 0 };
|
||||
}
|
||||
|
||||
/**
|
||||
* Read-only peek at a bucket — how it would judge the next call WITHOUT
|
||||
* consuming a unit.
|
||||
*
|
||||
* The login lockout needs this: it has to know whether an account is already
|
||||
* locked *before* the password is verified. Verifying first would clear the
|
||||
* counter on the success path, handing a locked account a session the moment
|
||||
* the right password turns up.
|
||||
*/
|
||||
export async function peekRateLimit(
|
||||
key: string,
|
||||
limit: number,
|
||||
windowMs: number,
|
||||
): Promise<RateLimitResult> {
|
||||
if (redis) {
|
||||
try {
|
||||
const [countRaw, ttl] = (await redis.eval(
|
||||
`local c = redis.call('GET', KEYS[1])
|
||||
if not c then c = 0 end
|
||||
local t = redis.call('PTTL', KEYS[1])
|
||||
if t < 0 then t = tonumber(ARGV[1]) end
|
||||
return {c, t}`,
|
||||
1,
|
||||
`ratelimit:${key}`,
|
||||
String(windowMs),
|
||||
)) as [string | null, number];
|
||||
const count = Number(countRaw ?? 0);
|
||||
return {
|
||||
ok: count <= limit,
|
||||
retryAfter: Math.max(1, Math.ceil(ttl / 1000)),
|
||||
};
|
||||
} catch {
|
||||
// Redis unavailable — fall through to the in-process bucket.
|
||||
}
|
||||
}
|
||||
|
||||
const bucket = buckets.get(`mem:${key}`);
|
||||
if (!bucket || Date.now() >= bucket.resetAt) {
|
||||
return { ok: true, retryAfter: 0 };
|
||||
}
|
||||
return {
|
||||
ok: bucket.count <= limit,
|
||||
retryAfter: Math.max(1, Math.ceil((bucket.resetAt - Date.now()) / 1000)),
|
||||
};
|
||||
}
|
||||
|
||||
/**
|
||||
* Drop a bucket entirely, both in Redis and in-process. Required wherever a
|
||||
* successful action must reset a counter — a failed-attempt budget is only
|
||||
* usable if a success clears it.
|
||||
*/
|
||||
export async function clearRateLimit(key: string): Promise<void> {
|
||||
buckets.delete(`mem:${key}`);
|
||||
if (!redis) return;
|
||||
try {
|
||||
await redis.del(`ratelimit:${key}`);
|
||||
} catch {
|
||||
// Best effort: a stale key only re-arms the lockout for its own TTL.
|
||||
}
|
||||
}
|
||||
|
||||
export async function clientIp(): Promise<string> {
|
||||
try {
|
||||
return resolveClientIp(await headers());
|
||||
|
||||
Reference in new issue
Block a user