Skip to content

Commit 612fb47

Browse files
committed
fix(mothership): refuse Chat table predicates over unsafe cells
A table read whose rows, counts, cursor or affected count depend on cell values is refused for Chat, before the predicate runs, when the table has an unvouched row or a secret in a referenced column. Inline run-file bytes are collected per storage key, and table-selection fallbacks are projected.
1 parent 2632dea commit 612fb47

7 files changed

Lines changed: 333 additions & 101 deletions

File tree

‎apps/sim/lib/mothership/__integration__/content-delivery.integration.ts‎

Lines changed: 62 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -51,8 +51,9 @@ vi.mock('@/lib/mothership/async-runs/repository', async (importOriginal) => ({
5151
}))
5252

5353
import { db } from '@sim/db'
54-
import { copilotChats } from '@sim/db/schema'
54+
import { copilotChats, userTableRowSecretProvenance } from '@sim/db/schema'
5555
import { sha256Hex } from '@sim/security/hash'
56+
import { eq, sql } from 'drizzle-orm'
5657
import contentRoutes from '@/lib/api/server/routes/copilot-content-routes.json'
5758
import { matchV2Route } from '@/lib/api/server/routes/in-process-transport'
5859
import { withWorkspaceInvocationScope } from '@/lib/core/application/workspace-invocation-scope'
@@ -329,16 +330,68 @@ describe('content reaching Chat is admitted per item', () => {
329330
expectPerItem(read, { secretAdmitted: true })
330331
})
331332

