Skip to content

Commit 0f9fafc

Browse files
committed
fix(mcp): keep public workflow MCP servers admin-only
Third instance of the same class, found by Cursor Bugbot. A public workflow MCP server skips authentication entirely on the serve path (api/mcp/serve/[serverId] returns early when isPublic), so anyone with the URL can invoke every workflow published on it — the same unauthenticated exposure already kept admin-only for the public workflow API and public chats. create_server and update_server had moved to write with no secondary gate on the transition to public. Renames the helper to canExposePublicly and moves it to lib/deployments/public-exposure: it now governs four surfaces (workflow public API via its operation, chat REST, chat copilot, MCP servers), so a chat-specific name and home no longer described it. Adds a behavioral test for the MCP path, verified to fail when the gate is removed. Also drops a cross-surface test added in this round that grepped source text — it passed with the gate deleted, which makes it worse than no test.
1 parent a8fd8b1 commit 0f9fafc

11 files changed

Lines changed: 216 additions & 59 deletions

File tree

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

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -25,12 +25,12 @@ import { afterAll, beforeAll, beforeEach, describe, expect, it, vi } from 'vites
2525

2626
const {
2727
mockCheckChatAccess,
28-
mockCanSetPublicChatAuth,
28+
mockCanExposePublicly,
2929
mockCheckNeedsRedeployment,
3030
mockValidateChatDeployAuth,
3131
} = vi.hoisted(() => ({
3232
mockCheckChatAccess: vi.fn(),
33-
mockCanSetPublicChatAuth: vi.fn(),
33+
mockCanExposePublicly: vi.fn(),
3434
mockCheckNeedsRedeployment: vi.fn(),
3535
mockValidateChatDeployAuth: vi.fn(),
3636
}))
@@ -51,8 +51,8 @@ vi.mock('@/lib/core/security/encryption', () => encryptionMock)
5151
vi.mock('@/app/api/chat/utils', () => ({
5252
checkChatAccess: mockCheckChatAccess,
5353
}))
54-
vi.mock('@/lib/chat/permissions', () => ({
55-
canSetPublicChatAuth: mockCanSetPublicChatAuth,
54+
vi.mock('@/lib/deployments/public-exposure', () => ({
55+
canExposePublicly: mockCanExposePublicly,
5656
}))
5757

5858
vi.mock('@/ee/access-control/utils/permission-check', () => {
@@ -88,7 +88,7 @@ describe('Chat Edit API Route', () => {
8888
vi.clearAllMocks()
8989
// Existing chat suites deploy with authType public; default to admin so they
9090
// keep testing what they were written to test.
91-
mockCanSetPublicChatAuth.mockResolvedValue(true)
91+
mockCanExposePublicly.mockResolvedValue(true)
9292
resetDbChainMock()
9393
mockPerformChatUndeploy.mockResolvedValue({ success: true })
9494

apps/sim/app/api/chat/manage/[id]/route.ts

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -8,11 +8,11 @@ import type { NextRequest } from 'next/server'
88
import { chatIdParamsSchema, updateChatContract } from '@/lib/api/contracts/chats'
99
import { getValidationErrorMessage, parseRequest } from '@/lib/api/server'
1010
import { getSession } from '@/lib/auth'
11-
import { canSetPublicChatAuth } from '@/lib/chat/permissions'
1211
import { isDev } from '@/lib/core/config/env-flags'
1312
import { encryptSecret } from '@/lib/core/security/encryption'
1413
import { getEmailDomain } from '@/lib/core/utils/urls'
1514
import { withRouteHandler } from '@/lib/core/utils/with-route-handler'
15+
import { canExposePublicly } from '@/lib/deployments/public-exposure'
1616
import { checkNeedsRedeployment } from '@/lib/workflows/deployment-status'
1717
import {
1818
getWorkflowDeploymentSummary,
@@ -130,10 +130,7 @@ export const PATCH = withRouteHandler(
130130
if (authType && authType !== existingChatRecord.authType && chatWorkspaceId) {
131131
// Only the transition *to* public is admin-gated. Leaving an already-public
132132
// chat as-is, or moving it off public, does not increase exposure.
133-
if (
134-
authType === 'public' &&
135-
!(await canSetPublicChatAuth(session.user.id, chatWorkspaceId))
136-
) {
133+
if (authType === 'public' && !(await canExposePublicly(session.user.id, chatWorkspaceId))) {
137134
return createErrorResponse('Only admins can make a chat public', 403)
138135
}
139136

apps/sim/app/api/chat/route.test.ts

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -18,11 +18,11 @@ import { afterAll, beforeEach, describe, expect, it, vi } from 'vitest'
1818

1919
const {
2020
mockCheckWorkflowAccessForChatCreation,
21-
mockCanSetPublicChatAuth,
21+
mockCanExposePublicly,
2222
mockValidateChatDeployAuth,
2323
} = vi.hoisted(() => ({
2424
mockCheckWorkflowAccessForChatCreation: vi.fn(),
25-
mockCanSetPublicChatAuth: vi.fn(),
25+
mockCanExposePublicly: vi.fn(),
2626
mockValidateChatDeployAuth: vi.fn(),
2727
}))
2828

@@ -36,8 +36,8 @@ vi.mock('@/app/api/chat/utils', () => ({
3636
checkWorkflowAccessForChatCreation: mockCheckWorkflowAccessForChatCreation,
3737
}))
3838

39-
vi.mock('@/lib/chat/permissions', () => ({
40-
canSetPublicChatAuth: mockCanSetPublicChatAuth,
39+
vi.mock('@/lib/deployments/public-exposure', () => ({
40+
canExposePublicly: mockCanExposePublicly,
4141
}))
4242

4343
vi.mock('@/ee/access-control/utils/permission-check', () => {
@@ -64,7 +64,7 @@ describe('Chat API Route', () => {
6464
vi.clearAllMocks()
6565
// Existing chat suites deploy with authType public; default to admin so they
6666
// keep testing what they were written to test.
67-
mockCanSetPublicChatAuth.mockResolvedValue(true)
67+
mockCanExposePublicly.mockResolvedValue(true)
6868
setEnv({ NODE_ENV: 'development', NEXT_PUBLIC_APP_URL: 'http://localhost:3000' })
6969

7070
mockCreateSuccessResponse.mockImplementation((data) => {
@@ -274,7 +274,7 @@ describe('Chat API Route', () => {
274274
workflow: { userId: 'user-id', workspaceId: 'workspace-1', isDeployed: true },
275275
})
276276
// Write access is enough to deploy a chat, but not to make one public.
277-
mockCanSetPublicChatAuth.mockResolvedValue(false)
277+
mockCanExposePublicly.mockResolvedValue(false)
278278

279279
const response = await POST(
280280
new NextRequest('http://localhost:3000/api/chat', {
@@ -290,7 +290,7 @@ describe('Chat API Route', () => {
290290
)
291291

292292
expect(response.status).toBe(403)
293-
expect(mockCanSetPublicChatAuth).toHaveBeenCalledWith('user-id', 'workspace-1')
293+
expect(mockCanExposePublicly).toHaveBeenCalledWith('user-id', 'workspace-1')
294294
})
295295

296296
it('lets a non-admin deploy a password-protected chat', async () => {
@@ -303,7 +303,7 @@ describe('Chat API Route', () => {
303303
hasAccess: true,
304304
workflow: { userId: 'user-id', workspaceId: 'workspace-1', isDeployed: true },
305305
})
306-
mockCanSetPublicChatAuth.mockResolvedValue(false)
306+
mockCanExposePublicly.mockResolvedValue(false)
307307

308308
const response = await POST(
309309
new NextRequest('http://localhost:3000/api/chat', {

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,8 +7,8 @@ import type { NextRequest } from 'next/server'
77
import { createChatContract } from '@/lib/api/contracts/chats'
88
import { getValidationErrorMessage, parseRequest } from '@/lib/api/server'
99
import { getSession } from '@/lib/auth'
10-
import { canSetPublicChatAuth } from '@/lib/chat/permissions'
1110
import { withRouteHandler } from '@/lib/core/utils/with-route-handler'
11+
import { canExposePublicly } from '@/lib/deployments/public-exposure'
1212
import { performChatDeploy } from '@/lib/workflows/orchestration'
1313
import { checkWorkflowAccessForChatCreation } from '@/app/api/chat/utils'
1414
import { createErrorResponse, createSuccessResponse } from '@/app/api/workflows/utils'
@@ -116,7 +116,7 @@ export const POST = withRouteHandler(async (request: NextRequest) => {
116116
if (workflowRecord.workspaceId) {
117117
if (
118118
authType === 'public' &&
119-
!(await canSetPublicChatAuth(session.user.id, workflowRecord.workspaceId))
119+
!(await canExposePublicly(session.user.id, workflowRecord.workspaceId))
120120
) {
121121
return createErrorResponse('Only admins can deploy a public chat', 403)
122122
}

apps/sim/lib/chat/permissions.ts

Lines changed: 0 additions & 22 deletions
This file was deleted.

apps/sim/app/api/chat/utils.permissions.test.ts renamed to apps/sim/lib/deployments/public-exposure.test.ts

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@ vi.mock('@/lib/workspaces/permissions/utils', () => ({
1313
}))
1414

1515
import { beforeEach, describe, expect, it, vi } from 'vitest'
16-
import { canSetPublicChatAuth } from '@/lib/chat/permissions'
16+
import { canExposePublicly } from '@/lib/deployments/public-exposure'
1717
import { checkWorkflowAccessForChatCreation } from '@/app/api/chat/utils'
1818

1919
/**
@@ -90,17 +90,17 @@ describe('public chat auth is admin-only', () => {
9090

9191
it('allows an admin', async () => {
9292
mockGetUserEntityPermissions.mockResolvedValue('admin')
93-
await expect(canSetPublicChatAuth('user-1', 'ws-1')).resolves.toBe(true)
93+
await expect(canExposePublicly('user-1', 'ws-1')).resolves.toBe(true)
9494
expect(mockGetUserEntityPermissions).toHaveBeenCalledWith('user-1', 'workspace', 'ws-1')
9595
})
9696

9797
it.each(['write', 'read'] as const)('refuses a %s member', async (permission) => {
9898
mockGetUserEntityPermissions.mockResolvedValue(permission)
99-
await expect(canSetPublicChatAuth('user-1', 'ws-1')).resolves.toBe(false)
99+
await expect(canExposePublicly('user-1', 'ws-1')).resolves.toBe(false)
100100
})
101101

102102
it('refuses a member with no permission on the workspace', async () => {
103103
mockGetUserEntityPermissions.mockResolvedValue(null)
104-
await expect(canSetPublicChatAuth('user-1', 'ws-1')).resolves.toBe(false)
104+
await expect(canExposePublicly('user-1', 'ws-1')).resolves.toBe(false)
105105
})
106106
})
Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
import { getUserEntityPermissions } from '@/lib/workspaces/permissions/utils'
2+
3+
/**
4+
* Whether the actor may expose a deployment to unauthenticated callers.
5+
*
6+
* Deploying is a `write` capability, but making a deployment *public* is not:
7+
* a public workflow API, a public chat, and a public workflow MCP server all
8+
* skip authentication entirely, so anyone holding the URL can invoke the
9+
* workflow and everything it references. That is a different risk class from
10+
* shipping a version, and it stays admin-only.
11+
*
12+
* Lives here rather than beside any one surface because four paths can set it —
13+
* the workflow public-API route, the REST chat create and update routes, the
14+
* copilot chat use case, and the workflow MCP server create/update use cases —
15+
* and a rule duplicated per callsite is a rule that drifts. Two of those paths
16+
* default to public, so a missing check silently reopens the boundary.
17+
*
18+
* Callers gate only the *transition to* public. Turning exposure off, or
19+
* leaving it unchanged, stays `write`: neither increases exposure.
20+
*/
21+
export async function canExposePublicly(userId: string, workspaceId: string): Promise<boolean> {
22+
const permission = await getUserEntityPermissions(userId, 'workspace', workspaceId)
23+
return permission === 'admin'
24+
}
Lines changed: 129 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,129 @@
1+
/**
2+
* @vitest-environment node
3+
*/
4+
import type { Principal } from '@sim/auth/principal'
5+
import { beforeEach, describe, expect, it, vi } from 'vitest'
6+
7+
const { mocks } = vi.hoisted(() => ({
8+
mocks: {
9+
canExposePublicly: vi.fn(),
10+
resolvePermission: vi.fn(),
11+
createServer: vi.fn(),
12+
updateServer: vi.fn(),
13+
loadWorkspace: vi.fn(),
14+
audit: vi.fn(),
15+
},
16+
}))
17+
18+
vi.mock('@sim/audit', () => ({
19+
AuditAction: { MCP_SERVER_ADDED: 'mcp.added', MCP_SERVER_UPDATED: 'mcp.updated' },
20+
AuditResourceType: { MCP_SERVER: 'mcp_server' },
21+
recordAudit: mocks.audit,
22+
}))
23+
24+
vi.mock('@sim/platform-authz/workspace', () => ({
25+
permissionSatisfies: (actual: string | null, required: string) => {
26+
const rank = { read: 1, write: 2, admin: 3 } as const
27+
return (
28+
actual !== null && rank[actual as keyof typeof rank] >= rank[required as keyof typeof rank]
29+
)
30+
},
31+
resolveEffectiveWorkspacePermission: mocks.resolvePermission,
32+
}))
33+
34+
// The delegation policy revalidates the copilot grant against live state; this
35+
// test is about the public-exposure gate, not that plumbing.
36+
vi.mock('@/lib/mcp/application/authorization', () => ({
37+
MCP_SERVER_DELEGATION_AUDIENCE: 'sim:mcp-servers',
38+
mcpServerDelegationPolicy: { audience: 'sim:mcp-servers', isWithinScope: () => true },
39+
}))
40+
41+
vi.mock('@/lib/deployments/public-exposure', () => ({
42+
canExposePublicly: mocks.canExposePublicly,
43+
}))
44+
45+
vi.mock('@/lib/mcp/orchestration', () => ({
46+
performCreateWorkflowMcpServer: mocks.createServer,
47+
performCreateWorkflowMcpTool: vi.fn(),
48+
performDeleteWorkflowMcpServer: vi.fn(),
49+
performDeleteWorkflowMcpTool: vi.fn(),
50+
performUpdateWorkflowMcpServer: mocks.updateServer,
51+
performUpdateWorkflowMcpTool: vi.fn(),
52+
}))
53+
54+
vi.mock('@/lib/workspaces/application/workspace-context', () => ({
55+
loadActiveWorkspaceApplicationContext: mocks.loadWorkspace,
56+
}))
57+
58+
vi.mock('@/lib/mcp/pubsub', () => ({ mcpPubSub: undefined }))
59+
60+
import { createWorkflowMcpDeploymentServer } from '@/lib/mcp/application/workflow-deployments'
61+
62+
// This operation only accepts delegated copilot principals.
63+
const WRITE_PRINCIPAL: Principal = {
64+
kind: 'delegated',
65+
serviceId: 'copilot',
66+
subjectUserId: 'editor-1',
67+
workspaceId: 'workspace-1',
68+
delegationId: 'copilot-1',
69+
audience: 'sim:mcp-servers',
70+
issuedAt: new Date('2026-08-08T00:00:00Z'),
71+
expiresAt: new Date('2999-08-08T00:00:00Z'),
72+
}
73+
74+
/**
75+
* A public workflow MCP server skips authentication on the serve path, so
76+
* anyone with the URL can invoke every workflow published on it. Creating the
77+
* server is `write`; making it public is admin-only.
78+
*/
79+
describe('workflow MCP server public exposure is admin-only', () => {
80+
beforeEach(() => {
81+
vi.clearAllMocks()
82+
mocks.resolvePermission.mockResolvedValue('write')
83+
mocks.loadWorkspace.mockResolvedValue({
84+
workspaceId: 'workspace-1',
85+
billedAccountUserId: 'billing-1',
86+
})
87+
mocks.createServer.mockResolvedValue({
88+
success: true,
89+
server: { id: 'srv-1', name: 'srv', isPublic: true },
90+
addedTools: [],
91+
})
92+
})
93+
94+
it('rejects a write member creating a public server', async () => {
95+
mocks.canExposePublicly.mockResolvedValue(false)
96+
97+
await expect(
98+
createWorkflowMcpDeploymentServer.execute({
99+
principal: WRITE_PRINCIPAL,
100+
input: { workspaceId: 'workspace-1', name: 'srv', isPublic: true },
101+
})
102+
).rejects.toMatchObject({ code: 'forbidden' })
103+
104+
expect(mocks.createServer).not.toHaveBeenCalled()
105+
})
106+
107+
it('allows a write member creating a private server', async () => {
108+
mocks.canExposePublicly.mockResolvedValue(false)
109+
110+
await createWorkflowMcpDeploymentServer.execute({
111+
principal: WRITE_PRINCIPAL,
112+
input: { workspaceId: 'workspace-1', name: 'srv', isPublic: false },
113+
})
114+
115+
expect(mocks.createServer).toHaveBeenCalled()
116+
expect(mocks.canExposePublicly).not.toHaveBeenCalled()
117+
})
118+
119+
it('allows an admin creating a public server', async () => {
120+
mocks.canExposePublicly.mockResolvedValue(true)
121+
122+
await createWorkflowMcpDeploymentServer.execute({
123+
principal: WRITE_PRINCIPAL,
124+
input: { workspaceId: 'workspace-1', name: 'srv', isPublic: true },
125+
})
126+
127+
expect(mocks.createServer).toHaveBeenCalledWith(expect.objectContaining({ isPublic: true }))
128+
})
129+
})

0 commit comments

Comments
 (0)