Skip to content

Commit 3385594

Browse files
committed
fix(mothership): withhold unregistered table secrets from sim_cli results
Table rows read through sim_cli reached the model without their persisted secret provenance, so stored secrets in cells were never redacted. Row use cases now report the provenance of the rows they return to an observing transport (mirroring the workspace-file delivery observer), and the agent CLI table transport imports it into the tool call's registry, answers 503 without a registry, and marks the registry incomplete when a row-bearing table route returns without reporting provenance. Provenance reported by detached work after the call settles is ignored. Export download links are refused outright: a signed link to the whole table as plaintext CSV cannot carry provenance once fetched. Run-state and enrichment error text (runState.error, blockErrors, enrichment provider errors) is captured from executor output without its secret provenance, so a read that returns any of it is withheld as well; reads whose run state carries no error text are unaffected.
1 parent 22277c1 commit 3385594

9 files changed

Lines changed: 856 additions & 22 deletions

File tree

‎apps/sim/executor/utils/resolved-secret-trace-registry.test.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1798,6 +1798,7 @@ describe('incompleteness diagnostics', () => {
17981798
'client-tool-content-unavailable',
17991799
'knowledge-result-provenance-unavailable',
18001800
'table-result-provenance-unavailable',
1801+
'table-run-state-provenance-unavailable',
18011802
'mounted-file-provenance-unavailable',
18021803
'workspace-file-provenance-unknown',
18031804
'file-source-unidentified',

‎apps/sim/executor/utils/resolved-secret-trace-registry.ts‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -70,6 +70,11 @@ export type ResolvedSecretIncompletenessReason =
7070
| 'knowledge-row-missing'
7171
| 'knowledge-row-content-mismatch'
7272
| 'table-result-provenance-unavailable'
73+
/**
74+
* A table result carried run-state or enrichment error text, captured from executor output that
75+
* can hold resolved secret plaintext, with no provenance persisted beside it.
76+
*/
77+
| 'table-run-state-provenance-unavailable'
7378
| 'mounted-file-provenance-unavailable'
7479
| 'workspace-file-provenance-unknown'
7580
| 'file-source-unidentified'

‎apps/sim/lib/api/server/routes/in-process-transport.ts‎

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,8 @@ type RouteHandler = (
2424
) => Promise<Response>
2525

2626
interface CompiledRoute {
27+
/** The generated route pattern, e.g. `/api/v2/tables/{tableId}/rows`. */
28+
pattern: string
2729
regex: RegExp
2830
params: string[]
2931
/** Literal segments — a more specific pattern wins over a parameterized one. */
@@ -32,6 +34,7 @@ interface CompiledRoute {
3234
}
3335

3436
interface MatchedRoute {
37+
pattern: string
3538
params: Record<string, string>
3639
literals: number
3740
load: () => Promise<object>
@@ -52,7 +55,13 @@ const COMPILED: CompiledRoute[] = V2_ROUTES.map((route) => {
5255
return '([^/]+)'
5356
})
5457
.join('/')
55-
return { regex: new RegExp(`^${source}$`), params, literals, load: route.load }
58+
return {
59+
pattern: route.pattern,
60+
regex: new RegExp(`^${source}$`),
61+
params,
62+
literals,
63+
load: route.load,
64+
}
5665
})
5766

5867
export function matchV2Route(pathname: string): MatchedRoute | null {
@@ -65,7 +74,7 @@ export function matchV2Route(pathname: string): MatchedRoute | null {
6574
route.params.forEach((name, index) => {
6675
params[name] = decodeURIComponent(match[index + 1] ?? '')
6776
})
68-
best = { params, literals: route.literals, load: route.load }
77+
best = { pattern: route.pattern, params, literals: route.literals, load: route.load }
6978
}
7079
return best
7180
}

‎apps/sim/lib/mothership/agent-cli/index.ts‎

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import { runCli } from '@/lib/mothership/agent-cli/run-cli'
1111
import { createScopedCliTransport } from '@/lib/mothership/agent-cli/scoped-transport'
1212
import { executeAgentCliService } from '@/lib/mothership/agent-cli/services'
1313
import { applySink } from '@/lib/mothership/agent-cli/sink'
14+
import { createTableReadTransport } from '@/lib/mothership/agent-cli/table-read-transport'
1415
import { createTracedCliTransport } from '@/lib/mothership/agent-cli/traced-transport'
1516
import { createWorkbenchFileProvenance } from '@/lib/mothership/agent-cli/workbench-file-provenance'
1617
import { resolveInvocationWorkspace } from '@/lib/mothership/application/workspace-target'
@@ -80,10 +81,14 @@ async function executeBoundAgentCliRequest(
8081
const files = sessionKey ? createWorkbenchFileProvenance({ ...context, sessionKey }) : undefined
8182
const reads = createFileReadTransport({
8283
endpoint,
83-
transport: createTracedCliTransport(
84+
transport: createTableReadTransport({
8485
endpoint,
85-
createScopedCliTransport(endpoint, invocationIdentity)
86-
),
86+
transport: createTracedCliTransport(
87+
endpoint,
88+
createScopedCliTransport(endpoint, invocationIdentity)
89+
),
90+
registry: context.resolvedSecretTraceRegistry,
91+
}),
8792
userId: context.userId,
8893
invocation: invocationIdentity,
8994
registry: context.resolvedSecretTraceRegistry,
Lines changed: 280 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,280 @@
1+
/** @vitest-environment node */
2+
import { beforeEach, describe, expect, it, vi } from 'vitest'
3+
4+
const mocks = vi.hoisted(() => ({ scoped: vi.fn(), decrypt: vi.fn() }))
5+
vi.mock('@/lib/core/security/encryption', () => ({ decryptSecret: mocks.decrypt }))
6+
vi.mock('@/lib/mothership/agent-cli/scoped-transport', () => ({
7+
createScopedCliTransport: () => mocks.scoped,
8+
}))
9+
vi.mock('@/lib/mothership/application/workspace-target', () => ({
10+
resolveInvocationWorkspace: async (owner: { userId: string }, workspaceId?: string) => ({
11+
workspaceId: workspaceId ?? 'workspace',
12+
userId: owner.userId,
13+
}),
14+
}))
15+
vi.mock('@/lib/execution/remote-sandbox/session-files', () => ({
16+
SESSION_SANDBOX_HOME: '/home/user',
17+
readSessionSandboxFile: vi.fn(),
18+
writeSessionSandboxFile: vi.fn(),
19+
}))
20+
vi.mock('@/lib/execution/remote-sandbox/session-file-snapshot', () => ({
21+
openSessionFileSnapshot: vi.fn(),
22+
}))
23+
24+
import { V2_ROUTES } from '@/lib/api/server/routes/v2-route-table.generated'
25+
import {
26+
createTableReadTransport,
27+
TABLE_ROUTES_WITHOUT_ROW_DATA,
28+
} from '@/lib/mothership/agent-cli/table-read-transport'
29+
import { inspectToolResultForCopilot } from '@/lib/mothership/request/tools/resolved-secret-result'
30+
import { executeSimCli } from '@/lib/mothership/tools/handlers/sim-cli'
31+
import { reportTableRowDelivery } from '@/lib/table/application/row-delivery-observer'
32+
import { ResolvedSecretTraceRegistry } from '@/executor/utils/resolved-secret-trace-registry'
33+
34+
const SECRET = 'PRIVATE_TABLE_CELL_CANARY_FOR_LOCAL_TEST'
35+
const scope = { userId: 'reader', workspaceId: 'workspace' }
36+
const endpoint = 'https://sim.test'
37+
const rowUrl = `${endpoint}/api/v2/tables/table/rows/row?workspaceId=workspace`
38+
const rowBody = { data: { id: 'row', data: { token: SECRET } } }
39+
40+
function registry() {
41+
return new ResolvedSecretTraceRegistry([], scope)
42+
}
43+
44+
function secretProvenance() {
45+
const source = new ResolvedSecretTraceRegistry(
46+
[{ name: 'TABLE_SECRET', plaintext: SECRET, encryptedValue: 'fixture-ciphertext' }],
47+
scope
48+
)
49+
source.recordResolved('TABLE_SECRET', SECRET, { propagated: true })
50+
return source.exportProvenance()
51+
}
52+
53+
/** Stands in for a v2 table route whose use case reports the rows it returns. */
54+
async function deliveringRoute() {
55+
await reportTableRowDelivery(secretProvenance(), [{ col_token: SECRET }])
56+
return Response.json(rowBody)
57+
}
58+
59+
function projected(output: string, trace: ResolvedSecretTraceRegistry) {
60+
return JSON.stringify(inspectToolResultForCopilot({ success: true, output }, trace, 'sim_cli'))
61+
}
62+
63+
describe('table provenance at the CLI and model-result boundary', () => {
64+
beforeEach(() => {
65+
vi.clearAllMocks()
66+
mocks.decrypt.mockResolvedValue({ decrypted: SECRET })
67+
})
68+
69+
it('activates reported row provenance so the model projection redacts the cell', async () => {
70+
const trace = registry()
71+
const inner = vi.fn(deliveringRoute)
72+
const response = await createTableReadTransport({
73+
endpoint,
74+
transport: inner,
75+
registry: trace,
76+
})(rowUrl)
77+
78+
expect(response.status).toBe(200)
79+
const output = await response.text()
80+
expect(output).toBe(JSON.stringify(rowBody))
81+
expect(trace.isPermanentlyIncomplete()).toBe(false)
82+
expect(projected(output, trace)).not.toContain(SECRET)
83+
})
84+
85+
it('withholds a row-bearing result that reported no provenance', async () => {
86+
const trace = registry()
87+
const response = await createTableReadTransport({
88+
endpoint,
89+
transport: async () => Response.json(rowBody),
90+
registry: trace,
91+
})(rowUrl)
92+
93+
expect(response.status).toBe(200)
94+
expect(trace.isPermanentlyIncomplete()).toBe(true)
95+
expect(projected(await response.text(), trace)).not.toContain(SECRET)
96+
})
97+
98+
it('withholds a result whose run state carries error text without provenance', async () => {
99+
const trace = registry()
100+
const response = await createTableReadTransport({
101+
endpoint,
102+
transport: async () => {
103+
await reportTableRowDelivery(secretProvenance(), [{ col_token: SECRET }], {
104+
unprovenancedErrorText: true,
105+
})
106+
return Response.json(rowBody)
107+
},
108+
registry: trace,
109+
})(rowUrl)
110+
111+
expect(response.status).toBe(200)
112+
expect(trace.isPermanentlyIncomplete()).toBe(true)
113+
expect(trace.getIncompletenessDiagnostics()?.reasons).toEqual([
114+
'table-run-state-provenance-unavailable',
115+
])
116+
})
117+
118+
it('keeps a delivered result without run-state error text complete', async () => {
119+
const trace = registry()
120+
await createTableReadTransport({
121+
endpoint,
122+
transport: async () => {
123+
await reportTableRowDelivery(secretProvenance(), [{ col_token: SECRET }], {
124+
unprovenancedErrorText: false,
125+
})
126+
return Response.json(rowBody)
127+
},
128+
registry: trace,
129+
})(rowUrl)
130+
131+
expect(trace.isPermanentlyIncomplete()).toBe(false)
132+
})
133+
134+
it.each(['GET', 'HEAD'])(
135+
'refuses to return a table export download link (%s)',
136+
async (method) => {
137+
const trace = registry()
138+
const inner = vi.fn(async () =>
139+
Response.json({ data: { url: 'https://signed.test/export.csv' } })
140+
)
141+
const response = await createTableReadTransport({
142+
endpoint,
143+
transport: inner,
144+
registry: trace,
145+
})(`${endpoint}/api/v2/tables/table/exports/export/download?workspaceId=workspace`, {
146+
method,
147+
})
148+
149+
expect(response.status).toBe(403)
150+
expect(await response.text()).not.toContain('signed.test')
151+
expect(inner).not.toHaveBeenCalled()
152+
expect(trace.isPermanentlyIncomplete()).toBe(false)
153+
}
154+
)
155+
156+
it('ignores provenance that detached work reports after the call settles', async () => {
157+
const trace = registry()
158+
let release!: () => void
159+
const released = new Promise<void>((resolve) => {
160+
release = resolve
161+
})
162+
let detached: Promise<void> | undefined
163+
await createTableReadTransport({
164+
endpoint,
165+
transport: async () => {
166+
detached = released.then(() =>
167+
reportTableRowDelivery(secretProvenance(), [{ col_token: SECRET }])
168+
)
169+
return Response.json({ error: { message: 'Row not found' } }, { status: 404 })
170+
},
171+
registry: trace,
172+
})(rowUrl)
173+
174+
release()
175+
await detached
176+
expect(projected(JSON.stringify(rowBody), trace)).toContain(SECRET)
177+
})
178+
179+
it('refuses row reads without a registry instead of returning plaintext', async () => {
180+
const inner = vi.fn(deliveringRoute)
181+
const response = await createTableReadTransport({ endpoint, transport: inner })(rowUrl)
182+
183+
expect(response.status).toBe(503)
184+
expect(await response.text()).not.toContain(SECRET)
185+
expect(inner).not.toHaveBeenCalled()
186+
})
187+
188+
it('keeps a failed row read from poisoning the turn', async () => {
189+
const trace = registry()
190+
const response = await createTableReadTransport({
191+
endpoint,
192+
transport: async () =>
193+
Response.json({ error: { message: 'Row not found' } }, { status: 404 }),
194+
registry: trace,
195+
})(rowUrl)
196+
197+
expect(response.status).toBe(404)
198+
expect(trace.isPermanentlyIncomplete()).toBe(false)
199+
})
200+
201+
it.each([
202+
['GET', `${endpoint}/api/v2/files/file?workspaceId=workspace`],
203+
['GET', `${endpoint}/api/v2/tables/table?workspaceId=workspace`],
204+
['POST', `${endpoint}/api/v2/tables/table/rows/search`],
205+
['DELETE', `${endpoint}/api/v2/tables/table/rows/row?workspaceId=workspace`],
206+
['GET', 'https://elsewhere.test/api/v2/tables/table/rows/row'],
207+
])('passes %s %s through untouched without a registry', async (method, url) => {
208+
const upstream = Response.json({ data: { ok: true } })
209+
const inner = vi.fn(async () => upstream)
210+
const response = await createTableReadTransport({ endpoint, transport: inner })(url, {
211+
method,
212+
})
213+
214+
expect(response).toBe(upstream)
215+
expect(inner).toHaveBeenCalledWith(url, { method })
216+
})
217+
218+
it('composes into the sim_cli stack so a table read through the real CLI is redacted', async () => {
219+
mocks.scoped.mockImplementation(deliveringRoute)
220+
const trace = registry()
221+
const result = await executeSimCli(
222+
{
223+
request: {
224+
invocation: {
225+
kind: 'cli',
226+
argv: ['tables', 'rows', 'get', 'table', 'row'],
227+
},
228+
},
229+
},
230+
{
231+
userId: 'reader',
232+
workspaceId: 'workspace',
233+
workflowId: '',
234+
chatId: 'chat',
235+
resolvedSecretTraceRegistry: trace,
236+
}
237+
)
238+
239+
expect(mocks.scoped).toHaveBeenCalled()
240+
expect(new URL(String(mocks.scoped.mock.calls[0]?.[0])).pathname).toBe(
241+
'/api/v2/tables/table/rows/row'
242+
)
243+
expect(JSON.stringify(result.output)).toContain(SECRET)
244+
expect(trace.isPermanentlyIncomplete()).toBe(false)
245+
expect(JSON.stringify(inspectToolResultForCopilot(result, trace, 'sim_cli'))).not.toContain(
246+
SECRET
247+
)
248+
})
249+
250+
/**
251+
* Every v2 table route is either declared row-free or must deliver provenance.
252+
* A new route lands in the row-bearing set by default — its results are withheld
253+
* until its use case reports delivery — and this list forces that to be a decision.
254+
*/
255+
it('classifies every v2 table route', async () => {
256+
const methods = ['GET', 'POST', 'PUT', 'PATCH', 'DELETE'] as const
257+
const declared = new Set<string>()
258+
for (const route of V2_ROUTES) {
259+
if (route.pattern !== '/api/v2/tables' && !route.pattern.startsWith('/api/v2/tables/'))
260+
continue
261+
const module = await route.load()
262+
for (const method of methods) {
263+
if (typeof Reflect.get(module, method) === 'function')
264+
declared.add(`${method} ${route.pattern}`)
265+
}
266+
}
267+
268+
expect([...TABLE_ROUTES_WITHOUT_ROW_DATA].filter((key) => !declared.has(key))).toEqual([])
269+
expect([...declared].filter((key) => !TABLE_ROUTES_WITHOUT_ROW_DATA.has(key)).sort()).toEqual([
270+
'GET /api/v2/tables/{tableId}/exports/{exportId}/download',
271+
'GET /api/v2/tables/{tableId}/rows',
272+
'GET /api/v2/tables/{tableId}/rows/{rowId}',
273+
'GET /api/v2/tables/{tableId}/rows/{rowId}/enrichment/{groupId}',
274+
'PATCH /api/v2/tables/{tableId}/rows/{rowId}',
275+
'POST /api/v2/tables/{tableId}/query',
276+
'POST /api/v2/tables/{tableId}/rows',
277+
'POST /api/v2/tables/{tableId}/rows/upsert',
278+
])
279+
}, 60_000)
280+
})

0 commit comments

Comments
 (0)