diff --git a/.changeset/reject-malformed-filter-array.md b/.changeset/reject-malformed-filter-array.md new file mode 100644 index 0000000000..82949b6dcf --- /dev/null +++ b/.changeset/reject-malformed-filter-array.md @@ -0,0 +1,47 @@ +--- +"@objectstack/metadata-protocol": minor +--- + +fix(metadata)!: a `$filter` array that is not a filter AST is rejected, not passed through (#4121) + +`isFilterAST` was being read as a *conversion* gate: an array it refused was +assigned to `options.where` unconverted, leaving each backend to make sense of a +value the protocol had already decided it could not parse. + +Item 2 of #3948, filed as error-locality work. The investigation found it is +more than that. + +**It closes the last silently-unfiltered shape.** #3948 made the drivers throw +on a bare triple with an unknown operator and on any element that is neither a +join keyword nor a condition array. What it could not reach is a lone `['and']` +or `['or']`: the driver sets its join mode, matches no element, emits **no +predicate**, and returns every row. `isFilterAST` refuses it (a logical node +needs `length >= 2`), so it arrived as an opaque `where` and no driver-side +check applied. That is now a 400. + +**For every other shape this is not a narrowing.** driver-sql throws on all of +them, driver-memory throws, driver-mongodb reaches its own parser and fails at +the server. Rejecting at the protocol changes *which* error the caller sees, not +*whether* there is one — and the message is in the request's own vocabulary +(`unrecognised operator "not in"`, `element 1 is number`, plus the recognised +operator list) rather than a driver's internal builder state. + +Scoped narrowly, because the regression to fear is rejecting something valid: + +- only `Array.isArray(filter)` values are in scope — a `where` **object** is + untouched, including `$and`/`$or`/`$gte` shapes; +- an empty `[]` is left alone: it means "no filter", and every path already + treats it that way; +- `isFilterAST` accepts nested arrays, so `[[a,'=',1],[b,'=',2]]` and + `['and', […], ['or', …]]` keep converting. A naive "arrays are suspect" rule + would have broken exactly those, which is why the accepted shapes are pinned + by more tests than the rejected ones. + +Errors carry `status: 400` and `code: 'INVALID_FILTER'`, matching the +`UNSUPPORTED_QUERY_PARAM` convention alongside. + +Verified: 12 new tests driving the real `findData` normalisation, not a +re-implementation of its rule — six for shapes that must keep converting, six +for shapes that must be rejected, including the exact message text. Reverting the +change fails six of them. Full `@objectstack/metadata-protocol` suite: **122 +tests across 19 files**, green. diff --git a/packages/metadata-protocol/src/protocol.malformed-filter.test.ts b/packages/metadata-protocol/src/protocol.malformed-filter.test.ts new file mode 100644 index 0000000000..07f7cc4162 --- /dev/null +++ b/packages/metadata-protocol/src/protocol.malformed-filter.test.ts @@ -0,0 +1,149 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * A `$filter` that looks like a filter AST but is not one is rejected at the + * protocol, not handed to the driver as an opaque `where` (#4121). + * + * `isFilterAST` was being read as a *conversion* gate: an array it refused was + * assigned to `options.where` unconverted, leaving each backend to make sense of + * it. #3948 made the drivers throw rather than skip, so rejecting here buys + * (a) one error instead of a different one per backend, (b) a message in the + * *request's* vocabulary rather than a builder's internal state, and (c) the one + * shape the driver-side fix could not reach. + * + * That shape is a lone `['and']`: the driver sets its join mode, matches no + * element, emits no predicate, and returns **every row** — silently. It is the + * last survivor of the class #3948 closed, which makes this a correctness fix + * and not only an ergonomic one. + * + * The regression this change could plausibly cause is rejecting a filter that + * IS valid, so the accepted shapes are pinned at least as hard as the rejected + * ones. `isFilterAST` accepts nested arrays, and a naive "arrays are suspect" + * rule would have broken `[[a,'=',1],[b,'=',2]]`. + */ +import { describe, it, expect, vi } from 'vitest'; +import { ObjectStackProtocolImplementation } from './protocol.js'; + +const SCHEMA = { + name: 'invoice', + fields: { + status: { name: 'status', type: 'text' }, + close_date: { name: 'close_date', type: 'date' }, + amount: { name: 'amount', type: 'number' }, + stage: { name: 'stage', type: 'text' }, + a: { name: 'a', type: 'number' }, + b: { name: 'b', type: 'number' }, + c: { name: 'c', type: 'number' }, + }, +}; + +function makeProtocol() { + const find = vi.fn(async () => []); + const engine = { + registry: { getObject: (n: string) => (n === 'invoice' ? SCHEMA : undefined) }, + find, + count: vi.fn(async () => 0), + }; + return { p: new ObjectStackProtocolImplementation(engine as any), find }; +} + +/** Run a `$filter` through the real `findData` normalisation. */ +async function whereFor(filter: unknown): Promise { + const { p, find } = makeProtocol(); + await p.findData({ object: 'invoice', query: { $filter: filter } } as never); + return (find.mock.calls[0] as unknown[])[1] as unknown; +} + +async function errorFor(filter: unknown): Promise<{ message: string; status?: number; code?: string }> { + const { p } = makeProtocol(); + try { + await p.findData({ object: 'invoice', query: { $filter: filter } } as never); + } catch (e) { + const err = e as Error & { status?: number; code?: string }; + return { message: err.message, status: err.status, code: err.code }; + } + throw new Error(`expected ${JSON.stringify(filter)} to be rejected`); +} + +describe('filters that must keep working', () => { + it('converts a bare comparison triple', async () => { + expect(await whereFor(['status', '=', 'active'])).toMatchObject({ where: { status: 'active' } }); + }); + + it('converts a flat list of conditions — the shape a naive rule would break', async () => { + const opts = await whereFor([['a', '=', 1], ['b', '=', 2]]) as { where: unknown }; + expect(opts.where).toBeDefined(); + expect(Array.isArray(opts.where)).toBe(false); + }); + + it('converts logical nodes, including nested ones', async () => { + for (const f of [ + ['and', ['a', '=', 1], ['b', '=', 2]], + ['or', ['a', '=', 1], ['b', '=', 2]], + ['and', ['a', '=', 1], ['or', ['b', '=', 2], ['c', '=', 3]]], + ]) { + const opts = await whereFor(f) as { where: unknown }; + expect(Array.isArray(opts.where), JSON.stringify(f)).toBe(false); + } + }); + + it('converts the date operators #3948 added to the AST vocabulary', async () => { + for (const op of ['before', 'after']) { + const opts = await whereFor(['close_date', op, '2024-01-01']) as { where: unknown }; + expect(Array.isArray(opts.where), op).toBe(false); + } + }); + + it('leaves an empty filter alone — it means "no filter"', async () => { + const opts = await whereFor([]) as { where: unknown }; + expect(opts.where).toEqual([]); + }); + + it('never touches a `where` object; only arrays are in scope', async () => { + for (const f of [{ status: 'active' }, { $and: [{ a: 1 }, { b: 2 }] }, { amount: { $gte: 100 } }]) { + const opts = await whereFor(f) as { where: unknown }; + expect(opts.where).toEqual(f); + } + }); +}); + +describe('filters that must be rejected', () => { + it('rejects a lone logical keyword — the remaining silent-unfiltered shape', async () => { + // The driver sets its join mode, matches nothing, emits no predicate, + // and returns every row. Nothing downstream can catch this. + for (const f of [['and'], ['or']]) { + const err = await errorFor(f); + expect(err.message).toContain('has no conditions to join'); + expect(err.status).toBe(400); + expect(err.code).toBe('INVALID_FILTER'); + } + }); + + it('rejects a triple whose operator is outside the AST vocabulary, and names it', async () => { + const err = await errorFor(['status', 'bogusop', 'x']); + expect(err.message).toContain('unrecognised operator "bogusop"'); + expect(err.status).toBe(400); + }); + + it('rejects the space-spelled `not in`, which no vocabulary defines', async () => { + const err = await errorFor(['stage', 'not in', ['won', 'lost']]); + expect(err.message).toContain('unrecognised operator "not in"'); + }); + + it('rejects an array of condition OBJECTS — a different filter dialect', async () => { + const err = await errorFor([{ field: 'a', operator: '=', value: 1 }]); + expect(err.status).toBe(400); + }); + + it('rejects a mixed array with a non-condition element, naming its position', async () => { + expect((await errorFor([['a', '=', 1], 42])).message).toContain('element 1 is number'); + expect((await errorFor([['a', '=', 1], null])).message).toContain('element 1 is null'); + }); + + it('quotes the recognised vocabulary, so the fix is in the error', async () => { + const err = await errorFor(['status', 'bogusop', 'x']); + expect(err.message).toContain('Recognised operators:'); + expect(err.message).toContain('not_in'); + expect(err.message).toContain('starts_with'); + }); +}); diff --git a/packages/metadata-protocol/src/protocol.ts b/packages/metadata-protocol/src/protocol.ts index 60d5aad8d0..74888ec3f6 100644 --- a/packages/metadata-protocol/src/protocol.ts +++ b/packages/metadata-protocol/src/protocol.ts @@ -18,7 +18,7 @@ import type { } from '@objectstack/spec/api'; import type { MetadataCacheRequest, MetadataCacheResponse, ServiceInfo, ApiRoutes, WellKnownCapabilities } from '@objectstack/spec/api'; import { readServiceSelfInfo } from '@objectstack/spec/api'; -import { parseFilterAST, isFilterAST, type DroppedFieldsEvent, type QueryAST } from '@objectstack/spec/data'; +import { parseFilterAST, isFilterAST, VALID_AST_OPERATORS, type DroppedFieldsEvent, type QueryAST } from '@objectstack/spec/data'; import { PLURAL_TO_SINGULAR, SINGULAR_TO_PLURAL } from '@objectstack/spec/shared'; import { type FormView, isAggregatedViewContainer } from '@objectstack/spec/ui'; import { METADATA_FORM_REGISTRY } from '@objectstack/spec/system'; @@ -300,6 +300,59 @@ function resolveOverlaySchema(type: string, _item: unknown): z.ZodTypeAny | null return getMetadataTypeSchema(singular) ?? null; } +/** + * A 400 for a `$filter` 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 + * query parameter, so the error names the offending element and the vocabulary + * it was checked against — not a driver-internal builder state. + * + * Diagnoses the three shapes `isFilterAST` refuses, in the order they occur in + * practice. #4121. + */ +function malformedFilterError(filter: unknown[]): Error { + const detail = describeMalformedFilter(filter); + const err: any = new Error( + `Malformed $filter: ${detail} A filter array is a comparison ` + + `[field, operator, value], a logical node ["and"|"or", ...conditions], or a ` + + `list of those. Recognised operators: ${[...VALID_AST_OPERATORS].sort().join(', ')}.`, + ); + err.status = 400; + err.code = 'INVALID_FILTER'; + return err; +} + +/** The specific reason a filter array failed `isFilterAST`, for the message above. */ +function describeMalformedFilter(filter: unknown[]): string { + const [first, second] = filter; + const isKeyword = typeof first === 'string' && ['and', 'or'].includes(first.toLowerCase()); + + // `["and"]` / `["or"]` with nothing to join. The one shape that still + // returned every row silently after #3948: the driver sets its join mode, + // matches no element, and emits no predicate. + if (isKeyword && filter.length < 2) { + return `logical node ["${String(first)}"] has no conditions to join.`; + } + // A bare triple whose operator is outside the AST vocabulary — the original + // `before` / `after` / `'not in'` case. + if (typeof first === 'string' && !isKeyword && typeof second === 'string' + && !VALID_AST_OPERATORS.has(second.toLowerCase())) { + return `unrecognised operator "${second}" in [${JSON.stringify(first)}, ...].`; + } + // An element that is neither a join keyword nor a nested condition. + const badIndex = filter.findIndex( + (item) => !Array.isArray(item) + && !(typeof item === 'string' && ['and', 'or'].includes(item.toLowerCase())), + ); + if (badIndex >= 0 && filter.some((item) => Array.isArray(item))) { + const bad = filter[badIndex]; + return `element ${badIndex} is ${bad === null ? 'null' : typeof bad}, ` + + `expected a condition array or a logical keyword.`; + } + return `${JSON.stringify(filter)} is not a recognised filter shape.`; +} + /** * Guarantee a `view` body carries a top-level `name`. * @@ -3102,6 +3155,20 @@ export class ObjectStackProtocolImplementation implements // 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. + // + // 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); } options.where = parsedFilter; }