Skip to content

Commit 96ab7c8

Browse files
authored
fix(mothership): keep untagged mcp discovery side-effect free and the catalog resilient (#9266)
* fix(mothership): keep untagged mcp discovery side-effect free and the catalog resilient Broad agent and plan catalog queries now discover every usable workspace MCP server (#9263), but a server's status, failure count, cooldown and OAuth row are shared by the workspace while its credentials resolve per requester. A member without a personal {{VAR}} the server references made discovery fail, which stamped the server failing and put it in a workspace-wide cooldown, so another member's tagged or targeted discovery and call re-resolution failed with McpServerCooldownError. For an OAuth server the requester never connected, discovery inserted an empty OAuth row and marked the server disconnected for everyone. - Best-effort (untagged, broad) discovery is a McpService mode that only reads shared health state: on failure it writes no status, failure count, cooldown or cache eviction, and on success only warms the tool cache. - Before reading the cache or connecting, it skips a server the requester cannot authenticate to: a referenced variable they have no value for (findMissingMcpConfigEnvVars, which records no secret usage), or an OAuth server with no authorized tokens (read, never created). Another member's cached tools are not served to them. - Tagged and targeted discovery keep today's health bookkeeping. - A failure listing the workspace's servers no longer fails the whole catalog: broad requests log it and keep built-ins and tagged servers; targeted MCP requests still fail with it. - A query with no content terms after tokenization (stop words only, or a script the tokenizer does not split) returns the unranked listing, as an empty query does, instead of total 0. Exact-ID ranking is unchanged. - A tagged server that is no longer usable is logged when dropped. * fix(mothership): gate best-effort mcp discovery on side-effect-free credential reads The best-effort gate read the OAuth row with loadOauthRow, which clears a column it cannot decrypt, so a broad catalog query after a key rotation erased the one grant every member shares. hasDecryptableOauthTokens reads the tokens column and tries to decrypt it without writing anything. findMissingMcpConfigEnvVars took a variable that failed to decrypt (which the environment snapshot reports as '') as present, so best-effort discovery connected with an empty credential instead of skipping. An empty effective value now counts as missing. * fix(mothership): keep the shared mcp oauth grant intact on best-effort refresh failures Best-effort discovery could still run the SDK's OAuth recovery while listing tools. A rejected refresh made it clear the tokens (and, for a rejected client, the registration) and start a new authorization flow, which disconnected the shared grant for every member. Its OAuth clients now use a SimMcpOauthProvider with preserveGrant, which refuses to invalidate credentials, register a client or start authorization before any write, so a failed refresh skips the server. A successful refresh still persists. It keeps the grant valid for everyone and must be saved where the server rotates refresh tokens. These clients are pooled apart from tracked ones. findMissingMcpConfigEnvVars counted every empty value as missing, but an empty variable is valid. Only a key the snapshot names in decryptionFailures whose effective value is empty now counts as missing.
1 parent 36dc539 commit 96ab7c8

14 files changed

Lines changed: 596 additions & 58 deletions

File tree

‎apps/sim/lib/mcp/application/use-cases.ts‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -143,6 +143,8 @@ export interface DiscoverMcpServerToolsInput {
143143
refresh?: boolean
144144
signal?: AbortSignal
145145
requireComplete?: boolean
146+
/** Discovery nobody asked for: reads the server's shared health state but never writes it. */
147+
bestEffort?: boolean
146148
}
147149

148150
/**
@@ -192,14 +194,18 @@ export const discoverMcpServerToolsUseCase = defineAuthorizedWorkspaceUseCase({
192194
})
193195
const refresh = input.refresh ? 'skip-cache' : 'cache-aside'
194196
const tools =
195-
input.signal || input.requireComplete
197+
input.signal || input.requireComplete || input.bestEffort
196198
? await mcpService.discoverServerTools(
197199
userId,
198200
context.server.id,
199201
context.workspaceId,
200202
refresh,
201203
undefined,
202-
{ signal: input.signal, requireComplete: input.requireComplete }
204+
{
205+
signal: input.signal,
206+
requireComplete: input.requireComplete,
207+
bestEffort: input.bestEffort,
208+
}
203209
)
204210
: await mcpService.discoverServerTools(
205211
userId,

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ export {
1818
clearTokens,
1919
clearVerifier,
2020
getOrCreateOauthRow,
21+
hasDecryptableOauthTokens,
2122
loadOauthRow,
2223
loadOauthRowByState,
2324
type McpOauthRow,
Lines changed: 94 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,94 @@
1+
import { auth } from '@modelcontextprotocol/sdk/client/auth.js'
2+
import { dbChainMockFns, encryptionMock, resetDbChainMock } from '@sim/testing'
3+
import { beforeEach, describe, expect, it, vi } from 'vitest'
4+
5+
vi.mock('@/lib/core/security/encryption', () => encryptionMock)
6+
7+
import { SimMcpOauthProvider } from '@/lib/mcp/oauth/provider'
8+
import type { McpOauthRow } from '@/lib/mcp/oauth/storage'
9+
10+
const SERVER_URL = 'https://mcp.example.com/mcp'
11+
const ISSUER = 'https://auth.example.com'
12+
13+
function grantRow(): McpOauthRow {
14+
return {
15+
id: 'oauth-row-1',
16+
mcpServerId: 'server-1',
17+
userId: 'authorizer-1',
18+
workspaceId: 'workspace-1',
19+
organizationId: null,
20+
clientInformation: { client_id: 'client-1', issuer: ISSUER },
21+
tokens: { access_token: 'expired', token_type: 'bearer', refresh_token: 'refresh-1' },
22+
codeVerifier: null,
23+
state: null,
24+
stateCreatedAt: null,
25+
updatedAt: new Date(),
26+
}
27+
}
28+
29+
/** An authorization server whose token endpoint answers every refresh with `token`. */
30+
function authorizationServer(token: { status: number; body: Record<string, unknown> }) {
31+
return vi.fn(async (input: string | URL) => {
32+
const url = String(input)
33+
const json = (status: number, body: unknown) =>
34+
new Response(JSON.stringify(body), {
35+
status,
36+
headers: { 'content-type': 'application/json' },
37+
})
38+
if (url.startsWith('https://mcp.example.com/.well-known/oauth-protected-resource'))
39+
return json(200, { resource: SERVER_URL, authorization_servers: [ISSUER] })
40+
if (url.startsWith(`${ISSUER}/.well-known/`))
41+
return json(200, {
42+
issuer: ISSUER,
43+
authorization_endpoint: `${ISSUER}/authorize`,
44+
token_endpoint: `${ISSUER}/token`,
45+
response_types_supported: ['code'],
46+
code_challenge_methods_supported: ['S256'],
47+
})
48+
if (url === `${ISSUER}/token`) return json(token.status, token.body)
49+
return json(404, {})
50+
})
51+
}
52+
53+
/**
54+
* Best-effort discovery connects with the workspace's one shared grant. A refresh that fails
55+
* there must leave the grant as it was: the SDK otherwise clears the tokens and starts a new
56+
* authorization flow, disconnecting the server for every member.
57+
*/
58+
describe('SimMcpOauthProvider preserving the shared grant', () => {
59+
beforeEach(() => {
60+
resetDbChainMock()
61+
})
62+
63+
it('writes nothing to the OAuth row when a refresh is rejected', async () => {
64+
const provider = new SimMcpOauthProvider({ row: grantRow(), preserveGrant: true })
65+
66+
await expect(
67+
auth(provider, {
68+
serverUrl: SERVER_URL,
69+
fetchFn: authorizationServer({ status: 400, body: { error: 'invalid_grant' } }),
70+
})
71+
).rejects.toThrow('MCP OAuth grant left unchanged')
72+
73+
expect(dbChainMockFns.update).not.toHaveBeenCalled()
74+
expect(dbChainMockFns.insert).not.toHaveBeenCalled()
75+
expect(provider.tokens()?.refresh_token).toBe('refresh-1')
76+
})
77+
78+
it('persists a successful refresh, which keeps the grant valid for everyone', async () => {
79+
const provider = new SimMcpOauthProvider({ row: grantRow(), preserveGrant: true })
80+
81+
await expect(
82+
auth(provider, {
83+
serverUrl: SERVER_URL,
84+
fetchFn: authorizationServer({
85+
status: 200,
86+
body: { access_token: 'fresh', token_type: 'bearer', refresh_token: 'refresh-2' },
87+
}),
88+
})
89+
).resolves.toBe('AUTHORIZED')
90+
91+
expect(dbChainMockFns.update).toHaveBeenCalledTimes(1)
92+
expect(provider.tokens()?.access_token).toBe('fresh')
93+
})
94+
})

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

Lines changed: 25 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -71,17 +71,37 @@ interface SimMcpOauthProviderInit {
7171
* Dynamic Client Registration and uses these for the auth/token exchange.
7272
*/
7373
preregistered?: PreregisteredClient
74+
/**
75+
* The grant belongs to every member and this caller did not ask to reauthorize it: a
76+
* refreshed token still persists, but clearing the grant, registering a client or starting
77+
* an authorization flow fails with {@link McpOauthGrantPreservedError} before any write.
78+
*/
79+
preserveGrant?: boolean
80+
}
81+
82+
/** A refresh failed where reauthorizing the shared grant is left to an explicit reconnect. */
83+
class McpOauthGrantPreservedError extends Error {
84+
constructor(step: string) {
85+
super(`MCP OAuth grant left unchanged: ${step} requires an explicit reconnect`)
86+
this.name = 'McpOauthGrantPreservedError'
87+
}
7488
}
7589

7690
export class SimMcpOauthProvider implements OAuthClientProvider {
7791
private row: McpOauthRow
7892
private readonly scope?: string
7993
private readonly preregistered?: PreregisteredClient
94+
private readonly preserveGrant: boolean
8095

81-
constructor({ row, scope, preregistered }: SimMcpOauthProviderInit) {
96+
constructor({ row, scope, preregistered, preserveGrant = false }: SimMcpOauthProviderInit) {
8297
this.row = row
8398
this.scope = preregistered?.scope ?? scope
8499
this.preregistered = preregistered
100+
this.preserveGrant = preserveGrant
101+
}
102+
103+
private assertMayChangeGrant(step: string): void {
104+
if (this.preserveGrant) throw new McpOauthGrantPreservedError(step)
85105
}
86106

87107
/** Deployment registrations may restrict consent even when discovery advertises more tools. */
@@ -108,6 +128,7 @@ export class SimMcpOauthProvider implements OAuthClientProvider {
108128
}
109129

110130
async state(): Promise<string> {
131+
this.assertMayChangeGrant('authorization')
111132
const state = generateId()
112133
await saveState(this.row.id, state, 'provider.state')
113134
return state
@@ -132,6 +153,7 @@ export class SimMcpOauthProvider implements OAuthClientProvider {
132153

133154
async saveClientInformation(info: OAuthClientInformationMixed): Promise<void> {
134155
if (this.preregistered) return
156+
this.assertMayChangeGrant('client registration')
135157
await saveClientInformationDb(this.row.id, info)
136158
this.row.clientInformation = info
137159
}
@@ -150,6 +172,7 @@ export class SimMcpOauthProvider implements OAuthClientProvider {
150172
}
151173

152174
async saveCodeVerifier(codeVerifier: string): Promise<void> {
175+
this.assertMayChangeGrant('authorization')
153176
await saveCodeVerifierDb(this.row.id, codeVerifier)
154177
this.row.codeVerifier = codeVerifier
155178
}
@@ -164,6 +187,7 @@ export class SimMcpOauthProvider implements OAuthClientProvider {
164187
async invalidateCredentials(
165188
scope: 'all' | 'client' | 'tokens' | 'verifier' | 'discovery'
166189
): Promise<void> {
190+
if (scope !== 'discovery') this.assertMayChangeGrant(`invalidating ${scope} credentials`)
167191
if (scope === 'all' || scope === 'client') {
168192
await clearClient(this.row.id)
169193
this.row.clientInformation = null

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

Lines changed: 21 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,12 @@ afterAll(resetRedisConfigMock)
1414

1515
vi.mock('@/lib/core/security/encryption', () => encryptionMock)
1616

17-
import { getOrCreateOauthRow, loadOauthRow, withMcpOauthRefreshLock } from './storage'
17+
import {
18+
getOrCreateOauthRow,
19+
hasDecryptableOauthTokens,
20+
loadOauthRow,
21+
withMcpOauthRefreshLock,
22+
} from './storage'
1823

1924
describe('MCP OAuth storage', () => {
2025
beforeEach(() => {
@@ -77,6 +82,21 @@ describe('MCP OAuth storage', () => {
7782
expect(row.userId).toBe('authorizer-1')
7883
expect(dbChainMockFns.insert).not.toHaveBeenCalled()
7984
})
85+
86+
/** The row is the workspace's one grant; a presence check must never be what erases it. */
87+
it('reports undecryptable tokens as unusable without clearing the shared row', async () => {
88+
dbChainMockFns.limit.mockResolvedValueOnce([{ tokens: 'ciphertext-from-a-rotated-key' }])
89+
encryptionMockFns.mockDecryptSecret.mockRejectedValueOnce(new Error('bad decrypt'))
90+
91+
expect(await hasDecryptableOauthTokens('server-1')).toBe(false)
92+
expect(dbChainMockFns.update).not.toHaveBeenCalled()
93+
expect(dbChainMockFns.set).not.toHaveBeenCalled()
94+
95+
dbChainMockFns.limit.mockResolvedValueOnce([{ tokens: 'ciphertext' }])
96+
expect(await hasDecryptableOauthTokens('server-1')).toBe(true)
97+
dbChainMockFns.limit.mockResolvedValueOnce([{ tokens: null }])
98+
expect(await hasDecryptableOauthTokens('server-1')).toBe(false)
99+
})
80100
})
81101

82102
describe('withMcpOauthRefreshLock', () => {

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

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -156,6 +156,26 @@ export async function loadOauthRow(params: { mcpServerId: string }): Promise<Mcp
156156
return mapOauthRow(row)
157157
}
158158

159+
/**
160+
* Whether the server's OAuth row holds tokens that decrypt, read without writing: unlike
161+
* {@link loadOauthRow}, an undecryptable column is reported, never cleared, so a caller that
162+
* only asks whether to try a connection cannot erase the grant every member shares.
163+
*/
164+
export async function hasDecryptableOauthTokens(mcpServerId: string): Promise<boolean> {
165+
const [row] = await db
166+
.select({ tokens: mcpServerOauth.tokens })
167+
.from(mcpServerOauth)
168+
.where(eq(mcpServerOauth.mcpServerId, mcpServerId))
169+
.limit(1)
170+
if (!row?.tokens) return false
171+
try {
172+
await decryptSecret(row.tokens)
173+
return true
174+
} catch {
175+
return false
176+
}
177+
}
178+
159179
export async function setOauthRowUser(rowId: string, userId: string): Promise<void> {
160180
await db
161181
.update(mcpServerOauth)

‎apps/sim/lib/mcp/resolve-config.test.ts‎

Lines changed: 61 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import { environmentUtilsMockFns } from '@sim/testing/mocks/environment-utils.mock'
22
import { beforeEach, describe, expect, it, vi } from 'vitest'
3-
import { resolveMcpConfigEnvVars } from '@/lib/mcp/resolve-config'
3+
import { findMissingMcpConfigEnvVars, resolveMcpConfigEnvVars } from '@/lib/mcp/resolve-config'
44

55
const mockGetEffectiveEnvironmentSnapshot =
66
environmentUtilsMockFns.mockGetEffectiveEnvironmentSnapshot
@@ -171,3 +171,63 @@ describe('resolveMcpConfigEnvVars secret provenance', () => {
171171
])
172172
})
173173
})
174+
175+
describe('findMissingMcpConfigEnvVars', () => {
176+
it('names the variables this requester has no value for, without reading env for literal configs', async () => {
177+
mockGetEffectiveEnvironmentSnapshot.mockReset()
178+
mockGetEffectiveEnvironmentSnapshot.mockResolvedValue({
179+
personalEncrypted: {},
180+
workspaceEncrypted: {},
181+
personalDecrypted: { MCP_HOST: 'api.example.com' },
182+
workspaceDecrypted: {},
183+
conflicts: [],
184+
decryptionFailures: [],
185+
})
186+
187+
expect(await findMissingMcpConfigEnvVars(BASE_CONFIG, 'user-b', 'workspace-1')).toEqual([
188+
'MCP_TOKEN',
189+
])
190+
expect(
191+
await findMissingMcpConfigEnvVars(
192+
{ ...BASE_CONFIG, url: 'https://api.example.com/mcp', headers: {} },
193+
'user-b',
194+
'workspace-1'
195+
)
196+
).toEqual([])
197+
expect(mockGetEffectiveEnvironmentSnapshot).toHaveBeenCalledTimes(1)
198+
})
199+
200+
it('counts a variable that failed to decrypt as missing', async () => {
201+
mockGetEffectiveEnvironmentSnapshot.mockResolvedValue({
202+
personalEncrypted: { MCP_HOST: 'host-encrypted', MCP_TOKEN: 'token-encrypted' },
203+
workspaceEncrypted: {},
204+
personalDecrypted: { MCP_HOST: 'api.example.com', MCP_TOKEN: '' },
205+
workspaceDecrypted: {},
206+
conflicts: [],
207+
decryptionFailures: ['MCP_TOKEN'],
208+
})
209+
210+
expect(await findMissingMcpConfigEnvVars(BASE_CONFIG, 'user-b', 'workspace-1')).toEqual([
211+
'MCP_TOKEN',
212+
])
213+
})
214+
215+
it('counts an intentionally empty value as present', async () => {
216+
mockGetEffectiveEnvironmentSnapshot.mockResolvedValue({
217+
personalEncrypted: { MCP_PATH_PREFIX: 'prefix-encrypted' },
218+
workspaceEncrypted: {},
219+
personalDecrypted: { MCP_PATH_PREFIX: '' },
220+
workspaceDecrypted: {},
221+
conflicts: [],
222+
decryptionFailures: [],
223+
})
224+
225+
expect(
226+
await findMissingMcpConfigEnvVars(
227+
{ ...BASE_CONFIG, url: 'https://example.com{{MCP_PATH_PREFIX}}/mcp', headers: {} },
228+
'user-b',
229+
'workspace-1'
230+
)
231+
).toEqual([])
232+
})
233+
})

‎apps/sim/lib/mcp/resolve-config.ts‎

Lines changed: 35 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,13 +6,14 @@
66

77
import { createLogger } from '@sim/logger'
88
import { getErrorMessage } from '@sim/utils/errors'
9+
import { omit } from '@sim/utils/object'
910
import {
1011
type EnvironmentResolutionSnapshot,
1112
getEffectiveEnvironmentSnapshot,
1213
} from '@/lib/environment/utils'
1314
import type { McpServerConfig } from '@/lib/mcp/types'
1415
import { recordSecretUsage } from '@/lib/secrets/usage/record'
15-
import { resolveEnvVarReferences } from '@/executor/utils/reference-validation'
16+
import { createEnvVarPattern, resolveEnvVarReferences } from '@/executor/utils/reference-validation'
1617
import {
1718
createIncompleteResolvedSecretTraceRegistry,
1819
createResolvedSecretTraceRegistry,
@@ -149,3 +150,36 @@ export async function resolveMcpConfigEnvVars(
149150
resolvedSecretTraceProvenance,
150151
}
151152
}
153+
154+
/**
155+
* The variables `config` references (url, headers) that `userId` has no value for in this
156+
* workspace. Read-only: it records no secret usage, so it can check whether a requester can
157+
* authenticate to a server before anything connects.
158+
*/
159+
export async function findMissingMcpConfigEnvVars(
160+
config: McpServerConfig,
161+
userId: string,
162+
workspaceId?: string
163+
): Promise<string[]> {
164+
const values = [config.url, ...Object.values(config.headers ?? {})].filter(
165+
(value): value is string => typeof value === 'string'
166+
)
167+
if (!values.some((value) => createEnvVarPattern().test(value))) return []
168+
const env = await getEffectiveEnvironmentSnapshot(userId, workspaceId)
169+
const merged = { ...env.personalDecrypted, ...env.workspaceDecrypted }
170+
/**
171+
* A value that failed to decrypt arrives as '' and is named in `decryptionFailures`; an
172+
* intentionally empty one is only ''. The list does not say which slice failed, so a key
173+
* counts as failed only when its effective value is also empty. The one ambiguity left, an
174+
* intentionally empty workspace value over a personal one that failed, counts as missing,
175+
* so best-effort discovery skips rather than connects.
176+
*/
177+
const failed = new Set(env.decryptionFailures)
178+
const envVars = omit(
179+
merged,
180+
Object.keys(merged).filter((key) => failed.has(key) && merged[key] === '')
181+
)
182+
const missingKeys: string[] = []
183+
for (const value of values) resolveEnvVarReferences(value, envVars, { missingKeys })
184+
return [...new Set(missingKeys)]
185+
}

0 commit comments

Comments
 (0)