Skip to content

Commit 3ebb639

Browse files
fix(table): reject empty filter groups and bind cursors to their sort
Bugbot round 2 on PR #6067, both verified: Empty groups (High). `{all: []}` fails the strict predicate branch (.min(1)) but slips the dual union via the legacy branch — an empty ARRAY inside a non-empty OBJECT — then downgraded to `{$and: []}`, which compiles to no WHERE clause: a run/cancel/delete scope silently widened to every row. validatePredicateShape now mirrors the contract's .min(1), which closes it at every dual-grammar boundary at once (toLegacyFilter, resolveBulkFilter, the GET native path). Cursor↔sort binding (Medium). CURSOR_SORT_CONFLICT only fired for keyset cursors; offset cursors — the shape sorted views actually emit — carried no record of their ordering, so one minted under sort A replayed under sort B (or none) silently paged the wrong sequence. Offset cursors are now stamped with a canonical fingerprint of their sort at mint (queryRows), and a shared assertCursorSortBinding enforces the match at all three consumers (both query routes and the copilot executor), replacing the three hand-rolled keyset-only checks. Keyset/compound cursors stay default-order-only by construction. The OpenAPI cursor wording now states the binding rather than overclaiming. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011M563cbFy2S74GvSDf2C3R
1 parent 9d8fa6c commit 3ebb639

11 files changed

Lines changed: 209 additions & 42 deletions

File tree

