diff --git a/docs/TODO.md b/docs/TODO.md index 0fe49a0..a2acafa 100644 --- a/docs/TODO.md +++ b/docs/TODO.md @@ -59,9 +59,9 @@ Do these in order. Each task is a thin end-to-end slice: test first, implement, - [x] Tests: each TTL rule, invalid TTLs, a lazy 404 with a faked clock, the sweep deleting only expired rows ## 8. Rate limiting -- [ ] Middleware applies `CREATE_LIMITER` to POST and `WRITE_LIMITER` to PUT/DELETE, keyed on `CF-Connecting-IP` -- [ ] Over the limit → `429 rate_limited` + `Retry-After`; `RATE_LIMIT_ENABLED=false` skips the check -- [ ] Tests: use a stub limiter binding to check both branches and the disabled flag +- [x] Middleware applies `CREATE_LIMITER` to POST and `WRITE_LIMITER` to PUT/DELETE, keyed on `CF-Connecting-IP` +- [x] Over the limit → `429 rate_limited` + `Retry-After`; `RATE_LIMIT_ENABLED=false` skips the check +- [x] Tests: use a stub limiter binding to check both branches and the disabled flag ## 9. CORS - [ ] Hono `cors()` driven by `CORS_ORIGINS` (`*` or an allowlist); methods, allowed headers and exposed headers as in api.md diff --git a/docs/api.md b/docs/api.md index c86c193..45b5841 100644 --- a/docs/api.md +++ b/docs/api.md @@ -78,7 +78,7 @@ Browsers can read all of these via `Access-Control-Expose-Headers`, along with ` ## Validation and errors -Check in this order: route/id → auth (non-create routes) → `X-Bin-TTL` (create only) → content type → size → JSON parse → top-level type. `X-Bin-TTL` is a header, so a bad one is rejected before any of the body is read. +Check in this order: rate limit (`POST`, `PUT`, `DELETE`) → route/id → auth (non-create routes) → `X-Bin-TTL` (create only) → content type → size → JSON parse → top-level type. The rate limit comes first because it protects the Worker: a limited client gets no D1 lookup, key check or body parsing. `X-Bin-TTL` is a header, so a bad one is rejected before any of the body is read. | Status | `code` | When | |---|---|---| @@ -117,6 +117,9 @@ Two Workers Rate Limiting bindings, keyed on `CF-Connecting-IP`: Reads have no limit in the MVP. Setting `RATE_LIMIT_ENABLED=false` turns both off. The binding's period can only be 10 s or 60 s. +- Over the limit → `429 rate_limited` with `Retry-After: `. A binding can't report when its window resets, so `Retry-After` is the binding's whole period (60 s). +- Requests without `CF-Connecting-IP` (Cloudflare always sends it) share a single bucket, keyed `unknown`, rather than going unlimited. + ## CORS `CORS_ORIGINS` is `*` (the default) or a comma-separated allowlist. Allowed methods: `GET, POST, PUT, DELETE, OPTIONS`. Allowed headers: `Authorization, Content-Type, X-Bin-Private, X-Bin-TTL`. The docs must warn that a Write Key embedded in frontend code gives every visitor write access. diff --git a/src/bins/rate-limit.ts b/src/bins/rate-limit.ts new file mode 100644 index 0000000..f848aa1 --- /dev/null +++ b/src/bins/rate-limit.ts @@ -0,0 +1,36 @@ +import { createMiddleware } from "hono/factory"; +import type { AppEnv } from "../context"; +import { errorResponse } from "../errors"; + +/** + * The period of both limiter bindings, in seconds: keep it equal to `ratelimits[].simple.period` in wrangler.jsonc. + * A binding can't report when its window resets, so `Retry-After` sends the whole period. + */ +export const RATE_LIMIT_PERIOD_SECONDS = 60; + +/** + * The limiter key for a request without `CF-Connecting-IP` (Cloudflare always sets it, so only local or unusual + * traffic lacks it). All such requests share this one bucket rather than going unlimited. + */ +export const MISSING_IP_KEY = "unknown"; + +/** The rate limit bindings in wrangler.jsonc. */ +export type LimiterBinding = "CREATE_LIMITER" | "WRITE_LIMITER"; + +/** + * Checks `binding` for the client's IP and answers `429 rate_limited` with `Retry-After` when it is over the limit + * (docs/api.md → Rate limiting). Needs `withConfig` first. With `RATE_LIMIT_ENABLED=false` the limiter isn't called. + */ +export function rateLimit(binding: LimiterBinding) { + return createMiddleware(async (c, next) => { + if (c.var.config.rateLimitEnabled) { + const key = c.req.header("CF-Connecting-IP")?.trim() || MISSING_IP_KEY; + const { success } = await c.env[binding].limit({ key }); + if (!success) { + c.header("Retry-After", String(RATE_LIMIT_PERIOD_SECONDS)); + return errorResponse(c, 429, "rate_limited", "Too many requests: try again later"); + } + } + await next(); + }); +} diff --git a/src/bins/routes.ts b/src/bins/routes.ts index 679ad0c..5d2a378 100644 --- a/src/bins/routes.ts +++ b/src/bins/routes.ts @@ -6,12 +6,18 @@ import { parseBinBody } from "./body"; import { resolveExpiresAt } from "./expiry"; import { generateBinId, generateReadKey, generateWriteKey, hashKey, isValidBinId } from "./keys"; import { binMetadataHeaders, toIso } from "./metadata"; +import { rateLimit } from "./rate-limit"; import { type Bin, findLive, insert, remove, replaceContent } from "./repository"; export const binRoutes = new OpenAPIHono(); binRoutes.use(withConfig); +// Rate limiting comes first, before the id, auth and body checks (docs/api.md → Validation and errors): it protects +// the Worker, so a limited client shouldn't get D1 lookups, key hashing or body parsing. Reads aren't limited. +binRoutes.post("/", rateLimit("CREATE_LIMITER")); +binRoutes.on(["PUT", "DELETE"], "/:id", rateLimit("WRITE_LIMITER")); + binRoutes.post("/", async (c) => { const now = Date.now(); // X-Bin-TTL is checked before the body (docs/api.md → Validation and errors): it is a header, so a bad one is diff --git a/test/rate-limit.test.ts b/test/rate-limit.test.ts new file mode 100644 index 0000000..d24f3d7 --- /dev/null +++ b/test/rate-limit.test.ts @@ -0,0 +1,243 @@ +import { createExecutionContext, waitOnExecutionContext } from "cloudflare:test"; +import { env } from "cloudflare:workers"; +import { beforeEach, describe, expect, it } from "vitest"; +import { generateBinId, generateWriteKey, hashKey } from "../src/bins/keys"; +import { MISSING_IP_KEY, RATE_LIMIT_PERIOD_SECONDS } from "../src/bins/rate-limit"; +import { type Bin, findLive, insert } from "../src/bins/repository"; +import { app } from "../src/index"; + +const BASE_URL = "https://keepjson.test"; +const CLIENT_IP = "203.0.113.7"; + +/** A limiter binding that allows or denies every call and remembers the keys it was called with. */ +type StubLimiter = RateLimit & { keys: string[] }; + +function stubLimiter(success: boolean): StubLimiter { + const keys: string[] = []; + return { + keys, + async limit({ key }) { + keys.push(key); + return { success }; + }, + }; +} + +type Limiters = { create: StubLimiter; write: StubLimiter }; + +function limiters(success: boolean): Limiters { + return { create: stubLimiter(success), write: stubLimiter(success) }; +} + +/** + * Calls the app directly, so the limiter bindings can be stubs and rate limiting can be turned on: the test worker + * itself runs with RATE_LIMIT_ENABLED=false (vitest.config.ts). + */ +async function send( + stubs: Limiters, + path: string, + init: RequestInit = {}, + options: { enabled?: boolean; ip?: string | null } = {}, +): Promise { + const headers = new Headers(init.headers); + const ip = options.ip === undefined ? CLIENT_IP : options.ip; + if (ip !== null) headers.set("CF-Connecting-IP", ip); + const testEnv: Env = { + ...env, + RATE_LIMIT_ENABLED: String(options.enabled ?? true), + CREATE_LIMITER: stubs.create, + WRITE_LIMITER: stubs.write, + }; + const ctx = createExecutionContext(); + const res = await app.fetch(new Request(`${BASE_URL}${path}`, { ...init, headers }), testEnv, ctx); + await waitOnExecutionContext(ctx); + return res; +} + +function postBin(stubs: Limiters, body = '{"keep": true}', options?: { enabled?: boolean; ip?: string | null }) { + return send(stubs, "/bins", { method: "POST", headers: { "Content-Type": "application/json" }, body }, options); +} + +function putBin(stubs: Limiters, id: string, writeKey: string, body = '{"replaced": true}') { + return send(stubs, `/bins/${id}`, { + method: "PUT", + headers: { "Content-Type": "application/json", Authorization: `Bearer ${writeKey}` }, + body, + }); +} + +function deleteBin(stubs: Limiters, id: string, writeKey: string) { + return send(stubs, `/bins/${id}`, { method: "DELETE", headers: { Authorization: `Bearer ${writeKey}` } }); +} + +/** Inserts a public Bin through the repository, returning it with its plaintext Write Key. */ +async function storeBin(): Promise<{ bin: Bin; writeKey: string }> { + const writeKey = generateWriteKey(); + const now = Date.now(); + const bin: Bin = { + id: generateBinId(), + content: '{"keep": true}', + isPrivate: false, + writeKeyHash: await hashKey(writeKey), + readKeyHash: null, + createdAt: now, + updatedAt: now, + expiresAt: null, + }; + await insert(env.DB, bin); + return { bin, writeKey }; +} + +async function countBins(): Promise { + const row = await env.DB.prepare("SELECT COUNT(*) AS n FROM bins").first<{ n: number }>(); + return row?.n ?? 0; +} + +async function expectRateLimited(res: Response) { + expect(res.status).toBe(429); + expect(res.headers.get("Retry-After")).toBe(String(RATE_LIMIT_PERIOD_SECONDS)); + expect(res.headers.get("Cache-Control")).toBe("no-store"); + expect(res.headers.has("X-Bin-Id")).toBe(false); + expect(await res.json()).toEqual({ error: { code: "rate_limited", message: expect.any(String) } }); +} + +beforeEach(async () => { + await env.DB.exec("DELETE FROM bins"); +}); + +describe("the test worker", () => { + it("runs with rate limiting turned off, so other test files can create many Bins", () => { + expect(env.RATE_LIMIT_ENABLED).toBe("false"); + }); +}); + +describe("rate limiting", () => { + it("sends the Retry-After period of the limiter bindings in wrangler.jsonc", () => { + expect(RATE_LIMIT_PERIOD_SECONDS).toBe(60); + }); + + describe("under the limit", () => { + it("creates a Bin, checking CREATE_LIMITER with the client's IP", async () => { + const stubs = limiters(true); + const res = await postBin(stubs); + expect(res.status).toBe(201); + expect(stubs.create.keys).toEqual([CLIENT_IP]); + expect(stubs.write.keys).toEqual([]); + expect(await countBins()).toBe(1); + }); + + it("replaces a Bin, checking WRITE_LIMITER with the client's IP", async () => { + const stubs = limiters(true); + const { bin, writeKey } = await storeBin(); + const res = await putBin(stubs, bin.id, writeKey); + expect(res.status).toBe(200); + expect(stubs.write.keys).toEqual([CLIENT_IP]); + expect(stubs.create.keys).toEqual([]); + expect((await findLive(env.DB, bin.id, Date.now()))?.content).toBe('{"replaced": true}'); + }); + + it("deletes a Bin, checking WRITE_LIMITER with the client's IP", async () => { + const stubs = limiters(true); + const { bin, writeKey } = await storeBin(); + const res = await deleteBin(stubs, bin.id, writeKey); + expect(res.status).toBe(204); + expect(stubs.write.keys).toEqual([CLIENT_IP]); + expect(stubs.create.keys).toEqual([]); + expect(await findLive(env.DB, bin.id, 0)).toBeNull(); + }); + + it("uses one shared fallback key when CF-Connecting-IP is missing", async () => { + const stubs = limiters(true); + const res = await postBin(stubs, "{}", { ip: null }); + expect(res.status).toBe(201); + expect(stubs.create.keys).toEqual([MISSING_IP_KEY]); + }); + }); + + describe("over the limit", () => { + it("rejects POST /bins with 429 rate_limited and creates nothing", async () => { + const stubs = limiters(false); + await expectRateLimited(await postBin(stubs)); + expect(stubs.create.keys).toEqual([CLIENT_IP]); + expect(await countBins()).toBe(0); + }); + + it("rejects PUT /bins/:id with 429 rate_limited and leaves the Bin unchanged", async () => { + const stubs = limiters(false); + const { bin, writeKey } = await storeBin(); + await expectRateLimited(await putBin(stubs, bin.id, writeKey)); + expect(stubs.write.keys).toEqual([CLIENT_IP]); + expect(await findLive(env.DB, bin.id, Date.now())).toEqual(bin); + }); + + it("rejects DELETE /bins/:id with 429 rate_limited and keeps the Bin", async () => { + const stubs = limiters(false); + const { bin, writeKey } = await storeBin(); + await expectRateLimited(await deleteBin(stubs, bin.id, writeKey)); + expect(stubs.write.keys).toEqual([CLIENT_IP]); + expect(await findLive(env.DB, bin.id, Date.now())).toEqual(bin); + }); + + it("checks the limit before the body is validated", async () => { + const stubs = limiters(false); + await expectRateLimited(await postBin(stubs, "not json")); + await expectRateLimited( + await send(stubs, "/bins", { method: "POST", headers: { "Content-Type": "text/plain" }, body: "{}" }), + ); + await expectRateLimited( + await send(stubs, "/bins", { + method: "POST", + headers: { "Content-Type": "application/json", "X-Bin-TTL": "-1" }, + body: "{}", + }), + ); + }); + + it("checks the limit before the id and the Write Key", async () => { + const stubs = limiters(false); + const { bin } = await storeBin(); + await expectRateLimited(await putBin(stubs, generateBinId(), generateWriteKey())); + await expectRateLimited(await deleteBin(stubs, "not-an-id", generateWriteKey())); + await expectRateLimited(await send(stubs, `/bins/${bin.id}`, { method: "DELETE" })); + expect(await countBins()).toBe(1); + }); + + it("never limits GET /bins/:id or GET /health", async () => { + const stubs = limiters(false); + const { bin } = await storeBin(); + expect((await send(stubs, `/bins/${bin.id}`)).status).toBe(200); + expect((await send(stubs, `/bins/${generateBinId()}`)).status).toBe(404); + expect((await send(stubs, "/health")).status).toBe(200); + expect(stubs.create.keys).toEqual([]); + expect(stubs.write.keys).toEqual([]); + }); + }); + + describe("with RATE_LIMIT_ENABLED=false", () => { + it("never calls a limiter, even one that would deny", async () => { + const stubs = limiters(false); + expect((await postBin(stubs, "{}", { enabled: false })).status).toBe(201); + const { bin, writeKey } = await storeBin(); + const put = await send( + stubs, + `/bins/${bin.id}`, + { + method: "PUT", + headers: { "Content-Type": "application/json", Authorization: `Bearer ${writeKey}` }, + body: "[]", + }, + { enabled: false }, + ); + expect(put.status).toBe(200); + const del = await send( + stubs, + `/bins/${bin.id}`, + { method: "DELETE", headers: { Authorization: `Bearer ${writeKey}` } }, + { enabled: false }, + ); + expect(del.status).toBe(204); + expect(stubs.create.keys).toEqual([]); + expect(stubs.write.keys).toEqual([]); + }); + }); +}); diff --git a/vitest.config.ts b/vitest.config.ts index b0a1eec..c725379 100644 --- a/vitest.config.ts +++ b/vitest.config.ts @@ -5,7 +5,15 @@ export default defineConfig({ plugins: [ cloudflareTest(async () => ({ wrangler: { configPath: "./wrangler.jsonc" }, - miniflare: { bindings: { TEST_MIGRATIONS: await readD1Migrations("./migrations") } }, + miniflare: { + bindings: { + TEST_MIGRATIONS: await readD1Migrations("./migrations"), + // These bindings override the wrangler.jsonc vars. Rate limiting is off for the test worker: the real + // CREATE_LIMITER (10 per 60 s) would start rejecting the many Bins the tests create from one client. + // test/rate-limit.test.ts turns it back on with stub limiter bindings. + RATE_LIMIT_ENABLED: "false", + }, + }, })), ], test: { setupFiles: ["./test/apply-migrations.ts"] },