mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-10-31 06:04:11 +01:00
## 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>
80 lines
2.6 KiB
TypeScript
80 lines
2.6 KiB
TypeScript
// 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,
|
|
};
|
|
},
|
|
};
|
|
}
|