Skip to content

Commit 9bed841

Browse files
icecrasher321icecrasher321
authored
improvement(kbs): ownership bindings (#4833)
* improvement(kbs): ownership association * address comments * more comments
1 parent 2e0f506 commit 9bed841

27 files changed

Lines changed: 18943 additions & 121 deletions

File tree

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

Lines changed: 153 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,153 @@
1+
/**
2+
* Tests for KB file authorization (`verifyKBFileAccess` via `verifyFileAccess`).
3+
*
4+
* These lock in the security-critical contract: access is granted only when a
5+
* trusted ownership binding names a workspace the caller can access AND an active
6+
* document still references the exact key. A planted `document.fileUrl` (the
7+
* reported vulnerability) can never grant access because ownership comes from the
8+
* binding, not the document.
9+
*
10+
* @vitest-environment node
11+
*/
12+
import { dbChainMock, dbChainMockFns } from '@sim/testing'
13+
import { beforeEach, describe, expect, it, vi } from 'vitest'
14+
15+
const { mockGetFileMetadataByKey, mockGetUserEntityPermissions } = vi.hoisted(() => ({
16+
mockGetFileMetadataByKey: vi.fn(),
17+
mockGetUserEntityPermissions: vi.fn(),
18+
}))
19+
20+
vi.mock('@sim/db', () => dbChainMock)
21+
22+
vi.mock('@/lib/uploads', () => ({
23+
getFileMetadata: vi.fn(),
24+
}))
25+
26+
vi.mock('@/lib/uploads/config', () => ({
27+
BLOB_CHAT_CONFIG: {},
28+
S3_CHAT_CONFIG: {},
29+
}))
30+
31+
vi.mock('@/lib/uploads/server/metadata', () => ({
32+
getFileMetadataByKey: mockGetFileMetadataByKey,
33+
}))
34+
35+
vi.mock('@/lib/uploads/utils/file-utils', () => ({
36+
inferContextFromKey: vi.fn(() => 'knowledge-base'),
37+
}))
38+
39+
vi.mock('@/lib/workspaces/permissions/utils', () => ({
40+
getUserEntityPermissions: mockGetUserEntityPermissions,
41+
}))
42+
43+
vi.mock('@/executor/constants', () => ({
44+
isUuid: vi.fn(() => false),
45+
}))
46+
47+
import { verifyFileAccess, verifyKBFileWriteAccess } from '@/app/api/files/authorization'
48+
49+
const CLOUD_KEY = 'kb/1780162789495-secret.txt'
50+
const USER_ID = 'user-1'
51+
52+
function grantAccess(cloudKey: string) {
53+
return verifyFileAccess(cloudKey, USER_ID, undefined, 'knowledge-base')
54+
}
55+
56+
describe('verifyKBFileAccess (binding-only)', () => {
57+
beforeEach(() => {
58+
vi.clearAllMocks()
59+
// Default liveness query result: one active document references the exact storage key.
60+
dbChainMockFns.limit.mockResolvedValue([{ id: 'doc-1' }])
61+
})
62+
63+
it('grants access when the binding owner workspace is accessible and an active document references the key', async () => {
64+
mockGetFileMetadataByKey.mockResolvedValue({ workspaceId: 'ws-1', deletedAt: null })
65+
mockGetUserEntityPermissions.mockResolvedValue('read')
66+
67+
await expect(grantAccess(CLOUD_KEY)).resolves.toBe(true)
68+
expect(mockGetUserEntityPermissions).toHaveBeenCalledWith(USER_ID, 'workspace', 'ws-1')
69+
})
70+
71+
it('denies when the caller lacks permission on the owner workspace (cross-tenant)', async () => {
72+
mockGetFileMetadataByKey.mockResolvedValue({ workspaceId: 'victim-ws', deletedAt: null })
73+
mockGetUserEntityPermissions.mockResolvedValue(null)
74+
75+
await expect(grantAccess(CLOUD_KEY)).resolves.toBe(false)
76+
})
77+
78+
it('denies when there is no ownership binding (planted or un-backfilled key)', async () => {
79+
mockGetFileMetadataByKey.mockResolvedValue(null)
80+
81+
await expect(grantAccess(CLOUD_KEY)).resolves.toBe(false)
82+
// Authorization never consults workspace permissions without a binding.
83+
expect(mockGetUserEntityPermissions).not.toHaveBeenCalled()
84+
})
85+
86+
it('denies when the binding is soft-deleted', async () => {
87+
mockGetFileMetadataByKey.mockResolvedValue({ workspaceId: 'ws-1', deletedAt: new Date() })
88+
89+
await expect(grantAccess(CLOUD_KEY)).resolves.toBe(false)
90+
expect(mockGetUserEntityPermissions).not.toHaveBeenCalled()
91+
})
92+
93+
it('denies when the binding has no workspace owner', async () => {
94+
mockGetFileMetadataByKey.mockResolvedValue({ workspaceId: null, deletedAt: null })
95+
96+
await expect(grantAccess(CLOUD_KEY)).resolves.toBe(false)
97+
expect(mockGetUserEntityPermissions).not.toHaveBeenCalled()
98+
})
99+
100+
it('denies when no active document references the key (archived/soft-deleted KB liveness)', async () => {
101+
mockGetFileMetadataByKey.mockResolvedValue({ workspaceId: 'ws-1', deletedAt: null })
102+
mockGetUserEntityPermissions.mockResolvedValue('admin')
103+
dbChainMockFns.limit.mockResolvedValue([])
104+
105+
await expect(grantAccess(CLOUD_KEY)).resolves.toBe(false)
106+
})
107+
108+
it('fails closed when the binding lookup throws', async () => {
109+
mockGetFileMetadataByKey.mockRejectedValue(new Error('db down'))
110+
111+
await expect(grantAccess(CLOUD_KEY)).resolves.toBe(false)
112+
})
113+
})
114+
115+
describe('verifyKBFileWriteAccess (binding-only delete authorization)', () => {
116+
beforeEach(() => {
117+
vi.clearAllMocks()
118+
})
119+
120+
it('grants delete when the caller has write on the owner workspace', async () => {
121+
mockGetFileMetadataByKey.mockResolvedValue({ workspaceId: 'ws-1', deletedAt: null })
122+
mockGetUserEntityPermissions.mockResolvedValue('write')
123+
124+
await expect(verifyKBFileWriteAccess(CLOUD_KEY, USER_ID)).resolves.toBe(true)
125+
})
126+
127+
it('grants delete when the caller is admin on the owner workspace', async () => {
128+
mockGetFileMetadataByKey.mockResolvedValue({ workspaceId: 'ws-1', deletedAt: null })
129+
mockGetUserEntityPermissions.mockResolvedValue('admin')
130+
131+
await expect(verifyKBFileWriteAccess(CLOUD_KEY, USER_ID)).resolves.toBe(true)
132+
})
133+
134+
it('denies delete when the caller has only read on the owner workspace', async () => {
135+
mockGetFileMetadataByKey.mockResolvedValue({ workspaceId: 'ws-1', deletedAt: null })
136+
mockGetUserEntityPermissions.mockResolvedValue('read')
137+
138+
await expect(verifyKBFileWriteAccess(CLOUD_KEY, USER_ID)).resolves.toBe(false)
139+
})
140+
141+
it('denies delete when there is no binding (no fallback)', async () => {
142+
mockGetFileMetadataByKey.mockResolvedValue(null)
143+
144+
await expect(verifyKBFileWriteAccess(CLOUD_KEY, USER_ID)).resolves.toBe(false)
145+
expect(mockGetUserEntityPermissions).not.toHaveBeenCalled()
146+
})
147+
148+
it('fails closed when the binding lookup throws', async () => {
149+
mockGetFileMetadataByKey.mockRejectedValue(new Error('db down'))
150+
151+
await expect(verifyKBFileWriteAccess(CLOUD_KEY, USER_ID)).resolves.toBe(false)
152+
})
153+
})

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

Lines changed: 103 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
import { db } from '@sim/db'
22
import { document, knowledgeBase, workspaceFile } from '@sim/db/schema'
33
import { createLogger } from '@sim/logger'
4-
import { and, eq, isNull, like, or } from 'drizzle-orm'
4+
import { and, eq, isNull } from 'drizzle-orm'
55
import { NextResponse } from 'next/server'
66
import { getFileMetadata } from '@/lib/uploads'
77
import type { StorageContext } from '@/lib/uploads/config'
@@ -371,73 +371,130 @@ async function verifyCopilotFileAccess(
371371
}
372372

373373
/**
374-
* Verify access to KB files
375-
* KB files: kb/filename
374+
* Whether an active KB document (non-archived/excluded/deleted, in a
375+
* non-deleted KB) in the owning workspace references exactly `cloudKey`, matched
376+
* on the document's persisted canonical `storageKey`. This is an exact, indexed
377+
* lookup — no URL parsing or wildcard matching at read time. It is a lifecycle
378+
* signal only: it reflects whether the file is still part of a live KB, not who
379+
* owns it (ownership comes from the binding).
380+
*/
381+
async function hasActiveKbDocumentForKey(cloudKey: string, workspaceId: string): Promise<boolean> {
382+
const rows = await db
383+
.select({ id: document.id })
384+
.from(document)
385+
.innerJoin(knowledgeBase, eq(document.knowledgeBaseId, knowledgeBase.id))
386+
.where(
387+
and(
388+
eq(knowledgeBase.workspaceId, workspaceId),
389+
eq(document.storageKey, cloudKey),
390+
eq(document.userExcluded, false),
391+
isNull(document.archivedAt),
392+
isNull(document.deletedAt),
393+
isNull(knowledgeBase.deletedAt)
394+
)
395+
)
396+
.limit(1)
397+
398+
return rows.length > 0
399+
}
400+
401+
/**
402+
* Verify access to KB files (`kb/<key>`).
403+
*
404+
* Authorization is determined entirely by clear state:
405+
* 1. Ownership — the trusted `workspace_files` binding (exact key) names the
406+
* owning workspace; the caller must have permission on it. Ownership is
407+
* never inferred from an attacker-authorable `document.fileUrl`.
408+
* 2. Liveness — an active document must still reference the exact key, so the
409+
* retained bytes of an archived document or soft-deleted KB are not
410+
* downloadable (the liveness document is not an authorization signal).
411+
*
412+
* A missing binding denies (the ownership backfill populates bindings for
413+
* pre-existing objects before this path is deployed).
376414
*/
377415
async function verifyKBFileAccess(
378416
cloudKey: string,
379417
userId: string,
380418
customConfig?: StorageConfig
381419
): Promise<boolean> {
382420
try {
383-
const activeKbFileDocuments = await db
384-
.select({
385-
workspaceId: knowledgeBase.workspaceId,
386-
})
387-
.from(document)
388-
.innerJoin(knowledgeBase, eq(document.knowledgeBaseId, knowledgeBase.id))
389-
.where(
390-
and(
391-
eq(document.userExcluded, false),
392-
isNull(document.archivedAt),
393-
isNull(document.deletedAt),
394-
isNull(knowledgeBase.deletedAt),
395-
or(
396-
like(document.fileUrl, `%${cloudKey}%`),
397-
like(document.fileUrl, `%${encodeURIComponent(cloudKey)}%`)
398-
)
399-
)
400-
)
401-
.limit(10)
402-
403-
for (const doc of activeKbFileDocuments) {
404-
if (!doc.workspaceId) {
405-
continue
406-
}
421+
const binding = await getFileMetadataByKey(cloudKey, 'knowledge-base', {
422+
includeDeleted: true,
423+
})
407424

408-
const permission = await getUserEntityPermissions(userId, 'workspace', doc.workspaceId)
409-
if (permission !== null) {
410-
logger.debug('KB file access granted (active document lookup)', {
411-
userId,
412-
workspaceId: doc.workspaceId,
413-
cloudKey,
414-
})
415-
return true
416-
}
425+
if (!binding) {
426+
logger.warn('KB file access denied: no ownership binding', { userId, cloudKey })
427+
return false
428+
}
429+
if (binding.deletedAt) {
430+
logger.warn('KB file access denied for deleted file binding', { userId, cloudKey })
431+
return false
432+
}
433+
if (!binding.workspaceId) {
434+
logger.warn('KB file binding missing workspace owner', { userId, cloudKey })
435+
return false
417436
}
418437

419-
// KB file access must resolve through an active KB document. Metadata alone is not enough
420-
// because parent archives intentionally keep the underlying file bytes around for history.
421-
const fileRecord = await getFileMetadataByKey(cloudKey, 'knowledge-base', {
422-
includeDeleted: true,
423-
})
438+
const permission = await getUserEntityPermissions(userId, 'workspace', binding.workspaceId)
439+
if (permission === null) {
440+
logger.warn('User does not have workspace access for KB file', {
441+
userId,
442+
workspaceId: binding.workspaceId,
443+
cloudKey,
444+
})
445+
return false
446+
}
424447

425-
if (fileRecord?.deletedAt) {
426-
logger.warn('KB file access denied for deleted file metadata', { userId, cloudKey })
448+
if (!(await hasActiveKbDocumentForKey(cloudKey, binding.workspaceId))) {
449+
logger.warn('KB file access denied: no active document references the file', {
450+
userId,
451+
cloudKey,
452+
})
427453
return false
428454
}
429455

430-
logger.warn('KB file access denied because no active KB document matched the file', {
431-
cloudKey,
456+
logger.debug('KB file access granted (ownership binding)', {
432457
userId,
458+
workspaceId: binding.workspaceId,
459+
cloudKey,
433460
})
434-
return false
461+
return true
435462
} catch (error) {
436463
logger.error('Error verifying KB file access', { cloudKey, userId, error })
437464
return false
438465
}
439466
}
440467

468+
/**
469+
* Authorize a destructive operation (delete) on a KB file.
470+
*
471+
* Binding-only: resolves the owning workspace from the trusted ownership binding
472+
* and requires write/admin permission. Never uses the transitional read fallback,
473+
* so a not-yet-bound key cannot be deleted cross-tenant.
474+
*/
475+
export async function verifyKBFileWriteAccess(cloudKey: string, userId: string): Promise<boolean> {
476+
try {
477+
const binding = await getFileMetadataByKey(cloudKey, 'knowledge-base')
478+
if (!binding?.workspaceId) {
479+
logger.warn('KB file delete denied: no ownership binding', { userId, cloudKey })
480+
return false
481+
}
482+
const permission = await getUserEntityPermissions(userId, 'workspace', binding.workspaceId)
483+
if (permission !== 'write' && permission !== 'admin') {
484+
logger.warn('KB file delete denied: write/admin required on owner workspace', {
485+
userId,
486+
workspaceId: binding.workspaceId,
487+
cloudKey,
488+
})
489+
return false
490+
}
491+
return true
492+
} catch (error) {
493+
logger.error('Error verifying KB file write access', { cloudKey, userId, error })
494+
return false
495+
}
496+
}
497+
441498
/**
442499
* Verify access to chat files
443500
* Chat files: chat/filename

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

Lines changed: 13 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ import type { StorageContext } from '@/lib/uploads/config'
99
import { deleteFile, hasCloudStorage } from '@/lib/uploads/core/storage-service'
1010
import { deleteFileMetadata } from '@/lib/uploads/server/metadata'
1111
import { extractStorageKey, inferContextFromKey } from '@/lib/uploads/utils/file-utils'
12-
import { verifyFileAccess } from '@/app/api/files/authorization'
12+
import { verifyFileAccess, verifyKBFileWriteAccess } from '@/app/api/files/authorization'
1313
import {
1414
createErrorResponse,
1515
createSuccessResponse,
@@ -64,13 +64,18 @@ export const POST = withRouteHandler(async (request: NextRequest) => {
6464

6565
const storageContext: StorageContext = context || inferContextFromKey(key)
6666

67-
const hasAccess = await verifyFileAccess(
68-
key,
69-
userId,
70-
undefined, // customConfig
71-
storageContext, // context
72-
!hasCloudStorage() // isLocal
73-
)
67+
// KB deletes are binding-only and require write/admin on the owning workspace;
68+
// they never use the transitional read fallback that file serving allows.
69+
const hasAccess =
70+
storageContext === 'knowledge-base'
71+
? await verifyKBFileWriteAccess(key, userId)
72+
: await verifyFileAccess(
73+
key,
74+
userId,
75+
undefined, // customConfig
76+
storageContext, // context
77+
!hasCloudStorage() // isLocal
78+
)
7479

7580
if (!hasAccess) {
7681
logger.warn('Unauthorized file delete attempt', { userId, key, context: storageContext })

0 commit comments

Comments
 (0)