Skip to content

Commit d0efae7

Browse files
committed
fix(tool-input): keep block-tool params selected across store replace
1 parent a7b0bd3 commit d0efae7

19 files changed

Lines changed: 444 additions & 76 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/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,

apps/sim/app/api/knowledge/utils.ts

Lines changed: 69 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -435,3 +435,72 @@ export async function checkChunkAccess(
435435
knowledgeBase: kbAccess.knowledgeBase!,
436436
}
437437
}
438+
439+
/**
440+
* Check if a user has write access to a chunk.
441+
*
442+
* Mirrors `checkChunkAccess` but requires write/admin on the knowledge base's
443+
* workspace (or KB ownership for legacy KBs), matching the permission needed to
444+
* create chunks. Used for chunk mutation (update and delete) so those operations
445+
* require the same permission as creation rather than read.
446+
*/
447+
export async function checkChunkWriteAccess(
448+
knowledgeBaseId: string,
449+
documentId: string,
450+
chunkId: string,
451+
userId: string
452+
): Promise<ChunkAccessCheck> {
453+
const kbAccess = await checkKnowledgeBaseWriteAccess(knowledgeBaseId, userId)
454+
455+
if (!kbAccess.hasAccess) {
456+
return {
457+
hasAccess: false,
458+
notFound: kbAccess.notFound,
459+
reason: kbAccess.notFound ? 'Knowledge base not found' : 'Unauthorized knowledge base access',
460+
}
461+
}
462+
463+
const doc = await db
464+
.select()
465+
.from(document)
466+
.where(
467+
and(
468+
eq(document.id, documentId),
469+
eq(document.knowledgeBaseId, knowledgeBaseId),
470+
eq(document.userExcluded, false),
471+
isNull(document.archivedAt),
472+
isNull(document.deletedAt)
473+
)
474+
)
475+
.limit(1)
476+
477+
if (doc.length === 0) {
478+
return { hasAccess: false, notFound: true, reason: 'Document not found' }
479+
}
480+
481+
const docData = doc[0] as DocumentData
482+
483+
if (docData.processingStatus !== 'completed') {
484+
return {
485+
hasAccess: false,
486+
reason: `Document is not ready for access (status: ${docData.processingStatus})`,
487+
}
488+
}
489+
490+
const chunk = await db
491+
.select()
492+
.from(embedding)
493+
.where(and(eq(embedding.id, chunkId), eq(embedding.documentId, documentId)))
494+
.limit(1)
495+
496+
if (chunk.length === 0) {
497+
return { hasAccess: false, notFound: true, reason: 'Chunk not found' }
498+
}
499+
500+
return {
501+
hasAccess: true,
502+
chunk: chunk[0] as EmbeddingData,
503+
document: docData,
504+
knowledgeBase: kbAccess.knowledgeBase!,
505+
}
506+
}

apps/sim/app/api/mcp/servers/route.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -134,10 +134,10 @@ export const POST = withRouteHandler(
134134
)
135135

136136
/**
137-
* DELETE - Delete an MCP server from the workspace (requires admin permission)
137+
* DELETE - Delete an MCP server from the workspace (requires write permission)
138138
*/
139139
export const DELETE = withRouteHandler(
140-
withMcpAuth('admin')(
140+
withMcpAuth('write')(
141141
async (request: NextRequest, { userId, userName, userEmail, workspaceId, requestId }) => {
142142
try {
143143
const { searchParams } = new URL(request.url)

apps/sim/app/api/mcp/workflow-servers/[id]/route.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -147,7 +147,7 @@ export const PATCH = withRouteHandler(
147147
* DELETE - Delete a workflow MCP server and all its tools
148148
*/
149149
export const DELETE = withRouteHandler(
150-
withMcpAuth<RouteParams>('admin')(
150+
withMcpAuth<RouteParams>('write')(
151151
async (
152152
request: NextRequest,
153153
{ userId, userName, userEmail, workspaceId, requestId },

apps/sim/app/api/schedules/route.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -220,8 +220,8 @@ export const POST = withRouteHandler(async (req: NextRequest) => {
220220
const { workspaceId, title, prompt, cronExpression, timezone, lifecycle, maxRuns, startDate } =
221221
parsed.data.body
222222

223-
const hasPermission = await verifyWorkspaceMembership(session.user.id, workspaceId)
224-
if (!hasPermission) {
223+
const permission = await verifyWorkspaceMembership(session.user.id, workspaceId)
224+
if (permission !== 'admin' && permission !== 'write') {
225225
return NextResponse.json({ error: 'Not authorized' }, { status: 403 })
226226
}
227227

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

Lines changed: 39 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -397,7 +397,42 @@ describe('Workflow By ID API Route', () => {
397397
expect(data.error).toBe('Cannot delete the only workflow in the workspace')
398398
})
399399

400-
it.concurrent('should deny deletion for non-admin users', async () => {
400+
it('should allow user with write permission to delete workflow', async () => {
401+
const mockWorkflow = {
402+
id: 'workflow-123',
403+
userId: 'other-user',
404+
name: 'Test Workflow',
405+
workspaceId: 'workspace-456',
406+
}
407+
408+
mockGetSession({ user: { id: 'user-123' } })
409+
410+
mockGetWorkflowById.mockResolvedValue(mockWorkflow)
411+
mockAuthorizeWorkflowByWorkspacePermission.mockResolvedValue({
412+
allowed: true,
413+
status: 200,
414+
workflow: mockWorkflow,
415+
workspacePermission: 'write',
416+
})
417+
418+
mockPerformDeleteWorkflow.mockResolvedValue({ success: true })
419+
420+
const req = new NextRequest('http://localhost:3000/api/workflows/workflow-123', {
421+
method: 'DELETE',
422+
})
423+
const params = Promise.resolve({ id: 'workflow-123' })
424+
425+
const response = await DELETE(req, { params })
426+
427+
expect(response.status).toBe(200)
428+
const data = await response.json()
429+
expect(data.success).toBe(true)
430+
expect(mockAuthorizeWorkflowByWorkspacePermission).toHaveBeenCalledWith(
431+
expect.objectContaining({ workflowId: 'workflow-123', action: 'write' })
432+
)
433+
})
434+
435+
it.concurrent('should deny deletion for read-only users', async () => {
401436
const mockWorkflow = {
402437
id: 'workflow-123',
403438
userId: 'other-user',
@@ -411,9 +446,9 @@ describe('Workflow By ID API Route', () => {
411446
mockAuthorizeWorkflowByWorkspacePermission.mockResolvedValue({
412447
allowed: false,
413448
status: 403,
414-
message: 'Unauthorized: Access denied to admin this workflow',
449+
message: 'Unauthorized: Access denied to write this workflow',
415450
workflow: mockWorkflow,
416-
workspacePermission: null,
451+
workspacePermission: 'read',
417452
})
418453

419454
const req = new NextRequest('http://localhost:3000/api/workflows/workflow-123', {
@@ -425,7 +460,7 @@ describe('Workflow By ID API Route', () => {
425460

426461
expect(response.status).toBe(403)
427462
const data = await response.json()
428-
expect(data.error).toBe('Unauthorized: Access denied to admin this workflow')
463+
expect(data.error).toBe('Unauthorized: Access denied to write this workflow')
429464
})
430465
})
431466

0 commit comments

Comments
 (0)