Skip to content

Commit 9b0f176

Browse files
mzxchandramzxchandra
committed
fix(mcp): gate public exposure on the REST routes, not just the use case
The admin-only public-exposure gate lived in the workflow MCP server use cases, but `POST /api/mcp/workflow-servers` and `PATCH .../[id]` call the orchestration layer directly and never reach them. `withMcpAuth('write')` was their only authorization, so a `write` member could publish an unauthenticated MCP server through Workspace Settings — the surface the gate was meant to cover. Gate both routes after auth, and fix the rule while it moves: the check tested the requested value rather than the transition, so an unchanged `isPublic: true` — which the edit form resubmits alongside a rename — would have 403'd a `write` member editing an already-public server. The transition rule is now `increasesPublicExposure`, shared by the routes and the use cases. The settings UI offered the Access control to non-admins with nothing disabled, so the write path led straight into a 403. Disable it with the same tooltip the public API and chat surfaces use. Also stop seeding a non-admin into the chat deploy form's `public` default: the selector's fallback effect ran before the parent's form reset and was clobbered back to public, leaving a write member on a disabled option with Launch Chat inert. Verified end to end against a `write` member in the browser: rename of a public server 200s, escalation 403s on both create and update, de-escalation and private create still work, and the admin path is unchanged. Covered by route tests and use-case tests, both mutation-checked. Migrating the two routes onto their use cases is filed in TODOS.md — the gate is duplicated until then.
1 parent 6ec8f56 commit 9b0f176

10 files changed

Lines changed: 467 additions & 32 deletions

File tree

‎TODOS.md‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,29 @@ of consuming `workflowOperations.deploy.minimumRole`. A future change to the
3232
central `minimumRole` leaves these three routes at a stale value. They also sit
3333
outside the operation's `principalKinds` policy.
3434

