Skip to content

Commit ed19b0b

Browse files
fix(misc): keep block-tool params selected across store replace, perms parity for delete (#4840)
* fix(tool-input): keep block-tool params selected across store replace * fix tests and extract kb ownership helper
1 parent fc7e35e commit ed19b0b

25 files changed

Lines changed: 505 additions & 90 deletions

File tree

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

Lines changed: 37 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,21 @@ interface AuthorizationResult {
2828
workspaceId?: string
2929
}
3030

31+
type WorkspacePermission = 'read' | 'write' | 'admin'
32+
33+
/**
34+
* Whether a resolved workspace permission satisfies a file operation. Read and
35+
* download paths accept any membership; destructive operations (`requireWrite`)
36+
* require write or admin, matching the permission needed to create the file.
37+
*/
38+
function workspacePermissionSatisfies(
39+
permission: WorkspacePermission | null,
40+
requireWrite: boolean
41+
): boolean {
42+
if (permission === null) return false
43+
return requireWrite ? permission === 'write' || permission === 'admin' : true
44+
}
45+
3146
/**
3247
* Lookup workspace file by storage key from database
3348
* @param key Storage key to lookup
@@ -117,11 +132,13 @@ export async function verifyFileAccess(
117132
userId: string,
118133
customConfig?: StorageConfig,
119134
context?: StorageContext | 'general',
120-
isLocal?: boolean
135+
isLocal?: boolean,
136+
options?: { requireWrite?: boolean }
121137
): Promise<boolean> {
138+
const requireWrite = options?.requireWrite ?? false
122139
try {
123140
if (context === 'general') {
124-
return await verifyRegularFileAccess(cloudKey, userId, customConfig, isLocal)
141+
return await verifyRegularFileAccess(cloudKey, userId, customConfig, isLocal, requireWrite)
125142
}
126143

127144
// Infer context from key if not explicitly provided
@@ -139,12 +156,12 @@ export async function verifyFileAccess(
139156

140157
// 1. Workspace / mothership files: Check database first (most reliable for both local and cloud)
141158
if (inferredContext === 'workspace' || inferredContext === 'mothership') {
142-
return await verifyWorkspaceFileAccess(cloudKey, userId, customConfig, isLocal)
159+
return await verifyWorkspaceFileAccess(cloudKey, userId, customConfig, isLocal, requireWrite)
143160
}
144161

145162
// 2. Execution files: workspace_id/workflow_id/execution_id/filename
146163
if (inferredContext === 'execution') {
147-
return await verifyExecutionFileAccess(cloudKey, userId, customConfig)
164+
return await verifyExecutionFileAccess(cloudKey, userId, customConfig, requireWrite)
148165
}
149166

150167
// 3. Copilot files: Check database first, then metadata, then path pattern (legacy)
@@ -159,12 +176,12 @@ export async function verifyFileAccess(
159176

160177
// 5. Chat files: chat/filename
161178
if (inferredContext === 'chat') {
162-
return await verifyChatFileAccess(cloudKey, userId, customConfig)
179+
return await verifyChatFileAccess(cloudKey, userId, customConfig, requireWrite)
163180
}
164181

165182
// 6. Regular uploads: UUID-filename or timestamp-filename
166183
// Check metadata for userId/workspaceId, or database for workspace files
167-
return await verifyRegularFileAccess(cloudKey, userId, customConfig, isLocal)
184+
return await verifyRegularFileAccess(cloudKey, userId, customConfig, isLocal, requireWrite)
168185
} catch (error) {
169186
logger.error('Error verifying file access:', { cloudKey, userId, error })
170187
// Deny access on error to be safe
@@ -180,7 +197,8 @@ async function verifyWorkspaceFileAccess(
180197
cloudKey: string,
181198
userId: string,
182199
customConfig?: StorageConfig,
183-
isLocal?: boolean
200+
isLocal?: boolean,
201+
requireWrite = false
184202
): Promise<boolean> {
185203
try {
186204
const anyWorkspaceFileRecord = await getFileMetadataByKey(cloudKey, 'workspace', {
@@ -202,7 +220,7 @@ async function verifyWorkspaceFileAccess(
202220
'workspace',
203221
workspaceFileRecord.workspaceId
204222
)
205-
if (permission !== null) {
223+
if (workspacePermissionSatisfies(permission, requireWrite)) {
206224
logger.debug('Workspace file access granted (database lookup)', {
207225
userId,
208226
workspaceId: workspaceFileRecord.workspaceId,
@@ -225,7 +243,7 @@ async function verifyWorkspaceFileAccess(
225243

226244
if (workspaceId) {
227245
const permission = await getUserEntityPermissions(userId, 'workspace', workspaceId)
228-
if (permission !== null) {
246+
if (workspacePermissionSatisfies(permission, requireWrite)) {
229247
logger.debug('Workspace file access granted (metadata)', {
230248
userId,
231249
workspaceId,
@@ -257,7 +275,8 @@ async function verifyWorkspaceFileAccess(
257275
async function verifyExecutionFileAccess(
258276
cloudKey: string,
259277
userId: string,
260-
customConfig?: StorageConfig
278+
customConfig?: StorageConfig,
279+
requireWrite = false
261280
): Promise<boolean> {
262281
const parts = cloudKey.split('/')
263282

@@ -285,7 +304,7 @@ async function verifyExecutionFileAccess(
285304
}
286305

287306
const permission = await getUserEntityPermissions(userId, 'workspace', workspaceId)
288-
if (permission === null) {
307+
if (!workspacePermissionSatisfies(permission, requireWrite)) {
289308
logger.warn('User does not have workspace access for execution file', {
290309
userId,
291310
workspaceId,
@@ -502,7 +521,8 @@ export async function verifyKBFileWriteAccess(cloudKey: string, userId: string):
502521
async function verifyChatFileAccess(
503522
cloudKey: string,
504523
userId: string,
505-
customConfig?: StorageConfig
524+
customConfig?: StorageConfig,
525+
requireWrite = false
506526
): Promise<boolean> {
507527
try {
508528
const config: StorageConfig = customConfig || (await getChatStorageConfig())
@@ -516,7 +536,7 @@ async function verifyChatFileAccess(
516536
}
517537

518538
const permission = await getUserEntityPermissions(userId, 'workspace', workspaceId)
519-
if (permission === null) {
539+
if (!workspacePermissionSatisfies(permission, requireWrite)) {
520540
logger.warn('User does not have workspace access for chat file', {
521541
userId,
522542
workspaceId,
@@ -542,7 +562,8 @@ async function verifyRegularFileAccess(
542562
cloudKey: string,
543563
userId: string,
544564
customConfig?: StorageConfig,
545-
isLocal?: boolean
565+
isLocal?: boolean,
566+
requireWrite = false
546567
): Promise<boolean> {
547568
try {
548569
// Priority 1: Check if this might be a workspace file (check database)
@@ -554,7 +575,7 @@ async function verifyRegularFileAccess(
554575
'workspace',
555576
workspaceFileRecord.workspaceId
556577
)
557-
if (permission !== null) {
578+
if (workspacePermissionSatisfies(permission, requireWrite)) {
558579
logger.debug('Regular file access granted (workspace file from database)', {
559580
userId,
560581
workspaceId: workspaceFileRecord.workspaceId,
@@ -589,7 +610,7 @@ async function verifyRegularFileAccess(
589610
// If file has workspaceId, verify workspace membership
590611
if (workspaceId) {
591612
const permission = await getUserEntityPermissions(userId, 'workspace', workspaceId)
592-
if (permission !== null) {
613+
if (workspacePermissionSatisfies(permission, requireWrite)) {
593614
logger.debug('Regular file access granted (workspace membership)', {
594615
userId,
595616
workspaceId,

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

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -64,8 +64,10 @@ export const POST = withRouteHandler(async (request: NextRequest) => {
6464

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

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.
67+
// Deletes require write/admin on the owning workspace (owner-scoped files
68+
// like copilot/regular uploads still authorize by ownership). KB deletes
69+
// are binding-only and never use the transitional read fallback that file
70+
// serving allows.
6971
const hasAccess =
7072
storageContext === 'knowledge-base'
7173
? await verifyKBFileWriteAccess(key, userId)
@@ -74,7 +76,8 @@ export const POST = withRouteHandler(async (request: NextRequest) => {
7476
userId,
7577
undefined, // customConfig
7678
storageContext, // context
77-
!hasCloudStorage() // isLocal
79+
!hasCloudStorage(), // isLocal
80+
{ requireWrite: true }
7881
)
7982

8083
if (!hasAccess) {

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

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@ import {
2424
type UploadTokenPayload,
2525
verifyUploadToken,
2626
} from '@/lib/uploads/core/upload-token'
27-
import { insertFileMetadata } from '@/lib/uploads/server/metadata'
27+
import { recordKnowledgeBaseFileOwnership } from '@/lib/uploads/server/metadata'
2828
import { QUOTA_EXEMPT_STORAGE_CONTEXTS, type StorageConfig } from '@/lib/uploads/shared/types'
2929
import { getUserEntityPermissions } from '@/lib/workspaces/permissions/utils'
3030

@@ -96,11 +96,10 @@ const recordKnowledgeBaseOwnership = async (
9696
if (payload.context !== 'knowledge-base' || !payload.workspaceId) {
9797
return
9898
}
99-
await insertFileMetadata({
99+
await recordKnowledgeBaseFileOwnership({
100100
key,
101101
userId: payload.userId,
102102
workspaceId: payload.workspaceId,
103-
context: 'knowledge-base',
104103
originalName: payload.fileName ?? key.split('/').pop() ?? key,
105104
contentType: payload.contentType ?? 'application/octet-stream',
106105
size: typeof payload.fileSize === 'number' ? payload.fileSize : 0,

apps/sim/app/api/files/presigned/batch/route.ts

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@ import {
1313
generateBatchPresignedUploadUrls,
1414
hasCloudStorage,
1515
} from '@/lib/uploads/core/storage-service'
16-
import { insertFileMetadataMany } from '@/lib/uploads/server/metadata'
16+
import { recordKnowledgeBaseFileOwnershipMany } from '@/lib/uploads/server/metadata'
1717
import { validateFileType } from '@/lib/uploads/utils/validation'
1818
import { getUserEntityPermissions } from '@/lib/workspaces/permissions/utils'
1919
import { createErrorResponse } from '@/app/api/files/utils'
@@ -151,12 +151,11 @@ export const POST = withRouteHandler(async (request: NextRequest) => {
151151

152152
if (uploadType === 'knowledge-base' && knowledgeBaseWorkspaceId) {
153153
const ownerWorkspaceId = knowledgeBaseWorkspaceId
154-
await insertFileMetadataMany(
154+
await recordKnowledgeBaseFileOwnershipMany(
155155
presignedUrls.map((urlResponse, index) => ({
156156
key: urlResponse.key,
157157
userId: sessionUserId,
158158
workspaceId: ownerWorkspaceId,
159-
context: 'knowledge-base',
160159
originalName: files[index].fileName,
161160
contentType: files[index].contentType,
162161
size: files[index].fileSize,

apps/sim/app/api/files/presigned/route.test.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -93,6 +93,8 @@ vi.mock('@/lib/uploads/contexts/execution/utils', () => ({
9393

9494
vi.mock('@/lib/uploads/server/metadata', () => ({
9595
insertFileMetadata: mockInsertFileMetadata,
96+
recordKnowledgeBaseFileOwnership: (ownership: Record<string, unknown>) =>
97+
mockInsertFileMetadata({ ...ownership, context: 'knowledge-base' }),
9698
}))
9799

98100
vi.mock('@/lib/uploads/utils/file-utils', () => ({

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

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@ import { USE_BLOB_STORAGE } from '@/lib/uploads/config'
1111
import { generateExecutionFileKey } from '@/lib/uploads/contexts/execution/utils'
1212
import { generateWorkspaceFileKey } from '@/lib/uploads/contexts/workspace/workspace-file-manager'
1313
import { generatePresignedUploadUrl, hasCloudStorage } from '@/lib/uploads/core/storage-service'
14-
import { insertFileMetadata } from '@/lib/uploads/server/metadata'
14+
import { insertFileMetadata, recordKnowledgeBaseFileOwnership } from '@/lib/uploads/server/metadata'
1515
import { isImageFileType } from '@/lib/uploads/utils/file-utils'
1616
import { validateAttachmentFileType, validateFileType } from '@/lib/uploads/utils/validation'
1717
import { getUserEntityPermissions } from '@/lib/workspaces/permissions/utils'
@@ -278,11 +278,10 @@ export const POST = withRouteHandler(async (request: NextRequest) => {
278278
metadata: { workspaceId },
279279
})
280280

281-
await insertFileMetadata({
281+
await recordKnowledgeBaseFileOwnership({
282282
key: presignedUrlResponse.key,
283283
userId: sessionUserId,
284284
workspaceId,
285-
context: 'knowledge-base',
286285
originalName: fileName,
287286
contentType,
288287
size: fileSize,

apps/sim/app/api/folders/[id]/route.test.ts

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -465,22 +465,27 @@ describe('Individual Folder API Route', () => {
465465
expect(response.status).toBe(403)
466466

467467
const data = await response.json()
468-
expect(data).toHaveProperty('error', 'Admin access required to delete folders')
468+
expect(data).toHaveProperty('error', 'Write or Admin access required to delete folders')
469469
})
470470

471-
it('should return 403 when user has only write permissions for delete', async () => {
471+
it('should allow folder deletion for write permissions', async () => {
472472
mockAuthenticatedUser()
473473
mockGetUserEntityPermissions.mockResolvedValue('write')
474474

475+
mockDbRef.current = createFolderDbMock({
476+
folderLookupResult: mockFolder,
477+
})
478+
475479
const req = createMockRequest('DELETE')
476480
const params = Promise.resolve({ id: 'folder-1' })
477481

478482
const response = await DELETE(req, { params })
479483

480-
expect(response.status).toBe(403)
484+
expect(response.status).toBe(200)
481485

482486
const data = await response.json()
483-
expect(data).toHaveProperty('error', 'Admin access required to delete folders')
487+
expect(data).toHaveProperty('success', true)
488+
expect(mockPerformDeleteFolder).toHaveBeenCalled()
484489
})
485490

486491
it('should allow folder deletion for admin permissions', async () => {

apps/sim/app/api/folders/[id]/route.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -140,9 +140,9 @@ export const DELETE = withRouteHandler(
140140
existingFolder.workspaceId
141141
)
142142

143-
if (workspacePermission !== 'admin') {
143+
if (!workspacePermission || workspacePermission === 'read') {
144144
return NextResponse.json(
145-
{ error: 'Admin access required to delete folders' },
145+
{ error: 'Write or Admin access required to delete folders' },
146146
{ status: 403 }
147147
)
148148
}

apps/sim/app/api/knowledge/[id]/documents/[documentId]/chunks/[chunkId]/route.ts

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ import { parseRequest } from '@/lib/api/server'
66
import { getSession } from '@/lib/auth'
77
import { withRouteHandler } from '@/lib/core/utils/with-route-handler'
88
import { deleteChunk, updateChunk } from '@/lib/knowledge/chunks/service'
9-
import { checkChunkAccess } from '@/app/api/knowledge/utils'
9+
import { checkChunkAccess, checkChunkWriteAccess } from '@/app/api/knowledge/utils'
1010

1111
const logger = createLogger('ChunkByIdAPI')
1212

@@ -75,7 +75,7 @@ export const PUT = withRouteHandler(
7575
return NextResponse.json({ error: 'Unauthorized' }, { status: 401 })
7676
}
7777

78-
const accessCheck = await checkChunkAccess(
78+
const accessCheck = await checkChunkWriteAccess(
7979
knowledgeBaseId,
8080
documentId,
8181
chunkId,
@@ -147,7 +147,7 @@ export const DELETE = withRouteHandler(
147147
return NextResponse.json({ error: 'Unauthorized' }, { status: 401 })
148148
}
149149

150-
const accessCheck = await checkChunkAccess(
150+
const accessCheck = await checkChunkWriteAccess(
151151
knowledgeBaseId,
152152
documentId,
153153
chunkId,

0 commit comments

Comments
 (0)