apps/docs/openapi-v2-tables.json

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -60,7 +60,7 @@
6060
"post": {
6161
"operationId": "v2QueryTableRows",
6262
"summary": "Query Rows",
63-
"description": "Query rows with a typed predicate filter, an ordered sort spec, and opaque cursor pagination. Row `data` is keyed by column NAME; `select` cells return option names, and filter operands on select columns accept option names (resolved case-insensitively).\n\n**Pagination contract:** page by passing the previous response's `nextCursor` back as `cursor`, and stop only when it is `null` — a page may return fewer than `limit` rows and still have more behind it, so page fullness is never a termination signal. A cursor encodes a position in the default row order and cannot be combined with `sort` (400 `CURSOR_SORT_CONFLICT`). `totalCount` is computed on the first page only (requests with a `cursor` return `totalCount: null`).",
63+
"description": "Query rows with a typed predicate filter, an ordered sort spec, and opaque cursor pagination. Row `data` is keyed by column NAME; `select` cells return option names, and filter operands on select columns accept option names (resolved case-insensitively).\n\n**Pagination contract:** page by passing the previous response's `nextCursor` back as `cursor`, and stop only when it is `null` — a page may return fewer than `limit` rows and still have more behind it, so page fullness is never a termination signal. A cursor is bound to the exact query shape it was minted under: keyset cursors to the default row order, offset cursors (sorted views) to that sort. Replaying one under a different `sort` returns 400 `CURSOR_SORT_CONFLICT`. `totalCount` is computed on the first page only (requests with a `cursor` return `totalCount: null`).",
6464
"tags": ["Tables v2"],
6565
"parameters": [
6666
{

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

Lines changed: 5 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@ import type { Sort, TableSchema } from '@/lib/table'
1010
import { buildIdByName, sortSpecNamesToIds } from '@/lib/table/column-keys'
1111
import { TableQueryValidationError } from '@/lib/table/errors'
1212
import { validatePredicate, validateSortSpec } from '@/lib/table/query-builder/validate'
13-
import { decodeCursor } from '@/lib/table/rows/cursor'
13+
import { assertCursorSortBinding, decodeCursor } from '@/lib/table/rows/cursor'
1414
import { queryRows } from '@/lib/table/rows/service'
1515
import { predicateToStorage } from '@/lib/table/select-values'
1616
import { rowWireTranslators } from '@/app/api/table/row-wire'
@@ -63,19 +63,6 @@ export const POST = withRouteHandler(async (request: NextRequest, context: RowQu
6363
const wire = rowWireTranslators(authResult.authType, schema)
6464
const cursor = body.cursor ? decodeCursor(body.cursor) : undefined
6565

66-
// A keyset cursor encodes a position on the DEFAULT `(order_key, id)`
67-
// order; combining it with a custom order would silently return page 1
68-
// of the sorted view relabeled as a next page.
69-
if (cursor?.after && body.sort?.length) {
70-
return NextResponse.json(
71-
{
72-
error: 'Cursor is not valid for a sorted query. Restart paging without the cursor.',
73-
code: 'CURSOR_SORT_CONFLICT',
74-
},
75-
{ status: 400 }
76-
)
77-
}
78-
7966
// Predicate/sort fields are column-NAME-keyed by construction (the caller
8067
// authors names), so validate against the schema then translate names →
8168
// storage ids unconditionally — unlike row data, this is not authType-dependent.
@@ -94,6 +81,10 @@ export const POST = withRouteHandler(async (request: NextRequest, context: RowQu
9481
? Object.fromEntries(sortSpec.map((s) => [s.field, s.direction]))
9582
: undefined
9683

84+
// Cursor↔sort binding: keyset cursors are default-order only; an offset
85+
// cursor must be replayed under the exact sort it was minted with.
86+
if (cursor) assertCursorSortBinding(cursor, sort)
87+
9788
const result = await queryRows(
9889
table,
9990
{

apps/sim/app/api/v2/tables/[tableId]/query/route.ts

Lines changed: 6 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@ import { buildIdByName, sortSpecNamesToIds } from '@/lib/table'
1010
import { namedRowMapper } from '@/lib/table/cell-format'
1111
import { TableQueryValidationError } from '@/lib/table/errors'
1212
import { validatePredicate, validateSortSpec } from '@/lib/table/query-builder/validate'
13-
import { decodeCursor } from '@/lib/table/rows/cursor'
13+
import { assertCursorSortBinding, decodeCursor } from '@/lib/table/rows/cursor'
1414
import { queryRows } from '@/lib/table/rows/service'
1515
import { predicateToStorage } from '@/lib/table/select-values'
1616
import { accessError, checkAccess, tablesV2GateError } from '@/app/api/table/utils'
@@ -72,18 +72,6 @@ export const POST = withRouteHandler(async (request: NextRequest, context: Query
7272
const schema = table.schema as TableSchema
7373
const cursor = cursorToken ? decodeCursor(cursorToken) : undefined
7474

75-
// A keyset cursor is bound to the DEFAULT `(order_key, id)` order; combining
76-
// it with a custom sort would relabel page 1 of the sorted view as a next page.
77-
if (cursor?.after && sort?.length) {
78-
return NextResponse.json(
79-
{
80-
error: 'Cursor is not valid for a sorted query. Restart paging without the cursor.',
81-
code: 'CURSOR_SORT_CONFLICT',
82-
},
83-
{ status: 400, headers: PRIVATE_NO_STORE }
84-
)
85-
}
86-
8775
const idByName = buildIdByName(schema)
8876
// Fuses the id→name key remap with select-cell value formatting, so a select
8977
// cell surfaces its option NAME rather than the stored option id.
@@ -102,6 +90,11 @@ export const POST = withRouteHandler(async (request: NextRequest, context: Query
10290
? Object.fromEntries(sortSpec.map((s) => [s.field, s.direction]))
10391
: undefined
10492

93+
// A cursor is only valid for the query shape it was minted under: keyset
94+
// cursors bind to the default order, offset cursors to their sort. Runs on
95+
// the STORAGE-keyed sort so the fingerprint matches what queryRows stamped.
96+
if (cursor) assertCursorSortBinding(cursor, sortObj)
97+
10598
// Public default is a bounded page (unlike the internal surface's unbounded
10699
// omit). `limit=0` is the explicit unbounded opt-in.
107100
const effectiveLimit =

apps/sim/lib/copilot/tools/server/table/user-table.ts

Lines changed: 9 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import { AuditAction, AuditResourceType, recordAudit } from '@sim/audit'
22
import { createLogger } from '@sim/logger'
3-
import { toError } from '@sim/utils/errors'
3+
import { getErrorMessage, toError } from '@sim/utils/errors'
44
import { generateId, generateShortId } from '@sim/utils/id'
55
import { UserTable } from '@/lib/copilot/generated/tool-catalog-v1'
66
import {
@@ -49,7 +49,7 @@ import { markTableJobRunning, releaseJobClaim } from '@/lib/table/jobs/service'
4949
import { assertRowDelete, assertRowUpdate, patchColumnIds } from '@/lib/table/mutation-locks'
5050
import { predicateToFilter } from '@/lib/table/query-builder/converters'
5151
import { validatePredicate, validateSortSpec } from '@/lib/table/query-builder/validate'
52-
import { decodeCursor } from '@/lib/table/rows/cursor'
52+
import { assertCursorSortBinding, decodeCursor } from '@/lib/table/rows/cursor'
5353
import {
5454
batchInsertRows,
5555
batchUpdateRows,
@@ -700,11 +700,13 @@ export const userTableServerTool: BaseServerTool<UserTableArgs, UserTableResult>
700700
// physically can't page). A keyset cursor is bound to the default order,
701701
// so it can't be combined with a fresh sort.
702702
const cursor = args.cursor ? decodeCursor(args.cursor) : undefined
703-
if (cursor?.after && sort) {
704-
return {
705-
success: false,
706-
message:
707-
'Cursor is not valid for a sorted query. Restart paging without the cursor (omit it on the first sorted page).',
703+
if (cursor) {
704+
try {
705+
// Keyset cursors bind to the default order; offset cursors to the
706+
// exact sort they were minted under.
707+
assertCursorSortBinding(cursor, sort)
708+
} catch (bindError) {
709+
return { success: false, message: getErrorMessage(bindError, 'Invalid cursor') }
708710
}
709711
}
710712

apps/sim/lib/table/__tests__/service-filter-threading.test.ts

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -403,7 +403,12 @@ describe('queryRows byte budget', () => {
403403

404404
expect(dbChainMockFns.offset).toHaveBeenCalledWith(40)
405405
expect(result.rows).toHaveLength(1)
406-
expect(decodeCursor(result.nextCursor as string)).toEqual({ offset: 41 })
406+
// The offset cursor is stamped with the sort it was minted under, so a
407+
// replay against a different ordering is rejected instead of paging wrong.
408+
expect(decodeCursor(result.nextCursor as string)).toEqual({
409+
offset: 41,
410+
sortKey: JSON.stringify([['name', 'asc']]),
411+
})
407412
})
408413

409414
it('emits a compound cursor when rows past the inbound anchor are unkeyed', async () => {

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

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -497,3 +497,7 @@ describe('isTablePredicate vs columns literally named all/any', () => {
497497
expect(isTablePredicate({ any: [{ field: 'a', op: 'eq', value: 1 }] })).toBe(true)
498498
})
499499
})
500+
501+
it('toLegacyFilter rejects an empty group instead of downgrading it to no WHERE', () => {
502+
expect(() => toLegacyFilter({ all: [] })).toThrow(/at least one condition/)
503+
})

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

Lines changed: 25 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,11 @@
33
*/
44
import { describe, expect, it } from 'vitest'
55
import { TableQueryValidationError } from '@/lib/table/errors'
6-
import { validatePredicate, validateSortSpec } from '@/lib/table/query-builder/validate'
6+
import {
7+
validatePredicate,
8+
validatePredicateShape,
9+
validateSortSpec,
10+
} from '@/lib/table/query-builder/validate'
711
import type { ColumnDefinition } from '@/lib/table/types'
812

913
const COLS: ColumnDefinition[] = [
@@ -175,3 +179,23 @@ describe('validatePredicate — legacy $-grammar diagnostics', () => {
175179
)
176180
})
177181
})
182+
183+
/**
184+
* Bugbot round 2: `{all: []}` passes the dual union via the non-empty-OBJECT
185+
* legacy branch, then compiled to no WHERE clause — silently widening a run /
186+
* cancel / delete scope to every row. The shape validator now mirrors the Zod
187+
* contract's .min(1).
188+
*/
189+
describe('empty groups are rejected at every layer', () => {
190+
it('validatePredicateShape rejects an empty group', () => {
191+
expect(() => validatePredicateShape({ all: [] })).toThrow(/at least one condition/)
192+
expect(() => validatePredicateShape({ any: [] })).toThrow(/at least one condition/)
193+
expect(() => validatePredicateShape({ all: [{ any: [] }] } as never)).toThrow(
194+
/at least one condition/
195+
)
196+
})
197+
198+
it('validatePredicate rejects it too', () => {
199+
expect(() => validatePredicate({ all: [] }, COLS)).toThrow(/at least one condition/)
200+
})
201+
})

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

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -145,6 +145,15 @@ function validateNode(node: PredicateNode, typeByName: Map<string, ColumnType> |
145145
'INVALID_FILTER'
146146
)
147147
}
148+
// Mirrors the Zod contract's .min(1). An empty group slips the strict union
149+
// (falling to the non-empty-OBJECT legacy branch) and compiles to no WHERE
150+
// clause — which on the run/cancel/delete scopes silently means EVERY row.
151+
if (members.length === 0) {
152+
throw new TableQueryValidationError(
153+
'A filter group must contain at least one condition.',
154+
'INVALID_FILTER'
155+
)
156+
}
148157
for (const child of members) validateNode(child, typeByName)
149158
return
150159
}
Lines changed: 78 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,78 @@
1+
/**
2+
* @vitest-environment node
3+
*
4+
* Opaque cursor encode/decode and the cursor↔sort binding. A cursor encodes a
5+
* position in one specific ordering; replaying it under any other ordering
6+
* silently pages the wrong sequence, so binding violations must throw
7+
* CURSOR_SORT_CONFLICT rather than return wrong rows.
8+
*/
9+
import { describe, expect, it } from 'vitest'
10+
import { TableQueryValidationError } from '@/lib/table/errors'
11+
import {
12+
assertCursorSortBinding,
13+
canonicalSortKey,
14+
decodeCursor,
15+
encodeCursor,
16+
} from '@/lib/table/rows/cursor'
17+
18+
const ROW = { id: 'row_1', orderKey: 'a1' }
19+
20+
describe('cursor↔sort binding (bugbot round 2)', () => {
21+
it('stamps an offset cursor with the sort it was minted under', () => {
22+
const token = encodeCursor({
23+
lastRow: { id: 'row_1', orderKey: null },
24+
keysetValid: false,
25+
nextOffset: 100,
26+
sort: { col_a: 'desc' },
27+
})
28+
const decoded = decodeCursor(token)
29+
expect(decoded.offset).toBe(100)
30+
expect(decoded.sortKey).toBe(canonicalSortKey({ col_a: 'desc' }))
31+
})
32+
33+
it('accepts replay under the identical sort', () => {
34+
const decoded = { offset: 100, sortKey: canonicalSortKey({ col_a: 'desc' }) }
35+
expect(() => assertCursorSortBinding(decoded, { col_a: 'desc' })).not.toThrow()
36+
})
37+
38+
it('rejects replay under a DIFFERENT sort', () => {
39+
const decoded = { offset: 100, sortKey: canonicalSortKey({ col_a: 'desc' }) }
40+
for (const sort of [{ col_a: 'asc' as const }, { col_b: 'desc' as const }, undefined]) {
41+
expect(() => assertCursorSortBinding(decoded, sort)).toThrow(TableQueryValidationError)
42+
try {
43+
assertCursorSortBinding(decoded, sort)
44+
} catch (e) {
45+
expect((e as TableQueryValidationError).code).toBe('CURSOR_SORT_CONFLICT')
46+
}
47+
}
48+
})
49+
50+
it('rejects adding a sort to an unsorted offset cursor', () => {
51+
const token = encodeCursor({
52+
lastRow: { id: 'row_1', orderKey: null },
53+
keysetValid: false,
54+
nextOffset: 50,
55+
})
56+
const decoded = decodeCursor(token)
57+
expect(decoded.sortKey).toBeUndefined()
58+
expect(() => assertCursorSortBinding(decoded, { col_a: 'asc' })).toThrow(
59+
/different sort|sorted query/
60+
)
61+
expect(() => assertCursorSortBinding(decoded, undefined)).not.toThrow()
62+
})
63+
64+
it('keyset cursors stay default-order only and never carry a sort stamp', () => {
65+
const token = encodeCursor({ lastRow: ROW, keysetValid: true, nextOffset: 10 })
66+
const decoded = decodeCursor(token)
67+
expect(decoded.after).toEqual({ orderKey: 'a1', id: 'row_1' })
68+
expect(decoded.sortKey).toBeUndefined()
69+
expect(() => assertCursorSortBinding(decoded, { col_a: 'asc' })).toThrow(/sorted query/)
70+
expect(() => assertCursorSortBinding(decoded, undefined)).not.toThrow()
71+
})
72+
73+
it('sort key order is significant (priority is part of the identity)', () => {
74+
expect(canonicalSortKey({ a: 'asc', b: 'desc' })).not.toBe(
75+
canonicalSortKey({ b: 'desc', a: 'asc' })
76+
)
77+
})
78+
})

apps/sim/lib/table/rows/cursor.ts

Lines changed: 65 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@
1616
*/
1717

1818
import { TableQueryValidationError } from '@/lib/table/errors'
19-
import type { TableRow, TableRowsCursor } from '@/lib/table/types'
19+
import type { Sort, TableRow, TableRowsCursor } from '@/lib/table/types'
2020

2121
/**
2222
* Cursor payload version. Every encoded token carries `v`; decode rejects any
@@ -26,7 +26,50 @@ import type { TableRow, TableRowsCursor } from '@/lib/table/types'
2626
const CURSOR_VERSION = 1
2727

2828
type CursorBody = { k: string; i: string } | { o: number } | { k: string; i: string; o: number }
29-
type CursorPayload = CursorBody & { v: number }
29+
type SortBinding = { s?: string }
30+
type CursorPayload = CursorBody & SortBinding & { v: number }
31+
32+
/**
33+
* Canonical fingerprint of a sort for cursor binding. Entry order is the sort
34+
* priority (built from the ordered spec upstream), so stringifying entries is
35+
* deterministic for equal sorts and distinct for different ones.
36+
*/
37+
export function canonicalSortKey(sort: Sort | null | undefined): string | undefined {
38+
if (!sort) return undefined
39+
const entries = Object.entries(sort)
40+
return entries.length > 0 ? JSON.stringify(entries) : undefined
41+
}
42+
43+
/**
44+
* A cursor is only valid for the exact query shape it was minted under:
45+
* keyset/compound cursors encode a position in the DEFAULT `(order_key, id)`
46+
* order, and an offset cursor from a sorted view encodes a position in THAT
47+
* sort. Replaying either against a different ordering silently pages the wrong
48+
* sequence — rows skipped or duplicated with no error. Throws
49+
* `CURSOR_SORT_CONFLICT` so callers restart paging without the cursor.
50+
*/
51+
export function assertCursorSortBinding(
52+
decoded: { after?: TableRowsCursor; offset?: number; sortKey?: string },
53+
sort: Sort | null | undefined
54+
): void {
55+
const requested = canonicalSortKey(sort)
56+
if (decoded.after && requested !== undefined) {
57+
throw new TableQueryValidationError(
58+
'Cursor is not valid for a sorted query. Restart paging without the cursor.',
59+
'CURSOR_SORT_CONFLICT'
60+
)
61+
}
62+
if (
63+
decoded.after === undefined &&
64+
decoded.offset !== undefined &&
65+
decoded.sortKey !== requested
66+
) {
67+
throw new TableQueryValidationError(
68+
'Cursor was created under a different sort. Restart paging without the cursor.',
69+
'CURSOR_SORT_CONFLICT'
70+
)
71+
}
72+
}
3073

3174
function invalidCursor(): never {
3275
throw new TableQueryValidationError('Invalid cursor', 'INVALID_CURSOR')
@@ -57,6 +100,8 @@ export function encodeCursor(args: {
57100
keysetValid: boolean
58101
nextOffset: number
59102
seekBase?: { anchor: TableRowsCursor; offsetFromAnchor: number }
103+
/** The sort the page was produced under — stamps offset cursors so they can't be replayed against a different ordering. */
104+
sort?: Sort | null
60105
}): string {
61106
let body: CursorBody
62107
if (args.keysetValid && args.lastRow.orderKey) {
@@ -74,12 +119,24 @@ export function encodeCursor(args: {
74119
} else {
75120
body = { o: args.nextOffset }
76121
}
77-
const payload: CursorPayload = { ...body, v: CURSOR_VERSION }
122+
const sortKey = canonicalSortKey(args.sort)
123+
const payload: CursorPayload = {
124+
...body,
125+
// Only the pure-offset shape can exist under a custom sort; keyset and
126+
// compound shapes are default-order by construction and carry no binding.
127+
...('k' in body || sortKey === undefined ? {} : { s: sortKey }),
128+
v: CURSOR_VERSION,
129+
}
78130
return toBase64Url(JSON.stringify(payload))
79131
}
80132

81133
/** Decodes an opaque cursor into the `queryRows` paging inputs it stands for. */
82-
export function decodeCursor(token: string): { after?: TableRowsCursor; offset?: number } {
134+
export function decodeCursor(token: string): {
135+
after?: TableRowsCursor
136+
offset?: number
137+
/** Sort fingerprint an offset cursor was minted under; absent = default order. */
138+
sortKey?: string
139+
} {
83140
let payload: unknown
84141
try {
85142
payload = JSON.parse(fromBase64Url(token))
@@ -105,7 +162,10 @@ export function decodeCursor(token: string): { after?: TableRowsCursor; offset?:
105162
return { after: { orderKey: record.k as string, id: record.i as string } }
106163
}
107164
if (hasOffset) {
108-
return { offset: record.o as number }
165+
return {
166+
offset: record.o as number,
167+
...(typeof record.s === 'string' ? { sortKey: record.s } : {}),
168+
}
109169
}
110170
invalidCursor()
111171
}

0 commit comments

Comments
 (0)