Skip to content

Commit a5dda7c

Browse files
committed
fix(authz): enforce credential and workspace boundaries
1 parent 7f39678 commit a5dda7c

11 files changed

Lines changed: 225 additions & 37 deletions

File tree

apps/sim/app/api/mcp/servers/[id]/refresh/route.test.ts

Lines changed: 16 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -5,31 +5,33 @@ import { dbChainMockFns, resetDbChainMock } from '@sim/testing'
55
import type { NextRequest } from 'next/server'
66
import { afterAll, beforeEach, describe, expect, it, vi } from 'vitest'
77

8-
const { mockClearCache, mockDiscoverServerTools } = vi.hoisted(() => ({
8+
const { mockClearCache, mockDiscoverServerTools, requiredRoles } = vi.hoisted(() => ({
99
mockClearCache: vi.fn(),
1010
mockDiscoverServerTools: vi.fn(),
11+
requiredRoles: [] as string[],
1112
}))
1213

1314
vi.mock('@/lib/core/utils/with-route-handler', () => ({
1415
withRouteHandler: (handler: unknown) => handler,
1516
}))
1617

1718
vi.mock('@/lib/mcp/middleware', () => ({
18-
withMcpAuth:
19-
() =>
20-
(
19+
withMcpAuth: (requiredRole: string) => {
20+
requiredRoles.push(requiredRole)
21+
return (
2122
handler: (
2223
request: NextRequest,
2324
context: { userId: string; workspaceId: string; requestId: string },
2425
routeContext: { params: Promise<{ id: string }> }
2526
) => Promise<Response>
2627
) =>
27-
(request: NextRequest, routeContext: { params: Promise<{ id: string }> }) =>
28-
handler(
29-
request,
30-
{ userId: 'user-1', workspaceId: 'workspace-1', requestId: 'request-1' },
31-
routeContext
32-
),
28+
(request: NextRequest, routeContext: { params: Promise<{ id: string }> }) =>
29+
handler(
30+
request,
31+
{ userId: 'user-1', workspaceId: 'workspace-1', requestId: 'request-1' },
32+
routeContext
33+
)
34+
},
3335
}))
3436

3537
vi.mock('@/lib/mcp/service', () => ({
@@ -72,6 +74,10 @@ describe('MCP server refresh route', () => {
7274
resetDbChainMock()
7375
})
7476

77+
it('requires workspace write permission because refresh persists workflow changes', () => {
78+
expect(requiredRoles).toEqual(['write'])
79+
})
80+
7581
it('preserves the service-persisted OAuth pending status', async () => {
7682
mockDiscoverServerTools.mockRejectedValueOnce(new Error('OAuth authorization required'))
7783

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -158,7 +158,7 @@ async function syncToolSchemasToWorkflows(
158158
}
159159

160160
export const POST = withRouteHandler(
161-
withMcpAuth<{ id: string }>('read')(
161+
withMcpAuth<{ id: string }>('write')(
162162
async (request: NextRequest, { userId, workspaceId, requestId }, { params }) => {
163163
try {
164164
const paramsValidation = mcpServerIdParamsSchema.safeParse(await params)

apps/sim/ee/workspace-forking/lib/create-fork.test.ts

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -91,7 +91,7 @@ vi.mock('@/lib/workspaces/policy', () => ({
9191

9292
import { createFork } from '@/ee/workspace-forking/lib/create-fork'
9393

94-
const SOURCE = { id: 'src-ws', name: 'Parent' } as never
94+
const SOURCE = { id: 'src-ws', name: 'Parent', allowPersonalApiKeys: false } as never
9595
const POLICY = {
9696
organizationId: null,
9797
workspaceMode: 'personal',
@@ -210,6 +210,15 @@ describe('createFork storage headroom gate', () => {
210210
)
211211
})
212212

213+
it('preserves the source workspace personal API-key policy in the child', async () => {
214+
const result = await createFork(forkParams())
215+
216+
expect(result.workspace.allowPersonalApiKeys).toBe(false)
217+
expect(dbChainMockFns.values).toHaveBeenCalledWith(
218+
expect.objectContaining({ allowPersonalApiKeys: false })
219+
)
220+
})
221+
213222
it('seeds identity mappings for copied FILES by storage key (a later sync must not re-offer them)', async () => {
214223
mockPlanForkFileCopies.mockResolvedValue({
215224
keyMap: new Map([['workspace/src-ws/a.png', 'workspace/child/a.png']]),

apps/sim/ee/workspace-forking/lib/create-fork.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -170,7 +170,7 @@ export async function createFork(params: CreateForkParams): Promise<CreateForkRe
170170
organizationId: policy.organizationId,
171171
workspaceMode: policy.workspaceMode,
172172
billedAccountUserId: policy.billedAccountUserId,
173-
allowPersonalApiKeys: true,
173+
allowPersonalApiKeys: source.allowPersonalApiKeys,
174174
forkedFromWorkspaceId: source.id,
175175
createdAt: now,
176176
updatedAt: now,
@@ -418,7 +418,7 @@ export async function createFork(params: CreateForkParams): Promise<CreateForkRe
418418
organizationId: policy.organizationId,
419419
workspaceMode: policy.workspaceMode,
420420
billedAccountUserId: policy.billedAccountUserId,
421-
allowPersonalApiKeys: true,
421+
allowPersonalApiKeys: source.allowPersonalApiKeys,
422422
forkedFromWorkspaceId: source.id,
423423
},
424424
workflowsCopied,

apps/sim/lib/knowledge/application/connectors.test.ts

Lines changed: 127 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,8 @@ const mocks = vi.hoisted(() => ({
1515
deleteConnector: vi.fn(),
1616
syncConnector: vi.fn(),
1717
resolveBilling: vi.fn(),
18+
getCredentialActorContext: vi.fn(),
19+
canUseCredential: vi.fn(),
1820
resolveTokenIdentity: vi.fn(),
1921
refreshToken: vi.fn(),
2022
validateConnectorConfig: vi.fn(),
@@ -56,6 +58,8 @@ vi.mock('@/lib/knowledge/orchestration/connectors', () => ({
5658
}))
5759

5860
vi.mock('@/lib/credentials/access', () => ({
61+
getCredentialActorContext: mocks.getCredentialActorContext,
62+
canUseCredential: mocks.canUseCredential,
5963
resolveCredentialTokenIdentity: mocks.resolveTokenIdentity,
6064
}))
6165

@@ -120,6 +124,17 @@ describe('knowledge connector application use cases', () => {
120124
mocks.resolvePermission.mockResolvedValue('write')
121125
mocks.resolveKnowledgeBase.mockResolvedValue(crossWorkspaceContext)
122126
mocks.resolveConnector.mockResolvedValue(connectorContext)
127+
mocks.getCredentialActorContext.mockResolvedValue({
128+
credential: { id: 'credential-1', workspaceId: 'workspace-a' },
129+
member: { role: 'member' },
130+
hasWorkspaceAccess: true,
131+
canWriteWorkspace: true,
132+
isAdmin: false,
133+
})
134+
mocks.canUseCredential.mockImplementation(
135+
(access: { hasWorkspaceAccess: boolean; member: unknown; isAdmin: boolean }) =>
136+
access.hasWorkspaceAccess && (Boolean(access.member) || access.isAdmin)
137+
)
123138
mocks.resolveTokenIdentity.mockResolvedValue({ kind: 'oauth', userId: 'credential-owner' })
124139
mocks.refreshToken.mockResolvedValue('access-token')
125140
mocks.validateConnectorConfig.mockResolvedValue({ valid: true })
@@ -296,6 +311,7 @@ describe('knowledge connector application use cases', () => {
296311
expect(mocks.resolvePermission.mock.invocationCallOrder[0]).toBeLessThan(
297312
mocks.updateConnector.mock.invocationCallOrder[0]
298313
)
314+
expect(mocks.getCredentialActorContext).toHaveBeenCalledWith('credential-1', 'shared-user')
299315
expect(mocks.resolveTokenIdentity).toHaveBeenCalledWith('credential-1', 'workspace-a')
300316
expect(mocks.refreshToken).toHaveBeenCalledWith(
301317
'credential-1',
@@ -305,6 +321,117 @@ describe('knowledge connector application use cases', () => {
305321
expect(mocks.validateConnectorConfig).toHaveBeenCalledWith('access-token', { space: 'ENG' })
306322
})
307323

324+
it('rejects connector creation when the writer cannot use the workspace credential', async () => {
325+
const sameWorkspaceContext = {
326+
...connectorContext,
327+
workspaceId: 'workspace-a',
328+
knowledgeBaseId: 'knowledge-a',
329+
knowledgeBase: { id: 'knowledge-a', name: 'Workspace A docs' },
330+
connector: { ...connectorContext.connector, knowledgeBaseId: 'knowledge-a' },
331+
}
332+
mocks.resolveKnowledgeBase.mockResolvedValueOnce(sameWorkspaceContext)
333+
mocks.getCredentialActorContext.mockResolvedValueOnce({
334+
credential: { id: 'credential-1', workspaceId: 'workspace-a' },
335+
member: null,
336+
hasWorkspaceAccess: true,
337+
canWriteWorkspace: true,
338+
isAdmin: false,
339+
})
340+
mocks.createConnector.mockImplementationOnce(
341+
async (input: { resolveAccessToken: (credentialId: string) => Promise<string | null> }) => {
342+
const accessToken = await input.resolveAccessToken('credential-1')
343+
return accessToken
344+
? { success: true, connector: sameWorkspaceContext.connector }
345+
: {
346+
success: false,
347+
error: 'Credential has no access token. Please reconnect your account.',
348+
errorCode: 'validation',
349+
}
350+
}
351+
)
352+
353+
await expect(
354+
createKnowledgeConnector.execute({
355+
principal: delegatedPrincipal,
356+
input: {
357+
knowledgeBaseId: 'knowledge-a',
358+
assertedWorkspaceId: 'workspace-a',
359+
connectorType: 'confluence',
360+
credentialId: 'credential-1',
361+
sourceConfig: {},
362+
syncIntervalMinutes: 1440,
363+
resolveBillingAttribution: mocks.resolveBilling,
364+
},
365+
})
366+
).rejects.toMatchObject({ code: 'validation' })
367+
368+
expect(mocks.getCredentialActorContext).toHaveBeenCalledWith('credential-1', 'shared-user')
369+
expect(mocks.resolveTokenIdentity).not.toHaveBeenCalled()
370+
expect(mocks.refreshToken).not.toHaveBeenCalled()
371+
})
372+
373+
it('rejects source-config revalidation after credential membership is removed', async () => {
374+
const sameWorkspaceContext = {
375+
...connectorContext,
376+
workspaceId: 'workspace-a',
377+
knowledgeBaseId: 'knowledge-a',
378+
knowledgeBase: { id: 'knowledge-a', name: 'Workspace A docs' },
379+
connector: { ...connectorContext.connector, knowledgeBaseId: 'knowledge-a' },
380+
}
381+
mocks.resolveConnector.mockResolvedValueOnce(sameWorkspaceContext)
382+
mocks.updateConnector.mockResolvedValueOnce({
383+
success: true,
384+
connector: { ...sameWorkspaceContext.connector, sourceConfig: { space: 'ENG' } },
385+
})
386+
387+
await updateKnowledgeConnector.execute({
388+
principal: delegatedPrincipal,
389+
input: {
390+
connectorId: 'connector-b',
391+
assertedWorkspaceId: 'workspace-a',
392+
updates: { sourceConfig: { space: 'ENG' } },
393+
},
394+
})
395+
396+
const orchestrationInput = mocks.updateConnector.mock.calls[0]?.[0] as {
397+
validateSourceConfig?: (
398+
connector: {
399+
connectorType: string
400+
credentialId: string
401+
encryptedApiKey: null
402+
},
403+
sourceConfig: Record<string, unknown>
404+
) => Promise<unknown>
405+
}
406+
if (!orchestrationInput.validateSourceConfig) {
407+
throw new Error('Application command did not provide source-config validation')
408+
}
409+
mocks.getCredentialActorContext.mockResolvedValueOnce({
410+
credential: { id: 'credential-1', workspaceId: 'workspace-a' },
411+
member: null,
412+
hasWorkspaceAccess: true,
413+
canWriteWorkspace: true,
414+
isAdmin: false,
415+
})
416+
417+
await expect(
418+
orchestrationInput.validateSourceConfig(
419+
{
420+
connectorType: 'confluence',
421+
credentialId: 'credential-1',
422+
encryptedApiKey: null,
423+
},
424+
{ space: 'ENG' }
425+
)
426+
).resolves.toEqual({
427+
message: 'Credential is no longer usable in this workspace. Please reconnect it.',
428+
errorCode: 'validation',
429+
})
430+
expect(mocks.resolveTokenIdentity).not.toHaveBeenCalled()
431+
expect(mocks.refreshToken).not.toHaveBeenCalled()
432+
expect(mocks.validateConnectorConfig).not.toHaveBeenCalled()
433+
})
434+
308435
it.each([
309436
[
310437
'create',

apps/sim/lib/knowledge/application/connectors.ts

Lines changed: 27 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,11 @@ import { decryptApiKey } from '@/lib/api-key/crypto'
66
import type { BillingAttributionSnapshot } from '@/lib/billing/core/billing-attribution'
77
import { OrchestrationError } from '@/lib/core/orchestration/types'
88
import { generateRequestId } from '@/lib/core/utils/request'
9-
import { resolveCredentialTokenIdentity } from '@/lib/credentials/access'
9+
import {
10+
canUseCredential,
11+
getCredentialActorContext,
12+
resolveCredentialTokenIdentity,
13+
} from '@/lib/credentials/access'
1014
import { defineAuthorizedKnowledgeUseCase } from '@/lib/knowledge/application/authorized-knowledge-use-case'
1115
import { resolveKnowledgeAttributedUserId } from '@/lib/knowledge/application/billing'
1216
import {
@@ -116,13 +120,29 @@ function requireConnectorWorkspaceId(context: ActiveKnowledgeResourceBaseContext
116120
return context.workspaceId
117121
}
118122

123+
async function resolveAuthorizedConnectorCredentialIdentity(input: {
124+
credentialId: string
125+
workspaceId: string
126+
actingUserId: string
127+
}) {
128+
const access = await getCredentialActorContext(input.credentialId, input.actingUserId)
129+
if (
130+
!access.credential ||
131+
access.credential.workspaceId !== input.workspaceId ||
132+
!canUseCredential(access)
133+
) {
134+
return null
135+
}
136+
return resolveCredentialTokenIdentity(input.credentialId, input.workspaceId)
137+
}
138+
119139
async function resolveConnectorCredentialAccessToken(input: {
120140
credentialId: string
121141
workspaceId: string
122142
actingUserId: string
123143
requestId: string
124144
}): Promise<string | null> {
125-
const identity = await resolveCredentialTokenIdentity(input.credentialId, input.workspaceId)
145+
const identity = await resolveAuthorizedConnectorCredentialIdentity(input)
126146
if (!identity) return null
127147
return refreshAccessTokenIfNeeded(
128148
input.credentialId,
@@ -163,10 +183,11 @@ async function validateConnectorSourceConfig(input: {
163183
errorCode: 'validation',
164184
}
165185
}
166-
const identity = await resolveCredentialTokenIdentity(
167-
input.connector.credentialId,
168-
input.workspaceId
169-
)
186+
const identity = await resolveAuthorizedConnectorCredentialIdentity({
187+
credentialId: input.connector.credentialId,
188+
workspaceId: input.workspaceId,
189+
actingUserId: input.actingUserId,
190+
})
170191
if (!identity) {
171192
return {
172193
message: 'Credential is no longer usable in this workspace. Please reconnect it.',

0 commit comments

Comments
 (0)