diff --git a/apps/sim/app/api/files/authorization.test.ts b/apps/sim/app/api/files/authorization.test.ts index 8cce46882fc..f0ac6468536 100644 --- a/apps/sim/app/api/files/authorization.test.ts +++ b/apps/sim/app/api/files/authorization.test.ts @@ -207,3 +207,118 @@ describe('public-context access (profile-pictures / og-images / workspace-logos) expect(mockGetUserEntityPermissions).not.toHaveBeenCalled() }) }) + +/** + * The `workspace/` prefix carries two module contexts — a Files-module workspace + * file and a mothership chat attachment — and both authorize identically here, by + * membership of the owning workspace. Filtering the binding lookup to `workspace` + * alone silently missed every attachment and fell through to object metadata, + * which cannot see a soft delete. + */ +describe('workspace-scoped access (workspace files and mothership attachments)', () => { + const ATTACHMENT_KEY = 'workspace/ws-1/1786000000000-a3f2-photo.png' + + beforeEach(() => { + vi.clearAllMocks() + // No legacy `workspace_file` row and no object metadata, so a denial can only + // come from the binding itself rather than a fallback happening to grant. + dbChainMockFns.limit.mockResolvedValue([]) + mockGetFileMetadata.mockResolvedValue({}) + }) + + function read(cloudKey: string, context: 'workspace' | 'mothership') { + return verifyFileAccess(cloudKey, USER_ID, undefined, context, false) + } + + interface BoundRow { + workspaceId: string + userId: string + context: string + deletedAt: Date | null + } + + /** + * Installs the single row bound to the key, applying the same `context` and + * `includeDeleted` filters the real `getFileMetadataByKey` applies. Honoring the + * arguments is the whole point: a mock that returns the row unconditionally would + * pass against a lookup hard-filtered to `context = 'workspace'`, which is exactly + * the bug these tests exist to catch. + */ + function bindRow(row: BoundRow) { + mockGetFileMetadataByKey.mockImplementation( + async (_key: string, context?: string, options?: { includeDeleted?: boolean }) => { + if (context && row.context !== context) return null + if (!options?.includeDeleted && row.deletedAt) return null + return row + } + ) + } + + it.each(['workspace', 'mothership'] as const)( + 'grants a %s-context binding on workspace membership', + async (rowContext) => { + bindRow({ + workspaceId: 'ws-1', + userId: USER_ID, + context: rowContext, + deletedAt: null, + }) + mockGetUserEntityPermissions.mockResolvedValue('read') + + await expect(read(ATTACHMENT_KEY, 'workspace')).resolves.toBe(true) + expect(mockGetUserEntityPermissions).toHaveBeenCalledWith(USER_ID, 'workspace', 'ws-1') + // The binding answered, so the weaker object-metadata path is never consulted. + expect(mockGetFileMetadata).not.toHaveBeenCalled() + } + ) + + it('resolves the binding regardless of which workspace-scoped context the caller names', async () => { + bindRow({ + workspaceId: 'ws-1', + userId: USER_ID, + context: 'mothership', + deletedAt: null, + }) + mockGetUserEntityPermissions.mockResolvedValue('read') + + await expect(read(ATTACHMENT_KEY, 'mothership')).resolves.toBe(true) + }) + + it('denies a soft-deleted attachment instead of falling through to object metadata', async () => { + bindRow({ + workspaceId: 'ws-1', + userId: USER_ID, + context: 'mothership', + deletedAt: new Date('2026-08-01T00:00:00Z'), + }) + mockGetFileMetadata.mockResolvedValue({ workspaceId: 'ws-1' }) + mockGetUserEntityPermissions.mockResolvedValue('admin') + + await expect(read(ATTACHMENT_KEY, 'workspace')).resolves.toBe(false) + expect(mockGetUserEntityPermissions).not.toHaveBeenCalled() + }) + + it('denies a cross-tenant read of an attachment', async () => { + bindRow({ + workspaceId: 'victim-ws', + userId: 'other-user', + context: 'mothership', + deletedAt: null, + }) + mockGetUserEntityPermissions.mockResolvedValue(null) + + await expect(read(ATTACHMENT_KEY, 'workspace')).resolves.toBe(false) + }) + + it('does not accept a binding whose context is not workspace-scoped', async () => { + bindRow({ + workspaceId: 'ws-1', + userId: USER_ID, + context: 'copilot', + deletedAt: null, + }) + + await expect(read(ATTACHMENT_KEY, 'workspace')).resolves.toBe(false) + expect(mockGetUserEntityPermissions).not.toHaveBeenCalled() + }) +}) diff --git a/apps/sim/app/api/files/authorization.ts b/apps/sim/app/api/files/authorization.ts index 64d3ee4650a..77d087bc859 100644 --- a/apps/sim/app/api/files/authorization.ts +++ b/apps/sim/app/api/files/authorization.ts @@ -8,6 +8,7 @@ import { getFileMetadata } from '@/lib/uploads' import type { StorageContext } from '@/lib/uploads/config' import type { StorageConfig } from '@/lib/uploads/core/storage-client' import { getFileMetadataByKey } from '@/lib/uploads/server/metadata' +import { isWorkspaceScopedContext } from '@/lib/uploads/shared/types' import { inferContextFromKey } from '@/lib/uploads/utils/file-utils' import { getUserEntityPermissions } from '@/lib/workspaces/permissions/utils' import { isUuid } from '@/executor/constants' @@ -30,13 +31,6 @@ 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`) @@ -50,18 +44,26 @@ function workspacePermissionSatisfies( } /** - * Lookup workspace file by storage key from database + * Lookup the workspace-scoped binding for a storage key. + * + * Matches either context stored under the `workspace/` prefix rather than + * `workspace` alone: the prefix does not say which module owns the object, and + * both are authorized identically here — by membership of the owning workspace. + * Filtering to one of them would silently miss the other and fall through to the + * weaker object-metadata path, which cannot see a soft delete. + * * @param key Storage key to lookup * @returns Workspace file info or null if not found */ async function lookupWorkspaceFileByKey( key: string, - options?: { includeDeleted?: boolean; context?: WorkspaceScopedContext } + options?: { includeDeleted?: boolean } ): Promise<{ workspaceId: string; uploadedBy: string } | null> { try { - const { includeDeleted = false, context = 'workspace' } = options ?? {} + const { includeDeleted = false } = options ?? {} // Priority 1: Check new workspaceFiles table - const fileRecord = await getFileMetadataByKey(key, context, { includeDeleted }) + const record = await getFileMetadataByKey(key, undefined, { includeDeleted }) + const fileRecord = isWorkspaceScopedContext(record?.context) ? record : undefined if (fileRecord) { return { @@ -164,15 +166,8 @@ 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, - inferredContext - ) + if (isWorkspaceScopedContext(inferredContext)) { + return await verifyWorkspaceFileAccess(cloudKey, userId, customConfig, isLocal, requireWrite) } // 2. Execution files: workspace_id/workflow_id/execution_id/filename @@ -214,13 +209,15 @@ async function verifyWorkspaceFileAccess( userId: string, customConfig?: StorageConfig, isLocal?: boolean, - requireWrite = false, - context: WorkspaceScopedContext = 'workspace' + requireWrite = false ): Promise { try { - const anyWorkspaceFileRecord = await getFileMetadataByKey(cloudKey, context, { + const anyRecord = await getFileMetadataByKey(cloudKey, undefined, { includeDeleted: true, }) + const anyWorkspaceFileRecord = isWorkspaceScopedContext(anyRecord?.context) + ? anyRecord + : undefined if (anyWorkspaceFileRecord?.deletedAt) { logger.warn('Workspace file access denied for archived file', { userId, @@ -230,7 +227,7 @@ async function verifyWorkspaceFileAccess( } // Priority 1: Check database (most reliable, works for both local and cloud) - const workspaceFileRecord = await lookupWorkspaceFileByKey(cloudKey, { context }) + const workspaceFileRecord = await lookupWorkspaceFileByKey(cloudKey) if (workspaceFileRecord) { const permission = await getUserEntityPermissions( userId, diff --git a/apps/sim/app/api/files/parse/route.ts b/apps/sim/app/api/files/parse/route.ts index a6c047ec217..a844c9377c7 100644 --- a/apps/sim/app/api/files/parse/route.ts +++ b/apps/sim/app/api/files/parse/route.ts @@ -23,6 +23,7 @@ import { } from '@/lib/uploads/contexts/workspace' import { UPLOAD_DIR_SERVER } from '@/lib/uploads/core/setup.server' import { getFileMetadataByKey } from '@/lib/uploads/server/metadata' +import { isWorkspaceScopedContext } from '@/lib/uploads/shared/types' import { extractCleanFilename, extractStorageKey, @@ -640,9 +641,13 @@ async function handleCloudFile( } let originalFilename: string | undefined - if (context === 'workspace') { + // Not filtered to `context = 'workspace'`: a chat attachment carries the same key + // prefix and has an `originalName` worth recovering too, and without it the parse + // result is labelled with the raw storage segment. Access was authorized above; + // this only recovers a display name. + if (isWorkspaceScopedContext(context)) { try { - const fileRecord = await getFileMetadataByKey(cloudKey, 'workspace') + const fileRecord = await getFileMetadataByKey(cloudKey) if (fileRecord) { originalFilename = fileRecord.originalName diff --git a/apps/sim/app/api/files/serve/[...path]/route.ts b/apps/sim/app/api/files/serve/[...path]/route.ts index a36b814fb16..7599c1b122b 100644 --- a/apps/sim/app/api/files/serve/[...path]/route.ts +++ b/apps/sim/app/api/files/serve/[...path]/route.ts @@ -166,9 +166,11 @@ export const GET = withRouteHandler( return await handleLocalFilePublic(fullPath) } - // 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. + // Which module owns the object decides which branch below may serve it, and that + // is the row's answer, not the prefix's — a `workspace/` key carries both Files + // module files and mothership chat attachments. Reading the prefix alone here is + // what sent every attachment into the workspace-file use case, which matches on + // `context = 'workspace'` and answered 404 for a file that was present. const storageContext = await resolveStoredFileContext(cloudKey) const workspacePrincipal = storageContext === 'workspace' diff --git a/apps/sim/lib/uploads/server/metadata.ts b/apps/sim/lib/uploads/server/metadata.ts index 4485a9e0661..1d61d15770e 100644 --- a/apps/sim/lib/uploads/server/metadata.ts +++ b/apps/sim/lib/uploads/server/metadata.ts @@ -5,7 +5,11 @@ 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' +import { + isWorkspaceScopedContext, + type StorageContext, + toLegacyWorkspaceFileSize, +} from '../shared/types' const logger = createLogger('FileMetadata') @@ -339,24 +343,30 @@ export async function getFileMetadataByKey( /** * Resolve the storage context a stored object must be read and authorized under. + * This is the sanctioned way to ask that question — `inferContextFromKey` alone + * answers only bucket and tenancy (see its contract). * - * 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. + * The two layers divide as follows. The key prefix is authoritative for *where + * the bytes live*: it is written server-side at upload and cannot be forged to + * change tenant. `workspace_files.context` is authoritative for *which module + * owns the object*: it too is server-authored, but unlike the key it is mutable, + * which it has to be — `materialize_file` promotes a chat attachment to a + * workspace file by flipping that column, and rewriting the storage key on every + * such transition would mean copying the bytes to say the same thing twice. + * + * So only the `workspace/` prefix is ambiguous — it carries the two + * `WORKSPACE_SCOPED_CONTEXTS` — and only it costs a lookup. Every other prefix + * maps to exactly one module and returns immediately. * * 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. + * anything, 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 + return isWorkspaceScopedContext(metadata?.context) ? metadata.context : inferred } /** diff --git a/apps/sim/lib/uploads/shared/types.ts b/apps/sim/lib/uploads/shared/types.ts index 37ff23c1bd1..e0fb1ffcb94 100644 --- a/apps/sim/lib/uploads/shared/types.ts +++ b/apps/sim/lib/uploads/shared/types.ts @@ -50,6 +50,26 @@ export type StorageContext = | 'logs' | 'workspace-logos' +/** + * The contexts stored under the `workspace/` key prefix. They share a bucket and + * a workspace tenancy scope and differ only in which module owns the object: the + * Files module, or a mothership chat that the file was attached to. + * + * The prefix cannot separate them, and it never will — `materialize_file` + * promotes an attachment to a workspace file by flipping the row, so ownership + * is mutable while the key is not. Anything that needs the owning module reads + * `workspace_files.context`; the prefix answers only bucket and tenancy. + */ +export const WORKSPACE_SCOPED_CONTEXTS = ['workspace', 'mothership'] as const + +export type WorkspaceScopedContext = (typeof WORKSPACE_SCOPED_CONTEXTS)[number] + +export function isWorkspaceScopedContext( + context: string | null | undefined +): context is WorkspaceScopedContext { + return WORKSPACE_SCOPED_CONTEXTS.includes(context as WorkspaceScopedContext) +} + export type MultipartCompletionPolicy = 'create-only' | 'replace' | 'reuse-existing' export interface FileInfo { diff --git a/apps/sim/lib/uploads/utils/file-utils.ts b/apps/sim/lib/uploads/utils/file-utils.ts index 46a56af66ac..98914498afb 100644 --- a/apps/sim/lib/uploads/utils/file-utils.ts +++ b/apps/sim/lib/uploads/utils/file-utils.ts @@ -736,6 +736,15 @@ export function isInternalFileUrl(fileUrl: string): boolean { * prefixes: `kb/` (server-side uploads) or `knowledge-base/` (direct/presigned * uploads, whose default key is `${context}/...`). Both map to the same * `knowledge-base` context. + * + * What this answers is *where the bytes live* — which bucket and which tenant — + * and for that the prefix is authoritative. It does NOT answer which product + * module owns the object: `workspace/` covers both a Files-module workspace file + * and a mothership chat attachment, which share a bucket and a workspace scope + * and differ only by `workspace_files.context`. Module ownership is also mutable + * (`materialize_file` promotes an attachment to a workspace file), so it cannot + * live in an immutable key. A caller that needs the owning module must read the + * row — see `resolveStoredFileContext` — never this prefix. */ export function inferContextFromKey(key: string): StorageContext { if (!key) { @@ -779,6 +788,11 @@ const PUBLIC_STORAGE_CONTEXTS = new Set([ * private `workspace/…` key from being relabeled with a world-readable context * to bypass authorization and read the shared bucket. * + * "Authoritative" is scoped to bucket and tenancy, which is all this defends. + * It is not a claim about which module owns the object; that is the row's job + * (`resolveStoredFileContext`), and reading it costs nothing here because the + * row is server-authored too — the value being refused above is the *caller's*. + * * Legacy keys predating context-prefixed keys cannot be inferred; for those the * persisted `context` is honored so existing files stay resolvable — except a * world-readable context, which would reopen the bypass on an un-inferrable key.