diff --git a/apps/api/src/__tests__/route-policy-enforcement.test.ts b/apps/api/src/__tests__/route-policy-enforcement.test.ts index b05710d10..218c7152c 100644 --- a/apps/api/src/__tests__/route-policy-enforcement.test.ts +++ b/apps/api/src/__tests__/route-policy-enforcement.test.ts @@ -16,6 +16,14 @@ const redisState = vi.hoisted(() => ({ })); const mcpAuthState = vi.hoisted(() => ({ userId: 'user-123' })); +// Route-policy unit fixtures deliberately stand in for authenticated ingress. +// Real middleware and Redis enforcement are exercised by the HTTP access suite. +vi.mock('../middleware/tokenAuthMiddleware', async (original) => ({ + ...(await original()), + getRequestAuth: (c: Context<{ Variables: Variables }>) => + c.get('authContext'), +})); + vi.mock('@roomote/redis', async (importOriginal) => { const actual = await importOriginal(); diff --git a/apps/api/src/handlers/mcp/__tests__/models.access.test.ts b/apps/api/src/handlers/mcp/__tests__/models.access.test.ts index a5cb0f77e..d19375177 100644 --- a/apps/api/src/handlers/mcp/__tests__/models.access.test.ts +++ b/apps/api/src/handlers/mcp/__tests__/models.access.test.ts @@ -6,6 +6,7 @@ import { createAuthToken, configureAuthClientEnv, createMcpAccessToken, + getLegacyRoomoteMcpResourceUrl, getRoomoteMcpResourceUrl, ROOMOTE_MCP_SCOPE, } from '@roomote/auth'; @@ -20,7 +21,12 @@ import { Env } from '@roomote/env'; import type { TaskModelOption } from '@roomote/types'; import { createApiApp } from '../../../server'; import { oauthGrantFixture } from './oauth-fixture'; -import { revokeMcpOAuthGrants } from '@roomote/redis'; +import { isMcpOAuthGrantLive, revokeMcpOAuthGrants } from '@roomote/redis'; + +vi.mock('@roomote/redis', async (original) => { + const actual = await original(); + return { ...actual, isMcpOAuthGrantLive: vi.fn(actual.isMcpOAuthGrantLive) }; +}); describe('model discovery through authenticated HTTP', () => { beforeAll(() => { @@ -98,10 +104,14 @@ describe('model discovery through authenticated HTTP', () => { }); const receipts: unknown[] = []; async function get(path: string, bearer: string | null = token) { + vi.mocked(isMcpOAuthGrantLive).mockClear(); const response = await fetch(`${base}${path}`, { headers: bearer ? { Authorization: `Bearer ${bearer}` } : {}, }); const body = await response.json(); + expect( + vi.mocked(isMcpOAuthGrantLive).mock.calls.length, + ).toBeLessThanOrEqual(1); receipts.push({ method: 'GET', path, @@ -163,6 +173,7 @@ describe('model discovery through authenticated HTTP', () => { arguments: { action: 'list_models', limit: 1, query: 'GPT' }, }, }; + vi.mocked(isMcpOAuthGrantLive).mockClear(); const result = await fetch(`${base}/mcp`, { method: 'POST', headers: { @@ -177,7 +188,69 @@ describe('model discovery through authenticated HTTP', () => { expect(text).toContain(models[1]!.id); expect(text).not.toContain('Unknown action'); expect(text).not.toContain('"isError":true'); + expect(isMcpOAuthGrantLive).toHaveBeenCalledOnce(); + const legacyResource = getLegacyRoomoteMcpResourceUrl(Env.TRPC_URL); + const legacy = await createMcpAccessToken({ + ...(await oauthGrantFixture(member.id, legacyResource)), + userId: member.id, + resource: legacyResource, + scopes: [ROOMOTE_MCP_SCOPE], + timeoutMs: 60000, + }); + const initialize = async (path: string, bearer: string) => { + vi.mocked(isMcpOAuthGrantLive).mockClear(); + const response = await fetch(`${base}${path}`, { + method: 'POST', + headers: { + Authorization: `Bearer ${bearer}`, + 'Content-Type': 'application/json', + Accept: 'application/json, text/event-stream', + }, + body: JSON.stringify({ + jsonrpc: '2.0', + id: 1, + method: 'initialize', + params: { + protocolVersion: '2025-03-26', + capabilities: {}, + clientInfo: { name: 'auth-test', version: '1' }, + }, + }), + }); + await response.text(); + expect(isMcpOAuthGrantLive).toHaveBeenCalledOnce(); + return response.status; + }; + expect(await initialize('/api/mcp-routing/roomote', legacy)).toBe(200); + expect(vi.mocked(isMcpOAuthGrantLive).mock.calls[0]![0]).toMatchObject({ + resource: legacyResource, + }); + expect(await initialize('/mcp', legacy)).toBe(403); + expect((await get('/mcp/models', legacy)).response.status).toBe(403); + expect( + (await get('/mcp/task-outputs/download', legacy)).response.status, + ).toBe(403); + // A live-grant storage failure must not fall back to member access on any + // mounted OAuth surface, and the next request must try Redis again. + for (const path of ['/mcp/models', '/mcp/task-outputs/download']) { + vi.mocked(isMcpOAuthGrantLive).mockRejectedValueOnce( + new Error('Redis unavailable'), + ); + expect((await get(path)).response.status).toBe(401); + expect(isMcpOAuthGrantLive).toHaveBeenCalledOnce(); + } + for (const [path, bearer] of [ + ['/mcp', token], + ['/api/mcp-routing/roomote', legacy], + ] as const) { + vi.mocked(isMcpOAuthGrantLive).mockRejectedValueOnce( + new Error('Redis unavailable'), + ); + expect(await initialize(path, bearer)).toBe(401); + } + expect((await get('/mcp/models')).response.status).toBe(200); await revokeMcpOAuthGrants(member.id); + expect(await initialize('/api/mcp-routing/roomote', legacy)).toBe(401); expect((await get('/mcp/models')).response.status).toBe(401); expect((await get('/mcp/task-outputs/download')).response.status).toBe( 401, @@ -204,6 +277,11 @@ describe('model discovery through authenticated HTTP', () => { expect((await get('/mcp/task-outputs/download')).response.status).toBe( 401, ); + await db + .update(users) + .set({ deletedAt: new Date() }) + .where(eq(users.id, member.id)); + expect((await get('/mcp/models', reconnected)).response.status).toBe(401); receipts.push({ method: 'POST', path: '/mcp', diff --git a/apps/api/src/handlers/mcp/__tests__/models.test.ts b/apps/api/src/handlers/mcp/__tests__/models.test.ts index 40c3a2296..518ad6597 100644 --- a/apps/api/src/handlers/mcp/__tests__/models.test.ts +++ b/apps/api/src/handlers/mcp/__tests__/models.test.ts @@ -4,15 +4,42 @@ import { Env } from '@roomote/env'; import type { TaskModelOption } from '@roomote/types'; import type { Variables } from '../../../types'; import { listMemberModels } from '../models'; +import { tokenAuthMiddleware } from '../../../middleware/tokenAuthMiddleware'; -const { options, grantLive } = vi.hoisted(() => ({ +const { options, grantLive, authState } = vi.hoisted(() => ({ options: vi.fn(), grantLive: vi.fn(), + authState: { credential: undefined as Variables['authContext'] }, +})); +vi.mock('@roomote/auth', async (original) => ({ + ...(await original()), + validateRunToken: vi.fn(async () => { + if (authState.credential?.tokenType === 'run') return authState.credential; + throw new Error('Not a run token'); + }), + validateMcpAccessToken: vi.fn(async () => { + if (authState.credential?.tokenType === 'mcp') return authState.credential; + throw new Error('Not an OAuth token'); + }), + validateAuthToken: vi.fn(async () => { + if (authState.credential?.tokenType === 'auth') return authState.credential; + throw new Error('Not a user token'); + }), })); vi.mock('@roomote/redis', () => ({ isMcpOAuthGrantLive: grantLive })); vi.mock('@roomote/db/server', async (original) => ({ ...(await original()), getDeploymentTaskModelOptions: options, + db: { + query: { + users: { + findFirst: vi.fn(async () => ({ id: 'member', deletedAt: null })), + }, + deploymentSettings: { + findFirst: vi.fn(async () => ({ metadata: null })), + }, + }, + }, })); const model = (id: string, displayName = id): TaskModelOption => ({ @@ -28,10 +55,12 @@ function app( }, ) { const router = new Hono<{ Variables: Variables }>(); + authState.credential = auth ?? undefined; router.use('*', async (c, next) => { - c.set('authContext', auth ?? undefined); + c.req.raw.headers.set('authorization', 'Bearer fixture'); await next(); }); + router.use('*', tokenAuthMiddleware()); router.get('/models', listMemberModels); return router; } @@ -48,6 +77,25 @@ describe('member model discovery', () => { ); }); + it('does not trust an authContext installed without authenticated ingress', async () => { + const router = new Hono<{ Variables: Variables }>(); + router.use('*', async (c, next) => { + c.set('authContext', { + userId: 'member', + tokenType: 'mcp', + version: 1, + resource: getRoomoteMcpResourceUrl(Env.R_PUBLIC_URL ?? Env.R_APP_URL), + scopes: [ROOMOTE_MCP_SCOPE], + clientId: 'fixture-client', + grantId: 'fixture-grant', + }); + await next(); + }); + router.get('/models', listMemberModels); + expect((await router.request('/models')).status).toBe(401); + expect(options).not.toHaveBeenCalled(); + }); + it('bounds large catalogs and traverses IDs without gaps or duplicates', async () => { const models = Array.from({ length: 235 }, (_, i) => model(`openai/example-${String(i).padStart(3, '0')}`), diff --git a/apps/api/src/handlers/mcp/__tests__/task-outputs.test.ts b/apps/api/src/handlers/mcp/__tests__/task-outputs.test.ts index dd319cf43..8d22fa2ab 100644 --- a/apps/api/src/handlers/mcp/__tests__/task-outputs.test.ts +++ b/apps/api/src/handlers/mcp/__tests__/task-outputs.test.ts @@ -1,7 +1,6 @@ -import type { Context, Next } from 'hono'; import { getRoomoteMcpResourceUrl } from '@roomote/auth'; import { Env } from '@roomote/env'; -import { revokeMcpOAuthGrants } from '@roomote/redis'; +import { isMcpOAuthGrantLive, revokeMcpOAuthGrants } from '@roomote/redis'; import { oauthGrantFixture } from './oauth-fixture'; import { automations, @@ -30,14 +29,26 @@ const state = vi.hoisted(() => ({ credentials: new Map(), object: vi.fn(), })); -vi.mock('../../../middleware', async (importOriginal) => ({ - ...(await importOriginal()), - tokenAuthMiddleware: - () => async (c: Context<{ Variables: Variables }>, next: Next) => { - const auth = state.credentials.get(c.req.header('authorization') ?? ''); - if (auth) c.set('authContext', auth); - await next(); - }, +vi.mock('@roomote/redis', async (original) => { + const actual = await original(); + return { ...actual, isMcpOAuthGrantLive: vi.fn(actual.isMcpOAuthGrantLive) }; +}); +vi.mock('@roomote/auth', async (importOriginal) => ({ + ...(await importOriginal()), + ...Object.fromEntries( + [ + ['validateRunToken', 'run'], + ['validateMcpAccessToken', 'mcp'], + ['validateAuthToken', 'auth'], + ].map(([name, kind]) => [ + name, + vi.fn(async (token: string) => { + const auth = state.credentials.get(`Bearer ${token}`); + if (auth?.tokenType === kind) return auth; + throw new Error('Invalid fixture credential'); + }), + ]), + ), })); vi.mock('../../artifacts/storage', async (importOriginal) => ({ ...(await importOriginal()), @@ -224,17 +235,20 @@ it('rejects previously issued download access immediately after its Redis grant ); const url = new URL(issued.data.url); const path = `${url.pathname}${url.search}`; + vi.mocked(isMcpOAuthGrantLive).mockClear(); expect( (await createApiApp().request(path, { headers: { authorization } })).status, ).toBe(200); + expect(isMcpOAuthGrantLive).toHaveBeenCalledOnce(); await revokeMcpOAuthGrants(owner.id); - // Keep the injected verified auth context: live Redis revocation, rather than - // removing a token from the test's authentication map, must reject the GET. + // Keep the signed-credential fixture: live Redis revocation must reject GET. expect(state.credentials.has(authorization)).toBe(true); state.object.mockClear(); + vi.mocked(isMcpOAuthGrantLive).mockClear(); expect( (await createApiApp().request(path, { headers: { authorization } })).status, ).toBe(401); + expect(isMcpOAuthGrantLive).toHaveBeenCalledOnce(); expect(state.object).not.toHaveBeenCalled(); }); diff --git a/apps/api/src/handlers/mcp/models.ts b/apps/api/src/handlers/mcp/models.ts index 4b6a5b35c..df95217c7 100644 --- a/apps/api/src/handlers/mcp/models.ts +++ b/apps/api/src/handlers/mcp/models.ts @@ -18,7 +18,7 @@ const cursorSchema = z export async function listMemberModels(c: Context<{ Variables: Variables }>) { c.header('Cache-Control', 'no-store, private'); try { - await resolveRoomoteMemberAuth(c.get('authContext')); + await resolveRoomoteMemberAuth(c); } catch (error) { if (error instanceof McpProxyError) return Response.json( diff --git a/apps/api/src/handlers/mcp/roomote-member-auth.ts b/apps/api/src/handlers/mcp/roomote-member-auth.ts index f8e60c45c..eec5b0fe7 100644 --- a/apps/api/src/handlers/mcp/roomote-member-auth.ts +++ b/apps/api/src/handlers/mcp/roomote-member-auth.ts @@ -1,16 +1,23 @@ -import { getRoomoteMcpResourceUrl, ROOMOTE_MCP_SCOPE } from '@roomote/auth'; +import type { Context } from 'hono'; +import { + getLegacyRoomoteMcpResourceUrl, + getRoomoteMcpResourceUrl, + ROOMOTE_MCP_SCOPE, +} from '@roomote/auth'; import { Env } from '@roomote/env'; -import { isMcpOAuthGrantLive } from '@roomote/redis'; import { isUserToken, PRODUCT_NAME } from '@roomote/types'; import type { Variables } from '../../types'; +import { getRequestAuth } from '../../middleware/tokenAuthMiddleware'; import type { McpAuth } from './middleware'; import { McpProxyError } from './proxy-utils'; /** Public member reads and downloads share the /mcp credential boundary. */ export async function resolveRoomoteMemberAuth( - auth: Variables['authContext'], + c: Context<{ Variables: Variables }>, + options: { allowLegacyAudience?: boolean } = {}, ): Promise { + const auth = c.get('authContext'); if (!auth) throw new McpProxyError(401, 'Authentication required'); if (auth.tokenType === 'run') { throw new McpProxyError( @@ -19,23 +26,20 @@ export async function resolveRoomoteMemberAuth( ); } if (isUserToken(auth)) return { userId: auth.userId, authContext: auth }; + // Internal in-process calls retain user/run semantics, but an OAuth object + // alone is not proof of live-grant authentication for this HTTP request. + if (getRequestAuth(c) !== auth) + throw new McpProxyError(401, 'Authentication required'); if ( auth.tokenType === 'mcp' && - auth.resource === - getRoomoteMcpResourceUrl(Env.R_PUBLIC_URL ?? Env.R_APP_URL) && + [ + getRoomoteMcpResourceUrl(Env.R_PUBLIC_URL ?? Env.R_APP_URL), + ...(options.allowLegacyAudience + ? [getLegacyRoomoteMcpResourceUrl(Env.TRPC_URL)] + : []), + ].includes(auth.resource) && auth.scopes.includes(ROOMOTE_MCP_SCOPE) ) { - let live = false; - try { - live = await isMcpOAuthGrantLive(auth); - } catch { - /* Fail closed. */ - } - if (!live) - throw new McpProxyError( - 401, - 'MCP authorization expired or revoked; reconnect your client', - ); return { userId: auth.userId, authContext: { diff --git a/apps/api/src/handlers/mcp/roomote.ts b/apps/api/src/handlers/mcp/roomote.ts index 6947567c2..c5073f240 100644 --- a/apps/api/src/handlers/mcp/roomote.ts +++ b/apps/api/src/handlers/mcp/roomote.ts @@ -1,4 +1,4 @@ -import { Hono } from 'hono'; +import { Hono, type Context } from 'hono'; import { NullableOptionalsMcpServer } from '@roomote/cloud-agents/mcp-nullable-optionals'; import { WebStandardStreamableHTTPServerTransport } from '@modelcontextprotocol/sdk/server/webStandardStreamableHttp.js'; import { @@ -22,20 +22,15 @@ import { CHAT_MESSAGE_CONTEXT_TOOL, environmentConfigSchema, MCP_INTEGRATIONS, - isUserToken, PRODUCT_NAME, } from '@roomote/types'; import { Env, getDefaultDocsUrl } from '@roomote/env'; -import { - getLegacyRoomoteMcpResourceUrl, - getRoomoteMcpResourceUrl, - ROOMOTE_MCP_SCOPE, -} from '@roomote/auth'; import { z } from 'zod'; import packageJson from '../../../../../package.json'; import type { Variables } from '../../types'; +import { getRequestAuth } from '../../middleware/tokenAuthMiddleware'; import { assertTaskRunTokenTargetExists, @@ -102,13 +97,10 @@ function getConfigRepositories(configValue: unknown): Array<{ } async function resolveRoomoteMcpAuth( - authContext: Variables['authContext'], + c: Context<{ Variables: Variables }>, options: { allowLegacyAudience: boolean }, ): Promise { - if (!options.allowLegacyAudience) { - const member = await resolveRoomoteMemberAuth(authContext); - return { userId: member.userId, tokenType: 'auth' }; - } + const authContext = getRequestAuth(c); if (!authContext) { throw new McpProxyError( 401, @@ -116,13 +108,7 @@ async function resolveRoomoteMcpAuth( ); } - if (isRunTokenContext(authContext)) { - if (!options.allowLegacyAudience) { - throw new McpProxyError( - 403, - 'Forbidden: member tools require a user-scoped access token', - ); - } + if (options.allowLegacyAudience && isRunTokenContext(authContext)) { await assertTaskRunTokenTargetExists(authContext); return { @@ -132,37 +118,8 @@ async function resolveRoomoteMcpAuth( }; } - if (isUserToken(authContext)) { - return { - userId: authContext.userId, - tokenType: 'auth', - }; - } - - if ( - authContext.tokenType === 'mcp' && - [ - getRoomoteMcpResourceUrl(Env.R_PUBLIC_URL ?? Env.R_APP_URL), - ...(options.allowLegacyAudience - ? [getLegacyRoomoteMcpResourceUrl(Env.TRPC_URL)] - : []), - ].includes(authContext.resource) && - authContext.scopes.includes(ROOMOTE_MCP_SCOPE) - ) { - await resolveRoomoteMemberAuth({ - ...authContext, - resource: getRoomoteMcpResourceUrl(Env.R_PUBLIC_URL ?? Env.R_APP_URL), - }); - return { - userId: authContext.userId, - tokenType: 'auth', - }; - } - - throw new McpProxyError( - 403, - `${PRODUCT_NAME} MCP requires a user-scoped auth token or task run token`, - ); + const member = await resolveRoomoteMemberAuth(c, options); + return { userId: member.userId, tokenType: 'auth' }; } async function buildAboutMePayload(options: { @@ -580,14 +537,14 @@ function createRoomoteMcpRouter(options: { const transport = createRoomoteTransport(); try { - const rawAuth = c.get('authContext'); + const rawAuth = getRequestAuth(c); if (!rawAuth) { throw new McpProxyError( 401, 'Unauthorized: missing or invalid bearer token', ); } - const auth = await resolveRoomoteMcpAuth(rawAuth, options); + const auth = await resolveRoomoteMcpAuth(c, options); // Null means the job runs as the deployment service principal; the // context tools are informational and support that case. Member tools // are only mounted on the public endpoint and retain the resolved user. diff --git a/apps/api/src/handlers/mcp/task-outputs.ts b/apps/api/src/handlers/mcp/task-outputs.ts index 5370c1448..f753e004d 100644 --- a/apps/api/src/handlers/mcp/task-outputs.ts +++ b/apps/api/src/handlers/mcp/task-outputs.ts @@ -21,6 +21,7 @@ import { asStringOrNull, taskOutputReadInputSchema, truncateAcpOutputText, + isUserToken, } from '@roomote/types'; import type { Variables } from '../../types'; @@ -38,7 +39,10 @@ import { resolveRoomoteMemberAuth } from './roomote-member-auth'; type MemberContext = Context<{ Variables: Variables & { mcpAuth: McpAuth } }>; async function requireReadableTask(taskId: string, auth: McpAuth) { - const member = await resolveRoomoteMemberAuth(auth.authContext); + // Tool invocations already carry the member identity resolved at ingress. + if (!auth.authContext || !isUserToken(auth.authContext)) + throw new McpProxyError(403, 'Member access required'); + const member = { ...auth, userId: auth.authContext.userId }; const task = await db.query.tasks.findFirst({ columns: { id: true }, where: and( @@ -234,7 +238,7 @@ export async function downloadMemberTaskArtifact( ): Promise { c.header('Cache-Control', 'private, no-store'); try { - const auth = await resolveRoomoteMemberAuth(c.get('authContext')); + const auth = await resolveRoomoteMemberAuth(c); const input = z .object({ taskId: z.string().regex(/^[0-9a-z]{13}$/), diff --git a/apps/api/src/middleware/__tests__/tokenAuthMiddleware.test.ts b/apps/api/src/middleware/__tests__/tokenAuthMiddleware.test.ts index 4471af16f..8158ad65c 100644 --- a/apps/api/src/middleware/__tests__/tokenAuthMiddleware.test.ts +++ b/apps/api/src/middleware/__tests__/tokenAuthMiddleware.test.ts @@ -188,6 +188,23 @@ describe('tokenAuthMiddleware token extraction', () => { }, ); + it('revalidates the same OAuth identity on every new request', async () => { + mockValidateMcpAccessToken.mockResolvedValue({ + tokenType: 'mcp', + userId: 'user-1', + clientId: 'client', + grantId: 'grant', + }); + const app = createApp(); + const headers = { authorization: 'Bearer oauth' }; + const first = await app.request('/mcp', { headers }); + expect((await first.json()).authContext).not.toBeNull(); + mockGrantLive.mockResolvedValue(false); + const second = await app.request('/mcp', { headers }); + expect((await second.json()).authContext).toBeNull(); + expect(mockGrantLive).toHaveBeenCalledTimes(2); + }); + it('accepts the run token from x-api-key on the inference gateway', async () => { const authContext = await requestAuthContext( '/api/inference/anthropic/v1/messages', diff --git a/apps/api/src/middleware/tokenAuthMiddleware.ts b/apps/api/src/middleware/tokenAuthMiddleware.ts index b5a3fc39d..9884a5891 100644 --- a/apps/api/src/middleware/tokenAuthMiddleware.ts +++ b/apps/api/src/middleware/tokenAuthMiddleware.ts @@ -16,6 +16,17 @@ import { import type { Variables } from '../types'; +// Only this middleware can publish a credential for this request. In particular, +// setting authContext alone must not attest that an OAuth grant was checked. +const authenticatedRequests = new WeakMap< + Context<{ Variables: Variables }>, + NonNullable +>(); + +export function getRequestAuth(c: Context<{ Variables: Variables }>) { + return authenticatedRequests.get(c); +} + async function deploymentAllowsTokenAuth(): Promise { const deployment = await db.query.deploymentSettings.findFirst({ where: eq(deploymentSettings.id, 'default'), @@ -106,6 +117,7 @@ export const tokenAuthMiddleware = () => // as that user is removed. if (!jobContext.userId || (user && user.deletedAt == null)) { c.set('authContext', jobContext); + authenticatedRequests.set(c, jobContext); isRunToken = true; } } @@ -150,6 +162,7 @@ export const tokenAuthMiddleware = () => // Removed users keep no standing access: their API tokens die with them. if (user && user.deletedAt == null) { c.set('authContext', userScopedAuth); + authenticatedRequests.set(c, userScopedAuth); } } catch (error) { console.error(