From 1cfed0c0ff05b27618f0d6f948e55d6dd2e779d5 Mon Sep 17 00:00:00 2001 From: Jannes Stubbemann Date: Wed, 15 Jul 2026 00:47:58 +0200 Subject: [PATCH] security(invites): widen invite-token entropy and rate-limit public invite endpoints (#8979) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - Companies onboard human members through shareable invite links; the `/api/invites/:token` endpoints are deliberately public so a recipient can view the invite and accept it without being logged in > - That publicness makes the invite token itself the only secret guarding company membership — and it was guessable: the token suffix carried only ~41 bits of entropy, and the endpoints had no rate limiting > - An attacker could therefore enumerate the token space online and accept an invite into someone else's company, gaining member access to its onboarding data, skills, and workspace > - This pull request widens invite tokens to 256 bits of entropy and puts a per-IP rate limit in front of every public `/invites/:token` sub-route > - The benefit is that invite links stop being brute-forceable while their shape, storage scheme, and UX stay exactly the same — existing links keep working ## Linked Issues or Issue Description No public issue exists; describing the problem in-PR (security/bug): **What happens:** Company invite tokens are **public**: anyone with the link can `GET /api/invites/:token`, fetch onboarding/logo/skills, and `POST /api/invites/:token/accept`. Two weaknesses combined to make them brute-forceable: 1. **Token entropy ~41 bits.** The token suffix was 8 chars over a 36-char alphabet (`8 * log2(36) ≈ 41.4` bits). That is online-enumerable. 2. **No rate limit on `/invites/:token*`.** The public endpoints had no throttling, so the ~41-bit space could be enumerated online. **Impact:** an attacker who guesses a live token can accept the invite and join the company as a member — unauthenticated, from any IP. **Expected:** invite tokens should be computationally infeasible to guess, and the public endpoints should throttle guessing attempts anyway (defense in depth). ## What Changed **Entropy** - `createInviteToken` now uses `crypto.randomBytes(32)` (256 bits) base64url-encoded, keeping the human-readable `pcp_invite_` prefix so link shape and UX are unchanged. The duplicate generator in `plugin-host-services.ts` is updated to match. - Tokens are stored **hashed** (sha256) in `invites.tokenHash`; the raw value is only returned once on creation. Storage scheme is unchanged. - **Backward compatible**: only newly minted tokens are affected; lookup is by hash of the presented value, so existing invite links keep working. **Rate limit** - New generic in-memory per-IP sliding-window limiter (`server/src/services/invite-rate-limit.ts`, 20 req/min/IP), applied as a router-level middleware on `/invites/:token` so every current and future sub-route is covered (summary, logo, onboarding, onboarding.txt, skills/index, skills/:name, test-resolution, and POST accept). - Returns `429` with `Retry-After` and `X-RateLimit-*` headers. In-memory ⇒ per-process, which bounds enumeration per replica. Mirrors the existing `company-search-rate-limit` pattern; no new dependency. - Adds a `tooManyRequests(429)` error helper in `server/src/errors.ts`. ## Verification - `invite-token-entropy.test.ts`: prefix preserved, suffix ≥ 128 bits / 22 chars, charset, 1000 unique tokens. - `invite-rate-limit.test.ts`: allows up to limit then 429s with retry-after; per-IP isolation; forgets hits after the window. - `invite-rate-limit-route.test.ts`: `GET /invites/:token` and `POST /invites/:token/accept` return 429 once the per-IP threshold is exceeded. - Manual: create an invite, open the link (works once per token as before), then hammer `GET /api/invites/` >20 times within a minute from one IP → `429` with `Retry-After`. - Server package typechecks clean for all touched files. ## Risks - Low risk. Token change affects only newly minted tokens; existing links resolve via the same sha256-hash lookup. - The limiter is in-memory and per-process: in multi-replica deployments each replica enforces its own 20 req/min/IP budget. That still bounds enumeration (per-replica) and matches the existing `company-search-rate-limit` approach; a shared store can be layered later if needed. - Legitimate users behind a single NAT/proxy IP share the 20 req/min budget for invite endpoints; the invite flow makes only a handful of requests, so headroom is ample. - No DB migration, no API shape change. > For core feature work, check [`ROADMAP.md`](ROADMAP.md) first and discuss it in `#dev` before opening the PR. Feature PRs that overlap with planned core work may need to be redirected — check the roadmap first. See `CONTRIBUTING.md`. ## Model Used - Claude (Anthropic) — Claude Fable 5 (`claude-fable-5`), extended thinking enabled, agentic tool use (code search, editing, local typecheck) via Claude Code. ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes #` / `Refs #` OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip` URLs) - [x] My branch name describes the change (e.g. `docs/...`, `fix/...`) and contains no internal Paperclip ticket id or instance-derived details - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [x] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [ ] All Paperclip CI gates are green - [ ] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge Supersedes #8147. --------- Co-authored-by: Claude Fable 5 --- .../__tests__/invite-rate-limit-route.test.ts | 120 ++++++++++++++++++ .../src/__tests__/invite-rate-limit.test.ts | 47 +++++++ .../__tests__/invite-token-entropy.test.ts | 27 ++++ server/src/errors.ts | 4 + server/src/routes/access.ts | 55 ++++++-- server/src/services/invite-rate-limit.ts | 79 ++++++++++++ server/src/services/plugin-host-services.ts | 11 +- 7 files changed, 327 insertions(+), 16 deletions(-) create mode 100644 server/src/__tests__/invite-rate-limit-route.test.ts create mode 100644 server/src/__tests__/invite-rate-limit.test.ts create mode 100644 server/src/__tests__/invite-token-entropy.test.ts create mode 100644 server/src/services/invite-rate-limit.ts diff --git a/server/src/__tests__/invite-rate-limit-route.test.ts b/server/src/__tests__/invite-rate-limit-route.test.ts new file mode 100644 index 0000000000..9cfae606cd --- /dev/null +++ b/server/src/__tests__/invite-rate-limit-route.test.ts @@ -0,0 +1,120 @@ +import express from "express"; +import request from "supertest"; +import { beforeEach, describe, expect, it, vi } from "vitest"; +import { createInviteRateLimiter } from "../services/invite-rate-limit.js"; + +function createSelectChain(rows: unknown[]) { + const query = { + then(resolve: (value: unknown[]) => unknown) { + return Promise.resolve(rows).then(resolve); + }, + leftJoin() { + return query; + }, + orderBy() { + return query; + }, + where() { + return query; + }, + }; + return { + from() { + return query; + }, + }; +} + +function createDbStub(...selectResponses: unknown[][]) { + let selectCall = 0; + return { + select() { + const rows = selectResponses[selectCall] ?? []; + selectCall += 1; + return createSelectChain(rows); + }, + }; +} + +async function createApp(db: Record) { + const [{ accessRoutes }, { errorHandler }] = await Promise.all([ + import("../routes/access.js"), + import("../middleware/index.js"), + ]); + const app = express(); + app.use((req, _res, next) => { + (req as any).actor = { type: "anon" }; + next(); + }); + app.use( + "/api", + accessRoutes(db as any, { + deploymentMode: "local_trusted", + deploymentExposure: "private", + bindHost: "127.0.0.1", + allowedHostnames: [], + inviteRateLimiter: createInviteRateLimiter({ + maxRequests: 1, + windowMs: 60_000, + now: () => 1_000, + }), + }), + ); + app.use(errorHandler); + return app; +} + +describe("invite-token endpoint rate limiting", () => { + beforeEach(() => { + vi.resetModules(); + }); + + it("returns 429 once the per-IP threshold is exceeded", async () => { + // No invite row -> route would 404, but the rate-limit middleware runs first + // and short-circuits on the second request. + const app = await createApp(createDbStub([], [], [], [], [])); + + const first = await request(app).get( + "/api/invites/pcp_invite_aaaaaaaaaaaaaaaaaaaaaa", + ); + expect(first.status).toBe(404); + + const limited = await request(app).get( + "/api/invites/pcp_invite_aaaaaaaaaaaaaaaaaaaaaa", + ); + expect(limited.status).toBe(429); + expect(limited.headers["retry-after"]).toBe("60"); + expect(limited.body).toMatchObject({ + error: "Too many invite requests", + details: { retryAfterSeconds: 60 }, + }); + }); + + it("also rate-limits the accept sub-route", async () => { + const app = await createApp(createDbStub([], [], [], [], [])); + + await request(app).get("/api/invites/pcp_invite_bbbbbbbbbbbbbbbbbbbbbb"); + + const limited = await request(app) + .post("/api/invites/pcp_invite_bbbbbbbbbbbbbbbbbbbbbb/accept") + .send({}); + expect(limited.status).toBe(429); + }); + + it("ignores client-supplied X-Forwarded-For — spoofed IPs do not reset the budget", async () => { + // `trust proxy` is unset here (Express default: trust nothing), so the + // rate-limit key is the socket's remote address. Rotating fake + // X-Forwarded-For values must NOT mint a fresh per-IP budget. + const app = await createApp(createDbStub([], [], [], [], [])); + + const first = await request(app) + .get("/api/invites/pcp_invite_cccccccccccccccccccccc") + .set("x-forwarded-for", "1.1.1.1"); + expect(first.status).toBe(404); + + const spoofed = await request(app) + .get("/api/invites/pcp_invite_cccccccccccccccccccccc") + .set("x-forwarded-for", "1.1.1.2"); + expect(spoofed.status).toBe(429); + }); +}); diff --git a/server/src/__tests__/invite-rate-limit.test.ts b/server/src/__tests__/invite-rate-limit.test.ts new file mode 100644 index 0000000000..b6ec6a4719 --- /dev/null +++ b/server/src/__tests__/invite-rate-limit.test.ts @@ -0,0 +1,47 @@ +import { describe, expect, it } from "vitest"; +import { createInviteRateLimiter } from "../services/invite-rate-limit.js"; + +describe("createInviteRateLimiter", () => { + it("allows requests up to the limit then blocks with a retry-after", () => { + const limiter = createInviteRateLimiter({ + maxRequests: 3, + windowMs: 60_000, + now: () => 1_000, + }); + + expect(limiter.consume("1.2.3.4").allowed).toBe(true); + expect(limiter.consume("1.2.3.4").allowed).toBe(true); + expect(limiter.consume("1.2.3.4").allowed).toBe(true); + + const blocked = limiter.consume("1.2.3.4"); + expect(blocked.allowed).toBe(false); + expect(blocked.remaining).toBe(0); + expect(blocked.retryAfterSeconds).toBe(60); + }); + + it("tracks each IP independently", () => { + const limiter = createInviteRateLimiter({ + maxRequests: 1, + windowMs: 60_000, + now: () => 1_000, + }); + + expect(limiter.consume("1.1.1.1").allowed).toBe(true); + expect(limiter.consume("1.1.1.1").allowed).toBe(false); + expect(limiter.consume("2.2.2.2").allowed).toBe(true); + }); + + it("forgets hits once the window has elapsed", () => { + let current = 1_000; + const limiter = createInviteRateLimiter({ + maxRequests: 1, + windowMs: 60_000, + now: () => current, + }); + + expect(limiter.consume("9.9.9.9").allowed).toBe(true); + expect(limiter.consume("9.9.9.9").allowed).toBe(false); + current += 60_001; + expect(limiter.consume("9.9.9.9").allowed).toBe(true); + }); +}); diff --git a/server/src/__tests__/invite-token-entropy.test.ts b/server/src/__tests__/invite-token-entropy.test.ts new file mode 100644 index 0000000000..c6370392b4 --- /dev/null +++ b/server/src/__tests__/invite-token-entropy.test.ts @@ -0,0 +1,27 @@ +import { describe, expect, it } from "vitest"; +import { createInviteToken } from "../routes/access.js"; + +const PREFIX = "pcp_invite_"; + +describe("createInviteToken", () => { + it("keeps the human-readable pcp_invite_ prefix", () => { + expect(createInviteToken().startsWith(PREFIX)).toBe(true); + }); + + it("carries at least 128 bits of randomness", () => { + const suffix = createInviteToken().slice(PREFIX.length); + // base64url over 32 random bytes => 43 chars, 256 bits of entropy. Each + // base64url char is 6 bits, so >= 22 chars guarantees >= 128 bits. + expect(suffix.length).toBeGreaterThanOrEqual(22); + expect(suffix).toMatch(/^[A-Za-z0-9_-]+$/); + const bits = suffix.length * 6; + expect(bits).toBeGreaterThanOrEqual(128); + }); + + it("produces unique, non-repeating tokens", () => { + const tokens = new Set( + Array.from({ length: 1000 }, () => createInviteToken()), + ); + expect(tokens.size).toBe(1000); + }); +}); diff --git a/server/src/errors.ts b/server/src/errors.ts index e502d7e8bc..6abad9c7ee 100644 --- a/server/src/errors.ts +++ b/server/src/errors.ts @@ -32,3 +32,7 @@ export function conflict(message: string, details?: unknown) { export function unprocessable(message: string, details?: unknown) { return new HttpError(422, message, details); } + +export function tooManyRequests(message = "Too many requests", details?: unknown) { + return new HttpError(429, message, details); +} diff --git a/server/src/routes/access.ts b/server/src/routes/access.ts index b341c71780..39889d19b2 100644 --- a/server/src/routes/access.ts +++ b/server/src/routes/access.ts @@ -53,8 +53,13 @@ import { conflict, notFound, unauthorized, - badRequest + badRequest, + tooManyRequests } from "../errors.js"; +import { + createInviteRateLimiter, + type InviteRateLimiter, +} from "../services/invite-rate-limit.js"; import { logger } from "../middleware/logger.js"; import { validate } from "../middleware/validate.js"; import { collectReachableInterfaceHosts } from "../runtime-api.js"; @@ -90,8 +95,12 @@ function hashToken(token: string) { } const INVITE_TOKEN_PREFIX = "pcp_invite_"; -const INVITE_TOKEN_ALPHABET = "abcdefghijklmnopqrstuvwxyz0123456789"; -const INVITE_TOKEN_SUFFIX_LENGTH = 8; +// 32 random bytes = 256 bits of entropy, base64url-encoded (43 chars). The +// invite token is public (anyone with the link can GET/accept the invite), so +// it must not be brute-forceable. The previous 8-char base36 suffix carried only +// ~41 bits, which is online-enumerable. The token is stored hashed (sha256) in +// `invites.tokenHash`; the raw value is only returned once on creation. +const INVITE_TOKEN_ENTROPY_BYTES = 32; const INVITE_TOKEN_MAX_RETRIES = 5; const COMPANY_INVITE_TTL_MS = 72 * 60 * 60 * 1000; const INVITE_RESOLUTION_DNS_TIMEOUT_MS = 3_000; @@ -101,12 +110,8 @@ type MemberGrantPayload = { scope?: Record | null; }; -function createInviteToken() { - const bytes = randomBytes(INVITE_TOKEN_SUFFIX_LENGTH); - let suffix = ""; - for (let idx = 0; idx < INVITE_TOKEN_SUFFIX_LENGTH; idx += 1) { - suffix += INVITE_TOKEN_ALPHABET[bytes[idx]! % INVITE_TOKEN_ALPHABET.length]; - } +export function createInviteToken() { + const suffix = randomBytes(INVITE_TOKEN_ENTROPY_BYTES).toString("base64url"); return `${INVITE_TOKEN_PREFIX}${suffix}`; } @@ -2598,6 +2603,7 @@ export function accessRoutes( bindHost: string; allowedHostnames: string[]; inviteResolutionNetwork?: Partial; + inviteRateLimiter?: InviteRateLimiter; } ) { const router = Router(); @@ -2608,6 +2614,37 @@ export function accessRoutes( ? { ...defaultInviteResolutionNetwork, ...opts.inviteResolutionNetwork } : inviteResolutionNetwork; + // Per-IP rate limit for the public, unauthenticated invite-token endpoints + // (`/invites/:token*`). The token is looked up by hash, so without a limit the + // token space would be online-enumerable. Applied as a router-level middleware + // so every current and future `/invites/:token` sub-route is covered. + // + // The key is deliberately NOT `requestIp()`: that helper prefers the + // client-supplied `X-Forwarded-For` header (fine for log/audit fields, + // but trivially spoofable as a rate-limit key — rotating fake XFF values + // would mint a fresh budget per request). `req.ip` honors Express's + // `trust proxy` setting (configured from TRUST_PROXY in app.ts, default: + // trust nothing), so it is the socket's remote address unless the + // operator explicitly trusts a proxy — an unforgeable key either way. + const inviteRateLimiter = opts.inviteRateLimiter ?? createInviteRateLimiter(); + router.use("/invites/:token", (req, res, next) => { + const result = inviteRateLimiter.consume( + req.ip || req.socket?.remoteAddress || "unknown", + ); + res.setHeader("X-RateLimit-Limit", String(result.limit)); + res.setHeader("X-RateLimit-Remaining", String(result.remaining)); + if (!result.allowed) { + res.setHeader("Retry-After", String(result.retryAfterSeconds)); + next( + tooManyRequests("Too many invite requests", { + retryAfterSeconds: result.retryAfterSeconds, + }), + ); + return; + } + next(); + }); + async function assertInstanceAdmin(req: Request) { if (req.actor.type !== "board") throw unauthorized(); if (isLocalImplicit(req)) return; diff --git a/server/src/services/invite-rate-limit.ts b/server/src/services/invite-rate-limit.ts new file mode 100644 index 0000000000..e89afc522b --- /dev/null +++ b/server/src/services/invite-rate-limit.ts @@ -0,0 +1,79 @@ +// Generic per-IP sliding-window rate limiter for the public, unauthenticated +// invite-token endpoints (`/invites/:token*`). These routes accept a token in +// the URL and look it up by hash, so without a limit the token space is +// online-enumerable. The limiter is in-memory and therefore per-process; for a +// horizontally-scaled deployment it bounds enumeration per instance, which is +// the meaningful protection here (a brute-force still has to defeat the limit on +// every replica). It intentionally has no external dependency. + +export const INVITE_RATE_LIMIT_WINDOW_MS = 60_000; +export const INVITE_RATE_LIMIT_MAX_REQUESTS = 20; + +export type InviteRateLimitResult = { + allowed: boolean; + limit: number; + remaining: number; + retryAfterSeconds: number; +}; + +export type InviteRateLimiter = { + consume(ip: string): InviteRateLimitResult; +}; + +export function createInviteRateLimiter(options: { + windowMs?: number; + maxRequests?: number; + now?: () => number; +} = {}): InviteRateLimiter { + const windowMs = options.windowMs ?? INVITE_RATE_LIMIT_WINDOW_MS; + const maxRequests = options.maxRequests ?? INVITE_RATE_LIMIT_MAX_REQUESTS; + const now = options.now ?? Date.now; + const hitsByKey = new Map(); + let lastSweep = 0; + + // Periodically drop keys whose hits have all aged out so a stream of unique + // spoofable IPs can't grow the map without bound. + function sweep(currentTime: number) { + if (currentTime - lastSweep < windowMs) return; + lastSweep = currentTime; + const cutoff = currentTime - windowMs; + for (const [key, hits] of hitsByKey) { + const recent = hits.filter((hit) => hit > cutoff); + if (recent.length === 0) hitsByKey.delete(key); + else hitsByKey.set(key, recent); + } + } + + return { + consume(ip: string) { + const currentTime = now(); + sweep(currentTime); + const cutoff = currentTime - windowMs; + const key = ip || "unknown"; + const recentHits = (hitsByKey.get(key) ?? []).filter((hit) => hit > cutoff); + + if (recentHits.length >= maxRequests) { + const oldestHit = recentHits[0] ?? currentTime; + hitsByKey.set(key, recentHits); + return { + allowed: false, + limit: maxRequests, + remaining: 0, + retryAfterSeconds: Math.max( + 1, + Math.ceil((oldestHit + windowMs - currentTime) / 1000), + ), + }; + } + + recentHits.push(currentTime); + hitsByKey.set(key, recentHits); + return { + allowed: true, + limit: maxRequests, + remaining: Math.max(0, maxRequests - recentHits.length), + retryAfterSeconds: 0, + }; + }, + }; +} diff --git a/server/src/services/plugin-host-services.ts b/server/src/services/plugin-host-services.ts index 6984f42974..f2798cc634 100644 --- a/server/src/services/plugin-host-services.ts +++ b/server/src/services/plugin-host-services.ts @@ -865,19 +865,16 @@ export function buildHostServices( }; const INVITE_TOKEN_PREFIX = "pcp_invite_"; - const INVITE_TOKEN_ALPHABET = "abcdefghijklmnopqrstuvwxyz0123456789"; - const INVITE_TOKEN_SUFFIX_LENGTH = 8; + // 256 bits of entropy, base64url-encoded. Keep in sync with createInviteToken + // in routes/access.ts. The token is public, so it must not be brute-forceable. + const INVITE_TOKEN_ENTROPY_BYTES = 32; const INVITE_TOKEN_MAX_RETRIES = 5; const COMPANY_INVITE_TTL_MS = 72 * 60 * 60 * 1000; const hashToken = (token: string) => createHash("sha256").update(token).digest("hex"); const createInviteToken = () => { - const bytes = randomBytes(INVITE_TOKEN_SUFFIX_LENGTH); - let suffix = ""; - for (let idx = 0; idx < INVITE_TOKEN_SUFFIX_LENGTH; idx += 1) { - suffix += INVITE_TOKEN_ALPHABET[bytes[idx]! % INVITE_TOKEN_ALPHABET.length]; - } + const suffix = randomBytes(INVITE_TOKEN_ENTROPY_BYTES).toString("base64url"); return `${INVITE_TOKEN_PREFIX}${suffix}`; };