From 99da0ab1be19e9a52987ab9c268b799ea6356ac1 Mon Sep 17 00:00:00 2001 From: Brian Fox Date: Tue, 16 Jun 2026 16:16:42 +0200 Subject: [PATCH 01/19] feat(workflow-executor): use stored OAuth credentials for MCP steps At an oauth2 MCP step the executor looks up the stored credential by (user, server), runs the refresh-token grant against the stored token endpoint behind an expiry-skew cache, injects the bearer token before connecting, retries once on a 401 across list-tools and the tool call, and pauses for re-authentication when no usable credential exists or the refresh is rejected. Bearer and none steps are unchanged. Adds additive auth-error classification to the shared ai-proxy McpClient consumed by this path. Behaviour stays dormant until the orchestrator serves authType and the frontend ships (deploy orchestrator first), so it is safe to deploy alone. Depends on the PR1 credential store. Co-Authored-By: Claude Opus 4.8 (1M context) --- packages/ai-proxy/src/ai-client.ts | 23 +- packages/ai-proxy/src/index.ts | 1 + packages/ai-proxy/src/mcp-auth-error.ts | 58 ++++ packages/ai-proxy/src/mcp-client.ts | 46 ++- packages/ai-proxy/src/tool-provider.ts | 3 + packages/ai-proxy/test/ai-client.test.ts | 40 +++ packages/ai-proxy/test/mcp-auth-error.test.ts | 55 ++++ packages/ai-proxy/test/mcp-client.test.ts | 94 ++++++ .../src/adapters/ai-client-adapter.ts | 16 +- .../adapters/always-error-ai-model-port.ts | 14 +- .../src/adapters/server-ai-adapter.ts | 16 +- .../step-outcome-to-update-step-mapper.ts | 4 + .../src/build-workflow-executor.ts | 22 +- packages/workflow-executor/src/defaults.ts | 3 + packages/workflow-executor/src/errors.ts | 39 +++ .../src/executors/mcp-step-executor.ts | 68 +++- .../src/executors/step-executor-factory.ts | 31 +- .../src/oauth/keyed-mutex.ts | 24 ++ .../src/oauth/refresh-grant.ts | 108 +++++++ .../src/oauth/token-service.ts | 178 +++++++++++ .../src/ports/ai-model-port.ts | 12 +- .../src/remote-tool-fetcher.ts | 84 ++++- packages/workflow-executor/src/runner.ts | 7 +- .../src/types/validated/step-outcome.ts | 8 + .../test/adapters/ai-client-adapter.test.ts | 11 + .../always-error-ai-model-port.test.ts | 9 + .../test/adapters/server-ai-adapter.test.ts | 13 + ...step-outcome-to-update-step-mapper.test.ts | 46 +++ .../workflow-executor/test/errors.test.ts | 34 ++ .../test/executors/mcp-step-executor.test.ts | 112 ++++++- .../executors/step-executor-factory.test.ts | 102 ++++++ .../test/oauth/keyed-mutex.test.ts | 79 +++++ .../test/oauth/refresh-grant.test.ts | 198 ++++++++++++ .../test/oauth/token-service.test.ts | 300 ++++++++++++++++++ .../test/remote-tool-fetcher.test.ts | 187 ++++++++++- .../workflow-executor/test/runner.test.ts | 2 +- .../test/types/step-outcome.test.ts | 45 ++- 37 files changed, 2036 insertions(+), 56 deletions(-) create mode 100644 packages/ai-proxy/src/mcp-auth-error.ts create mode 100644 packages/ai-proxy/test/mcp-auth-error.test.ts create mode 100644 packages/workflow-executor/src/oauth/keyed-mutex.ts create mode 100644 packages/workflow-executor/src/oauth/refresh-grant.ts create mode 100644 packages/workflow-executor/src/oauth/token-service.ts create mode 100644 packages/workflow-executor/test/executors/step-executor-factory.test.ts create mode 100644 packages/workflow-executor/test/oauth/keyed-mutex.test.ts create mode 100644 packages/workflow-executor/test/oauth/refresh-grant.test.ts create mode 100644 packages/workflow-executor/test/oauth/token-service.test.ts diff --git a/packages/ai-proxy/src/ai-client.ts b/packages/ai-proxy/src/ai-client.ts index 87feb1f405..8ebbc5890e 100644 --- a/packages/ai-proxy/src/ai-client.ts +++ b/packages/ai-proxy/src/ai-client.ts @@ -1,3 +1,4 @@ +import type { McpServerLoadFailure } from './mcp-client'; import type { AiConfiguration } from './provider'; import type RemoteTool from './remote-tool'; import type { ToolProvider } from './tool-provider'; @@ -39,13 +40,31 @@ export class AiClient { } async loadRemoteTools(configs: Record): Promise { + return (await this.loadRemoteToolsWithFailures(configs)).tools; + } + + // Same load as loadRemoteTools, but also returns the classified per-server failures providers + // surface (only MCP providers do today). The default loadRemoteTools drops them, so existing + // consumers are unaffected. + async loadRemoteToolsWithFailures( + configs: Record, + ): Promise<{ tools: RemoteTool[]; failures: McpServerLoadFailure[] }> { await this.disposeToolProviders('Error closing previous remote tool connection'); const providers = createToolProviders(configs, this.logger); - const toolsByProvider = await Promise.all(providers.map(p => p.loadTools())); + const resultsByProvider = await Promise.all( + providers.map(async provider => + provider.loadToolsWithFailures + ? provider.loadToolsWithFailures() + : { tools: await provider.loadTools(), failures: [] }, + ), + ); this.toolProviders = providers; - return toolsByProvider.flat(); + return { + tools: resultsByProvider.flatMap(result => result.tools), + failures: resultsByProvider.flatMap(result => result.failures), + }; } async closeConnections(): Promise { diff --git a/packages/ai-proxy/src/index.ts b/packages/ai-proxy/src/index.ts index f0bf0a6e4b..197797caa1 100644 --- a/packages/ai-proxy/src/index.ts +++ b/packages/ai-proxy/src/index.ts @@ -20,6 +20,7 @@ export * from './remote-tools'; export { default as RemoteTool } from './remote-tool'; export * from './router'; export * from './mcp-client'; +export * from './mcp-auth-error'; export * from './oauth-token-injector'; export * from './errors'; export * from './tool-provider'; diff --git a/packages/ai-proxy/src/mcp-auth-error.ts b/packages/ai-proxy/src/mcp-auth-error.ts new file mode 100644 index 0000000000..ea39b476e3 --- /dev/null +++ b/packages/ai-proxy/src/mcp-auth-error.ts @@ -0,0 +1,58 @@ +// Classifies errors surfaced while connecting to or calling an MCP server. The MCP SDK / HTTP +// transport reports failures in several shapes (a numeric status field, or only a message string), +// so the checks walk the cause chain and inspect both structured status and the message text. +const AUTH_STATUSES = new Set([401, 403]); +const AUTH_PATTERN = /\b40[13]\b|unauthorized|forbidden/i; +const CONNECTION_PATTERN = + /econnrefused|econnreset|etimedout|enotfound|eai_again|fetch failed|network|socket|timeout|connect/i; + +export type McpLoadFailureKind = 'auth' | 'connection' | 'unknown'; + +function statusOf(value: unknown): number | undefined { + const candidate = value as { code?: unknown; status?: unknown; statusCode?: unknown }; + + for (const field of [candidate?.code, candidate?.status, candidate?.statusCode]) { + if (typeof field === 'number') return field; + } + + return undefined; +} + +function messageOf(value: unknown): string { + if (value instanceof Error) return value.message; + if (typeof value === 'string') return value; + + return ''; +} + +function errorChain(error: unknown): unknown[] { + const links: unknown[] = []; + let current: unknown = error; + + while (current && links.length < 10 && !links.includes(current)) { + links.push(current); + current = (current as { cause?: unknown }).cause; + } + + return links; +} + +export function isMcpAuthError(error: unknown): boolean { + return errorChain(error).some(link => { + const status = statusOf(link); + + return ( + (status !== undefined && AUTH_STATUSES.has(status)) || AUTH_PATTERN.test(messageOf(link)) + ); + }); +} + +export function classifyMcpLoadError(error: unknown): McpLoadFailureKind { + if (isMcpAuthError(error)) return 'auth'; + + const isConnectionFailure = errorChain(error).some(link => + CONNECTION_PATTERN.test(messageOf(link)), + ); + + return isConnectionFailure ? 'connection' : 'unknown'; +} diff --git a/packages/ai-proxy/src/mcp-client.ts b/packages/ai-proxy/src/mcp-client.ts index bb834c576e..0008a1bf8b 100644 --- a/packages/ai-proxy/src/mcp-client.ts +++ b/packages/ai-proxy/src/mcp-client.ts @@ -4,18 +4,28 @@ import type { Logger } from '@forestadmin/datasource-toolkit'; import { MultiServerMCPClient } from '@langchain/mcp-adapters'; import { McpConnectionError } from './errors'; +import { type McpLoadFailureKind, classifyMcpLoadError } from './mcp-auth-error'; import McpServerRemoteTool from './mcp-server-remote-tool'; export type McpServers = MultiServerMCPClient['config']['mcpServers']; export type McpServerConfig = MultiServerMCPClient['config']['mcpServers'][string] & { id?: string; + // Executor-side routing hint served by the orchestrator; stripped before reaching the SDK. + authType?: string; }; export type McpConfiguration = { configs: Record; } & Omit; +export interface McpServerLoadFailure { + server: string; + mcpServerId?: string; + kind: McpLoadFailureKind; + error: Error; +} + export default class McpClient implements ToolProvider { private readonly mcpClients: Record = {}; private readonly mcpServerIdsByName: Record = {}; @@ -26,8 +36,11 @@ export default class McpClient implements ToolProvider { // split the config into several clients to be more resilient // if a mcp server is down, the others will still work Object.entries(config.configs).forEach(([name, serverConfig]) => { - const { id: mcpServerId, ...rest } = serverConfig as McpServerConfig & - Record; + const { + id: mcpServerId, + authType, + ...rest + } = serverConfig as McpServerConfig & Record; this.mcpServerIdsByName[name] = mcpServerId; this.mcpClients[name] = new MultiServerMCPClient({ mcpServers: { [name]: rest as McpServerConfig }, @@ -36,9 +49,15 @@ export default class McpClient implements ToolProvider { }); } - async loadTools(): Promise { + // Exposes per-server failures classified by cause (auth vs connection) alongside the tools that + // did load, so a caller holding a per-user token can tell a revoked token (retry after refresh) + // from an unreachable server (genuine failure). loadTools() keeps its tools-only contract. + async loadToolsWithFailures(): Promise<{ + tools: McpServerRemoteTool[]; + failures: McpServerLoadFailure[]; + }> { const tools: McpServerRemoteTool[] = []; - const errors: Array<{ server: string; error: Error }> = []; + const failures: McpServerLoadFailure[] = []; await Promise.all( Object.entries(this.mcpClients).map(async ([name, client]) => { @@ -55,22 +74,31 @@ export default class McpClient implements ToolProvider { tools.push(...extendedTools); } catch (error) { this.logger?.('Error', `Error loading tools for ${name}`, error as Error); - errors.push({ server: name, error: error as Error }); + failures.push({ + server: name, + mcpServerId: this.mcpServerIdsByName[name], + kind: classifyMcpLoadError(error), + error: error as Error, + }); } }), ); // Surface partial failures to provide better feedback - if (errors.length > 0) { - const errorMessage = errors.map(e => `${e.server}: ${e.error.message}`).join('; '); + if (failures.length > 0) { + const errorMessage = failures.map(f => `${f.server}: ${f.error.message}`).join('; '); this.logger?.( 'Error', - `Failed to load tools from ${errors.length}/${Object.keys(this.mcpClients).length} ` + + `Failed to load tools from ${failures.length}/${Object.keys(this.mcpClients).length} ` + `MCP server(s): ${errorMessage}`, ); } - return tools; + return { tools, failures }; + } + + async loadTools(): Promise { + return (await this.loadToolsWithFailures()).tools; } async checkConnection(): Promise { diff --git a/packages/ai-proxy/src/tool-provider.ts b/packages/ai-proxy/src/tool-provider.ts index ed2a2408fe..a7d0eff4df 100644 --- a/packages/ai-proxy/src/tool-provider.ts +++ b/packages/ai-proxy/src/tool-provider.ts @@ -1,7 +1,10 @@ +import type { McpServerLoadFailure } from './mcp-client'; import type RemoteTool from './remote-tool'; export interface ToolProvider { loadTools(): Promise; + // Optional richer variant: providers that can classify per-server failures expose them here. + loadToolsWithFailures?(): Promise<{ tools: RemoteTool[]; failures: McpServerLoadFailure[] }>; checkConnection(): Promise; dispose(): Promise; } diff --git a/packages/ai-proxy/test/ai-client.test.ts b/packages/ai-proxy/test/ai-client.test.ts index d3919dfc3b..a56572fb2e 100644 --- a/packages/ai-proxy/test/ai-client.test.ts +++ b/packages/ai-proxy/test/ai-client.test.ts @@ -242,6 +242,46 @@ describe('loadRemoteTools', () => { }); }); +describe('loadRemoteToolsWithFailures', () => { + beforeEach(() => { + jest.clearAllMocks(); + }); + + it('aggregates tools and classified failures from providers that expose them', async () => { + const mcpTool = { name: 'mcp-tool' }; + const failure = { + server: 'slack', + mcpServerId: 'srv-a', + kind: 'auth' as const, + error: new Error('401'), + }; + mockedCreateToolProviders.mockReturnValue([ + mockProvider({ + loadToolsWithFailures: jest + .fn() + .mockResolvedValue({ tools: [mcpTool], failures: [failure] }), + }), + ]); + + const result = await new AiClient({}).loadRemoteToolsWithFailures({} as never); + + expect(result.tools).toEqual([mcpTool]); + expect(result.failures).toEqual([failure]); + }); + + it('falls back to loadTools with no failures for providers that do not classify', async () => { + const integrationTool = { name: 'zendesk-tool' }; + mockedCreateToolProviders.mockReturnValue([ + mockProvider({ loadTools: jest.fn().mockResolvedValue([integrationTool]) }), + ]); + + const result = await new AiClient({}).loadRemoteToolsWithFailures({} as never); + + expect(result.tools).toEqual([integrationTool]); + expect(result.failures).toEqual([]); + }); +}); + describe('closeConnections', () => { beforeEach(() => { jest.clearAllMocks(); diff --git a/packages/ai-proxy/test/mcp-auth-error.test.ts b/packages/ai-proxy/test/mcp-auth-error.test.ts new file mode 100644 index 0000000000..c55ae57dd4 --- /dev/null +++ b/packages/ai-proxy/test/mcp-auth-error.test.ts @@ -0,0 +1,55 @@ +import { classifyMcpLoadError, isMcpAuthError } from '../src/mcp-auth-error'; + +function withCause(message: string, cause: unknown): Error { + const error = new Error(message); + (error as { cause?: unknown }).cause = cause; + + return error; +} + +describe('isMcpAuthError', () => { + it('detects a 401 numeric status field', () => { + expect(isMcpAuthError({ code: 401 })).toBe(true); + }); + + it('detects a 403 numeric status field', () => { + expect(isMcpAuthError(Object.assign(new Error('denied'), { status: 403 }))).toBe(true); + }); + + it('detects 401 / unauthorized / forbidden in the message', () => { + expect(isMcpAuthError(new Error('Request failed with status code 401'))).toBe(true); + expect(isMcpAuthError(new Error('Unauthorized'))).toBe(true); + expect(isMcpAuthError(new Error('403 Forbidden'))).toBe(true); + }); + + it('walks the cause chain', () => { + expect( + isMcpAuthError(withCause('wrapper', Object.assign(new Error('inner'), { status: 401 }))), + ).toBe(true); + }); + + it('returns false for non-auth errors and nullish input', () => { + expect(isMcpAuthError(new Error('ECONNREFUSED'))).toBe(false); + expect(isMcpAuthError(new Error('500 Internal Server Error'))).toBe(false); + expect(isMcpAuthError(undefined)).toBe(false); + }); +}); + +describe('classifyMcpLoadError', () => { + it("classifies auth failures as 'auth'", () => { + expect(classifyMcpLoadError(new Error('HTTP 403 Forbidden'))).toBe('auth'); + expect(classifyMcpLoadError({ status: 401 })).toBe('auth'); + }); + + it("classifies network failures as 'connection'", () => { + expect(classifyMcpLoadError(new Error('connect ECONNREFUSED 127.0.0.1:3000'))).toBe( + 'connection', + ); + expect(classifyMcpLoadError(new Error('fetch failed'))).toBe('connection'); + expect(classifyMcpLoadError(new Error('socket hang up'))).toBe('connection'); + }); + + it("classifies everything else as 'unknown'", () => { + expect(classifyMcpLoadError(new Error('tool schema invalid'))).toBe('unknown'); + }); +}); diff --git a/packages/ai-proxy/test/mcp-client.test.ts b/packages/ai-proxy/test/mcp-client.test.ts index ad128c2fbd..22cd950a57 100644 --- a/packages/ai-proxy/test/mcp-client.test.ts +++ b/packages/ai-proxy/test/mcp-client.test.ts @@ -1,6 +1,7 @@ import type { McpConfiguration } from '../src'; import { tool } from '@langchain/core/tools'; +import { MultiServerMCPClient } from '@langchain/mcp-adapters'; import { McpConnectionError } from '../src'; import McpClient from '../src/mcp-client'; @@ -487,3 +488,96 @@ describe('McpClient', () => { }); }); }); + +// Additive auth-error classification: loadToolsWithFailures exposes per-server failures alongside +// the loaded tools so a token holder can tell a revoked token from an unreachable server. +describe('McpClient.loadToolsWithFailures', () => { + beforeEach(() => { + jest.clearAllMocks(); + }); + + const makeTool = (name: string) => + tool(() => {}, { name, description: name, schema: undefined, responseFormat: 'content' }); + + const singleServer = (id?: string) => + ({ + configs: { + slack: { transport: 'stdio', command: 'npx', args: [], env: {}, ...(id ? { id } : {}) }, + }, + } as unknown as McpConfiguration); + + it('returns the loaded tools and no failures when every server loads', async () => { + getToolsMock.mockResolvedValue([makeTool('t1')]); + + const result = await new McpClient(singleServer()).loadToolsWithFailures(); + + expect(result.tools).toHaveLength(1); + expect(result.failures).toEqual([]); + }); + + it("classifies a 401 as an 'auth' failure tagged with server and mcpServerId, keeping other tools", async () => { + getToolsMock + .mockRejectedValueOnce(new Error('Request failed: 401 Unauthorized')) + .mockResolvedValueOnce([makeTool('t2')]); + const client = new McpClient({ + configs: { + slack: { transport: 'stdio', command: 'npx', args: [], env: {}, id: 'srv-a' }, + github: { transport: 'stdio', command: 'npx', args: [], env: {} }, + }, + } as unknown as McpConfiguration); + + const result = await client.loadToolsWithFailures(); + + expect(result.tools).toHaveLength(1); + expect(result.failures).toEqual([ + expect.objectContaining({ server: 'slack', mcpServerId: 'srv-a', kind: 'auth' }), + ]); + }); + + it("classifies a connection error as 'connection', not 'auth'", async () => { + getToolsMock.mockRejectedValue(new Error('connect ECONNREFUSED 127.0.0.1:3000')); + + const result = await new McpClient(singleServer()).loadToolsWithFailures(); + + expect(result.failures[0].kind).toBe('connection'); + }); + + it('logs the aggregated failure summary and does not throw on a per-server failure', async () => { + const logger = jest.fn(); + getToolsMock.mockRejectedValue(new Error('401')); + + const result = await new McpClient(singleServer(), logger).loadToolsWithFailures(); + + expect(result.tools).toEqual([]); + expect(logger).toHaveBeenCalledWith( + 'Error', + expect.stringContaining('Failed to load tools from 1/1'), + ); + }); + + it('loadTools delegates and returns only the tools', async () => { + getToolsMock.mockResolvedValue([makeTool('t3')]); + + const tools = await new McpClient(singleServer()).loadTools(); + + expect(tools).toHaveLength(1); + }); +}); + +// authType is an executor-side routing hint; like id it must be stripped in the constructor so it +// never reaches MultiServerMCPClient. +describe('McpClient constructor — authType stripping', () => { + it('strips authType and id, passing only the transport config to MultiServerMCPClient', () => { + (MultiServerMCPClient as jest.Mock).mockClear(); + + // eslint-disable-next-line no-new + new McpClient({ + configs: { + slack: { type: 'http', url: 'https://example.com/mcp', id: 'srv-a', authType: 'oauth2' }, + }, + } as unknown as McpConfiguration); + + const passedConfig = (MultiServerMCPClient as jest.Mock).mock.calls[0][0].mcpServers.slack; + expect(passedConfig).toEqual({ type: 'http', url: 'https://example.com/mcp' }); + }); +}); diff --git a/packages/workflow-executor/src/adapters/ai-client-adapter.ts b/packages/workflow-executor/src/adapters/ai-client-adapter.ts index fc5a98df13..43369caf16 100644 --- a/packages/workflow-executor/src/adapters/ai-client-adapter.ts +++ b/packages/workflow-executor/src/adapters/ai-client-adapter.ts @@ -1,5 +1,11 @@ import type { AiModelPort, GetModelOptions } from '../ports/ai-model-port'; -import type { AiConfiguration, BaseChatModel, RemoteTool, ToolConfig } from '@forestadmin/ai-proxy'; +import type { + AiConfiguration, + BaseChatModel, + McpServerLoadFailure, + RemoteTool, + ToolConfig, +} from '@forestadmin/ai-proxy'; import { AiClient } from '@forestadmin/ai-proxy'; @@ -26,6 +32,14 @@ export default class AiClientAdapter implements AiModelPort { return this.callPort('loadRemoteTools', () => this.aiClient.loadRemoteTools(configs)); } + loadRemoteToolsWithFailures( + configs: Record, + ): Promise<{ tools: RemoteTool[]; failures: McpServerLoadFailure[] }> { + return this.callPort('loadRemoteToolsWithFailures', () => + this.aiClient.loadRemoteToolsWithFailures(configs), + ); + } + closeConnections(): Promise { return this.callPort('closeConnections', () => this.aiClient.closeConnections()); } diff --git a/packages/workflow-executor/src/adapters/always-error-ai-model-port.ts b/packages/workflow-executor/src/adapters/always-error-ai-model-port.ts index 24a7426c5c..a51e5ef061 100644 --- a/packages/workflow-executor/src/adapters/always-error-ai-model-port.ts +++ b/packages/workflow-executor/src/adapters/always-error-ai-model-port.ts @@ -1,5 +1,10 @@ import type { AiModelPort } from '../ports/ai-model-port'; -import type { BaseChatModel, RemoteTool, ToolConfig } from '@forestadmin/ai-proxy'; +import type { + BaseChatModel, + McpServerLoadFailure, + RemoteTool, + ToolConfig, +} from '@forestadmin/ai-proxy'; import { AiModelPortError } from '../errors'; @@ -22,6 +27,13 @@ export default class AlwaysErrorAiModelPort implements AiModelPort { return Promise.resolve([]); } + loadRemoteToolsWithFailures( + // eslint-disable-next-line @typescript-eslint/no-unused-vars + _configs: Record, + ): Promise<{ tools: RemoteTool[]; failures: McpServerLoadFailure[] }> { + return Promise.resolve({ tools: [], failures: [] }); + } + closeConnections(): Promise { return Promise.resolve(); } diff --git a/packages/workflow-executor/src/adapters/server-ai-adapter.ts b/packages/workflow-executor/src/adapters/server-ai-adapter.ts index fae591047b..2d05307d30 100644 --- a/packages/workflow-executor/src/adapters/server-ai-adapter.ts +++ b/packages/workflow-executor/src/adapters/server-ai-adapter.ts @@ -1,5 +1,11 @@ import type { AiModelPort, GetModelOptions } from '../ports/ai-model-port'; -import type { AiConfiguration, BaseChatModel, RemoteTool, ToolConfig } from '@forestadmin/ai-proxy'; +import type { + AiConfiguration, + BaseChatModel, + McpServerLoadFailure, + RemoteTool, + ToolConfig, +} from '@forestadmin/ai-proxy'; import { AiClient } from '@forestadmin/ai-proxy'; @@ -38,6 +44,14 @@ export default class ServerAiAdapter implements AiModelPort { return this.callPort('loadRemoteTools', () => this.aiClient.loadRemoteTools(configs)); } + loadRemoteToolsWithFailures( + configs: Record, + ): Promise<{ tools: RemoteTool[]; failures: McpServerLoadFailure[] }> { + return this.callPort('loadRemoteToolsWithFailures', () => + this.aiClient.loadRemoteToolsWithFailures(configs), + ); + } + closeConnections(): Promise { return this.callPort('closeConnections', () => this.aiClient.closeConnections()); } diff --git a/packages/workflow-executor/src/adapters/step-outcome-to-update-step-mapper.ts b/packages/workflow-executor/src/adapters/step-outcome-to-update-step-mapper.ts index bc050f6324..b3ce575512 100644 --- a/packages/workflow-executor/src/adapters/step-outcome-to-update-step-mapper.ts +++ b/packages/workflow-executor/src/adapters/step-outcome-to-update-step-mapper.ts @@ -32,6 +32,10 @@ export default function toUpdateStepRequest( context.selectedOption = outcome.selectedOption; } + if (outcome.type === 'mcp' && outcome.awaitingInputReason !== undefined) { + context.awaitingInputReason = outcome.awaitingInputReason; + } + const attributes: ServerStepHistoryUpdate = { done: outcome.status !== 'awaiting-input', context, diff --git a/packages/workflow-executor/src/build-workflow-executor.ts b/packages/workflow-executor/src/build-workflow-executor.ts index 1b7d77855f..adfe7e4121 100644 --- a/packages/workflow-executor/src/build-workflow-executor.ts +++ b/packages/workflow-executor/src/build-workflow-executor.ts @@ -25,6 +25,7 @@ import { DEFAULT_STEP_TIMEOUT_S, } from './defaults'; import ExecutorHttpServer from './http/executor-http-server'; +import OAuthTokenService from './oauth/token-service'; import Runner from './runner'; import SchemaCache from './schema-cache'; import DatabaseMcpOAuthCredentialsStore from './stores/database-mcp-oauth-credentials-store'; @@ -252,9 +253,23 @@ export function buildDatabaseExecutor(options: DatabaseExecutorOptions): Workflo if (mergedOptions.logging === undefined) mergedOptions.logging = false; const sequelize = uri ? new Sequelize(uri, mergedOptions) : new Sequelize(mergedOptions); + const mcpOAuthCredentialsStore = new DatabaseMcpOAuthCredentialsStore({ + sequelize, + schema: mergedOptions.schema, + }); + const credentialEncryption = new CredentialEncryption(); + // Shares the store + encryption with the deposit endpoint so runtime reads and writes (rotation) + // go through the same instance the HTTP server migrates on start. + const mcpOAuthTokenService = new OAuthTokenService({ + store: mcpOAuthCredentialsStore, + encryption: credentialEncryption, + logger: deps.logger, + }); + const runner = new Runner({ ...deps, runStore: new DatabaseStore({ sequelize, schema: mergedOptions.schema }), + mcpOAuthTokenService, }); const server = new ExecutorHttpServer({ @@ -263,11 +278,8 @@ export function buildDatabaseExecutor(options: DatabaseExecutorOptions): Workflo authSecret: options.authSecret, workflowPort: deps.workflowPort, logger: deps.logger, - mcpOAuthCredentialsStore: new DatabaseMcpOAuthCredentialsStore({ - sequelize, - schema: mergedOptions.schema, - }), - credentialEncryption: new CredentialEncryption(), + mcpOAuthCredentialsStore, + credentialEncryption, }); return createWorkflowExecutor(runner, server, deps.logger); diff --git a/packages/workflow-executor/src/defaults.ts b/packages/workflow-executor/src/defaults.ts index 9a1221cfb2..3ad7cd7669 100644 --- a/packages/workflow-executor/src/defaults.ts +++ b/packages/workflow-executor/src/defaults.ts @@ -9,3 +9,6 @@ export const DEFAULT_STOP_TIMEOUT_S = 30; export const DEFAULT_MAX_CHAIN_DEPTH = 50; export const DEFAULT_SCHEMA_CACHE_TTL_S = 10 * 60; export const DEFAULT_LOGGER_LEVEL: LoggerLevel = 'Info'; +// Refresh an OAuth access token this many seconds before it actually expires, so a token never +// goes stale mid-request between the skew check and the downstream MCP call. +export const DEFAULT_OAUTH_EXPIRY_SKEW_S = 60; diff --git a/packages/workflow-executor/src/errors.ts b/packages/workflow-executor/src/errors.ts index 4cabf62afd..7e65ea5af2 100644 --- a/packages/workflow-executor/src/errors.ts +++ b/packages/workflow-executor/src/errors.ts @@ -1,6 +1,7 @@ /* eslint-disable max-classes-per-file */ import type { MalformedRunInfo } from './ports/workflow-port'; import type { RecordId } from './types/validated/collection'; +import type { AwaitingInputReason } from './types/validated/step-outcome'; import type { z } from 'zod'; export function causeMessage(error: unknown): string | undefined { @@ -350,6 +351,44 @@ export class ConfigurationError extends Error { } } +// Carries the typed pause reason so the step executor can emit an awaiting-input outcome (and the +// orchestrator/front can prompt the user to reconnect) instead of a generic error. +export class OAuthReauthRequiredError extends WorkflowExecutorError { + readonly awaitingInputReason: AwaitingInputReason; + + constructor( + mcpServerId: string, + awaitingInputReason: AwaitingInputReason = 'needs-oauth-reauth', + ) { + super( + `OAuth re-authentication required for mcpServerId="${mcpServerId}"`, + 'This tool needs to be reconnected before it can run. Please re-authorize it and try again.', + ); + this.awaitingInputReason = awaitingInputReason; + } +} + +// Transient refresh failure (network error, 5xx, malformed response). Surfaces as a retryable step +// error rather than a re-auth pause — re-authenticating would not fix a temporarily unreachable endpoint. +export class OAuthRefreshError extends WorkflowExecutorError { + constructor(message: string, cause?: unknown) { + super( + `OAuth token refresh failed: ${message}`, + 'Could not refresh the connection to this tool. Please try again.', + ); + if (cause !== undefined) this.cause = cause; + } +} + +// The token endpoint rejected the refresh token (RFC 6749 invalid_grant). Internal control-flow +// signal: the token service catches it to drive the re-read + single-retry, then converts a genuine +// failure into OAuthReauthRequiredError. +export class OAuthInvalidGrantError extends WorkflowExecutorError { + constructor(detail?: string) { + super(`OAuth refresh token rejected${detail ? `: ${detail}` : ''}`); + } +} + // Boundary error — the deposit endpoint maps it to a typed HTTP response so the frontend can tell // an operator to provision the key, not a generic or re-consent failure. export class ExecutorEncryptionKeyMissingError extends Error { diff --git a/packages/workflow-executor/src/executors/mcp-step-executor.ts b/packages/workflow-executor/src/executors/mcp-step-executor.ts index 2447c8a837..ce45d008d6 100644 --- a/packages/workflow-executor/src/executors/mcp-step-executor.ts +++ b/packages/workflow-executor/src/executors/mcp-step-executor.ts @@ -1,16 +1,22 @@ import type { ExecutionContext, StepExecutionResult } from '../types/execution-context'; import type { McpStepExecutionData, McpToolCall } from '../types/step-execution-data'; import type { McpStepDefinition } from '../types/validated/step-definition'; -import type { RecordStepStatus } from '../types/validated/step-outcome'; +import type { AwaitingInputReason, RecordStepStatus } from '../types/validated/step-outcome'; import type { RemoteTool } from '@forestadmin/ai-proxy'; -import { DynamicStructuredTool, HumanMessage, SystemMessage } from '@forestadmin/ai-proxy'; +import { + DynamicStructuredTool, + HumanMessage, + SystemMessage, + isMcpAuthError, +} from '@forestadmin/ai-proxy'; import { z } from 'zod'; import { McpToolInvocationError, McpToolNotFoundError, NoMcpToolsError, + OAuthReauthRequiredError, StepStateError, } from '../errors'; import BaseStepExecutor from './base-step-executor'; @@ -28,14 +34,18 @@ export default class McpStepExecutor extends BaseStepExecutor private readonly mcpServerName?: string; + private readonly reloadWithFreshAuth?: () => Promise; + constructor( context: ExecutionContext, remoteTools: readonly RemoteTool[], mcpServerName?: string, + reloadWithFreshAuth?: () => Promise, ) { super(context); this.remoteTools = remoteTools; this.mcpServerName = mcpServerName; + this.reloadWithFreshAuth = reloadWithFreshAuth; } protected override getExtraLogContext(): Record { @@ -48,6 +58,7 @@ export default class McpStepExecutor extends BaseStepExecutor protected buildOutcomeResult(outcome: { status: RecordStepStatus; error?: string; + awaitingInputReason?: AwaitingInputReason; }): StepExecutionResult { return { stepOutcome: { @@ -74,6 +85,22 @@ export default class McpStepExecutor extends BaseStepExecutor } protected async doExecute(): Promise { + try { + return await this.runStep(); + } catch (error) { + // An unrefreshable OAuth credential pauses the step for re-authentication rather than failing it. + if (error instanceof OAuthReauthRequiredError) { + return this.buildOutcomeResult({ + status: 'awaiting-input', + awaitingInputReason: error.awaitingInputReason, + }); + } + + throw error; + } + } + + private async runStep(): Promise { // Branch A -- Re-entry after pending execution found in RunStore const pending = await this.patchAndReloadPendingData( this.context.incomingPendingData, @@ -124,13 +151,7 @@ export default class McpStepExecutor extends BaseStepExecutor recordId: this.context.baseRecordRef.recordId, }, { - operation: async () => { - try { - return await tool.base.invoke(target.input); - } catch (cause) { - throw new McpToolInvocationError(target.name, cause); - } - }, + operation: () => this.invokeWithReauthRetry(tool, target), beforeCall: () => this.context.runStore.saveStepExecution(this.context.runId, { ...existingExecution, @@ -195,6 +216,35 @@ export default class McpStepExecutor extends BaseStepExecutor return this.buildOutcomeResult({ status: 'success' }); } + // No-op for bearer/none steps (no reloadWithFreshAuth). For an OAuth2 step, a 401 on the call means + // the token was rejected after listing tools succeeded: force one refresh, rebuild the tool, retry + // once. A second 401 pauses the step for re-authentication. + private async invokeWithReauthRetry(tool: RemoteTool, target: McpToolCall): Promise { + try { + return await tool.base.invoke(target.input); + } catch (cause) { + if (!this.reloadWithFreshAuth || !isMcpAuthError(cause)) { + throw new McpToolInvocationError(target.name, cause); + } + + const refreshedTools = await this.reloadWithFreshAuth(); + const refreshedTool = refreshedTools.find( + t => t.base.name === target.name && t.sourceId === target.sourceId, + ); + if (!refreshedTool) throw new McpToolNotFoundError(target.name); + + try { + return await refreshedTool.base.invoke(target.input); + } catch (retryCause) { + if (isMcpAuthError(retryCause)) { + throw new OAuthReauthRequiredError(this.context.stepDefinition.mcpServerId); + } + + throw new McpToolInvocationError(target.name, retryCause); + } + } + } + private async formatToolResult(tool: McpToolCall, toolResult: unknown): Promise { if (toolResult === null || toolResult === undefined) return null; diff --git a/packages/workflow-executor/src/executors/step-executor-factory.ts b/packages/workflow-executor/src/executors/step-executor-factory.ts index 0c42454778..15b4bee36b 100644 --- a/packages/workflow-executor/src/executors/step-executor-factory.ts +++ b/packages/workflow-executor/src/executors/step-executor-factory.ts @@ -23,6 +23,7 @@ import type { } from '../types/validated/step-definition'; import { + OAuthReauthRequiredError, StepStateError, WorkflowExecutorError, causeMessage, @@ -57,7 +58,7 @@ export default class StepExecutorFactory { step: AvailableStepExecution, contextConfig: StepContextConfig, activityLogPort: ActivityLogPort, - fetchRemoteTools: (mcpServerId: string) => Promise, + fetchRemoteTools: (mcpServerId: string, userId: number) => Promise, incomingPendingData?: unknown, ): Promise { try { @@ -88,11 +89,12 @@ export default class StepExecutorFactory { case StepType.Mcp: { const mcpContext = context as ExecutionContext; - const { tools, mcpServerName } = await fetchRemoteTools( + const { tools, mcpServerName, reloadWithFreshAuth } = await fetchRemoteTools( mcpContext.stepDefinition.mcpServerId, + step.user.id, ); - return new McpStepExecutor(mcpContext, tools, mcpServerName); + return new McpStepExecutor(mcpContext, tools, mcpServerName, reloadWithFreshAuth); } case StepType.Guidance: @@ -103,6 +105,29 @@ export default class StepExecutorFactory { ); } } catch (error) { + // Re-auth required while loading tools is an expected pause, not a failure: emit awaiting-input + // with the typed reason so the user can reconnect, rather than erroring the step. + if (error instanceof OAuthReauthRequiredError) { + contextConfig.logger('Info', 'MCP step paused for OAuth re-authentication', { + runId: step.runId, + stepId: step.stepId, + stepIndex: step.stepIndex, + awaitingInputReason: error.awaitingInputReason, + }); + + return { + execute: async (): Promise => ({ + stepOutcome: { + type: 'mcp', + stepId: step.stepId, + stepIndex: step.stepIndex, + status: 'awaiting-input', + awaitingInputReason: error.awaitingInputReason, + }, + }), + }; + } + contextConfig.logger('Error', 'Step execution failed unexpectedly', { runId: step.runId, stepId: step.stepId, diff --git a/packages/workflow-executor/src/oauth/keyed-mutex.ts b/packages/workflow-executor/src/oauth/keyed-mutex.ts new file mode 100644 index 0000000000..76dc6769c0 --- /dev/null +++ b/packages/workflow-executor/src/oauth/keyed-mutex.ts @@ -0,0 +1,24 @@ +// Serializes async work per key: callers sharing a key run one at a time (FIFO), while different +// keys proceed in parallel. Used to collapse concurrent token refreshes for the same (user, server) +// into a single in-flight refresh within one process. +export default class KeyedMutex { + private readonly tails = new Map>(); + + async runExclusive(key: string, task: () => Promise): Promise { + const previous = this.tails.get(key) ?? Promise.resolve(); + const result = previous.then(() => task()); + // Store a non-rejecting tail so the next caller chains regardless of this task's outcome. + const tail = result.then( + () => undefined, + () => undefined, + ); + this.tails.set(key, tail); + + try { + return await result; + } finally { + // Drop the entry once this was the last queued task for the key, so the map can't grow. + if (this.tails.get(key) === tail) this.tails.delete(key); + } + } +} diff --git a/packages/workflow-executor/src/oauth/refresh-grant.ts b/packages/workflow-executor/src/oauth/refresh-grant.ts new file mode 100644 index 0000000000..6d1147a959 --- /dev/null +++ b/packages/workflow-executor/src/oauth/refresh-grant.ts @@ -0,0 +1,108 @@ +import { OAuthInvalidGrantError, OAuthRefreshError } from '../errors'; + +export interface RefreshGrantParams { + tokenEndpoint: string; + refreshToken: string; + clientId?: string | null; + clientSecret?: string | null; + tokenEndpointAuthMethod?: string | null; + scopes?: string | null; +} + +export interface RefreshGrantResult { + accessToken: string; + expiresInS?: number; + // Present only when the authorization server rotates the refresh token. + refreshToken?: string; +} + +interface TokenEndpointResponse { + access_token?: unknown; + expires_in?: unknown; + refresh_token?: unknown; + error?: unknown; + error_description?: unknown; +} + +function buildRequest(params: RefreshGrantParams): { + headers: Record; + body: URLSearchParams; +} { + const headers: Record = { + 'content-type': 'application/x-www-form-urlencoded', + accept: 'application/json', + }; + const body = new URLSearchParams({ + grant_type: 'refresh_token', + refresh_token: params.refreshToken, + }); + + if (params.scopes) body.set('scope', params.scopes); + + const { clientId, clientSecret, tokenEndpointAuthMethod } = params; + + if (clientSecret) { + if (tokenEndpointAuthMethod === 'client_secret_post') { + if (clientId) body.set('client_id', clientId); + body.set('client_secret', clientSecret); + } else { + const credentials = `${encodeURIComponent(clientId ?? '')}:${encodeURIComponent( + clientSecret, + )}`; + headers.authorization = `Basic ${Buffer.from(credentials).toString('base64')}`; + } + } else if (clientId) { + body.set('client_id', clientId); + } + + return { headers, body }; +} + +// Runs the OAuth2 refresh-token grant (RFC 6749 §6) against the stored token endpoint. Distinguishes +// a non-retryable invalid_grant (revoked / rotated) from a transient failure so the caller can decide +// between forcing re-auth and retrying. +export default async function refreshAccessToken( + params: RefreshGrantParams, +): Promise { + const { headers, body } = buildRequest(params); + + let response: Awaited>; + + try { + response = await fetch(params.tokenEndpoint, { method: 'POST', headers, body }); + } catch (cause) { + throw new OAuthRefreshError('the token endpoint could not be reached', cause); + } + + let payload: TokenEndpointResponse = {}; + + try { + payload = (await response.json()) as TokenEndpointResponse; + } catch { + // Non-JSON body — handled by the status checks below. + } + + if (!response.ok) { + if (payload.error === 'invalid_grant') { + throw new OAuthInvalidGrantError( + typeof payload.error_description === 'string' ? payload.error_description : undefined, + ); + } + + throw new OAuthRefreshError( + typeof payload.error === 'string' + ? payload.error + : `the token endpoint returned ${response.status}`, + ); + } + + if (typeof payload.access_token !== 'string' || !payload.access_token) { + throw new OAuthRefreshError('the token endpoint response had no access_token'); + } + + return { + accessToken: payload.access_token, + expiresInS: typeof payload.expires_in === 'number' ? payload.expires_in : undefined, + refreshToken: typeof payload.refresh_token === 'string' ? payload.refresh_token : undefined, + }; +} diff --git a/packages/workflow-executor/src/oauth/token-service.ts b/packages/workflow-executor/src/oauth/token-service.ts new file mode 100644 index 0000000000..ac1b6de5fb --- /dev/null +++ b/packages/workflow-executor/src/oauth/token-service.ts @@ -0,0 +1,178 @@ +import type { RefreshGrantParams, RefreshGrantResult } from './refresh-grant'; +import type CredentialEncryption from '../crypto/credential-encryption'; +import type { Logger } from '../ports/logger-port'; +import type { StoredMcpOAuthCredential } from '../stores/mcp-oauth-credentials-store'; +import type McpOAuthCredentialsStore from '../stores/mcp-oauth-credentials-store'; + +import { DEFAULT_OAUTH_EXPIRY_SKEW_S } from '../defaults'; +import { OAuthInvalidGrantError, OAuthReauthRequiredError } from '../errors'; +import KeyedMutex from './keyed-mutex'; +import defaultRefreshAccessToken from './refresh-grant'; + +interface CachedToken { + accessToken: string; + // Absent when the grant omitted expires_in: the token is used once but never served from cache. + expiresAtMs?: number; +} + +export interface OAuthTokenServiceOptions { + store: McpOAuthCredentialsStore; + encryption: CredentialEncryption; + logger?: Logger; + expirySkewS?: number; + refreshAccessToken?: (params: RefreshGrantParams) => Promise; + now?: () => number; +} + +// Acquires an MCP OAuth access token for a (user, server): serves a cached token until it nears +// expiry, otherwise runs the refresh-token grant under a per-key mutex (one in-flight refresh per +// user+server in this process) and recovers from a concurrent refresh-token rotation via a single +// re-read + retry. A missing credential or a genuinely rejected refresh raises +// OAuthReauthRequiredError so the step pauses for re-authentication. +export default class OAuthTokenService { + private readonly store: McpOAuthCredentialsStore; + private readonly encryption: CredentialEncryption; + private readonly logger?: Logger; + private readonly expirySkewMs: number; + private readonly refreshAccessToken: (params: RefreshGrantParams) => Promise; + private readonly now: () => number; + private readonly cache = new Map(); + private readonly mutex = new KeyedMutex(); + + constructor(options: OAuthTokenServiceOptions) { + this.store = options.store; + this.encryption = options.encryption; + this.logger = options.logger; + this.expirySkewMs = (options.expirySkewS ?? DEFAULT_OAUTH_EXPIRY_SKEW_S) * 1000; + this.refreshAccessToken = options.refreshAccessToken ?? defaultRefreshAccessToken; + this.now = options.now ?? Date.now; + } + + async getAccessToken( + userId: number, + mcpServerId: string, + options?: { forceRefresh?: boolean }, + ): Promise { + const key = `${userId}:${mcpServerId}`; + const forceRefresh = options?.forceRefresh ?? false; + + if (!forceRefresh) { + const cached = this.readCache(key); + if (cached) return cached; + } + + return this.mutex.runExclusive(key, async () => { + // Re-check inside the lock: a concurrent caller may have just refreshed for this key. + if (!forceRefresh) { + const cached = this.readCache(key); + if (cached) return cached; + } + + return this.refreshAndCache(userId, mcpServerId, key); + }); + } + + private readCache(key: string): string | undefined { + const entry = this.cache.get(key); + if (!entry || entry.expiresAtMs === undefined) return undefined; + if (this.now() >= entry.expiresAtMs - this.expirySkewMs) return undefined; + + return entry.accessToken; + } + + private async refreshAndCache(userId: number, mcpServerId: string, key: string): Promise { + const credential = await this.store.get(userId, mcpServerId); + if (!credential) throw new OAuthReauthRequiredError(mcpServerId); + + const result = await this.runGrantWithRotationRetry(credential, userId, mcpServerId); + + this.cache.set(key, { + accessToken: result.accessToken, + expiresAtMs: + result.expiresInS !== undefined ? this.now() + result.expiresInS * 1000 : undefined, + }); + + if (result.refreshToken) { + await this.persistRotatedRefreshToken(credential, result.refreshToken); + } + + return result.accessToken; + } + + // On invalid_grant a peer instance likely rotated the refresh token out from under us: re-read the + // row and retry once with the current token. A second invalid_grant — or an unchanged token, which + // means no peer rotated it — is a genuine revocation and forces re-auth. + private async runGrantWithRotationRetry( + credential: StoredMcpOAuthCredential, + userId: number, + mcpServerId: string, + ): Promise { + try { + return await this.refreshAccessToken(this.toGrantParams(credential)); + } catch (error) { + if (!(error instanceof OAuthInvalidGrantError)) throw error; + + // A peer rotated the refresh token iff the stored value changed since we read it. + const latest = await this.store.get(userId, mcpServerId); + const wasRotated = + latest !== null && + latest.refreshTokenEnc.toString('base64') !== credential.refreshTokenEnc.toString('base64'); + + if (!latest || !wasRotated) { + throw new OAuthReauthRequiredError(mcpServerId); + } + + try { + return await this.refreshAccessToken(this.toGrantParams(latest)); + } catch (retryError) { + if (retryError instanceof OAuthInvalidGrantError) { + throw new OAuthReauthRequiredError(mcpServerId); + } + + throw retryError; + } + } + } + + private toGrantParams(credential: StoredMcpOAuthCredential): RefreshGrantParams { + return { + tokenEndpoint: credential.tokenEndpoint, + refreshToken: this.encryption.decrypt(credential.refreshTokenEnc), + clientId: credential.clientId, + clientSecret: credential.clientSecretEnc + ? this.encryption.decrypt(credential.clientSecretEnc) + : null, + tokenEndpointAuthMethod: credential.tokenEndpointAuthMethod, + scopes: credential.scopes, + }; + } + + private async persistRotatedRefreshToken( + credential: StoredMcpOAuthCredential, + refreshToken: string, + ): Promise { + const encrypted = this.encryption.encrypt(refreshToken); + + try { + await this.store.upsert({ + userId: credential.userId, + mcpServerId: credential.mcpServerId, + refreshTokenEnc: encrypted.ciphertext, + clientId: credential.clientId, + clientSecretEnc: credential.clientSecretEnc, + clientSecretExpiresAt: credential.clientSecretExpiresAt, + tokenEndpoint: credential.tokenEndpoint, + tokenEndpointAuthMethod: credential.tokenEndpointAuthMethod, + scopes: credential.scopes, + encKeyVersion: encrypted.encKeyVersion, + }); + } catch (error) { + // A failed write-back is not fatal: the access token just obtained is valid. The next refresh + // would hit invalid_grant and recover via the re-read path. + this.logger?.('Error', 'Failed to persist rotated MCP OAuth refresh token', { + mcpServerId: credential.mcpServerId, + error: error instanceof Error ? error.message : String(error), + }); + } + } +} diff --git a/packages/workflow-executor/src/ports/ai-model-port.ts b/packages/workflow-executor/src/ports/ai-model-port.ts index 5241a7a32c..30d0f18abf 100644 --- a/packages/workflow-executor/src/ports/ai-model-port.ts +++ b/packages/workflow-executor/src/ports/ai-model-port.ts @@ -1,4 +1,9 @@ -import type { BaseChatModel, RemoteTool, ToolConfig } from '@forestadmin/ai-proxy'; +import type { + BaseChatModel, + McpServerLoadFailure, + RemoteTool, + ToolConfig, +} from '@forestadmin/ai-proxy'; export interface GetModelOptions { aiConfigName?: string; @@ -8,5 +13,10 @@ export interface GetModelOptions { export interface AiModelPort { getModel(options?: GetModelOptions): BaseChatModel; loadRemoteTools(configs: Record): Promise; + // Loads tools and exposes per-server failures classified by cause (auth vs connection), so the + // OAuth path can tell a revoked token from an unreachable server. Default consumers use loadRemoteTools. + loadRemoteToolsWithFailures( + configs: Record, + ): Promise<{ tools: RemoteTool[]; failures: McpServerLoadFailure[] }>; closeConnections(): Promise; } diff --git a/packages/workflow-executor/src/remote-tool-fetcher.ts b/packages/workflow-executor/src/remote-tool-fetcher.ts index 1b79c44c24..c933d33720 100644 --- a/packages/workflow-executor/src/remote-tool-fetcher.ts +++ b/packages/workflow-executor/src/remote-tool-fetcher.ts @@ -1,8 +1,15 @@ +import type OAuthTokenService from './oauth/token-service'; import type { AiModelPort } from './ports/ai-model-port'; import type { Logger } from './ports/logger-port'; import type { WorkflowPort } from './ports/workflow-port'; import type { RemoteTool, ToolConfig } from '@forestadmin/ai-proxy'; +import { injectOauthTokens } from '@forestadmin/ai-proxy'; + +import { ConfigurationError, OAuthReauthRequiredError } from './errors'; + +const OAUTH2_AUTH_TYPE = 'oauth2'; + // Match by config.id, not by Record key: server names can collide across configs. export function scopeConfigsToServer( configs: Record, @@ -11,23 +18,38 @@ export function scopeConfigsToServer( return Object.fromEntries(Object.entries(configs).filter(([, cfg]) => cfg.id === mcpServerId)); } +function readAuthType(config: ToolConfig | undefined): string | undefined { + return (config as { authType?: string } | undefined)?.authType; +} + export interface FetchRemoteToolsResult { tools: RemoteTool[]; mcpServerName?: string; + // Present only for OAuth2 servers: re-mints the token (forced refresh) and reloads the tools, so + // the executor can retry once after an upstream 401 on a tool call. Throws OAuthReauthRequiredError + // when the credential can no longer be refreshed. + reloadWithFreshAuth?: () => Promise; } export default class RemoteToolFetcher { private readonly workflowPort: WorkflowPort; private readonly aiModelPort: AiModelPort; private readonly logger: Logger; - - constructor(workflowPort: WorkflowPort, aiModelPort: AiModelPort, logger: Logger) { + private readonly oauthTokenService?: OAuthTokenService; + + constructor( + workflowPort: WorkflowPort, + aiModelPort: AiModelPort, + logger: Logger, + oauthTokenService?: OAuthTokenService, + ) { this.workflowPort = workflowPort; this.aiModelPort = aiModelPort; this.logger = logger; + this.oauthTokenService = oauthTokenService; } - async fetch(mcpServerId: string): Promise { + async fetch(mcpServerId: string, userId: number): Promise { const configs = await this.workflowPort.getMcpServerConfigs(); const scoped = scopeConfigsToServer(configs, mcpServerId); const [mcpServerName] = Object.keys(scoped); @@ -36,13 +58,67 @@ export default class RemoteToolFetcher { if (Object.keys(scoped).length === 0) return { tools: [], mcpServerName }; - const tools = await this.aiModelPort.loadRemoteTools(scoped); + if (readAuthType(scoped[mcpServerName]) === OAUTH2_AUTH_TYPE) { + return this.fetchOAuthTools(scoped, mcpServerName, mcpServerId, userId); + } + const tools = await this.aiModelPort.loadRemoteTools(scoped); this.errorOnPartialLoadFailure(scoped, tools, mcpServerId, mcpServerName); return { tools, mcpServerName }; } + // OAuth2 path: acquire a per-user access token, inject it as a Bearer header, then list tools. + // Tool listing is the first authenticated call, so an auth failure here forces one token refresh + // and a single retry; a still-failing load is a genuine re-auth case. + private async fetchOAuthTools( + scoped: Record, + mcpServerName: string, + mcpServerId: string, + userId: number, + ): Promise { + if (!this.oauthTokenService) { + throw new ConfigurationError( + `OAuth-protected MCP server "${mcpServerId}" requires the database-backed executor ` + + `(no credential store is configured)`, + ); + } + + const tokenService = this.oauthTokenService; + + const attemptLoad = async ( + forceRefresh: boolean, + ): Promise<{ tools: RemoteTool[]; hasAuthFailure: boolean }> => { + const token = await tokenService.getAccessToken(userId, mcpServerId, { forceRefresh }); + const injected = + injectOauthTokens({ + configs: scoped, + tokensByMcpServerName: { [mcpServerName]: `Bearer ${token}` }, + }) ?? scoped; + const { tools, failures } = await this.aiModelPort.loadRemoteToolsWithFailures(injected); + + return { tools, hasAuthFailure: failures.some(failure => failure.kind === 'auth') }; + }; + + const reloadWithFreshAuth = async (): Promise => { + const attempt = await attemptLoad(true); + if (attempt.hasAuthFailure) throw new OAuthReauthRequiredError(mcpServerId); + this.errorOnPartialLoadFailure(scoped, attempt.tools, mcpServerId, mcpServerName); + + return attempt.tools; + }; + + const initial = await attemptLoad(false); + + if (initial.hasAuthFailure) { + return { tools: await reloadWithFreshAuth(), mcpServerName, reloadWithFreshAuth }; + } + + this.errorOnPartialLoadFailure(scoped, initial.tools, mcpServerId, mcpServerName); + + return { tools: initial.tools, mcpServerName, reloadWithFreshAuth }; + } + // Distinguish "no configs at all" (deployment misconfig) from "configs exist but none match" // (orchestrator/executor drift on server id) — both yield zero tools, but ops need to know // which one to fix. diff --git a/packages/workflow-executor/src/runner.ts b/packages/workflow-executor/src/runner.ts index ae07bb9e04..0d1b13cc30 100644 --- a/packages/workflow-executor/src/runner.ts +++ b/packages/workflow-executor/src/runner.ts @@ -1,4 +1,5 @@ import type { StepContextConfig } from './executors/step-executor-factory'; +import type OAuthTokenService from './oauth/token-service'; import type { ActivityLogPortFactory } from './ports/activity-log-port'; import type { AgentPort } from './ports/agent-port'; import type { AiModelPort } from './ports/ai-model-port'; @@ -50,6 +51,9 @@ export interface RunnerConfig { // Max number of ADDITIONAL steps auto-chained via /update-step response before yielding to the // next poll cycle (counted after the initial step). 0 disables chaining entirely. Default 50. maxChainDepth?: number; + // Wired only by the database-backed executor; absent in-memory, where the fetcher raises a + // ConfigurationError for oauth2 steps. + mcpOAuthTokenService?: OAuthTokenService; } // eslint-disable-next-line @typescript-eslint/no-var-requires, import/no-dynamic-require, global-require @@ -70,6 +74,7 @@ export default class Runner { config.workflowPort, config.aiModelPort, this.logger, + config.mcpOAuthTokenService, ); } @@ -313,7 +318,7 @@ export default class Runner { currentStep, this.contextConfig, this.config.activityLogPortFactory.forRun(currentToken), - mcpServerId => this.remoteToolFetcher.fetch(mcpServerId), + (mcpServerId, userId) => this.remoteToolFetcher.fetch(mcpServerId, userId), currentIncomingData, ); result = await executor.execute(); diff --git a/packages/workflow-executor/src/types/validated/step-outcome.ts b/packages/workflow-executor/src/types/validated/step-outcome.ts index 964e37e170..7f1b07f0e4 100644 --- a/packages/workflow-executor/src/types/validated/step-outcome.ts +++ b/packages/workflow-executor/src/types/validated/step-outcome.ts @@ -9,6 +9,12 @@ export type BaseStepStatus = z.infer; export const RecordStepStatusSchema = z.enum(['success', 'error', 'awaiting-input']); export type RecordStepStatus = z.infer; +// Typed reason for an awaiting-input pause the user must resolve out of band. The field name is +// `awaitingInputReason` (the orchestrator renamed it from `reason`); the Forest server validates it +// and the front reads it to prompt the matching action. +export const AwaitingInputReasonSchema = z.enum(['needs-oauth-reauth']); +export type AwaitingInputReason = z.infer; + export type StepStatus = BaseStepStatus | RecordStepStatus; /** @@ -48,6 +54,8 @@ export const McpStepOutcomeSchema = z ...baseOutcomeFields, type: z.literal('mcp'), status: RecordStepStatusSchema, + /** Present when status is 'awaiting-input' because the step paused for re-authentication. */ + awaitingInputReason: AwaitingInputReasonSchema.optional(), }) .strict(); export type McpStepOutcome = z.infer; diff --git a/packages/workflow-executor/test/adapters/ai-client-adapter.test.ts b/packages/workflow-executor/test/adapters/ai-client-adapter.test.ts index 2c87b82951..5dee091eac 100644 --- a/packages/workflow-executor/test/adapters/ai-client-adapter.test.ts +++ b/packages/workflow-executor/test/adapters/ai-client-adapter.test.ts @@ -2,12 +2,14 @@ import AiClientAdapter from '../../src/adapters/ai-client-adapter'; const mockGetModel = jest.fn().mockReturnValue({ invoke: jest.fn() }); const mockLoadRemoteTools = jest.fn().mockResolvedValue([]); +const mockLoadRemoteToolsWithFailures = jest.fn().mockResolvedValue({ tools: [], failures: [] }); const mockCloseConnections = jest.fn().mockResolvedValue(undefined); jest.mock('@forestadmin/ai-proxy', () => ({ AiClient: jest.fn().mockImplementation(() => ({ getModel: mockGetModel, loadRemoteTools: mockLoadRemoteTools, + loadRemoteToolsWithFailures: mockLoadRemoteToolsWithFailures, closeConnections: mockCloseConnections, })), })); @@ -44,6 +46,15 @@ describe('AiClientAdapter', () => { expect(mockLoadRemoteTools).toHaveBeenCalledWith(configs); }); + it('delegates loadRemoteToolsWithFailures to AiClient', async () => { + const adapter = new AiClientAdapter([]); + const configs = {}; + + await adapter.loadRemoteToolsWithFailures(configs); + + expect(mockLoadRemoteToolsWithFailures).toHaveBeenCalledWith(configs); + }); + it('delegates closeConnections to AiClient', async () => { const adapter = new AiClientAdapter([]); diff --git a/packages/workflow-executor/test/adapters/always-error-ai-model-port.test.ts b/packages/workflow-executor/test/adapters/always-error-ai-model-port.test.ts index 91489f26f6..8a2cab8722 100644 --- a/packages/workflow-executor/test/adapters/always-error-ai-model-port.test.ts +++ b/packages/workflow-executor/test/adapters/always-error-ai-model-port.test.ts @@ -32,6 +32,15 @@ describe('AlwaysErrorAiModelPort', () => { }); }); + describe('loadRemoteToolsWithFailures', () => { + it('returns no tools and no failures', async () => { + await expect(port.loadRemoteToolsWithFailures({} as never)).resolves.toEqual({ + tools: [], + failures: [], + }); + }); + }); + describe('closeConnections', () => { it('resolves without error', async () => { await expect(port.closeConnections()).resolves.toBeUndefined(); diff --git a/packages/workflow-executor/test/adapters/server-ai-adapter.test.ts b/packages/workflow-executor/test/adapters/server-ai-adapter.test.ts index cd765e16ce..42b917973e 100644 --- a/packages/workflow-executor/test/adapters/server-ai-adapter.test.ts +++ b/packages/workflow-executor/test/adapters/server-ai-adapter.test.ts @@ -2,6 +2,7 @@ import ServerAiAdapter from '../../src/adapters/server-ai-adapter'; const mockGetModel = jest.fn().mockReturnValue({ id: 'fake-model' }); const mockLoadRemoteTools = jest.fn().mockResolvedValue([]); +const mockLoadRemoteToolsWithFailures = jest.fn().mockResolvedValue({ tools: [], failures: [] }); const mockCloseConnections = jest.fn().mockResolvedValue(undefined); const mockAiClientConstructor = jest.fn(); @@ -12,6 +13,7 @@ jest.mock('@forestadmin/ai-proxy', () => ({ return { getModel: mockGetModel, loadRemoteTools: mockLoadRemoteTools, + loadRemoteToolsWithFailures: mockLoadRemoteToolsWithFailures, closeConnections: mockCloseConnections, }; }), @@ -132,6 +134,17 @@ describe('ServerAiAdapter', () => { }); }); + describe('loadRemoteToolsWithFailures', () => { + it('delegates to internal AiClient with the given configs', async () => { + const configs = {}; + + const result = await adapter.loadRemoteToolsWithFailures(configs); + + expect(mockLoadRemoteToolsWithFailures).toHaveBeenCalledWith(configs); + expect(result).toEqual({ tools: [], failures: [] }); + }); + }); + describe('closeConnections', () => { it('delegates to internal AiClient', async () => { await adapter.closeConnections(); diff --git a/packages/workflow-executor/test/adapters/step-outcome-to-update-step-mapper.test.ts b/packages/workflow-executor/test/adapters/step-outcome-to-update-step-mapper.test.ts index cdef5e425f..d050ace556 100644 --- a/packages/workflow-executor/test/adapters/step-outcome-to-update-step-mapper.test.ts +++ b/packages/workflow-executor/test/adapters/step-outcome-to-update-step-mapper.test.ts @@ -179,4 +179,50 @@ describe('toUpdateStepRequest', () => { expect(body.stepUpdate.stepIndex).toBe(7); }); + + describe('awaitingInputReason propagation', () => { + it('puts awaitingInputReason in the update-step context (done=false) when an mcp step pauses for reauth', () => { + const outcome: StepOutcome = { + type: 'mcp', + stepId: 'step-1', + stepIndex: 4, + status: 'awaiting-input', + awaitingInputReason: 'needs-oauth-reauth', + }; + + const body = toUpdateStepRequest('42', outcome); + + expect(body.stepUpdate.attributes).toEqual({ + done: false, + context: { status: 'awaiting-input', awaitingInputReason: 'needs-oauth-reauth' }, + }); + expect(body.executionStatus).toEqual({ type: 'awaiting-input' }); + }); + + it('omits awaitingInputReason for an awaiting-input pause without a reason', () => { + const outcome: StepOutcome = { + type: 'mcp', + stepId: 'step-1', + stepIndex: 0, + status: 'awaiting-input', + }; + + const body = toUpdateStepRequest('42', outcome); + + expect(body.stepUpdate.attributes.context).toEqual({ status: 'awaiting-input' }); + }); + + it('omits awaitingInputReason for a success outcome', () => { + const outcome: StepOutcome = { + type: 'mcp', + stepId: 'step-1', + stepIndex: 0, + status: 'success', + }; + + const body = toUpdateStepRequest('42', outcome); + + expect(body.stepUpdate.attributes.context).toEqual({ status: 'success' }); + }); + }); }); diff --git a/packages/workflow-executor/test/errors.test.ts b/packages/workflow-executor/test/errors.test.ts index be03b9562d..17ffbf92e5 100644 --- a/packages/workflow-executor/test/errors.test.ts +++ b/packages/workflow-executor/test/errors.test.ts @@ -2,6 +2,9 @@ import { AiModelPortError, InvalidPendingDataError, NoMcpToolsError, + OAuthInvalidGrantError, + OAuthReauthRequiredError, + OAuthRefreshError, PendingDataNotFoundError, extractErrorMessage, } from '../src/errors'; @@ -143,3 +146,34 @@ describe('InvalidPendingDataError', () => { expect(err.userMessage).toBe('The request body is invalid.'); }); }); + +describe('OAuthReauthRequiredError', () => { + it("defaults awaitingInputReason to 'needs-oauth-reauth' and names the server in the technical message", () => { + const err = new OAuthReauthRequiredError('srv-1'); + + expect(err.awaitingInputReason).toBe('needs-oauth-reauth'); + expect(err.message).toMatch(/srv-1/); + }); + + it('keeps the user-facing message free of the internal server id', () => { + const err = new OAuthReauthRequiredError('srv-1'); + + expect(err.userMessage).not.toMatch(/srv-1/); + }); +}); + +describe('OAuthRefreshError', () => { + it('prefixes the technical message and stores the cause', () => { + const cause = new Error('5xx'); + const err = new OAuthRefreshError('endpoint unreachable', cause); + + expect(err.message).toMatch(/endpoint unreachable/); + expect(err.cause).toBe(cause); + }); +}); + +describe('OAuthInvalidGrantError', () => { + it('includes the detail when provided', () => { + expect(new OAuthInvalidGrantError('token expired').message).toMatch(/token expired/); + }); +}); diff --git a/packages/workflow-executor/test/executors/mcp-step-executor.test.ts b/packages/workflow-executor/test/executors/mcp-step-executor.test.ts index 7f88825796..dc54c63566 100644 --- a/packages/workflow-executor/test/executors/mcp-step-executor.test.ts +++ b/packages/workflow-executor/test/executors/mcp-step-executor.test.ts @@ -8,7 +8,7 @@ import type { McpStepDefinition } from '../../src/types/validated/step-definitio import RemoteTool from '@forestadmin/ai-proxy/src/remote-tool'; -import { RunStorePortError, StepStateError } from '../../src/errors'; +import { OAuthReauthRequiredError, RunStorePortError, StepStateError } from '../../src/errors'; import ActivityLog from '../../src/executors/activity-log'; import AgentWithLog from '../../src/executors/agent-with-log'; import McpStepExecutor from '../../src/executors/mcp-step-executor'; @@ -1137,3 +1137,113 @@ describe('McpStepExecutor', () => { }); }); }); + +// The executor's OAuth responsibility is the tool-call 401 retry and the re-auth pause mapping. +// authType identification, the credential lookup, list-tools retry and Bearer injection live in +// RemoteToolFetcher / StepExecutorFactory and are covered by their own suites. +describe('McpStepExecutor — OAuth2 tool-call re-authentication', () => { + const authError = () => new Error('Request failed with status 401'); + + function makeAutomated(invoke: jest.Mock) { + const tool = new MockRemoteTool({ + name: 'send_notification', + sourceId: 'mcp-server-1', + invoke, + }); + const { model } = makeMockModel('send_notification', { message: 'Hello' }); + const context = makeContext({ + model, + stepDefinition: makeStep({ executionType: StepExecutionMode.FullyAutomated }), + }); + + return { tool, context }; + } + + it('force-refreshes, rebuilds the tool, and retries once when the call returns 401', async () => { + const { tool, context } = makeAutomated(jest.fn().mockRejectedValue(authError())); + const freshInvoke = jest.fn().mockResolvedValue('ok-after-refresh'); + const freshTool = new MockRemoteTool({ + name: 'send_notification', + sourceId: 'mcp-server-1', + invoke: freshInvoke, + }); + const reloadWithFreshAuth = jest.fn().mockResolvedValue([freshTool]); + + const result = await new McpStepExecutor(context, [tool], 'srv', reloadWithFreshAuth).execute(); + + expect(result.stepOutcome.status).toBe('success'); + expect(reloadWithFreshAuth).toHaveBeenCalledTimes(1); + expect(freshInvoke).toHaveBeenCalledWith({ message: 'Hello' }); + }); + + it("pauses with awaiting-input/'needs-oauth-reauth' when the credential can no longer be refreshed", async () => { + const { tool, context } = makeAutomated(jest.fn().mockRejectedValue(authError())); + const reloadWithFreshAuth = jest.fn().mockRejectedValue(new OAuthReauthRequiredError('srv')); + + const result = await new McpStepExecutor(context, [tool], 'srv', reloadWithFreshAuth).execute(); + + expect(result.stepOutcome).toMatchObject({ + status: 'awaiting-input', + awaitingInputReason: 'needs-oauth-reauth', + }); + }); + + it("pauses with 'needs-oauth-reauth' when the retried call still returns 401", async () => { + const { tool, context } = makeAutomated(jest.fn().mockRejectedValue(authError())); + const freshTool = new MockRemoteTool({ + name: 'send_notification', + sourceId: 'mcp-server-1', + invoke: jest.fn().mockRejectedValue(authError()), + }); + const reloadWithFreshAuth = jest.fn().mockResolvedValue([freshTool]); + + const result = await new McpStepExecutor(context, [tool], 'srv', reloadWithFreshAuth).execute(); + + expect(result.stepOutcome).toMatchObject({ + status: 'awaiting-input', + awaitingInputReason: 'needs-oauth-reauth', + }); + expect(reloadWithFreshAuth).toHaveBeenCalledTimes(1); + }); + + it('does not refresh for a non-auth tool error — surfaces it as a step error', async () => { + const { tool, context } = makeAutomated(jest.fn().mockRejectedValue(new Error('boom'))); + const reloadWithFreshAuth = jest.fn(); + + const result = await new McpStepExecutor(context, [tool], 'srv', reloadWithFreshAuth).execute(); + + expect(result.stepOutcome.status).toBe('error'); + expect(reloadWithFreshAuth).not.toHaveBeenCalled(); + }); + + it('does not refresh on a 401 for a bearer/none step (no reload hook)', async () => { + const { tool, context } = makeAutomated(jest.fn().mockRejectedValue(authError())); + + const result = await new McpStepExecutor(context, [tool], 'srv').execute(); + + expect(result.stepOutcome.status).toBe('error'); + }); + + it('surfaces a non-auth error from the retried call as a step error (not a reauth pause)', async () => { + const { tool, context } = makeAutomated(jest.fn().mockRejectedValue(authError())); + const freshTool = new MockRemoteTool({ + name: 'send_notification', + sourceId: 'mcp-server-1', + invoke: jest.fn().mockRejectedValue(new Error('downstream 500')), + }); + const reloadWithFreshAuth = jest.fn().mockResolvedValue([freshTool]); + + const result = await new McpStepExecutor(context, [tool], 'srv', reloadWithFreshAuth).execute(); + + expect(result.stepOutcome.status).toBe('error'); + }); + + it('errors when the refreshed tool set no longer contains the selected tool', async () => { + const { tool, context } = makeAutomated(jest.fn().mockRejectedValue(authError())); + const reloadWithFreshAuth = jest.fn().mockResolvedValue([]); + + const result = await new McpStepExecutor(context, [tool], 'srv', reloadWithFreshAuth).execute(); + + expect(result.stepOutcome.status).toBe('error'); + }); +}); diff --git a/packages/workflow-executor/test/executors/step-executor-factory.test.ts b/packages/workflow-executor/test/executors/step-executor-factory.test.ts new file mode 100644 index 0000000000..88e2019957 --- /dev/null +++ b/packages/workflow-executor/test/executors/step-executor-factory.test.ts @@ -0,0 +1,102 @@ +import type { StepContextConfig } from '../../src/executors/step-executor-factory'; +import type { ActivityLogPort } from '../../src/ports/activity-log-port'; +import type { AvailableStepExecution } from '../../src/types/execution-context'; + +import { OAuthReauthRequiredError } from '../../src/errors'; +import StepExecutorFactory from '../../src/executors/step-executor-factory'; +import SchemaCache from '../../src/schema-cache'; +import { StepExecutionMode, StepType } from '../../src/types/validated/step-definition'; + +const activityLogPort = { + createPending: jest.fn().mockResolvedValue({ id: 'log-1', index: '0' }), + markSucceeded: jest.fn().mockResolvedValue(undefined), + markFailed: jest.fn().mockResolvedValue(undefined), +} as unknown as ActivityLogPort; + +function makeStep(): AvailableStepExecution { + return { + runId: 'run-1', + stepId: 'step-1', + stepIndex: 0, + collectionId: 'col-1', + baseRecordRef: { collectionName: 'customers', recordId: [1], stepIndex: 0 }, + stepDefinition: { + type: StepType.Mcp, + executionType: StepExecutionMode.FullyAutomated, + mcpServerId: 'srv-1', + }, + previousSteps: [], + user: { + id: 7, + email: 'a@b.com', + firstName: 'A', + lastName: 'B', + team: 't', + renderingId: 1, + role: 'admin', + permissionLevel: 'admin', + tags: {}, + }, + } as unknown as AvailableStepExecution; +} + +function makeContextConfig(): StepContextConfig { + return { + aiModelPort: { getModel: jest.fn().mockReturnValue({}) }, + agentPort: {}, + workflowPort: {}, + runStore: {}, + schemaCache: new SchemaCache(), + logger: jest.fn(), + } as unknown as StepContextConfig; +} + +describe('StepExecutorFactory.create — MCP OAuth re-auth', () => { + it('maps OAuthReauthRequiredError from tool loading to an awaiting-input outcome with the typed reason', async () => { + const fetchRemoteTools = jest.fn().mockRejectedValue(new OAuthReauthRequiredError('srv-1')); + + const executor = await StepExecutorFactory.create( + makeStep(), + makeContextConfig(), + activityLogPort, + fetchRemoteTools, + ); + const result = await executor.execute(); + + expect(result.stepOutcome).toEqual({ + type: 'mcp', + stepId: 'step-1', + stepIndex: 0, + status: 'awaiting-input', + awaitingInputReason: 'needs-oauth-reauth', + }); + }); + + it('maps a generic tool-loading failure to an error outcome (no awaitingInputReason)', async () => { + const fetchRemoteTools = jest.fn().mockRejectedValue(new Error('kaboom')); + + const executor = await StepExecutorFactory.create( + makeStep(), + makeContextConfig(), + activityLogPort, + fetchRemoteTools, + ); + const result = await executor.execute(); + + expect(result.stepOutcome.status).toBe('error'); + expect(result.stepOutcome).not.toHaveProperty('awaitingInputReason'); + }); + + it('passes the step user id to the tool fetcher', async () => { + const fetchRemoteTools = jest.fn().mockResolvedValue({ tools: [], mcpServerName: 'srv' }); + + await StepExecutorFactory.create( + makeStep(), + makeContextConfig(), + activityLogPort, + fetchRemoteTools, + ); + + expect(fetchRemoteTools).toHaveBeenCalledWith('srv-1', 7); + }); +}); diff --git a/packages/workflow-executor/test/oauth/keyed-mutex.test.ts b/packages/workflow-executor/test/oauth/keyed-mutex.test.ts new file mode 100644 index 0000000000..02d0e756e9 --- /dev/null +++ b/packages/workflow-executor/test/oauth/keyed-mutex.test.ts @@ -0,0 +1,79 @@ +import KeyedMutex from '../../src/oauth/keyed-mutex'; + +function deferred() { + let resolve!: (value: T) => void; + let reject!: (reason?: unknown) => void; + const promise = new Promise((res, rej) => { + resolve = res; + reject = rej; + }); + + return { promise, resolve, reject }; +} + +describe('KeyedMutex', () => { + describe('runExclusive', () => { + it('returns the value the task resolves with', async () => { + const mutex = new KeyedMutex(); + + await expect(mutex.runExclusive('k', async () => 42)).resolves.toBe(42); + }); + + it('runs tasks sharing a key one at a time, in order', async () => { + const mutex = new KeyedMutex(); + const events: string[] = []; + const first = deferred(); + + const firstRun = mutex.runExclusive('user:1', async () => { + events.push('first:start'); + await first.promise; + events.push('first:end'); + }); + + const secondRun = mutex.runExclusive('user:1', async () => { + events.push('second:start'); + }); + + // The second task must not start while the first holds the key. + await Promise.resolve(); + expect(events).toEqual(['first:start']); + + first.resolve(); + await Promise.all([firstRun, secondRun]); + + expect(events).toEqual(['first:start', 'first:end', 'second:start']); + }); + + it('runs tasks for different keys concurrently', async () => { + const mutex = new KeyedMutex(); + const events: string[] = []; + const blocker = deferred(); + + const blocked = mutex.runExclusive('user:1', async () => { + events.push('a:start'); + await blocker.promise; + }); + const other = mutex.runExclusive('user:2', async () => { + events.push('b:start'); + }); + + await other; + // 'user:2' completed without waiting for the still-blocked 'user:1'. + expect(events).toEqual(['a:start', 'b:start']); + + blocker.resolve(); + await blocked; + }); + + it('lets a later task on the same key run after an earlier task rejects', async () => { + const mutex = new KeyedMutex(); + + const failed = mutex.runExclusive('user:1', async () => { + throw new Error('boom'); + }); + + await expect(failed).rejects.toThrow('boom'); + await expect(mutex.runExclusive('user:1', async () => 'ok')).resolves.toBe('ok'); + }); + }); +}); diff --git a/packages/workflow-executor/test/oauth/refresh-grant.test.ts b/packages/workflow-executor/test/oauth/refresh-grant.test.ts new file mode 100644 index 0000000000..ff1a3876a1 --- /dev/null +++ b/packages/workflow-executor/test/oauth/refresh-grant.test.ts @@ -0,0 +1,198 @@ +import { OAuthInvalidGrantError, OAuthRefreshError } from '../../src/errors'; +import refreshAccessToken from '../../src/oauth/refresh-grant'; + +function mockResponse(options: { + ok: boolean; + status: number; + payload?: unknown; + nonJson?: boolean; +}) { + return { + ok: options.ok, + status: options.status, + json: options.nonJson + ? () => Promise.reject(new Error('not json')) + : () => Promise.resolve(options.payload ?? {}), + } as unknown as Response; +} + +describe('refreshAccessToken', () => { + let fetchSpy: jest.SpyInstance; + + beforeEach(() => { + fetchSpy = jest.spyOn(global, 'fetch'); + }); + + afterEach(() => { + fetchSpy.mockRestore(); + }); + + function lastRequest() { + const [url, init] = fetchSpy.mock.calls[0]; + const body = init.body as URLSearchParams; + + return { url, headers: init.headers as Record, body }; + } + + it('posts a refresh_token grant with the refresh token to the token endpoint', async () => { + fetchSpy.mockResolvedValue( + mockResponse({ ok: true, status: 200, payload: { access_token: 'at' } }), + ); + + await refreshAccessToken({ tokenEndpoint: 'https://idp/token', refreshToken: 'rt-1' }); + + const { url, body, headers } = lastRequest(); + expect(url).toBe('https://idp/token'); + expect(fetchSpy.mock.calls[0][1].method).toBe('POST'); + expect(headers['content-type']).toBe('application/x-www-form-urlencoded'); + expect(body.get('grant_type')).toBe('refresh_token'); + expect(body.get('refresh_token')).toBe('rt-1'); + }); + + it('sends client credentials via Basic auth for client_secret_basic (the default with a secret)', async () => { + fetchSpy.mockResolvedValue( + mockResponse({ ok: true, status: 200, payload: { access_token: 'at' } }), + ); + + await refreshAccessToken({ + tokenEndpoint: 'https://idp/token', + refreshToken: 'rt-1', + clientId: 'cid', + clientSecret: 'secret', + }); + + const { headers, body } = lastRequest(); + expect(headers.authorization).toBe(`Basic ${Buffer.from('cid:secret').toString('base64')}`); + expect(body.get('client_secret')).toBeNull(); + }); + + it('sends client credentials in the body for client_secret_post', async () => { + fetchSpy.mockResolvedValue( + mockResponse({ ok: true, status: 200, payload: { access_token: 'at' } }), + ); + + await refreshAccessToken({ + tokenEndpoint: 'https://idp/token', + refreshToken: 'rt-1', + clientId: 'cid', + clientSecret: 'secret', + tokenEndpointAuthMethod: 'client_secret_post', + }); + + const { headers, body } = lastRequest(); + expect(headers.authorization).toBeUndefined(); + expect(body.get('client_id')).toBe('cid'); + expect(body.get('client_secret')).toBe('secret'); + }); + + it('sends only client_id for a public client (no secret)', async () => { + fetchSpy.mockResolvedValue( + mockResponse({ ok: true, status: 200, payload: { access_token: 'at' } }), + ); + + await refreshAccessToken({ + tokenEndpoint: 'https://idp/token', + refreshToken: 'rt-1', + clientId: 'cid', + }); + + const { headers, body } = lastRequest(); + expect(headers.authorization).toBeUndefined(); + expect(body.get('client_id')).toBe('cid'); + expect(body.get('client_secret')).toBeNull(); + }); + + it('includes the scope parameter only when scopes are provided', async () => { + fetchSpy.mockResolvedValue( + mockResponse({ ok: true, status: 200, payload: { access_token: 'at' } }), + ); + + await refreshAccessToken({ + tokenEndpoint: 'https://idp/token', + refreshToken: 'rt-1', + scopes: 'a b', + }); + expect(lastRequest().body.get('scope')).toBe('a b'); + + fetchSpy.mockClear(); + await refreshAccessToken({ tokenEndpoint: 'https://idp/token', refreshToken: 'rt-1' }); + expect(lastRequest().body.get('scope')).toBeNull(); + }); + + it('returns the access token, expiry, and a rotated refresh token on success', async () => { + fetchSpy.mockResolvedValue( + mockResponse({ + ok: true, + status: 200, + payload: { access_token: 'at-1', expires_in: 3600, refresh_token: 'rt-2' }, + }), + ); + + const result = await refreshAccessToken({ + tokenEndpoint: 'https://idp/token', + refreshToken: 'rt-1', + }); + + expect(result).toEqual({ accessToken: 'at-1', expiresInS: 3600, refreshToken: 'rt-2' }); + }); + + it('leaves expiresInS undefined when the response omits expires_in', async () => { + fetchSpy.mockResolvedValue( + mockResponse({ ok: true, status: 200, payload: { access_token: 'at-1' } }), + ); + + const result = await refreshAccessToken({ + tokenEndpoint: 'https://idp/token', + refreshToken: 'rt-1', + }); + + expect(result.expiresInS).toBeUndefined(); + expect(result.refreshToken).toBeUndefined(); + }); + + it('throws OAuthInvalidGrantError when the endpoint returns error invalid_grant', async () => { + fetchSpy.mockResolvedValue( + mockResponse({ ok: false, status: 400, payload: { error: 'invalid_grant' } }), + ); + + await expect( + refreshAccessToken({ tokenEndpoint: 'https://idp/token', refreshToken: 'rt-1' }), + ).rejects.toBeInstanceOf(OAuthInvalidGrantError); + }); + + it('throws OAuthRefreshError for a non-invalid_grant error response', async () => { + fetchSpy.mockResolvedValue( + mockResponse({ ok: false, status: 400, payload: { error: 'invalid_client' } }), + ); + + await expect( + refreshAccessToken({ tokenEndpoint: 'https://idp/token', refreshToken: 'rt-1' }), + ).rejects.toBeInstanceOf(OAuthRefreshError); + }); + + it('throws OAuthRefreshError on a 5xx from the token endpoint', async () => { + fetchSpy.mockResolvedValue(mockResponse({ ok: false, status: 503, nonJson: true })); + + await expect( + refreshAccessToken({ tokenEndpoint: 'https://idp/token', refreshToken: 'rt-1' }), + ).rejects.toBeInstanceOf(OAuthRefreshError); + }); + + it('throws OAuthRefreshError when the endpoint is unreachable', async () => { + fetchSpy.mockRejectedValue(new Error('ECONNREFUSED')); + + await expect( + refreshAccessToken({ tokenEndpoint: 'https://idp/token', refreshToken: 'rt-1' }), + ).rejects.toBeInstanceOf(OAuthRefreshError); + }); + + it('throws OAuthRefreshError when a 200 response carries no access_token', async () => { + fetchSpy.mockResolvedValue( + mockResponse({ ok: true, status: 200, payload: { token_type: 'bearer' } }), + ); + + await expect( + refreshAccessToken({ tokenEndpoint: 'https://idp/token', refreshToken: 'rt-1' }), + ).rejects.toBeInstanceOf(OAuthRefreshError); + }); +}); diff --git a/packages/workflow-executor/test/oauth/token-service.test.ts b/packages/workflow-executor/test/oauth/token-service.test.ts new file mode 100644 index 0000000000..a3c7ef6709 --- /dev/null +++ b/packages/workflow-executor/test/oauth/token-service.test.ts @@ -0,0 +1,300 @@ +import type CredentialEncryption from '../../src/crypto/credential-encryption'; +import type { RefreshGrantParams, RefreshGrantResult } from '../../src/oauth/refresh-grant'; +import type McpOAuthCredentialsStore from '../../src/stores/mcp-oauth-credentials-store'; +import type { StoredMcpOAuthCredential } from '../../src/stores/mcp-oauth-credentials-store'; + +import { + OAuthInvalidGrantError, + OAuthReauthRequiredError, + OAuthRefreshError, +} from '../../src/errors'; +import OAuthTokenService from '../../src/oauth/token-service'; + +const USER_ID = 7; +const SERVER_ID = 'srv-1'; + +function makeCredential( + overrides: Partial = {}, +): StoredMcpOAuthCredential { + return { + id: 1, + userId: USER_ID, + mcpServerId: SERVER_ID, + refreshTokenEnc: Buffer.from('enc-rt-1'), + clientId: 'cid', + clientSecretEnc: Buffer.from('enc-secret'), + clientSecretExpiresAt: null, + tokenEndpoint: 'https://idp/token', + tokenEndpointAuthMethod: 'client_secret_basic', + scopes: 'a b', + encKeyVersion: 1, + ...overrides, + }; +} + +function setup(options?: { + credential?: StoredMcpOAuthCredential | null; + refresh?: jest.Mock, [RefreshGrantParams]>; + expirySkewS?: number; +}) { + const credential = options?.credential === undefined ? makeCredential() : options.credential; + const get = jest.fn().mockResolvedValue(credential); + const upsert = jest.fn().mockResolvedValue(undefined); + const store = { get, upsert } as unknown as McpOAuthCredentialsStore; + + const decrypt = jest.fn((buf: Buffer) => `decrypted:${buf.toString()}`); + const encrypt = jest.fn((plain: string) => ({ + ciphertext: Buffer.from(`enc:${plain}`), + encKeyVersion: 2, + })); + const encryption = { decrypt, encrypt } as unknown as CredentialEncryption; + + const refresh = + options?.refresh ?? jest.fn().mockResolvedValue({ accessToken: 'at-1', expiresInS: 3600 }); + + let clock = 1_000_000; + const now = () => clock; + + const service = new OAuthTokenService({ + store, + encryption, + refreshAccessToken: refresh, + expirySkewS: options?.expirySkewS ?? 60, + now, + }); + + return { + service, + get, + upsert, + decrypt, + encrypt, + refresh, + advance: (ms: number) => { + clock += ms; + }, + }; +} + +describe('OAuthTokenService.getAccessToken', () => { + describe('acquisition', () => { + it('looks up the credential by user and server, refreshes, and returns the access token', async () => { + const { service, get, refresh } = setup(); + + const token = await service.getAccessToken(USER_ID, SERVER_ID); + + expect(token).toBe('at-1'); + expect(get).toHaveBeenCalledWith(USER_ID, SERVER_ID); + expect(refresh).toHaveBeenCalledTimes(1); + }); + + it('decrypts the refresh token and client secret and passes the stored endpoint to the grant', async () => { + const { service, refresh } = setup(); + + await service.getAccessToken(USER_ID, SERVER_ID); + + expect(refresh).toHaveBeenCalledWith({ + tokenEndpoint: 'https://idp/token', + refreshToken: 'decrypted:enc-rt-1', + clientId: 'cid', + clientSecret: 'decrypted:enc-secret', + tokenEndpointAuthMethod: 'client_secret_basic', + scopes: 'a b', + }); + }); + + it('passes a null client secret to the grant when none is stored', async () => { + const { service, refresh } = setup({ credential: makeCredential({ clientSecretEnc: null }) }); + + await service.getAccessToken(USER_ID, SERVER_ID); + + expect(refresh).toHaveBeenCalledWith(expect.objectContaining({ clientSecret: null })); + }); + + it('raises OAuthReauthRequiredError and never refreshes when no credential is stored', async () => { + const { service, refresh } = setup({ credential: null }); + + await expect(service.getAccessToken(USER_ID, SERVER_ID)).rejects.toBeInstanceOf( + OAuthReauthRequiredError, + ); + expect(refresh).not.toHaveBeenCalled(); + }); + + it('propagates a transient refresh failure without converting it to a re-auth pause', async () => { + const refresh = jest.fn().mockRejectedValue(new OAuthRefreshError('5xx')); + const { service } = setup({ refresh }); + + await expect(service.getAccessToken(USER_ID, SERVER_ID)).rejects.toBeInstanceOf( + OAuthRefreshError, + ); + }); + }); + + describe('expiry-skew cache', () => { + it('serves the cached token on a second call within the skew window', async () => { + const { service, refresh } = setup(); + + const first = await service.getAccessToken(USER_ID, SERVER_ID); + const second = await service.getAccessToken(USER_ID, SERVER_ID); + + expect(first).toBe(second); + expect(refresh).toHaveBeenCalledTimes(1); + }); + + it('refreshes again once the token is within the skew window of expiry', async () => { + const { service, refresh, advance } = setup({ expirySkewS: 60 }); + + await service.getAccessToken(USER_ID, SERVER_ID); // expires_in 3600s, skew 60s + advance((3600 - 60) * 1000); // now exactly at the skew threshold + + await service.getAccessToken(USER_ID, SERVER_ID); + + expect(refresh).toHaveBeenCalledTimes(2); + }); + + it('does not cache a token when the grant omits expires_in', async () => { + const refresh = jest.fn().mockResolvedValue({ accessToken: 'at-1' }); + const { service } = setup({ refresh }); + + await service.getAccessToken(USER_ID, SERVER_ID); + await service.getAccessToken(USER_ID, SERVER_ID); + + expect(refresh).toHaveBeenCalledTimes(2); + }); + + it('forceRefresh bypasses a still-valid cached token', async () => { + const { service, refresh } = setup(); + + await service.getAccessToken(USER_ID, SERVER_ID); + await service.getAccessToken(USER_ID, SERVER_ID, { forceRefresh: true }); + + expect(refresh).toHaveBeenCalledTimes(2); + }); + + it('caches independently per (user, server)', async () => { + const { service, refresh } = setup(); + + await service.getAccessToken(USER_ID, SERVER_ID); + await service.getAccessToken(USER_ID, 'other-server'); + + expect(refresh).toHaveBeenCalledTimes(2); + }); + }); + + describe('refresh-token rotation', () => { + it('persists the rotated refresh token, encrypted, when the grant returns a new one', async () => { + const refresh = jest + .fn() + .mockResolvedValue({ accessToken: 'at-1', expiresInS: 3600, refreshToken: 'rt-2' }); + const { service, upsert, encrypt } = setup({ refresh }); + + await service.getAccessToken(USER_ID, SERVER_ID); + + expect(encrypt).toHaveBeenCalledWith('rt-2'); + expect(upsert).toHaveBeenCalledWith( + expect.objectContaining({ + userId: USER_ID, + mcpServerId: SERVER_ID, + refreshTokenEnc: Buffer.from('enc:rt-2'), + encKeyVersion: 2, + }), + ); + }); + + it('does not write back when the grant returns no new refresh token', async () => { + const { service, upsert } = setup(); + + await service.getAccessToken(USER_ID, SERVER_ID); + + expect(upsert).not.toHaveBeenCalled(); + }); + + it('still returns the token when the rotation write-back fails', async () => { + const refresh = jest + .fn() + .mockResolvedValue({ accessToken: 'at-1', expiresInS: 3600, refreshToken: 'rt-2' }); + const { service, upsert } = setup({ refresh }); + (upsert as jest.Mock).mockRejectedValue(new Error('db down')); + + await expect(service.getAccessToken(USER_ID, SERVER_ID)).resolves.toBe('at-1'); + }); + }); + + describe('invalid_grant — concurrent rotation recovery', () => { + it('re-reads the credential and retries once with the rotated token', async () => { + const rotated = makeCredential({ refreshTokenEnc: Buffer.from('enc-rt-rotated') }); + const get = jest.fn().mockResolvedValueOnce(makeCredential()).mockResolvedValueOnce(rotated); + const refresh = jest + .fn() + .mockRejectedValueOnce(new OAuthInvalidGrantError()) + .mockResolvedValueOnce({ accessToken: 'at-after-retry', expiresInS: 3600 }); + + const service = new OAuthTokenService({ + store: { get, upsert: jest.fn() } as unknown as McpOAuthCredentialsStore, + encryption: { + decrypt: (buf: Buffer) => `decrypted:${buf.toString()}`, + encrypt: jest.fn(), + } as unknown as CredentialEncryption, + refreshAccessToken: refresh, + }); + + const token = await service.getAccessToken(USER_ID, SERVER_ID); + + expect(token).toBe('at-after-retry'); + expect(refresh).toHaveBeenNthCalledWith( + 2, + expect.objectContaining({ refreshToken: 'decrypted:enc-rt-rotated' }), + ); + }); + + it('raises OAuthReauthRequiredError when the re-read shows the same (unrotated) token', async () => { + const refresh = jest.fn().mockRejectedValue(new OAuthInvalidGrantError()); + const { service } = setup({ refresh }); + + await expect(service.getAccessToken(USER_ID, SERVER_ID)).rejects.toBeInstanceOf( + OAuthReauthRequiredError, + ); + expect(refresh).toHaveBeenCalledTimes(1); + }); + + it('raises OAuthReauthRequiredError when the retry also returns invalid_grant', async () => { + const rotated = makeCredential({ refreshTokenEnc: Buffer.from('enc-rt-rotated') }); + const get = jest.fn().mockResolvedValueOnce(makeCredential()).mockResolvedValueOnce(rotated); + const refresh = jest.fn().mockRejectedValue(new OAuthInvalidGrantError()); + + const service = new OAuthTokenService({ + store: { get, upsert: jest.fn() } as unknown as McpOAuthCredentialsStore, + encryption: { + decrypt: (buf: Buffer) => buf.toString(), + encrypt: jest.fn(), + } as unknown as CredentialEncryption, + refreshAccessToken: refresh, + }); + + await expect(service.getAccessToken(USER_ID, SERVER_ID)).rejects.toBeInstanceOf( + OAuthReauthRequiredError, + ); + expect(refresh).toHaveBeenCalledTimes(2); + }); + }); + + describe('serialization', () => { + it('collapses concurrent requests for the same (user, server) into a single refresh', async () => { + let resolveRefresh!: (value: RefreshGrantResult) => void; + const refresh = jest.fn().mockReturnValue( + new Promise(resolve => { + resolveRefresh = resolve; + }), + ); + const { service } = setup({ refresh }); + + const a = service.getAccessToken(USER_ID, SERVER_ID); + const b = service.getAccessToken(USER_ID, SERVER_ID); + resolveRefresh({ accessToken: 'at-shared', expiresInS: 3600 }); + + await expect(a).resolves.toBe('at-shared'); + await expect(b).resolves.toBe('at-shared'); + expect(refresh).toHaveBeenCalledTimes(1); + }); + }); +}); diff --git a/packages/workflow-executor/test/remote-tool-fetcher.test.ts b/packages/workflow-executor/test/remote-tool-fetcher.test.ts index 7527816adc..4cb74df672 100644 --- a/packages/workflow-executor/test/remote-tool-fetcher.test.ts +++ b/packages/workflow-executor/test/remote-tool-fetcher.test.ts @@ -1,16 +1,25 @@ +import type OAuthTokenService from '../src/oauth/token-service'; import type { AiModelPort } from '../src/ports/ai-model-port'; import type { Logger } from '../src/ports/logger-port'; import type { WorkflowPort } from '../src/ports/workflow-port'; import type { RemoteTool, ToolConfig } from '@forestadmin/ai-proxy'; +import { ConfigurationError, OAuthReauthRequiredError } from '../src/errors'; import RemoteToolFetcher, { scopeConfigsToServer } from '../src/remote-tool-fetcher'; +const USER_ID = 1; + +type AiModelMethods = Pick; + function createMockWorkflowPort(): jest.Mocked> { return { getMcpServerConfigs: jest.fn().mockResolvedValue({}) }; } -function createMockAiModelPort(): jest.Mocked> { - return { loadRemoteTools: jest.fn().mockResolvedValue([]) }; +function createMockAiModelPort(): jest.Mocked { + return { + loadRemoteTools: jest.fn().mockResolvedValue([]), + loadRemoteToolsWithFailures: jest.fn().mockResolvedValue({ tools: [], failures: [] }), + }; } function createMockLogger(): jest.MockedFunction { @@ -23,8 +32,9 @@ function makeRemoteTool(sourceId: string, mcpServerId?: string): RemoteTool { function makeFetcher(overrides?: { workflowPort?: Partial>>; - aiModelPort?: Partial>>; + aiModelPort?: Partial>; logger?: jest.MockedFunction; + tokenService?: OAuthTokenService; }) { const workflowPort = { ...createMockWorkflowPort(), ...overrides?.workflowPort }; const aiModelPort = { ...createMockAiModelPort(), ...overrides?.aiModelPort }; @@ -33,6 +43,7 @@ function makeFetcher(overrides?: { workflowPort as unknown as WorkflowPort, aiModelPort as unknown as AiModelPort, logger, + overrides?.tokenService, ); return { fetcher, workflowPort, aiModelPort, logger }; @@ -41,6 +52,15 @@ function makeFetcher(overrides?: { const cfg = (id: string | undefined): ToolConfig => ({ id, url: 'https://x.example', type: 'http' as const, headers: {} } as unknown as ToolConfig); +const oauthCfg = (id: string): ToolConfig => + ({ + id, + authType: 'oauth2', + url: 'https://x.example', + type: 'http' as const, + headers: {}, + } as unknown as ToolConfig); + // --------------------------------------------------------------------------- // scopeConfigsToServer (pure) // --------------------------------------------------------------------------- @@ -82,7 +102,7 @@ describe('RemoteToolFetcher.fetch', () => { }, }); - await fetcher.fetch('id-A'); + await fetcher.fetch('id-A', USER_ID); expect(aiModelPort.loadRemoteTools).toHaveBeenCalledWith({ 'srv-a': cfg('id-A') }); }); @@ -92,7 +112,7 @@ describe('RemoteToolFetcher.fetch', () => { workflowPort: { getMcpServerConfigs: jest.fn().mockResolvedValue({}) }, }); - const result = await fetcher.fetch('id-A'); + const result = await fetcher.fetch('id-A', USER_ID); expect(result).toEqual({ tools: [], mcpServerName: undefined }); expect(aiModelPort.loadRemoteTools).not.toHaveBeenCalled(); @@ -107,7 +127,7 @@ describe('RemoteToolFetcher.fetch', () => { aiModelPort: { loadRemoteTools: jest.fn().mockResolvedValue(remoteTools) }, }); - const result = await fetcher.fetch('id-A'); + const result = await fetcher.fetch('id-A', USER_ID); expect(result).toEqual({ tools: remoteTools, mcpServerName: 'srv-a' }); }); @@ -121,7 +141,7 @@ describe('RemoteToolFetcher.fetch', () => { }, }); - await fetcher.fetch('id-missing'); + await fetcher.fetch('id-missing', USER_ID); expect(logger).toHaveBeenCalledWith( 'Warn', @@ -139,7 +159,7 @@ describe('RemoteToolFetcher.fetch', () => { workflowPort: { getMcpServerConfigs: jest.fn().mockResolvedValue({}) }, }); - await fetcher.fetch('id-A'); + await fetcher.fetch('id-A', USER_ID); expect(logger).toHaveBeenCalledWith( 'Warn', @@ -160,7 +180,7 @@ describe('RemoteToolFetcher.fetch', () => { }, }); - await fetcher.fetch('id-A'); + await fetcher.fetch('id-A', USER_ID); expect(logger.mock.calls.find(c => c[0] === 'Warn')).toBeUndefined(); }); @@ -173,7 +193,7 @@ describe('RemoteToolFetcher.fetch', () => { aiModelPort: { loadRemoteTools: jest.fn().mockResolvedValue([]) }, }); - await fetcher.fetch('id-A'); + await fetcher.fetch('id-A', USER_ID); expect(logger).toHaveBeenCalledWith('Error', 'MCP servers failed to load tools', { requestedMcpServerId: 'id-A', @@ -192,7 +212,7 @@ describe('RemoteToolFetcher.fetch', () => { }, }); - await fetcher.fetch('id-A'); + await fetcher.fetch('id-A', USER_ID); expect(logger.mock.calls.find(c => c[0] === 'Error')).toBeUndefined(); }); @@ -214,7 +234,7 @@ describe('RemoteToolFetcher.fetch', () => { }, }); - await fetcher.fetch('id-zendesk'); + await fetcher.fetch('id-zendesk', USER_ID); expect(logger.mock.calls.find(c => c[0] === 'Error')).toBeUndefined(); }); @@ -232,7 +252,7 @@ describe('RemoteToolFetcher.fetch', () => { aiModelPort: { loadRemoteTools: jest.fn().mockResolvedValue([]) }, }); - await fetcher.fetch('id-zendesk'); + await fetcher.fetch('id-zendesk', USER_ID); expect(logger).toHaveBeenCalledWith('Error', 'MCP servers failed to load tools', { requestedMcpServerId: 'id-zendesk', @@ -250,7 +270,7 @@ describe('RemoteToolFetcher.fetch', () => { aiModelPort: { loadRemoteTools: jest.fn().mockResolvedValue(remoteTools) }, }); - const result = await fetcher.fetch('id-A'); + const result = await fetcher.fetch('id-A', USER_ID); expect(result.tools).toBe(remoteTools); }); @@ -265,7 +285,7 @@ describe('RemoteToolFetcher.fetch', () => { }, }); - await expect(fetcher.fetch('id-A')).rejects.toThrow('MCP unreachable'); + await expect(fetcher.fetch('id-A', USER_ID)).rejects.toThrow('MCP unreachable'); expect(logger.mock.calls.find(c => c[0] === 'Error')).toBeUndefined(); }); @@ -276,7 +296,142 @@ describe('RemoteToolFetcher.fetch', () => { }, }); - await expect(fetcher.fetch('id-A')).rejects.toThrow('orchestrator down'); + await expect(fetcher.fetch('id-A', USER_ID)).rejects.toThrow('orchestrator down'); expect(aiModelPort.loadRemoteTools).not.toHaveBeenCalled(); }); }); + +describe('RemoteToolFetcher.fetch — OAuth2 servers', () => { + const makeTokenService = (getAccessToken: jest.Mock): OAuthTokenService => + ({ getAccessToken } as unknown as OAuthTokenService); + + const authFailure = { + server: 'srv-a', + mcpServerId: 'id-A', + kind: 'auth', + error: new Error('401'), + }; + + it('acquires a token, injects it as a Bearer header, and returns the loaded tools + a reload hook', async () => { + const tool = makeRemoteTool('srv-a', 'id-A'); + const getAccessToken = jest.fn().mockResolvedValue('tok-1'); + const loadRemoteToolsWithFailures = jest + .fn() + .mockResolvedValue({ tools: [tool], failures: [] }); + const { fetcher } = makeFetcher({ + workflowPort: { + getMcpServerConfigs: jest.fn().mockResolvedValue({ 'srv-a': oauthCfg('id-A') }), + }, + aiModelPort: { loadRemoteToolsWithFailures }, + tokenService: makeTokenService(getAccessToken), + }); + + const result = await fetcher.fetch('id-A', USER_ID); + + expect(getAccessToken).toHaveBeenCalledWith(USER_ID, 'id-A', { forceRefresh: false }); + expect(loadRemoteToolsWithFailures).toHaveBeenCalledWith({ + 'srv-a': expect.objectContaining({ headers: { Authorization: 'Bearer tok-1' } }), + }); + expect(result.tools).toEqual([tool]); + expect(typeof result.reloadWithFreshAuth).toBe('function'); + }); + + it('throws ConfigurationError when no token service is wired (in-memory executor)', async () => { + const { fetcher } = makeFetcher({ + workflowPort: { + getMcpServerConfigs: jest.fn().mockResolvedValue({ 'srv-a': oauthCfg('id-A') }), + }, + }); + + await expect(fetcher.fetch('id-A', USER_ID)).rejects.toBeInstanceOf(ConfigurationError); + }); + + it('force-refreshes and retries list-tools once on an auth failure, then succeeds', async () => { + const tool = makeRemoteTool('srv-a', 'id-A'); + const getAccessToken = jest.fn().mockResolvedValue('tok'); + const loadRemoteToolsWithFailures = jest + .fn() + .mockResolvedValueOnce({ tools: [], failures: [authFailure] }) + .mockResolvedValueOnce({ tools: [tool], failures: [] }); + const { fetcher } = makeFetcher({ + workflowPort: { + getMcpServerConfigs: jest.fn().mockResolvedValue({ 'srv-a': oauthCfg('id-A') }), + }, + aiModelPort: { loadRemoteToolsWithFailures }, + tokenService: makeTokenService(getAccessToken), + }); + + const result = await fetcher.fetch('id-A', USER_ID); + + expect(getAccessToken).toHaveBeenNthCalledWith(1, USER_ID, 'id-A', { forceRefresh: false }); + expect(getAccessToken).toHaveBeenNthCalledWith(2, USER_ID, 'id-A', { forceRefresh: true }); + expect(result.tools).toEqual([tool]); + }); + + it('raises OAuthReauthRequiredError when the auth failure persists after a forced refresh', async () => { + const getAccessToken = jest.fn().mockResolvedValue('tok'); + const loadRemoteToolsWithFailures = jest + .fn() + .mockResolvedValue({ tools: [], failures: [authFailure] }); + const { fetcher } = makeFetcher({ + workflowPort: { + getMcpServerConfigs: jest.fn().mockResolvedValue({ 'srv-a': oauthCfg('id-A') }), + }, + aiModelPort: { loadRemoteToolsWithFailures }, + tokenService: makeTokenService(getAccessToken), + }); + + await expect(fetcher.fetch('id-A', USER_ID)).rejects.toBeInstanceOf(OAuthReauthRequiredError); + }); + + it('propagates OAuthReauthRequiredError from token acquisition (no stored credential)', async () => { + const getAccessToken = jest.fn().mockRejectedValue(new OAuthReauthRequiredError('id-A')); + const { fetcher } = makeFetcher({ + workflowPort: { + getMcpServerConfigs: jest.fn().mockResolvedValue({ 'srv-a': oauthCfg('id-A') }), + }, + tokenService: makeTokenService(getAccessToken), + }); + + await expect(fetcher.fetch('id-A', USER_ID)).rejects.toBeInstanceOf(OAuthReauthRequiredError); + }); + + it('reloadWithFreshAuth forces a refresh and returns freshly-authed tools', async () => { + const tool = makeRemoteTool('srv-a', 'id-A'); + const getAccessToken = jest.fn().mockResolvedValue('tok'); + const loadRemoteToolsWithFailures = jest + .fn() + .mockResolvedValue({ tools: [tool], failures: [] }); + const { fetcher } = makeFetcher({ + workflowPort: { + getMcpServerConfigs: jest.fn().mockResolvedValue({ 'srv-a': oauthCfg('id-A') }), + }, + aiModelPort: { loadRemoteToolsWithFailures }, + tokenService: makeTokenService(getAccessToken), + }); + + const { reloadWithFreshAuth } = await fetcher.fetch('id-A', USER_ID); + if (!reloadWithFreshAuth) throw new Error('expected reloadWithFreshAuth to be defined'); + getAccessToken.mockClear(); + const reloaded = await reloadWithFreshAuth(); + + expect(getAccessToken).toHaveBeenCalledWith(USER_ID, 'id-A', { forceRefresh: true }); + expect(reloaded).toEqual([tool]); + }); + + it('leaves the token service untouched for a bearer/none server', async () => { + const getAccessToken = jest.fn(); + const tool = makeRemoteTool('srv-a', 'id-A'); + const { fetcher, aiModelPort } = makeFetcher({ + workflowPort: { getMcpServerConfigs: jest.fn().mockResolvedValue({ 'srv-a': cfg('id-A') }) }, + aiModelPort: { loadRemoteTools: jest.fn().mockResolvedValue([tool]) }, + tokenService: makeTokenService(getAccessToken), + }); + + const result = await fetcher.fetch('id-A', USER_ID); + + expect(getAccessToken).not.toHaveBeenCalled(); + expect(aiModelPort.loadRemoteTools).toHaveBeenCalled(); + expect(result.reloadWithFreshAuth).toBeUndefined(); + }); +}); diff --git a/packages/workflow-executor/test/runner.test.ts b/packages/workflow-executor/test/runner.test.ts index e0afcde406..d8708935a3 100644 --- a/packages/workflow-executor/test/runner.test.ts +++ b/packages/workflow-executor/test/runner.test.ts @@ -1711,7 +1711,7 @@ describe('StepExecutorFactory.create — factory', () => { fetchRemoteTools, ); expect(executor).toBeInstanceOf(McpStepExecutor); - expect(fetchRemoteTools).toHaveBeenCalledWith('srv-42'); + expect(fetchRemoteTools).toHaveBeenCalledWith('srv-42', 1); expect( ( executor as unknown as { getExtraLogContext(): Record } diff --git a/packages/workflow-executor/test/types/step-outcome.test.ts b/packages/workflow-executor/test/types/step-outcome.test.ts index b137ea0bc6..419077e64b 100644 --- a/packages/workflow-executor/test/types/step-outcome.test.ts +++ b/packages/workflow-executor/test/types/step-outcome.test.ts @@ -1,5 +1,8 @@ import { StepType } from '../../src/types/validated/step-definition'; -import { stepTypeToOutcomeType } from '../../src/types/validated/step-outcome'; +import { + McpStepOutcomeSchema, + stepTypeToOutcomeType, +} from '../../src/types/validated/step-outcome'; describe('stepTypeToOutcomeType', () => { it('maps Condition to condition', () => { @@ -34,3 +37,43 @@ describe('stepTypeToOutcomeType', () => { expect(stepTypeToOutcomeType('future-step-type' as StepType)).toBe('record'); }); }); + +describe('McpStepOutcomeSchema — awaitingInputReason', () => { + const base = { type: 'mcp' as const, stepId: 'step-1', stepIndex: 0 }; + + it("accepts an awaiting-input outcome carrying awaitingInputReason 'needs-oauth-reauth'", () => { + const parsed = McpStepOutcomeSchema.parse({ + ...base, + status: 'awaiting-input', + awaitingInputReason: 'needs-oauth-reauth', + }); + + expect(parsed.awaitingInputReason).toBe('needs-oauth-reauth'); + }); + + it('allows awaitingInputReason to be omitted', () => { + const parsed = McpStepOutcomeSchema.parse({ ...base, status: 'awaiting-input' }); + + expect(parsed.awaitingInputReason).toBeUndefined(); + }); + + it('rejects an unknown awaitingInputReason value', () => { + expect(() => + McpStepOutcomeSchema.parse({ + ...base, + status: 'awaiting-input', + awaitingInputReason: 'nope', + }), + ).toThrow(); + }); + + it("rejects the legacy 'reason' key under the strict schema", () => { + expect(() => + McpStepOutcomeSchema.parse({ + ...base, + status: 'awaiting-input', + reason: 'needs-oauth-reauth', + }), + ).toThrow(); + }); +}); From 37dbf8d93348bc5ccabda21a7671695f307482fa Mon Sep 17 00:00:00 2001 From: Brian Fox Date: Tue, 16 Jun 2026 16:37:35 +0200 Subject: [PATCH 02/19] fix(workflow-executor): write rotated refresh token to the current row On the invalid_grant concurrent-rotation retry path the write-back used the pre-retry credential for the non-token fields; thread the credential whose token produced the grant through so a concurrent re-deposit is not partially reverted. Addresses review on #1665. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../src/oauth/token-service.ts | 23 +++++++++++----- .../test/oauth/token-service.test.ts | 27 +++++++++++++++++++ 2 files changed, 43 insertions(+), 7 deletions(-) diff --git a/packages/workflow-executor/src/oauth/token-service.ts b/packages/workflow-executor/src/oauth/token-service.ts index ac1b6de5fb..6c40555430 100644 --- a/packages/workflow-executor/src/oauth/token-service.ts +++ b/packages/workflow-executor/src/oauth/token-service.ts @@ -84,7 +84,11 @@ export default class OAuthTokenService { const credential = await this.store.get(userId, mcpServerId); if (!credential) throw new OAuthReauthRequiredError(mcpServerId); - const result = await this.runGrantWithRotationRetry(credential, userId, mcpServerId); + const { result, credential: grantedCredential } = await this.runGrantWithRotationRetry( + credential, + userId, + mcpServerId, + ); this.cache.set(key, { accessToken: result.accessToken, @@ -93,22 +97,25 @@ export default class OAuthTokenService { }); if (result.refreshToken) { - await this.persistRotatedRefreshToken(credential, result.refreshToken); + await this.persistRotatedRefreshToken(grantedCredential, result.refreshToken); } return result.accessToken; } // On invalid_grant a peer instance likely rotated the refresh token out from under us: re-read the - // row and retry once with the current token. A second invalid_grant — or an unchanged token, which - // means no peer rotated it — is a genuine revocation and forces re-auth. + // row and retry once with the current token; a second invalid_grant (or an unchanged token) is a + // genuine revocation and forces re-auth. Returns the credential whose token produced the grant so + // the caller writes the rotated token back onto that (current) row, not the stale one. private async runGrantWithRotationRetry( credential: StoredMcpOAuthCredential, userId: number, mcpServerId: string, - ): Promise { + ): Promise<{ result: RefreshGrantResult; credential: StoredMcpOAuthCredential }> { try { - return await this.refreshAccessToken(this.toGrantParams(credential)); + const result = await this.refreshAccessToken(this.toGrantParams(credential)); + + return { result, credential }; } catch (error) { if (!(error instanceof OAuthInvalidGrantError)) throw error; @@ -123,7 +130,9 @@ export default class OAuthTokenService { } try { - return await this.refreshAccessToken(this.toGrantParams(latest)); + const result = await this.refreshAccessToken(this.toGrantParams(latest)); + + return { result, credential: latest }; } catch (retryError) { if (retryError instanceof OAuthInvalidGrantError) { throw new OAuthReauthRequiredError(mcpServerId); diff --git a/packages/workflow-executor/test/oauth/token-service.test.ts b/packages/workflow-executor/test/oauth/token-service.test.ts index a3c7ef6709..1525490fe7 100644 --- a/packages/workflow-executor/test/oauth/token-service.test.ts +++ b/packages/workflow-executor/test/oauth/token-service.test.ts @@ -247,6 +247,33 @@ describe('OAuthTokenService.getAccessToken', () => { ); }); + it('persists the rotated token onto the re-read credential fields, not the stale ones', async () => { + const original = makeCredential({ scopes: 'a b' }); + const latest = makeCredential({ + refreshTokenEnc: Buffer.from('enc-rt-rotated'), + scopes: 'a b c', + }); + const get = jest.fn().mockResolvedValueOnce(original).mockResolvedValueOnce(latest); + const upsert = jest.fn().mockResolvedValue(undefined); + const refresh = jest + .fn() + .mockRejectedValueOnce(new OAuthInvalidGrantError()) + .mockResolvedValueOnce({ accessToken: 'at', expiresInS: 3600, refreshToken: 'rt-3' }); + + const service = new OAuthTokenService({ + store: { get, upsert } as unknown as McpOAuthCredentialsStore, + encryption: { + decrypt: (buf: Buffer) => buf.toString(), + encrypt: () => ({ ciphertext: Buffer.from('enc:rt-3'), encKeyVersion: 2 }), + } as unknown as CredentialEncryption, + refreshAccessToken: refresh, + }); + + await service.getAccessToken(USER_ID, SERVER_ID); + + expect(upsert).toHaveBeenCalledWith(expect.objectContaining({ scopes: 'a b c' })); + }); + it('raises OAuthReauthRequiredError when the re-read shows the same (unrotated) token', async () => { const refresh = jest.fn().mockRejectedValue(new OAuthInvalidGrantError()); const { service } = setup({ refresh }); From 2dce276bae781038a21ad9d7ec4d8b0f8d923b62 Mon Sep 17 00:00:00 2001 From: Brian Fox Date: Tue, 16 Jun 2026 16:37:37 +0200 Subject: [PATCH 03/19] fix(ai-proxy): treat only 401 (not 403) as a refreshable MCP auth error A 403 is a permission/scope failure that a token refresh or re-consent cannot resolve, so it no longer triggers the refresh + re-auth flow (which looped) and instead surfaces as an ordinary failure. The spec specifies retry on 401. Addresses review on #1665. Co-Authored-By: Claude Opus 4.8 (1M context) --- packages/ai-proxy/src/mcp-auth-error.ts | 8 +++++--- packages/ai-proxy/test/mcp-auth-error.test.ts | 18 ++++++++---------- 2 files changed, 13 insertions(+), 13 deletions(-) diff --git a/packages/ai-proxy/src/mcp-auth-error.ts b/packages/ai-proxy/src/mcp-auth-error.ts index ea39b476e3..c53a05dd82 100644 --- a/packages/ai-proxy/src/mcp-auth-error.ts +++ b/packages/ai-proxy/src/mcp-auth-error.ts @@ -1,8 +1,10 @@ -// Classifies errors surfaced while connecting to or calling an MCP server. The MCP SDK / HTTP +// Classifies errors surfaced while connecting to or calling an MCP server. Only 401 (the token was +// rejected) is a refreshable auth failure; 403 is a permission/scope problem a token refresh or +// re-consent cannot resolve, so it is left to surface as an ordinary failure. The MCP SDK / HTTP // transport reports failures in several shapes (a numeric status field, or only a message string), // so the checks walk the cause chain and inspect both structured status and the message text. -const AUTH_STATUSES = new Set([401, 403]); -const AUTH_PATTERN = /\b40[13]\b|unauthorized|forbidden/i; +const AUTH_STATUSES = new Set([401]); +const AUTH_PATTERN = /\b401\b|unauthorized/i; const CONNECTION_PATTERN = /econnrefused|econnreset|etimedout|enotfound|eai_again|fetch failed|network|socket|timeout|connect/i; diff --git a/packages/ai-proxy/test/mcp-auth-error.test.ts b/packages/ai-proxy/test/mcp-auth-error.test.ts index c55ae57dd4..0d79760134 100644 --- a/packages/ai-proxy/test/mcp-auth-error.test.ts +++ b/packages/ai-proxy/test/mcp-auth-error.test.ts @@ -12,14 +12,9 @@ describe('isMcpAuthError', () => { expect(isMcpAuthError({ code: 401 })).toBe(true); }); - it('detects a 403 numeric status field', () => { - expect(isMcpAuthError(Object.assign(new Error('denied'), { status: 403 }))).toBe(true); - }); - - it('detects 401 / unauthorized / forbidden in the message', () => { + it('detects 401 / unauthorized in the message', () => { expect(isMcpAuthError(new Error('Request failed with status code 401'))).toBe(true); expect(isMcpAuthError(new Error('Unauthorized'))).toBe(true); - expect(isMcpAuthError(new Error('403 Forbidden'))).toBe(true); }); it('walks the cause chain', () => { @@ -28,7 +23,9 @@ describe('isMcpAuthError', () => { ).toBe(true); }); - it('returns false for non-auth errors and nullish input', () => { + it('returns false for 403 (forbidden), non-auth errors, and nullish input', () => { + expect(isMcpAuthError(Object.assign(new Error('denied'), { status: 403 }))).toBe(false); + expect(isMcpAuthError(new Error('403 Forbidden'))).toBe(false); expect(isMcpAuthError(new Error('ECONNREFUSED'))).toBe(false); expect(isMcpAuthError(new Error('500 Internal Server Error'))).toBe(false); expect(isMcpAuthError(undefined)).toBe(false); @@ -36,8 +33,8 @@ describe('isMcpAuthError', () => { }); describe('classifyMcpLoadError', () => { - it("classifies auth failures as 'auth'", () => { - expect(classifyMcpLoadError(new Error('HTTP 403 Forbidden'))).toBe('auth'); + it("classifies a 401 as 'auth'", () => { + expect(classifyMcpLoadError(new Error('HTTP 401 Unauthorized'))).toBe('auth'); expect(classifyMcpLoadError({ status: 401 })).toBe('auth'); }); @@ -49,7 +46,8 @@ describe('classifyMcpLoadError', () => { expect(classifyMcpLoadError(new Error('socket hang up'))).toBe('connection'); }); - it("classifies everything else as 'unknown'", () => { + it("classifies a 403 (forbidden) and anything else as 'unknown'", () => { + expect(classifyMcpLoadError(new Error('HTTP 403 Forbidden'))).toBe('unknown'); expect(classifyMcpLoadError(new Error('tool schema invalid'))).toBe('unknown'); }); }); From b345697c52a07f4a304ed7fa12a362de8a0c828c Mon Sep 17 00:00:00 2001 From: Brian Fox Date: Tue, 16 Jun 2026 18:22:43 +0200 Subject: [PATCH 04/19] feat(workflow-executor): support OAuth MCP steps in the in-memory executor [PRD-367] PR1 wired an in-memory credential store + deposit endpoint into buildInMemoryExecutor, so the previous "in-memory raises ConfigurationError for oauth2 steps" behavior was inconsistent: a credential could be deposited but never used. Wire an OAuthTokenService into the in-memory runner (sharing the same store instance the deposit endpoint writes to) so oauth2 steps work end-to-end in dev, matching the database executor. The token service is now a required RunnerConfig/RemoteToolFetcher collaborator (both executors provide it), so the unreachable ConfigurationError guard and its fetcher test are removed. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../src/build-workflow-executor.ts | 16 ++++++++++++++-- .../src/remote-tool-fetcher.ts | 13 +++---------- packages/workflow-executor/src/runner.ts | 6 +++--- .../test/build-workflow-executor.test.ts | 9 +++++++++ .../integration/workflow-execution.test.ts | 14 ++++++++++++-- .../test/remote-tool-fetcher.test.ts | 19 +++++++------------ .../workflow-executor/test/runner.test.ts | 4 ++++ 7 files changed, 52 insertions(+), 29 deletions(-) diff --git a/packages/workflow-executor/src/build-workflow-executor.ts b/packages/workflow-executor/src/build-workflow-executor.ts index adfe7e4121..5a2a81f49d 100644 --- a/packages/workflow-executor/src/build-workflow-executor.ts +++ b/packages/workflow-executor/src/build-workflow-executor.ts @@ -223,9 +223,21 @@ function createWorkflowExecutor( export function buildInMemoryExecutor(options: ExecutorOptions): WorkflowExecutor { const deps = buildCommonDependencies(options); + const mcpOAuthCredentialsStore = new InMemoryMcpOAuthCredentialsStore(); + const credentialEncryption = new CredentialEncryption(); + // Shares the store + encryption with the deposit endpoint so runtime reads and writes (rotation) + // go through the same instance the HTTP server exposes. In-memory is dev-only: credentials live + // only for the process lifetime, but oauth2 steps work end-to-end just like the database executor. + const mcpOAuthTokenService = new OAuthTokenService({ + store: mcpOAuthCredentialsStore, + encryption: credentialEncryption, + logger: deps.logger, + }); + const runner = new Runner({ ...deps, runStore: new InMemoryStore(), + mcpOAuthTokenService, }); const server = new ExecutorHttpServer({ @@ -234,8 +246,8 @@ export function buildInMemoryExecutor(options: ExecutorOptions): WorkflowExecuto authSecret: options.authSecret, workflowPort: deps.workflowPort, logger: deps.logger, - mcpOAuthCredentialsStore: new InMemoryMcpOAuthCredentialsStore(), - credentialEncryption: new CredentialEncryption(), + mcpOAuthCredentialsStore, + credentialEncryption, }); return createWorkflowExecutor(runner, server, deps.logger); diff --git a/packages/workflow-executor/src/remote-tool-fetcher.ts b/packages/workflow-executor/src/remote-tool-fetcher.ts index c933d33720..0bd01c0527 100644 --- a/packages/workflow-executor/src/remote-tool-fetcher.ts +++ b/packages/workflow-executor/src/remote-tool-fetcher.ts @@ -6,7 +6,7 @@ import type { RemoteTool, ToolConfig } from '@forestadmin/ai-proxy'; import { injectOauthTokens } from '@forestadmin/ai-proxy'; -import { ConfigurationError, OAuthReauthRequiredError } from './errors'; +import { OAuthReauthRequiredError } from './errors'; const OAUTH2_AUTH_TYPE = 'oauth2'; @@ -35,13 +35,13 @@ export default class RemoteToolFetcher { private readonly workflowPort: WorkflowPort; private readonly aiModelPort: AiModelPort; private readonly logger: Logger; - private readonly oauthTokenService?: OAuthTokenService; + private readonly oauthTokenService: OAuthTokenService; constructor( workflowPort: WorkflowPort, aiModelPort: AiModelPort, logger: Logger, - oauthTokenService?: OAuthTokenService, + oauthTokenService: OAuthTokenService, ) { this.workflowPort = workflowPort; this.aiModelPort = aiModelPort; @@ -77,13 +77,6 @@ export default class RemoteToolFetcher { mcpServerId: string, userId: number, ): Promise { - if (!this.oauthTokenService) { - throw new ConfigurationError( - `OAuth-protected MCP server "${mcpServerId}" requires the database-backed executor ` + - `(no credential store is configured)`, - ); - } - const tokenService = this.oauthTokenService; const attemptLoad = async ( diff --git a/packages/workflow-executor/src/runner.ts b/packages/workflow-executor/src/runner.ts index 0d1b13cc30..0b79f7823d 100644 --- a/packages/workflow-executor/src/runner.ts +++ b/packages/workflow-executor/src/runner.ts @@ -51,9 +51,9 @@ export interface RunnerConfig { // Max number of ADDITIONAL steps auto-chained via /update-step response before yielding to the // next poll cycle (counted after the initial step). 0 disables chaining entirely. Default 50. maxChainDepth?: number; - // Wired only by the database-backed executor; absent in-memory, where the fetcher raises a - // ConfigurationError for oauth2 steps. - mcpOAuthTokenService?: OAuthTokenService; + // Per-user OAuth access-token service for oauth2 MCP steps. Wired by both the in-memory and + // database executors, sharing the credential store the HTTP deposit endpoint writes to. + mcpOAuthTokenService: OAuthTokenService; } // eslint-disable-next-line @typescript-eslint/no-var-requires, import/no-dynamic-require, global-require diff --git a/packages/workflow-executor/test/build-workflow-executor.test.ts b/packages/workflow-executor/test/build-workflow-executor.test.ts index 0efc4697f6..60bd8f7edc 100644 --- a/packages/workflow-executor/test/build-workflow-executor.test.ts +++ b/packages/workflow-executor/test/build-workflow-executor.test.ts @@ -3,6 +3,7 @@ import { buildDatabaseExecutor, buildInMemoryExecutor } from '../src/build-workf import CredentialEncryption from '../src/crypto/credential-encryption'; import { DEFAULT_SCHEMA_CACHE_TTL_S } from '../src/defaults'; import ExecutorHttpServer from '../src/http/executor-http-server'; +import OAuthTokenService from '../src/oauth/token-service'; import Runner from '../src/runner'; import SchemaCache from '../src/schema-cache'; import DatabaseMcpOAuthCredentialsStore from '../src/stores/database-mcp-oauth-credentials-store'; @@ -71,6 +72,14 @@ describe('buildInMemoryExecutor', () => { ); }); + it('wires an OAuth token service into the in-memory runner', () => { + buildInMemoryExecutor(BASE_OPTIONS); + + expect(MockedRunner).toHaveBeenCalledWith( + expect.objectContaining({ mcpOAuthTokenService: expect.any(OAuthTokenService) }), + ); + }); + it('creates ForestServerWorkflowPort with default forestServerUrl', () => { buildInMemoryExecutor(BASE_OPTIONS); diff --git a/packages/workflow-executor/test/integration/workflow-execution.test.ts b/packages/workflow-executor/test/integration/workflow-execution.test.ts index bd637b5410..dffdf619f7 100644 --- a/packages/workflow-executor/test/integration/workflow-execution.test.ts +++ b/packages/workflow-executor/test/integration/workflow-execution.test.ts @@ -9,8 +9,10 @@ import jsonwebtoken from 'jsonwebtoken'; import request from 'supertest'; import { z } from 'zod'; +import createConsoleLogger from '../../src/adapters/console-logger'; import CredentialEncryption from '../../src/crypto/credential-encryption'; import ExecutorHttpServer from '../../src/http/executor-http-server'; +import OAuthTokenService from '../../src/oauth/token-service'; import Runner from '../../src/runner'; import SchemaCache from '../../src/schema-cache'; import InMemoryMcpOAuthCredentialsStore from '../../src/stores/in-memory-mcp-oauth-credentials-store'; @@ -194,6 +196,13 @@ function createIntegrationSetup(overrides?: { const agentPort = overrides?.agentPort ?? createMockAgentPort(); const runStore = new InMemoryStore(); const schemaCache = new SchemaCache(); + const mcpOAuthCredentialsStore = new InMemoryMcpOAuthCredentialsStore(); + const credentialEncryption = new CredentialEncryption(); + const mcpOAuthTokenService = new OAuthTokenService({ + store: mcpOAuthCredentialsStore, + encryption: credentialEncryption, + logger: createConsoleLogger(), + }); const runner = new Runner({ agentPort, @@ -212,6 +221,7 @@ function createIntegrationSetup(overrides?: { pollingIntervalS: overrides?.pollingIntervalS ?? 60, envSecret: ENV_SECRET, authSecret: AUTH_SECRET, + mcpOAuthTokenService, }); const server = new ExecutorHttpServer({ @@ -219,8 +229,8 @@ function createIntegrationSetup(overrides?: { runner, authSecret: AUTH_SECRET, workflowPort, - mcpOAuthCredentialsStore: new InMemoryMcpOAuthCredentialsStore(), - credentialEncryption: new CredentialEncryption(), + mcpOAuthCredentialsStore, + credentialEncryption, }); return { runner, server, workflowPort, agentPort, runStore, aiClient, model }; diff --git a/packages/workflow-executor/test/remote-tool-fetcher.test.ts b/packages/workflow-executor/test/remote-tool-fetcher.test.ts index 4cb74df672..67c1635043 100644 --- a/packages/workflow-executor/test/remote-tool-fetcher.test.ts +++ b/packages/workflow-executor/test/remote-tool-fetcher.test.ts @@ -4,7 +4,7 @@ import type { Logger } from '../src/ports/logger-port'; import type { WorkflowPort } from '../src/ports/workflow-port'; import type { RemoteTool, ToolConfig } from '@forestadmin/ai-proxy'; -import { ConfigurationError, OAuthReauthRequiredError } from '../src/errors'; +import { OAuthReauthRequiredError } from '../src/errors'; import RemoteToolFetcher, { scopeConfigsToServer } from '../src/remote-tool-fetcher'; const USER_ID = 1; @@ -39,11 +39,16 @@ function makeFetcher(overrides?: { const workflowPort = { ...createMockWorkflowPort(), ...overrides?.workflowPort }; const aiModelPort = { ...createMockAiModelPort(), ...overrides?.aiModelPort }; const logger = overrides?.logger ?? createMockLogger(); + const tokenService = + overrides?.tokenService ?? + ({ + getAccessToken: jest.fn().mockResolvedValue('access-token'), + } as unknown as OAuthTokenService); const fetcher = new RemoteToolFetcher( workflowPort as unknown as WorkflowPort, aiModelPort as unknown as AiModelPort, logger, - overrides?.tokenService, + tokenService, ); return { fetcher, workflowPort, aiModelPort, logger }; @@ -336,16 +341,6 @@ describe('RemoteToolFetcher.fetch — OAuth2 servers', () => { expect(typeof result.reloadWithFreshAuth).toBe('function'); }); - it('throws ConfigurationError when no token service is wired (in-memory executor)', async () => { - const { fetcher } = makeFetcher({ - workflowPort: { - getMcpServerConfigs: jest.fn().mockResolvedValue({ 'srv-a': oauthCfg('id-A') }), - }, - }); - - await expect(fetcher.fetch('id-A', USER_ID)).rejects.toBeInstanceOf(ConfigurationError); - }); - it('force-refreshes and retries list-tools once on an auth failure, then succeeds', async () => { const tool = makeRemoteTool('srv-a', 'id-A'); const getAccessToken = jest.fn().mockResolvedValue('tok'); diff --git a/packages/workflow-executor/test/runner.test.ts b/packages/workflow-executor/test/runner.test.ts index d8708935a3..bca43c0321 100644 --- a/packages/workflow-executor/test/runner.test.ts +++ b/packages/workflow-executor/test/runner.test.ts @@ -1,4 +1,5 @@ import type { StepContextConfig } from '../src/executors/step-executor-factory'; +import type OAuthTokenService from '../src/oauth/token-service'; import type { AgentPort } from '../src/ports/agent-port'; import type { AiModelPort } from '../src/ports/ai-model-port'; import type { Logger } from '../src/ports/logger-port'; @@ -120,6 +121,9 @@ function createRunnerConfig( schemaCache: new SchemaCache(), envSecret: VALID_ENV_SECRET, authSecret: VALID_AUTH_SECRET, + mcpOAuthTokenService: { + getAccessToken: jest.fn().mockResolvedValue('access-token'), + } as unknown as OAuthTokenService, ...overrides, }; } From b1de994f93320b6f7d3fbdef44fda953ed12687f Mon Sep 17 00:00:00 2001 From: Brian Fox Date: Tue, 16 Jun 2026 18:24:29 +0200 Subject: [PATCH 05/19] fix(workflow-executor): repoint token service at the relocated credentials port [PRD-367] The PR1 rebase moved the credentials store interface + types from stores/mcp-oauth-credentials-store to ports/mcp-oauth-credentials-store (the store file now holds only the Database/InMemory implementations). Import McpOAuthCredentialsStore and StoredMcpOAuthCredential from the new port path so the package compiles against the rebased PR1 base. Co-Authored-By: Claude Opus 4.8 (1M context) --- packages/workflow-executor/src/oauth/token-service.ts | 6 ++++-- packages/workflow-executor/test/oauth/token-service.test.ts | 6 ++++-- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/packages/workflow-executor/src/oauth/token-service.ts b/packages/workflow-executor/src/oauth/token-service.ts index 6c40555430..3e80e57198 100644 --- a/packages/workflow-executor/src/oauth/token-service.ts +++ b/packages/workflow-executor/src/oauth/token-service.ts @@ -1,8 +1,10 @@ import type { RefreshGrantParams, RefreshGrantResult } from './refresh-grant'; import type CredentialEncryption from '../crypto/credential-encryption'; import type { Logger } from '../ports/logger-port'; -import type { StoredMcpOAuthCredential } from '../stores/mcp-oauth-credentials-store'; -import type McpOAuthCredentialsStore from '../stores/mcp-oauth-credentials-store'; +import type { + McpOAuthCredentialsStore, + StoredMcpOAuthCredential, +} from '../ports/mcp-oauth-credentials-store'; import { DEFAULT_OAUTH_EXPIRY_SKEW_S } from '../defaults'; import { OAuthInvalidGrantError, OAuthReauthRequiredError } from '../errors'; diff --git a/packages/workflow-executor/test/oauth/token-service.test.ts b/packages/workflow-executor/test/oauth/token-service.test.ts index 1525490fe7..13f544a16f 100644 --- a/packages/workflow-executor/test/oauth/token-service.test.ts +++ b/packages/workflow-executor/test/oauth/token-service.test.ts @@ -1,7 +1,9 @@ import type CredentialEncryption from '../../src/crypto/credential-encryption'; import type { RefreshGrantParams, RefreshGrantResult } from '../../src/oauth/refresh-grant'; -import type McpOAuthCredentialsStore from '../../src/stores/mcp-oauth-credentials-store'; -import type { StoredMcpOAuthCredential } from '../../src/stores/mcp-oauth-credentials-store'; +import type { + McpOAuthCredentialsStore, + StoredMcpOAuthCredential, +} from '../../src/ports/mcp-oauth-credentials-store'; import { OAuthInvalidGrantError, From f50f18e8aeec2d6b867be5ed58234f5b950a0e09 Mon Sep 17 00:00:00 2001 From: Brian Fox Date: Tue, 23 Jun 2026 17:09:46 +0200 Subject: [PATCH 06/19] feat(workflow-executor): re-consent on decrypt failure after key rotation [PRD-367] MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR1 dropped enc_key_version from the credential store (no version-aware decrypt path), so the rotation write-back no longer carries encKeyVersion. Per PRD-367 key-rotation handling, a decrypt failure with the encryption key PRESENT (auth-tag mismatch from a since-rotated/hard-swapped key) is recoverable: toGrantParams now classifies it as OAuthReauthRequiredError (needs-oauth-reauth) so re-consent re-deposits under the new key. A missing key (ExecutorEncryptionKeyMissingError) stays terminal — re-consent cannot help and a re-deposit would 503. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../src/oauth/token-service.ts | 38 +++++++++----- .../test/oauth/token-service.test.ts | 51 +++++++++++++++++-- 2 files changed, 73 insertions(+), 16 deletions(-) diff --git a/packages/workflow-executor/src/oauth/token-service.ts b/packages/workflow-executor/src/oauth/token-service.ts index 3e80e57198..0f1815858c 100644 --- a/packages/workflow-executor/src/oauth/token-service.ts +++ b/packages/workflow-executor/src/oauth/token-service.ts @@ -7,7 +7,11 @@ import type { } from '../ports/mcp-oauth-credentials-store'; import { DEFAULT_OAUTH_EXPIRY_SKEW_S } from '../defaults'; -import { OAuthInvalidGrantError, OAuthReauthRequiredError } from '../errors'; +import { + ExecutorEncryptionKeyMissingError, + OAuthInvalidGrantError, + OAuthReauthRequiredError, +} from '../errors'; import KeyedMutex from './keyed-mutex'; import defaultRefreshAccessToken from './refresh-grant'; @@ -145,17 +149,28 @@ export default class OAuthTokenService { } } + // Decrypt happens here. A decrypt failure with the key PRESENT (auth-tag mismatch — the row was + // encrypted under a since-rotated/hard-swapped key, or is corrupt) is recoverable: re-consent + // re-deposits under the current key, so surface it as needs-oauth-reauth. A missing key + // (ExecutorEncryptionKeyMissingError) is an operator misconfig, not re-consent-resolvable, so it + // propagates as a terminal error (a re-deposit would just 503 at the deposit endpoint). private toGrantParams(credential: StoredMcpOAuthCredential): RefreshGrantParams { - return { - tokenEndpoint: credential.tokenEndpoint, - refreshToken: this.encryption.decrypt(credential.refreshTokenEnc), - clientId: credential.clientId, - clientSecret: credential.clientSecretEnc - ? this.encryption.decrypt(credential.clientSecretEnc) - : null, - tokenEndpointAuthMethod: credential.tokenEndpointAuthMethod, - scopes: credential.scopes, - }; + try { + return { + tokenEndpoint: credential.tokenEndpoint, + refreshToken: this.encryption.decrypt(credential.refreshTokenEnc), + clientId: credential.clientId, + clientSecret: credential.clientSecretEnc + ? this.encryption.decrypt(credential.clientSecretEnc) + : null, + tokenEndpointAuthMethod: credential.tokenEndpointAuthMethod, + scopes: credential.scopes, + }; + } catch (error) { + if (error instanceof ExecutorEncryptionKeyMissingError) throw error; + + throw new OAuthReauthRequiredError(credential.mcpServerId); + } } private async persistRotatedRefreshToken( @@ -175,7 +190,6 @@ export default class OAuthTokenService { tokenEndpoint: credential.tokenEndpoint, tokenEndpointAuthMethod: credential.tokenEndpointAuthMethod, scopes: credential.scopes, - encKeyVersion: encrypted.encKeyVersion, }); } catch (error) { // A failed write-back is not fatal: the access token just obtained is valid. The next refresh diff --git a/packages/workflow-executor/test/oauth/token-service.test.ts b/packages/workflow-executor/test/oauth/token-service.test.ts index 13f544a16f..5104065761 100644 --- a/packages/workflow-executor/test/oauth/token-service.test.ts +++ b/packages/workflow-executor/test/oauth/token-service.test.ts @@ -6,6 +6,7 @@ import type { } from '../../src/ports/mcp-oauth-credentials-store'; import { + ExecutorEncryptionKeyMissingError, OAuthInvalidGrantError, OAuthReauthRequiredError, OAuthRefreshError, @@ -29,7 +30,6 @@ function makeCredential( tokenEndpoint: 'https://idp/token', tokenEndpointAuthMethod: 'client_secret_basic', scopes: 'a b', - encKeyVersion: 1, ...overrides, }; } @@ -47,7 +47,6 @@ function setup(options?: { const decrypt = jest.fn((buf: Buffer) => `decrypted:${buf.toString()}`); const encrypt = jest.fn((plain: string) => ({ ciphertext: Buffer.from(`enc:${plain}`), - encKeyVersion: 2, })); const encryption = { decrypt, encrypt } as unknown as CredentialEncryption; @@ -198,7 +197,6 @@ describe('OAuthTokenService.getAccessToken', () => { userId: USER_ID, mcpServerId: SERVER_ID, refreshTokenEnc: Buffer.from('enc:rt-2'), - encKeyVersion: 2, }), ); }); @@ -266,7 +264,7 @@ describe('OAuthTokenService.getAccessToken', () => { store: { get, upsert } as unknown as McpOAuthCredentialsStore, encryption: { decrypt: (buf: Buffer) => buf.toString(), - encrypt: () => ({ ciphertext: Buffer.from('enc:rt-3'), encKeyVersion: 2 }), + encrypt: () => ({ ciphertext: Buffer.from('enc:rt-3') }), } as unknown as CredentialEncryption, refreshAccessToken: refresh, }); @@ -326,4 +324,49 @@ describe('OAuthTokenService.getAccessToken', () => { expect(refresh).toHaveBeenCalledTimes(1); }); }); + + describe('decrypt failure classification', () => { + it('surfaces a decrypt failure with the key present as needs-oauth-reauth (recoverable)', async () => { + const service = new OAuthTokenService({ + store: { + get: jest.fn().mockResolvedValue(makeCredential()), + upsert: jest.fn(), + } as unknown as McpOAuthCredentialsStore, + encryption: { + // A since-rotated/hard-swapped key fails GCM auth-tag verification with a generic error. + decrypt: () => { + throw new Error('Unsupported state or unable to authenticate data'); + }, + encrypt: jest.fn(), + } as unknown as CredentialEncryption, + refreshAccessToken: jest.fn(), + }); + + await expect(service.getAccessToken(USER_ID, SERVER_ID)).rejects.toBeInstanceOf( + OAuthReauthRequiredError, + ); + }); + + it('rethrows a missing-key error as terminal, never reaching the grant', async () => { + const refreshAccessToken = jest.fn(); + const service = new OAuthTokenService({ + store: { + get: jest.fn().mockResolvedValue(makeCredential()), + upsert: jest.fn(), + } as unknown as McpOAuthCredentialsStore, + encryption: { + decrypt: () => { + throw new ExecutorEncryptionKeyMissingError(); + }, + encrypt: jest.fn(), + } as unknown as CredentialEncryption, + refreshAccessToken, + }); + + await expect(service.getAccessToken(USER_ID, SERVER_ID)).rejects.toBeInstanceOf( + ExecutorEncryptionKeyMissingError, + ); + expect(refreshAccessToken).not.toHaveBeenCalled(); + }); + }); }); From c121cb653dff9ee090edf1150e81f6f48ad9ab8a Mon Sep 17 00:00:00 2001 From: Brian Fox Date: Tue, 23 Jun 2026 17:09:48 +0200 Subject: [PATCH 07/19] feat(workflow-executor): add GET /list-mcp-tools design-time tool listing [PRD-367] MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit New GET /list-mcp-tools?mcpServerId= route on the executor HTTP server for the orchestrator-engine MCP-server details page: resolves the caller's vault credential (user_id from the validated JWT, never the request), refreshes, injects the Bearer, and returns the server's tool definitions — reusing RemoteToolFetcher, no new fetch/refresh logic. A missing/unrefreshable credential returns a typed needs-oauth-reauth (409), not a generic error or empty list. Wired into both the database and in-memory executors so oauth2 tool listing works in dev too. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../src/build-workflow-executor.ts | 17 +++++ .../src/http/executor-http-server.ts | 51 ++++++++++++- .../test/http/executor-http-server.test.ts | 10 +++ .../http/mcp-oauth-credentials-route.test.ts | 75 ++++++++++++++++++- .../integration/workflow-execution.test.ts | 9 +++ 5 files changed, 159 insertions(+), 3 deletions(-) diff --git a/packages/workflow-executor/src/build-workflow-executor.ts b/packages/workflow-executor/src/build-workflow-executor.ts index 5a2a81f49d..3ff0f086a0 100644 --- a/packages/workflow-executor/src/build-workflow-executor.ts +++ b/packages/workflow-executor/src/build-workflow-executor.ts @@ -26,6 +26,7 @@ import { } from './defaults'; import ExecutorHttpServer from './http/executor-http-server'; import OAuthTokenService from './oauth/token-service'; +import RemoteToolFetcher from './remote-tool-fetcher'; import Runner from './runner'; import SchemaCache from './schema-cache'; import DatabaseMcpOAuthCredentialsStore from './stores/database-mcp-oauth-credentials-store'; @@ -234,6 +235,13 @@ export function buildInMemoryExecutor(options: ExecutorOptions): WorkflowExecuto logger: deps.logger, }); + const remoteToolFetcher = new RemoteToolFetcher( + deps.workflowPort, + deps.aiModelPort, + deps.logger, + mcpOAuthTokenService, + ); + const runner = new Runner({ ...deps, runStore: new InMemoryStore(), @@ -248,6 +256,7 @@ export function buildInMemoryExecutor(options: ExecutorOptions): WorkflowExecuto logger: deps.logger, mcpOAuthCredentialsStore, credentialEncryption, + remoteToolFetcher, }); return createWorkflowExecutor(runner, server, deps.logger); @@ -278,6 +287,13 @@ export function buildDatabaseExecutor(options: DatabaseExecutorOptions): Workflo logger: deps.logger, }); + const remoteToolFetcher = new RemoteToolFetcher( + deps.workflowPort, + deps.aiModelPort, + deps.logger, + mcpOAuthTokenService, + ); + const runner = new Runner({ ...deps, runStore: new DatabaseStore({ sequelize, schema: mergedOptions.schema }), @@ -292,6 +308,7 @@ export function buildDatabaseExecutor(options: DatabaseExecutorOptions): Workflo logger: deps.logger, mcpOAuthCredentialsStore, credentialEncryption, + remoteToolFetcher, }); return createWorkflowExecutor(runner, server, deps.logger); diff --git a/packages/workflow-executor/src/http/executor-http-server.ts b/packages/workflow-executor/src/http/executor-http-server.ts index e59ee00888..ce313b419d 100644 --- a/packages/workflow-executor/src/http/executor-http-server.ts +++ b/packages/workflow-executor/src/http/executor-http-server.ts @@ -2,6 +2,7 @@ import type CredentialEncryption from '../crypto/credential-encryption'; import type { Logger } from '../ports/logger-port'; import type { McpOAuthCredentialsStore } from '../ports/mcp-oauth-credentials-store'; import type { WorkflowPort } from '../ports/workflow-port'; +import type RemoteToolFetcher from '../remote-tool-fetcher'; import type Runner from '../runner'; import type { Server } from 'http'; @@ -25,7 +26,11 @@ import { } from './mcp-oauth-credentials'; import serializeStepForWire from './step-serializer'; import createConsoleLogger from '../adapters/console-logger'; -import { ExecutorEncryptionKeyMissingError, extractErrorMessage } from '../errors'; +import { + ExecutorEncryptionKeyMissingError, + OAuthReauthRequiredError, + extractErrorMessage, +} from '../errors'; // eslint-disable-next-line @typescript-eslint/no-var-requires, import/no-dynamic-require, global-require const { version } = require('../../package.json') as { version: string }; @@ -38,6 +43,7 @@ export interface ExecutorHttpServerOptions { logger?: Logger; mcpOAuthCredentialsStore: McpOAuthCredentialsStore; credentialEncryption: CredentialEncryption; + remoteToolFetcher: RemoteToolFetcher; } export default class ExecutorHttpServer { @@ -153,7 +159,15 @@ export default class ExecutorHttpServer { ); router.post('/runs/:runId/trigger', this.handleTrigger.bind(this)); - const { mcpOAuthCredentialsStore: credentialsStore, credentialEncryption } = this.options; + const { + mcpOAuthCredentialsStore: credentialsStore, + credentialEncryption, + remoteToolFetcher, + } = this.options; + + // Design-time tool listing for the MCP-server details page: resolve the caller's vault + // credential, refresh, and list the oauth2 server's tools — no workflow run involved. + router.get('/list-mcp-tools', ctx => this.handleListMcpTools(ctx, remoteToolFetcher)); router.post('/mcp-oauth-credentials', ctx => this.handleDepositCredentials(ctx, credentialsStore, credentialEncryption), @@ -285,4 +299,37 @@ export default class ExecutorHttpServer { await store.delete(userId, ctx.params.mcpServerId); ctx.status = 204; } + + // Design-time tool listing: resolve the caller's vault credential for the target oauth2 server, + // refresh + inject the Bearer, and return the server's tool definitions. user_id comes from the + // validated JWT, so a caller only ever lists with their own stored credential. A missing/dead + // credential surfaces as a typed needs-oauth-reauth response (not a generic error or empty list) + // so the details page can prompt a reconnect — mirroring the run path's awaiting-input reason. + private async handleListMcpTools(ctx: Koa.Context, fetcher: RemoteToolFetcher): Promise { + const userId = (ctx.state.user as BearerClaims).id; + const { mcpServerId } = ctx.query; + + if (typeof mcpServerId !== 'string' || mcpServerId.length === 0) { + throw new BadRequestHttpError('Missing required query parameter "mcpServerId"'); + } + + try { + const { tools } = await fetcher.fetch(mcpServerId, userId); + ctx.status = 200; + ctx.body = { + tools: tools.map(tool => ({ name: tool.base.name, description: tool.base.description })), + }; + } catch (err) { + // Set the body directly so the error middleware (which would map this to a generic 400 and + // drop the typed reason) doesn't touch it — the frontend needs the reason to prompt reconnect. + if (err instanceof OAuthReauthRequiredError) { + ctx.status = 409; + ctx.body = { awaitingInputReason: err.awaitingInputReason, mcpServerId }; + + return; + } + + throw err; + } + } } diff --git a/packages/workflow-executor/test/http/executor-http-server.test.ts b/packages/workflow-executor/test/http/executor-http-server.test.ts index 01dcae2f31..f5a9767fbf 100644 --- a/packages/workflow-executor/test/http/executor-http-server.test.ts +++ b/packages/workflow-executor/test/http/executor-http-server.test.ts @@ -1,6 +1,7 @@ import type { Logger } from '../../src/ports/logger-port'; import type { McpOAuthCredentialsStore } from '../../src/ports/mcp-oauth-credentials-store'; import type { WorkflowPort } from '../../src/ports/workflow-port'; +import type RemoteToolFetcher from '../../src/remote-tool-fetcher'; import type Runner from '../../src/runner'; import jsonwebtoken from 'jsonwebtoken'; @@ -48,6 +49,12 @@ function createMockWorkflowPort(overrides: Partial = {}): Workflow } as unknown as WorkflowPort; } +function createMockFetcher(): RemoteToolFetcher { + return { + fetch: jest.fn().mockResolvedValue({ tools: [], mcpServerName: undefined }), + } as unknown as RemoteToolFetcher; +} + function createServer( overrides: { runner?: Runner; @@ -55,6 +62,7 @@ function createServer( logger?: jest.MockedFunction; mcpOAuthCredentialsStore?: McpOAuthCredentialsStore; credentialEncryption?: CredentialEncryption; + remoteToolFetcher?: RemoteToolFetcher; } = {}, ) { return new ExecutorHttpServer({ @@ -66,6 +74,7 @@ function createServer( mcpOAuthCredentialsStore: overrides.mcpOAuthCredentialsStore ?? new InMemoryMcpOAuthCredentialsStore(), credentialEncryption: overrides.credentialEncryption ?? new CredentialEncryption(), + remoteToolFetcher: overrides.remoteToolFetcher ?? createMockFetcher(), }); } @@ -82,6 +91,7 @@ describe('ExecutorHttpServer', () => { logger, mcpOAuthCredentialsStore: { init } as unknown as McpOAuthCredentialsStore, credentialEncryption: {} as unknown as CredentialEncryption, + remoteToolFetcher: createMockFetcher(), }); await server.start(); diff --git a/packages/workflow-executor/test/http/mcp-oauth-credentials-route.test.ts b/packages/workflow-executor/test/http/mcp-oauth-credentials-route.test.ts index b13a726a82..753b03a067 100644 --- a/packages/workflow-executor/test/http/mcp-oauth-credentials-route.test.ts +++ b/packages/workflow-executor/test/http/mcp-oauth-credentials-route.test.ts @@ -21,7 +21,7 @@ import jsonwebtoken from 'jsonwebtoken'; import request from 'supertest'; -import { ExecutorEncryptionKeyMissingError } from '../../src/errors'; +import { ExecutorEncryptionKeyMissingError, OAuthReauthRequiredError } from '../../src/errors'; import ExecutorHttpServer from '../../src/http/executor-http-server'; const AUTH_SECRET = 'test-auth-secret'; @@ -71,6 +71,10 @@ function createMockEncryption() { }; } +function createMockFetcher() { + return { fetch: jest.fn().mockResolvedValue({ tools: [], mcpServerName: undefined }) }; +} + function createServer(overrides: Record = {}) { return new ExecutorHttpServer({ port: 0, @@ -79,6 +83,7 @@ function createServer(overrides: Record = {}) { workflowPort: createMockWorkflowPort(), mcpOAuthCredentialsStore: createMockStore(), credentialEncryption: createMockEncryption(), + remoteToolFetcher: createMockFetcher(), ...overrides, } as never); } @@ -426,3 +431,71 @@ describe('DELETE /mcp-oauth-credentials/:mcpServerId', () => { expect(store.delete).not.toHaveBeenCalled(); }); }); + +describe('GET /list-mcp-tools', () => { + const toolDef = (name: string, description: string) => ({ base: { name, description } }); + + it('returns 401 when no token is provided', async () => { + const server = createServer(); + + const response = await request(server.callback).get('/list-mcp-tools?mcpServerId=mcp-server-1'); + + expect(response.status).toBe(401); + }); + + it('lists the tools for (token user, mcpServerId), resolving the vault credential', async () => { + const fetcher = { + fetch: jest.fn().mockResolvedValue({ + tools: [toolDef('search', 'Search things'), toolDef('create', 'Create a thing')], + mcpServerName: 'srv', + }), + }; + const server = createServer({ remoteToolFetcher: fetcher }); + const token = signToken({ id: 7 }); + + const response = await request(server.callback) + .get('/list-mcp-tools?mcpServerId=mcp-server-1') + .set('Authorization', `Bearer ${token}`); + + expect(response.status).toBe(200); + // user_id comes from the validated JWT (7), never the request. + expect(fetcher.fetch).toHaveBeenCalledWith('mcp-server-1', 7); + expect(response.body).toEqual({ + tools: [ + { name: 'search', description: 'Search things' }, + { name: 'create', description: 'Create a thing' }, + ], + }); + }); + + it('returns 400 when mcpServerId is missing', async () => { + const fetcher = { fetch: jest.fn() }; + const server = createServer({ remoteToolFetcher: fetcher }); + const token = signToken({ id: 7 }); + + const response = await request(server.callback) + .get('/list-mcp-tools') + .set('Authorization', `Bearer ${token}`); + + expect(response.status).toBe(400); + expect(fetcher.fetch).not.toHaveBeenCalled(); + }); + + it('returns a typed needs-oauth-reauth (409) when the credential is missing or unrefreshable', async () => { + const fetcher = { + fetch: jest.fn().mockRejectedValue(new OAuthReauthRequiredError('mcp-server-1')), + }; + const server = createServer({ remoteToolFetcher: fetcher }); + const token = signToken({ id: 7 }); + + const response = await request(server.callback) + .get('/list-mcp-tools?mcpServerId=mcp-server-1') + .set('Authorization', `Bearer ${token}`); + + expect(response.status).toBe(409); + expect(response.body).toEqual({ + awaitingInputReason: 'needs-oauth-reauth', + mcpServerId: 'mcp-server-1', + }); + }); +}); diff --git a/packages/workflow-executor/test/integration/workflow-execution.test.ts b/packages/workflow-executor/test/integration/workflow-execution.test.ts index dffdf619f7..14a76f55d6 100644 --- a/packages/workflow-executor/test/integration/workflow-execution.test.ts +++ b/packages/workflow-executor/test/integration/workflow-execution.test.ts @@ -13,6 +13,7 @@ import createConsoleLogger from '../../src/adapters/console-logger'; import CredentialEncryption from '../../src/crypto/credential-encryption'; import ExecutorHttpServer from '../../src/http/executor-http-server'; import OAuthTokenService from '../../src/oauth/token-service'; +import RemoteToolFetcher from '../../src/remote-tool-fetcher'; import Runner from '../../src/runner'; import SchemaCache from '../../src/schema-cache'; import InMemoryMcpOAuthCredentialsStore from '../../src/stores/in-memory-mcp-oauth-credentials-store'; @@ -224,6 +225,13 @@ function createIntegrationSetup(overrides?: { mcpOAuthTokenService, }); + const remoteToolFetcher = new RemoteToolFetcher( + workflowPort, + aiClient, + createConsoleLogger(), + mcpOAuthTokenService, + ); + const server = new ExecutorHttpServer({ port: 0, runner, @@ -231,6 +239,7 @@ function createIntegrationSetup(overrides?: { workflowPort, mcpOAuthCredentialsStore, credentialEncryption, + remoteToolFetcher, }); return { runner, server, workflowPort, agentPort, runStore, aiClient, model }; From f77ba06e9916e00d2a7696257def1fa611d6d486 Mon Sep 17 00:00:00 2001 From: Brian Fox Date: Tue, 23 Jun 2026 17:50:06 +0200 Subject: [PATCH 08/19] fix(workflow-executor): guard against non-object token-endpoint responses [PRD-367] A literal JSON null (or other non-object) body from the token endpoint overwrote the {} parse default, so the subsequent payload.error / payload.access_token reads threw a TypeError instead of the typed OAuthRefreshError. Keep the {} default for non-object bodies so the status checks still surface a typed OAuthRefreshError. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../workflow-executor/src/oauth/refresh-grant.ts | 5 ++++- .../test/oauth/refresh-grant.test.ts | 12 ++++++++++++ 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/packages/workflow-executor/src/oauth/refresh-grant.ts b/packages/workflow-executor/src/oauth/refresh-grant.ts index 6d1147a959..74fab606be 100644 --- a/packages/workflow-executor/src/oauth/refresh-grant.ts +++ b/packages/workflow-executor/src/oauth/refresh-grant.ts @@ -77,7 +77,10 @@ export default async function refreshAccessToken( let payload: TokenEndpointResponse = {}; try { - payload = (await response.json()) as TokenEndpointResponse; + const parsed: unknown = await response.json(); + // A non-object body (e.g. literal null) would make the property reads below throw a TypeError; + // keep the {} default so the status checks still surface a typed OAuthRefreshError. + if (parsed && typeof parsed === 'object') payload = parsed as TokenEndpointResponse; } catch { // Non-JSON body — handled by the status checks below. } diff --git a/packages/workflow-executor/test/oauth/refresh-grant.test.ts b/packages/workflow-executor/test/oauth/refresh-grant.test.ts index ff1a3876a1..766e83311a 100644 --- a/packages/workflow-executor/test/oauth/refresh-grant.test.ts +++ b/packages/workflow-executor/test/oauth/refresh-grant.test.ts @@ -49,6 +49,18 @@ describe('refreshAccessToken', () => { expect(body.get('refresh_token')).toBe('rt-1'); }); + it('throws OAuthRefreshError (not a TypeError) when the token endpoint body is literal null', async () => { + fetchSpy.mockResolvedValue({ + ok: false, + status: 500, + json: () => Promise.resolve(null), + } as unknown as Response); + + await expect( + refreshAccessToken({ tokenEndpoint: 'https://idp/token', refreshToken: 'rt-1' }), + ).rejects.toBeInstanceOf(OAuthRefreshError); + }); + it('sends client credentials via Basic auth for client_secret_basic (the default with a secret)', async () => { fetchSpy.mockResolvedValue( mockResponse({ ok: true, status: 200, payload: { access_token: 'at' } }), From b6f316e0375d4978fa41c4ff11b4edd82e317292 Mon Sep 17 00:00:00 2001 From: Brian Fox Date: Thu, 25 Jun 2026 19:05:26 +0200 Subject: [PATCH 09/19] fix(workflow-executor): evict cached access token on credential disconnect [PRD-367] MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit On DELETE /mcp-oauth-credentials/:mcpServerId, evict the in-process cached access token for (user, server) so a disconnect takes effect immediately — otherwise the executor keeps serving the cached token until it expires even though the credential row is gone (surfaced by end-to-end testing). Wires OAuthTokenService into both executor builders + the HTTP server (optional on the options so credential-free tests need not construct one), and adds OAuthTokenService.evict with coverage. Co-Authored-By: Claude Opus 4.8 --- .../src/build-workflow-executor.ts | 2 ++ .../src/http/executor-http-server.ts | 7 +++++++ .../workflow-executor/src/oauth/token-service.ts | 6 ++++++ .../http/mcp-oauth-credentials-route.test.ts | 16 ++++++++++++++++ .../test/oauth/token-service.test.ts | 13 +++++++++++++ 5 files changed, 44 insertions(+) diff --git a/packages/workflow-executor/src/build-workflow-executor.ts b/packages/workflow-executor/src/build-workflow-executor.ts index 3ff0f086a0..19ecff299e 100644 --- a/packages/workflow-executor/src/build-workflow-executor.ts +++ b/packages/workflow-executor/src/build-workflow-executor.ts @@ -257,6 +257,7 @@ export function buildInMemoryExecutor(options: ExecutorOptions): WorkflowExecuto mcpOAuthCredentialsStore, credentialEncryption, remoteToolFetcher, + oauthTokenService: mcpOAuthTokenService, }); return createWorkflowExecutor(runner, server, deps.logger); @@ -309,6 +310,7 @@ export function buildDatabaseExecutor(options: DatabaseExecutorOptions): Workflo mcpOAuthCredentialsStore, credentialEncryption, remoteToolFetcher, + oauthTokenService: mcpOAuthTokenService, }); return createWorkflowExecutor(runner, server, deps.logger); diff --git a/packages/workflow-executor/src/http/executor-http-server.ts b/packages/workflow-executor/src/http/executor-http-server.ts index ce313b419d..9db68d54e1 100644 --- a/packages/workflow-executor/src/http/executor-http-server.ts +++ b/packages/workflow-executor/src/http/executor-http-server.ts @@ -1,4 +1,5 @@ import type CredentialEncryption from '../crypto/credential-encryption'; +import type OAuthTokenService from '../oauth/token-service'; import type { Logger } from '../ports/logger-port'; import type { McpOAuthCredentialsStore } from '../ports/mcp-oauth-credentials-store'; import type { WorkflowPort } from '../ports/workflow-port'; @@ -44,6 +45,9 @@ export interface ExecutorHttpServerOptions { mcpOAuthCredentialsStore: McpOAuthCredentialsStore; credentialEncryption: CredentialEncryption; remoteToolFetcher: RemoteToolFetcher; + // The runtime always provides this (build-workflow-executor); optional so tests that don't + // exercise credential deletion don't all have to construct one. + oauthTokenService?: OAuthTokenService; } export default class ExecutorHttpServer { @@ -297,6 +301,9 @@ export default class ExecutorHttpServer { const userId = (ctx.state.user as BearerClaims).id; await store.delete(userId, ctx.params.mcpServerId); + // Evict any in-process cached access token so the disconnect is immediate, not deferred until + // the cached token expires (the row is gone, but the runtime would otherwise still serve it). + this.options.oauthTokenService?.evict(userId, ctx.params.mcpServerId); ctx.status = 204; } diff --git a/packages/workflow-executor/src/oauth/token-service.ts b/packages/workflow-executor/src/oauth/token-service.ts index 0f1815858c..c91d19d7ef 100644 --- a/packages/workflow-executor/src/oauth/token-service.ts +++ b/packages/workflow-executor/src/oauth/token-service.ts @@ -78,6 +78,12 @@ export default class OAuthTokenService { }); } + // Drop the cached access token for a (user, server). Called on credential delete so a disconnect + // takes effect immediately, instead of the executor serving the cached token until it expires. + evict(userId: number, mcpServerId: string): void { + this.cache.delete(`${userId}:${mcpServerId}`); + } + private readCache(key: string): string | undefined { const entry = this.cache.get(key); if (!entry || entry.expiresAtMs === undefined) return undefined; diff --git a/packages/workflow-executor/test/http/mcp-oauth-credentials-route.test.ts b/packages/workflow-executor/test/http/mcp-oauth-credentials-route.test.ts index 753b03a067..7688029490 100644 --- a/packages/workflow-executor/test/http/mcp-oauth-credentials-route.test.ts +++ b/packages/workflow-executor/test/http/mcp-oauth-credentials-route.test.ts @@ -75,6 +75,10 @@ function createMockFetcher() { return { fetch: jest.fn().mockResolvedValue({ tools: [], mcpServerName: undefined }) }; } +function createMockTokenService() { + return { evict: jest.fn() }; +} + function createServer(overrides: Record = {}) { return new ExecutorHttpServer({ port: 0, @@ -406,6 +410,18 @@ describe('DELETE /mcp-oauth-credentials/:mcpServerId', () => { expect(store.delete).toHaveBeenCalledWith(7, 'mcp-server-1'); }); + it('evicts the in-process cached access token so the disconnect takes effect immediately', async () => { + const oauthTokenService = createMockTokenService(); + const server = createServer({ oauthTokenService }); + const token = signToken({ id: 7 }); + + await request(server.callback) + .delete('/mcp-oauth-credentials/mcp-server-1') + .set('Authorization', `Bearer ${token}`); + + expect(oauthTokenService.evict).toHaveBeenCalledWith(7, 'mcp-server-1'); + }); + it("does not delete another user's credential", async () => { const store = createMockStore(); const server = createServer({ mcpOAuthCredentialsStore: store }); diff --git a/packages/workflow-executor/test/oauth/token-service.test.ts b/packages/workflow-executor/test/oauth/token-service.test.ts index 5104065761..72db9442ff 100644 --- a/packages/workflow-executor/test/oauth/token-service.test.ts +++ b/packages/workflow-executor/test/oauth/token-service.test.ts @@ -142,6 +142,19 @@ describe('OAuthTokenService.getAccessToken', () => { expect(refresh).toHaveBeenCalledTimes(1); }); + it('evict drops the cached token so the next acquire refreshes again', async () => { + const { service, refresh } = setup(); + + await service.getAccessToken(USER_ID, SERVER_ID); + await service.getAccessToken(USER_ID, SERVER_ID); + expect(refresh).toHaveBeenCalledTimes(1); // second call served from cache + + service.evict(USER_ID, SERVER_ID); + await service.getAccessToken(USER_ID, SERVER_ID); + + expect(refresh).toHaveBeenCalledTimes(2); // cache dropped → refreshed again + }); + it('refreshes again once the token is within the skew window of expiry', async () => { const { service, refresh, advance } = setup({ expirySkewS: 60 }); From a27a5c4fb34d21b94eab24e8ff6d9c1cd929044a Mon Sep 17 00:00:00 2001 From: Brian Fox Date: Mon, 29 Jun 2026 21:11:01 +0200 Subject: [PATCH 10/19] fix(workflow-executor): address PR review on oauth2 runtime [PRD-624] MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - list-mcp-tools: map ExecutorEncryptionKeyMissingError to the typed 503 { code } the deposit endpoint already returns, so the details page shows the admin message instead of a generic error. - token-service: encrypt the rotated refresh token inside the best-effort write-back try, so an encrypt failure can't fail an otherwise-valid getAccessToken. - ai-proxy isMcpAuthError: an explicit status is authoritative — a 403 is not refreshable even when its message says "unauthorized"; fall back to the message only when no status is present. - Trim comments to why-not-how. Co-Authored-By: Claude Opus 4.8 --- packages/ai-proxy/src/mcp-auth-error.ts | 7 ++++--- packages/ai-proxy/test/mcp-auth-error.test.ts | 6 ++++++ packages/workflow-executor/src/errors.ts | 2 +- .../src/executors/mcp-step-executor.ts | 4 ---- .../src/http/executor-http-server.ts | 9 +++++++++ .../workflow-executor/src/oauth/refresh-grant.ts | 4 ++-- .../workflow-executor/src/oauth/token-service.ts | 4 ++-- .../src/types/validated/step-outcome.ts | 5 ++--- .../test/http/mcp-oauth-credentials-route.test.ts | 15 +++++++++++++++ .../test/oauth/token-service.test.ts | 13 +++++++++++++ 10 files changed, 54 insertions(+), 15 deletions(-) diff --git a/packages/ai-proxy/src/mcp-auth-error.ts b/packages/ai-proxy/src/mcp-auth-error.ts index c53a05dd82..e72ffc2c3d 100644 --- a/packages/ai-proxy/src/mcp-auth-error.ts +++ b/packages/ai-proxy/src/mcp-auth-error.ts @@ -42,10 +42,11 @@ function errorChain(error: unknown): unknown[] { export function isMcpAuthError(error: unknown): boolean { return errorChain(error).some(link => { const status = statusOf(link); + // An explicit status is authoritative — a 403 is not a refreshable auth error even if its + // message says "unauthorized". Fall back to the message only when no status is present. + if (status !== undefined) return AUTH_STATUSES.has(status); - return ( - (status !== undefined && AUTH_STATUSES.has(status)) || AUTH_PATTERN.test(messageOf(link)) - ); + return AUTH_PATTERN.test(messageOf(link)); }); } diff --git a/packages/ai-proxy/test/mcp-auth-error.test.ts b/packages/ai-proxy/test/mcp-auth-error.test.ts index 0d79760134..39c1d157bb 100644 --- a/packages/ai-proxy/test/mcp-auth-error.test.ts +++ b/packages/ai-proxy/test/mcp-auth-error.test.ts @@ -30,6 +30,12 @@ describe('isMcpAuthError', () => { expect(isMcpAuthError(new Error('500 Internal Server Error'))).toBe(false); expect(isMcpAuthError(undefined)).toBe(false); }); + + it('lets an explicit status win over an "unauthorized" message (a 403 stays non-auth)', () => { + expect(isMcpAuthError(Object.assign(new Error('Unauthorized scope'), { status: 403 }))).toBe( + false, + ); + }); }); describe('classifyMcpLoadError', () => { diff --git a/packages/workflow-executor/src/errors.ts b/packages/workflow-executor/src/errors.ts index 7e65ea5af2..3c370fb563 100644 --- a/packages/workflow-executor/src/errors.ts +++ b/packages/workflow-executor/src/errors.ts @@ -472,7 +472,7 @@ export class SourceRecordMissingError extends WorkflowExecutorError { // Boundary error — surfaces from Runner.start() and is caught at the CLI/HTTP layer, not by step executors. export class AgentProbeError extends Error { - // Manual `cause` assignment: Error accepts it natively since Node 16.9 but our TS target is ES2020. + // Manual `cause` assignment: our ES2020 TS target doesn't type the native Error `cause` option. readonly cause?: unknown; constructor(message: string, options?: { cause?: unknown }) { diff --git a/packages/workflow-executor/src/executors/mcp-step-executor.ts b/packages/workflow-executor/src/executors/mcp-step-executor.ts index ce45d008d6..73727ba862 100644 --- a/packages/workflow-executor/src/executors/mcp-step-executor.ts +++ b/packages/workflow-executor/src/executors/mcp-step-executor.ts @@ -101,7 +101,6 @@ export default class McpStepExecutor extends BaseStepExecutor } private async runStep(): Promise { - // Branch A -- Re-entry after pending execution found in RunStore const pending = await this.patchAndReloadPendingData( this.context.incomingPendingData, ); @@ -112,7 +111,6 @@ export default class McpStepExecutor extends BaseStepExecutor ); } - // Branches B & C -- First call const tools = this.requireTools(); const { toolName, args } = await this.selectTool(tools); const selectedTool = tools.find(t => t.base.name === toolName); @@ -120,11 +118,9 @@ export default class McpStepExecutor extends BaseStepExecutor const target: McpToolCall = { name: toolName, sourceId: selectedTool.sourceId, input: args }; if (this.context.stepDefinition.executionType === StepExecutionMode.FullyAutomated) { - // Branch B -- direct execution return this.executeToolAndPersist(target); } - // Branch C -- Awaiting confirmation await this.context.runStore.saveStepExecution(this.context.runId, { type: 'mcp', stepIndex: this.context.stepIndex, diff --git a/packages/workflow-executor/src/http/executor-http-server.ts b/packages/workflow-executor/src/http/executor-http-server.ts index 9db68d54e1..6d3c33ffba 100644 --- a/packages/workflow-executor/src/http/executor-http-server.ts +++ b/packages/workflow-executor/src/http/executor-http-server.ts @@ -336,6 +336,15 @@ export default class ExecutorHttpServer { return; } + // Key-missing is an operator misconfig, not re-consent-resolvable: return the same typed 503 + // the deposit endpoint uses so the details page shows the admin message, not a generic error. + if (err instanceof ExecutorEncryptionKeyMissingError) { + ctx.status = 503; + ctx.body = { code: err.code }; + + return; + } + throw err; } } diff --git a/packages/workflow-executor/src/oauth/refresh-grant.ts b/packages/workflow-executor/src/oauth/refresh-grant.ts index 74fab606be..cc250a5fe6 100644 --- a/packages/workflow-executor/src/oauth/refresh-grant.ts +++ b/packages/workflow-executor/src/oauth/refresh-grant.ts @@ -78,8 +78,8 @@ export default async function refreshAccessToken( try { const parsed: unknown = await response.json(); - // A non-object body (e.g. literal null) would make the property reads below throw a TypeError; - // keep the {} default so the status checks still surface a typed OAuthRefreshError. + // Ignore a non-object body (e.g. literal null), keeping the {} default so the status checks + // below still surface a typed OAuthRefreshError. if (parsed && typeof parsed === 'object') payload = parsed as TokenEndpointResponse; } catch { // Non-JSON body — handled by the status checks below. diff --git a/packages/workflow-executor/src/oauth/token-service.ts b/packages/workflow-executor/src/oauth/token-service.ts index c91d19d7ef..31e070d88f 100644 --- a/packages/workflow-executor/src/oauth/token-service.ts +++ b/packages/workflow-executor/src/oauth/token-service.ts @@ -183,9 +183,9 @@ export default class OAuthTokenService { credential: StoredMcpOAuthCredential, refreshToken: string, ): Promise { - const encrypted = this.encryption.encrypt(refreshToken); - try { + const encrypted = this.encryption.encrypt(refreshToken); + await this.store.upsert({ userId: credential.userId, mcpServerId: credential.mcpServerId, diff --git a/packages/workflow-executor/src/types/validated/step-outcome.ts b/packages/workflow-executor/src/types/validated/step-outcome.ts index 7f1b07f0e4..6d68c73613 100644 --- a/packages/workflow-executor/src/types/validated/step-outcome.ts +++ b/packages/workflow-executor/src/types/validated/step-outcome.ts @@ -9,9 +9,8 @@ export type BaseStepStatus = z.infer; export const RecordStepStatusSchema = z.enum(['success', 'error', 'awaiting-input']); export type RecordStepStatus = z.infer; -// Typed reason for an awaiting-input pause the user must resolve out of band. The field name is -// `awaitingInputReason` (the orchestrator renamed it from `reason`); the Forest server validates it -// and the front reads it to prompt the matching action. +// Typed reason for an awaiting-input pause the user must resolve out of band: the Forest server +// validates it and the front reads it to prompt the matching action. export const AwaitingInputReasonSchema = z.enum(['needs-oauth-reauth']); export type AwaitingInputReason = z.infer; diff --git a/packages/workflow-executor/test/http/mcp-oauth-credentials-route.test.ts b/packages/workflow-executor/test/http/mcp-oauth-credentials-route.test.ts index 7688029490..cd8bc42ef4 100644 --- a/packages/workflow-executor/test/http/mcp-oauth-credentials-route.test.ts +++ b/packages/workflow-executor/test/http/mcp-oauth-credentials-route.test.ts @@ -514,4 +514,19 @@ describe('GET /list-mcp-tools', () => { mcpServerId: 'mcp-server-1', }); }); + + it('returns the typed 503 executor_encryption_key_missing when the key is absent', async () => { + const fetcher = { + fetch: jest.fn().mockRejectedValue(new ExecutorEncryptionKeyMissingError()), + }; + const server = createServer({ remoteToolFetcher: fetcher }); + const token = signToken({ id: 7 }); + + const response = await request(server.callback) + .get('/list-mcp-tools?mcpServerId=mcp-server-1') + .set('Authorization', `Bearer ${token}`); + + expect(response.status).toBe(503); + expect(response.body).toEqual({ code: 'executor_encryption_key_missing' }); + }); }); diff --git a/packages/workflow-executor/test/oauth/token-service.test.ts b/packages/workflow-executor/test/oauth/token-service.test.ts index 72db9442ff..6f68d848a5 100644 --- a/packages/workflow-executor/test/oauth/token-service.test.ts +++ b/packages/workflow-executor/test/oauth/token-service.test.ts @@ -231,6 +231,19 @@ describe('OAuthTokenService.getAccessToken', () => { await expect(service.getAccessToken(USER_ID, SERVER_ID)).resolves.toBe('at-1'); }); + + it('still returns the token when encrypting the rotated refresh token throws', async () => { + const refresh = jest + .fn() + .mockResolvedValue({ accessToken: 'at-1', expiresInS: 3600, refreshToken: 'rt-2' }); + const { service, encrypt, upsert } = setup({ refresh }); + (encrypt as jest.Mock).mockImplementation(() => { + throw new Error('key unavailable'); + }); + + await expect(service.getAccessToken(USER_ID, SERVER_ID)).resolves.toBe('at-1'); + expect(upsert).not.toHaveBeenCalled(); + }); }); describe('invalid_grant — concurrent rotation recovery', () => { From 6bc22e642a3d863788bce5a72edc4ef217869880 Mon Sep 17 00:00:00 2001 From: Brian Fox Date: Mon, 29 Jun 2026 21:14:14 +0200 Subject: [PATCH 11/19] docs(workflow-executor): correct write-back-failure comment [PRD-624] The re-read recovery path only handles a peer instance's rotation, not our own failed write-back (the stored token is unchanged, so re-read sees no rotation and forces re-auth). Describe the actual fallback: re-authentication. Co-Authored-By: Claude Opus 4.8 --- packages/workflow-executor/src/oauth/token-service.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/packages/workflow-executor/src/oauth/token-service.ts b/packages/workflow-executor/src/oauth/token-service.ts index 31e070d88f..5c06e9104c 100644 --- a/packages/workflow-executor/src/oauth/token-service.ts +++ b/packages/workflow-executor/src/oauth/token-service.ts @@ -198,8 +198,9 @@ export default class OAuthTokenService { scopes: credential.scopes, }); } catch (error) { - // A failed write-back is not fatal: the access token just obtained is valid. The next refresh - // would hit invalid_grant and recover via the re-read path. + // A failed write-back is not fatal: the access token just obtained is valid for this call. + // The rotated refresh token is lost, so a later refresh forces re-authentication — the safe + // fallback, and better than failing the current operation over a transient write error. this.logger?.('Error', 'Failed to persist rotated MCP OAuth refresh token', { mcpServerId: credential.mcpServerId, error: error instanceof Error ? error.message : String(error), From 5b0ddb7ea3eb901fea10ffe6228f626a9df89206 Mon Sep 17 00:00:00 2001 From: Brian Fox Date: Mon, 29 Jun 2026 21:58:38 +0200 Subject: [PATCH 12/19] fix(workflow-executor): validate OAuth token endpoint against SSRF [PRD-624] MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The token endpoint is where the executor POSTs the refresh grant (with client credentials), but it was stored as an unconstrained string, so an authenticated caller could aim the executor at an internal address (SSRF, incl. cloud metadata). Add assertSafeTokenEndpoint, enforced at deposit (400) and again before the refresh POST (defence in depth for pre-existing rows): - scheme must be http(s); https required in production (http stays allowed off-prod for the local OAuth sim); - link-local / cloud-metadata (169.254.0.0/16, fe80::/10) blocked everywhere; - loopback blocked in production; - RFC1918 private ranges stay allowed — private OAuth providers are supported. Co-Authored-By: Claude Opus 4.8 --- packages/workflow-executor/src/errors.ts | 12 +++ .../src/http/mcp-oauth-credentials.ts | 19 ++++- .../src/oauth/refresh-grant.ts | 6 ++ .../src/oauth/token-endpoint-url.ts | 76 +++++++++++++++++ .../http/mcp-oauth-credentials-route.test.ts | 14 ++++ .../test/oauth/refresh-grant.test.ts | 14 +++- .../test/oauth/token-endpoint-url.test.ts | 84 +++++++++++++++++++ 7 files changed, 223 insertions(+), 2 deletions(-) create mode 100644 packages/workflow-executor/src/oauth/token-endpoint-url.ts create mode 100644 packages/workflow-executor/test/oauth/token-endpoint-url.test.ts diff --git a/packages/workflow-executor/src/errors.ts b/packages/workflow-executor/src/errors.ts index 3c370fb563..5e62c8df74 100644 --- a/packages/workflow-executor/src/errors.ts +++ b/packages/workflow-executor/src/errors.ts @@ -389,6 +389,18 @@ export class OAuthInvalidGrantError extends WorkflowExecutorError { } } +// The token endpoint is where the executor POSTs the refresh grant (with client credentials), so an +// unconstrained value is an SSRF vector. A rejected one fails closed (terminal) before any network +// call, rather than letting an authenticated caller aim the executor at an internal address. +export class InvalidTokenEndpointError extends WorkflowExecutorError { + constructor(reason: string) { + super( + `Invalid OAuth token endpoint: ${reason}`, + 'This tool is misconfigured (invalid token endpoint). Contact your administrator.', + ); + } +} + // Boundary error — the deposit endpoint maps it to a typed HTTP response so the frontend can tell // an operator to provision the key, not a generic or re-consent failure. export class ExecutorEncryptionKeyMissingError extends Error { diff --git a/packages/workflow-executor/src/http/mcp-oauth-credentials.ts b/packages/workflow-executor/src/http/mcp-oauth-credentials.ts index fcaa4474b5..4340bf452d 100644 --- a/packages/workflow-executor/src/http/mcp-oauth-credentials.ts +++ b/packages/workflow-executor/src/http/mcp-oauth-credentials.ts @@ -3,6 +3,8 @@ import type { McpOAuthCredentialInput } from '../ports/mcp-oauth-credentials-sto import { z } from 'zod'; +import assertSafeTokenEndpoint from '../oauth/token-endpoint-url'; + // String lengths mirror the DB column limits, so oversized values are rejected here at the boundary // rather than at insert. .strict() matters for security: it blocks a body-supplied user id, since // identity comes only from the JWT. @@ -16,7 +18,22 @@ export const depositCredentialsBodySchema = z .string() .refine(value => !Number.isNaN(Date.parse(value)), { message: 'must be a parseable date' }) .optional(), - tokenEndpoint: z.string().min(1).max(2048), + tokenEndpoint: z + .string() + .min(1) + .max(2048) + .superRefine((value, ctx) => { + // The executor POSTs the refresh grant here, so reject SSRF-prone endpoints at deposit + // rather than discovering them at refresh time. + try { + assertSafeTokenEndpoint(value); + } catch (error) { + ctx.addIssue({ + code: z.ZodIssueCode.custom, + message: error instanceof Error ? error.message : 'invalid token endpoint', + }); + } + }), tokenEndpointAuthMethod: z.string().max(64).optional(), scopes: z.string().max(2048).optional(), }) diff --git a/packages/workflow-executor/src/oauth/refresh-grant.ts b/packages/workflow-executor/src/oauth/refresh-grant.ts index cc250a5fe6..283aa969df 100644 --- a/packages/workflow-executor/src/oauth/refresh-grant.ts +++ b/packages/workflow-executor/src/oauth/refresh-grant.ts @@ -1,4 +1,5 @@ import { OAuthInvalidGrantError, OAuthRefreshError } from '../errors'; +import assertSafeTokenEndpoint from './token-endpoint-url'; export interface RefreshGrantParams { tokenEndpoint: string; @@ -64,6 +65,11 @@ function buildRequest(params: RefreshGrantParams): { export default async function refreshAccessToken( params: RefreshGrantParams, ): Promise { + // Defence in depth: deposit validation rejects SSRF-prone endpoints, but a row predating that + // validation could still carry one — re-check before the outbound POST. Throws (terminal) rather + // than reaching the network. + assertSafeTokenEndpoint(params.tokenEndpoint); + const { headers, body } = buildRequest(params); let response: Awaited>; diff --git a/packages/workflow-executor/src/oauth/token-endpoint-url.ts b/packages/workflow-executor/src/oauth/token-endpoint-url.ts new file mode 100644 index 0000000000..386b4c0a69 --- /dev/null +++ b/packages/workflow-executor/src/oauth/token-endpoint-url.ts @@ -0,0 +1,76 @@ +import net from 'net'; + +import { InvalidTokenEndpointError } from '../errors'; + +// Constrains the OAuth token endpoint without breaking intended private-provider support: +// - scheme must be http(s); in production it must be https (http stays allowed off-prod, since the +// local OAuth sim runs on http); +// - link-local (incl. cloud metadata, e.g. 169.254.169.254) is never a valid endpoint — blocked +// on every environment; +// - loopback / the executor host itself is blocked in production (allowed off-prod for the sim); +// - RFC1918 private ranges stay allowed — a private OAuth provider refreshes from inside the +// customer network by design (see PRD-367). +// IP-literal hosts cover the direct SSRF the deposit flow enables; hostnames are not DNS-resolved +// here, as a resolve-then-fetch check is racy (the resolution at fetch time can differ). + +function isProduction(): boolean { + return process.env.NODE_ENV === 'production'; +} + +function unbracket(hostname: string): string { + return hostname.startsWith('[') && hostname.endsWith(']') ? hostname.slice(1, -1) : hostname; +} + +function classify(host: string): { loopback: boolean; linkLocal: boolean } { + const kind = net.isIP(host); + + if (kind === 4) { + const [a, b] = host.split('.').map(Number); + + return { loopback: a === 127, linkLocal: a === 169 && b === 254 }; + } + + if (kind === 6) { + const normalized = host.toLowerCase().split('%')[0]; // drop any zone id + const firstHextet = normalized.split(':')[0]; + + return { + loopback: normalized === '::1' || normalized === '0:0:0:0:0:0:0:1', + linkLocal: /^fe[89ab]/.test(firstHextet), // fe80::/10 + }; + } + + // Not an IP literal — only the obvious local alias is treated as loopback; other hostnames are + // left to the scheme rule (not resolved). + return { loopback: host.toLowerCase() === 'localhost', linkLocal: false }; +} + +export default function assertSafeTokenEndpoint(raw: string): void { + let url: URL; + + try { + url = new URL(raw); + } catch { + throw new InvalidTokenEndpointError('it is not a valid absolute URL'); + } + + if (url.protocol !== 'https:' && url.protocol !== 'http:') { + throw new InvalidTokenEndpointError('it must use http or https'); + } + + const { loopback, linkLocal } = classify(unbracket(url.hostname)); + + if (linkLocal) { + throw new InvalidTokenEndpointError('it points at a link-local / metadata address'); + } + + if (isProduction()) { + if (url.protocol !== 'https:') { + throw new InvalidTokenEndpointError('it must use https'); + } + + if (loopback) { + throw new InvalidTokenEndpointError('it points at the executor host (loopback)'); + } + } +} diff --git a/packages/workflow-executor/test/http/mcp-oauth-credentials-route.test.ts b/packages/workflow-executor/test/http/mcp-oauth-credentials-route.test.ts index cd8bc42ef4..d88fc08024 100644 --- a/packages/workflow-executor/test/http/mcp-oauth-credentials-route.test.ts +++ b/packages/workflow-executor/test/http/mcp-oauth-credentials-route.test.ts @@ -327,6 +327,20 @@ describe('POST /mcp-oauth-credentials', () => { expect(store.upsert).not.toHaveBeenCalled(); }); + it('returns 400 when tokenEndpoint points at a link-local / metadata address (SSRF guard)', async () => { + const store = createMockStore(); + const server = createServer({ mcpOAuthCredentialsStore: store }); + const token = signToken({ id: 1 }); + + const response = await request(server.callback) + .post('/mcp-oauth-credentials') + .set('Authorization', `Bearer ${token}`) + .send({ ...validBody, tokenEndpoint: 'http://169.254.169.254/latest/meta-data' }); + + expect(response.status).toBe(400); + expect(store.upsert).not.toHaveBeenCalled(); + }); + it('returns 400 when a field has the wrong type', async () => { const store = createMockStore(); const server = createServer({ mcpOAuthCredentialsStore: store }); diff --git a/packages/workflow-executor/test/oauth/refresh-grant.test.ts b/packages/workflow-executor/test/oauth/refresh-grant.test.ts index 766e83311a..69cca91aaf 100644 --- a/packages/workflow-executor/test/oauth/refresh-grant.test.ts +++ b/packages/workflow-executor/test/oauth/refresh-grant.test.ts @@ -1,4 +1,8 @@ -import { OAuthInvalidGrantError, OAuthRefreshError } from '../../src/errors'; +import { + InvalidTokenEndpointError, + OAuthInvalidGrantError, + OAuthRefreshError, +} from '../../src/errors'; import refreshAccessToken from '../../src/oauth/refresh-grant'; function mockResponse(options: { @@ -34,6 +38,14 @@ describe('refreshAccessToken', () => { return { url, headers: init.headers as Record, body }; } + it('rejects a link-local token endpoint without making a request (SSRF guard)', async () => { + await expect( + refreshAccessToken({ tokenEndpoint: 'http://169.254.169.254/token', refreshToken: 'rt-1' }), + ).rejects.toBeInstanceOf(InvalidTokenEndpointError); + + expect(fetchSpy).not.toHaveBeenCalled(); + }); + it('posts a refresh_token grant with the refresh token to the token endpoint', async () => { fetchSpy.mockResolvedValue( mockResponse({ ok: true, status: 200, payload: { access_token: 'at' } }), diff --git a/packages/workflow-executor/test/oauth/token-endpoint-url.test.ts b/packages/workflow-executor/test/oauth/token-endpoint-url.test.ts new file mode 100644 index 0000000000..553cff30ab --- /dev/null +++ b/packages/workflow-executor/test/oauth/token-endpoint-url.test.ts @@ -0,0 +1,84 @@ +import { InvalidTokenEndpointError } from '../../src/errors'; +import assertSafeTokenEndpoint from '../../src/oauth/token-endpoint-url'; + +describe('assertSafeTokenEndpoint', () => { + const ORIGINAL_NODE_ENV = process.env.NODE_ENV; + + afterEach(() => { + process.env.NODE_ENV = ORIGINAL_NODE_ENV; + }); + + describe('any environment', () => { + beforeEach(() => { + process.env.NODE_ENV = 'development'; + }); + + it('accepts an https endpoint with a public host', () => { + expect(() => assertSafeTokenEndpoint('https://idp.example.com/oauth/token')).not.toThrow(); + }); + + it('accepts http on loopback off-production (the local OAuth sim)', () => { + expect(() => assertSafeTokenEndpoint('http://127.0.0.1:9100/token')).not.toThrow(); + expect(() => assertSafeTokenEndpoint('http://localhost:9100/token')).not.toThrow(); + }); + + it('rejects a value that is not a valid absolute URL', () => { + expect(() => assertSafeTokenEndpoint('not a url')).toThrow(InvalidTokenEndpointError); + }); + + it('rejects a non-http(s) scheme', () => { + expect(() => assertSafeTokenEndpoint('file:///etc/passwd')).toThrow( + InvalidTokenEndpointError, + ); + expect(() => assertSafeTokenEndpoint('ftp://idp/token')).toThrow(InvalidTokenEndpointError); + }); + + it('rejects a link-local / cloud-metadata address even off-production', () => { + expect(() => assertSafeTokenEndpoint('http://169.254.169.254/latest/meta-data')).toThrow( + InvalidTokenEndpointError, + ); + expect(() => assertSafeTokenEndpoint('https://169.254.169.254/token')).toThrow( + InvalidTokenEndpointError, + ); + }); + + it('rejects an IPv6 link-local address', () => { + expect(() => assertSafeTokenEndpoint('http://[fe80::1]/token')).toThrow( + InvalidTokenEndpointError, + ); + }); + }); + + describe('in production', () => { + beforeEach(() => { + process.env.NODE_ENV = 'production'; + }); + + it('requires https (rejects http)', () => { + expect(() => assertSafeTokenEndpoint('http://idp.example.com/token')).toThrow( + InvalidTokenEndpointError, + ); + }); + + it('rejects loopback (the executor host itself)', () => { + expect(() => assertSafeTokenEndpoint('https://127.0.0.1/token')).toThrow( + InvalidTokenEndpointError, + ); + expect(() => assertSafeTokenEndpoint('https://localhost/token')).toThrow( + InvalidTokenEndpointError, + ); + expect(() => assertSafeTokenEndpoint('https://[::1]/token')).toThrow( + InvalidTokenEndpointError, + ); + }); + + it('still allows a private RFC1918 host over https (private providers are supported)', () => { + expect(() => assertSafeTokenEndpoint('https://10.0.0.5/token')).not.toThrow(); + expect(() => assertSafeTokenEndpoint('https://192.168.1.10:8443/token')).not.toThrow(); + }); + + it('accepts a public https endpoint', () => { + expect(() => assertSafeTokenEndpoint('https://idp.example.com/oauth/token')).not.toThrow(); + }); + }); +}); From 0da9298424111d39a074566c4ab1fa6e9592ace0 Mon Sep 17 00:00:00 2001 From: Brian Fox Date: Mon, 29 Jun 2026 22:07:21 +0200 Subject: [PATCH 13/19] fix(workflow-executor): block IPv4-mapped IPv6 in token-endpoint guard [PRD-624] MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The URL parser normalizes `::ffff:127.0.0.1` to the hex form `::ffff:7f00:1`, which the IPv6 branch classified as neither loopback nor link-local — so a mapped loopback/metadata address slipped past the SSRF block in production. Extract the embedded IPv4 from `::ffff:` addresses and run it through the IPv4 loopback/link-local checks. Co-Authored-By: Claude Opus 4.8 --- .../src/oauth/token-endpoint-url.ts | 33 ++++++++++++++++--- .../test/oauth/token-endpoint-url.test.ts | 12 +++++++ 2 files changed, 40 insertions(+), 5 deletions(-) diff --git a/packages/workflow-executor/src/oauth/token-endpoint-url.ts b/packages/workflow-executor/src/oauth/token-endpoint-url.ts index 386b4c0a69..c702d26b01 100644 --- a/packages/workflow-executor/src/oauth/token-endpoint-url.ts +++ b/packages/workflow-executor/src/oauth/token-endpoint-url.ts @@ -21,16 +21,39 @@ function unbracket(hostname: string): string { return hostname.startsWith('[') && hostname.endsWith(']') ? hostname.slice(1, -1) : hostname; } +function classifyIpv4(host: string): { loopback: boolean; linkLocal: boolean } { + const [a, b] = host.split('.').map(Number); + + return { loopback: a === 127, linkLocal: a === 169 && b === 254 }; // 127.0.0.0/8, 169.254.0.0/16 +} + +// Returns the embedded IPv4 of an IPv4-mapped IPv6 address (otherwise null). The WHATWG URL parser +// normalizes `::ffff:127.0.0.1` to the hex form `::ffff:7f00:1`, which would otherwise dodge the +// IPv4 loopback/link-local checks and still reach the executor's loopback / metadata interface. +function embeddedIpv4(host: string): string | null { + const tail = host.toLowerCase().match(/^::ffff:(.+)$/)?.[1]; + if (!tail) return null; + + if (net.isIPv4(tail)) return tail; + + const hex = tail.match(/^([0-9a-f]{1,4}):([0-9a-f]{1,4})$/); + if (!hex) return null; + + const hi = parseInt(hex[1], 16); // high 16 bits → first two octets + const lo = parseInt(hex[2], 16); // low 16 bits → last two octets + + return `${Math.floor(hi / 256)}.${hi % 256}.${Math.floor(lo / 256)}.${lo % 256}`; +} + function classify(host: string): { loopback: boolean; linkLocal: boolean } { const kind = net.isIP(host); - if (kind === 4) { - const [a, b] = host.split('.').map(Number); - - return { loopback: a === 127, linkLocal: a === 169 && b === 254 }; - } + if (kind === 4) return classifyIpv4(host); if (kind === 6) { + const mapped = embeddedIpv4(host); + if (mapped) return classifyIpv4(mapped); + const normalized = host.toLowerCase().split('%')[0]; // drop any zone id const firstHextet = normalized.split(':')[0]; diff --git a/packages/workflow-executor/test/oauth/token-endpoint-url.test.ts b/packages/workflow-executor/test/oauth/token-endpoint-url.test.ts index 553cff30ab..d199289522 100644 --- a/packages/workflow-executor/test/oauth/token-endpoint-url.test.ts +++ b/packages/workflow-executor/test/oauth/token-endpoint-url.test.ts @@ -47,6 +47,12 @@ describe('assertSafeTokenEndpoint', () => { InvalidTokenEndpointError, ); }); + + it('rejects an IPv4-mapped IPv6 metadata address (e.g. ::ffff:169.254.169.254)', () => { + expect(() => assertSafeTokenEndpoint('http://[::ffff:169.254.169.254]/token')).toThrow( + InvalidTokenEndpointError, + ); + }); }); describe('in production', () => { @@ -72,6 +78,12 @@ describe('assertSafeTokenEndpoint', () => { ); }); + it('rejects an IPv4-mapped IPv6 loopback (e.g. ::ffff:127.0.0.1)', () => { + expect(() => assertSafeTokenEndpoint('https://[::ffff:127.0.0.1]/token')).toThrow( + InvalidTokenEndpointError, + ); + }); + it('still allows a private RFC1918 host over https (private providers are supported)', () => { expect(() => assertSafeTokenEndpoint('https://10.0.0.5/token')).not.toThrow(); expect(() => assertSafeTokenEndpoint('https://192.168.1.10:8443/token')).not.toThrow(); From 68886dcb91a608a3b15eb465a028baeca2242644 Mon Sep 17 00:00:00 2001 From: Brian Fox Date: Mon, 29 Jun 2026 22:19:01 +0200 Subject: [PATCH 14/19] docs(workflow-executor): use US spelling "defense" in refresh-grant comment [PRD-624] Match the codebase convention (run-to-available-step-mapper.ts). Co-Authored-By: Claude Opus 4.8 --- packages/workflow-executor/src/oauth/refresh-grant.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/workflow-executor/src/oauth/refresh-grant.ts b/packages/workflow-executor/src/oauth/refresh-grant.ts index 283aa969df..3cbd628a3b 100644 --- a/packages/workflow-executor/src/oauth/refresh-grant.ts +++ b/packages/workflow-executor/src/oauth/refresh-grant.ts @@ -65,7 +65,7 @@ function buildRequest(params: RefreshGrantParams): { export default async function refreshAccessToken( params: RefreshGrantParams, ): Promise { - // Defence in depth: deposit validation rejects SSRF-prone endpoints, but a row predating that + // Defense in depth: deposit validation rejects SSRF-prone endpoints, but a row predating that // validation could still carry one — re-check before the outbound POST. Throws (terminal) rather // than reaching the network. assertSafeTokenEndpoint(params.tokenEndpoint); From 110ffb81327fa392aafe9b619aac0b1a2f99993c Mon Sep 17 00:00:00 2001 From: Brian Fox Date: Mon, 29 Jun 2026 22:26:03 +0200 Subject: [PATCH 15/19] docs(workflow-executor): rewrite token-endpoint guard comment [PRD-624] Drop the local-test-tool and ticket references; explain the why (SSRF rationale, the deliberate RFC1918 allowance, why no DNS resolution) rather than enumerating the rules. Co-Authored-By: Claude Opus 4.8 --- .../src/oauth/token-endpoint-url.ts | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/packages/workflow-executor/src/oauth/token-endpoint-url.ts b/packages/workflow-executor/src/oauth/token-endpoint-url.ts index c702d26b01..f6c1359670 100644 --- a/packages/workflow-executor/src/oauth/token-endpoint-url.ts +++ b/packages/workflow-executor/src/oauth/token-endpoint-url.ts @@ -2,16 +2,14 @@ import net from 'net'; import { InvalidTokenEndpointError } from '../errors'; -// Constrains the OAuth token endpoint without breaking intended private-provider support: -// - scheme must be http(s); in production it must be https (http stays allowed off-prod, since the -// local OAuth sim runs on http); -// - link-local (incl. cloud metadata, e.g. 169.254.169.254) is never a valid endpoint — blocked -// on every environment; -// - loopback / the executor host itself is blocked in production (allowed off-prod for the sim); -// - RFC1918 private ranges stay allowed — a private OAuth provider refreshes from inside the -// customer network by design (see PRD-367). -// IP-literal hosts cover the direct SSRF the deposit flow enables; hostnames are not DNS-resolved -// here, as a resolve-then-fetch check is racy (the resolution at fetch time can differ). +// The token endpoint comes from the deposit body, and the executor POSTs the refresh grant (with +// client credentials) to it — so an unconstrained value is an SSRF vector. The guard rejects hosts +// that are never a legitimate endpoint but are prime SSRF targets (loopback, link-local / +// cloud-metadata) and requires TLS, while deliberately allowing private (RFC1918) hosts: a private +// OAuth provider reached over the internal network is a supported deployment, so blocking it would +// break a real use case. http and loopback are relaxed off-production for local development. +// Only IP literals are inspected — resolving a hostname here would be racy (it can resolve to a +// different address at fetch time), not safer. function isProduction(): boolean { return process.env.NODE_ENV === 'production'; From 709576bf5c826ddc4f6b735d093280c03b80ff7b Mon Sep 17 00:00:00 2001 From: Brian Fox Date: Tue, 30 Jun 2026 09:36:10 +0200 Subject: [PATCH 16/19] fix(workflow-executor): block the unspecified address in token-endpoint guard [PRD-624] MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 0.0.0.0 (and ::) connects to loopback on Linux, so it was an SSRF bypass the guard accepted. Classify the unspecified range (0.0.0.0/8, ::) and reject it on every environment — it is never a valid token endpoint. Surfaced by evaluating ipaddr.js, which flags it as `unspecified`; the WHATWG URL parser already normalizes the decimal/hex/short-form IP encodings to dotted form, so those were already covered. Co-Authored-By: Claude Opus 4.8 --- .../src/oauth/token-endpoint-url.ts | 21 ++++++++++++++----- .../test/oauth/token-endpoint-url.test.ts | 9 ++++++++ 2 files changed, 25 insertions(+), 5 deletions(-) diff --git a/packages/workflow-executor/src/oauth/token-endpoint-url.ts b/packages/workflow-executor/src/oauth/token-endpoint-url.ts index f6c1359670..e9ed248333 100644 --- a/packages/workflow-executor/src/oauth/token-endpoint-url.ts +++ b/packages/workflow-executor/src/oauth/token-endpoint-url.ts @@ -19,10 +19,16 @@ function unbracket(hostname: string): string { return hostname.startsWith('[') && hostname.endsWith(']') ? hostname.slice(1, -1) : hostname; } -function classifyIpv4(host: string): { loopback: boolean; linkLocal: boolean } { +type HostClass = { loopback: boolean; linkLocal: boolean; unspecified: boolean }; + +function classifyIpv4(host: string): HostClass { const [a, b] = host.split('.').map(Number); - return { loopback: a === 127, linkLocal: a === 169 && b === 254 }; // 127.0.0.0/8, 169.254.0.0/16 + return { + loopback: a === 127, // 127.0.0.0/8 + linkLocal: a === 169 && b === 254, // 169.254.0.0/16 + unspecified: a === 0, // 0.0.0.0/8 — "this host"; connects to loopback on Linux + }; } // Returns the embedded IPv4 of an IPv4-mapped IPv6 address (otherwise null). The WHATWG URL parser @@ -43,7 +49,7 @@ function embeddedIpv4(host: string): string | null { return `${Math.floor(hi / 256)}.${hi % 256}.${Math.floor(lo / 256)}.${lo % 256}`; } -function classify(host: string): { loopback: boolean; linkLocal: boolean } { +function classify(host: string): HostClass { const kind = net.isIP(host); if (kind === 4) return classifyIpv4(host); @@ -58,12 +64,13 @@ function classify(host: string): { loopback: boolean; linkLocal: boolean } { return { loopback: normalized === '::1' || normalized === '0:0:0:0:0:0:0:1', linkLocal: /^fe[89ab]/.test(firstHextet), // fe80::/10 + unspecified: normalized === '::' || normalized === '0:0:0:0:0:0:0:0', }; } // Not an IP literal — only the obvious local alias is treated as loopback; other hostnames are // left to the scheme rule (not resolved). - return { loopback: host.toLowerCase() === 'localhost', linkLocal: false }; + return { loopback: host.toLowerCase() === 'localhost', linkLocal: false, unspecified: false }; } export default function assertSafeTokenEndpoint(raw: string): void { @@ -79,7 +86,11 @@ export default function assertSafeTokenEndpoint(raw: string): void { throw new InvalidTokenEndpointError('it must use http or https'); } - const { loopback, linkLocal } = classify(unbracket(url.hostname)); + const { loopback, linkLocal, unspecified } = classify(unbracket(url.hostname)); + + if (unspecified) { + throw new InvalidTokenEndpointError('it points at the unspecified address (0.0.0.0 / ::)'); + } if (linkLocal) { throw new InvalidTokenEndpointError('it points at a link-local / metadata address'); diff --git a/packages/workflow-executor/test/oauth/token-endpoint-url.test.ts b/packages/workflow-executor/test/oauth/token-endpoint-url.test.ts index d199289522..8b1a19c55a 100644 --- a/packages/workflow-executor/test/oauth/token-endpoint-url.test.ts +++ b/packages/workflow-executor/test/oauth/token-endpoint-url.test.ts @@ -53,6 +53,15 @@ describe('assertSafeTokenEndpoint', () => { InvalidTokenEndpointError, ); }); + + it('rejects the unspecified address, which routes to loopback (0.0.0.0 / ::)', () => { + expect(() => assertSafeTokenEndpoint('https://0.0.0.0/token')).toThrow( + InvalidTokenEndpointError, + ); + expect(() => assertSafeTokenEndpoint('https://[::]/token')).toThrow( + InvalidTokenEndpointError, + ); + }); }); describe('in production', () => { From 8de88881a09952fda3ff2e2df411c3a32431b0a3 Mon Sep 17 00:00:00 2001 From: Brian Fox Date: Tue, 30 Jun 2026 09:47:17 +0200 Subject: [PATCH 17/19] fix(workflow-executor): reject localhost FQDN/.localhost aliases in token guard [PRD-624] MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Only the bare `localhost` was treated as loopback, so `localhost.` and the `.localhost` TLD (RFC 6761) — both resolving to loopback — slipped past the guard in production. Match any host whose last label is `localhost` (with or without a trailing dot), without over-blocking real domains like auth.localhost.example.com. Co-Authored-By: Claude Opus 4.8 --- .../src/oauth/token-endpoint-url.ts | 12 +++++++++--- .../test/oauth/token-endpoint-url.test.ts | 15 +++++++++++++++ 2 files changed, 24 insertions(+), 3 deletions(-) diff --git a/packages/workflow-executor/src/oauth/token-endpoint-url.ts b/packages/workflow-executor/src/oauth/token-endpoint-url.ts index e9ed248333..9049cdf793 100644 --- a/packages/workflow-executor/src/oauth/token-endpoint-url.ts +++ b/packages/workflow-executor/src/oauth/token-endpoint-url.ts @@ -68,9 +68,15 @@ function classify(host: string): HostClass { }; } - // Not an IP literal — only the obvious local alias is treated as loopback; other hostnames are - // left to the scheme rule (not resolved). - return { loopback: host.toLowerCase() === 'localhost', linkLocal: false, unspecified: false }; + // Not an IP literal. Reject the reserved loopback names — bare `localhost`, the `.localhost` TLD + // (RFC 6761), and their trailing-dot FQDN forms — since they resolve to loopback. Other hostnames + // are left to the scheme rule; their DNS is not resolved here (a hostname pointed at an internal + // IP is the filtering-agent's job, not this string check). + return { + loopback: /^localhost\.?$|\.localhost\.?$/.test(host.toLowerCase()), + linkLocal: false, + unspecified: false, + }; } export default function assertSafeTokenEndpoint(raw: string): void { diff --git a/packages/workflow-executor/test/oauth/token-endpoint-url.test.ts b/packages/workflow-executor/test/oauth/token-endpoint-url.test.ts index 8b1a19c55a..df7ae04d7a 100644 --- a/packages/workflow-executor/test/oauth/token-endpoint-url.test.ts +++ b/packages/workflow-executor/test/oauth/token-endpoint-url.test.ts @@ -87,6 +87,21 @@ describe('assertSafeTokenEndpoint', () => { ); }); + it('rejects loopback hostname aliases (localhost. and the .localhost TLD)', () => { + expect(() => assertSafeTokenEndpoint('https://localhost./token')).toThrow( + InvalidTokenEndpointError, + ); + expect(() => assertSafeTokenEndpoint('https://foo.localhost/token')).toThrow( + InvalidTokenEndpointError, + ); + }); + + it('does not over-block a real domain that merely contains "localhost"', () => { + expect(() => + assertSafeTokenEndpoint('https://auth.localhost.example.com/token'), + ).not.toThrow(); + }); + it('rejects an IPv4-mapped IPv6 loopback (e.g. ::ffff:127.0.0.1)', () => { expect(() => assertSafeTokenEndpoint('https://[::ffff:127.0.0.1]/token')).toThrow( InvalidTokenEndpointError, From 81b560719bdd50fedbea949eaa545b71d2b7d9c1 Mon Sep 17 00:00:00 2001 From: Brian Fox Date: Tue, 30 Jun 2026 09:56:06 +0200 Subject: [PATCH 18/19] test(workflow-executor): cover trailing-dot IP forms in token-endpoint guard [PRD-624] Regression test proving `127.0.0.1.` / `169.254.169.254.` are rejected: the guard runs `new URL()` first, which normalizes the trailing dot before classification, so the trailing-dot form is not a bypass. Co-Authored-By: Claude Opus 4.8 --- .../test/oauth/token-endpoint-url.test.ts | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/packages/workflow-executor/test/oauth/token-endpoint-url.test.ts b/packages/workflow-executor/test/oauth/token-endpoint-url.test.ts index df7ae04d7a..62fe158106 100644 --- a/packages/workflow-executor/test/oauth/token-endpoint-url.test.ts +++ b/packages/workflow-executor/test/oauth/token-endpoint-url.test.ts @@ -102,6 +102,15 @@ describe('assertSafeTokenEndpoint', () => { ).not.toThrow(); }); + it('rejects trailing-dot IP literals (URL parsing normalizes the dot before classification)', () => { + expect(() => assertSafeTokenEndpoint('https://127.0.0.1./token')).toThrow( + InvalidTokenEndpointError, + ); + expect(() => assertSafeTokenEndpoint('https://169.254.169.254./token')).toThrow( + InvalidTokenEndpointError, + ); + }); + it('rejects an IPv4-mapped IPv6 loopback (e.g. ::ffff:127.0.0.1)', () => { expect(() => assertSafeTokenEndpoint('https://[::ffff:127.0.0.1]/token')).toThrow( InvalidTokenEndpointError, From 5f1c6f36e6ab4cf6d0f5b62afa8fc8a8cdc41598 Mon Sep 17 00:00:00 2001 From: Brian Fox Date: Thu, 2 Jul 2026 17:20:38 +0200 Subject: [PATCH 19/19] =?UTF-8?q?fix(workflow-executor):=20address=20PR=20?= =?UTF-8?q?review=20=E2=80=94=20SSRF=20redirect,=20deposit=20evict,=20fail?= =?UTF-8?q?-closed=20env=20[PRD-624]?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - refresh-grant: reject redirects on the token POST (redirect:'manual' + any 3xx → OAuthRefreshError) so a validated token endpoint can't 3xx the grant body (refresh token / client secret) to an internal host, bypassing the endpoint validation. - executor-http-server: evict the cached access token on deposit too, so a reconnect / re-deposit takes effect immediately instead of serving the prior token until expiry (mirrors the delete path). - token-endpoint-url: fail closed — the strict https/no-loopback rules now apply unless NODE_ENV explicitly opts into the dev/test relaxation, so an unset or misspelled NODE_ENV can no longer silently allow loopback targets or cleartext http. Co-Authored-By: Claude Opus 4.8 --- .../src/http/executor-http-server.ts | 3 +++ .../src/oauth/refresh-grant.ts | 14 +++++++++- .../src/oauth/token-endpoint-url.ts | 17 +++++++----- .../http/mcp-oauth-credentials-route.test.ts | 14 ++++++++++ .../test/oauth/refresh-grant.test.ts | 27 +++++++++++++++++++ .../test/oauth/token-endpoint-url.test.ts | 27 +++++++++++++++++++ 6 files changed, 94 insertions(+), 8 deletions(-) diff --git a/packages/workflow-executor/src/http/executor-http-server.ts b/packages/workflow-executor/src/http/executor-http-server.ts index 6d3c33ffba..aa7c167003 100644 --- a/packages/workflow-executor/src/http/executor-http-server.ts +++ b/packages/workflow-executor/src/http/executor-http-server.ts @@ -290,6 +290,9 @@ export default class ExecutorHttpServer { throw err; } + // Evict any cached access token so a reconnect/re-deposit takes effect immediately, instead of + // serving the token minted from the previous credential until it expires (mirrors delete). + this.options.oauthTokenService?.evict(userId, parsed.data.mcpServerId); ctx.status = 200; ctx.body = { stored: true }; } diff --git a/packages/workflow-executor/src/oauth/refresh-grant.ts b/packages/workflow-executor/src/oauth/refresh-grant.ts index 3cbd628a3b..ee0d95aee7 100644 --- a/packages/workflow-executor/src/oauth/refresh-grant.ts +++ b/packages/workflow-executor/src/oauth/refresh-grant.ts @@ -75,11 +75,23 @@ export default async function refreshAccessToken( let response: Awaited>; try { - response = await fetch(params.tokenEndpoint, { method: 'POST', headers, body }); + // Never follow redirects: a 3xx to an internal host would re-send the grant body (refresh + // token, plus the client secret in client_secret_post) there and parse its reply as a token — + // bypassing the endpoint validation above. Any redirect is treated as a failure. + response = await fetch(params.tokenEndpoint, { + method: 'POST', + headers, + body, + redirect: 'manual', + }); } catch (cause) { throw new OAuthRefreshError('the token endpoint could not be reached', cause); } + if (response.type === 'opaqueredirect' || (response.status >= 300 && response.status < 400)) { + throw new OAuthRefreshError('the token endpoint attempted a redirect'); + } + let payload: TokenEndpointResponse = {}; try { diff --git a/packages/workflow-executor/src/oauth/token-endpoint-url.ts b/packages/workflow-executor/src/oauth/token-endpoint-url.ts index 9049cdf793..4e413f5a82 100644 --- a/packages/workflow-executor/src/oauth/token-endpoint-url.ts +++ b/packages/workflow-executor/src/oauth/token-endpoint-url.ts @@ -7,12 +7,15 @@ import { InvalidTokenEndpointError } from '../errors'; // that are never a legitimate endpoint but are prime SSRF targets (loopback, link-local / // cloud-metadata) and requires TLS, while deliberately allowing private (RFC1918) hosts: a private // OAuth provider reached over the internal network is a supported deployment, so blocking it would -// break a real use case. http and loopback are relaxed off-production for local development. -// Only IP literals are inspected — resolving a hostname here would be racy (it can resolve to a -// different address at fetch time), not safer. - -function isProduction(): boolean { - return process.env.NODE_ENV === 'production'; +// break a real use case. http and loopback are relaxed only when NODE_ENV explicitly opts in +// (development/test) — see isRelaxedEnv. Only IP literals are inspected — resolving a hostname here +// would be racy (it can resolve to a different address at fetch time), not safer. + +// Fail closed: the strict rules (https + no loopback) apply everywhere UNLESS NODE_ENV explicitly +// opts into the local-dev relaxation. An unset or misspelled NODE_ENV stays strict, so a +// misconfigured deploy can't silently allow loopback targets or cleartext http. +function isRelaxedEnv(): boolean { + return process.env.NODE_ENV === 'development' || process.env.NODE_ENV === 'test'; } function unbracket(hostname: string): string { @@ -102,7 +105,7 @@ export default function assertSafeTokenEndpoint(raw: string): void { throw new InvalidTokenEndpointError('it points at a link-local / metadata address'); } - if (isProduction()) { + if (!isRelaxedEnv()) { if (url.protocol !== 'https:') { throw new InvalidTokenEndpointError('it must use https'); } diff --git a/packages/workflow-executor/test/http/mcp-oauth-credentials-route.test.ts b/packages/workflow-executor/test/http/mcp-oauth-credentials-route.test.ts index d88fc08024..1adebb2f74 100644 --- a/packages/workflow-executor/test/http/mcp-oauth-credentials-route.test.ts +++ b/packages/workflow-executor/test/http/mcp-oauth-credentials-route.test.ts @@ -153,6 +153,20 @@ describe('POST /mcp-oauth-credentials', () => { ); }); + it('evicts any cached access token on deposit so a reconnect takes effect immediately', async () => { + const tokenService = createMockTokenService(); + const server = createServer({ oauthTokenService: tokenService }); + const token = signToken({ id: 7 }); + + const response = await request(server.callback) + .post('/mcp-oauth-credentials') + .set('Authorization', `Bearer ${token}`) + .send(validBody); + + expect(response.status).toBe(200); + expect(tokenService.evict).toHaveBeenCalledWith(7, 'mcp-server-1'); + }); + it('rejects a body that tries to supply a user id (the token is the only source)', async () => { const store = createMockStore(); const server = createServer({ mcpOAuthCredentialsStore: store }); diff --git a/packages/workflow-executor/test/oauth/refresh-grant.test.ts b/packages/workflow-executor/test/oauth/refresh-grant.test.ts index 69cca91aaf..66b8acaf29 100644 --- a/packages/workflow-executor/test/oauth/refresh-grant.test.ts +++ b/packages/workflow-executor/test/oauth/refresh-grant.test.ts @@ -46,6 +46,33 @@ describe('refreshAccessToken', () => { expect(fetchSpy).not.toHaveBeenCalled(); }); + it('sends the request with redirect:manual so redirects are never followed', async () => { + fetchSpy.mockResolvedValue( + mockResponse({ ok: true, status: 200, payload: { access_token: 'at' } }), + ); + + await refreshAccessToken({ tokenEndpoint: 'https://idp/token', refreshToken: 'rt-1' }); + + expect(fetchSpy.mock.calls[0][1].redirect).toBe('manual'); + }); + + it('rejects a redirect response without following it or reading the body (SSRF via redirect)', async () => { + const jsonSpy = jest.fn(); + fetchSpy.mockResolvedValue({ + ok: false, + status: 302, + type: 'opaqueredirect', + json: jsonSpy, + } as unknown as Response); + + await expect( + refreshAccessToken({ tokenEndpoint: 'https://idp/token', refreshToken: 'rt-1' }), + ).rejects.toBeInstanceOf(OAuthRefreshError); + + // The grant body (refresh token / client secret) must not be re-sent nor the reply parsed. + expect(jsonSpy).not.toHaveBeenCalled(); + }); + it('posts a refresh_token grant with the refresh token to the token endpoint', async () => { fetchSpy.mockResolvedValue( mockResponse({ ok: true, status: 200, payload: { access_token: 'at' } }), diff --git a/packages/workflow-executor/test/oauth/token-endpoint-url.test.ts b/packages/workflow-executor/test/oauth/token-endpoint-url.test.ts index 62fe158106..f3ea4672f4 100644 --- a/packages/workflow-executor/test/oauth/token-endpoint-url.test.ts +++ b/packages/workflow-executor/test/oauth/token-endpoint-url.test.ts @@ -126,4 +126,31 @@ describe('assertSafeTokenEndpoint', () => { expect(() => assertSafeTokenEndpoint('https://idp.example.com/oauth/token')).not.toThrow(); }); }); + + describe('fails closed on a non-explicit environment', () => { + it('applies the strict rules when NODE_ENV is unset (not silently relaxed)', () => { + delete process.env.NODE_ENV; + expect(() => assertSafeTokenEndpoint('http://idp.example.com/token')).toThrow( + InvalidTokenEndpointError, + ); + expect(() => assertSafeTokenEndpoint('https://127.0.0.1/token')).toThrow( + InvalidTokenEndpointError, + ); + }); + + it('applies the strict rules when NODE_ENV is an unexpected value (e.g. misspelled)', () => { + process.env.NODE_ENV = 'produciton'; + expect(() => assertSafeTokenEndpoint('http://idp.example.com/token')).toThrow( + InvalidTokenEndpointError, + ); + expect(() => assertSafeTokenEndpoint('https://localhost/token')).toThrow( + InvalidTokenEndpointError, + ); + }); + + it('relaxes only for an explicit development/test opt-in', () => { + process.env.NODE_ENV = 'test'; + expect(() => assertSafeTokenEndpoint('http://127.0.0.1:9101/token')).not.toThrow(); + }); + }); });