Skip to content

Commit c66ecef

Browse files
fix(table): storage-validate async-route filters, reject dual-group nodes, resolve coerced select names
Bugbot round 3 on PR #6067, all three verified: Async routes (Medium). delete-async / cancel-runs / columns/run validated only the toLegacyFilter downgrade, which compiles a typo'd field into a clause that silently matches nothing — a filtered delete/run/stop no-ops where the sync bulk routes 400. tableFilterError is now grammar-aware and takes the WIRE filter: predicates go through validateStoragePredicate (same keying the sync routes enforce), legacy filters keep the buildFilterClause check. Dual all/any node (High). {all:[...], any:[...]} fails both strictObject branches, survives the legacy union, and every group-first traversal reads `all` and silently DROPS `any` — half the conditions vanish, widening a bulk delete/update. validateNode (covering every boundary and the copilot tool's validatePredicate) and predicateToFilter (lossless-or-throw) both reject it; nesting expresses the same intent unambiguously. Coerced select names (Medium). The block builder serializes without schema access, so an option NAME that looks numeric/boolean ("123") arrives scalar-coerced and resolveSelectOptionId bailed on non-strings — the filter compared 123 against the stored option id and matched nothing. Stringify scalar operands before matching, fixing every name-keyed caller (v1 and v2 blocks) at the resolution seam instead of per-surface. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011M563cbFy2S74GvSDf2C3R
1 parent 3ebb639 commit c66ecef

11 files changed

Lines changed: 179 additions & 15 deletions

File tree

