diff --git a/apps/sim/app/api/mothership/local-files/stage/route.ts b/apps/sim/app/api/mothership/local-files/stage/route.ts index 84dbb7e403f..bf247ddd0f3 100644 --- a/apps/sim/app/api/mothership/local-files/stage/route.ts +++ b/apps/sim/app/api/mothership/local-files/stage/route.ts @@ -13,7 +13,10 @@ import { } from '@/lib/copilot/request/http' import { encodeVfsSegment } from '@/lib/copilot/vfs/path-utils' import { withRouteHandler } from '@/lib/core/utils/with-route-handler' -import { trackChatUpload } from '@/lib/uploads/contexts/workspace/workspace-file-manager' +import { + trackChatUpload, + WorkspaceFileKeyOwnershipError, +} from '@/lib/uploads/contexts/workspace/workspace-file-manager' import { getUserEntityPermissions } from '@/lib/workspaces/permissions/utils' const logger = createLogger('StageLocalFileUploadAPI') @@ -95,6 +98,13 @@ export const POST = withRouteHandler(async (request: NextRequest) => { uploadPath: `uploads/${encodeVfsSegment(displayName)}`, }) } catch (error) { + if (error instanceof WorkspaceFileKeyOwnershipError) { + // The caller supplied a key they may not bind — a client error, not ours. + logger.warn('Rejected chat upload staging for an unowned storage key', { + error: error.message, + }) + return NextResponse.json({ error: 'Storage key is not available' }, { status: 403 }) + } logger.error('Failed to stage local file upload', error) return createInternalServerErrorResponse('Failed to stage local file upload') } diff --git a/apps/sim/lib/copilot/chat/payload.test.ts b/apps/sim/lib/copilot/chat/payload.test.ts index d0a23aa9706..b0f8609527e 100644 --- a/apps/sim/lib/copilot/chat/payload.test.ts +++ b/apps/sim/lib/copilot/chat/payload.test.ts @@ -4,10 +4,12 @@ import { workflowsUtilsMock } from '@sim/testing' import { beforeEach, describe, expect, it, vi } from 'vitest' -const { mockCreateUserToolSchema, mockGetHighestPrioritySubscription } = vi.hoisted(() => ({ - mockCreateUserToolSchema: vi.fn(() => ({ type: 'object', properties: {} })), - mockGetHighestPrioritySubscription: vi.fn(), -})) +const { mockCreateUserToolSchema, mockGetHighestPrioritySubscription, mockTrackChatUpload } = + vi.hoisted(() => ({ + mockCreateUserToolSchema: vi.fn(() => ({ type: 'object', properties: {} })), + mockGetHighestPrioritySubscription: vi.fn(), + mockTrackChatUpload: vi.fn(), + })) vi.mock('@/lib/billing/core/subscription', () => ({ getHighestPrioritySubscription: mockGetHighestPrioritySubscription, @@ -104,6 +106,10 @@ vi.mock('@/tools/params', () => ({ createUserToolSchema: mockCreateUserToolSchema, })) +vi.mock('@/lib/uploads/contexts/workspace/workspace-file-manager', () => ({ + trackChatUpload: mockTrackChatUpload, +})) + import { buildCopilotRequestPayload, buildIntegrationToolSchemas, @@ -209,6 +215,53 @@ describe('buildIntegrationToolSchemas', () => { describe('buildCopilotRequestPayload', () => { beforeEach(() => { vi.clearAllMocks() + mockTrackChatUpload.mockResolvedValue({ displayName: 'payroll.xlsx' }) + }) + + describe('file attachment tracking', () => { + const attachmentParams = { + message: 'hi', + userId: 'mallory', + userMessageId: 'msg-1', + mode: 'agent', + model: 'claude-opus-4-8', + workspaceId: 'ws-1', + chatId: 'chat-1', + fileAttachments: [ + { id: 'a1', key: 'workspace/ws-1/1731000000000-ab12cd34-payroll.xlsx', size: 1 }, + ], + } + + /** + * Tracking writes `workspace_files` rows. A read-only member reaching the + * chat endpoint must not gain that write through an attachment. + */ + it.each(['read', undefined])('does not track attachments for permission %s', async (perm) => { + await buildCopilotRequestPayload( + { ...attachmentParams, userPermission: perm }, + { selectedModel: 'claude-opus-4-8' } + ) + + expect(mockTrackChatUpload).not.toHaveBeenCalled() + }) + + it.each(['write', 'admin'])('tracks attachments for permission %s', async (perm) => { + await buildCopilotRequestPayload( + { ...attachmentParams, userPermission: perm }, + { selectedModel: 'claude-opus-4-8' } + ) + + expect(mockTrackChatUpload).toHaveBeenCalledWith( + 'ws-1', + 'mallory', + 'chat-1', + 'workspace/ws-1/1731000000000-ab12cd34-payroll.xlsx', + expect.anything(), + expect.anything(), + 1, + 'msg-1' + ) + }) }) it('passes workspaceContext through to the Go request payload', async () => { diff --git a/apps/sim/lib/copilot/chat/payload.ts b/apps/sim/lib/copilot/chat/payload.ts index cac3f97f28a..4ba3cf6c37e 100644 --- a/apps/sim/lib/copilot/chat/payload.ts +++ b/apps/sim/lib/copilot/chat/payload.ts @@ -1,5 +1,6 @@ import type { BrowserKnownSession } from '@sim/browser-protocol' import { createLogger } from '@sim/logger' +import { isPermissionType, permissionSatisfies } from '@sim/platform-authz/predicates' import { toError } from '@sim/utils/errors' import { LRUCache } from 'lru-cache' import { getHighestPrioritySubscription } from '@/lib/billing/core/subscription' @@ -333,10 +334,25 @@ export async function buildCopilotRequestPayload( const effectiveMode = mode === 'agent' ? 'build' : mode const transportMode = effectiveMode === 'build' ? 'agent' : effectiveMode - // Track uploaded files in the DB and build context tags instead of base64 inlining + // Track uploaded files in the DB and build context tags instead of base64 inlining. + // Tracking writes `workspace_files` rows, so it needs the same write grant the + // upload routes that issue these keys already require — reaching the chat + // endpoint with `read` must not confer a file-write capability. const uploadContexts: Array<{ type: string; content: string; tag?: string; path?: string }> = [] + // `userPermission` is typed `string` for legacy reasons, so narrow it before + // comparing — an unrecognized value must fail the gate, not rank below it. + const canWriteWorkspaceFiles = + isPermissionType(params.userPermission) && permissionSatisfies(params.userPermission, 'write') if (chatId && params.workspaceId && fileAttachments && fileAttachments.length > 0) { - for (const f of fileAttachments) { + if (!canWriteWorkspaceFiles) { + logger.warn('Dropping chat file attachments without workspace write access', { + chatId, + workspaceId: params.workspaceId, + attachmentCount: fileAttachments.length, + }) + } + const trackableAttachments = canWriteWorkspaceFiles ? fileAttachments : [] + for (const f of trackableAttachments) { const filename = (f.filename ?? f.name ?? 'file') as string const mediaType = (f.media_type ?? f.mimeType ?? 'application/octet-stream') as string try { diff --git a/apps/sim/lib/uploads/contexts/workspace/track-chat-upload.test.ts b/apps/sim/lib/uploads/contexts/workspace/track-chat-upload.test.ts index ca103139be3..d1c84d65fd2 100644 --- a/apps/sim/lib/uploads/contexts/workspace/track-chat-upload.test.ts +++ b/apps/sim/lib/uploads/contexts/workspace/track-chat-upload.test.ts @@ -2,7 +2,7 @@ * @vitest-environment node */ -import { dbChainMockFns, resetDbChainMock } from '@sim/testing' +import { dbChainMockFns, queueTableRows, resetDbChainMock, schemaMock } from '@sim/testing' import { beforeEach, describe, expect, it, vi } from 'vitest' const { @@ -10,11 +10,15 @@ const { mockDecrementStorageUsageForBillingContext, mockIncrementStorageUsageForBillingContext, mockResolveStorageBillingContext, + mockHasCloudStorage, + mockHeadObject, } = vi.hoisted(() => ({ mockCheckStorageQuotaForBillingContext: vi.fn(), mockDecrementStorageUsageForBillingContext: vi.fn(), mockIncrementStorageUsageForBillingContext: vi.fn(), mockResolveStorageBillingContext: vi.fn(), + mockHasCloudStorage: vi.fn(), + mockHeadObject: vi.fn(), })) vi.mock('@/lib/billing/storage', () => ({ @@ -24,12 +28,40 @@ vi.mock('@/lib/billing/storage', () => ({ resolveStorageBillingContext: mockResolveStorageBillingContext, })) +vi.mock('@/lib/uploads/core/storage-service', () => ({ + deleteFile: vi.fn(), + downloadFile: vi.fn(), + hasCloudStorage: mockHasCloudStorage, + headObject: mockHeadObject, + uploadFile: vi.fn(), +})) + import { CHAT_DISPLAY_NAME_INDEX, suffixedName, trackChatUpload } from './workspace-file-manager' const CHAT_ID = '11111111-1111-1111-1111-111111111111' -const WORKSPACE_ID = 'ws_1' +const WORKSPACE_ID = '22222222-2222-2222-2222-222222222222' +const OTHER_WORKSPACE_ID = '33333333-3333-3333-3333-333333333333' const USER_ID = 'user_1' -const S3_KEY = 'mothership/abc/123-image.png' +const OTHER_USER_ID = 'user_2' +const S3_KEY = `workspace/${WORKSPACE_ID}/1731000000000-ab12cd34-image.png` + +/** Row shape `resolveClaimableChatUploadRow` selects, with claimable defaults. */ +function existingRow(overrides: Record = {}) { + return { + id: 'wf_existing', + userId: USER_ID, + workspaceId: WORKSPACE_ID, + context: 'mothership', + chatId: null, + deletedAt: null, + ...overrides, + } +} + +/** Queue the key-ownership lookup that runs before every bind. */ +function queueOwnershipLookup(rows: unknown[]): void { + queueTableRows(schemaMock.workspaceFiles, rows) +} function expectNoWorkspaceStorageAccounting(): void { expect(mockCheckStorageQuotaForBillingContext).not.toHaveBeenCalled() @@ -69,9 +101,12 @@ describe('trackChatUpload', () => { beforeEach(() => { vi.clearAllMocks() resetDbChainMock() + mockHasCloudStorage.mockReturnValue(true) + mockHeadObject.mockResolvedValue({ size: 1024 }) }) it('finalizes an existing direct upload without workspace storage accounting', async () => { + queueOwnershipLookup([existingRow()]) dbChainMockFns.returning.mockResolvedValueOnce([{ id: 'wf_existing' }]) const result = await trackChatUpload( @@ -96,7 +131,7 @@ describe('trackChatUpload', () => { }) it('finalizes a presigned upload without workspace storage accounting', async () => { - dbChainMockFns.returning.mockResolvedValueOnce([]) + queueOwnershipLookup([]) const result = await trackChatUpload( WORKSPACE_ID, @@ -122,6 +157,7 @@ describe('trackChatUpload', () => { }) it('stamps message_id on the UPDATE arm when the birth message is known', async () => { + queueOwnershipLookup([existingRow()]) dbChainMockFns.returning.mockResolvedValueOnce([{ id: 'wf_existing' }]) await trackChatUpload( @@ -141,7 +177,7 @@ describe('trackChatUpload', () => { }) it('stamps message_id on the fallback INSERT arm and nulls it when omitted', async () => { - dbChainMockFns.returning.mockResolvedValueOnce([]) + queueOwnershipLookup([]) await trackChatUpload( WORKSPACE_ID, @@ -159,6 +195,7 @@ describe('trackChatUpload', () => { ) // Legacy callers without a message id write an explicit NULL ("birth unknown"). + queueOwnershipLookup([existingRow()]) dbChainMockFns.returning.mockResolvedValueOnce([{ id: 'wf_existing' }]) await trackChatUpload(WORKSPACE_ID, USER_ID, CHAT_ID, S3_KEY, 'image.png', 'image/png', 1024) expect(dbChainMockFns.set).toHaveBeenLastCalledWith( @@ -173,9 +210,8 @@ describe('trackChatUpload', () => { constraint_name: CHAT_DISPLAY_NAME_INDEX, }) - dbChainMockFns.returning.mockResolvedValueOnce([]) + queueOwnershipLookup([]) dbChainMockFns.values.mockRejectedValueOnce(displayNameCollision) - dbChainMockFns.returning.mockResolvedValueOnce([]) dbChainMockFns.values.mockResolvedValueOnce(undefined) const result = await trackChatUpload( @@ -204,7 +240,7 @@ describe('trackChatUpload', () => { constraint_name: 'workspace_files_key_active_unique', }) - dbChainMockFns.returning.mockResolvedValueOnce([]) + queueOwnershipLookup([]) dbChainMockFns.values.mockRejectedValueOnce(keyCollision) await expect( @@ -216,6 +252,7 @@ describe('trackChatUpload', () => { }) it('rethrows metadata errors without workspace storage accounting', async () => { + queueOwnershipLookup([existingRow()]) dbChainMockFns.returning.mockRejectedValueOnce(new Error('connection lost')) await expect( @@ -224,4 +261,239 @@ describe('trackChatUpload', () => { expectNoWorkspaceStorageAccounting() }) + + describe('storage key ownership', () => { + /** + * A caller-supplied key that resolves to a different workspace must never + * reach the binding write — that is cross-tenant key smuggling. + */ + it('rejects a key addressed to another workspace', async () => { + const foreignKey = `workspace/${OTHER_WORKSPACE_ID}/1731000000000-ab12cd34-image.png` + + await expect( + trackChatUpload(WORKSPACE_ID, USER_ID, CHAT_ID, foreignKey, 'image.png', 'image/png', 1024) + ).rejects.toThrow('not available for a chat attachment') + + expect(dbChainMockFns.set).not.toHaveBeenCalled() + expect(dbChainMockFns.values).not.toHaveBeenCalled() + }) + + it('rejects a key with no workspace prefix at all', async () => { + await expect( + trackChatUpload( + WORKSPACE_ID, + USER_ID, + CHAT_ID, + 'mothership/abc/123-image.png', + 'image.png', + 'image/png', + 1024 + ) + ).rejects.toThrow('not available for a chat attachment') + + expect(dbChainMockFns.set).not.toHaveBeenCalled() + expect(dbChainMockFns.values).not.toHaveBeenCalled() + }) + + /** + * The reported attack: a member hands in another member's workspace-file key + * and the UPDATE re-parents that row to the caller's chat, hiding it from the + * workspace Files listing and exposing it to the chat-delete FK cascade. + */ + it("refuses to re-parent another member's workspace file", async () => { + queueOwnershipLookup([ + existingRow({ id: 'wf_victim', userId: OTHER_USER_ID, context: 'workspace' }), + ]) + + await expect( + trackChatUpload(WORKSPACE_ID, USER_ID, CHAT_ID, S3_KEY, 'image.png', 'image/png', 1024) + ).rejects.toThrow('not available for a chat attachment') + + expect(dbChainMockFns.set).not.toHaveBeenCalled() + expect(dbChainMockFns.values).not.toHaveBeenCalled() + }) + + it("refuses to re-parent another member's chat upload", async () => { + queueOwnershipLookup([existingRow({ id: 'wf_victim', userId: OTHER_USER_ID })]) + + await expect( + trackChatUpload(WORKSPACE_ID, USER_ID, CHAT_ID, S3_KEY, 'image.png', 'image/png', 1024) + ).rejects.toThrow('not available for a chat attachment') + + expect(dbChainMockFns.set).not.toHaveBeenCalled() + expect(dbChainMockFns.values).not.toHaveBeenCalled() + }) + + /** The caller's own workspace file is still a Files-tab file, not a chat upload. */ + it("refuses to convert the caller's own workspace file into a chat upload", async () => { + queueOwnershipLookup([existingRow({ context: 'workspace' })]) + + await expect( + trackChatUpload(WORKSPACE_ID, USER_ID, CHAT_ID, S3_KEY, 'image.png', 'image/png', 1024) + ).rejects.toThrow('not available for a chat attachment') + + expect(dbChainMockFns.set).not.toHaveBeenCalled() + expect(dbChainMockFns.values).not.toHaveBeenCalled() + }) + + /** + * The active-key unique index is partial on `deleted_at IS NULL`, so an + * INSERT over an archived row would succeed and mint a binding granting read + * access to the archived file's bytes. + */ + it('refuses to mint a binding over a soft-deleted record for the same key', async () => { + queueOwnershipLookup([ + existingRow({ id: 'wf_archived', deletedAt: new Date('2026-01-01T00:00:00Z') }), + ]) + + await expect( + trackChatUpload(WORKSPACE_ID, USER_ID, CHAT_ID, S3_KEY, 'image.png', 'image/png', 1024) + ).rejects.toThrow('not available for a chat attachment') + + expect(dbChainMockFns.values).not.toHaveBeenCalled() + }) + + it('scopes the UPDATE to the caller-owned row id rather than the raw key', async () => { + queueOwnershipLookup([existingRow({ id: 'wf_mine' })]) + dbChainMockFns.returning.mockResolvedValueOnce([{ id: 'wf_mine' }]) + + await trackChatUpload(WORKSPACE_ID, USER_ID, CHAT_ID, S3_KEY, 'image.png', 'image/png', 1024) + + expect(dbChainMockFns.where).toHaveBeenCalled() + expect(dbChainMockFns.set).toHaveBeenCalledWith( + expect.objectContaining({ chatId: CHAT_ID, context: 'mothership' }) + ) + }) + + /** The row vanished between the ownership check and the write — fail closed. */ + it('fails closed when the owned row disappears before the update lands', async () => { + queueOwnershipLookup([existingRow({ id: 'wf_mine' })]) + dbChainMockFns.returning.mockResolvedValueOnce([]) + + await expect( + trackChatUpload(WORKSPACE_ID, USER_ID, CHAT_ID, S3_KEY, 'image.png', 'image/png', 1024) + ).rejects.toThrow('not available for a chat attachment') + + expect(dbChainMockFns.values).not.toHaveBeenCalled() + }) + + /** + * The ownership lookup and the write are separate statements, so a + * concurrent `materialize_file` can flip the row to context='workspace' + * in between. The UPDATE must re-assert every ownership predicate rather + * than matching on the captured row id alone, or it would drag a saved + * workspace file back into chat scope. + */ + it('re-asserts every ownership predicate in the update, not just the row id', async () => { + queueOwnershipLookup([existingRow({ id: 'wf_mine' })]) + dbChainMockFns.returning.mockResolvedValueOnce([{ id: 'wf_mine' }]) + + await trackChatUpload(WORKSPACE_ID, USER_ID, CHAT_ID, S3_KEY, 'image.png', 'image/png', 1024) + + expect(dbChainMockFns.where.mock.calls.at(-1)?.[0]).toEqual({ + type: 'and', + conditions: [ + { type: 'eq', left: 'id', right: 'wf_mine' }, + { type: 'eq', left: 'userId', right: USER_ID }, + { type: 'eq', left: 'workspaceId', right: WORKSPACE_ID }, + { type: 'eq', left: 'context', right: 'mothership' }, + { type: 'isNull', column: 'deletedAt' }, + { + type: 'or', + conditions: [ + { type: 'isNull', column: 'chatId' }, + { type: 'eq', left: 'chatId', right: CHAT_ID }, + ], + }, + ], + }) + }) + + /** + * An upload binds to exactly one chat. Matches the 409 the sibling + * `local-files/stage` route already returns for this case. + */ + it('refuses to relink an upload already bound to a different chat', async () => { + queueOwnershipLookup([existingRow({ chatId: 'other-chat-id' })]) + + await expect( + trackChatUpload(WORKSPACE_ID, USER_ID, CHAT_ID, S3_KEY, 'image.png', 'image/png', 1024) + ).rejects.toThrow('not available for a chat attachment') + + expect(dbChainMockFns.set).not.toHaveBeenCalled() + expect(dbChainMockFns.values).not.toHaveBeenCalled() + }) + + it('still re-links an upload already bound to this same chat', async () => { + queueOwnershipLookup([existingRow({ chatId: CHAT_ID })]) + dbChainMockFns.returning.mockResolvedValueOnce([{ id: 'wf_existing' }]) + + const result = await trackChatUpload( + WORKSPACE_ID, + USER_ID, + CHAT_ID, + S3_KEY, + 'image.png', + 'image/png', + 1024 + ) + + expect(result).toEqual({ displayName: 'image.png' }) + }) + + it('requires the object to exist in storage before minting a new binding', async () => { + queueOwnershipLookup([]) + mockHeadObject.mockResolvedValueOnce(null) + + await expect( + trackChatUpload(WORKSPACE_ID, USER_ID, CHAT_ID, S3_KEY, 'image.png', 'image/png', 1024) + ).rejects.toThrow('not available for a chat attachment') + + expect(dbChainMockFns.values).not.toHaveBeenCalled() + }) + + /** + * The probe is hygiene, not authorization — a provider 5xx must not drop a + * legitimate >50MB multipart upload, which is the only path that reaches it. + * Only a definitive not-found (`null`) rejects. + */ + it('proceeds when the storage existence probe throws a transient error', async () => { + queueOwnershipLookup([]) + mockHeadObject.mockRejectedValueOnce(new Error('503 SlowDown')) + + const result = await trackChatUpload( + WORKSPACE_ID, + USER_ID, + CHAT_ID, + S3_KEY, + 'image.png', + 'image/png', + 1024 + ) + + expect(result).toEqual({ displayName: 'image.png' }) + expect(dbChainMockFns.values).toHaveBeenCalledWith( + expect.objectContaining({ key: S3_KEY, context: 'mothership' }) + ) + }) + + /** Local-storage deployments have no headObject to consult; ownership still gates. */ + it('skips the storage existence probe when cloud storage is not configured', async () => { + mockHasCloudStorage.mockReturnValue(false) + queueOwnershipLookup([]) + + const result = await trackChatUpload( + WORKSPACE_ID, + USER_ID, + CHAT_ID, + S3_KEY, + 'image.png', + 'image/png', + 1024 + ) + + expect(result).toEqual({ displayName: 'image.png' }) + expect(mockHeadObject).not.toHaveBeenCalled() + }) + }) }) diff --git a/apps/sim/lib/uploads/contexts/workspace/workspace-file-manager.ts b/apps/sim/lib/uploads/contexts/workspace/workspace-file-manager.ts index c0dbbbecce9..a40d1f64634 100644 --- a/apps/sim/lib/uploads/contexts/workspace/workspace-file-manager.ts +++ b/apps/sim/lib/uploads/contexts/workspace/workspace-file-manager.ts @@ -9,7 +9,7 @@ import { workspaceFiles } from '@sim/db/schema' import { createLogger } from '@sim/logger' import { getErrorMessage, getPostgresConstraintName, getPostgresErrorCode } from '@sim/utils/errors' import { generateShortId } from '@sim/utils/id' -import { and, eq, isNotNull, isNull, sql } from 'drizzle-orm' +import { and, eq, isNotNull, isNull, or, sql } from 'drizzle-orm' import type { ShareRecord } from '@/lib/api/contracts/public-shares' import { decrementStorageUsageForBillingContextInTx, @@ -590,12 +590,91 @@ const MAX_CHAT_DISPLAY_NAME_RETRIES = 1000 /** Postgres constraint name for the partial unique index on `(chat_id, display_name)`. */ export const CHAT_DISPLAY_NAME_INDEX = 'workspace_files_chat_display_name_unique' +/** + * Raised when a caller-supplied storage key may not be bound to a chat upload — + * it addresses another workspace, or it already has a `workspace_files` record + * the caller does not own as an active chat upload. + */ +export class WorkspaceFileKeyOwnershipError extends Error { + readonly code = 'KEY_NOT_OWNED' as const + constructor(key: string) { + super(`Storage key is not available for a chat attachment: ${key}`) + } +} + +type ClaimableChatUploadRow = { kind: 'update'; id: string } | { kind: 'insert' } + +/** + * Decide how `trackChatUpload` may bind `s3Key`, or reject the key outright. + * + * Only two outcomes are safe. Either the caller already owns an active + * chat-upload row for the key (re-linking their own upload to a chat), or the + * key has no `workspace_files` record whatsoever and a fresh binding can be + * minted. Anything else — another member's row, a `context='workspace'` file, + * or a soft-deleted record whose object is still readable through the binding — + * belongs to somebody else's file and must not be touched. + * + * Soft-deleted rows count: the active-key unique index is partial on + * `deleted_at IS NULL`, so inserting over an archived row would succeed and + * hand the caller read access to the archived file's bytes. + * + * An upload also binds to exactly one chat: a row already linked to a different + * chat is not claimable, matching the 409 the sibling `local-files/stage` route + * returns for the same case. Re-sending the key within its own chat still works. + */ +async function resolveClaimableChatUploadRow( + workspaceId: string, + userId: string, + chatId: string, + s3Key: string +): Promise { + const rows = await db + .select({ + id: workspaceFiles.id, + userId: workspaceFiles.userId, + workspaceId: workspaceFiles.workspaceId, + context: workspaceFiles.context, + chatId: workspaceFiles.chatId, + deletedAt: workspaceFiles.deletedAt, + }) + .from(workspaceFiles) + .where(eq(workspaceFiles.key, s3Key)) + + if (rows.length === 0) { + return { kind: 'insert' } + } + + const owned = rows.find( + (row) => + row.userId === userId && + row.workspaceId === workspaceId && + row.context === 'mothership' && + row.deletedAt === null && + (row.chatId === null || row.chatId === chatId) + ) + + if (!owned) { + throw new WorkspaceFileKeyOwnershipError(s3Key) + } + + return { kind: 'update', id: owned.id } +} + /** * Track a file that was already uploaded to workspace S3 as a chat-scoped upload. * Links the existing workspaceFiles metadata record (created by the storage service * during upload) to the chat by setting chatId and context='mothership'. * Falls back to inserting a new record if none exists for the key. * + * `s3Key` reaches this function from client-supplied request bodies, and + * `workspace_files.key` is the trusted binding every file authorization check + * resolves the owning workspace from. So the key is treated as untrusted here: + * it must address the target workspace, it may only re-link a chat-upload row + * the caller already owns, and minting a brand-new binding requires the key to + * have no prior record at all. Without those invariants a member could hand in + * another member's key and re-parent their file (hiding it from the workspace + * Files listing, or destroying it through the chat-delete FK cascade). + * * Allocates a collision-free `displayName` (the partial unique index on * (chat_id, display_name) WHERE context='mothership' enforces this) and returns it * so callers can surface the same name to the model in the VFS read hint. @@ -610,27 +689,72 @@ export async function trackChatUpload( size: number, messageId?: string ): Promise<{ displayName: string }> { + if (parseWorkspaceFileKey(s3Key) !== workspaceId) { + throw new WorkspaceFileKeyOwnershipError(s3Key) + } + + const claimable = await resolveClaimableChatUploadRow(workspaceId, userId, chatId, s3Key) + + if (claimable.kind === 'insert' && hasCloudStorage()) { + // Hygiene only — the format and no-prior-record guards above already carry + // authorization, and a binding to a nonexistent object grants nothing + // readable. So reject only on a definitive not-found (`null`); a provider + // 5xx/throttle throws, and failing the attachment on that would drop a + // legitimate >50MB multipart upload (the sole path reaching this branch). + let head: Awaited> = null + try { + head = await headObject(s3Key, 'workspace') + } catch (error) { + logger.warn('Chat upload existence probe failed; proceeding on the ownership guards', { + key: s3Key, + error: getErrorMessage(error), + }) + head = { size } + } + if (!head) { + throw new WorkspaceFileKeyOwnershipError(s3Key) + } + } + for (let n = 1; n <= MAX_CHAT_DISPLAY_NAME_RETRIES; n++) { const candidate = suffixedName(fileName, n) try { - const updated = await db - .update(workspaceFiles) - .set({ - chatId, - messageId: messageId ?? null, - context: 'mothership', - displayName: candidate, - }) - .where( - and( - eq(workspaceFiles.key, s3Key), - eq(workspaceFiles.workspaceId, workspaceId), - isNull(workspaceFiles.deletedAt) + if (claimable.kind === 'update') { + const updated = await db + .update(workspaceFiles) + .set({ + chatId, + messageId: messageId ?? null, + context: 'mothership', + displayName: candidate, + }) + .where( + and( + eq(workspaceFiles.id, claimable.id), + eq(workspaceFiles.userId, userId), + eq(workspaceFiles.workspaceId, workspaceId), + eq(workspaceFiles.context, 'mothership'), + isNull(workspaceFiles.deletedAt), + // Compare-and-swap on the chat binding: an upload belongs to one + // chat. Two overlapping requests both observe `chat_id IS NULL`, + // but only the first satisfies this predicate — the loser matches + // zero rows and fails closed instead of stealing the binding and + // its delete-cascade lifecycle. + or(isNull(workspaceFiles.chatId), eq(workspaceFiles.chatId, chatId)) + ) ) - ) - .returning({ id: workspaceFiles.id }) + .returning({ id: workspaceFiles.id }) + + if (updated.length === 0) { + // The ownership lookup is a separate statement, so re-assert every + // predicate here — this UPDATE is the atomic check. A concurrent + // `materialize_file` flips the same row to context='workspace' and + // clears chatId; matching on id alone would drag that saved file back + // into chat scope, hiding it from the Files listing and re-exposing it + // to the chat-delete cascade. + throw new WorkspaceFileKeyOwnershipError(s3Key) + } - if (updated.length > 0) { logger.info( `Linked existing file record to chat: ${fileName} (display: ${candidate}) for chat ${chatId}` )