improvement(kbs): ownership bindings

apps/sim/app/api/files/authorization.test.ts

@@ -0,0 +1,153 @@
+/**
+ * Tests for KB file authorization (`verifyKBFileAccess` via `verifyFileAccess`).
+ *
+ * These lock in the security-critical contract: access is granted only when a
+ * trusted ownership binding names a workspace the caller can access AND an active
+ * document still references the exact key. A planted `document.fileUrl` (the
+ * reported vulnerability) can never grant access because ownership comes from the
+ * binding, not the document.
+ *
+ * @vitest-environment node
+ */
+import { dbChainMock, dbChainMockFns } from '@sim/testing'
+import { beforeEach, describe, expect, it, vi } from 'vitest'
+
+const { mockGetFileMetadataByKey, mockGetUserEntityPermissions } = vi.hoisted(() => ({
+  mockGetFileMetadataByKey: vi.fn(),
+  mockGetUserEntityPermissions: vi.fn(),
+}))
+
+vi.mock('@sim/db', () => dbChainMock)
+
+vi.mock('@/lib/uploads', () => ({
+  getFileMetadata: vi.fn(),
+}))
+
+vi.mock('@/lib/uploads/config', () => ({
+  BLOB_CHAT_CONFIG: {},
+  S3_CHAT_CONFIG: {},
+}))
+
+vi.mock('@/lib/uploads/server/metadata', () => ({
+  getFileMetadataByKey: mockGetFileMetadataByKey,
+}))
+
+vi.mock('@/lib/uploads/utils/file-utils', () => ({
+  inferContextFromKey: vi.fn(() => 'knowledge-base'),
+}))
+
+vi.mock('@/lib/workspaces/permissions/utils', () => ({
+  getUserEntityPermissions: mockGetUserEntityPermissions,
+}))
+
+vi.mock('@/executor/constants', () => ({
+  isUuid: vi.fn(() => false),
+}))
+
+import { verifyFileAccess, verifyKBFileWriteAccess } from '@/app/api/files/authorization'
+
+const CLOUD_KEY = 'kb/1780162789495-secret.txt'
+const USER_ID = 'user-1'
+
+function grantAccess(cloudKey: string) {
+  return verifyFileAccess(cloudKey, USER_ID, undefined, 'knowledge-base')
+}
+
+describe('verifyKBFileAccess (binding-only)', () => {
+  beforeEach(() => {
+    vi.clearAllMocks()
+    // Default liveness query result: one active document references the exact storage key.
+    dbChainMockFns.limit.mockResolvedValue([{ id: 'doc-1' }])
+  })
+
+  it('grants access when the binding owner workspace is accessible and an active document references the key', async () => {
+    mockGetFileMetadataByKey.mockResolvedValue({ workspaceId: 'ws-1', deletedAt: null })
+    mockGetUserEntityPermissions.mockResolvedValue('read')
+
+    await expect(grantAccess(CLOUD_KEY)).resolves.toBe(true)
+    expect(mockGetUserEntityPermissions).toHaveBeenCalledWith(USER_ID, 'workspace', 'ws-1')
+  })
+
+  it('denies when the caller lacks permission on the owner workspace (cross-tenant)', async () => {
+    mockGetFileMetadataByKey.mockResolvedValue({ workspaceId: 'victim-ws', deletedAt: null })
+    mockGetUserEntityPermissions.mockResolvedValue(null)
+
+    await expect(grantAccess(CLOUD_KEY)).resolves.toBe(false)
+  })
+
+  it('denies when there is no ownership binding (planted or un-backfilled key)', async () => {
+    mockGetFileMetadataByKey.mockResolvedValue(null)
+
+    await expect(grantAccess(CLOUD_KEY)).resolves.toBe(false)
+    // Authorization never consults workspace permissions without a binding.
+    expect(mockGetUserEntityPermissions).not.toHaveBeenCalled()
+  })
+
+  it('denies when the binding is soft-deleted', async () => {
+    mockGetFileMetadataByKey.mockResolvedValue({ workspaceId: 'ws-1', deletedAt: new Date() })
+
+    await expect(grantAccess(CLOUD_KEY)).resolves.toBe(false)
+    expect(mockGetUserEntityPermissions).not.toHaveBeenCalled()
+  })
+
+  it('denies when the binding has no workspace owner', async () => {
+    mockGetFileMetadataByKey.mockResolvedValue({ workspaceId: null, deletedAt: null })
+
+    await expect(grantAccess(CLOUD_KEY)).resolves.toBe(false)
+    expect(mockGetUserEntityPermissions).not.toHaveBeenCalled()
+  })
+
+  it('denies when no active document references the key (archived/soft-deleted KB liveness)', async () => {
+    mockGetFileMetadataByKey.mockResolvedValue({ workspaceId: 'ws-1', deletedAt: null })
+    mockGetUserEntityPermissions.mockResolvedValue('admin')
+    dbChainMockFns.limit.mockResolvedValue([])
+
+    await expect(grantAccess(CLOUD_KEY)).resolves.toBe(false)
+  })
+
+  it('fails closed when the binding lookup throws', async () => {
+    mockGetFileMetadataByKey.mockRejectedValue(new Error('db down'))
+
+    await expect(grantAccess(CLOUD_KEY)).resolves.toBe(false)
+  })
+})
+
+describe('verifyKBFileWriteAccess (binding-only delete authorization)', () => {
+  beforeEach(() => {
+    vi.clearAllMocks()
+  })
+
+  it('grants delete when the caller has write on the owner workspace', async () => {
+    mockGetFileMetadataByKey.mockResolvedValue({ workspaceId: 'ws-1', deletedAt: null })
+    mockGetUserEntityPermissions.mockResolvedValue('write')
+
+    await expect(verifyKBFileWriteAccess(CLOUD_KEY, USER_ID)).resolves.toBe(true)
+  })
+
+  it('grants delete when the caller is admin on the owner workspace', async () => {
+    mockGetFileMetadataByKey.mockResolvedValue({ workspaceId: 'ws-1', deletedAt: null })
+    mockGetUserEntityPermissions.mockResolvedValue('admin')
+
+    await expect(verifyKBFileWriteAccess(CLOUD_KEY, USER_ID)).resolves.toBe(true)
+  })
+
+  it('denies delete when the caller has only read on the owner workspace', async () => {
+    mockGetFileMetadataByKey.mockResolvedValue({ workspaceId: 'ws-1', deletedAt: null })
+    mockGetUserEntityPermissions.mockResolvedValue('read')
+
+    await expect(verifyKBFileWriteAccess(CLOUD_KEY, USER_ID)).resolves.toBe(false)
+  })
+
+  it('denies delete when there is no binding (no fallback)', async () => {
+    mockGetFileMetadataByKey.mockResolvedValue(null)
+
+    await expect(verifyKBFileWriteAccess(CLOUD_KEY, USER_ID)).resolves.toBe(false)
+    expect(mockGetUserEntityPermissions).not.toHaveBeenCalled()
+  })
+
+  it('fails closed when the binding lookup throws', async () => {
+    mockGetFileMetadataByKey.mockRejectedValue(new Error('db down'))
+
+    await expect(verifyKBFileWriteAccess(CLOUD_KEY, USER_ID)).resolves.toBe(false)
+  })
+})

