Skip to content

Commit dc8c8e5

Browse files
committed
fix(security): make the chat-upload ownership check atomic with its write
The ownership lookup and the binding UPDATE were separate statements, and the UPDATE matched on the captured row id alone. A concurrent `materialize_file` sets `context='workspace'` and clears `chatId` on that same row, so the tracking write still matched and dragged the saved file back into chat scope — hiding it from every workspace-file listing and re-exposing it to the chat-delete cascade, with materialize's storage-usage increment left stranded. Re-assert every ownership predicate in the UPDATE so the statement is itself the atomic check. The lookup now only decides UPDATE-vs-INSERT and is no longer load-bearing for authorization, which also makes the existing `updated.length === 0` fail-closed branch correct rather than dead.
1 parent a752842 commit dc8c8e5

2 files changed

Lines changed: 40 additions & 3 deletions

File tree

apps/sim/lib/uploads/contexts/workspace/track-chat-upload.test.ts

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -376,6 +376,31 @@ describe('trackChatUpload', () => {
376376
expect(dbChainMockFns.values).not.toHaveBeenCalled()
377377
})
378378

379+
/**
380+
* The ownership lookup and the write are separate statements, so a
381+
* concurrent `materialize_file` can flip the row to context='workspace'
382+
* in between. The UPDATE must re-assert every ownership predicate rather
383+
* than matching on the captured row id alone, or it would drag a saved
384+
* workspace file back into chat scope.
385+
*/
386+
it('re-asserts every ownership predicate in the update, not just the row id', async () => {
387+
queueOwnershipLookup([existingRow({ id: 'wf_mine' })])
388+
dbChainMockFns.returning.mockResolvedValueOnce([{ id: 'wf_mine' }])
389+
390+
await trackChatUpload(WORKSPACE_ID, USER_ID, CHAT_ID, S3_KEY, 'image.png', 'image/png', 1024)
391+
392+
expect(dbChainMockFns.where.mock.calls.at(-1)?.[0]).toEqual({
393+
type: 'and',
394+
conditions: [
395+
{ type: 'eq', left: 'id', right: 'wf_mine' },
396+
{ type: 'eq', left: 'userId', right: USER_ID },
397+
{ type: 'eq', left: 'workspaceId', right: WORKSPACE_ID },
398+
{ type: 'eq', left: 'context', right: 'mothership' },
399+
{ type: 'isNull', column: 'deletedAt' },
400+
],
401+
})
402+
})
403+
379404
it('requires the object to exist in storage before minting a new binding', async () => {
380405
queueOwnershipLookup([])
381406
mockHeadObject.mockResolvedValueOnce(null)

apps/sim/lib/uploads/contexts/workspace/workspace-file-manager.ts

Lines changed: 15 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -707,12 +707,24 @@ export async function trackChatUpload(
707707
context: 'mothership',
708708
displayName: candidate,
709709
})
710-
.where(eq(workspaceFiles.id, claimable.id))
710+
.where(
711+
and(
712+
eq(workspaceFiles.id, claimable.id),
713+
eq(workspaceFiles.userId, userId),
714+
eq(workspaceFiles.workspaceId, workspaceId),
715+
eq(workspaceFiles.context, 'mothership'),
716+
isNull(workspaceFiles.deletedAt)
717+
)
718+
)
711719
.returning({ id: workspaceFiles.id })
712720

713721
if (updated.length === 0) {
714-
// The row was deleted or re-owned between the ownership check and the
715-
// write. Fail closed rather than silently minting a new binding.
722+
// The ownership lookup is a separate statement, so re-assert every
723+
// predicate here — this UPDATE is the atomic check. A concurrent
724+
// `materialize_file` flips the same row to context='workspace' and
725+
// clears chatId; matching on id alone would drag that saved file back
726+
// into chat scope, hiding it from the Files listing and re-exposing it
727+
// to the chat-delete cascade.
716728
throw new WorkspaceFileKeyOwnershipError(s3Key)
717729
}
718730

0 commit comments

Comments
 (0)