security(invites): widen invite-token entropy and rate-limit public invite endpoints (#8979)
## 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/<token>` >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 <noreply@anthropic.com>
This commit is contained in:
parent
b4e7ba5143
commit
1cfed0c0ff
|
|
@ -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<string, unknown>) {
|
||||
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);
|
||||
});
|
||||
});
|
||||
|
|
@ -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);
|
||||
});
|
||||
});
|
||||
|
|
@ -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);
|
||||
});
|
||||
});
|
||||
|
|
@ -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);
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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<string, unknown> | 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<InviteResolutionNetwork>;
|
||||
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;
|
||||
|
|
|
|||
|
|
@ -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<string, number[]>();
|
||||
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,
|
||||
};
|
||||
},
|
||||
};
|
||||
}
|
||||
|
|
@ -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}`;
|
||||
};
|
||||
|
||||
|
|
|
|||
Loading…
Reference in New Issue