apps/sim/app/api/files/authorization.ts

@@ -1,7 +1,7 @@
 import { db } from '@sim/db'
 import { document, knowledgeBase, workspaceFile } from '@sim/db/schema'
 import { createLogger } from '@sim/logger'
-import { and, eq, isNull, like, or } from 'drizzle-orm'
+import { and, eq, isNull } from 'drizzle-orm'
 import { NextResponse } from 'next/server'
 import { getFileMetadata } from '@/lib/uploads'
 import type { StorageContext } from '@/lib/uploads/config'
@@ -371,73 +371,130 @@ async function verifyCopilotFileAccess(
 }
 
 /**
- * Verify access to KB files
- * KB files: kb/filename
+ * Whether an active KB document (non-archived/excluded/deleted, in a
+ * non-deleted KB) in the owning workspace references exactly `cloudKey`, matched
+ * on the document's persisted canonical `storageKey`. This is an exact, indexed
+ * lookup — no URL parsing or wildcard matching at read time. It is a lifecycle
+ * signal only: it reflects whether the file is still part of a live KB, not who
+ * owns it (ownership comes from the binding).
+ */
+async function hasActiveKbDocumentForKey(cloudKey: string, workspaceId: string): Promise<boolean> {
+  const rows = await db
+    .select({ id: document.id })
+    .from(document)
+    .innerJoin(knowledgeBase, eq(document.knowledgeBaseId, knowledgeBase.id))
+    .where(
+      and(
+        eq(knowledgeBase.workspaceId, workspaceId),
+        eq(document.storageKey, cloudKey),
+        eq(document.userExcluded, false),
+        isNull(document.archivedAt),
+        isNull(document.deletedAt),
+        isNull(knowledgeBase.deletedAt)
+      )
+    )
+    .limit(1)
+
+  return rows.length > 0
+}
+
+/**
+ * Verify access to KB files (`kb/<key>`).
+ *
+ * Authorization is determined entirely by clear state:
+ *   1. Ownership — the trusted `workspace_files` binding (exact key) names the
+ *      owning workspace; the caller must have permission on it. Ownership is
+ *      never inferred from an attacker-authorable `document.fileUrl`.
+ *   2. Liveness — an active document must still reference the exact key, so the
+ *      retained bytes of an archived document or soft-deleted KB are not
+ *      downloadable (the liveness document is not an authorization signal).
+ *
+ * A missing binding denies (the ownership backfill populates bindings for
+ * pre-existing objects before this path is deployed).
  */
 async function verifyKBFileAccess(
   cloudKey: string,
   userId: string,
   customConfig?: StorageConfig
 ): Promise<boolean> {
   try {
-    const activeKbFileDocuments = await db
-      .select({
-        workspaceId: knowledgeBase.workspaceId,
-      })
-      .from(document)
-      .innerJoin(knowledgeBase, eq(document.knowledgeBaseId, knowledgeBase.id))
-      .where(
-        and(
-          eq(document.userExcluded, false),
-          isNull(document.archivedAt),
-          isNull(document.deletedAt),
-          isNull(knowledgeBase.deletedAt),
-          or(
-            like(document.fileUrl, `%${cloudKey}%`),
-            like(document.fileUrl, `%${encodeURIComponent(cloudKey)}%`)
-          )
-        )
-      )
-      .limit(10)
-
-    for (const doc of activeKbFileDocuments) {
-      if (!doc.workspaceId) {
-        continue
-      }
+    const binding = await getFileMetadataByKey(cloudKey, 'knowledge-base', {
+      includeDeleted: true,
+    })
 
-      const permission = await getUserEntityPermissions(userId, 'workspace', doc.workspaceId)
-      if (permission !== null) {
-        logger.debug('KB file access granted (active document lookup)', {
-          userId,
-          workspaceId: doc.workspaceId,
-          cloudKey,
-        })
-        return true
-      }
+    if (!binding) {
+      logger.warn('KB file access denied: no ownership binding', { userId, cloudKey })
+      return false
+    }
+    if (binding.deletedAt) {
+      logger.warn('KB file access denied for deleted file binding', { userId, cloudKey })
+      return false
+    }
+    if (!binding.workspaceId) {
+      logger.warn('KB file binding missing workspace owner', { userId, cloudKey })
+      return false
     }
 
-    // KB file access must resolve through an active KB document. Metadata alone is not enough
-    // because parent archives intentionally keep the underlying file bytes around for history.
-    const fileRecord = await getFileMetadataByKey(cloudKey, 'knowledge-base', {
-      includeDeleted: true,
-    })
+    const permission = await getUserEntityPermissions(userId, 'workspace', binding.workspaceId)
+    if (permission === null) {
+      logger.warn('User does not have workspace access for KB file', {
+        userId,
+        workspaceId: binding.workspaceId,
+        cloudKey,
+      })
+      return false
+    }
 
-    if (fileRecord?.deletedAt) {
-      logger.warn('KB file access denied for deleted file metadata', { userId, cloudKey })
+    if (!(await hasActiveKbDocumentForKey(cloudKey, binding.workspaceId))) {
+      logger.warn('KB file access denied: no active document references the file', {
+        userId,
+        cloudKey,
+      })
       return false
     }
 
-    logger.warn('KB file access denied because no active KB document matched the file', {
-      cloudKey,
+    logger.debug('KB file access granted (ownership binding)', {
       userId,
+      workspaceId: binding.workspaceId,
+      cloudKey,
     })
-    return false
+    return true
   } catch (error) {
     logger.error('Error verifying KB file access', { cloudKey, userId, error })
     return false
   }
 }
 
+/**
+ * Authorize a destructive operation (delete) on a KB file.
+ *
+ * Binding-only: resolves the owning workspace from the trusted ownership binding
+ * and requires write/admin permission. Never uses the transitional read fallback,
+ * so a not-yet-bound key cannot be deleted cross-tenant.
+ */
+export async function verifyKBFileWriteAccess(cloudKey: string, userId: string): Promise<boolean> {
+  try {
+    const binding = await getFileMetadataByKey(cloudKey, 'knowledge-base')
+    if (!binding?.workspaceId) {
+      logger.warn('KB file delete denied: no ownership binding', { userId, cloudKey })
+      return false
+    }
+    const permission = await getUserEntityPermissions(userId, 'workspace', binding.workspaceId)
+    if (permission !== 'write' && permission !== 'admin') {
+      logger.warn('KB file delete denied: write/admin required on owner workspace', {
+        userId,
+        workspaceId: binding.workspaceId,
+        cloudKey,
+      })
+      return false
+    }
+    return true
+  } catch (error) {
+    logger.error('Error verifying KB file write access', { cloudKey, userId, error })
+    return false
+  }
+}
+
 /**
  * Verify access to chat files
  * Chat files: chat/filename

apps/sim/app/api/files/delete/route.ts

@@ -9,7 +9,7 @@ import type { StorageContext } from '@/lib/uploads/config'
 import { deleteFile, hasCloudStorage } from '@/lib/uploads/core/storage-service'
 import { deleteFileMetadata } from '@/lib/uploads/server/metadata'
 import { extractStorageKey, inferContextFromKey } from '@/lib/uploads/utils/file-utils'
-import { verifyFileAccess } from '@/app/api/files/authorization'
+import { verifyFileAccess, verifyKBFileWriteAccess } from '@/app/api/files/authorization'
 import {
   createErrorResponse,
   createSuccessResponse,
@@ -64,13 +64,18 @@ export const POST = withRouteHandler(async (request: NextRequest) => {
 
       const storageContext: StorageContext = context || inferContextFromKey(key)
 
-      const hasAccess = await verifyFileAccess(
-        key,
-        userId,
-        undefined, // customConfig
-        storageContext, // context
-        !hasCloudStorage() // isLocal
-      )
+      // KB deletes are binding-only and require write/admin on the owning workspace;
+      // they never use the transitional read fallback that file serving allows.
+      const hasAccess =
+        storageContext === 'knowledge-base'
+          ? await verifyKBFileWriteAccess(key, userId)
+          : await verifyFileAccess(
+              key,
+              userId,
+              undefined, // customConfig
+              storageContext, // context
+              !hasCloudStorage() // isLocal
+            )
 
       if (!hasAccess) {
         logger.warn('Unauthorized file delete attempt', { userId, key, context: storageContext })