Skip to content

Commit 75cba46

Browse files
committed
fix(search-mcp): preserve existing API OAuth grants
1 parent e3fa8bc commit 75cba46

7 files changed

Lines changed: 105 additions & 15 deletions

File tree

apps/sim/lib/api/server/routes/v2-api-key-auth.test.ts

Lines changed: 39 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,6 @@ const mocks = vi.hoisted(() => ({
1313

1414
vi.mock('@/lib/core/config/env-flags', () => mocks.envFlags)
1515
vi.mock('@/lib/api-key/crypto', () => ({ hashApiKey: (value: string) => `hash:${value}` }))
16-
vi.mock('@/lib/auth/oauth-provider', () => ({ OAUTH_ACCESS_TOKEN_PREFIX: 'sim_oat_' }))
1716
vi.mock('@sim/security/hash', () => ({ sha256Hex: (value: string) => `oauth-hash:${value}` }))
1817
vi.mock('@/lib/api-key/service', () => ({ updateApiKeyLastUsed: mocks.updateLastUsed }))
1918
vi.mock('@/lib/billing/core/billing-attribution', () => ({
@@ -284,6 +283,45 @@ describe('v2 bearer token authentication', () => {
284283
expect(result.keyType).toBe('personal')
285284
})
286285

286+
it.each(['api:read', 'api:write'])(
287+
'authenticates existing %s grants when Search MCP opts in',
288+
async (scope) => {
289+
queueTableRows(schemaMock.oauthAccessToken, [tokenRow({ resource: null, scopes: [scope] })])
290+
291+
await expect(
292+
authenticateV2ApiKey(
293+
{ apiKey: null, bearer: 'sim_oat_secret' },
294+
{
295+
resource: 'https://sim.example/api/mcp/search/organizations/one',
296+
allowUnboundApiTokens: true,
297+
}
298+
)
299+
).resolves.toMatchObject({
300+
principal: { kind: 'oauth_access_token', userId: 'user-1', scopes: [scope] },
301+
})
302+
}
303+
)
304+
305+
it('still refuses invalid or differently bound API grants when Search MCP opts in', async () => {
306+
for (const overrides of [
307+
{ resource: 'https://sim.example/api/mcp/search/organizations/other' },
308+
{ expiresAt: new Date(0) },
309+
{ clientDisabled: true },
310+
{ userBanned: true },
311+
]) {
312+
queueTableRows(schemaMock.oauthAccessToken, [tokenRow(overrides)])
313+
await expect(
314+
authenticateV2ApiKey(
315+
{ apiKey: null, bearer: 'sim_oat_secret' },
316+
{
317+
resource: 'https://sim.example/api/mcp/search/organizations/one',
318+
allowUnboundApiTokens: true,
319+
}
320+
)
321+
).rejects.toBeInstanceOf(V2ApiKeyUnauthenticatedError)
322+
}
323+
})
324+
287325
it('preserves auth-disabled deployment behavior without verifying an OAuth token', async () => {
288326
mocks.envFlags.isAuthDisabled = true
289327

apps/sim/lib/api/server/routes/v2-api-key-auth.ts

Lines changed: 13 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,11 @@ import type { V2CredentialHeaders } from '@/lib/api/server/routes/v2-credential-
1111
import { hashApiKey } from '@/lib/api-key/crypto'
1212
import { updateApiKeyLastUsed } from '@/lib/api-key/service'
1313
import { ANONYMOUS_USER_ID } from '@/lib/auth/constants'
14-
import { InvalidOAuthAccessTokenError, verifyOAuthAccessToken } from '@/lib/auth/oauth-access-token'
14+
import {
15+
InvalidOAuthAccessTokenError,
16+
type OAuthAccessTokenOptions,
17+
verifyOAuthAccessToken,
18+
} from '@/lib/auth/oauth-access-token'
1519
import { resolveWorkspaceBillingPayer } from '@/lib/billing/core/billing-attribution'
1620
import { getHighestPrioritySubscription } from '@/lib/billing/core/subscription'
1721
import { isAuthDisabled } from '@/lib/core/config/env-flags'
@@ -156,11 +160,14 @@ async function authenticateApiKey(apiKeyHeader: string): Promise<V2ApiKeyAuthCon
156160
* token and per user, on the user's own plan. A client that holds many tokens
157161
* for one user still shares that user's bucket.
158162
*/
159-
async function authenticateBearer(token: string, resource?: string): Promise<V2ApiKeyAuthContext> {
163+
async function authenticateBearer(
164+
token: string,
165+
options: OAuthAccessTokenOptions
166+
): Promise<V2ApiKeyAuthContext> {
160167
let principal: OAuthAccessTokenPrincipal
161168
try {
162-
principal = resource
163-
? await verifyOAuthAccessToken(token, { resource })
169+
principal = options.resource
170+
? await verifyOAuthAccessToken(token, options)
164171
: await verifyOAuthAccessToken(token)
165172
} catch (error) {
166173
if (error instanceof InvalidOAuthAccessTokenError) {
@@ -188,7 +195,7 @@ async function authenticateBearer(token: string, resource?: string): Promise<V2A
188195
*/
189196
export async function authenticateV2ApiKey(
190197
credential: V2CredentialHeaders,
191-
options: { resource?: string } = {}
198+
options: OAuthAccessTokenOptions = {}
192199
): Promise<V2ApiKeyAuthContext> {
193200
if (isAuthDisabled) {
194201
return {
@@ -204,7 +211,7 @@ export async function authenticateV2ApiKey(
204211
}
205212
}
206213
if (credential.apiKey) return authenticateApiKey(credential.apiKey)
207-
if (credential.bearer) return authenticateBearer(credential.bearer, options.resource)
214+
if (credential.bearer) return authenticateBearer(credential.bearer, options)
208215
if (credential.malformedOAuthBearer) {
209216
throw new V2ApiKeyUnauthenticatedError('Invalid access token', 'bearer')
210217
}

apps/sim/lib/auth/oauth-access-token.test.ts

Lines changed: 26 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,6 @@
44
import { dbChainMockFns, queueTableRows, resetDbChainMock, schemaMock } from '@sim/testing'
55
import { beforeEach, describe, expect, it, vi } from 'vitest'
66

7-
vi.mock('@/lib/auth/oauth-provider', () => ({ OAUTH_ACCESS_TOKEN_PREFIX: 'sim_oat_' }))
87
vi.mock('@sim/security/hash', () => ({ sha256Hex: (value: string) => `hash:${value}` }))
98

109
import {
@@ -146,4 +145,30 @@ describe('verifyOAuthAccessToken', () => {
146145
reason: 'wrong_resource',
147146
})
148147
})
148+
149+
it.each(['api:read', 'api:write'])(
150+
'preserves %s tokens only when the resource explicitly accepts API grants',
151+
async (scope) => {
152+
const resource = 'https://sim.example/api/mcp/search/organizations/one'
153+
queueTableRows(schemaMock.oauthAccessToken, [row({ scopes: [scope] })])
154+
await expect(
155+
verifyOAuthAccessToken('sim_oat_api', { resource, allowUnboundApiTokens: true })
156+
).resolves.toMatchObject({ userId: 'user-1', scopes: [scope] })
157+
}
158+
)
159+
160+
it('never relaxes audience checks for Search-only or differently bound grants', async () => {
161+
const resource = 'https://sim.example/api/mcp/search/organizations/one'
162+
for (const token of [
163+
row({ scopes: ['search:read'] }),
164+
row({ scopes: ['offline_access'] }),
165+
row({ resource: `${resource}-other`, scopes: ['api:read'] }),
166+
row({ resource: `${resource}-other`, scopes: ['search:read'] }),
167+
]) {
168+
queueTableRows(schemaMock.oauthAccessToken, [token])
169+
await expect(
170+
verifyOAuthAccessToken('sim_oat_other', { resource, allowUnboundApiTokens: true })
171+
).rejects.toMatchObject({ reason: 'wrong_resource' })
172+
}
173+
})
149174
})

apps/sim/lib/auth/oauth-access-token.ts

Lines changed: 18 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,11 @@ import { createLogger } from '@sim/logger'
55
import { sha256Hex } from '@sim/security/hash'
66
import { eq } from 'drizzle-orm'
77
import { isAccountBlocked } from '@/lib/auth/ban'
8-
import { OAUTH_ACCESS_TOKEN_PREFIX } from '@/lib/auth/oauth-provider'
8+
import {
9+
OAUTH_ACCESS_TOKEN_PREFIX,
10+
OAUTH_API_READ_SCOPE,
11+
oauthScopeSatisfies,
12+
} from '@/lib/auth/oauth-provider'
913

1014
const logger = createLogger('OAuthAccessToken')
1115

@@ -66,6 +70,12 @@ function looksLikeOAuthAccessToken(token: string): boolean {
6670
return token.startsWith(OAUTH_ACCESS_TOKEN_PREFIX)
6771
}
6872

73+
export interface OAuthAccessTokenOptions {
74+
resource?: string
75+
/** Search MCP also accepts existing full-API grants; Search-only tokens still need an audience. */
76+
allowUnboundApiTokens?: boolean
77+
}
78+
6979
/**
7080
* Resolves an opaque OAuth access token to the principal it stands for.
7181
*
@@ -78,7 +88,7 @@ function looksLikeOAuthAccessToken(token: string): boolean {
7888
*/
7989
export async function verifyOAuthAccessToken(
8090
token: string,
81-
options: { resource?: string } = {}
91+
options: OAuthAccessTokenOptions = {}
8292
): Promise<OAuthAccessTokenPrincipal> {
8393
if (!looksLikeOAuthAccessToken(token)) throw new InvalidOAuthAccessTokenError('malformed')
8494
const raw = token.slice(OAUTH_ACCESS_TOKEN_PREFIX.length)
@@ -105,7 +115,12 @@ export async function verifyOAuthAccessToken(
105115
.limit(1)
106116

107117
if (!row) throw new InvalidOAuthAccessTokenError('unknown')
108-
if ((row.resource ?? null) !== (options.resource ?? null)) {
118+
const acceptsUnboundApiToken =
119+
options.allowUnboundApiTokens &&
120+
options.resource &&
121+
row.resource == null &&
122+
oauthScopeSatisfies(row.scopes, OAUTH_API_READ_SCOPE)
123+
if ((row.resource ?? null) !== (options.resource ?? null) && !acceptsUnboundApiToken) {
109124
throw new InvalidOAuthAccessTokenError('wrong_resource')
110125
}
111126
if (row.expiresAt <= new Date()) throw new InvalidOAuthAccessTokenError('expired')

apps/sim/lib/knowledge/mcp/route-handler.test.ts

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -169,7 +169,7 @@ describe('organization MCP request admission', () => {
169169
expect(mocks.close).toHaveBeenCalledOnce()
170170
expect(mocks.authenticate).toHaveBeenCalledWith(
171171
{ apiKey: 'personal-key', bearer: null },
172-
{ resource }
172+
{ resource, allowUnboundApiTokens: true }
173173
)
174174
})
175175
it('preserves API keys supplied in the MCP bearer header', async () => {
@@ -178,7 +178,7 @@ describe('organization MCP request admission', () => {
178178
expect((await post(req)).status).toBe(200)
179179
expect(mocks.authenticate).toHaveBeenCalledWith(
180180
{ apiKey: 'personal-key', bearer: null },
181-
{ resource }
181+
{ resource, allowUnboundApiTokens: true }
182182
)
183183
})
184184
it('authenticates OAuth bearer tokens and checks organization membership', async () => {
@@ -198,7 +198,10 @@ describe('organization MCP request admission', () => {
198198
keyType: 'oauth',
199199
})
200200
expect((await post(req)).status).toBe(200)
201-
expect(mocks.authenticate).toHaveBeenCalledWith({ apiKey: null, bearer: token }, { resource })
201+
expect(mocks.authenticate).toHaveBeenCalledWith(
202+
{ apiKey: null, bearer: token },
203+
{ resource, allowUnboundApiTokens: true }
204+
)
202205
expect(mocks.index).toHaveBeenCalledWith({ kind: 'organization', organizationId: 'org-1' })
203206
})
204207
it('rejects workspace API keys even if the workspace ID matches the organization ID', async () => {

apps/sim/lib/knowledge/mcp/route-handler.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,7 @@ function mcpAuth(resource: string) {
3636
apiKey: apiKey ?? (oauthBearer ? null : (bearer ?? null)),
3737
bearer: oauthBearer,
3838
},
39-
{ resource }
39+
{ resource, allowUnboundApiTokens: true }
4040
)
4141
},
4242
}

packages/db/schema.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4247,6 +4247,7 @@ export const oauthRefreshToken = pgTable(
42474247
'oauth_refresh_token_generation_check',
42484248
sql`${table.generation} BETWEEN 0 AND 1000`
42494249
),
4250+
/** contract-pending(after #7613 is fully deployed): validate oauth_refresh_token_search_resource_check separately so rollout avoids a token-table scan. */
42504251
searchResourceCheck: check(
42514252
'oauth_refresh_token_search_resource_check',
42524253
sql`NOT ('search:read' = ANY(${table.scopes})) OR ${table.resource} IS NOT NULL`
@@ -4281,6 +4282,7 @@ export const oauthAccessToken = pgTable(
42814282
userClientIdx: index('oauth_access_token_user_client_idx').on(table.userId, table.clientId),
42824283
/** Drives the cleanup pass; nothing else reads tokens by expiry. */
42834284
expiresAtIdx: index('oauth_access_token_expires_at_idx').on(table.expiresAt),
4285+
/** contract-pending(after #7613 is fully deployed): validate oauth_access_token_search_resource_check separately so rollout avoids a token-table scan. */
42844286
searchResourceCheck: check(
42854287
'oauth_access_token_search_resource_check',
42864288
sql`NOT ('search:read' = ANY(${table.scopes})) OR ${table.resource} IS NOT NULL`

0 commit comments

Comments
 (0)