Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ const SHARED_CLIENTS = [
clientIdKey: 'HUBSPOT_MCP_CLIENT_ID',
clientSecretKey: 'HUBSPOT_MCP_CLIENT_SECRET',
url: 'https://mcp.hubspot.com',
authorizationServer: 'https://mcp.hubspot.com',
tokenAuthMethod: 'client_secret_post',
scope: undefined,
},
Expand All @@ -61,6 +62,7 @@ const SHARED_CLIENTS = [
clientIdKey: 'ZOOM_MCP_CLIENT_ID',
clientSecretKey: 'ZOOM_MCP_CLIENT_SECRET',
url: 'https://mcp.zoom.us/mcp/meeting/streamable',
authorizationServer: 'https://zoom.us',
tokenAuthMethod: 'client_secret_basic',
scope: 'meeting:read:search meeting:read:assets',
},
Expand Down Expand Up @@ -208,6 +210,7 @@ describe.each(SHARED_CLIENTS)('$name shared member connector', (connector) => {
expect(await loadPreregisteredClient(mcpServer.id)).toEqual({
clientId: 'saved-client',
clientSecret: 'saved-secret',
issuer: connector.authorizationServer,
})
for (const change of [
{ oauthClientId: null, oauthClientSecret: encrypted },
Expand Down Expand Up @@ -332,7 +335,7 @@ describe.each(SHARED_CLIENTS)('$name shared member connector', (connector) => {
'restricts generic OAuth %s to registered read permissions',
async (phase) => {
const { mcpServer } = await create()
const issuer = 'https://oauth.fixture.test'
const issuer = connector.authorizationServer
const loadProvider = async () =>
new SimMcpOauthProvider({
row: await getOrCreateOauthRow({ mcpServerId: mcpServer.id, organizationId: org }),
Expand Down Expand Up @@ -396,6 +399,72 @@ describe.each(SHARED_CLIENTS)('$name shared member connector', (connector) => {
}
)
}
it.each(['code exchange', 'legacy refresh'] as const)(
'never presents shared credentials to a substituted authorization server on %s',
async (phase) => {
const { mcpServer } = await create()
const substitute = 'https://oauth.substitute.test'
const provider = new SimMcpOauthProvider({
row: await getOrCreateOauthRow({ mcpServerId: mcpServer.id, organizationId: org }),
preregistered: await loadPreregisteredClient(mcpServer.id),
})
if (phase === 'code exchange') await provider.saveCodeVerifier('fixture-verifier')
else
await provider.saveTokens({
access_token: 'fixture-access',
refresh_token: 'fixture-refresh',
token_type: 'Bearer',
})
const disclosed: string[] = []
const reached = new Set<string>()
const fetchFn: typeof fetch = async (request, init) => {
const url = new URL(
typeof request === 'string' ? request : request instanceof URL ? request : request.url
)
if (url.origin === substitute) reached.add(url.pathname)
const sent = `${new Headers(init?.headers).get('authorization') ?? ''} ${String(init?.body ?? '')}`
if (
url.origin === substitute &&
(sent.includes(SECRET) ||
sent.includes(Buffer.from(`fixture-shared-client:${SECRET}`).toString('base64')) ||
sent.includes('fixture-refresh'))
)
disclosed.push(`${init?.method ?? 'GET'} ${url.pathname}`)
if (url.pathname.includes('oauth-protected-resource'))
return Response.json({ resource: connector.url, authorization_servers: [substitute] })
if (
url.pathname.includes('oauth-authorization-server') ||
url.pathname.includes('openid-configuration')
)
return Response.json({
issuer: substitute,
authorization_endpoint: `${substitute}/authorize`,
token_endpoint: `${substitute}/token`,
registration_endpoint: `${substitute}/register`,
response_types_supported: ['code'],
code_challenge_methods_supported: ['S256'],
})
if (url.pathname === '/register')
return Response.json({
client_id: 'substitute-client',
redirect_uris: [provider.redirectUrl],
})
if (url.pathname === '/token')
return Response.json({ access_token: 'substitute-access', token_type: 'Bearer' })
throw new Error(`Unexpected OAuth fixture request: ${url.origin}${url.pathname}`)
}
const attempt = oauth.mcpAuthGuarded(provider, {
serverUrl: connector.url,
fetchFn,
...(phase === 'code exchange' ? { authorizationCode: 'fixture-code' } : {}),
})
if (phase === 'code exchange')
await expect(attempt).rejects.toThrow(/Existing OAuth client information is required/)
else await expect(attempt).rejects.toBeInstanceOf(McpOauthRedirectRequired)
expect(reached.has('/.well-known/oauth-authorization-server')).toBe(true)
expect(disclosed).toEqual([])
}
)
it('binds the public OAuth round trip to the shared client and rejects rotation before exchange', async () => {
const { mcpServer } = await create()
const token = generateId()
Expand Down Expand Up @@ -427,7 +496,7 @@ describe.each(SHARED_CLIENTS)('$name shared member connector', (connector) => {
})
let exchanges = 0
let challenge: string | null = null
const issuer = 'https://oauth.fixture.test'
const issuer = connector.authorizationServer
/** Only provider HTTP is substituted; SDK, PKCE, Redis, use cases, encryption and database are real. */
const fetchFn: typeof fetch = async (request, init) => {
const url = new URL(
Expand Down
4 changes: 4 additions & 0 deletions apps/sim/lib/credential-groups/managed-mcp-connectors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,8 @@ interface DatabricksManagedMcpConnector extends ManagedMcpConnectorMetadata {
interface FixedPreregisteredManagedMcpConnector extends ManagedMcpConnectorMetadata {
id: 'hubspot' | 'zoom'
url: string
/** The only authorization server the deployment client secret is ever presented to. */
authorizationServer: string
oauthClientRegistration: 'preregistered'
}

Expand All @@ -45,6 +47,7 @@ export const MANAGED_MCP_CONNECTORS = {
name: 'Zoom',
description: 'Search past meetings, transcripts and notes using your Zoom account',
url: 'https://mcp.zoom.us/mcp/meeting/streamable',
authorizationServer: 'https://zoom.us',
oauthClientRegistration: 'preregistered',
},
lucid: {
Expand All @@ -60,6 +63,7 @@ export const MANAGED_MCP_CONNECTORS = {
name: 'HubSpot',
description: 'Search CRM records using each person’s HubSpot permissions',
url: 'https://mcp.hubspot.com',
authorizationServer: 'https://mcp.hubspot.com',
oauthClientRegistration: 'preregistered',
},
coda: {
Expand Down
9 changes: 7 additions & 2 deletions apps/sim/lib/mcp/oauth/managed-provider.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,11 @@ import type {
} from '@modelcontextprotocol/sdk/shared/auth.js'
import { generateId } from '@sim/utils/id'
import { getBaseUrl } from '@/lib/core/utils/urls'
import { McpOauthRedirectRequired, type PreregisteredClient } from '@/lib/mcp/oauth/provider'
import {
McpOauthRedirectRequired,
type PreregisteredClient,
withPinnedIssuer,
} from '@/lib/mcp/oauth/provider'
import { clearClient, type McpOauthRow, saveClientInformation } from '@/lib/mcp/oauth/storage'

interface ManagedMcpOauthProviderInit {
Expand Down Expand Up @@ -78,6 +82,7 @@ export class ManagedMcpOauthProvider implements OAuthClientProvider {
token_endpoint_auth_method:
this.preregistered.tokenEndpointAuthMethod ??
(this.preregistered.clientSecret ? 'client_secret_post' : 'none'),
issuer: this.preregistered.issuer ?? this.currentTokens?.issuer,
}
}

Expand All @@ -88,7 +93,7 @@ export class ManagedMcpOauthProvider implements OAuthClientProvider {
}

tokens(): OAuthTokens | undefined {
return this.currentTokens
return withPinnedIssuer(this.currentTokens, this.preregistered)
}

async saveTokens(tokens: OAuthTokens): Promise<void> {
Expand Down
28 changes: 24 additions & 4 deletions apps/sim/lib/mcp/oauth/provider.ts
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,24 @@ export interface PreregisteredClient {
configurationFingerprint?: string
scope?: string
tokenEndpointAuthMethod?: 'client_secret_basic' | 'client_secret_post'
/**
* Authorization server the credentials are registered with. When unset, they are bound to
* the issuer of the current grant, so the SDK never presents the secret to another server.
*/
issuer?: string
}

/**
* Grants saved before the SDK stamped `issuer` are otherwise refreshed against whichever
* authorization server the MCP server names. A pinned registration could only have obtained
* them from its own server, so they are bound to it.
*/
export function withPinnedIssuer(
tokens: OAuthTokens | undefined,
preregistered: PreregisteredClient | undefined
): OAuthTokens | undefined {
if (!tokens || tokens.issuer || !preregistered?.issuer) return tokens
return { ...tokens, issuer: preregistered.issuer }
}

interface SimMcpOauthProviderInit {
Expand Down Expand Up @@ -96,7 +114,6 @@ export class SimMcpOauthProvider implements OAuthClientProvider {
}

clientInformation(): OAuthClientInformationMixed | undefined {
if (this.row.clientInformation) return this.row.clientInformation
if (this.preregistered) {
return {
client_id: this.preregistered.clientId,
Expand All @@ -107,9 +124,10 @@ export class SimMcpOauthProvider implements OAuthClientProvider {
token_endpoint_auth_method:
this.preregistered.tokenEndpointAuthMethod ??
(this.preregistered.clientSecret ? 'client_secret_post' : 'none'),
issuer: this.preregistered.issuer ?? this.row.tokens?.issuer,
Comment thread
waleedlatif1 marked this conversation as resolved.
Comment thread
waleedlatif1 marked this conversation as resolved.
Comment thread
waleedlatif1 marked this conversation as resolved.
}
}
return undefined
return this.row.clientInformation ?? undefined
}

async saveClientInformation(info: OAuthClientInformationMixed): Promise<void> {
Expand All @@ -119,7 +137,7 @@ export class SimMcpOauthProvider implements OAuthClientProvider {
}

tokens(): OAuthTokens | undefined {
return this.row.tokens ?? undefined
return withPinnedIssuer(this.row.tokens ?? undefined, this.preregistered)
}

async saveTokens(tokens: OAuthTokens): Promise<void> {
Expand Down Expand Up @@ -226,6 +244,8 @@ export async function loadPreregisteredClient(
if (!row.clientId || !row.clientSecret)
throw new Error('HubSpot OAuth registration is incomplete')
}
const issuer =
row.connectorId === 'hubspot' ? MANAGED_MCP_CONNECTORS.hubspot.authorizationServer : undefined
if (!row.clientId) return undefined
let clientSecret: string | undefined
if (row.clientSecret) {
Expand All @@ -240,5 +260,5 @@ export async function loadPreregisteredClient(
throw new Error('Failed to decrypt preregistered MCP OAuth client secret')
}
}
return { clientId: row.clientId, clientSecret }
return { clientId: row.clientId, clientSecret, issuer }
Comment thread
cubic-dev-ai[bot] marked this conversation as resolved.
}
21 changes: 20 additions & 1 deletion apps/sim/lib/mcp/oauth/revoke.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,11 @@ describe('revokeMcpOauthTokens — SSRF guard', () => {
resetDbChainMock()

mockLoadOauthRow.mockResolvedValue({
tokens: { access_token: 'access-secret', refresh_token: 'refresh-secret' },
tokens: {
access_token: 'access-secret',
refresh_token: 'refresh-secret',
issuer: PUBLIC_SERVER_URL,
},
clientInformation: { client_id: 'client-123' },
})

Expand All @@ -84,6 +88,7 @@ describe('revokeMcpOauthTokens — SSRF guard', () => {
})

mockDiscoverOAuthServerInfo.mockResolvedValue({
authorizationServerUrl: PUBLIC_SERVER_URL,
authorizationServerMetadata: {
issuer: PUBLIC_SERVER_URL,
revocation_endpoint: BLOCKED_ENDPOINT,
Expand Down Expand Up @@ -118,6 +123,20 @@ describe('revokeMcpOauthTokens — SSRF guard', () => {
expect(mockValidateMcpServerSsrf).toHaveBeenCalledWith(BLOCKED_ENDPOINT, 'contentFetch')
})

it.each([
['issued by a different authorization server', 'https://previous-issuer.example.com'],
['without a recorded issuer', undefined],
])('never revokes tokens %s', async (_case, issuer) => {
mockLoadOauthRow.mockResolvedValue({
tokens: { access_token: 'access-secret', refresh_token: 'refresh-secret', issuer },
clientInformation: { client_id: 'client-123' },
})

await revokeMcpOauthTokens('server-1', 'workspace-1')

expect(mockValidateMcpServerSsrf).not.toHaveBeenCalledWith(BLOCKED_ENDPOINT, 'contentFetch')
})

it('never issues an outbound request to the blocked revocation endpoint', async () => {
await revokeMcpOauthTokens('server-1', 'workspace-1')

Expand Down
24 changes: 23 additions & 1 deletion apps/sim/lib/mcp/oauth/revoke.ts
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,14 @@ export async function revokeMcpOauthTokens(
const info = await discoverOAuthServerInfo(server.url, { fetchFn: ssrfGuardedFetch }).catch(
() => undefined
)
const metadata = info?.authorizationServerMetadata as
if (
!info ||
!row.tokens.issuer ||
!isSameAuthorizationServer(row.tokens.issuer, info.authorizationServerUrl)
) {
return
}
const metadata = info.authorizationServerMetadata as
| (Record<string, unknown> & { revocation_endpoint?: string })
| undefined
const revocationEndpoint = metadata?.revocation_endpoint
Expand Down Expand Up @@ -79,6 +86,21 @@ export async function revokeMcpOauthTokens(
}
}

/**
* Tokens are only posted, with the client secret, to the authorization server the SDK stamped
* them for. Unstamped legacy tokens are never revoked: their issuer cannot be verified.
*/
function isSameAuthorizationServer(issuer: string, authorizationServerUrl: string): boolean {
const normalize = (value: string) => {
try {
return new URL(value).href
} catch {
return value
}
}
Comment thread
waleedlatif1 marked this conversation as resolved.
return normalize(issuer) === normalize(authorizationServerUrl)
}

async function postRevoke(
endpoint: string,
token: string,
Expand Down
2 changes: 2 additions & 0 deletions apps/sim/lib/mcp/oauth/shared-clients.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ export function getSharedHubSpotMcpClient() {
return {
clientId,
clientSecret,
issuer: MANAGED_MCP_CONNECTORS.hubspot.authorizationServer,
Comment thread
waleedlatif1 marked this conversation as resolved.
configurationFingerprint: sha256Hex(
JSON.stringify([
'shared-hubspot-mcp',
Expand All @@ -40,6 +41,7 @@ export function getSharedZoomMcpClient() {
clientSecret,
scope,
tokenEndpointAuthMethod,
issuer: MANAGED_MCP_CONNECTORS.zoom.authorizationServer,
configurationFingerprint: sha256Hex(
JSON.stringify([
'shared-zoom-mcp',
Expand Down
4 changes: 2 additions & 2 deletions apps/sim/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@
"@hookform/resolvers": "5.2.2",
"@linear/sdk": "91.0.0",
"@marsidev/react-turnstile": "1.4.2",
"@modelcontextprotocol/sdk": "1.29.0",
"@modelcontextprotocol/sdk": "1.31.0",
"@monaco-editor/react": "4.7.0",
"@napi-rs/canvas": "0.1.100",
"@opentelemetry/api": "^1.9.0",
Expand Down Expand Up @@ -247,7 +247,7 @@
"remark-stringify": "11.0.0",
"resend": "^4.1.2",
"rss-parser": "3.13.0",
"sharp": "0.35.4",
"sharp": "0.35.5",
"sim": "workspace:*",
"socket.io-client": "4.8.1",
"ssh2": "^1.17.0",
Expand Down
Loading
Loading