Skip to content

Commit 705ea6f

Browse files
fix(table): reject hybrid predicate nodes at the wire instead of silently narrowing
Found by running the HTTP suite: a node carrying BOTH a group key and a leaf's `field`/`op`/`value` returned 200, not 400. Zod strips unrecognized keys by default, so `{ all: [...], field, op, value }` parsed clean against the group branch with the leaf half quietly deleted. The hybrid guard added in eaf4179 could never fire — the keys were gone before `validatePredicate` ran. On the bulk paths that turns "delete archived rows for tenant acme" into "delete EVERY row for tenant acme". Both node shapes are now `strictObject`. Strict on the group alone would be worse than the bug: the union would fall through to the leaf branch, which is the more dangerous reading of the two. The bulk schemas are unaffected by design — their legacy `$`-object branch accepts any non-empty object, so it absorbs the hybrid WITHOUT stripping, the route's `isTablePredicate` check routes it back to `validatePredicate`, and the runtime guard rejects it there. Tests now pin both layers so removing either one fails loudly. Verified against a running server on the `hello` table: 18/18 HTTP checks pass, including the previously-failing hybrid case. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011M563cbFy2S74GvSDf2C3R
1 parent ead6391 commit 705ea6f

2 files changed

Lines changed: 87 additions & 3 deletions

File tree

apps/sim/lib/api/contracts/tables-predicate.test.ts

Lines changed: 74 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ import {
1212
rowQueryBodySchema,
1313
updateRowsByFilterBodySchema,
1414
} from '@/lib/api/contracts/tables'
15+
import { validatePredicate } from '@/lib/table/query-builder/validate'
1516

1617
describe('rowQueryBodySchema', () => {
1718
it('accepts a predicate/sort object, leaves limit unbounded, has no offset', () => {
@@ -149,3 +150,76 @@ describe('predicate depth / size guard', () => {
149150
).toBe(true)
150151
})
151152
})
153+
154+
/**
155+
* Zod strips unrecognized keys by default, so before the schemas were made
156+
* strict a hybrid node parsed clean against the group branch with its leaf half
157+
* silently deleted — turning "delete archived rows for tenant acme" into
158+
* "delete EVERY row for tenant acme". `validatePredicate`'s hybrid guard could
159+
* not catch it: the keys were gone before it ran.
160+
*/
161+
describe('hybrid group+leaf nodes are rejected, not silently narrowed', () => {
162+
const hybrid = {
163+
all: [{ field: 'tenant_id', op: 'eq', value: 'acme' }],
164+
field: 'status',
165+
op: 'eq',
166+
value: 'archived',
167+
}
168+
169+
it('rejects rather than dropping the leaf half', () => {
170+
const result = predicateSchema.safeParse(hybrid)
171+
expect(result.success).toBe(false)
172+
// The dangerous outcome: parsing succeeds having quietly widened the filter.
173+
expect(result.success ? result.data : null).not.toEqual({ all: hybrid.all })
174+
})
175+
176+
/**
177+
* The bulk schemas union the predicate tree with the legacy `$`-object, and
178+
* that legacy branch accepts any non-empty object — so it absorbs the hybrid
179+
* and the SCHEMA cannot reject it. Crucially the legacy branch does NOT strip,
180+
* so `all` survives, the route's `isTablePredicate` check routes it back to
181+
* `validatePredicate`, and the hybrid guard there rejects it (→ 400 via
182+
* `route.ts:325`). Asserted here so a future change to either layer that
183+
* removes one of them fails loudly.
184+
*/
185+
it('keeps the hybrid intact through the bulk schemas so the runtime guard can see it', () => {
186+
for (const parsed of [
187+
deleteTableRowsBodySchema.safeParse({ workspaceId: 'ws-1', filter: hybrid }),
188+
updateRowsByFilterBodySchema.safeParse({
189+
workspaceId: 'ws-1',
190+
filter: hybrid,
191+
data: { active: false },
192+
}),
193+
]) {
194+
expect(parsed.success).toBe(true)
195+
// The leaf half must NOT have been silently dropped on the way through.
196+
expect(parsed.success && parsed.data.filter).toMatchObject({
197+
all: hybrid.all,
198+
field: 'status',
199+
})
200+
}
201+
})
202+
203+
it('and validatePredicate then rejects it', () => {
204+
expect(() =>
205+
validatePredicate(hybrid as never, [
206+
{ name: 'tenant_id', type: 'string' },
207+
{ name: 'status', type: 'string' },
208+
])
209+
).toThrow(/not both/)
210+
})
211+
212+
it('rejects an unknown key on a leaf (a typo must not be dropped)', () => {
213+
expect(
214+
predicateSchema.safeParse({ all: [{ field: 'a', op: 'eq', vlaue: 'typo' }] }).success
215+
).toBe(false)
216+
})
217+
218+
it('still accepts well-formed nodes', () => {
219+
expect(
220+
predicateSchema.safeParse({
221+
all: [{ field: 'a', op: 'eq', value: 1 }, { any: [{ field: 'b', op: 'isNull' }] }],
222+
}).success
223+
).toBe(true)
224+
})
225+
})

apps/sim/lib/api/contracts/tables.ts

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -319,7 +319,17 @@ function predicateTreeTooLarge(root: unknown): string | null {
319319
* (unknown column, json-op rejection) is enforced server-side by
320320
* `validatePredicate` once the table's columns are known.
321321
*/
322-
const predicateLeafSchema = z.object({
322+
/**
323+
* Both node shapes are `strictObject`, and that is load-bearing rather than
324+
* fussiness. Zod strips unrecognized keys by default, so a hybrid node carrying
325+
* BOTH a group key and a leaf's `field`/`op`/`value` parsed clean against the
326+
* group branch with the leaf half silently deleted. On the bulk paths that turns
327+
* "delete archived rows for tenant acme" into "delete every row for tenant acme".
328+
* `validatePredicate`'s hybrid guard could never catch it — the keys were gone
329+
* before it ran. Strict on BOTH branches is required: strict on the group alone
330+
* would just fall through to the leaf branch, which is the more dangerous reading.
331+
*/
332+
const predicateLeafSchema = z.strictObject({
323333
field: z.string().min(1, 'field is required').max(128),
324334
op: z.enum(FILTER_OPS),
325335
value: z.unknown().optional(),
@@ -333,13 +343,13 @@ const predicateTreeSchema: z.ZodType<TablePredicate> = z.lazy(() =>
333343
z.union([
334344
// `.min(1)`: an empty group compiles to no WHERE clause, which on the bulk
335345
// delete/update paths reads as "match everything" rather than "match nothing".
336-
z.object({
346+
z.strictObject({
337347
all: z
338348
.array(predicateNodeSchema)
339349
.min(1, 'A filter group must contain at least one condition')
340350
.max(MAX_PREDICATE_GROUP_SIZE),
341351
}),
342-
z.object({
352+
z.strictObject({
343353
any: z
344354
.array(predicateNodeSchema)
345355
.min(1, 'A filter group must contain at least one condition')

0 commit comments

Comments
 (0)