Skip to content

Commit ee40593

Browse files
committed
fix(table): govern CSV-import auto-fire by the person the route already gated
A synchronous CSV import passed an explicit null governed subject, so the workflow and enrichment cells the appended rows auto-fire ran with no per-tool gate — including when a session member with a restricting permission group started the import. The subject now comes from the same `TableAccessPrincipal` `checkAccess` gated `tables.use` against, so a route cannot gate one subject and dispatch under another.
1 parent 3e30fe3 commit ee40593

6 files changed

Lines changed: 117 additions & 17 deletions

File tree

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

Lines changed: 11 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -21,9 +21,11 @@ import { performTableCsvImport } from '@/lib/table/orchestration'
2121
import { getUserSettings } from '@/lib/users/queries'
2222
import {
2323
accessError,
24+
capabilityGovernedUserId,
2425
checkAccess,
2526
csvProxyBodyCapResponse,
2627
multipartErrorResponse,
28+
type TableAccessPrincipal,
2729
} from '@/app/api/table/utils'
2830

2931
const logger = createLogger('TableImportCSVExisting')
@@ -96,11 +98,8 @@ export const POST = withRouteHandler(async (request: NextRequest, { params }: Ro
9698
)
9799
}
98100

99-
const accessResult = await checkAccess(
100-
tableId,
101-
{ kind: 'user', userId: authResult.userId },
102-
'write'
103-
)
101+
const principal: TableAccessPrincipal = { kind: 'user', userId: authResult.userId }
102+
const accessResult = await checkAccess(tableId, principal, 'write')
104103
if (!accessResult.ok) return accessError(accessResult, requestId, tableId)
105104

106105
const { table } = accessResult
@@ -160,6 +159,13 @@ export const POST = withRouteHandler(async (request: NextRequest, { params }: Ro
160159
createColumns,
161160
timezone,
162161
requestId,
162+
/**
163+
* An append starts the table's workflow columns on every row it lands, so
164+
* those cells are governed by the same person `checkAccess` just gated —
165+
* not left ungoverned, which would let an import run tools this member's
166+
* permission group withholds.
167+
*/
168+
capabilityGovernedUserId: capabilityGovernedUserId(principal),
163169
})
164170

