fix(agent-bff): reject unknown keys on list and count bodies

[Tonours]

What

List and count bodies reject an undeclared top-level key with 400 invalid_request. Same for a stray key inside a sort clause, page, or a filter node.

Body	Before	After
{"filters": {...}}	200, all rows	400 Unrecognized key: "filters"
{"projections": ["id"]}	200, all fields	400
{"sort": [{"field": "a", "direciton": "desc"}]}	200, sorted ascending	400
{"parentId": "7"} on a plain list	200, ignored	400
{"projection": [...]} on a count	200, ignored	400
{"filter": ..., "timezone": ..., "parentId": ...}	200	200, unchanged

fixes PRD-1098

Why

The bodies were parsed with plain zod objects, which strip unknown keys. filters instead of filter was dropped silently, so the query ran unfiltered and answered 200: a typo returned the wrong rows with a success status. The TS client protects nobody using curl, plain JS or an LLM-built body, and that is this BFF's audience.

How

Every request object is z.strictObject. filter and parentId are declared but not re-validated: each already has its own validator, and a second rule would only disagree with the first at the margins.

timezone is typed, z.string().regex(/\S/).optional(). It cannot disagree with resolveTimezone, which is strictly stricter — it needs a non-empty valid IANA name and rejects the rest with its own error. What the type adds is a 400 for a non-string, and that case mattered: bodyTimezone returns undefined for anything that is not a string, so timezone: null used to be dropped and the request resolved from the header or the deployment default. A blank used to take the same silent path from the other side — resolveTimezone skips empty candidates rather than rejecting them — so the regex refuses one instead. A date filter (Today, PreviousXDays) then ran in the wrong timezone, in a 200, with no trace.

The published components are derived from those same schemas, with a satisfies OverridesOf<...> clause that refuses a decoration for a key the runtime does not accept. The document cannot advertise a shape the parser rejects. Body types are z.infer of the same schemas.

The document closes too, structurally

Every request body in both document forms carries additionalProperties: false, including the per-collection SortClause_${key} and every leaf and branch of the filter tree — a generated client now refuses direciton or valu before the request leaves.

Getting there meant dropping the allOf composition on relation bodies: additionalProperties: false on one branch forbids the key the other adds, and oas31 has no unevaluatedProperties to lift that. Relation bodies now spread the foreign collection's properties into one flat closed object with parentId, which is what schemas.ts already did with RelationListFlatInputs.extend(...).

Breaking

A stray top-level key now draws a 400 where it used to give a silently different result set. The full surface, on all four routes (list, count, relation list, relation count):

• any undeclared top-level key;
• an undeclared key inside a sort element or inside page;
• a non-string or blank timezone (null and "" included), which used to be silently dropped or skipped;
• parentId on a plain list or count, where a parent id means nothing;
• projection, sort or page on a count or a relation count, where paging and projection mean nothing.

The last two are the likely ones in practice: a client reusing one body builder across route families used to have the extra keys stripped.

Observability

Every body the parser rejects logs a Warn carrying the reason — the offending key name, never a submitted value, which a test asserts. That covers every throw site, not only the zod one: a non-object body, a non-object filter, a stray key anywhere inside the filter tree, and a bad parentId, which on relation routes is parsed before the schema. Without it this contract change would ship blind: BffHttpError is serialized and returned before error-middleware reaches its logger, and there is no access log, so nobody would see a client that has been sending filters for six months.

When several rules fail at once, the message names the unrecognized key rather than whichever issue zod happened to list first — {"filters": {}, "page": {"limit": 0}} used to answer page.limit: Too small, sending the caller to fix the wrong thing.

Known limitations

One sibling of the same bug stays open, worth a ticket:

• values misspelled on an action body: the key is read with a cast, not parsed, so valeus executes the action with no submitted values at all.

PRD-1099 — a non-JSON content type makes @koa/bodyparser set ctx.request.body = {}, so a fetch POST with no Content-Type header loses the whole body and answers 200 unfiltered. That one is the same symptom as this ticket one layer up, and it is fixed in #1856.

Stale claim spotted, not rewritten here: the ConditionTree description says relation list and count forward the filter without checks, while both checks run.

How to test

yarn workspace @forestadmin/agent-bff test && yarn workspace @forestadmin/agent-bff lint

By hand, POST a list body with filters instead of filter: 400 naming the key, where it used to return every row.

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]

PRD-1098

[qltysh]

Coverage Impact

This PR will not change total coverage.

Modified Files with Diff Coverage (5)

Rating	File	% Diff	Uncovered Line #s
	packages/agent-bff/src/data/request-schemas.ts	100.0%
	packages/agent-bff/src/openapi/unfolded-paths.ts	100.0%
	packages/agent-bff/src/data/agent-query.ts	100.0%
	packages/agent-bff/src/openapi/schemas.ts	100.0%
	packages/agent-bff/src/data/data-routes-middleware.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]

No blocker: the closure holds end-to-end on list, count and both relation parsers, and filter/timezone/parentId still pass. A few remarks inline, mostly about the unfolded document staying more open than the generic one.

[nbouliol]

Closure verified end-to-end on list, count and both relation parsers, and the generic document is derived from the same schemas rather than restated. Remaining remarks are about the unfolded document staying more open than the runtime, none blocking.

[Tonours]

Second push, from the review passes.

timezone is typed now. The PR said re-typing it would turn a tolerated null into a 400, and that reasoning was wrong: z.string() cannot disagree with resolveTimezone, which is strictly stricter — it needs a non-empty valid IANA name and rejects everything else with its own error. All the type adds is a 400 for a non-string, and that case is not benign. bodyTimezone (timezone-middleware.ts:18) returns undefined for anything that is not a string, so timezone: null was dropped and the request resolved from the header or the deployment default. A Today or PreviousXDays filter then ran in the wrong timezone — rows around midnight in or out of the result — in a 200, with no trace. That is the same class of bug as filters, which is what this PR is about. The test that pinned the old behaviour was named "should leave a null timezone to the middleware"; the middleware never handled it, it discarded it.

