Skip to content

Commit d88c822

Browse files
committed
fix(security): compare-and-swap the chat binding on chat uploads
Two overlapping chat requests could both observe the same claimable row with `chat_id IS NULL` and both satisfy the update predicate, so the later write silently moved the upload to its own chat — taking over the delete-cascade lifecycle of a file the first chat had already bound. Scope the update to `chat_id IS NULL OR chat_id = <target>` so the statement is a compare-and-swap: the loser matches zero rows and fails closed. The resolver applies the same rule so the update-vs-insert decision stays coherent. This also makes an upload bind to exactly one chat, matching the 409 the sibling `local-files/stage` route already returns for the same case; verified no client flow relinks a key across chats (drafts are per-chat, every retry path replays under a pinned chat id, forking copies blobs to fresh keys).
1 parent 760f07a commit d88c822

2 files changed

Lines changed: 57 additions & 4 deletions

File tree

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

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,7 @@ function existingRow(overrides: Record<string, unknown> = {}) {
5252
userId: USER_ID,
5353
workspaceId: WORKSPACE_ID,
5454
context: 'mothership',
55+
chatId: null,
5556
deletedAt: null,
5657
...overrides,
5758
}
@@ -397,10 +398,49 @@ describe('trackChatUpload', () => {
397398
{ type: 'eq', left: 'workspaceId', right: WORKSPACE_ID },
398399
{ type: 'eq', left: 'context', right: 'mothership' },
399400
{ type: 'isNull', column: 'deletedAt' },
401+
{
402+
type: 'or',
403+
conditions: [
404+
{ type: 'isNull', column: 'chatId' },
405+
{ type: 'eq', left: 'chatId', right: CHAT_ID },
406+
],
407+
},
400408
],
401409
})
402410
})
403411

412+
/**
413+
* An upload binds to exactly one chat. Matches the 409 the sibling
414+
* `local-files/stage` route already returns for this case.
415+
*/
416+
it('refuses to relink an upload already bound to a different chat', async () => {
417+
queueOwnershipLookup([existingRow({ chatId: 'other-chat-id' })])
418+
419+
await expect(
420+
trackChatUpload(WORKSPACE_ID, USER_ID, CHAT_ID, S3_KEY, 'image.png', 'image/png', 1024)
421+
).rejects.toThrow('not available for a chat attachment')
422+
423+
expect(dbChainMockFns.set).not.toHaveBeenCalled()
424+
expect(dbChainMockFns.values).not.toHaveBeenCalled()
425+
})
426+
427+
it('still re-links an upload already bound to this same chat', async () => {
428+
queueOwnershipLookup([existingRow({ chatId: CHAT_ID })])
429+
dbChainMockFns.returning.mockResolvedValueOnce([{ id: 'wf_existing' }])
430+
431+
const result = await trackChatUpload(
432+
WORKSPACE_ID,
433+
USER_ID,
434+
CHAT_ID,
435+
S3_KEY,
436+
'image.png',
437+
'image/png',
438+
1024
439+
)
440+
441+
expect(result).toEqual({ displayName: 'image.png' })
442+
})
443+
404444
it('requires the object to exist in storage before minting a new binding', async () => {
405445
queueOwnershipLookup([])
406446
mockHeadObject.mockResolvedValueOnce(null)

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

Lines changed: 17 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ import { workspaceFiles } from '@sim/db/schema'
99
import { createLogger } from '@sim/logger'
1010
import { getErrorMessage, getPostgresConstraintName, getPostgresErrorCode } from '@sim/utils/errors'
1111
import { generateShortId } from '@sim/utils/id'
12-
import { and, eq, isNotNull, isNull, sql } from 'drizzle-orm'
12+
import { and, eq, isNotNull, isNull, or, sql } from 'drizzle-orm'
1313
import type { ShareRecord } from '@/lib/api/contracts/public-shares'
1414
import {
1515
decrementStorageUsageForBillingContextInTx,
@@ -617,10 +617,15 @@ type ClaimableChatUploadRow = { kind: 'update'; id: string } | { kind: 'insert'
617617
* Soft-deleted rows count: the active-key unique index is partial on
618618
* `deleted_at IS NULL`, so inserting over an archived row would succeed and
619619
* hand the caller read access to the archived file's bytes.
620+
*
621+
* An upload also binds to exactly one chat: a row already linked to a different
622+
* chat is not claimable, matching the 409 the sibling `local-files/stage` route
623+
* returns for the same case. Re-sending the key within its own chat still works.
620624
*/
621625
async function resolveClaimableChatUploadRow(
622626
workspaceId: string,
623627
userId: string,
628+
chatId: string,
624629
s3Key: string
625630
): Promise<ClaimableChatUploadRow> {
626631
const rows = await db
@@ -629,6 +634,7 @@ async function resolveClaimableChatUploadRow(
629634
userId: workspaceFiles.userId,
630635
workspaceId: workspaceFiles.workspaceId,
631636
context: workspaceFiles.context,
637+
chatId: workspaceFiles.chatId,
632638
deletedAt: workspaceFiles.deletedAt,
633639
})
634640
.from(workspaceFiles)
@@ -643,7 +649,8 @@ async function resolveClaimableChatUploadRow(
643649
row.userId === userId &&
644650
row.workspaceId === workspaceId &&
645651
row.context === 'mothership' &&
646-
row.deletedAt === null
652+
row.deletedAt === null &&
653+
(row.chatId === null || row.chatId === chatId)
647654
)
648655

649656
if (!owned) {
@@ -686,7 +693,7 @@ export async function trackChatUpload(
686693
throw new WorkspaceFileKeyOwnershipError(s3Key)
687694
}
688695

689-
const claimable = await resolveClaimableChatUploadRow(workspaceId, userId, s3Key)
696+
const claimable = await resolveClaimableChatUploadRow(workspaceId, userId, chatId, s3Key)
690697

691698
if (claimable.kind === 'insert' && hasCloudStorage()) {
692699
// Hygiene only — the format and no-prior-record guards above already carry
@@ -727,7 +734,13 @@ export async function trackChatUpload(
727734
eq(workspaceFiles.userId, userId),
728735
eq(workspaceFiles.workspaceId, workspaceId),
729736
eq(workspaceFiles.context, 'mothership'),
730-
isNull(workspaceFiles.deletedAt)
737+
isNull(workspaceFiles.deletedAt),
738+
// Compare-and-swap on the chat binding: an upload belongs to one
739+
// chat. Two overlapping requests both observe `chat_id IS NULL`,
740+
// but only the first satisfies this predicate — the loser matches
741+
// zero rows and fails closed instead of stealing the binding and
742+
// its delete-cascade lifecycle.
743+
or(isNull(workspaceFiles.chatId), eq(workspaceFiles.chatId, chatId))
731744
)
732745
)
733746
.returning({ id: workspaceFiles.id })

0 commit comments

Comments
 (0)