35+
### Route the workflow MCP server routes through their application use cases
36+
**Priority:** P2
37+
38+
`apps/sim/app/api/mcp/workflow-servers/route.ts` (POST) and
39+
`apps/sim/app/api/mcp/workflow-servers/[id]/route.ts` (PATCH) call
40+
`performCreateWorkflowMcpServer` / `performUpdateWorkflowMcpServer` directly
41+
instead of going through `createWorkflowMcpDeploymentServer` /
42+
`updateWorkflowMcpDeploymentServer`. Everything those use cases declare —
43+
`minimumRole`, `workspaceApiKey`, `principalKinds`, the delegation policy, and
44+
semantic audit — is bypassed on the primary UI path.
45+
46+
This already cost us one real hole: the admin-only public-exposure gate was
47+
added to the use cases and silently did not cover the settings UI, so a `write`
48+
member could publish an unauthenticated MCP server. The gate is now duplicated
49+
into both routes (see the TSDoc at each callsite) — that closes the hole but
50+
leaves two copies of a security rule, which is exactly what
51+
`.claude/rules/global.md` "Application Operation Boundary" forbids.
52+
53+
Migrating needs `withMcpAuth`'s `{ userId, workspaceId }` context to become a
54+
`Principal` (these routes accept MCP API-key auth, not just sessions), so it is
55+
a real refactor rather than a mechanical swap. Delete the duplicated gates when
56+
it lands.
57+
3558
### Add `mship-tools:check` to CI
3659
**Priority:** P2
3760

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

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import {
99
workflowMcpServerParamsSchema,
1010
} from '@/lib/api/contracts/workflow-mcp-servers'
1111
import { withRouteHandler } from '@/lib/core/utils/with-route-handler'
12+
import { canExposePublicly, increasesPublicExposure } from '@/lib/deployments/public-exposure'
1213
import {
1314
mcpBodyReadErrorResponse,
1415
readMcpJsonBodyWithLimit,
@@ -109,6 +110,42 @@ export const PATCH = withRouteHandler(
109110

110111
logger.info(`[${requestId}] Updating workflow MCP server: ${serverId}`)
111112

113+
/**
114+
* `withMcpAuth('write')` covers managing the server, but a public
115+
* server skips authentication on the serve path, so publishing one is
116+
* admin-only. Only the transition is gated: the edit form resubmits the
117+
* server's current visibility alongside whatever field changed, so a
118+
* `write` member must still be able to rename an already-public server.
119+
*
120+
* This route calls the orchestration layer directly rather than the
121+
* application use case, so the use case's gate does not apply here —
122+
* see the TODO about migrating both MCP server routes.
123+
*/
124+
if (body.isPublic === true) {
125+
const [current] = await db
126+
.select({ isPublic: workflowMcpServer.isPublic })
127+
.from(workflowMcpServer)
128+
.where(
129+
and(
130+
eq(workflowMcpServer.id, serverId),
131+
eq(workflowMcpServer.workspaceId, workspaceId),
132+
isNull(workflowMcpServer.deletedAt)
133+
)
134+
)
135+
.limit(1)
136+
137+
if (
138+
increasesPublicExposure(body.isPublic, current?.isPublic) &&
139+
!(await canExposePublicly(userId, workspaceId))
140+
) {
141+
return createMcpErrorResponse(
142+
new Error('Only admins can make an MCP server public'),
143+
'Only admins can make an MCP server public',
144+
403
145+
)
146+
}
147+
}
148+
112149
const result = await performUpdateWorkflowMcpServer({
113150
serverId,
114151
workspaceId,

‎apps/sim/app/api/mcp/workflow-servers/public-exposure.test.ts‎

Lines changed: 169 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,169 @@
1+
/**
2+
* @vitest-environment node
3+
*/
4+
import { beforeEach, describe, expect, it, vi } from 'vitest'
5+
6+
const { mocks } = vi.hoisted(() => ({
7+
mocks: {
8+
/** Mutable so a test can decide whether the server is already public. */
9+
currentServer: { id: 'srv-1', workspaceId: 'workspace-1', isPublic: false } as Record<
10+
string,
11+
unknown
12+
>,
13+
body: {} as Record<string, unknown>,
14+
getUserEntityPermissions: vi.fn(),
15+
createServer: vi.fn(),
16+
updateServer: vi.fn(),
17+
errorResponse: vi.fn(),
18+
},
19+
}))
20+
21+
/**
22+
* `withMcpAuth('write')` is the only authorization these routes carry, so the
23+
* stub grants exactly that: a `write` caller who has already passed auth. What
24+
* is under test is whether the public-exposure gate runs *after* it.
25+
*/
26+
vi.mock('@/lib/mcp/middleware', () => ({
27+
withMcpAuth: () => (handler: unknown) => (request: unknown, routeContext: unknown) =>
28+
(handler as (r: unknown, c: Record<string, string>, rc: unknown) => Promise<unknown>)(
29+
request,
30+
{
31+
userId: 'editor-1',
32+
userName: 'Editor',
33+
userEmail: 'editor@example.com',
34+
workspaceId: 'workspace-1',
35+
requestId: 'req-1',
36+
},
37+
routeContext
38+
),
39+
readMcpJsonBodyWithLimit: async () => mocks.body,
40+
mcpBodyReadErrorResponse: () => null,
41+
}))
42+
43+
vi.mock('@/lib/mcp/utils', () => ({
44+
createMcpErrorResponse: (_error: unknown, message: string, status: number) =>
45+
mocks.errorResponse({ message, status }) ?? { message, status },
46+
createMcpSuccessResponse: (data: unknown) => ({ status: 200, data }),
47+
mcpOrchestrationStatus: () => 500,
48+
}))
49+
50+
vi.mock('@/lib/mcp/orchestration', () => ({
51+
performCreateWorkflowMcpServer: mocks.createServer,
52+
performUpdateWorkflowMcpServer: mocks.updateServer,
53+
performDeleteWorkflowMcpServer: vi.fn(),
54+
}))
55+
56+
vi.mock('@/lib/workspaces/permissions/utils', () => ({
57+
getUserEntityPermissions: mocks.getUserEntityPermissions,
58+
}))
59+
60+
vi.mock('@sim/db', () => {
61+
const chain: Record<string, unknown> = {}
62+
for (const method of ['select', 'from', 'where', 'limit']) {
63+
chain[method] = vi.fn(() => chain)
64+
}
65+
;(chain as { then?: unknown }).then = (resolve: (rows: unknown[]) => unknown) =>
66+
resolve([mocks.currentServer])
67+
return { db: chain }
68+
})
69+
70+
vi.mock('@sim/db/schema', () => ({
71+
workflowMcpServer: { id: {}, workspaceId: {}, deletedAt: {}, isPublic: {} },
72+
workflowMcpTool: {},
73+
}))
74+
75+
vi.mock('drizzle-orm', () => ({
76+
and: vi.fn(),
77+
eq: vi.fn(),
78+
inArray: vi.fn(),
79+
isNull: vi.fn(),
80+
sql: vi.fn(),
81+
}))
82+
83+
import { PATCH } from '@/app/api/mcp/workflow-servers/[id]/route'
84+
import { POST } from '@/app/api/mcp/workflow-servers/route'
85+
86+
const request = {} as never
87+
const routeContext = { params: Promise.resolve({ id: 'srv-1' }) } as never
88+
89+
/**
90+
* These routes call the orchestration layer directly rather than the
91+
* application use case, so the use case's admin gate does not cover them. A
92+
* public server needs no authentication to invoke, so a `write` member must not
93+
* be able to publish one through the settings UI, which is what these hit.
94+
*/
95+
describe('workflow MCP server REST routes gate public exposure', () => {
96+
beforeEach(() => {
97+
vi.clearAllMocks()
98+
mocks.currentServer.isPublic = false
99+
mocks.createServer.mockResolvedValue({
100+
success: true,
101+
server: { id: 'srv-1', name: 'srv' },
102+
addedTools: [],
103+
})
104+
mocks.updateServer.mockResolvedValue({
105+
success: true,
106+
server: { id: 'srv-1', name: 'renamed' },
107+
updatedFields: ['name'],
108+
})
109+
})
110+
111+
it('rejects a write member creating a public server', async () => {
112+
mocks.getUserEntityPermissions.mockResolvedValue('write')
113+
mocks.body = { name: 'srv', isPublic: true }
114+
115+
const response = (await POST(request, routeContext)) as { status: number }
116+
117+
expect(response.status).toBe(403)
118+
expect(mocks.createServer).not.toHaveBeenCalled()
119+
})
120+
121+
it('allows an admin creating a public server', async () => {
122+
mocks.getUserEntityPermissions.mockResolvedValue('admin')
123+
mocks.body = { name: 'srv', isPublic: true }
124+
125+
await POST(request, routeContext)
126+
127+
expect(mocks.createServer).toHaveBeenCalledWith(expect.objectContaining({ isPublic: true }))
128+
})
129+
130+
it('allows a write member creating a private server', async () => {
131+
mocks.getUserEntityPermissions.mockResolvedValue('write')
132+
mocks.body = { name: 'srv', isPublic: false }
133+
134+
await POST(request, routeContext)
135+
136+
expect(mocks.createServer).toHaveBeenCalled()
137+
})
138+
139+
it('rejects a write member flipping a private server to public', async () => {
140+
mocks.getUserEntityPermissions.mockResolvedValue('write')
141+
mocks.body = { isPublic: true }
142+
143+
const response = (await PATCH(request, routeContext)) as { status: number }
144+
145+
expect(response.status).toBe(403)
146+
expect(mocks.updateServer).not.toHaveBeenCalled()
147+
})
148+
149+
it('allows a write member renaming an already-public server', async () => {
150+
mocks.currentServer.isPublic = true
151+
mocks.getUserEntityPermissions.mockResolvedValue('write')
152+
mocks.body = { name: 'renamed', isPublic: true }
153+
154+
await PATCH(request, routeContext)
155+
156+
expect(mocks.updateServer).toHaveBeenCalledWith(expect.objectContaining({ name: 'renamed' }))
157+
})
158+
159+
it('allows a write member making a public server private', async () => {
160+
mocks.currentServer.isPublic = true
161+
mocks.getUserEntityPermissions.mockResolvedValue('write')
162+
mocks.body = { isPublic: false }
163+
164+
await PATCH(request, routeContext)
165+
166+
expect(mocks.updateServer).toHaveBeenCalled()
167+
expect(mocks.getUserEntityPermissions).not.toHaveBeenCalled()
168+
})
169+
})

‎apps/sim/app/api/mcp/workflow-servers/route.ts‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import { and, eq, inArray, isNull, sql } from 'drizzle-orm'
66
import type { NextRequest } from 'next/server'
77
import { createWorkflowMcpServerBodySchema } from '@/lib/api/contracts/workflow-mcp-servers'
88
import { withRouteHandler } from '@/lib/core/utils/with-route-handler'
9+
import { canExposePublicly, increasesPublicExposure } from '@/lib/deployments/public-exposure'
910
import {
1011
mcpBodyReadErrorResponse,
1112
readMcpJsonBodyWithLimit,
@@ -115,6 +116,24 @@ export const POST = withRouteHandler(
115116
workflowIds: body.workflowIds,
116117
})
117118

119+
/**
120+
* `withMcpAuth('write')` covers managing the server, but a public
121+
* server skips authentication on the serve path, so publishing one is
122+
* admin-only. This route calls the orchestration layer directly rather
123+
* than the application use case, so the use case's gate does not apply
124+
* here — see the TODO about migrating both MCP server routes.
125+
*/
126+
if (
127+
increasesPublicExposure(body.isPublic, false) &&
128+
!(await canExposePublicly(userId, workspaceId))
129+
) {
130+
return createMcpErrorResponse(
131+
new Error('Only admins can make an MCP server public'),
132+
'Only admins can make an MCP server public',
133+
403
134+
)
135+
}
136+
118137
const result = await performCreateWorkflowMcpServer({
119138
workspaceId,
120139
userId,

‎apps/sim/app/workspace/[workspaceId]/settings/components/workflow-mcp-servers/components/create-workflow-mcp-server-modal/create-workflow-mcp-server-modal.tsx‎

Lines changed: 28 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -12,8 +12,10 @@ import {
1212
ChipModalHeader,
1313
ChipSelect,
1414
type ComboboxOption,
15+
Tooltip,
1516
} from '@sim/emcn'
1617
import { createLogger } from '@sim/logger'
18+
import { useUserPermissionsContext } from '@/app/workspace/[workspaceId]/providers/workspace-permissions-provider'
1719
import { useCreateWorkflowMcpServer } from '@/hooks/queries/workflow-mcp-servers'
1820

1921
const logger = createLogger('CreateWorkflowMcpServerModal')
@@ -38,6 +40,13 @@ export function CreateWorkflowMcpServerModal({
3840
workflowOptions,
3941
}: CreateWorkflowMcpServerModalProps) {
4042
const createServerMutation = useCreateWorkflowMcpServer()
43+
/**
44+
* A public server is callable with no authentication, so publishing one is
45+
* admin-only — the same boundary the public workflow API and public chats
46+
* enforce. The server rejects it regardless; this keeps a `write` member from
47+
* being offered an option that would only 403 on save.
48+
*/
49+
const canSetPublicAccess = useUserPermissionsContext().canAdmin
4150

4251
const [formData, setFormData] = useState({ ...INITIAL_FORM_DATA })
4352
const [selectedWorkflowIds, setSelectedWorkflowIds] = useState<string[]>([])
@@ -115,13 +124,25 @@ export function CreateWorkflowMcpServerModal({
115124
)}
116125
<ChipModalField type='custom' title='Access'>
117126
<div className='flex items-center gap-3'>
118-
<ButtonGroup
119-
value={formData.isPublic ? 'public' : 'private'}
120-
onValueChange={(value) => setFormData({ ...formData, isPublic: value === 'public' })}
121-
>
122-
<ButtonGroupItem value='private'>API Key</ButtonGroupItem>
123-
<ButtonGroupItem value='public'>Public</ButtonGroupItem>
124-
</ButtonGroup>
127+
<Tooltip.Root>
128+
<Tooltip.Trigger asChild>
129+
<span className='inline-flex w-fit'>
130+
<ButtonGroup
131+
value={formData.isPublic ? 'public' : 'private'}
132+
onValueChange={(value) =>
133+
setFormData({ ...formData, isPublic: value === 'public' })
134+
}
135+
disabled={!canSetPublicAccess}
136+
>
137+
<ButtonGroupItem value='private'>API Key</ButtonGroupItem>
138+
<ButtonGroupItem value='public'>Public</ButtonGroupItem>
139+
</ButtonGroup>
140+
</span>
141+
</Tooltip.Trigger>
142+
{!canSetPublicAccess && (
143+
<Tooltip.Content>Only admins can change public access</Tooltip.Content>
144+
)}
145+
</Tooltip.Root>
125146
{formData.isPublic && (
126147
<span className='text-[var(--text-muted)] text-caption'>
127148
No authentication required

‎apps/sim/app/workspace/[workspaceId]/settings/components/workflow-mcp-servers/workflow-mcp-servers.tsx‎

Lines changed: 25 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ import {
1919
Code,
2020
type ComboboxOption,
2121
Label,
22+
Tooltip,
2223
useCopyToClipboard,
2324
} from '@sim/emcn'
2425
import { ArrowLeft, Check, Clipboard, Plus, Server } from '@sim/emcn/icons'
@@ -102,6 +103,13 @@ function ServerDetailView({
102103
const allowPersonalApiKeys = false
103104
/** Managing this server only needs `write`, but minting a workspace key needs `admin`. */
104105
const canManageWorkspaceKeys = workspacePermissions.canAdmin
106+
/**
107+
* A public server is callable with no authentication, so changing its access
108+
* is admin-only — the same boundary the public workflow API and public chats
109+
* enforce. The server rejects the change regardless; this keeps a `write`
110+
* member from being offered an action that would only 403.
111+
*/
112+
const canSetPublicAccess = workspacePermissions.canAdmin
105113
const defaultKeyType = 'workspace'
106114

107115
const addToWorkspaceMutation = useCreateMcpServer()
@@ -824,13 +832,23 @@ function ServerDetailView({
824832
/>
825833
<ChipModalField type='custom' title='Access'>
826834
<div className='flex flex-col gap-1.5'>
827-
<ButtonGroup
828-
value={editServerIsPublic ? 'public' : 'private'}
829-
onValueChange={(value) => setEditServerIsPublic(value === 'public')}
830-
>
831-
<ButtonGroupItem value='private'>API Key</ButtonGroupItem>
832-
<ButtonGroupItem value='public'>Public</ButtonGroupItem>
833-
</ButtonGroup>
835+
<Tooltip.Root>
836+
<Tooltip.Trigger asChild>
837+
<span className='inline-flex w-fit'>
838+
<ButtonGroup
839+
value={editServerIsPublic ? 'public' : 'private'}
840+
onValueChange={(value) => setEditServerIsPublic(value === 'public')}
841+
disabled={!canSetPublicAccess}
842+
>
843+
<ButtonGroupItem value='private'>API Key</ButtonGroupItem>
844+
<ButtonGroupItem value='public'>Public</ButtonGroupItem>
845+
</ButtonGroup>
846+
</span>
847+
</Tooltip.Trigger>
848+
{!canSetPublicAccess && (
849+
<Tooltip.Content>Only admins can change public access</Tooltip.Content>
850+
)}
851+
</Tooltip.Root>
834852
<p className='text-[var(--text-muted)] text-caption'>
835853
{editServerIsPublic
836854
? 'Anyone with the URL can call this server without authentication'

0 commit comments

Comments
 (0)