fix(mothership): tenant-check outputTable writes and route them through replaceTableRows#5011
fix(mothership): tenant-check outputTable writes and route them through replaceTableRows#5011TheodoreSpeaks wants to merge 2 commits into
Conversation
…gh replaceTableRows maybeWriteOutputToTable / maybeWriteReadCsvToTable accepted any table id without verifying it belongs to the caller's workspace, so a foreign table's rows could be wiped and replaced cross-tenant. They also wrote rows with raw drizzle keyed by column *name*, bypassing the service layer's job-slot lock, validation, plan row limits, rowCount maintenance, and the stable column-id keying every other writer uses. Both handlers now reject tables outside the caller's workspace and delegate to replaceTableRows with name→id remapped rows. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
PR SummaryHigh Risk Overview
Adds Reviewed by Cursor Bugbot for commit 68c3ebb. Bugbot is set up for automated code reviews on this repo. Configure here. |
|
@greptile review |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 49afd5f. Configure here.
| if (replaceResult.error) { | ||
| span.setAttribute(TraceAttr.CopilotTableOutcome, CopilotTableOutcome.InvalidShape) | ||
| return { success: false, error: replaceResult.error } | ||
| } |
There was a problem hiding this comment.
Abort ignored during table replace
Medium Severity
Replacing the inline delete/insert loop with a single replaceTableRowsFromWire call drops the per-chunk abortSignal checks that used to throw inside the transaction. If the request aborts while replaceTableRows is running, the replace still commits, yet the executor can mark the tool cancelled afterward—so the table is mutated while the run reports cancellation.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 49afd5f. Configure here.
There was a problem hiding this comment.
Intentional, matching the codebase's abort semantics: aborts gate before mutations (same as assertServerToolNotAborted in the user_table server tool), and the mutation itself is atomic. The old per-chunk checks threw inside the transaction, which rolled the whole replace back — so the only behavioral difference is that an abort landing mid-transaction no longer wastes the rollback; it can't leave the table half-written either way, since replaceTableRows commits all-or-nothing. Supporting mid-transaction cancellation would need a signal-aware service API, which isn't worth it for this window.
Greptile SummaryThis PR closes two security/correctness gaps in the Mothership
Confidence Score: 5/5Safe to merge. The changes are narrowly scoped to replacing two hand-rolled write paths with the existing service layer and adding a workspace ownership guard; the fix is well-tested and the new helper is straightforward. Both handlers now correctly reject cross-tenant table IDs and use the service layer for all writes, including its row-order lock, validation, and capacity checks. The test suite covers workspace rejection, column-id remapping, per-row empty-key fail-fast, and service-error surfacing for both handlers. No latent issues found. No files require special attention. Important Files Changed
Reviews (3): Last reviewed commit: "fix(mothership): fail outputTable writes..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if (idKeyedRows.every((row) => Object.keys(row).length === 0)) { | ||
| return { | ||
| error: `None of the row keys match columns on table "${table.name}" (columns: ${table.schema.columns.map((c) => c.name).join(', ')})`, | ||
| } | ||
| } |
There was a problem hiding this comment.
{} objects
The fail-fast guard fires only when every row maps to an empty object. If a user supplies mixed input where some rows have matching column names and others do not, the mismatched rows become {} objects forwarded to replaceTableRows. Depending on the service's validation, those rows may be inserted as blank records. A more targeted guard — filtering out zero-length rows before calling the service — would prevent silent data degradation.
Greptile SummaryThis PR fixes two bugs in the Mothership
Confidence Score: 3/5The workspace tenant check is implemented correctly and closes the cross-tenant table destruction path. The column-key translation and service delegation are sound. However, the fail-fast guard lets through batches where only some rows have matching columns, silently producing empty rows for non-matching records, and the service's in-batch uniqueness check looks up row values by column display name while the rows are now id-keyed — meaning duplicate values in unique:true columns are not caught. The critical security fix is correct, but the partially-matching-row behavior can silently insert blank records, and the uniqueness enforcement that routing through the service was supposed to provide is effectively a no-op for modern (id-keyed) columns — a pre-existing bug in the service that this PR makes newly relevant for this path. apps/sim/lib/copilot/request/tools/tables.ts (fail-fast guard and reliance on service uniqueness check) and apps/sim/lib/table/service.ts line 2052 (pre-existing col.name vs getColumnId(col) mismatch in the uniqueness loop). Important Files Changed
Sequence DiagramsequenceDiagram
participant Caller as Copilot Handler
participant Tables as tables.ts
participant Service as table/service.ts
participant DB as Database
Caller->>Tables: maybeWriteOutputToTable(toolName, params, result, ctx)
Tables->>Service: getTableById(outputTable)
Service-->>Tables: table
Tables->>Tables: "Check table.workspaceId === ctx.workspaceId"
alt workspace mismatch or table missing
Tables-->>Caller: "{success:false, error:Table not found}"
else workspace OK
Tables->>Tables: buildIdByName(table.schema)
Tables->>Tables: rowDataNameToId(wireRows) id-keyed rows
Tables->>Tables: idKeyedRows.every(empty)? fail fast
Tables->>Service: "replaceTableRows({tableId, rows, workspaceId, userId}, table, requestId)"
Service->>DB: acquireRowOrderLock(tableId)
Service->>DB: DELETE WHERE tableId
Service->>DB: INSERT id-keyed rows in batches
Service-->>Tables: "{deletedCount, insertedCount}"
Tables-->>Caller: "{success:true, output:{rowCount}}"
end
Reviews (2): Last reviewed commit: "fix(mothership): tenant-check outputTabl..." | Re-trigger Greptile |
… no columns Review feedback: the all-rows-empty guard let mixed batches slip unmatched rows through as empty objects. Reject on the first row that maps to zero columns, naming the row. Adds the missing CSV-suite parity tests (no-matching-headers, service-error surfacing). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@greptile review |
1 participant
Problem
The Mothership
outputTableside-channels (maybeWriteOutputToTableforfunction_executeresults,maybeWriteReadCsvToTableforreadCSV/JSON imports) had two serious issues:getTableById(outputTable)was never compared against the caller's workspace, so a prompt-supplied table id from another workspace would have its rows deleted and replaced. The replacement rows were written under the caller'sworkspaceId, making them invisible to the victim's tenant-scoped reads — the table was effectively destroyed.DELETE+ chunkedINSERTtransaction, skipping everythingreplaceTableRowsprovides: the row-order lock that serializes concurrent replaces, schema/row-size validation, plan row limits,rowCountmaintenance on the definition, scaled statement timeouts — and the stable column-id keying every other writer uses (rows were stored keyed by column name, so on id-keyed tables the data didn't surface in the grid).Fix
Both handlers now:
buildIdByName+rowDataNameToId), failing fast when no keys match any columnreplaceTableRowsThe handler-level
MAX_OUTPUT_TABLE_ROWS(10k) guard is unchanged.Tests
New
tables.test.ts: workspace-mismatch rejection (both handlers), name→id remapping through the service, no-matching-columns fail-fast, service-validation error surfacing.bun run lint:checkandtsc --noEmitclean.🤖 Generated with Claude Code