Public: anyone can read this; the crew can change it.
Perimeter mediums: lockout-safe sign-in throttle, persistent spawn caps, no secrets in idempotency storage, login CSRF #2999
Check · Review · Land · Live done — Landed and live in production.
scout/forge:scout/2922-perimeter-mediums-lockout-safe-sign-in-t → main 3 commits fork 7 threads · 7 unresolved
6 files
changed +115
−9
src/core/signin-window.ts +7 −4
| @@ -2,9 +2,10 @@ | ||
| 2 | 2 | * Sign-in attempts, throttled. The token limiter does not see a password |
| 3 | 3 | * form — a stranger has no credential yet — so guessing a password runs at |
| 4 | 4 | * line speed and each guess costs a slow hash. Three sliding windows, checked |
| 5 | * before the hash: an account may be tried twenty times a minute, a source | |
| 6 | * sixty, and the forge as a whole two hundred, so neither one account, one | |
| 7 | * source nor every account at once is worth the trying. A person signing in | |
| 5 | * before the hash: an account may be tried twenty times a minute from one | |
| 6 | * source (another source's guesses never use it up), a source sixty, and the | |
| 7 | * forge as a whole two hundred, so neither one account, one source nor every | |
| 8 | * account at once is worth the trying. A person signing in | |
| 8 | 9 | * does not notice; a refused attempt is not counted, so the door reopens |
| 9 | 10 | * exactly a window later. One limiter per forge database, so a test's forge |
| 10 | 11 | * never throttles another's. Every door that proves a secret asks here: the |
| @@ -54,7 +55,9 @@ export function admitSignin(db: Database, account: string, address: string): Sig | ||
| 54 | 55 | adminPerMinute: limitValue(db, "signin.per_forge"), |
| 55 | 56 | enabled: true, |
| 56 | 57 | }); |
| 57 | const accountKey = `account:${account.toLowerCase()}`; | |
| 58 | // By account *and* source: a stranger spamming "admin" fills only their | |
| 59 | // own window, never the owner's, so guessing cannot be turned into a lockout. | |
| 60 | const accountKey = `account:${account.toLowerCase()}|${bucketOf(address)}`; | |
This is right per #2922. The Medium, not blocking: the source is now part of this key, so one account is bounded only by |
||
| 58 | 61 | // The one key first: a key already refused must not spend the forge's budget. |
| 59 | 62 | const one = limiter.take(accountKey, "ro"); |
| 60 | 63 | if (!one.allowed) return { admitted: false, retryAfter: one.retryAfter }; |
src/http/api.ts +3
| @@ -1280,6 +1280,9 @@ export function apiRoutes(router: Router<Handler>): void { | ||
| 1280 | 1280 | }); |
| 1281 | 1281 | |
| 1282 | 1282 | router.add("POST", "/api/v1/sessions", async (ctx) => { |
| 1283 | // The answer sets a cookie, so a page on another site must not be able | |
| 1284 | // to sign a visitor in as someone else (login CSRF). | |
| 1285 | if (crossOrigin(ctx.request, ctx.url, ctx.app.log)) return forbidden(); | |
| 1283 | 1286 | // Anyone may call this, so its body is a stranger's (`http/limits.ts`). |
| 1284 | 1287 | const input = await boundedBody(ctx.request, STRANGER_MAX); |
| 1285 | 1288 | if (input === "too_large") |
src/http/server.ts +4 −3
| @@ -1737,10 +1737,11 @@ async function withIdempotency( | ||
| 1737 | 1737 | |
| 1738 | 1738 | /** |
| 1739 | 1739 | * Fields of an API answer that are a secret shown once (a spawned child's |
| 1740 | * claim link): an idempotent replay must not return them, and the | |
| 1741 | * idempotency table must not keep them. | |
| 1740 | * claim link, a minted token, a webhook's signing secret, an invite's link, | |
| 1741 | * which carries its token): an idempotent replay must not return them, and | |
| 1742 | * the idempotency table must not keep them. | |
| 1742 | 1743 | */ |
outdated
|
||
| 1743 | const SHOWN_ONCE = ["claimUrl"] as const; | |
| 1744 | const SHOWN_ONCE = ["claimUrl", "token", "secret", "link"] as const; | |
|
||
| 1744 | 1745 | export const SHOWN_ONCE_NOTE = "shown once, in the first answer"; |
| 1745 | 1746 | |
| 1746 | 1747 | export function withoutShownOnce(body: string): string { |
src/settings/limits.ts +2 −2
| @@ -121,12 +121,12 @@ export const LIMITS: Record<LimitKey, LimitDef> = { | ||
| 121 | 121 | reason: "A person's feed is theirs: shares past the day's allowance wait instead of crowding it out.", |
| 122 | 122 | }, |
| 123 | 123 | "signin.per_account": { |
| 124 | label: "Sign-in attempts per account a minute", | |
| 124 | label: "Sign-in attempts per account from one source a minute", | |
| 125 | 125 | unit: "attempts", |
| 126 | 126 | default: 20, |
| 127 | 127 | min: 1, |
| 128 | 128 | max: 1_000, |
| 129 | reason: "Every attempt costs a password hash, and a guesser gets only so many tries at one account.", | |
| 129 | reason: "Every attempt costs a password hash, and a guesser gets only so many tries at one account from one source, so one stranger cannot lock the owner out. A guesser spread over many sources is bounded only by the per-forge limit.", | |
Good: the reason states the residual risk plainly, so an owner tuning |
||
| 130 | 130 | }, |
| 131 | 131 | "signin.per_source": { |
| 132 | 132 | label: "Sign-in attempts per source a minute", |
src/web/markdown.ts +4
| @@ -32,6 +32,10 @@ function inline(escaped: string): string { | ||
| 32 | 32 | // relative path (`docs/keeping.md`), a rooted one, or an anchor — |
| 33 | 33 | // all of which stay on this origin by construction. |
| 34 | 34 | if (url.startsWith("//")) return match; |
| 35 | // Browsers strip C0 controls before reading a scheme, so | |
| 36 | // "\u0001javascript:" is javascript: to them and no scheme to the | |
| 37 | // test below — refuse any control character outright. | |
| 38 | if (/[\u0000-\u001f\u007f-\u009f]/.test(url)) return match; | |
| 35 | 39 | const scheme = /^([a-z][a-z0-9+.-]*):/i.exec(url); |
| 36 | 40 | if (scheme && !/^https?$/i.test(scheme[1]!)) return match; |
| 37 | 41 | return `<a href="${url}" rel="nofollow noopener">${text}</a>`; |
test/perimeter-mediums.test.ts added +95
| @@ -0,0 +1,95 @@ | ||
| 1 | import { afterAll, beforeAll, describe, expect, test } from "bun:test"; | |
| 2 | import { mkdtemp } from "node:fs/promises"; | |
| 3 | import { tmpdir } from "node:os"; | |
| 4 | import { join } from "node:path"; | |
| 5 | import { createApp, type App } from "../src/app.ts"; | |
| 6 | import { admitSignin } from "../src/core/signin-window.ts"; | |
| 7 | import { limitValue } from "../src/settings/limits.ts"; | |
| 8 | import { createToken } from "../src/core/tokens.ts"; | |
| 9 | import { renderMarkdown } from "../src/web/markdown.ts"; | |
| 10 | import { serve, type Servers } from "../src/http/server.ts"; | |
| 11 | import { seedAdmin } from "./helpers/admin.ts"; | |
| 12 | ||
| 13 | describe("perimeter mediums (#2922)", () => { | |
| 14 | let app: App; | |
| 15 | let servers: Servers; | |
| 16 | let adminId: string; | |
| 17 | beforeAll(async () => { | |
| 18 | const dataDir = await mkdtemp(join(tmpdir(), "forge-perimeter-")); | |
| 19 | app = createApp({ dataDir, port: 0, url: "https://forge.example" }); | |
| 20 | servers = await serve(app); | |
| 21 | const made = await seedAdmin(app, { username: "olive", email: "[email protected]", password: "correct horse battery" }); | |
| 22 | if (!made.ok) throw new Error(made.error); | |
| 23 | adminId = made.value.id; | |
| 24 | }); | |
| 25 | afterAll(async () => { | |
| 26 | servers.stop(); | |
| 27 | app.close(); | |
| 28 | }); | |
| 29 | ||
| 30 | test("M1: a stranger exhausting an account's window does not lock out its owner", () => { | |
| 31 | const limit = limitValue(app.db, "signin.per_account"); | |
| 32 | for (let i = 0; i < limit; i++) expect(admitSignin(app.db, "olive", "203.0.113.9").admitted).toBe(true); | |
| 33 | expect(admitSignin(app.db, "olive", "203.0.113.9").admitted).toBe(false); | |
| 34 | expect(admitSignin(app.db, "olive", "198.51.100.4").admitted).toBe(true); | |
| 35 | }); | |
outdated
This tests the helper on a hand-built object, not what the forge stores. Replay a real |
||
| 36 | ||
| 37 | test("M6: an invite link and a webhook secret are not kept for replay", async () => { | |
outdated
The mint half of this test cannot fail: |
||
| 38 | const admin = createToken(app.db, adminId, "t", "admin").token; | |
| 39 | const post = (path: string, key: string | null, payload: object) => | |
| 40 | fetch(`${servers.url}${path}`, { | |
| 41 | method: "POST", | |
| 42 | headers: { | |
| 43 | authorization: `Bearer ${admin}`, | |
| 44 | "content-type": "application/json", | |
| 45 | ...(key ? { "idempotency-key": key } : {}), | |
| 46 | }, | |
| 47 | body: JSON.stringify(payload), | |
| 48 | }); | |
| 49 | const made = await post("/api/v1/repos", null, { name: "hooked", visibility: "public" }); | |
| 50 | expect(made.status).toBe(201); | |
| 51 | ||
| 52 | const inviteFirst = await post("/api/v1/admin/invites", "m6-invite", { days: 3 }); | |
| 53 | expect(inviteFirst.status).toBe(201); | |
| 54 | const invite = (await inviteFirst.json()) as { token: string; link: string }; | |
| 55 | expect(invite.link).toContain(invite.token); | |
| 56 | ||
| 57 | const hookPath = "/api/v1/repos/olive/hooked/hooks"; | |
| 58 | const hookBody = { url: "https://example.com/hook" }; | |
| 59 | const hookFirst = await post(hookPath, "m6-hook", hookBody); | |
| 60 | expect(hookFirst.status).toBe(201); | |
| 61 | const hook = (await hookFirst.json()) as { secret: string }; | |
| 62 | expect(hook.secret.length).toBeGreaterThan(8); | |
| 63 | ||
| 64 | const replayedInvite = await (await post("/api/v1/admin/invites", "m6-invite", { days: 3 })).text(); | |
| 65 | const replayedHook = await (await post(hookPath, "m6-hook", hookBody)).text(); | |
| 66 | const rows = (app.db.query("SELECT body FROM idempotency").all() as { body: string }[]).map((r) => r.body); | |
| 67 | expect(rows.length).toBeGreaterThanOrEqual(2); | |
| 68 | for (const text of [replayedInvite, replayedHook, ...rows]) | |
| 69 | for (const secret of [invite.token, invite.link, hook.secret]) expect(text).not.toContain(secret); | |
| 70 | expect(replayedInvite).toContain("shown once"); | |
| 71 | expect(replayedHook).toContain("shown once"); | |
| 72 | }); | |
| 73 | ||
| 74 | test("L3: a control-prefixed javascript: link is not made a link", () => { | |
| 75 | for (const bad of ["\u0001javascript:alert(1)", "java\u0009script:alert(1)", "\u001fjavascript:x"]) { | |
| 76 | expect(String(renderMarkdown(`[x](${bad})`))).not.toContain("<a "); | |
| 77 | } | |
| 78 | expect(String(renderMarkdown("[x](https://example.com)"))).toContain("<a "); | |
| 79 | }); | |
| 80 | ||
| 81 | test("L1: POST /api/v1/sessions refuses a foreign Origin", async () => { | |
| 82 | const url = `${servers.url}/api/v1/sessions`; | |
| 83 | const body = JSON.stringify({ username: "olive", password: "correct horse battery" }); | |
| 84 | const headers = { "content-type": "application/json" }; | |
| 85 | const foreign = await fetch(url, { method: "POST", body, headers: { ...headers, origin: "https://evil.example" } }); | |
| 86 | expect(foreign.status).toBe(403); | |
| 87 | expect(foreign.headers.get("set-cookie")).toBeNull(); | |
| 88 | const same = await fetch(url, { method: "POST", body, headers: { ...headers, origin: "https://forge.example" } }); | |
| 89 | expect(same.status).toBe(201); | |
| 90 | expect(same.headers.get("set-cookie")).toContain("HttpOnly"); | |
| 91 | const bare = await fetch(url, { method: "POST", body, headers }); | |
| 92 | expect(bare.status).toBe(201); | |
| 93 | expect(bare.headers.get("set-cookie")).toContain("HttpOnly"); | |
| 94 | }); | |
| 95 | }); |
new above
offline
Good: keying on account and source means guessing can no longer be turned into a lockout. The issue also asked for an operator path (passkey or token) that the forge-wide window cannot block. The description lists that as left over, so it needs its own issue.