Skip to content

Commit f587e82

Browse files
committed
fix(mcp): address PR review for OAuth feature
- provider: clear `state` from DB in `invalidateCredentials` to prevent stale state values from matching during CSRF check - storage: encrypt PKCE `codeVerifier` at rest to match `tokens` / `clientInformation` security posture - queries: `useForceRefreshMcpTools` now writes the fetched payload directly into the query cache instead of invalidating, eliminating the duplicate network round-trip - mcp settings: track per-server OAuth pending state so a "Connecting…" spinner only disables the card whose flow is in progress - mcp settings: surface existing `oauthClientId` in edit modal so the Advanced section auto-expands and displays the saved value
1 parent 4598067 commit f587e82

4 files changed

Lines changed: 20 additions & 7 deletions

File tree

apps/sim/app/workspace/[workspaceId]/settings/components/mcp/mcp.tsx

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -169,12 +169,14 @@ export function MCP({ initialServerId }: MCPProps) {
169169
url?: string
170170
timeout?: number
171171
headers?: { key: string; value: string }[]
172+
oauthClientId?: string
172173
}
173174
| undefined
174175
>(undefined)
175176

176177
const [searchTerm, setSearchTerm] = useState('')
177178
const [deletingServers, setDeletingServers] = useState<Set<string>>(() => new Set())
179+
const [connectingOauthServers, setConnectingOauthServers] = useState<Set<string>>(() => new Set())
178180

179181
const [showDeleteDialog, setShowDeleteDialog] = useState(false)
180182
const [serverToDelete, setServerToDelete] = useState<{ id: string; name: string } | null>(null)
@@ -356,6 +358,7 @@ export function MCP({ initialServerId }: MCPProps) {
356358
url: server.url || '',
357359
timeout: 30000,
358360
headers,
361+
oauthClientId: server.oauthClientId || undefined,
359362
})
360363
setShowEditModal(true)
361364
}, [])
@@ -460,19 +463,26 @@ export function MCP({ initialServerId }: MCPProps) {
460463
<Button
461464
variant='primary'
462465
size='sm'
463-
disabled={startOauthMutation.isPending}
466+
disabled={connectingOauthServers.has(server.id)}
464467
onClick={async () => {
468+
setConnectingOauthServers((prev) => new Set(prev).add(server.id))
465469
try {
466470
await startOauthMutation.mutateAsync({ serverId: server.id })
467471
} catch (e) {
468472
logger.error('Failed to start MCP OAuth', e)
469473
toast.error(
470474
e instanceof Error ? e.message : 'Failed to start authorization'
471475
)
476+
} finally {
477+
setConnectingOauthServers((prev) => {
478+
const next = new Set(prev)
479+
next.delete(server.id)
480+
return next
481+
})
472482
}
473483
}}
474484
>
475-
{startOauthMutation.isPending ? 'Connecting…' : 'Connect with OAuth'}
485+
{connectingOauthServers.has(server.id) ? 'Connecting…' : 'Connect with OAuth'}
476486
</Button>
477487
</div>
478488
</div>

apps/sim/hooks/queries/mcp.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -116,8 +116,8 @@ export function useForceRefreshMcpTools() {
116116

117117
return useMutation({
118118
mutationFn: (workspaceId: string) => fetchMcpTools(workspaceId, true),
119-
onSettled: (_data, _error, workspaceId) => {
120-
queryClient.invalidateQueries({ queryKey: mcpKeys.toolsList(workspaceId) })
119+
onSuccess: (tools, workspaceId) => {
120+
queryClient.setQueryData(mcpKeys.toolsList(workspaceId), tools)
121121
},
122122
})
123123
}

apps/sim/lib/mcp/oauth/provider.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import { decryptSecret } from '@/lib/core/security/encryption'
1111
import { getBaseUrl } from '@/lib/core/utils/urls'
1212
import {
1313
clearClient,
14+
clearState,
1415
clearTokens,
1516
clearVerifier,
1617
type McpOauthRow,
@@ -135,6 +136,7 @@ export class SimMcpOauthProvider {
135136
}
136137
if (scope === 'all' || scope === 'verifier') {
137138
await clearVerifier(this.row.id)
139+
await clearState(this.row.id)
138140
this.row.codeVerifier = null
139141
this.row.state = null
140142
}

apps/sim/lib/mcp/oauth/storage.ts

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -98,7 +98,7 @@ export async function loadOauthRow(params: {
9898
? await decryptClientInformation(row.clientInformation)
9999
: null,
100100
tokens: row.tokens ? await decryptTokens(row.tokens) : null,
101-
codeVerifier: row.codeVerifier,
101+
codeVerifier: row.codeVerifier ? (await decryptSecret(row.codeVerifier)).decrypted : null,
102102
state: row.state,
103103
}
104104
}
@@ -119,7 +119,7 @@ export async function loadOauthRowByState(state: string): Promise<McpOauthRow |
119119
? await decryptClientInformation(row.clientInformation)
120120
: null,
121121
tokens: row.tokens ? await decryptTokens(row.tokens) : null,
122-
codeVerifier: row.codeVerifier,
122+
codeVerifier: row.codeVerifier ? (await decryptSecret(row.codeVerifier)).decrypted : null,
123123
state: row.state,
124124
}
125125
}
@@ -144,9 +144,10 @@ export async function saveTokens(rowId: string, tokens: OAuthTokens): Promise<vo
144144
}
145145

146146
export async function saveCodeVerifier(rowId: string, verifier: string): Promise<void> {
147+
const { encrypted } = await encryptSecret(verifier)
147148
await db
148149
.update(mcpServerOauth)
149-
.set({ codeVerifier: verifier, updatedAt: new Date() })
150+
.set({ codeVerifier: encrypted, updatedAt: new Date() })
150151
.where(eq(mcpServerOauth.id, rowId))
151152
}
152153

0 commit comments

Comments
 (0)