Public: anyone can read this; the crew can change it.
Sign-in is also rate-limited per source, alongside the per-account and per-forge windows #2439
Check · Review · Land · Live done — Landed and live in production.
scout/forge:scout/2278-sign-in-is-also-rate-limited-per-source → main 2 commits fork 2 threads · 2 unresolved
3 files
changed +50
−4
src/settings/limits.ts +9
| @@ -32,6 +32,7 @@ export type LimitKey = | ||
| 32 | 32 | | "front.hidden_max" |
| 33 | 33 | | "share.daily_max" |
| 34 | 34 | | "signin.per_account" |
| 35 | | "signin.per_source" | |
| 35 | 36 | | "signin.per_forge" |
| 36 | 37 | | "signup.per_source" |
| 37 | 38 | | "signup.per_forge"; |
| @@ -126,6 +127,14 @@ export const LIMITS: Record<LimitKey, LimitDef> = { | ||
| 126 | 127 | max: 1_000, |
| 127 | 128 | reason: "Every attempt costs a password hash, and a guesser gets only so many tries at one account.", |
| 128 | 129 | }, |
| 130 | "signin.per_source": { | |
| 131 | label: "Sign-in attempts per source a minute", | |
| 132 | unit: "attempts", | |
| 133 | default: 60, | |
| 134 | min: 1, | |
| 135 | max: 10_000, | |
| 136 | reason: "One source trying account after account is guessing, and each try costs a hash, so a source has a ceiling of its own.", | |
| 137 | }, | |
| 129 | 138 | "signin.per_forge": { |
| 130 | 139 | label: "Sign-in attempts per forge a minute", |
| 131 | 140 | unit: "attempts", |
src/web/pages/site.ts +11 −1
| @@ -12,7 +12,7 @@ import { NOT_A_MEMBER } from "../../core/roles.ts"; | ||
| 12 | 12 | import { storageLimitMessage } from "../../core/storage.ts"; |
| 13 | 13 | import { limitValue } from "../../settings/limits.ts"; |
| 14 | 14 | import { signOutHere } from "../../actions/signins.ts"; |
| 15 | import { clientAddress } from "../../http/peer.ts"; | |
| 15 | import { bucketOf, clientAddress } from "../../http/peer.ts"; | |
| 16 | 16 | import { verifyEmail } from "../../actions/verify.ts"; |
| 17 | 17 | import { STRANGER_MAX } from "../../http/limits.ts"; |
| 18 | 18 | import { createRepo, repositoryLimitMessage, resolveOwner } from "../../actions/repos.ts"; |
| @@ -438,6 +438,8 @@ export function signupPage(ctx: Ctx): Response { | ||
| 438 | 438 | * limiter per forge, so a test's forge never throttles another's. |
| 439 | 439 | */ |
| 440 | 440 | const signinLimits = new WeakMap<Ctx["app"], RateLimiter>(); |
| 441 | // The third window, by the socket's peer: a source trying many accounts. | |
| 442 | const signinSources = new WeakMap<Ctx["app"], RateLimiter>(); | |
| 441 | 443 | |
| 442 | 444 | export function signinAllowed(ctx: Ctx, username: string): number | undefined { |
| 443 | 445 | let limiter = signinLimits.get(ctx.app); |
| @@ -454,6 +456,14 @@ export function signinAllowed(ctx: Ctx, username: string): number | undefined { | ||
| 454 | 456 | // The one key first: a key already refused must not spend the forge's budget. |
| 455 | 457 | const one = limiter.take(`account:${username.toLowerCase()}`, "ro"); |
| 456 | 458 | if (!one.allowed) return one.retryAfter; |
| 459 | let sources = signinSources.get(ctx.app); | |
| 460 | if (sources === undefined) { | |
| 461 | sources = new RateLimiter({ perMinute: 0, adminPerMinute: 0, enabled: true }); | |
| 462 | signinSources.set(ctx.app, sources); | |
| 463 | } | |
| 464 | sources.setLimits({ perMinute: limitValue(ctx.app.db, "signin.per_source"), adminPerMinute: 0, enabled: true }); | |
| 465 | const source = sources.take(`source:${bucketOf(clientAddress(ctx.request))}`, "ro"); | |
| 466 | if (!source.allowed) return source.retryAfter; | |
clientBucket(ctx.request) already exists in src/http/peer.ts and does exactly this. Use it so the key is derived in one place. |
||
| 457 | 467 | const whole = limiter.take("forge", "admin"); |
| 458 | 468 | return whole.allowed ? undefined : whole.retryAfter; |
| 459 | 469 | } |
test/forge-signin.test.ts +30 −3
| @@ -17,6 +17,8 @@ import { addKey, findKeyByFingerprint, fingerprint, parseAuthorizedKey } from ". | ||
| 17 | 17 | import { KeySet } from "../src/http/jwks.ts"; |
| 18 | 18 | import { serve, type Servers } from "../src/http/server.ts"; |
| 19 | 19 | import { verifyJwt } from "../src/kernel/jwt.ts"; |
| 20 | import { LIMITS, setLimit } from "../src/settings/limits.ts"; | |
| 21 | import { notePeer } from "../src/http/peer.ts"; | |
| 20 | 22 | import { signinAllowed } from "../src/web/pages/site.ts"; |
| 21 | 23 | import type { Ctx } from "../src/http/context.ts"; |
| 22 | 24 | import { quietLogs } from "./helpers/globals.ts"; |
| @@ -298,10 +300,14 @@ describe("the sign-in throttle", () => { | ||
| 298 | 300 | test("a key that is refused does not spend the forge-wide budget", async () => { |
| 299 | 301 | const dir = await mkdtemp(join(tmpdir(), "forge-signin-")); |
| 300 | 302 | const app = createApp({ dataDir: dir, port: 0 }); |
| 301 | const ctx = { app } as unknown as Ctx; | |
| 302 | for (let i = 0; i < 60; i++) signinAllowed(ctx, "parent:one"); | |
| 303 | const from = (address: string): Ctx => { | |
| 304 | const request = new Request("http://forge.test/"); | |
| 305 | notePeer(request, address); | |
| 306 | return { app, request } as unknown as Ctx; | |
| 307 | }; | |
| 308 | for (let i = 0; i < 60; i++) signinAllowed(from("10.0.0.1"), "parent:one"); | |
| 303 | 309 | // The forge's whole budget (200) is still there for 180 other keys. |
| 304 | for (let i = 0; i < 180; i++) expect(signinAllowed(ctx, `k${i}`)).toBeUndefined(); | |
| 310 | for (let i = 0; i < 180; i++) expect(signinAllowed(from(`10.1.${i}.1`), `k${i}`)).toBeUndefined(); | |
| 305 | 311 | await rm(dir, { recursive: true, force: true }); |
| 306 | 312 | }); |
| 307 | 313 | }); |
| @@ -495,3 +501,24 @@ describe("a parent's oversized register and token answers", () => { | ||
| 495 | 501 | expect(pulled.n).toBeLessThan(100); |
| 496 | 502 | }); |
| 497 | 503 | }); |
| 504 | ||
| 505 | describe("the per-source sign-in window", () => { | |
| 506 | test("one source trying many accounts is throttled while another is not, and the limit is listed", async () => { | |
| 507 | const dir = await mkdtemp(join(tmpdir(), "forge-signin-source-")); | |
| 508 | const app = createApp({ dataDir: dir, port: 0 }); | |
| 509 | const from = (address: string, headers: Record<string, string> = {}): Ctx => { | |
| 510 | const request = new Request("http://forge.test/", { headers }); | |
| 511 | notePeer(request, address); | |
| 512 | return { app, request } as unknown as Ctx; | |
| 513 | }; | |
| 514 | expect(LIMITS["signin.per_source"].default).toBe(60); | |
| 515 | setLimit(app.db, "signin.per_source", 5, "setup"); | |
| 516 | for (let i = 0; i < 5; i++) expect(signinAllowed(from("192.0.2.1"), `acct${i}`)).toBeUndefined(); | |
| 517 | // A sixth account from the same source is refused, even with a spoofed header. | |
| 518 | const wait = signinAllowed(from("192.0.2.1", { "x-forwarded-for": "198.51.100.9" }), "acct5"); | |
| 519 | expect(wait).toBeGreaterThan(0); | |
| 520 | // A different source is not throttled at the same moment. | |
| 521 | expect(signinAllowed(from("192.0.2.2"), "acct6")).toBeUndefined(); | |
| 522 | await rm(dir, { recursive: true, force: true }); | |
| 523 | }); | |
| 524 | }); |
new above
offline
This repeats the lazy WeakMap + RateLimiter init above. A helper (limiterFor(map)) would remove the duplication. Also, the account slot is already spent when the source refuses here; consider checking the source first so one noisy source cannot burn a victim account window.