fix(bff): cap page.limit and rate-limit agent requests by Tonours · Pull Request #1858 · ForestAdmin/agent-nodejs

Tonours · 2026-08-28T17:32:19Z

What

Two guards on the agent BFF edge:

page.limit capped at 1000. Above it: 400 invalid_request — rejected, not clamped, so a caller cannot believe it got what it asked for. The cap is published as Page.properties.limit.maximum.
Per-identity rate limiting on every authenticated /agent/* request: 429 too_many_requests with an integer Retry-After. Defaults 300 requests / 60 s, tunable through BFF_RATE_LIMIT_MAX_REQUESTS and BFF_RATE_LIMIT_WINDOW_MS; an invalid value fails boot rather than falling back.

fixes PRD-1100

Why

A valid key could pull an entire collection in one request: limit: 1000000000 was forwarded verbatim as page[size]. And nothing throttled the cadence — 400 concurrent requests, zero 429.

How

Buckets key on the resolved identity, mode-prefixed (api-key: / oauth:). Rejected keys and anonymous requests die at 401 before the limiter and never allocate a bucket.
The counter increments before the chain runs, so a concurrent burst cannot slip past the limit. Covered by a 10-parallel test: exactly maxRequests pass.
At the 10 000 bucket ceiling a new identity fails closed rather than evicting a live window.
A request the limiter cannot bucket — no identity on the context — is rejected with 401 rather than passed through. The limiter's contract is that every authenticated agent request is counted; letting an unidentifiable one through would make any future route with a looser auth step silently unlimited.

Two different 429s

The same code emits 429 for two reasons, and the document now says so instead of blaming the caller for both:

Quota — this identity exceeded its window. Retry-After is when its window resets.
Saturation — the bucket table is full and no window can be opened. The caller exceeded nothing. Retry-After is a lower bound: the earliest reset among the identities currently holding a window, after which a slot may free up — the freed slot goes to whoever asks first.

A client that backs off on the wrong signal is the reason this distinction is published rather than left in the message alone.

How to test

cd packages/agent-bff
yarn jest test/data test/rate-limit test/http test/config test/openapi test/cli-core.test.ts
yarn lint && yarn build

POST /agent/v1/users/list with {"page":{"limit":1000000000,"offset":0}} returns 400 invalid_request.
With BFF_RATE_LIMIT_MAX_REQUESTS=2, the third authenticated request in the window returns 429 with Retry-After.

Known limitation

Requests rejected by the body parser (400/413) do not count toward the quota. They are bounded at 16 KB and never reach the agent.

Definition of Done

General

Write an explicit title for the Pull Request, following Conventional Commits specification
Test manually the implemented changes
Validate the code quality (indentation, syntax, style, simplicity, readability)

Security

Consider the security impact of the changes made

linear-code · 2026-08-28T17:32:23Z

PRD-1100

qltysh · 2026-08-28T17:33:30Z

1 new issue

Tool	Category	Rule	Count
qlty	Structure	Function with high complexity (count = 14): createRateLimitMiddleware	1

qltysh · 2026-08-28T17:37:53Z

Coverage Impact

This PR will not change total coverage.

Modified Files with Diff Coverage (7)

Rating	File	% Diff	Uncovered Line #s
	packages/agent-bff/src/openapi/openapi-document.ts	100.0%
	packages/agent-bff/src/data/request-schemas.ts	100.0%
	packages/agent-bff/src/cli-core.ts	100.0%
	packages/agent-bff/src/config/env-config.ts	100.0%
	packages/agent-bff/src/http/bff-local-errors.ts	100.0%
	packages/agent-bff/src/rate-limit/rate-limit-middleware.ts	100.0%
	packages/agent-bff/src/rate-limit/agent-path.ts	100.0%
	Total	100.0%

🚦 See full report on Qlty Cloud »

🛟 Help

Diff Coverage: Coverage for added or modified lines of code (excludes deleted files). Learn more.
Total Coverage: Coverage for the whole repository, calculated as the sum of all File Coverage. Learn more.
File Coverage: Covered Lines divided by Covered Lines plus Missed Lines. (Excludes non-executable lines including blank lines and comments.)
- Indirect Changes: Changes to File Coverage for files that were not modified in this PR. Learn more.

nbouliol

Reviewed the two guards. The cap side is clean: MAX_PAGE_LIMIT is defined once and reused by the published prose, so the document and the runtime can't drift, and I confirmed the limiter really does sit after createAuthModeMiddleware/apiKeyStep, so anonymous and rejected-key traffic 401s without ever allocating a bucket. Test coverage on the limiter is well above the usual bar.

The main thread below is the published contract: the 429 and Retry-After descriptions hold for the quota path but not for the saturation path the same code can produce.

…uses

Tonours · 2026-09-01T21:42:52Z

Second push, from the review pass.

The two new env vars were undocumented. packages/agent-bff/README.md carries the exhaustive env table and neither BFF_RATE_LIMIT_MAX_REQUESTS nor BFF_RATE_LIMIT_WINDOW_MS was in it — so an operator hitting 429 at the default 300/60s had no documented knob, on a feature that changes production behaviour on day one. Both rows added, along with the saturation case and the Retry-After semantics, and the config-validation bullet now names BFF_RATE_LIMIT_* among the values that fail boot.

Two notes I am leaving as they are, with the reasoning:

The throw unauthorized() added last push is unreachable in the mounted chain — auth-mode.ts already 401s when neither a bearer nor a key is present, the OAuth step sets principal, the api-key step sets apiKeyIdentity or rethrows, and the docs viewer is mounted outside the agent chain. That is the point: it is defence in depth against the future route the thread above describes, not a live path. Verified no current route regresses — /agent/openapi.json, /agent/v1/context and the AI route all sit behind the same auth step, and /health is not an agent path.

cli-core.test.ts "should leave a small burst untouched at the default configuration" passes with the limiter removed. It guards against an over-eager default rather than proving the limiter works, which the dedicated suite does. Kept, worth knowing it is not a regression test.

Commit titles on this branch: one is 73 chars and one uses the agent-bff scope where the others use bff. The repo squashes on the PR title, so neither reaches main.

nbouliol

Re-reviewed at fe244e5. All five comments from the last pass are addressed, and the fail-closed test was properly renamed and re-asserted rather than left pinning the old behaviour — I checked that nothing legitimate now hits the new throw, since createCorsMiddleware short-circuits OPTIONS at 204 before the chain.

Three things below. The first is the one that matters: the 429 split tells callers to branch on message text, which this package's own error contract rules out.

One more that falls outside the diff so I couldn't anchor it: README.md:114 still says the request edge "enforces three cross-cutting concerns" (Auth-mode / CORS / Timezone) — the limiter makes four, and it's currently documented only as env rows.

Unrelated to this PR: the red check is packages/ai-proxy/test/llm.integration.test.ts:503 (39 passed, 1 failed), a live-provider test in a package this branch doesn't touch.

nbouliol · 2026-09-02T08:36:08Z

  415: 'The request declares a character set the server cannot decode. Other content types are NOT rejected: a form-urlencoded body is parsed and validated like JSON (its values arrive as strings, so typed fields such as page.limit fail with 400), while any other non-JSON content type is read as an absent body, silently dropping filters and pagination',
  422: 'A field is unknown, not filterable, or is a nested relation path',
-  429: 'The agent rate-limited the request',
+  429: 'The BFF rate-limited the request, for one of two reasons: the caller identity exceeded its per-window budget, or the limiter is saturated and cannot open a window for a new identity. The message distinguishes them, and Retry-After carries the seconds to wait. On data and action routes the agent may also rate-limit the request itself',


The message distinguishes them asks callers to branch on message text, which README:116 rules out ("consumers branch on error.type, never on message text") — and both throw sites carry the same too_many_requests. Give saturation its own type or a details discriminator, or say plainly that the two are indistinguishable to a caller?

nbouliol · 2026-09-02T08:36:08Z

 - A malformed value (a non-http(s) `*_URL`, a `HTTP_PORT` that is not a decimal integer in 0–65535,
-  a non-IANA `BFF_DEFAULT_TIMEZONE`, a non-boolean `BFF_OPENAPI_ENABLED`) fails fast at boot: the
-  process exits with a clear error and never echoes the offending value.
+  a non-IANA `BFF_DEFAULT_TIMEZONE`, a non-boolean `BFF_OPENAPI_ENABLED`, a `BFF_RATE_LIMIT_*` that is


BFF_RATE_LIMIT_MAX_REQUESTS=20000 and BFF_RATE_LIMIT_WINDOW_MS=500 are positive decimal integers that still fail boot (max 10 000, min 1 000). Worth stating the ranges here and in the two rows above, the way HTTP_PORT and BFF_AI_TIMEOUT_MS do?

nbouliol · 2026-09-02T08:36:08Z

+
+    const key = bucketKeyOf(ctx.state as AuthEdgeState);
+
+    if (key === undefined) throw unauthorized();


unauthorized() takes the default "Missing or invalid credentials", which reads like a real credential failure on a path that should be unreachable — permissions-routes-middleware.ts:42 passes 'No caller identity for this request' for the same condition.

nbouliol

Third pass, at 2637468. All three comments from the last round are fixed, and the 429 one properly — details.cause is a real machine-readable discriminator rather than a reworded sentence, which is the right answer to the type-first contract. I checked the README ranges against env-config.ts:176-187 and they agree exactly on both bounds, and tooManyRequests has only the two callers so the new parameter regressed nothing.

Two things left, both about the new cause contract rather than the limiter itself.

nbouliol · 2026-09-02T15:43:47Z

  415: 'The request declares a character set the server cannot decode. Other content types are NOT rejected: a form-urlencoded body is parsed and validated like JSON (its values arrive as strings, so typed fields such as page.limit fail with 400), while any other non-JSON content type is read as an absent body, silently dropping filters and pagination',
  422: 'A field is unknown, not filterable, or is a nested relation path',
-  429: 'The agent rate-limited the request',
+  429: 'The BFF rate-limited the request, for one of two reasons: the caller identity exceeded its per-window budget, or the limiter is saturated and cannot open a window for a new identity. The `details.cause` field of the error body distinguishes them (`limit_exceeded` or `limiter_saturated`), and Retry-After carries the seconds to wait. On data and action routes the agent may also rate-limit the request itself',


A relayed agent 429 never carries cause — mapAgentError sets details to the agent's own payload — so a caller branching on it sees undefined or arbitrary data. The Retry-After description just below (72-73) already spells out the parallel case; worth the same clause here?

nbouliol · 2026-09-02T15:43:47Z

+        throw tooManyRequests(
+          retryAfterSeconds(earliestReset, current),
+          'Too many requests: the rate limiter is saturated',
+          { cause: 'limiter_saturated' },


Now that clients branch on these, the two values live in 7 places across 5 files with no shared source (here, :101, the OpenAPI prose at :40, README.md:98, and both tests) — a typo in any one silently breaks the contract. Export a RateLimitCause union next to tooManyRequests and reference it?

Tonours added 5 commits August 28, 2026 18:38

fix(bff): cap page.limit at 1000

af9d3bb

feat(bff): rate-limit agent edge per identity

eb2dbd5

refactor(bff): share agent path boundary predicate

521c1aa

fix(bff): declare context 429 and truthful saturation retry

1974e74

test(bff): pin rate limit boundaries and doc prose

e7146e7

refactor(bff): consolidate parsers and typed contracts

83a356b

Tonours force-pushed the fix/prd-1100-page-limit-throttle branch from 8a8c0e3 to a0d1aee Compare August 31, 2026 09:13

test(agent-bff): cover the unbucketed anonymous rate-limit path

e44c369

Tonours force-pushed the fix/prd-1100-page-limit-throttle branch from a0d1aee to e44c369 Compare August 31, 2026 09:14

nbouliol reviewed Sep 1, 2026

View reviewed changes

Tonours added 2 commits September 1, 2026 23:38

fix(bff): fail closed on an unbucketable request and split the 429 ca…

9bd92b5

…uses

docs(bff): document the rate limit env vars

fe244e5

nbouliol reviewed Sep 2, 2026

View reviewed changes

Tonours added 3 commits September 2, 2026 12:58

fix(bff): distinguish the two 429 causes in error details

63110a3

fix(bff): name the missing identity in the rate limit 401

cf149eb

docs(bff): state the rate limit env var ranges

2637468

nbouliol reviewed Sep 2, 2026

View reviewed changes


		const key = bucketKeyOf(ctx.state as AuthEdgeState);

		if (key === undefined) throw unauthorized();

Uh oh!

Conversation

Tonours commented Aug 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What

Why

How

Two different 429s

How to test

Known limitation

Definition of Done

General

Security

Uh oh!

linear-code Bot commented Aug 28, 2026

Uh oh!

qltysh Bot commented Aug 28, 2026

1 new issue

Uh oh!

qltysh Bot commented Aug 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nbouliol left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Tonours commented Sep 1, 2026

Uh oh!

nbouliol left a comment

Choose a reason for hiding this comment

Uh oh!

nbouliol Sep 2, 2026

Choose a reason for hiding this comment

Uh oh!

nbouliol Sep 2, 2026

Choose a reason for hiding this comment

Uh oh!

nbouliol Sep 2, 2026

Choose a reason for hiding this comment

Uh oh!

nbouliol left a comment

Choose a reason for hiding this comment

Uh oh!

nbouliol Sep 2, 2026

Choose a reason for hiding this comment

Uh oh!

nbouliol Sep 2, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Tonours commented Aug 28, 2026 •

edited

Loading

qltysh Bot commented Aug 28, 2026 •

edited

Loading