diff --git a/.changeset/rest-list-malformed-filter-rejected.md b/.changeset/rest-list-malformed-filter-rejected.md new file mode 100644 index 0000000000..2f4977c438 --- /dev/null +++ b/.changeset/rest-list-malformed-filter-rejected.md @@ -0,0 +1,48 @@ +--- +"@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 — 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_FILTER`, 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` (each value is a valid filter — the + *request* is ambiguous, so it does not share the malformed-filter code). + 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). + +**One wire code for one condition.** #4121 landed `400 INVALID_FILTER` for +malformed filter *arrays* on this same code path while this fix was in flight; +the non-array rejections above use that code too, so a caller asking "did my +filter run?" never has to know which branch caught it. The export route's +filter guard moves from `INVALID_REQUEST` to `INVALID_FILTER` to match — a wire +change on an existing route, and the reason it is worth making is that a client +otherwise has to handle two codes for one condition depending on which URL it +called. The route's `orderby` guard keeps `INVALID_REQUEST` (it is not a +filter). + +**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..0ddec579b6 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_FILTER` — 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_FILTER` 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/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` diff --git a/packages/metadata-protocol/src/protocol.ts b/packages/metadata-protocol/src/protocol.ts index 74888ec3f6..fefa6156fa 100644 --- a/packages/metadata-protocol/src/protocol.ts +++ b/packages/metadata-protocol/src/protocol.ts @@ -301,7 +301,7 @@ function resolveOverlaySchema(type: string, _item: unknown): z.ZodTypeAny | null } /** - * A 400 for a `$filter` that looks like a filter AST but is not one. + * A 400 for a `$filter` ARRAY that looks like a filter AST but is not one. * * The message has to be *actionable from the request*, which is the whole point * of rejecting here rather than letting a driver fail later: the caller sent a @@ -309,9 +309,11 @@ function resolveOverlaySchema(type: string, _item: unknown): z.ZodTypeAny | null * it was checked against — not a driver-internal builder state. * * Diagnoses the three shapes `isFilterAST` refuses, in the order they occur in - * practice. #4121. + * practice. #4121. Sibling of {@link unusableFilterError}, which covers the + * non-array ways a filter fails to become one (#4181); both emit + * `INVALID_FILTER` so the condition has one wire code however it was reached. */ -function malformedFilterError(filter: unknown[]): Error { +function malformedFilterArrayError(filter: unknown[]): Error { const detail = describeMalformedFilter(filter); const err: any = new Error( `Malformed $filter: ${detail} A filter array is a comparison ` + @@ -853,6 +855,33 @@ const ODATA_SPELLING: Readonly> = { filter: '$filter', select: '$select', expand: '$expand', }; +/** + * [#4181] A filter the normalizer cannot turn into a usable `FilterCondition` + * by any route other than the array shapes {@link malformedFilterArrayError} + * already diagnoses: unparseable JSON, or JSON that parses to something no + * driver can read (a number, a bare string, `null`). + * + * Carries `INVALID_FILTER` — the standard-catalog code (`errors.zod.ts`, + * "Invalid filter expression") that #4121 introduced on this same code path for + * the array case. One condition, one wire code, however the caller reached it: + * a `$filter` array with a bad operator and a `?filter=` that is not JSON are + * the same answer to the same question ("this filter cannot run"). + * + * The message states the filter was NOT APPLIED, because that is the part a + * caller cannot infer: the pre-#4181 behavior was an ordinary-looking 200 over + * the unfiltered set. + */ +function unusableFilterError(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_FILTER'; + 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, ''); @@ -3140,37 +3169,103 @@ 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); - } else if (Array.isArray(parsedFilter) && parsedFilter.length > 0) { - // `isFilterAST` was being read as a *conversion* gate, so an array - // it refused was assigned to `where` unconverted — an opaque value - // the driver then had to make sense of. Every driver now fails on - // it, so this is not a narrowing; it moves the failure to where the - // malformed filter actually arrived, with the request's own - // vocabulary in the message instead of a driver-internal one. + // 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. // - // It also closes what the driver-side fix could not: a lone - // `['and']` / `['or']` sets the join mode, matches no element, and - // emits NO predicate — the last shape that still returned every row - // silently after #3948. An empty `[]` is left alone: it means "no - // filter", and every path already treats it that way. #4121. - throw malformedFilterError(parsedFilter); + // 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 unusableFilterError(filterKey, 'must be valid JSON'); + } + } + // Filter AST array → FilterCondition object + if (isFilterAST(parsedFilter)) { + parsedFilter = parseFilterAST(parsedFilter); + } else if (Array.isArray(parsedFilter) && parsedFilter.length > 0) { + // [#4121] `isFilterAST` was being read as a *conversion* gate, + // so an array it refused was assigned to `where` unconverted — + // an opaque value the driver then had to make sense of. Every + // driver now fails on it, so this is not a narrowing; it moves + // the failure to where the malformed filter actually arrived, + // with the request's own vocabulary in the message instead of a + // driver-internal one. + // + // It also closes what the driver-side fix could not: a lone + // `['and']` / `['or']` sets the join mode, matches no element, + // and emits NO predicate — the last shape that still returned + // every row silently after #3948. An empty `[]` is left alone: + // it means "no filter", and every path already treats it that + // way — so it falls through to the shape check below, which + // passes it (an array IS an object). + throw malformedFilterArrayError(parsedFilter); + } + // [#4181] 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. #4121 above catches the + // array shapes; this catches the scalar ones, and together they are + // what lets the #4164 merge below trust `where` to be an object. + if (parsedFilter === null || typeof parsedFilter !== 'object') { + throw unusableFilterError( + 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) @@ -3268,14 +3363,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..80f4e9cabf 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_FILTER' }); + 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,150 @@ 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_FILTER, 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_FILTER', 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_FILTER' }); + 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('an unrunnable filter has ONE wire code however it was reached (#4121 + #4181)', async () => { + // #4121 rejects malformed filter ARRAYS; #4181 rejects the non-array + // ways a filter fails to become one. They landed independently on + // the same code path — a caller asking "did my filter run?" must not + // have to know which branch caught it. + const { protocol } = makeProtocol(); + const codes = new Set(); + for (const bad of [ + '{oops', // #4181 — unparseable JSON + '5', // #4181 — parses, not a filter + [['status', '!!', 'x']], // #4121 — array, unknown operator + ['and'], // #4121 — lone join keyword + ]) { + const err: any = await protocol + .findData({ object: 'showcase_task', query: { filter: bad } }) + .then(() => undefined, (e: any) => e); + expect(err, `'${JSON.stringify(bad)}' must be rejected`).toBeDefined(); + codes.add(`${err.status}/${err.code}`); + } + expect([...codes]).toEqual(['400/INVALID_FILTER']); + }); + + 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..9a9ea3004b 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_FILTER' }); + }); + + 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_FILTER' }); + }); + + 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..a87303fa35 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'; } /** @@ -4731,7 +4733,7 @@ export class RestServer { if (typeof q.filter === 'string' && q.filter.length > 0) { try { filter = JSON.parse(q.filter); } catch { - res.status(400).json({ code: 'INVALID_REQUEST', error: 'filter must be JSON' }); + res.status(400).json({ code: 'INVALID_FILTER', error: 'filter must be JSON' }); return; } } else if (q.filter && typeof q.filter === 'object') { @@ -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(',')) { 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 () => {