fix(platform): REST v1 attributable limits, real cursors, true spec - #3165
Merged
Conversation
The /api/v1 door charged `rest:api` on the leftmost X-Forwarded-For entry — the one the caller writes — before authentication, so rotating the header minted unlimited fresh buckets, pinning a victim's NAT address starved their traffic, and strangers drained budgets without a key. The lane top-ups (rest:execute, rest:upload) reused the same key. Authenticated requests now charge the key holder (`user:<id>`; the top- ups key the same way). A Bearer key that fails to authenticate charges the new pre-auth `rest:auth-fail-ip` lane on the client IP derived through the deployment's trusted-proxy list — the same proxy-addr walk the login and Slack lanes use, now started from the TCP peer that @hono/node-server exposes, so a backend reached without the shipped proxy ignores any forged chain. A request without a Bearer header charges nothing. Better Auth's own per-key window surfaces as 429 instead of being laundered into "Invalid API key". Tests: the door's attribution (10 cases), getClientIp with the real peer, and an integration check that rotates the leftmost hop over one real source and reads the limiter rows back.
GET /api/v1/contacts and /products emitted `continueCursor` / `isDone` but never read `?cursor=`, so a spec-following pager received page one forever and nothing past the first page was reachable through the door. Both now parse the cursor through one shared keyset codec (`<updatedAt>:<id>` — the format threads and project files already used, now shared instead of hand-rolled per family), pass it to the service, and clamp `limit` so no client value becomes a zero or negative LIMIT; products honour the documented `status` filter and an automation's run listing honours `limit`. Tests: the codec and clamp, both families' pass-through and round-trip against a recording Sql double, and an integration check that walks contacts and products to the last page in pages of three.
`/automations/:name{.+}/runs` (and /versions, /triggers, /projects) sat
beside the catch-all `/automations/:name{.+}`. Under Hono's RegExpRouter
the greedy `(.+)` swallows the suffix, so every one of those GETs was
answered by the single-automation read as "Automation not found";
production only escaped because the root app happens to fall back to
the TrieRouter. The lazy `:name{.+?}` resolves the same paths correctly
under both routers, so the documented sub-routes no longer depend on
which router the app happens to get. Covered by the spec parity test.
`public/openapi.json` (the Swagger UI at /docs) described envelopes and
parameters the handlers never produced: `pageOf` + cursor params on
`/automations` and `/automations/{name}/runs`, which answer named arrays
(`{automations}`, `{runs}`, the latter a `limit` window); the legacy
documents/websites/products entries carried the Convex-era
`{data, cursor, hasMore}` lists, `_id`/`_creationTime` rows, and 200
objects where the handlers answer 201 `{id}` and 204; agents' DELETE
answers `{deleted}` not 204; contacts documented a `locale` filter that
nothing reads; knowledge entries, messages, versions and triggers had no
schema at all; and the rate-limit prose still said "IP-keyed".
The document now lives in `scripts/openapi/spec.ts` — side-effect free,
with the former legacy JSON folded into the same builders and every
shape taken from the handler code — and `generate-openapi.ts` only
writes it. `scripts/openapi/spec.test.ts` is the drift guard the retired
convex-era test used to be: every documented /api/v1 path+method is a
registered route and vice versa, and scripted handler responses
validate (ajv) against the spec's 200 schemas. Regenerated with
`bun run generate:openapi`.
The developer pages said the API was rate-limited per client IP before authentication and that contacts and products could not page past their first page. Both were true and both are fixed: budgets key on the key holder (failed keys throttle per source IP; keyless requests cost nothing), and every page envelope pages to the last page while the run listing is a `limit` window. en/de/fr rewritten natively.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem class
The public REST v1 API's rate limiting was spoofable and charged before auth, its documented pagination cursor was ignored on two families, and the OpenAPI document at
/docsdisagreed with the handlers. Base:5c438f5b7.Per finding
1. REST door keys rate limits on spoofable leftmost X-Forwarded-For — fixed (
1c2f158ee)Verified on base:
rest/v1.ts:47-52tookxff.split(',')[0](the caller-written hop) as therest:apikey and charged it before authentication;shared.tschargeLanereused it forrest:execute/rest:upload.user:<id>, the user the key acts as (checkUserRateLimit); the lane top-ups key the same way. A misdirected-org request still counts (charged right after auth).rest:auth-fail-iprule (20/min, burst 40) on the trusted-proxy-derived client IP; over budget the door answers 429 +Retry-Afterbefore the 401. A request without a Bearer header costs nothing and charges nothing — a stranger can no longer drain any key holder's budget.getClientIp(proxy-addr, right-to-left over thedefaultorg'slogin_policy.trustedProxies, defaultloopback+uniquelocal— the same helper the login, SSE and Slack lanes use).getClientIpgained an optional TCPpeer(read from@hono/node-server'sc.env.incoming): an untrusted peer is the client and its forwarded headers are never consulted, so a backend reached without the shipped Caddy cannot be fed a forged chain; a trusted peer hands the walk to its chain as before. IPv4-mapped peers are normalized.X-Forwarded-Forwith{remote_host}, so behind it the chain is one honest hop. Deployments fronted by a public edge configure it in Settings → Governance → Login policy (trustedProxies), exactly as the login throttle already requires.TOO_MANY_REQUESTSthrown fromgetSession) was laundered into401 Invalid API key; it now answers 429 withRetry-After: 60.2. GET /contacts and /products ignore the documented cursor — fixed (
dfdab0930)Verified on base: neither handler read
?cursor=; both emitted a JSON-stringified cursor. Both now parse the cursor through one shared keyset codec inrest/shared.ts(<updatedAt>:<id>— the format threads and project files already hand-rolled; they now share it), pass it tolistContacts/listProducts, and clamplimitto 1..200 (a?limit=0/negative used to reach Postgres asLIMIT 0/LIMIT -n). Products honour the documentedstatusfilter.GET /automations/{name}/runshonourslimit(the store supported it; the handler never passed it). Contacts' documentedlocalefilter had no service support and is removed from the spec instead.3. OpenAPI spec drifted from the handlers — fixed (
4c0365e52, plus85c632826)The committed
openapi.jsonwas semantically identical to the generator output (only oxfmt formatting differed), so the source was the problem. The document now lives inscripts/openapi/spec.ts(side-effect free); the legacy documents/websites/products JSON is folded into the same builders with the handlers' real shapes;generate-openapi.tsonly writes. Corrected:/automationsand/automations/{name}/runsare named arrays ({automations},{runs}with alimitwindow),{page,isDone,continueCursor}replaces the Convex-era{data,cursor,hasMore},id/createdAtreplace_id/_creationTime, 201{id}/ 204 where the handlers answer that, agents' DELETE answers{deleted}, schemas added for knowledge entries, messages, versions, triggers, runs (fullRunrow), agents, skills, bulk results; rate-limit prose no longer says "IP-keyed". Regenerated withbun run generate:openapi(spec version 1.2.0).scripts/openapi/spec.test.tsis the drift guard the retired convex-era test used to be: every documented/api/v1path+method is a registered Hono route and vice versa, and scripted handler responses validate (ajv) against the spec's 200 schemas. Writing it exposed a latent routing hazard: under Hono's default RegExpRouter the greedy:name{.+}catch-all swallows/runs,/versions,/triggers(production only escapes because the root app falls back to TrieRouter).85c632826switches to:name{.+?}, which resolves correctly under both routers (probed empirically).Already fixed in base (refuted for this branch)
DELETE/PATCHfreeze guards (fix(platform): enforce the document write matrix and record freeze #3136):v1-core.tsDELETE routes throughdeleteDocumentHardand mapsDOCUMENT_RECORD_PROTECTEDto 409; PATCH goes throughupdateDocument— both present at base5c438f5b7.Docs (
cec190a28)docs/{en,de,fr}/develop/rate-limits.mdandapi-reference.md: budgets key on the key holder, failed keys throttle per source IP, keyless requests cost nothing; contacts/products page to the last page; the run listing is alimitwindow. Written natively perwrite-translations; docs structural suite 30 files / 194 tests green.Tests
backend/rest/v1.test.ts(door attribution)backend/core/lib/utils/client_ip.test.ts(+peer cases)backend/rest/v1-core.test.ts(cursor pass-through, round-trip, clamp, status filter)backend/rest/shared.test.ts(codec + clamp)scripts/openapi/spec.test.ts(route set ↔ spec, ajv shape parity)backend/integration-check.ts—REST door rate-limit attribution,REST pagination walks to the last pageGates (observed)
bunx tsc --noEmit(platform): exit 0.bunx oxlint --type-awareon every touched file: clean.oxfmt --checkon touched files incl. the regeneratedopenapi.json: clean.backend:integrationon throwaway tale-db + MinIO: branch 346/346; base (new probes against base handlers) 344/346 — exactly the two new probes red (spoofed-hop rows=46, never a 429; contacts/products loop 25 pages of the same rows, runs ignorelimit).@tale/docs test: 30 files / 194 tests pass.Cross-class discoveries (not fixed here)
getClientIpwithout the TCP peer, so a backend reached without the proxy still honours a forgedX-Forwarded-Forthere — they can adopt the newpeeroption the same way.mcp_http.test.tsstill points at the retiredconvex/lib/rest/helpers.test.tsin a comment.