Rejections are logged. BffHttpError is serialized and returned before error-middleware reaches its logger, and there is no access log — so this contract change would have shipped with no way to see it working. Every rejected body now logs a Warn with the reason. The reason is a key name, never a submitted value, and a test asserts that.

The message names the right key. assertFlatInputs took issues[0], and zod does not order issues by relevance: {"filters": {}, "page": {"limit": 0}} answered page.limit: Too small, so the caller fixed their page, replayed, and got a second 400 for the typo that actually changed their results. It now prefers the unrecognized_keys issue.

One published claim corrected. CLOSED_BODY_NOTE said a leaf carrying valu "runs with no value". Optimistic — the leaf is built with value: undefined, TypeGetter reads it as null, FieldValidator accepts it, and query-converter emits { [Op.eq]: undefined }. What comes back is datasource-dependent. The note now says the contract does not define it.

Not doing here, and why:

• Non-JSON content type. @koa/bodyparser sets ctx.request.body = {} when the content type is neither JSON nor urlencoded, so a fetch POST with no Content-Type header loses filter, projection, sort, page and timezone and answers 200 on the whole collection — this ticket's symptom, one layer up, and it bypasses the strict parse entirely. It is PRD-1099 and it is fixed in fix(agent-bff): reject non-json content types with 415 #1856. Worth knowing the two interact: curl -d sends urlencoded, so the body becomes one long unknown key and this PR catches it; only the fetch path needs fix(agent-bff): reject non-json content types with 415 #1856.
• Strict filter leaves and values on an action body — named in Known limitations, each worth its own ticket. The action one is partly closed by fix(agent-bff): enforce the action input contract on execute #1860, which rejects a missing required field at execute.

[nbouliol]

The five points from the previous round are fixed and verified — SortClause_${key} closed including the degraded branch, relation bodies flattened out of the allOf while keeping the foreign refs and required: ['parentId'], OverridesOf, the count break tested and documented, and the relation prose mentioning search.

The filter-tree strictness in 259dc95 is the one that landed without the document following it: the published note, the FilterLeaf_* schema, one test's premise and the Known limitations bullet all still describe the old behaviour. Details inline.

[nbouliol]

assertNoStrayKey now rejects valu on a leaf with a 400, so this note says the opposite of what the runtime does — and it ships on all four bodies in both document forms. Worth rewriting alongside the Known limitations bullet that still lists the leaf typo as open?

[nbouliol]

This locks in the pre-259dc95 contract: the runtime no longer forwards the extra key, it 400s. FilterLeaf_* and Filter_* still publish no additionalProperties: false, so a generated client sends valu and gets rejected.

[nbouliol]

One description serves both bodies, so RelationCountRequest_* advertises sort and projection while body(foreign.properties.count) now closes without them — a client following the prose gets a 400. schemas.ts keeps the bare note on the count for that reason.

[nbouliol]

z.string() still admits "", and resolveTimezone skips empty candidates rather than rejecting them, so {"timezone": ""} falls back to the header or the default — the same silent wrong-timezone path the How section says this closes. .regex(/\S/) like ParentIdInput?

[nbouliol]

Only the zod path logs, so the filter-tree strays, a non-object body, filter must be an object and every parseParentId failure still reject silently — and on relation routes parseParentId runs before the schema. "Every rejected body logs a Warn" needs either the other throw sites or a narrower claim.

[nbouliol]

Eight of the ten points from the two earlier rounds are fully closed. Worth calling out that the filter-tree closure avoids the JSON Schema trap the allOf composition hit: additionalProperties: false sits inside each anyOf member rather than beside it, so it actually bites, and the published key sets match LEAF_KEYS/BRANCH_KEYS exactly.

Two are partial: the blank-timezone rejection holds on the four data routes but not on the action ones, and serializePage is the one body rejection still throwing without a log. Nothing blocking.

One line in the description to narrow: "Every request body in both document forms carries additionalProperties: false" — action bodies deliberately do not (schemas.ts:152 is a plain z.object, per the hook rationale at unfolded-paths.ts:404).

[nbouliol]

These two sentences disagree: the timezone middleware runs before the data routes, so with X-Forest-Timezone: Europe/Paris and {"timezone": ""} the header does resolve and the body still 400s. Worth saying the header wins only when the body key is absent?

[nbouliol]

The Timezone component is reused here, but action bodies are never zod-parsed — action-routes-middleware.ts:191 reads them by cast — so {"timezone": ""} still resolves from the header or the default on /actions/*, which is what its description now says cannot happen.

[nbouliol]

agent-query.ts:82 keeps filter: {} allowed on purpose, but it matches neither union member, so a generated client refuses a body the BFF accepts. An empty-object member, or dropping the runtime allowance?

[nbouliol]

serializePage at :143 is the one body rejection still throwing without this, so Observability's "covers every throw site" is one short. It is also the only message carrying submitted values, so routing it through here needs the message reworded first.

[nbouliol]

logger is now threaded through six functions, including the exported parseParentId, only to emit one fixed message. A try/catch here and in the two relation parsers logging error.message would remove all six parameters, keep the exported signature, and would have caught the serializePage site.

[nbouliol]

/\S/ is now the second inline non-blank rule in this file, and it is hard-coded twice more as '\\S' in unfolded-paths.ts. One named NonBlankString would carry the rule and one message.

[nbouliol]

This asserts the same {type, status} as the non-string test above, so it cannot tell a blank-timezone rejection from any other 400 — worth asserting the reason names timezone.