Skip to content

Commit 9312157

Browse files
committed
fix(security): anchor KB file ownership to earliest document in any state
A KB file's owner is now the earliest document referencing its key regardless of state (active/archived/deleted/excluded); access is granted only when that owning document is still active. Closes the residual where an attacker could plant an active document to claim a file whose original document was archived or deleted.
1 parent 237be12 commit 9312157

2 files changed

Lines changed: 57 additions & 17 deletions

File tree

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

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -149,6 +149,33 @@ describe('verifyKBFileAccess', () => {
149149
)
150150
})
151151

152+
it('denies a planted active document when the original owner document is archived', async () => {
153+
// The earliest (victim) document is archived, so the file is retired; the attacker's
154+
// later active document must not become the owner and grant cross-tenant access.
155+
dbChainMockFns.limit.mockResolvedValueOnce([
156+
{ workspaceId: 'ws-victim', fileUrl: internalUrlFor(VICTIM_KEY), archivedAt: new Date() },
157+
{ workspaceId: 'ws-attacker', fileUrl: internalUrlFor(VICTIM_KEY) },
158+
])
159+
mockGetUserEntityPermissions.mockResolvedValue('admin')
160+
161+
const granted = await verifyFileAccess(VICTIM_KEY, ATTACKER_USER, undefined, 'knowledge-base')
162+
163+
expect(granted).toBe(false)
164+
expect(mockGetUserEntityPermissions).not.toHaveBeenCalled()
165+
})
166+
167+
it('denies access when the owning document is soft-deleted (retired file not served)', async () => {
168+
dbChainMockFns.limit.mockResolvedValueOnce([
169+
{ workspaceId: 'ws-owner', fileUrl: internalUrlFor(VICTIM_KEY), deletedAt: new Date() },
170+
])
171+
mockGetUserEntityPermissions.mockResolvedValue('admin')
172+
173+
const granted = await verifyFileAccess(VICTIM_KEY, 'owner-user', undefined, 'knowledge-base')
174+
175+
expect(granted).toBe(false)
176+
expect(mockGetUserEntityPermissions).not.toHaveBeenCalled()
177+
})
178+
152179
it('denies access when a substring document points at a different key', async () => {
153180
dbChainMockFns.limit.mockResolvedValueOnce([
154181
{ workspaceId: 'ws-attacker', fileUrl: internalUrlFor(`${VICTIM_KEY}-decoy`) },

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

Lines changed: 30 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -432,49 +432,62 @@ function resolveInternalKbKey(fileUrl: string | null, allowedOrigins: Set<string
432432
* KB files: kb/filename
433433
*
434434
* Access is authorized against the workspace that *owns* the storage object,
435-
* never against an arbitrary document that merely references it. Ownership is
436-
* resolved by requiring a document's `fileUrl` to canonically resolve to the
437-
* exact requested storage key (not a substring/`LIKE` match), and by pinning to
438-
* the earliest such document — so a later document planted in another workspace
439-
* cannot authorize the planting user against another tenant's file.
435+
* never against an arbitrary document that merely references it. The owner is the
436+
* earliest document (in any state) whose `fileUrl` canonically resolves to the
437+
* exact requested key — not a substring/`LIKE` match. Access is granted only when
438+
* that owning document is still active; an archived or deleted owner keeps the file
439+
* retired, and a document planted later in another workspace can never become the
440+
* owner.
440441
*/
441442
async function verifyKBFileAccess(
442443
cloudKey: string,
443444
userId: string,
444445
customConfig?: StorageConfig
445446
): Promise<boolean> {
446447
try {
447-
// LIKE only narrows candidates; ownership is decided below, pinned to the earliest upload.
448+
// Ownership spans every document state: the earliest document to reference the key owns
449+
// it, so a document planted later in another workspace can never claim a file whose
450+
// original document was archived or deleted. LIKE only narrows candidates; the exact
451+
// owner is resolved below.
448452
const candidateDocuments = await db
449453
.select({
450454
workspaceId: knowledgeBase.workspaceId,
451455
fileUrl: document.fileUrl,
456+
archivedAt: document.archivedAt,
457+
deletedAt: document.deletedAt,
458+
userExcluded: document.userExcluded,
459+
kbDeletedAt: knowledgeBase.deletedAt,
452460
})
453461
.from(document)
454462
.innerJoin(knowledgeBase, eq(document.knowledgeBaseId, knowledgeBase.id))
455463
.where(
456-
and(
457-
eq(document.userExcluded, false),
458-
isNull(document.archivedAt),
459-
isNull(document.deletedAt),
460-
isNull(knowledgeBase.deletedAt),
461-
or(
462-
like(document.fileUrl, `%${cloudKey}%`),
463-
like(document.fileUrl, `%${encodeURIComponent(cloudKey)}%`)
464-
)
464+
or(
465+
like(document.fileUrl, `%${cloudKey}%`),
466+
like(document.fileUrl, `%${encodeURIComponent(cloudKey)}%`)
465467
)
466468
)
467469
.orderBy(asc(document.uploadedAt))
468470
.limit(50)
469471

470-
// Owner is the earliest document whose fileUrl resolves to EXACTLY this key; substring
471-
// matches and cross-workspace references never establish ownership.
472472
const allowedOrigins = getInternalServeOrigins()
473473
const owningDocument = candidateDocuments.find(
474474
(doc) => resolveInternalKbKey(doc.fileUrl, allowedOrigins) === cloudKey
475475
)
476476

477477
if (owningDocument) {
478+
const isActive =
479+
!owningDocument.archivedAt &&
480+
!owningDocument.deletedAt &&
481+
!owningDocument.userExcluded &&
482+
!owningDocument.kbDeletedAt
483+
484+
// The owning document is archived/deleted/excluded: the file is retired and not served,
485+
// and the attacker's later active document is not the owner, so it cannot claim it.
486+
if (!isActive) {
487+
logger.warn('KB file access denied: owning document is not active', { userId, cloudKey })
488+
return false
489+
}
490+
478491
if (!owningDocument.workspaceId) {
479492
logger.warn('KB file access denied: owning document has no workspace', {
480493
userId,

0 commit comments

Comments
 (0)