From 4965459a98218d7624c433e43b6084d6ca31ba99 Mon Sep 17 00:00:00 2001 From: Vikhyath Mondreti Date: Mon, 17 Aug 2026 17:57:32 -0700 Subject: [PATCH] improvement(files): make the row the documented owner of module context MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follows the serve fix by stating the contract it relies on, so the next reader does not re-derive module ownership from the key prefix. The prefix is authoritative for where the bytes live — bucket and tenant — and nothing more. Which module owns an object is `workspace_files.context`, which is server-authored like the key but, unlike the key, mutable: a chat attachment becomes a workspace file when `materialize_file` flips that column, and encoding a mutable fact in an immutable key would mean copying the bytes on every such transition just to restate them. `resolveTrustedFileContext` claimed the prefix was flatly authoritative. That claim is what made routing on it look safe. It is now scoped to what it actually defends — a caller-supplied context can still never relabel a private key — and `resolveStoredFileContext` is documented as the sanctioned way to ask who owns an object rather than as a workaround. `verifyWorkspaceFileAccess` resolved its binding with the lookup filtered to `context = 'workspace'`, so an attachment missed the row and fell through to object metadata, which cannot see a soft delete. It now matches either workspace-scoped context, which is also what every caller already wanted: the LLM-attachment and presigned-URL paths pass 'workspace' for attachment keys today. A soft-deleted attachment is now denied on all of them. The parse route carried the same gate and labelled parsed attachments with the raw storage segment instead of the uploaded filename. Module-scoped filters are deliberately untouched: the Files module, its folder manager, forking and the workspace-file use cases all match `context = 'workspace'` because they mean the Files module, not the bucket. --- apps/sim/app/api/files/authorization.test.ts | 115 ++++++++++++++++++ apps/sim/app/api/files/authorization.ts | 45 ++++--- apps/sim/app/api/files/parse/route.ts | 9 +- .../app/api/files/serve/[...path]/route.ts | 8 +- apps/sim/lib/uploads/server/metadata.ts | 30 +++-- apps/sim/lib/uploads/shared/types.ts | 20 +++ apps/sim/lib/uploads/utils/file-utils.ts | 14 +++ 7 files changed, 202 insertions(+), 39 deletions(-) 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.