Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 22 additions & 7 deletions apps/sim/app/api/files/authorization.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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`)
Expand All @@ -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 {
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -200,10 +214,11 @@ async function verifyWorkspaceFileAccess(
userId: string,
customConfig?: StorageConfig,
isLocal?: boolean,
requireWrite = false
requireWrite = false,
context: WorkspaceScopedContext = 'workspace'
): Promise<boolean> {
try {
const anyWorkspaceFileRecord = await getFileMetadataByKey(cloudKey, 'workspace', {
const anyWorkspaceFileRecord = await getFileMetadataByKey(cloudKey, context, {
includeDeleted: true,
})
if (anyWorkspaceFileRecord?.deletedAt) {
Expand All @@ -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,
Expand Down
49 changes: 47 additions & 2 deletions apps/sim/app/api/files/serve/[...path]/route.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ const {
mockIsUsingCloudStorage,
mockDownloadCopilotFile,
mockInferContextFromKey,
mockResolveStoredFileContext,
mockParseWorkspaceFileKey,
mockAuthenticateWorkspaceFile,
mockReadWorkspaceFileContentByKey,
Expand All @@ -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(),
Expand Down Expand Up @@ -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', () => ({
Expand Down Expand Up @@ -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',
Expand Down Expand Up @@ -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({
Expand Down Expand Up @@ -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)
Expand Down
27 changes: 14 additions & 13 deletions apps/sim/app/api/files/serve/[...path]/route.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Expand Down Expand Up @@ -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 })
Expand Down Expand Up @@ -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 })
Expand Down Expand Up @@ -285,19 +289,16 @@ async function handleLocalFile(
filename: string,
userId: string,
options: ServeOptions,
signal: AbortSignal | undefined
signal: AbortSignal | undefined,
context: StorageContext
): Promise<NextResponse> {
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
)

Expand Down Expand Up @@ -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)
Expand All @@ -344,12 +345,12 @@ async function handleCloudProxy(
cloudKey: string,
userId: string,
options: ServeOptions,
signal: AbortSignal | undefined
signal: AbortSignal | undefined,
context: StorageContext
): Promise<NextResponse> {
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,
Expand Down
36 changes: 36 additions & 0 deletions apps/sim/lib/uploads/server/metadata.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import {
insertFileMetadataMany,
insertImmutableFileMetadata,
recordKnowledgeBaseFileOwnership,
resolveStoredFileContext,
} from '@/lib/uploads/server/metadata'

describe('recordKnowledgeBaseFileOwnership', () => {
Expand Down Expand Up @@ -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()
})
})
23 changes: 23 additions & 0 deletions apps/sim/lib/uploads/server/metadata.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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')
Expand Down Expand Up @@ -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<StorageContext> {
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.
Expand Down
Loading