mirror of
https://github.com/simstudioai/sim.git
synced 2026-09-24 15:45:35 +08:00
fix(files): serve mothership chat attachments stored under a workspace key (#6789)
A mothership chat attachment is minted with the same storage key shape as a
workspace file — `resolveUploadStorage` calls `generateWorkspaceFileKey` for
`mothership_attachment` — but its row is written with `context = 'mothership'`.
The serve route branched on the key prefix alone, so every attachment entered
the workspace-file use case, which matches on `context = 'workspace'` and
resolved it to nothing: `{"error":"FileNotFoundError","message":"File not
found"}` for every chat thumbnail and click-through. The `context=mothership`
query param on the serve URL is decorative; the route never read it.
Serve now resolves the storage context from the key's stored binding, which is
server-authored at upload and the only thing that separates the two, and passes
it into the cloud and local handlers instead of re-inferring. Genuine workspace
files still go through the authorized use case.
`verifyWorkspaceFileAccess` takes the context too, so an attachment authorizes
from its database row rather than falling through to storage-object metadata.
A soft-deleted attachment is now denied, matching workspace files.
The route tests are what let this ship: they mocked `inferContextFromKey` to
return 'mothership' for a `workspace/…` key, which it never does. With the mock
made honest, nine of them fail against the old route.
This commit is contained in:
@@ -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<boolean> {
|
||||
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,
|
||||
|
||||
@@ -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)
|
||||
|
||||
@@ -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<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
|
||||
)
|
||||
|
||||
@@ -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<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,
|
||||
|
||||
@@ -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()
|
||||
})
|
||||
})
|
||||
|
||||
@@ -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<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.
|
||||
|
||||
Reference in New Issue
Block a user