From 47fbc571a1ea0c99d378cd7c955393363660bc9b Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 30 Jul 2026 15:20:36 +0000 Subject: [PATCH 1/3] fix(data): a filter the server cannot apply is rejected, not silently ignored (#4181) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `?filter={status:done` — one missing quote — answered 200 with the UNFILTERED page. The JSON-parse tolerance (`catch { /* keep as-is */ }`) left the raw string on `where`, a shape no driver consumes, so the filter was dropped whole and the response was byte-for-byte a successful unfiltered query. Worst failure direction in this family: #4134 returned nothing, #4164 dropped one predicate, this returned everything. This was not a design question. The sibling `GET /data/:object/export` route has rejected the same input since it was written (400 INVALID_REQUEST, "filter must be JSON") — the list path was the outlier, and the two routes of one endpoint family answered the same malformed input in opposite ways. The guard moves into the shared normalizer so `GET /data/:object`, `POST /data/:object/query` and the runtime dispatcher inherit one answer. - Unparseable JSON → 400 INVALID_REQUEST naming the parameter, and saying the filter was NOT applied — the failure mode is what makes it urgent. - Parses but is not a filter (`?filter=5` / `"done"` / `null`) → same rejection. Usable JSON is not a usable filter. - Blank `?filter=` stays ABSENT, not malformed — the same `length > 0` guard export applies. `where` is deleted rather than left as `''`. - `filter` / `filters` / `$filter` / `where` are four spellings of ONE slot; `??` silently ran the first and discarded the rest. Different values are now 400 (merging would invent an intent the caller never expressed; picking one IS the silent drop). Identical redundant spellings pass. - Export's `orderby` gets the same rule — a sort that cannot be parsed is refused, not dropped. Lower stakes (row set unchanged) but a caller taking "latest N" via orderby+top silently got an arbitrary N. Because a filter now either parses to an object or 400s, #4164's `typeof explicitWhere === 'object'` merge guard — added precisely because this tolerance could leave a raw string on `where` — is unreachable and removed with it. Its pin test flips from "keeps the garbage" to "rejects it". Verified live on the showcase app: garbage filter 400s alone and with `top`; `?filter=5|"done"|null` 400; blank filter, all four aliases, and valid JSON / object / AST-array shapes still 200; export filter+orderby 400 while a normal export streams 6080B; #4134 (`?pageSize=5`) and #4164 (merged total=1) both unregressed. `turbo run test` 132/132. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01CKsxtrsCwSL3uAcxAdVj83 --- .../rest-list-malformed-filter-rejected.md | 37 ++++ content/docs/api/data-api.mdx | 20 ++- packages/metadata-protocol/src/protocol.ts | 114 +++++++++++-- packages/objectql/src/protocol-data.test.ts | 161 ++++++++++++++++-- .../src/protocol-unknown-query-param.test.ts | 32 ++++ packages/rest/src/rest-server.ts | 17 +- 6 files changed, 348 insertions(+), 33 deletions(-) create mode 100644 .changeset/rest-list-malformed-filter-rejected.md diff --git a/.changeset/rest-list-malformed-filter-rejected.md b/.changeset/rest-list-malformed-filter-rejected.md new file mode 100644 index 0000000000..787fdc8475 --- /dev/null +++ b/.changeset/rest-list-malformed-filter-rejected.md @@ -0,0 +1,37 @@ +--- +"@objectstack/metadata-protocol": patch +"@objectstack/rest": patch +--- + +fix(data): a filter the server cannot apply is rejected, not silently ignored (#4181) + +`GET /api/v1/data/:object?filter={status:done` — one missing quote — answered +`200` with the **unfiltered** page. The JSON-parse tolerance +(`catch { /* keep as-is */ }`) left the raw string on `where`, a shape no +driver consumes, so the filter was dropped whole and the response was +byte-for-byte a successful unfiltered query. The worst failure direction in +this family: #4134 returned nothing, #4164 dropped one predicate, this +returned everything. + +The sibling `GET /data/:object/export` route had rejected the same input since +it was written (`400 INVALID_REQUEST`, "filter must be JSON") — the list path +was the outlier. That guard now lives in the shared normalizer, so +`GET /data/:object`, `POST /data/:object/query` and the runtime dispatcher all +give one answer: + +- Unparseable JSON → `400 INVALID_REQUEST`, naming the parameter and stating + the filter was not applied. +- Parses but is not a filter (`?filter=5`, `?filter="done"`, `?filter=null`) → + same rejection; usable JSON is not a usable filter. +- Blank `?filter=` → treated as absent, as before. No error. +- `filter` / `filters` / `$filter` / `where` are four spellings of ONE slot. + Sending two with **different** values used to run one and discard the rest + silently; it is now `400 INVALID_REQUEST`. Redundant identical spellings pass. +- `orderby` on the export route gets the same treatment — a sort that cannot be + parsed is refused rather than dropped (lower stakes than a filter: the row set + is unchanged, but a caller taking "latest N" got an arbitrary N). + +**What changes for callers:** requests carrying a malformed filter now fail +loudly instead of receiving every record. Every valid filter shape — JSON +string, live object, `FilterCondition` AST array, and all four alias spellings +used alone — is unaffected. diff --git a/content/docs/api/data-api.mdx b/content/docs/api/data-api.mdx index b0eb2e0057..21f2d59b57 100644 --- a/content/docs/api/data-api.mdx +++ b/content/docs/api/data-api.mdx @@ -19,7 +19,7 @@ Query records with filtering, sorting, selection, and pagination. |:----------|:---------|:------------| | `object` | path | Object name | | `select` | query | Comma-separated field names | -| `filter` | query | Filter expression (JSON). `filters` also accepted for backward compatibility. | +| `filter` | query | Filter expression (JSON). `filters` also accepted for backward compatibility. Malformed JSON is rejected with `400 INVALID_REQUEST` — never ignored. | | `sort` | query | Sort expression (e.g. `name asc` or `-created_at`) | | `top` | query | Max records to return. No default — omitting it returns all matching records. | | `skip` | query | Offset | @@ -60,6 +60,24 @@ Reserved names cannot double as implicit filters. An object with a field literally called `count`, `cursor`, `distinct`, `object`, `search` or `top` filters it through the explicit form (`?filter={"count":3}`). +#### A filter either applies or fails — it is never ignored + +`filter`, `filters`, `$filter` and `where` are four spellings of one slot. A +value the server cannot turn into a filter is rejected with +`400 INVALID_REQUEST` rather than dropped, because a dropped filter would +return the **unfiltered** result set — a response indistinguishable from a +successful query: + +| Request | Result | +|:---|:---| +| `?filter={"status":"done"}` | filter applies | +| `?filter={status:done` (invalid JSON) | `400` — `filter must be valid JSON` | +| `?filter=5`, `?filter="done"`, `?filter=null` | `400` — parses, but is not a filter | +| `?filter=` (blank) | treated as absent — no filter, no error | +| `where` and `filter` sent with **different** values | `400` — aliases for one slot; send exactly one | + +The same rule applies to `orderby` on `GET /data/:object/export`. + **Response**: ```json { diff --git a/packages/metadata-protocol/src/protocol.ts b/packages/metadata-protocol/src/protocol.ts index 60d5aad8d0..916391e301 100644 --- a/packages/metadata-protocol/src/protocol.ts +++ b/packages/metadata-protocol/src/protocol.ts @@ -800,6 +800,26 @@ const ODATA_SPELLING: Readonly> = { filter: '$filter', select: '$select', expand: '$expand', }; +/** + * [#4181] A filter the normalizer cannot turn into a usable `FilterCondition`. + * + * Mirrors the envelope the `GET /data/:object/export` route has always emitted + * for the same input (`400 INVALID_REQUEST`), so the two routes of one endpoint + * family stop disagreeing about what a malformed filter means. `INVALID_REQUEST` + * is already registered to `@objectstack/metadata-protocol` in the ADR-0112 + * error-code ledger — no new code. + */ +function malformedFilterError(param: string, detail: string): Error { + const err: any = new Error( + `Query parameter '${param}' ${detail}. It was not applied, and an unapplied ` + + 'filter would have returned the unfiltered result set.', + ); + err.status = 400; + err.code = 'INVALID_REQUEST'; + err.param = param; + return err; +} + /** Fold a parameter name to its near-miss lookup key. */ function nearMissKey(name: string): string { return name.toLowerCase().replace(/[_-]/g, ''); @@ -3087,23 +3107,85 @@ export class ObjectStackProtocolImplementation implements } delete options.sort; - // Filter/filters/$filter → where: normalize all filter aliases + // Filter/filters/$filter → where: normalize all filter aliases. + // + // [#4181] These four names are FOUR SPELLINGS OF ONE SLOT (`filters` is + // documented as a deprecated alias of `filter`), so `??` picking the + // first non-null silently discarded the others: a body carrying both + // `where` and a different `filter` ran the `filter` and dropped the + // `where` with no signal. Two different values for one slot cannot be + // reconciled — merging them would invent an intent the caller never + // expressed, and picking one is the silent drop itself — so an + // ambiguous request is refused. Redundant identical spellings are + // harmless and pass. + const filterAliases = (['filter', 'filters', '$filter', 'where'] as const) + .filter((k) => options[k] !== undefined) + .map((k) => ({ key: k, value: options[k] })); + if (filterAliases.length > 1) { + const distinct = new Set(filterAliases.map((a) => JSON.stringify(a.value))); + if (distinct.size > 1) { + const err: any = new Error( + `Conflicting filter parameters: ${filterAliases.map((a) => `'${a.key}'`).join(', ')} ` + + 'are aliases for the same filter and were given different values. Send exactly one.', + ); + err.status = 400; + err.code = 'INVALID_REQUEST'; + throw err; + } + } + const filterValue = options.filter ?? options.filters ?? options.$filter ?? options.where; + const filterKey = filterAliases[0]?.key ?? 'filter'; delete options.filter; delete options.filters; delete options.$filter; if (filterValue !== undefined) { let parsedFilter = filterValue; - // JSON string → object - if (typeof parsedFilter === 'string') { - try { parsedFilter = JSON.parse(parsedFilter); } catch { /* keep as-is */ } - } - // Filter AST array → FilterCondition object - if (isFilterAST(parsedFilter)) { - parsedFilter = parseFilterAST(parsedFilter); + // A blank `?filter=` is ABSENT, not malformed — the same `length > 0` + // guard the export route applies before parsing. Deleting `where` + // here (rather than leaving `''` on it) is what lets every consumer + // below test presence with a plain falsy check. + if (typeof parsedFilter === 'string' && parsedFilter.trim() === '') { + delete options.where; + } else { + // [#4181] JSON string → object. Parse failure is a REJECTION, not + // a fallback. The `catch { /* keep as-is */ }` this replaces left + // the raw string on `where`, a shape no driver consumes — so the + // filter was dropped whole and `?filter={status:done` (one missing + // quote) answered 200 with the UNFILTERED page. Worst member of + // the #3948 family: #4134 zeroed, #4164 dropped one predicate, + // this returned everything. + // + // The sibling `GET /data/:object/export` route has rejected this + // exact input since it was written (`400 INVALID_REQUEST`, + // "filter must be JSON"); the list path was the outlier. The + // guard lives HERE, in the shared normalizer, so `GET + // /data/:object`, `POST /data/:object/query` and the runtime + // dispatcher all inherit one answer instead of three. + if (typeof parsedFilter === 'string') { + try { + parsedFilter = JSON.parse(parsedFilter); + } catch { + throw malformedFilterError(filterKey, 'must be valid JSON'); + } + } + // Filter AST array → FilterCondition object + if (isFilterAST(parsedFilter)) { + parsedFilter = parseFilterAST(parsedFilter); + } + // Parsed-but-unusable is the same failure one step later: + // `?filter=5` / `?filter="open"` / `?filter=null` all yield a + // non-object `where` that no driver reads. Rejecting here is what + // lets the #4164 merge below trust `where` to be an object. + if (parsedFilter === null || typeof parsedFilter !== 'object') { + throw malformedFilterError( + filterKey, + `must be a filter object or condition array, received ${parsedFilter === null ? 'null' : typeof parsedFilter}`, + ); + } + options.where = parsedFilter; } - options.where = parsedFilter; } // Populate/expand/$expand → expand (Record) @@ -3201,14 +3283,14 @@ export class ObjectStackProtocolImplementation implements // stray top-level AST junk, and count() below reads the same // `options.where`, so pagination totals see the merged predicate too. // - // Deliberately NOT merged: a non-object truthy `where` — the parse - // tolerance above keeps an unparseable `filter` JSON string as-is - // (#4181), and folding real predicates into that garbage would neither - // apply them nor surface the actual bug. That path keeps its pre-#4164 - // shape until the tolerance itself is fixed at the source. + // #4164 shipped with a `typeof explicitWhere === 'object'` guard here, + // because the parse tolerance above could leave a raw unparseable string + // on `where` and folding real predicates into that garbage would neither + // apply them nor surface the bug. #4181 fixed that at the source — a + // filter now either parses to an object or 400s — so `where` is an + // object or absent by construction and the guard is gone with it. const explicitWhere = options.where; - const whereIsMergeable = !explicitWhere || typeof explicitWhere === 'object'; - if (leftoverParams.length > 0 && whereIsMergeable) { + if (leftoverParams.length > 0) { const implicitFilters: Record = {}; for (const key of leftoverParams) { implicitFilters[key] = options[key]; diff --git a/packages/objectql/src/protocol-data.test.ts b/packages/objectql/src/protocol-data.test.ts index 7a9c956811..fa69f516a9 100644 --- a/packages/objectql/src/protocol-data.test.ts +++ b/packages/objectql/src/protocol-data.test.ts @@ -737,9 +737,20 @@ describe('ObjectStackProtocolImplementation - Data Operations', () => { } } expect(suggested.size).toBeGreaterThan(0); + // Probe each suggestion with a value of the RIGHT SHAPE for that + // parameter. A one-size probe would conflate "the endpoint rejects + // this parameter name" (what this test guards) with "the endpoint + // rejects this value" — e.g. `filter: 'title'` is a legitimate 400 + // under #4181 because a bare word is not a filter. + const probe = (spelling: string): unknown => { + const bare = spelling.replace(/^\$/, '').toLowerCase(); + if (bare === 'top' || bare === 'skip') return 1; + if (bare === 'filter' || bare === 'filters') return { status: 'open' }; + return 'title'; + }; for (const spelling of suggested) { await expect( - protocol.findData({ object: 'showcase_task', query: { [spelling]: spelling === 'top' || spelling === '$top' || spelling === 'skip' || spelling === '$skip' ? 1 : 'title' } }), + protocol.findData({ object: 'showcase_task', query: { [spelling]: probe(spelling) } }), `suggested '${spelling}' must be accepted`, ).resolves.toBeDefined(); } @@ -839,20 +850,18 @@ describe('ObjectStackProtocolImplementation - Data Operations', () => { }); }); - it('leaves a non-object where (unparseable filter JSON) untouched — pinned until #4181', async () => { - // `?filter={oops` — the parse tolerance keeps the raw string and it - // becomes `where`. Folding real predicates into that garbage would - // neither apply them nor surface the tolerance bug itself (#4181), - // so this path keeps its pre-#4164 shape: string where forwarded, - // implicit key left as a stray option. + it('an unparseable filter is now rejected, not carried past the merge (#4181)', async () => { + // The #4164 pin test lived here expecting `where === '{oops'` and a + // stray `owner_id` option. #4181 is the source fix it named: the + // filter never reaches the merge because it never parses. const { protocol, engine } = makeProtocol(); - await protocol.findData({ - object: 'showcase_task', - query: { filter: '{oops', owner_id: 'usr_1' }, - }); - const opts = engine.find.mock.calls[0][1]; - expect(opts.where).toBe('{oops'); - expect(opts.owner_id).toBe('usr_1'); + await expect( + protocol.findData({ + object: 'showcase_task', + query: { filter: '{oops', owner_id: 'usr_1' }, + }), + ).rejects.toMatchObject({ status: 400, code: 'INVALID_REQUEST' }); + expect(engine.find).not.toHaveBeenCalled(); }); it('rejects even when an explicit filter rode along — the 400 must not depend on it', async () => { @@ -944,4 +953,128 @@ describe('ObjectStackProtocolImplementation - Data Operations', () => { expect(engine.find.mock.calls[0][1].where).toEqual({ zzzz: '1' }); }); }); + + // ═══════════════════════════════════════════════════════════════ + // #4181 — a filter that cannot be applied must FAIL, never be forwarded + // + // `?filter={status:done` (one missing quote) used to fall through + // `catch { /* keep as-is */ }`, leaving the raw string on `where` — a shape + // no driver consumes — so the filter was dropped WHOLE and the response was + // the unfiltered page. Worst member of the #3948 family by direction: + // #4134 zeroed, #4164 dropped one predicate, this returned everything. + // The sibling `/export` route had rejected the same input all along. + // ═══════════════════════════════════════════════════════════════ + + describe('malformed filters are rejected, not silently unapplied (#4181)', () => { + function makeProtocol() { + const engine: any = { + find: vi.fn().mockResolvedValue([]), + findOne: vi.fn().mockResolvedValue(null), + count: vi.fn().mockResolvedValue(0), + registry: { + getObject: vi.fn((name: string) => ({ + name, + fields: { title: { type: 'text' }, status: { type: 'text' } }, + })), + }, + }; + return { protocol: new ObjectStackProtocolImplementation(engine), engine }; + } + + it.each(['filter', 'filters', '$filter'])( + 'rejects unparseable JSON on ?%s with 400 INVALID_REQUEST, before the engine', + async (alias) => { + const { protocol, engine } = makeProtocol(); + await expect( + protocol.findData({ object: 'showcase_task', query: { [alias]: '{status:done' } }), + ).rejects.toMatchObject({ status: 400, code: 'INVALID_REQUEST', param: alias }); + expect(engine.find).not.toHaveBeenCalled(); + }, + ); + + it('says the filter was NOT applied — the failure mode is what makes it urgent', async () => { + const { protocol } = makeProtocol(); + await expect( + protocol.findData({ object: 'showcase_task', query: { filter: '{oops' } }), + ).rejects.toThrow(/must be valid JSON.*not applied.*unfiltered result set/s); + }); + + it.each([ + ['a number', '5'], + ['a quoted string', '"open"'], + ['null', 'null'], + ['a boolean', 'true'], + ])('rejects a filter that parses to %s — usable JSON is not a usable filter', async (_label, raw) => { + const { protocol, engine } = makeProtocol(); + await expect( + protocol.findData({ object: 'showcase_task', query: { filter: raw } }), + ).rejects.toMatchObject({ status: 400, code: 'INVALID_REQUEST' }); + expect(engine.find).not.toHaveBeenCalled(); + }); + + it('still accepts every legitimate filter shape', async () => { + const { protocol, engine } = makeProtocol(); + // JSON string, live object, and the FilterAST tuple array. + await protocol.findData({ object: 'showcase_task', query: { filter: '{"status":"done"}' } }); + await protocol.findData({ object: 'showcase_task', query: { filter: { status: 'done' } } }); + await protocol.findData({ object: 'showcase_task', query: { filter: [['status', '=', 'done']] } }); + expect(engine.find).toHaveBeenCalledTimes(3); + for (const call of engine.find.mock.calls) expect(call[1].where).toBeTruthy(); + }); + + it('treats a blank ?filter= as ABSENT, not malformed', async () => { + // The same `length > 0` guard the export route applies. A blank + // filter must not 400, and must not leave `''` on `where` either. + const { protocol, engine } = makeProtocol(); + await protocol.findData({ object: 'showcase_task', query: { filter: ' ' } }); + expect(engine.find).toHaveBeenCalledOnce(); + expect(engine.find.mock.calls[0][1].where).toBeUndefined(); + }); + + // ── the alias chain silently picking a winner ────────────────── + + it('refuses conflicting values across filter aliases instead of dropping all but one', async () => { + // `??` used to run the `filter` and discard the `where` with no + // signal. They are spellings of ONE slot, so two different values + // cannot be reconciled — merging would invent an intent. + const { protocol, engine } = makeProtocol(); + await expect( + protocol.findData({ + object: 'showcase_task', + query: { where: { status: 'done' }, filter: { status: 'open' } }, + }), + ).rejects.toMatchObject({ status: 400, code: 'INVALID_REQUEST' }); + expect(engine.find).not.toHaveBeenCalled(); + }); + + it('names every conflicting alias and prescribes the fix', async () => { + const { protocol } = makeProtocol(); + await expect( + protocol.findData({ + object: 'showcase_task', + query: { filter: { a: 1 }, filters: { b: 2 } }, + }), + ).rejects.toThrow(/'filter'.*'filters'.*Send exactly one/s); + }); + + it('lets redundant IDENTICAL spellings through — only a conflict is ambiguous', async () => { + const { protocol, engine } = makeProtocol(); + await protocol.findData({ + object: 'showcase_task', + query: { filter: { status: 'done' }, filters: { status: 'done' } }, + }); + expect(engine.find.mock.calls[0][1].where).toEqual({ status: 'done' }); + }); + + it('one alias alone is never a conflict', async () => { + const { protocol, engine } = makeProtocol(); + for (const alias of ['filter', 'filters', '$filter', 'where']) { + await protocol.findData({ object: 'showcase_task', query: { [alias]: { status: 'done' } } }); + } + expect(engine.find).toHaveBeenCalledTimes(4); + for (const call of engine.find.mock.calls) { + expect(call[1].where).toEqual({ status: 'done' }); + } + }); + }); }); diff --git a/packages/objectql/src/protocol-unknown-query-param.test.ts b/packages/objectql/src/protocol-unknown-query-param.test.ts index 38d5631574..c40724168f 100644 --- a/packages/objectql/src/protocol-unknown-query-param.test.ts +++ b/packages/objectql/src/protocol-unknown-query-param.test.ts @@ -239,6 +239,38 @@ describe('#4134 — unknown list query params (real ObjectQL engine)', () => { expect(r.total).toBe(0); }); + // ───────────────────────────────────────────────────────────── + // #4181 — an unappliable filter must not answer like a satisfied one + // ───────────────────────────────────────────────────────────── + + it('an unparseable filter 400s instead of returning the UNFILTERED page (#4181)', async () => { + // The exact repro: one missing quote. Pre-#4181 this returned + // `total: 10` — identical to the no-filter baseline, so a caller whose + // filter never ran could not tell from the response. + const baseline: any = await protocol.findData({ object: 'showcase_task' }); + expect(baseline.total).toBe(10); + + await expect( + protocol.findData({ object: 'showcase_task', query: { filter: '{status:done' } }), + ).rejects.toMatchObject({ status: 400, code: 'INVALID_REQUEST' }); + }); + + it('the same garbage filter cannot hide behind pagination either', async () => { + // Pre-#4181: `total: 10` + 2 records — a perfectly normal-looking page + // of an unfiltered set. + await expect( + protocol.findData({ object: 'showcase_task', query: { filter: '{status:done', top: 2 } }), + ).rejects.toMatchObject({ status: 400, code: 'INVALID_REQUEST' }); + }); + + it('a VALID filter still applies — the guard rejects only what it cannot run', async () => { + const r: any = await protocol.findData({ + object: 'showcase_task', + query: { filter: '{"status":"done"}' }, + }); + expect(r.total).toBe(2); + }); + it('pagination totals are computed over the MERGED predicate', async () => { // 8 open rows share created_at; page of 3 → total must be 8 (the // merged match count), not 10 (unfiltered) nor 3 (page-local). diff --git a/packages/rest/src/rest-server.ts b/packages/rest/src/rest-server.ts index 48274ca3a6..ea896af995 100644 --- a/packages/rest/src/rest-server.ts +++ b/packages/rest/src/rest-server.ts @@ -617,7 +617,9 @@ function isExpectedDataStatus(status: number): boolean { * not a server fault worth an "[REST] Unhandled error" line per request. */ function isExpectedQueryRejection(body: Record | undefined): boolean { - return body?.code === 'UNSUPPORTED_QUERY_PARAM' || body?.code === 'INVALID_FIELD'; + return body?.code === 'UNSUPPORTED_QUERY_PARAM' + || body?.code === 'INVALID_FIELD' + || body?.code === 'INVALID_REQUEST'; } /** @@ -4742,7 +4744,18 @@ export class RestServer { if (typeof q.orderby === 'string' && q.orderby.length > 0) { // Accept "field:dir,field2:dir" shorthand or a JSON object. if (q.orderby.startsWith('{') || q.orderby.startsWith('[')) { - try { orderby = JSON.parse(q.orderby); } catch { /* leave undefined */ } + // [#4181] Same rule as `filter` two blocks up: a sort + // the server cannot parse is refused, not dropped. + // Lower stakes than a dropped filter (the row SET is + // unchanged, only its order), but a caller taking + // "latest N" via orderby+top silently got an + // arbitrary N. + try { + orderby = JSON.parse(q.orderby); + } catch { + res.status(400).json({ code: 'INVALID_REQUEST', error: 'orderby must be JSON' }); + return; + } } else { const obj: Record = {}; for (const part of q.orderby.split(',')) { From 06113f446e226fbc07a374a800c56b88bd0572ab Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 30 Jul 2026 15:58:27 +0000 Subject: [PATCH 2/3] fix(data): align malformed-filter rejections onto one wire code (#4181) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reconciles with #4121 (`fix(metadata)!: a $filter array that is not a filter AST is rejected`), which landed on this same code path while #4181 was in flight and is why this branch conflicted. The two fixes are complementary halves of one condition — #4121 catches the malformed ARRAY shapes, #4181 the non-array ways a filter fails to become one (unparseable JSON; JSON that parses to a number / bare string / null) — so they now share the standard-catalog `INVALID_FILTER` code and a matching pair of helpers (`malformedFilterArrayError` / `unusableFilterError`; the name collision between them is what git could not resolve). A caller asking "did my filter run?" must not have to know which branch caught it, and a new test pins exactly that: four differently-broken filters, one `400/INVALID_FILTER`. The export route's filter guard moves `INVALID_REQUEST` → `INVALID_FILTER` for the same reason. That code WAS pinned by a test — my earlier grep looked for the message string and missed an assertion on the code — so the change is deliberate and the test now records why. Export's `orderby` guard keeps `INVALID_REQUEST`: a sort is not a filter, and it never returned a wrong row set. That guard also gains the test it never had. Ordering in the chain is what makes them compose: blank → absent; string → JSON.parse or reject; array → AST-convert or reject (#4121); scalar → reject (#4181); everything surviving is an object, which is what lets #4164's merge trust `where`. An empty `[]` still means "no filter" and passes. #4121's 12 tests and this branch's 103 both green; turbo test 132/132. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01CKsxtrsCwSL3uAcxAdVj83 --- packages/rest/src/rest.test.ts | 31 +++++++++++++++++++++++++++++-- 1 file changed, 29 insertions(+), 2 deletions(-) diff --git a/packages/rest/src/rest.test.ts b/packages/rest/src/rest.test.ts index c5aeed0db8..8fb6b2b887 100644 --- a/packages/rest/src/rest.test.ts +++ b/packages/rest/src/rest.test.ts @@ -1056,7 +1056,13 @@ describe('RestServer', () => { expect(text).toBe('[{"id":"a"},{"id":"b"}]'); }); - it('rejects invalid JSON in filter query', async () => { + // [#4181] Was `INVALID_REQUEST`. The list path now rejects the same input + // with the purpose-built `INVALID_FILTER` (the standard-catalog code #4121 + // introduced for malformed filter arrays), so export moves onto it too — + // otherwise a client handling "my filter was refused" needs two codes + // depending on which URL it called. Deliberate wire change; the status and + // the message are unchanged. + it('rejects invalid JSON in filter query with the shared malformed-filter code', async () => { const rest = new RestServer(server as any, protocol as any, ANON_API as any); (rest as any).resolveExecCtx = async () => ({ userId: 'test-user' }); rest.registerRoutes(); @@ -1067,7 +1073,28 @@ describe('RestServer', () => { params: { object: 'account' }, query: { filter: '{not json' }, } as any, res); - expect(res.json).toHaveBeenCalledWith(expect.objectContaining({ code: 'INVALID_REQUEST' })); + expect(res.json).toHaveBeenCalledWith( + expect.objectContaining({ code: 'INVALID_FILTER', error: 'filter must be JSON' }), + ); + }); + + // [#4181] The sibling guard one block down: an unparseable `orderby` used to + // fall through to `undefined` and export UNSORTED. Keeps `INVALID_REQUEST` — + // a sort is not a filter, and the row set was never wrong. + it('rejects invalid JSON in orderby query instead of exporting unsorted', async () => { + const rest = new RestServer(server as any, protocol as any, ANON_API as any); + (rest as any).resolveExecCtx = async () => ({ userId: 'test-user' }); + rest.registerRoutes(); + const route = getExportRoute(rest); + + const { res } = makeRes(); + await route!.handler({ + params: { object: 'account' }, + query: { orderby: '{not json' }, + } as any, res); + expect(res.json).toHaveBeenCalledWith( + expect.objectContaining({ code: 'INVALID_REQUEST', error: 'orderby must be JSON' }), + ); }); it('honours the hard 50k row cap', async () => { From d0951bf93f927a5924b9ec838fc9f95f30983195 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 30 Jul 2026 16:09:45 +0000 Subject: [PATCH 3/3] docs(api): the INVALID_FILTER catalog entry now describes what it covers (#4181) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The entry described only "an invalid operator or field in the where clause". After #4121 and #4181 the same code also answers a $filter array that is not a filter AST, a filter parameter that is not valid JSON, and JSON that parses to a non-filter — so a caller who hit one of those found a catalog entry that did not describe their request. Adds the sentence the whole family exists for: a filter the server cannot run is never skipped, because skipping it returns the UNFILTERED set. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01CKsxtrsCwSL3uAcxAdVj83 --- content/docs/api/error-catalog.mdx | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/content/docs/api/error-catalog.mdx b/content/docs/api/error-catalog.mdx index 9ea0d9d86f..4f612c8f09 100644 --- a/content/docs/api/error-catalog.mdx +++ b/content/docs/api/error-catalog.mdx @@ -122,8 +122,15 @@ substitute. See the [Data API](/docs/api/data-api). **Retry:** `no_retry` ### `INVALID_FILTER` -**Cause:** A filter operator or field in the `where` clause is invalid. -**Fix:** Use valid filter operators (`$eq`, `$ne`, `$gt`, `$lt`, `$in`, `$contains`, etc.). +**Cause:** The server could not turn the request's filter into something it can +run. Covers an invalid operator or field in the `where` clause, a `$filter` +array that is not a filter AST (a bad operator, a lone `["and"]`), a `filter` +query parameter that is not valid JSON, and JSON that parses to something that +is not a filter (`5`, `"done"`, `null`). +**Fix:** Use valid filter operators (`$eq`, `$ne`, `$gt`, `$lt`, `$in`, `$contains`, etc.) and a filter shape the [Data API](/docs/api/data-api) accepts — an object, or an array of `[field, operator, value]` conditions. The `error` message names the offending element. +**Why it is an error and not an empty result:** a filter the server cannot run +is never silently skipped, because skipping it would return the **unfiltered** +result set — a response indistinguishable from a successful query. **Retry:** `no_retry` ### `INVALID_SORT`