diff --git a/apps/sim/app/api/files/authorization.ts b/apps/sim/app/api/files/authorization.ts index 819081ff2e0..64d3ee4650a 100644 --- a/apps/sim/app/api/files/authorization.ts +++ b/apps/sim/app/api/files/authorization.ts @@ -30,6 +30,13 @@ interface AuthorizationResult { type WorkspacePermission = 'read' | 'write' | 'admin' +/** + * The two contexts stored under a `workspace/…` key. They share a bucket and a + * workspace-membership permission model; only the owning module differs — a + * mothership attachment belongs to a chat, a workspace file to the Files module. + */ +type WorkspaceScopedContext = 'workspace' | 'mothership' + /** * Whether a resolved workspace permission satisfies a file operation. Read and * download paths accept any membership; destructive operations (`requireWrite`) @@ -49,12 +56,12 @@ function workspacePermissionSatisfies( */ async function lookupWorkspaceFileByKey( key: string, - options?: { includeDeleted?: boolean } + options?: { includeDeleted?: boolean; context?: WorkspaceScopedContext } ): Promise<{ workspaceId: string; uploadedBy: string } | null> { try { - const { includeDeleted = false } = options ?? {} + const { includeDeleted = false, context = 'workspace' } = options ?? {} // Priority 1: Check new workspaceFiles table - const fileRecord = await getFileMetadataByKey(key, 'workspace', { includeDeleted }) + const fileRecord = await getFileMetadataByKey(key, context, { includeDeleted }) if (fileRecord) { return { @@ -158,7 +165,14 @@ export async function verifyFileAccess( // 1. Workspace / mothership files: Check database first (most reliable for both local and cloud) if (inferredContext === 'workspace' || inferredContext === 'mothership') { - return await verifyWorkspaceFileAccess(cloudKey, userId, customConfig, isLocal, requireWrite) + return await verifyWorkspaceFileAccess( + cloudKey, + userId, + customConfig, + isLocal, + requireWrite, + inferredContext + ) } // 2. Execution files: workspace_id/workflow_id/execution_id/filename @@ -200,10 +214,11 @@ async function verifyWorkspaceFileAccess( userId: string, customConfig?: StorageConfig, isLocal?: boolean, - requireWrite = false + requireWrite = false, + context: WorkspaceScopedContext = 'workspace' ): Promise { try { - const anyWorkspaceFileRecord = await getFileMetadataByKey(cloudKey, 'workspace', { + const anyWorkspaceFileRecord = await getFileMetadataByKey(cloudKey, context, { includeDeleted: true, }) if (anyWorkspaceFileRecord?.deletedAt) { @@ -215,7 +230,7 @@ async function verifyWorkspaceFileAccess( } // Priority 1: Check database (most reliable, works for both local and cloud) - const workspaceFileRecord = await lookupWorkspaceFileByKey(cloudKey) + const workspaceFileRecord = await lookupWorkspaceFileByKey(cloudKey, { context }) if (workspaceFileRecord) { const permission = await getUserEntityPermissions( userId, diff --git a/apps/sim/app/api/files/serve/[...path]/route.test.ts b/apps/sim/app/api/files/serve/[...path]/route.test.ts index 799fb19b97c..27a8d39ce37 100644 --- a/apps/sim/app/api/files/serve/[...path]/route.test.ts +++ b/apps/sim/app/api/files/serve/[...path]/route.test.ts @@ -20,6 +20,7 @@ const { mockIsUsingCloudStorage, mockDownloadCopilotFile, mockInferContextFromKey, + mockResolveStoredFileContext, mockParseWorkspaceFileKey, mockAuthenticateWorkspaceFile, mockReadWorkspaceFileContentByKey, @@ -44,6 +45,7 @@ const { mockIsUsingCloudStorage: vi.fn(), mockDownloadCopilotFile: vi.fn(), mockInferContextFromKey: vi.fn(), + mockResolveStoredFileContext: vi.fn(), mockParseWorkspaceFileKey: vi.fn(), mockAuthenticateWorkspaceFile: vi.fn(), mockReadWorkspaceFileContentByKey: vi.fn(), @@ -79,6 +81,10 @@ vi.mock('@/lib/uploads/utils/file-utils', () => ({ inferContextFromKey: mockInferContextFromKey, })) +vi.mock('@/lib/uploads/server/metadata', () => ({ + resolveStoredFileContext: mockResolveStoredFileContext, +})) + vi.mock('@/lib/uploads/setup.server', () => ({})) vi.mock('@/lib/execution/sandbox/run-task', () => ({ @@ -129,7 +135,11 @@ describe('File Serve API Route', () => { mockReadFile.mockResolvedValue(Buffer.from('test content')) mockIsUsingCloudStorage.mockReturnValue(false) storageServiceMockFns.mockHasCloudStorage.mockReturnValue(true) - mockInferContextFromKey.mockReturnValue('mothership') + // A `workspace/…` key is what both a workspace file and a mothership chat + // attachment carry; only the stored binding tells them apart, so the default + // here is the attachment and the workspace cases opt in explicitly. + mockInferContextFromKey.mockReturnValue('workspace') + mockResolveStoredFileContext.mockResolvedValue('mothership') mockParseWorkspaceFileKey.mockReturnValue(undefined) mockAuthenticateWorkspaceFile.mockResolvedValue({ kind: 'session', @@ -240,7 +250,7 @@ describe('File Serve API Route', () => { workflowId: 'workflow-1', }, } - mockInferContextFromKey.mockReturnValue('workspace') + mockResolveStoredFileContext.mockResolvedValue('workspace') mockParseWorkspaceFileKey.mockReturnValue('test-workspace-id') mockAuthenticateWorkspaceFile.mockResolvedValue(principal) mockResolveServableDocBytes.mockResolvedValue({ @@ -276,6 +286,41 @@ describe('File Serve API Route', () => { expect(mockVerifyFileAccess).not.toHaveBeenCalled() }) + it('serves a mothership chat attachment stored under a workspace key', async () => { + /** + * The attachment shares the `workspace/…` prefix but is recorded as + * `context = 'mothership'`, so the workspace-file use case — which matches on + * `context = 'workspace'` — would answer 404 for a file that is right there. + */ + mockIsUsingCloudStorage.mockReturnValue(true) + storageServiceMockFns.mockDownloadFile.mockResolvedValue(Buffer.from('attachment bytes')) + mockGetContentType.mockReturnValue('image/png') + + const req = new NextRequest( + 'http://localhost:3000/api/files/serve/workspace/test-workspace-id/1234567890-photo.png?preview=1' + ) + const response = await GET(req, { + params: Promise.resolve({ + path: ['workspace', 'test-workspace-id', '1234567890-photo.png'], + }), + }) + + expect(response.status).toBe(200) + expect(mockReadWorkspaceFileContentByKey).not.toHaveBeenCalled() + expect(mockAuthenticateWorkspaceFile).not.toHaveBeenCalled() + expect(mockVerifyFileAccess).toHaveBeenCalledWith( + 'workspace/test-workspace-id/1234567890-photo.png', + 'test-user-id', + undefined, + 'mothership', + false + ) + expect(storageServiceMockFns.mockDownloadFile).toHaveBeenCalledWith({ + key: 'workspace/test-workspace-id/1234567890-photo.png', + context: 'mothership', + }) + }) + it('should return 404 when file not found', async () => { mockVerifyFileAccess.mockResolvedValue(false) mockFindLocalFile.mockReturnValue(null) diff --git a/apps/sim/app/api/files/serve/[...path]/route.ts b/apps/sim/app/api/files/serve/[...path]/route.ts index 0899cdd0dfa..a36b814fb16 100644 --- a/apps/sim/app/api/files/serve/[...path]/route.ts +++ b/apps/sim/app/api/files/serve/[...path]/route.ts @@ -18,6 +18,7 @@ import type { StorageContext } from '@/lib/uploads/config' import { parseWorkspaceFileKey } from '@/lib/uploads/contexts/workspace/workspace-file-manager' import { downloadFile } from '@/lib/uploads/core/storage-service' import { resolveServableImageBytes } from '@/lib/uploads/server/image-derivative' +import { resolveStoredFileContext } from '@/lib/uploads/server/metadata' import { inferContextFromKey } from '@/lib/uploads/utils/file-utils' import { internalWorkspaceFileServeAuth } from '@/lib/workspace-files/api' import { readWorkspaceFileContentByKey } from '@/lib/workspace-files/application/read-workspace-file-content-by-key' @@ -165,7 +166,10 @@ export const GET = withRouteHandler( return await handleLocalFilePublic(fullPath) } - const storageContext = inferContextFromKey(cloudKey) + // Resolved from the key's stored binding, not its prefix alone: a mothership chat + // attachment carries a `workspace/…` key but is not a workspace file, and the + // workspace-file use case below would resolve it to a 404. + const storageContext = await resolveStoredFileContext(cloudKey) const workspacePrincipal = storageContext === 'workspace' ? await internalWorkspaceFileServeAuth.authenticate(request, { path }) @@ -201,10 +205,10 @@ export const GET = withRouteHandler( if (!userId) throw new Error('Authenticated file serve request is missing a user ID') if (isUsingCloudStorage()) { - return await handleCloudProxy(cloudKey, userId, options, request.signal) + return await handleCloudProxy(cloudKey, userId, options, request.signal, storageContext) } - return await handleLocalFile(cloudKey, userId, options, request.signal) + return await handleLocalFile(cloudKey, userId, options, request.signal, storageContext) } catch (error) { if (error instanceof InternalUnauthenticatedError) { logger.warn('Unauthorized file access attempt', { error: error.message }) @@ -285,19 +289,16 @@ async function handleLocalFile( filename: string, userId: string, options: ServeOptions, - signal: AbortSignal | undefined + signal: AbortSignal | undefined, + context: StorageContext ): Promise { const ownerKey = `user:${userId}` try { - const contextParam: StorageContext | undefined = inferContextFromKey(filename) as - | StorageContext - | undefined - const hasAccess = await verifyFileAccess( filename, userId, undefined, // customConfig - contextParam, // context + context, true // isLocal ) @@ -332,7 +333,7 @@ async function handleLocalFile( buffer: fileBuffer, contentType, filename: displayName, - cacheControl: resolveServeCacheControl(options.versioned, contextParam), + cacheControl: resolveServeCacheControl(options.versioned, context), }) } catch (error) { logServeFailure('Error reading local file:', error) @@ -344,12 +345,12 @@ async function handleCloudProxy( cloudKey: string, userId: string, options: ServeOptions, - signal: AbortSignal | undefined + signal: AbortSignal | undefined, + context: StorageContext ): Promise { const ownerKey = `user:${userId}` try { - const context = inferContextFromKey(cloudKey) - logger.info(`Inferred context: ${context} from key pattern: ${cloudKey}`) + logger.info(`Resolved context: ${context} for key: ${cloudKey}`) const hasAccess = await verifyFileAccess( cloudKey, diff --git a/apps/sim/lib/uploads/server/metadata.test.ts b/apps/sim/lib/uploads/server/metadata.test.ts index 19512aa03f9..585adba9715 100644 --- a/apps/sim/lib/uploads/server/metadata.test.ts +++ b/apps/sim/lib/uploads/server/metadata.test.ts @@ -18,6 +18,7 @@ import { insertFileMetadataMany, insertImmutableFileMetadata, recordKnowledgeBaseFileOwnership, + resolveStoredFileContext, } from '@/lib/uploads/server/metadata' describe('recordKnowledgeBaseFileOwnership', () => { @@ -318,3 +319,38 @@ describe('insertFileMetadataMany active-key idempotence', () => { expect(dbChainMockFns.insert).not.toHaveBeenCalled() }) }) + +describe('resolveStoredFileContext', () => { + beforeEach(() => { + vi.clearAllMocks() + resetDbChainMock() + }) + + const workspaceKey = 'workspace/workspace-1/1234567890-abcdef-photo.png' + + it('reports a mothership attachment stored under a workspace key', async () => { + queueTableRows(workspaceFiles, [ + { id: 'file-1', key: workspaceKey, context: 'mothership', deletedAt: null }, + ]) + + await expect(resolveStoredFileContext(workspaceKey)).resolves.toBe('mothership') + }) + + it('keeps a workspace file on the workspace context', async () => { + queueTableRows(workspaceFiles, [ + { id: 'file-1', key: workspaceKey, context: 'workspace', deletedAt: null }, + ]) + + await expect(resolveStoredFileContext(workspaceKey)).resolves.toBe('workspace') + }) + + it('falls back to the inferred context for an unbound key', async () => { + await expect(resolveStoredFileContext(workspaceKey)).resolves.toBe('workspace') + }) + + it('trusts the prefix without a lookup when it cannot be a workspace key', async () => { + await expect(resolveStoredFileContext('copilot/file.png')).resolves.toBe('copilot') + + expect(dbChainMockFns.select).not.toHaveBeenCalled() + }) +}) diff --git a/apps/sim/lib/uploads/server/metadata.ts b/apps/sim/lib/uploads/server/metadata.ts index ec6e64c2204..4485a9e0661 100644 --- a/apps/sim/lib/uploads/server/metadata.ts +++ b/apps/sim/lib/uploads/server/metadata.ts @@ -4,6 +4,7 @@ import { createLogger } from '@sim/logger' import { generateId } from '@sim/utils/id' import { and, eq, inArray, isNotNull, isNull, sql } from 'drizzle-orm' import type { DbOrTx, DbTransaction } from '@/lib/db/types' +import { inferContextFromKey } from '@/lib/uploads/utils/file-utils' import { type StorageContext, toLegacyWorkspaceFileSize } from '../shared/types' const logger = createLogger('FileMetadata') @@ -336,6 +337,28 @@ export async function getFileMetadataByKey( return record ?? null } +/** + * Resolve the storage context a stored object must be read and authorized under. + * + * A `workspace/…` key prefix is not by itself proof of a workspace file. A + * mothership chat attachment is minted with the same prefix — same bucket, same + * workspace scope — but is recorded as `context = 'mothership'` and never enters + * the Files module, so every workspace-file lookup (which matches on + * `context = 'workspace'`) resolves it to nothing. The row bound to the key is + * the only thing that separates the two, and it is server-authored at upload + * time, so it is as trustworthy as the prefix itself. + * + * An unbound key keeps its inferred context: absent metadata is not evidence of + * an attachment, and the caller's own not-found handling is the right answer. + */ +export async function resolveStoredFileContext(key: string): Promise { + const inferred = inferContextFromKey(key) + if (inferred !== 'workspace') return inferred + + const metadata = await getFileMetadataByKey(key) + return metadata?.context === 'mothership' ? 'mothership' : inferred +} + /** * Get active (non-deleted) file metadata for multiple keys in a single query. * Batches what would otherwise be N `getFileMetadataByKey` calls.