165171
if (!outcome.success) {

apps/sim/app/api/table/import-csv/route.ts

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -131,6 +131,14 @@ export const POST = withRouteHandler(async (request: NextRequest) => {
131131
folderId,
132132
timezone,
133133
requestId,
134+
/**
135+
* The session or internal-JWT person this route already gated
136+
* `tables.create` against. A table created here has no workflow columns
137+
* yet, so nothing auto-fires today; naming the subject anyway keeps the
138+
* rule "the id the surface gated is the id the write dispatches under"
139+
* with no producer-specific exception to re-argue.
140+
*/
141+
capabilityGovernedUserId: userId,
134142
})
135143

136144
if (!outcome.success) {

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

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -268,8 +268,14 @@ function roleSubjectUserId(principal: TableAccessPrincipal): string {
268268
/**
269269
* The id whose permission group governs the request, or `null` when no group
270270
* does. Only a `user` principal has one — see {@link TableAccessPrincipal}.
271+
*
272+
* Exported because the gate is not the only thing that needs the subject: a
273+
* write that lands rows auto-fires the table's workflow and enrichment cells,
274+
* and those cells must run under the same person this check just gated, not
275+
* under whatever id the surface had nearest. One statement of the rule, so a
276+
* route cannot gate one subject and dispatch another.
271277
*/
272-
function capabilityGovernedUserId(principal: TableAccessPrincipal): string | null {
278+
export function capabilityGovernedUserId(principal: TableAccessPrincipal): string | null {
273279
return principal.kind === 'user' ? principal.userId : null
274280
}
275281

apps/sim/lib/table/import-data.ts

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -265,7 +265,14 @@ export async function importAppendRows(
265265
table: TableDefinition,
266266
additions: { id?: string; name: string; type: string; required?: boolean; unique?: boolean }[],
267267
rows: RowData[],
268-
ctx: { workspaceId: string; userId?: string; requestId: string }
268+
ctx: {
269+
workspaceId: string
270+
userId?: string
271+
requestId: string
272+
/** Gate subject for cells the appended rows auto-fire — the subject the
273+
* importing surface resolved from its principal, or `null` for none. */
274+
capabilityGovernedUserId: string | null
275+
}
269276
): Promise<{ inserted: TableRow[]; table: TableDefinition }> {
270277
// Gate capacity before opening the tx — the lookup is a separate pool read.
271278
const rowLimit = await assertRowCapacity({
@@ -294,8 +301,7 @@ export async function importAppendRows(
294301
rows: batch,
295302
workspaceId: ctx.workspaceId,
296303
userId: ctx.userId,
297-
/** CSV import is auto-fire: no acting person governs the rows it lands. */
298-
capabilityGovernedUserId: null,
304+
capabilityGovernedUserId: ctx.capabilityGovernedUserId,
299305
secretProvenance: batch.map(createExactEmptyTableRowSecretProvenance),
300306
},
301307
working,

apps/sim/lib/table/orchestration/import.test.ts

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -91,6 +91,7 @@ function importParams(overrides: Record<string, unknown> = {}) {
9191
mode: 'append' as const,
9292
timezone: 'UTC',
9393
requestId: 'req-1',
94+
capabilityGovernedUserId: 'user-1' as string | null,
9495
...overrides,
9596
}
9697
}
@@ -133,6 +134,43 @@ describe('performTableCsvImport', () => {
133134
expect(mockSignalSchemaChanged).toHaveBeenCalledWith('table-1')
134135
})
135136

137+
/**
138+
* The rows an import lands start the table's workflow columns, and those
139+
* cells gate their tools on the governed subject. Dropping it here would run
140+
* the importing member's cells with no per-tool gate at all — the one thing
141+
* `null` means on this field.
142+
*/
143+
it('dispatches the auto-fired cells under the importing person', async () => {
144+
await performTableCsvImport(importParams({ capabilityGovernedUserId: 'user-9' }))
145+
146+
expect(mockDispatchAfterBatchInsert).toHaveBeenCalledWith(
147+
expect.anything(),
148+
expect.anything(),
149+
'req-1',
150+
'user-1',
151+
'user-9'
152+
)
153+
expect(mockImportAppendRows).toHaveBeenCalledWith(
154+
expect.anything(),
155+
expect.anything(),
156+
expect.anything(),
157+
expect.objectContaining({ capabilityGovernedUserId: 'user-9' })
158+
)
159+
})
160+
161+
/** An actorless import still says so explicitly rather than by omission. */
162+
it('carries a null subject through unchanged', async () => {
163+
await performTableCsvImport(importParams({ capabilityGovernedUserId: null }))
164+
165+
expect(mockDispatchAfterBatchInsert).toHaveBeenCalledWith(
166+
expect.anything(),
167+
expect.anything(),
168+
'req-1',
169+
'user-1',
170+
null
171+
)
172+
})
173+
136174
it('reports the deleted count on a replace', async () => {
137175
const result = await performTableCsvImport(importParams({ mode: 'replace' }))
138176

@@ -344,6 +382,7 @@ describe('performCreateTableFromCsv', () => {
344382
folderId: null,
345383
timezone: 'UTC',
346384
requestId: 'req-1',
385+
capabilityGovernedUserId: 'user-1',
347386
}
348387
}
349388

apps/sim/lib/table/orchestration/import.ts

Lines changed: 43 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -234,6 +234,15 @@ export interface PerformTableCsvImportParams {
234234
/** IANA zone used to read naive datetimes (Excel/Sheets exports carry no offset). */
235235
timezone: string
236236
requestId?: string
237+
/**
238+
* The person whose permission group gates any cell this import auto-fires,
239+
* or `null` when no person is behind it. Required with an explicit `null`
240+
* rather than optional, matching `insertDispatch`: an import lands rows, and
241+
* landing rows starts workflow and enrichment cells on the table's workflow
242+
* columns. Threaded from the surface that holds the principal rather than
243+
* re-derived here — the route has already gated the same subject.
244+
*/
245+
capabilityGovernedUserId: string | null
237246
}
238247

239248
export interface TableCsvImportData extends ImportRejectionFields {
@@ -271,8 +280,17 @@ export interface PerformTableCsvImportResult {
271280
export async function performTableCsvImport(
272281
params: PerformTableCsvImportParams
273282
): Promise<PerformTableCsvImportResult> {
274-
const { table, workspaceId, userId, fileStream, fileName, fallbackDelimiter, mode, timezone } =
275-
params
283+
const {
284+
table,
285+
workspaceId,
286+
userId,
287+
fileStream,
288+
fileName,
289+
fallbackDelimiter,
290+
mode,
291+
timezone,
292+
capabilityGovernedUserId,
293+
} = params
276294
const requestId = params.requestId ?? generateRequestId()
277295

278296
if (table.archivedAt) return fail('Cannot import into an archived table', 'validation')
@@ -367,11 +385,11 @@ export async function performTableCsvImport(
367385
workspaceId,
368386
userId,
369387
requestId,
388+
capabilityGovernedUserId,
370389
})
371390
// Fire trigger + scheduler AFTER the tx commits — both read through the
372391
// global db connection and would otherwise see no rows.
373-
/** CSV import is auto-fire: no acting person governs the rows it lands. */
374-
dispatchAfterBatchInsert(finalTable, inserted, requestId, userId, null)
392+
dispatchAfterBatchInsert(finalTable, inserted, requestId, userId, capabilityGovernedUserId)
375393

376394
logger.info(`[${requestId}] Append CSV imported`, {
377395
tableId: table.id,
@@ -419,6 +437,16 @@ export async function performTableCsvImport(
419437
export interface PerformCreateTableFromCsvParams {
420438
workspaceId: string
421439
userId: string
440+
/**
441+
* The person whose permission group gates any cell this import auto-fires,
442+
* or `null` when no person is behind it. Required with an explicit `null`
443+
* rather than optional, matching `insertDispatch`: an import lands rows, and
444+
* landing rows starts workflow and enrichment cells on the table's workflow
445+
* columns. Threaded from the surface that holds the principal rather than
446+
* re-derived here — the route has already gated the same subject.
447+
*/
448+
capabilityGovernedUserId: string | null
449+
422450
/** Multipart file stream. The caller still owns destroying it. */
423451
fileStream: Readable
424452
fileName: string
@@ -462,8 +490,16 @@ export interface PerformCreateTableFromCsvResult {
462490
export async function performCreateTableFromCsv(
463491
params: PerformCreateTableFromCsvParams
464492
): Promise<PerformCreateTableFromCsvResult> {
465-
const { workspaceId, userId, fileStream, fileName, fallbackDelimiter, folderId, timezone } =
466-
params
493+
const {
494+
workspaceId,
495+
userId,
496+
fileStream,
497+
fileName,
498+
fallbackDelimiter,
499+
folderId,
500+
timezone,
501+
capabilityGovernedUserId,
502+
} = params
467503
const requestId = params.requestId ?? generateRequestId()
468504

469505
const { delimiter, stream } = await sniffCsvDelimiterFromStream(fileStream, fallbackDelimiter)
@@ -508,8 +544,7 @@ export async function performCreateTableFromCsv(
508544
rows: coerced as RowData[],
509545
workspaceId,
510546
userId,
511-
/** CSV import is auto-fire: no acting person governs the rows it lands. */
512-
capabilityGovernedUserId: null,
547+
capabilityGovernedUserId,
513548
secretProvenance: coerced.map(createExactEmptyTableRowSecretProvenance),
514549
},
515550
// The created table's rowCount is frozen at 0; pass the running total so the

0 commit comments

Comments
 (0)