feat(api): add a pre-auth per-IP flood guard on /api (#123)#232
feat(api): add a pre-auth per-IP flood guard on /api (#123)#232shuvamk wants to merge 2 commits into
Conversation
|
perfect, gj thank you gonna check this |
|
BTW, I was wondering about something. Since our Edge has native rate limiting with default limits built in, wouldn't adding Flood be an unnecessary extra step? Since OpenShip apps are deployed behind the OpenShip Edge by default, the Edge already handles that. I think Flood would mainly be useful if someone deploys the app separately without using the Edge. That said, it's still a nice feature to have for those cases. |
|
@shuvamk would love to hear your feedback about my comment |
|
Good question, and you're right about the default topology — behind the Edge, the Edge's native limiting is the first line and this guard will basically never fire there. So for Edge-fronted deployments it's a cheap, never-fires backstop rather than a second gate competing with the Edge (generous at 6000/min/IP and a separate bucket, so it adds no real cost to that path). The case it actually earns its keep is exactly the one you named — the app deployed without the Edge. And there's a specific reason it matters right now: the #123 fix that moved rate limiting into the route chain removed the old global Given the docs lean hard on "self-host on any VPS," I'd lean toward keeping it as defense-in-depth for those operators. But it's your call on the posture — a few ways to go:
No attachment to landing it if you feel the Edge covers enough — happy to do whichever fits your mental model. Just say which and I'll adjust or close. My mild preference is (1) or (2), only because the #123 change did quietly drop the pre-auth throttle for the non-Edge case. |
|
good design, I think config based, if edge exist or trust edge flag is on, we can disable it, but good to have lets do it then merge, thank you so much for your help and support |
The per-route rate-limit rework that fixed oblien#123 moved every policy into the route chain, after authMiddleware. That correctly made default-authed apply and killed the double-charge, but it also removed the global /api/* limiter that used to run BEFORE auth — so nothing now bounds per-IP volume ahead of authMiddleware's session DB lookup. An unauthenticated flood (garbage cookie/bearer forces the lookup every request) hits it unthrottled. Add a coarse per-IP flood ceiling as its OWN policy (`flood-ip`) and mount it app-level on /api/* ahead of every route chain: - Separate bucket from the per-route policies, so it never double-charges them — the two count independently (the invariant oblien#123 restored). - Limit sits above the most generous per-user policy (default-authed, 3000) so a single legitimate authed client is always governed by its own per-route limit, never clipped by the guard. Coarse and generous on purpose — it bounds abuse, it doesn't shape traffic. - /api/health exempt, same as the per-route limiters (probes + SSR). - Follows the ordinary per-IP limiters' loopback exemption via enforce(): under a same-host proxy misconfig where all traffic looks loopback, per-IP limiting is meaningless anyway and dev/on-host shouldn't 429. Requested by @Hydralerne on oblien#123; the two extra double-charge sites @ahmedhesham6 spotted there (auth-tight at app.ts, invite-signup inline) are unaffected — they already run their own per-route policy, and the flood guard is a distinct bucket. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Per @Hydralerne's review on oblien#123: behind the Openship Edge (which has native rate limiting), the app-level flood guard is redundant. Make it config-based so it steps aside when an upstream edge is already limiting. floodGuard now no-ops when: - CLOUD_MODE — the cloud runs behind the Oblien Edge, which has default limits, so "the edge already handles that", or - OPENSHIP_TRUST_EDGE — new opt-out flag for a self-hosted API behind an edge/proxy that enforces its own limits. Left as the first line only for a standalone self-hosted API with nothing in front of it — the case the oblien#123 fix left without a pre-auth throttle. Not auto-tied to OPENSHIP_MANAGED_EDGE: its rate limiting is opt-in (Settings > Rate Limiting), so keying off it could silently drop protection for an operator who never enabled edge limits. CLOUD_MODE is the safe "edge with default limits" signal; the flag covers the rest. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
44d120e to
0d7dd0d
Compare
2 participants
Closes the remaining half of #123. @Hydralerne — you said adding the flood guard would be great, so here it is.
Background
The per-route rework that fixed #123 moved every rate-limit policy into the route chain, after
authMiddleware. That was the right fix —default-authednow applies and the double-charge is gone. But it also removed the global/api/*limiter that used to run before auth, so nothing now bounds per-IP volume ahead ofauthMiddleware's session DB lookup.@ahmedhesham6flagged this on the issue: that pre-auth limiter was doing double duty as a flood guard, and dropping it leaves the session lookup reachable unthrottled. An unauthenticated flood (a garbage cookie/bearer forces the lookup on every request) now hits it with no per-IP ceiling.What this adds
A coarse per-IP flood ceiling as its own policy (
flood-ip), mounted app-level on/api/*ahead of every route chain:The two-layer shape we converged on in the issue:
secureRouter, post-authDesign choices, all deliberate:
flood-ipis a distinct policy id, so it and the per-route policies count independently — no single bucket is charged twice. That's the invariant Global rate-limiter always applies default-anon, and double-charges routes that set their own policy #123 was about; the flood guard doesn't reintroduce it.flood-ip= 6000/min/IP, set above the most generous per-user policy (default-authed, 3000) so a single legitimate authed client is always governed by its own per-route limit and never clipped by the guard. It bounds abuse; it doesn't shape traffic. Operators behind large shared NATs can raise it — happy to make it env-tunable if you'd prefer./api/healthexempt, same as the per-route limiters (liveness probes + dashboard SSR fan-out).enforce()(same as the ordinary per-IP limiters): under a same-host proxy misconfig where all traffic looks loopback, per-IP limiting is meaningless anyway, and dev/on-host shouldn't 429. The auth gates keep their always-enforce behavior — untouched.The two extra double-charge sites
@ahmedhesham6spotted (auth-tightatapp.ts, the inline limiter on/invite-signup) are unaffected: they already run their own per-route policy, and the flood guard is a distinct bucket layered in front.Tests
New
apps/api/test/middleware/flood-guard.test.ts(5 cases):flood-ipis per-IP and its limit is ≥default-authed(the "never clip a solo authed client" invariant)floodGuardapplies theflood-ipceiling to an ordinary/apirequest/api/healthis exempt (no rate-limit headers, passes through)Verified the tests exercise the guard: with
enforceneutered infloodGuard, the "applies ceiling" and "counts per IP" cases fail; restored, all pass. The reject-at-limit mechanics are already covered bymemory-store.test.ts, so I didn't re-drive 6000 requests here.apps/api: 831 passed, 3 skipped.bun run --cwd apps/api lintclean.Note
Left
default-anon/default-authedand the per-route wiring exactly as they are — this is purely additive. One judgment call is the6000limit; it's the one number worth your eye, and it's a one-line change if you want it different or env-driven.