apps/sim/app/api/table/[tableId]/cancel-runs/route.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,7 @@ export const POST = withRouteHandler(async (request: NextRequest, { params }: Ro
4747
return NextResponse.json({ error: 'Invalid workspace ID' }, { status: 400 })
4848
}
4949

50-
const filterError = tableFilterError(filter, table.schema.columns)
50+
const filterError = tableFilterError(wireFilter, table.schema.columns)
5151
if (filterError) return filterError
5252

5353
const cancelled = await cancelWorkflowGroupRuns(tableId, scope === 'row' ? rowId : undefined, {

apps/sim/app/api/table/[tableId]/columns/run/route.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,7 @@ export const POST = withRouteHandler(async (request: NextRequest, { params }: Ro
4343
if (!access.ok) return accessError(access, requestId, tableId)
4444

4545
// Validate the filter up front (the dispatcher reuses it) so a bad field fails fast.
46-
const filterError = tableFilterError(filter, access.table.schema.columns)
46+
const filterError = tableFilterError(wireFilter, access.table.schema.columns)
4747
if (filterError) return filterError
4848

4949
const { dispatchId } = await runWorkflowColumn({

apps/sim/app/api/table/[tableId]/delete-async/route.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -78,7 +78,7 @@ export const POST = withRouteHandler(async (request: NextRequest, { params }: Ro
7878
assertRowDelete(table)
7979

8080
// Validate the filter up front so the caller gets immediate feedback (the worker reuses it).
81-
const filterError = tableFilterError(filter, table.schema.columns)
81+
const filterError = tableFilterError(wireFilter, table.schema.columns)
8282
if (filterError) return filterError
8383

8484
// Rows inserted after this instant are spared (created_at <= cutoff in the worker).

apps/sim/app/api/table/utils.test.ts

Lines changed: 49 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,8 @@
33
*/
44
import { describe, expect, it } from 'vitest'
55
import { TableRowLimitError } from '@/lib/table/billing'
6-
import { rootErrorMessage, rowWriteErrorResponse } from '@/app/api/table/utils'
6+
import type { ColumnDefinition } from '@/lib/table/types'
7+
import { rootErrorMessage, rowWriteErrorResponse, tableFilterError } from '@/app/api/table/utils'
78

89
/** Mimics drizzle's DrizzleQueryError: message is the failed SQL, real error on `cause`. */
910
function wrapLikeDrizzle(cause: Error): Error {
@@ -53,3 +54,50 @@ describe('rowWriteErrorResponse', () => {
5354
expect(rowWriteErrorResponse(wrapLikeDrizzle(new Error('deadlock detected')))).toBeNull()
5455
})
5556
})
57+
58+
/**
59+
* The async destructive routes (delete-async, cancel-runs, columns/run)
60+
* validate the WIRE filter here. The predicate branch must reject unknown
61+
* storage keys the way the sync bulk routes do — the `toLegacyFilter`
62+
* downgrade compiles a typo'd field into a clause that silently matches
63+
* nothing, turning a scoped delete/run into a no-op.
64+
*/
65+
describe('tableFilterError', () => {
66+
const columns: ColumnDefinition[] = [{ id: 'col_status', name: 'status', type: 'string' }]
67+
68+
it('returns null for an absent filter and a valid id-keyed predicate', () => {
69+
expect(tableFilterError(undefined, columns)).toBeNull()
70+
expect(
71+
tableFilterError({ all: [{ field: 'col_status', op: 'eq', value: 'x' }] }, columns)
72+
).toBeNull()
73+
expect(tableFilterError({ all: [{ field: 'createdAt', op: 'isNotNull' }] }, columns)).toBeNull()
74+
})
75+
76+
it('400s a predicate naming an unknown storage key', async () => {
77+
const response = tableFilterError(
78+
{ all: [{ field: 'statuss', op: 'eq', value: 'x' }] },
79+
columns
80+
)
81+
expect(response?.status).toBe(400)
82+
const body = await response?.json()
83+
expect(body.error).toMatch(/Unknown filter column "statuss"/)
84+
})
85+
86+
it('400s a structurally invalid predicate (empty group, dual group keys)', () => {
87+
expect(tableFilterError({ all: [] } as never, columns)?.status).toBe(400)
88+
expect(
89+
tableFilterError(
90+
{
91+
all: [{ field: 'col_status', op: 'eq', value: 'a' }],
92+
any: [{ field: 'col_status', op: 'eq', value: 'b' }],
93+
} as never,
94+
columns
95+
)?.status
96+
).toBe(400)
97+
})
98+
99+
it('still validates the legacy grammar through buildFilterClause', () => {
100+
expect(tableFilterError({ col_status: 'x' }, columns)).toBeNull()
101+
expect(tableFilterError({ col_status: { $regex: 'x' } } as never, columns)?.status).toBe(400)
102+
})
103+
})

apps/sim/app/api/table/utils.ts

Lines changed: 21 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -9,10 +9,12 @@ import {
99
} from '@/lib/api/contracts/tables'
1010
import { isFeatureEnabled } from '@/lib/core/config/feature-flags'
1111
import type { MultipartError } from '@/lib/core/utils/multipart'
12-
import type { ColumnDefinition, Filter, TableDefinition } from '@/lib/table'
12+
import type { ColumnDefinition, Filter, TableDefinition, TablePredicate } from '@/lib/table'
1313
import { buildFilterClause, getTableById, TableQueryValidationError } from '@/lib/table'
1414
import { USER_TABLE_ROWS_SQL_NAME } from '@/lib/table/constants'
1515
import { TableLockedError } from '@/lib/table/mutation-locks'
16+
import { isTablePredicate } from '@/lib/table/query-builder/converters'
17+
import { validateStoragePredicate } from '@/lib/table/query-builder/validate'
1618
import { getUserEntityPermissions } from '@/lib/workspaces/permissions/utils'
1719
import { getWorkspaceOrganizationId } from '@/lib/workspaces/utils'
1820

@@ -54,17 +56,30 @@ export function tableLockErrorResponse(error: unknown): NextResponse | null {
5456
}
5557

5658
/**
57-
* Validates a `filter` against the table's column schema, returning a 400 response on a bad field
58-
* (or `null` when the filter is valid or absent). Shared by the routes that accept a filter
59-
* (`delete-async`, `columns/run`) so a bad field fails fast with a clear message.
59+
* Validates a wire `filter` (either grammar) against the table's column schema,
60+
* returning a 400 response on a bad field (or `null` when the filter is valid or
61+
* absent). Shared by the routes that accept a filter (`delete-async`,
62+
* `cancel-runs`, `columns/run`) so a bad field fails fast with a clear message.
63+
*
64+
* Pass the WIRE filter, not the `toLegacyFilter` downgrade: the downgrade
65+
* compiles cleanly through `buildFilterClause` even when a predicate leaf names
66+
* a column that doesn't exist, so validating only the downgraded form lets a
67+
* typo'd field become a clause that silently matches nothing — a no-op where
68+
* the sync bulk routes 400.
6069
*/
6170
export function tableFilterError(
62-
filter: Filter | undefined,
71+
filter: Filter | TablePredicate | undefined,
6372
columns: ColumnDefinition[]
6473
): NextResponse | null {
6574
if (!filter) return null
6675
try {
67-
buildFilterClause(filter, USER_TABLE_ROWS_SQL_NAME, columns)
76+
if (isTablePredicate(filter)) {
77+
// These routes speak storage keys (session grid uses column ids; system
78+
// columns keep their names) — same keying the sync bulk routes validate.
79+
validateStoragePredicate(filter, columns)
80+
} else {
81+
buildFilterClause(filter, USER_TABLE_ROWS_SQL_NAME, columns)
82+
}
6883
return null
6984
} catch (error) {
7085
if (error instanceof TableQueryValidationError) {

apps/sim/lib/table/query-builder/__tests__/converters.test.ts

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -468,6 +468,15 @@ describe('toLegacyFilter / predicateToFilter hybrid safety', () => {
468468
expect(() => predicateToFilter(hybrid)).toThrow(/not both/)
469469
})
470470

471+
it('throws on a node with both group keys instead of dropping the "any" half', () => {
472+
const dual = {
473+
all: [{ field: 'tenant_id', op: 'eq', value: 'acme' }],
474+
any: [{ field: 'status', op: 'eq', value: 'archived' }],
475+
} as never
476+
expect(() => predicateToFilter(dual)).toThrow(/either "all" or "any"/)
477+
expect(() => toLegacyFilter(dual)).toThrow(/either "all" or "any"/)
478+
})
479+
471480
it('toLegacyFilter shape-validates before converting', () => {
472481
expect(() => toLegacyFilter(hybrid)).toThrow(/not both/)
473482
// malformed group value is also caught, not crashed on

apps/sim/lib/table/query-builder/__tests__/validate.test.ts

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -143,6 +143,20 @@ describe('validatePredicate — leaves that would silently widen a bulk write',
143143
).toThrow(/not both/)
144144
})
145145

146+
it('rejects a node carrying both group keys instead of dropping the "any" half', () => {
147+
// Every group-first traversal reads `all` and drops `any` — half the
148+
// caller's conditions would vanish, widening a bulk delete/update.
149+
expect(() =>
150+
validatePredicate(
151+
{
152+
all: [{ field: 'status', op: 'eq', value: 'archived' }],
153+
any: [{ field: 'status', op: 'eq', value: 'stale' }],
154+
} as never,
155+
COLS
156+
)
157+
).toThrow(/either "all" or "any"/)
158+
})
159+
146160
it('caps in/nin list length', () => {
147161
const huge = Array.from({ length: 1001 }, (_, i) => `v${i}`)
148162
expect(() =>

apps/sim/lib/table/query-builder/converters.ts

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -475,6 +475,14 @@ export function predicateToFilter(predicate: TablePredicate): Filter {
475475
'INVALID_FILTER'
476476
)
477477
}
478+
// A node with BOTH group keys would convert `all` and silently drop `any`
479+
// — same widening failure as the hybrid above. Lossless-or-throw.
480+
if ('all' in node && 'any' in node) {
481+
throw new TableQueryValidationError(
482+
'A filter group must use either "all" or "any", not both — nest one group inside the other instead.',
483+
'INVALID_FILTER'
484+
)
485+
}
478486
// Group-first, matching isPredicateGroup/validateNode/buildPredicateNode. A
479487
// leaf-first test would execute a different predicate than the one validated.
480488
if ('all' in node) return { $and: node.all.map(nodeToFilter) }

apps/sim/lib/table/query-builder/validate.ts

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -137,6 +137,17 @@ function validateNode(node: PredicateNode, typeByName: Map<string, ColumnType> |
137137
'INVALID_FILTER'
138138
)
139139
}
140+
// Same ambiguity with BOTH group keys: every group-first traversal
141+
// (`predicateNamesToIds`, `predicateToFilter`, `buildPredicateNode`) reads
142+
// `all` and silently DROPS `any`, so half the caller's conditions vanish —
143+
// on a bulk delete/update that widens the matched set. Reject rather than
144+
// pick a winner; nesting expresses the intent unambiguously.
145+
if ('all' in node && 'any' in node) {
146+
throw new TableQueryValidationError(
147+
'A filter group must use either "all" or "any", not both — nest one group inside the other instead.',
148+
'INVALID_FILTER'
149+
)
150+
}
140151
if ('all' in node || 'any' in node) {
141152
const members = 'all' in node ? node.all : node.any
142153
if (!Array.isArray(members)) {

apps/sim/lib/table/select-values.test.ts

Lines changed: 51 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,11 @@
22
* @vitest-environment node
33
*/
44
import { describe, expect, it } from 'vitest'
5-
import { resolveFilterSelectValues, selectValueToNames } from '@/lib/table/select-values'
5+
import {
6+
resolveFilterSelectValues,
7+
resolvePredicateSelectValues,
8+
selectValueToNames,
9+
} from '@/lib/table/select-values'
610
import type { ColumnDefinition } from '@/lib/table/types'
711

812
const status: ColumnDefinition = {
@@ -100,3 +104,49 @@ describe('resolveFilterSelectValues', () => {
100104
).toEqual({ $or: [{ col_status: 'opt_open' }, { col_title: 'x' }] })
101105
})
102106
})
107+
108+
/**
109+
* The block builder serializes without schema access, so an option NAME that
110+
* looks numeric or boolean arrives scalar-coerced ("123" → 123). Resolution
111+
* must still find the option, or a correctly-authored builder filter compares
112+
* a number against the stored id string and matches nothing.
113+
*/
114+
describe('resolvePredicateSelectValues — scalar-coerced option names', () => {
115+
const numericStatus: ColumnDefinition = {
116+
id: 'col_code',
117+
name: 'code',
118+
type: 'select',
119+
options: [
120+
{ id: 'opt_123', name: '123' },
121+
{ id: 'opt_true', name: 'true' },
122+
],
123+
}
124+
const columns = [numericStatus]
125+
126+
it('resolves a coerced numeric name to its option id', () => {
127+
expect(
128+
resolvePredicateSelectValues({ all: [{ field: 'col_code', op: 'eq', value: 123 }] }, columns)
129+
).toEqual({ all: [{ field: 'col_code', op: 'eq', value: 'opt_123' }] })
130+
})
131+
132+
it('resolves a coerced boolean name, including inside in/contains', () => {
133+
expect(
134+
resolvePredicateSelectValues(
135+
{ any: [{ field: 'col_code', op: 'in', value: [true, 123] }] },
136+
columns
137+
)
138+
).toEqual({ any: [{ field: 'col_code', op: 'in', value: ['opt_true', 'opt_123'] }] })
139+
expect(
140+
resolvePredicateSelectValues(
141+
{ all: [{ field: 'col_code', op: 'contains', value: 123 }] },
142+
columns
143+
)
144+
).toEqual({ all: [{ field: 'col_code', op: 'contains', value: 'opt_123' }] })
145+
})
146+
147+
it('leaves a scalar with no matching option name as-is', () => {
148+
expect(
149+
resolvePredicateSelectValues({ all: [{ field: 'col_code', op: 'eq', value: 999 }] }, columns)
150+
).toEqual({ all: [{ field: 'col_code', op: 'eq', value: 999 }] })
151+
})
152+
})

0 commit comments

Comments
 (0)