332-
it('drops row-search matches that would probe unadmitted cells (#18b)', async () => {
333-
const read = await cli('POST', `/api/v2/tables/${fixture.table.tableId}/rows/search`, {
333+
it('refuses predicates over a table whose cells it cannot vouch for (#18b)', async () => {
334+
const table = `/api/v2/tables/${fixture.table.tableId}`
335+
const guess = { field: 'note', op: 'contains', value: 'second' }
336+
const refused = [
337+
await cli('POST', `${table}/rows/search`, { workspaceId: ws(), q: 'second' }),
338+
await cli('POST', `${table}/query`, { workspaceId: ws(), predicate: guess, limit: 1 }),
339+
await cli('POST', `${table}/query/count`, { workspaceId: ws(), predicate: guess }),
340+
]
341+
for (const read of refused) {
342+
expect(read.status).toBe(403)
343+
expect(read.body).not.toContain(CANARY.unknown)
344+
expect(read.body).not.toContain(CANARY.secret)
345+
}
346+
/** The same predicate reads nothing but row ids, so it stays available. */
347+
const byId = await cli('POST', `${table}/query`, {
334348
workspaceId: ws(),
335-
q: 'second',
349+
predicate: { field: 'id', op: 'in', value: [fixture.table.rows.clean] },
336350
})
337-
expect(read.status).toBe(200)
338-
const rows = new Set(
339-
JSON.parse(read.body).data.matches.map((match: { rowId: string }) => match.rowId)
340-
)
341-
expect(rows).toEqual(new Set([fixture.table.rows.clean]))
351+
expect(byId.status).toBe(200)
352+
expect(modelView(byId)).toContain(CANARY.clean)
353+
})
354+
355+
it('runs predicates over a table whose every cell is vouched for', async () => {
356+
const created = await cli('POST', '/api/v2/tables', {
357+
workspaceId: ws(),
358+
name: `clean_${generateId().slice(0, 8)}`,
359+
schema: { columns: [{ name: 'note', type: 'string' }] },
360+
})
361+
expect(created.status).toBe(201)
362+
const table = `/api/v2/tables/${JSON.parse(created.body).data.id}`
363+
for (const note of ['alpha second', 'beta']) {
364+
expect(
365+
(await cli('POST', `${table}/rows`, { workspaceId: ws(), data: { note } })).status
366+
).toBe(201)
367+
}
368+
const guess = { field: 'note', op: 'contains', value: 'second' }
369+
const search = await cli('POST', `${table}/rows/search`, { workspaceId: ws(), q: 'second' })
370+
expect(search.status).toBe(200)
371+
expect(JSON.parse(search.body).data.matches).toHaveLength(1)
372+
const query = await cli('POST', `${table}/query`, { workspaceId: ws(), predicate: guess })
373+
expect(query.status).toBe(200)
374+
expect(modelView(query)).toContain('alpha second')
375+
const count = await cli('POST', `${table}/query/count`, {
376+
workspaceId: ws(),
377+
predicate: guess,
378+
})
379+
expect(count.status).toBe(200)
380+
381+
/** A sidecar that claims exact but holds no valid entry list vouches for nothing. */
382+
const malformed = await cli('POST', `${table}/rows`, {
383+
workspaceId: ws(),
384+
data: { note: 'gamma' },
385+
})
386+
await db
387+
.update(userTableRowSecretProvenance)
388+
.set({ entries: sql`'{"not":"a list"}'::jsonb` })
389+
.where(eq(userTableRowSecretProvenance.rowId, JSON.parse(malformed.body).data.id))
390+
const afterCorruption = await cli('POST', `${table}/query`, {
391+
workspaceId: ws(),
392+
predicate: guess,
393+
})
394+
expect(afterCorruption.status).toBe(403)
342395
})
343396

344397
it('shares only clean files publicly on Chat’s behalf (#9, D1)', async () => {

‎apps/sim/lib/mothership/chat/process-contents.ts‎

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1276,12 +1276,22 @@ async function resolveTableSelectionResource(
12761276

12771277
const rendered = `${describe(sizeClause(lines.length, selectedRowCount - lines.length))}${lines.join('\n')}`
12781278
const projected = projectResolvedSecretModelContent(rendered, selectionRegistry)
1279+
/** A fallback names the table, which is redacted like the rest; unredactable, it is omitted. */
1280+
const fallback = (reason: string) => {
1281+
const named = `Selected ${scope} from table ${JSON.stringify(table.name)} (${selectedRowCount} rows, ${columns.length} columns). ${reason}`
1282+
const projectedName = projectResolvedSecretModelContent(named, selectionRegistry)
1283+
return projectedName.safe && typeof projectedName.value === 'string'
1284+
? projectedName.value
1285+
: `Selected ${scope} (${selectedRowCount} rows, ${columns.length} columns). ${reason}`
1286+
}
12791287
const preview =
12801288
!cellsProjected || !projected.safe || typeof projected.value !== 'string'
1281-
? `Selected ${scope} from table "${table.name}" (${selectedRowCount} rows, ${columns.length} columns). Cell values could not be checked for secret values, so none were inlined.`
1289+
? fallback('Cell values could not be checked for secret values, so none were inlined.')
12821290
: projected.value.length <= MAX_TABLE_SELECTION_PREVIEW_LENGTH
12831291
? `${projected.value}${omittedNote}`
1284-
: `Selected ${scope} from table "${table.name}" (${selectedRowCount} rows, ${columns.length} columns). The column headings exceed the selection content limit; no cell values were inlined.`
1292+
: fallback(
1293+
'The column headings exceed the selection content limit; no cell values were inlined.'
1294+
)
12851295
/** The descriptor names columns and rows; it is shown to the model too, so it is redacted too. */
12861296
const projectedSelection = projectResolvedSecretModelContent(selection, selectionRegistry)
12871297
const descriptor =

‎apps/sim/lib/table/application/rows.test.ts‎

Lines changed: 61 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@ import {
2020
} from '@sim/testing/mocks/table-rows-service.mock'
2121
import { workspaceAuthzMock, workspaceAuthzMockFns } from '@sim/testing/mocks/workspace-authz.mock'
2222
import { beforeEach, describe, expect, it, vi } from 'vitest'
23-
import type { TableDefinition } from '@/lib/table/types'
23+
import type { TableDefinition, TablePredicate } from '@/lib/table/types'
2424

2525
const {
2626
mockIsScopeCompatible,
@@ -29,9 +29,9 @@ const {
2929
mockRowProvenance,
3030
mockFindRowMatches,
3131
mockClassifyColumns,
32-
mockTableHasUnvouchedRows,
32+
mockTableHasCellsUnsafe,
3333
} = vi.hoisted(() => ({
34-
mockTableHasUnvouchedRows: vi.fn(),
34+
mockTableHasCellsUnsafe: vi.fn(),
3535
mockFindRowMatches: vi.fn(),
3636
mockClassifyColumns: vi.fn(),
3737
mockIsScopeCompatible: vi.fn(),
@@ -60,7 +60,7 @@ vi.mock('@/lib/table/column-types', () => ({
6060
vi.mock('@/lib/table/rows/secret-provenance', () => ({
6161
...tableRowsSecretProvenanceMock,
6262
classifyStoredTableRowColumns: mockClassifyColumns,
63-
tableHasUnvouchedRows: mockTableHasUnvouchedRows,
63+
tableHasCellsUnsafeForPredicate: mockTableHasCellsUnsafe,
6464
TableRowProvenanceReader: class extends tableRowsSecretProvenanceMock.TableRowProvenanceReader {
6565
rowProvenance(rowId: string) {
6666
return mockRowProvenance(rowId)
@@ -997,54 +997,86 @@ describe('row delivery to Chat', () => {
997997
}
998998
)
999999

1000-
describe('query counts Chat sees', () => {
1001-
const filtered = (principal: Caller) =>
1000+
describe('predicates Chat runs over cell values', () => {
1001+
const query = (principal: Caller, predicate?: TablePredicate) =>
10021002
queryTableRows.execute({
10031003
principal,
1004-
input: {
1005-
tableId: TABLE.id,
1006-
limit: 25,
1007-
includeTotal: true,
1008-
predicate: { all: [{ field: 'name', op: 'eq', value: 'guess' }] },
1009-
},
1004+
input: { tableId: TABLE.id, limit: 25, includeTotal: true, predicate },
10101005
})
1006+
const BY_NAME: TablePredicate = { all: [{ field: 'name', op: 'eq', value: 'guess' }] }
10111007

10121008
beforeEach(() => {
10131009
mockQueryRows.mockResolvedValue({ ...PAGE, rowCount: 1, totalCount: 3 })
1010+
mockTableHasCellsUnsafe.mockResolvedValue(false)
10141011
})
10151012

10161013
it('counts only the rows delivered on the page', async () => {
10171014
mockRowProvenance.mockReturnValue({ status: 'unknown' })
1018-
mockTableHasUnvouchedRows.mockResolvedValue(false)
1019-
const result = await observeContentDelivery({ admit: gate(false) }, () => filtered(CHAT))
1015+
const result = await observeContentDelivery({ admit: gate(false) }, () =>
1016+
query(CHAT, BY_NAME)
1017+
)
10201018
expect(result).toMatchObject({ rows: [], rowCount: 0, totalCount: 3 })
10211019
})
10221020

1023-
it('refuses a filtered total over a table holding rows Chat may not read', async () => {
1021+
it('refuses a predicate over cells it cannot vouch for, before running it', async () => {
1022+
mockTableHasCellsUnsafe.mockResolvedValue(true)
1023+
await expect(
1024+
observeContentDelivery({ admit: gate(true) }, () => query(CHAT, BY_NAME))
1025+
).rejects.toBeInstanceOf(ContentRefusedError)
1026+
expect(mockTableHasCellsUnsafe).toHaveBeenCalledWith(TABLE.id, new Set(['column-name']))
1027+
expect(mockQueryRows).not.toHaveBeenCalled()
1028+
})
1029+
1030+
it('refuses when a cell it reads becomes unsafe during the read', async () => {
1031+
mockTableHasCellsUnsafe.mockResolvedValueOnce(false).mockResolvedValueOnce(true)
10241032
mockRowProvenance.mockReturnValue({ status: 'exact', entries: [] })
1025-
mockTableHasUnvouchedRows.mockResolvedValue(true)
10261033
await expect(
1027-
observeContentDelivery({ admit: gate(true) }, () => filtered(CHAT))
1034+
observeContentDelivery({ admit: gate(true) }, () => query(CHAT, BY_NAME))
10281035
).rejects.toBeInstanceOf(ContentRefusedError)
10291036
})
10301037

1031-
it('keeps an unfiltered total, which says only how many rows exist', async () => {
1038+
it('checks nothing for an id-only predicate or no predicate, which read no cell', async () => {
10321039
mockRowProvenance.mockReturnValue({ status: 'exact', entries: [] })
1033-
mockTableHasUnvouchedRows.mockResolvedValue(true)
1034-
const result = await observeContentDelivery({ admit: gate(true) }, () =>
1035-
queryTableRows.execute({
1036-
principal: CHAT,
1037-
input: { tableId: TABLE.id, limit: 25, includeTotal: true },
1038-
})
1040+
await observeContentDelivery({ admit: gate(true) }, () => query(CHAT))
1041+
await observeContentDelivery({ admit: gate(true) }, () =>
1042+
query(CHAT, { all: [{ field: 'id', op: 'in', value: ['row-1'] }] })
10391043
)
1040-
expect(result).toMatchObject({ rowCount: 1, totalCount: 3 })
1041-
expect(mockTableHasUnvouchedRows).not.toHaveBeenCalled()
1044+
expect(mockTableHasCellsUnsafe).not.toHaveBeenCalled()
1045+
})
1046+
1047+
it('refuses a free-text search over a table with any unsafe cell', async () => {
1048+
mockTableHasCellsUnsafe.mockResolvedValue(true)
1049+
await expect(
1050+
observeContentDelivery({ admit: gate(true) }, () =>
1051+
searchTableRows.execute({ principal: CHAT, input: { tableId: TABLE.id, q: 'guess' } })
1052+
)
1053+
).rejects.toBeInstanceOf(ContentRefusedError)
1054+
expect(mockTableHasCellsUnsafe).toHaveBeenCalledWith(TABLE.id, 'all')
1055+
expect(mockFindRowMatches).not.toHaveBeenCalled()
1056+
})
1057+
1058+
it('refuses an update by filter over unsafe cells before changing anything', async () => {
1059+
mockTableHasCellsUnsafe.mockResolvedValue(true)
1060+
await expect(
1061+
observeContentDelivery({ admit: gate(true) }, () =>
1062+
updateTableRows.execute({
1063+
principal: CHAT,
1064+
input: {
1065+
tableId: TABLE.id,
1066+
filter: { name: 'guess' },
1067+
data: { name: 'x' },
1068+
dataKeying: 'names',
1069+
},
1070+
})
1071+
)
1072+
).rejects.toBeInstanceOf(ContentRefusedError)
1073+
expect(mockUpdateRowsByFilter).not.toHaveBeenCalled()
10421074
})
10431075

1044-
it('answers a person with the stored counts, unread', async () => {
1045-
const result = await filtered(PRINCIPAL)
1076+
it('answers a person with the stored counts, unchecked', async () => {
1077+
const result = await query(PRINCIPAL, BY_NAME)
10461078
expect(result).toMatchObject({ rowCount: 1, totalCount: 3 })
1047-
expect(mockTableHasUnvouchedRows).not.toHaveBeenCalled()
1079+
expect(mockTableHasCellsUnsafe).not.toHaveBeenCalled()
10481080
})
10491081
})
10501082

0 commit comments

Comments
 (0)