Skip to content

Commit 4dd7c72

Browse files
committed
fix(mcp): require a verified issuer before revocation and pin fixture issuers
1 parent 8eb364c commit 4dd7c72

3 files changed

Lines changed: 17 additions & 9 deletions

File tree

‎apps/sim/lib/credential-groups/__integration__/hubspot-mcp.integration.ts‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,7 @@ const SHARED_CLIENTS = [
5252
clientIdKey: 'HUBSPOT_MCP_CLIENT_ID',
5353
clientSecretKey: 'HUBSPOT_MCP_CLIENT_SECRET',
5454
url: 'https://mcp.hubspot.com',
55+
authorizationServer: 'https://mcp.hubspot.com',
5556
tokenAuthMethod: 'client_secret_post',
5657
scope: undefined,
5758
},
@@ -61,6 +62,7 @@ const SHARED_CLIENTS = [
6162
clientIdKey: 'ZOOM_MCP_CLIENT_ID',
6263
clientSecretKey: 'ZOOM_MCP_CLIENT_SECRET',
6364
url: 'https://mcp.zoom.us/mcp/meeting/streamable',
65+
authorizationServer: 'https://zoom.us',
6466
tokenAuthMethod: 'client_secret_basic',
6567
scope: 'meeting:read:search meeting:read:assets',
6668
},
@@ -208,6 +210,7 @@ describe.each(SHARED_CLIENTS)('$name shared member connector', (connector) => {
208210
expect(await loadPreregisteredClient(mcpServer.id)).toEqual({
209211
clientId: 'saved-client',
210212
clientSecret: 'saved-secret',
213+
issuer: connector.authorizationServer,
211214
})
212215
for (const change of [
213216
{ oauthClientId: null, oauthClientSecret: encrypted },
@@ -332,7 +335,7 @@ describe.each(SHARED_CLIENTS)('$name shared member connector', (connector) => {
332335
'restricts generic OAuth %s to registered read permissions',
333336
async (phase) => {
334337
const { mcpServer } = await create()
335-
const issuer = 'https://oauth.fixture.test'
338+
const issuer = connector.authorizationServer
336339
const loadProvider = async () =>
337340
new SimMcpOauthProvider({
338341
row: await getOrCreateOauthRow({ mcpServerId: mcpServer.id, organizationId: org }),
@@ -427,7 +430,7 @@ describe.each(SHARED_CLIENTS)('$name shared member connector', (connector) => {
427430
})
428431
let exchanges = 0
429432
let challenge: string | null = null
430-
const issuer = 'https://oauth.fixture.test'
433+
const issuer = connector.authorizationServer
431434
/** Only provider HTTP is substituted; SDK, PKCE, Redis, use cases, encryption and database are real. */
432435
const fetchFn: typeof fetch = async (request, init) => {
433436
const url = new URL(

‎apps/sim/lib/mcp/oauth/revoke.test.ts‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -73,7 +73,11 @@ describe('revokeMcpOauthTokens — SSRF guard', () => {
7373
resetDbChainMock()
7474

7575
mockLoadOauthRow.mockResolvedValue({
76-
tokens: { access_token: 'access-secret', refresh_token: 'refresh-secret' },
76+
tokens: {
77+
access_token: 'access-secret',
78+
refresh_token: 'refresh-secret',
79+
issuer: PUBLIC_SERVER_URL,
80+
},
7781
clientInformation: { client_id: 'client-123' },
7882
})
7983

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

8690
mockDiscoverOAuthServerInfo.mockResolvedValue({
91+
authorizationServerUrl: PUBLIC_SERVER_URL,
8792
authorizationServerMetadata: {
8893
issuer: PUBLIC_SERVER_URL,
8994
revocation_endpoint: BLOCKED_ENDPOINT,

‎apps/sim/lib/mcp/oauth/revoke.ts‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -43,8 +43,8 @@ export async function revokeMcpOauthTokens(
4343
)
4444
if (
4545
!info ||
46-
(row.tokens.issuer &&
47-
!isSameAuthorizationServer(row.tokens.issuer, info.authorizationServerUrl))
46+
!row.tokens.issuer ||
47+
!isSameAuthorizationServer(row.tokens.issuer, info.authorizationServerUrl)
4848
) {
4949
return
5050
}
@@ -87,15 +87,15 @@ export async function revokeMcpOauthTokens(
8787
}
8888

8989
/**
90-
* Mirrors the SDK's issuer binding: tokens stamped for one authorization server are never
91-
* posted, with the client secret, to a different one the MCP server now advertises.
90+
* Tokens are only posted, with the client secret, to the authorization server the SDK stamped
91+
* them for. Unstamped legacy tokens are never revoked: their issuer cannot be verified.
9292
*/
9393
function isSameAuthorizationServer(issuer: string, authorizationServerUrl: string): boolean {
9494
const normalize = (value: string) => {
9595
try {
96-
return new URL(value).href.replace(/\/$/, '')
96+
return new URL(value).href
9797
} catch {
98-
return value.replace(/\/$/, '')
98+
return value
9999
}
100100
}
101101
return normalize(issuer) === normalize(authorizationServerUrl)

0 commit comments

Comments
 (0)