From e518846d6c8f312cc08d2159139ba7385329707f Mon Sep 17 00:00:00 2001 From: Brian Fox Date: Tue, 2 Jun 2026 18:53:27 +0200 Subject: [PATCH 1/7] feat(workflow-executor): add OAuth credential store + deposit endpoint Add the ai_mcp_oauth_credentials table (002 migration) + store, an HKDF/AES-GCM credential encryption helper read lazily from FOREST_EXECUTOR_ENCRYPTION_KEY, and a koaJwt-authed POST/DELETE /mcp-oauth-credentials endpoint. Additive and dormant: nothing reads the table or calls the endpoint yet. Refs PRD-367 Co-Authored-By: Claude Opus 4.8 --- packages/workflow-executor/CLAUDE.md | 5 +- .../src/build-workflow-executor.ts | 4 + .../src/crypto/credential-encryption.ts | 89 +++++ packages/workflow-executor/src/errors.ts | 11 + .../src/http/executor-http-server.ts | 121 +++++- packages/workflow-executor/src/index.ts | 9 + .../src/stores/mcp-oauth-credentials-store.ts | 217 +++++++++++ .../test/crypto/credential-encryption.test.ts | 170 ++++++++ .../http/mcp-oauth-credentials-route.test.ts | 364 ++++++++++++++++++ .../mcp-oauth-credentials-store.test.ts | 256 ++++++++++++ 10 files changed, 1242 insertions(+), 4 deletions(-) create mode 100644 packages/workflow-executor/src/crypto/credential-encryption.ts create mode 100644 packages/workflow-executor/src/stores/mcp-oauth-credentials-store.ts create mode 100644 packages/workflow-executor/test/crypto/credential-encryption.test.ts create mode 100644 packages/workflow-executor/test/http/mcp-oauth-credentials-route.test.ts create mode 100644 packages/workflow-executor/test/stores/mcp-oauth-credentials-store.test.ts diff --git a/packages/workflow-executor/CLAUDE.md b/packages/workflow-executor/CLAUDE.md index 5ce53ec59d..60ea19d91a 100644 --- a/packages/workflow-executor/CLAUDE.md +++ b/packages/workflow-executor/CLAUDE.md @@ -59,6 +59,7 @@ src/ ├── stores/ # RunStore implementations │ ├── in-memory-store.ts # InMemoryStore — Map-based, for tests │ ├── database-store.ts # DatabaseStore — Sequelize + umzug migrations +│ ├── mcp-oauth-credentials-store.ts # McpOAuthCredentialsStore — ai_mcp_oauth_credentials (002 migration) │ └── build-run-store.ts # Factory functions: buildDatabaseRunStore, buildInMemoryRunStore ├── adapters/ # Port implementations │ ├── agent-client-agent-port.ts # AgentPort via @forestadmin/agent-client @@ -72,7 +73,9 @@ src/ │ ├── load-related-record-step-executor.ts # AI-powered relation loading step (with confirmation flow) │ └── guidance-step-executor.ts # Manual guidance step (saves user input, no AI) ├── http/ # HTTP server (optional, for frontend data access) -│ └── executor-http-server.ts # Koa server: GET /runs/:runId, POST /runs/:runId/trigger +│ └── executor-http-server.ts # Koa server: GET /runs/:runId, POST /runs/:runId/trigger, POST+DELETE /mcp-oauth-credentials +├── crypto/ # At-rest encryption +│ └── credential-encryption.ts # CredentialEncryption — HKDF (FOREST_EXECUTOR_ENCRYPTION_KEY) + AES-GCM, lazy key, fail-closed └── index.ts # Barrel exports ``` diff --git a/packages/workflow-executor/src/build-workflow-executor.ts b/packages/workflow-executor/src/build-workflow-executor.ts index c5f44cd43c..99e1b24007 100644 --- a/packages/workflow-executor/src/build-workflow-executor.ts +++ b/packages/workflow-executor/src/build-workflow-executor.ts @@ -13,6 +13,7 @@ import ConsoleLogger from './adapters/console-logger'; import ForestServerWorkflowPort from './adapters/forest-server-workflow-port'; import ForestadminClientActivityLogPortFactory from './adapters/forestadmin-client-activity-log-port-factory'; import ServerAiAdapter from './adapters/server-ai-adapter'; +import CredentialEncryption from './crypto/credential-encryption'; import { DEFAULT_AI_INVOKE_TIMEOUT_MS, DEFAULT_FOREST_SERVER_URL, @@ -25,6 +26,7 @@ import Runner from './runner'; import SchemaCache from './schema-cache'; import DatabaseStore from './stores/database-store'; import InMemoryStore from './stores/in-memory-store'; +import McpOAuthCredentialsStore from './stores/mcp-oauth-credentials-store'; const FORCE_EXIT_DELAY_MS = 5000; @@ -237,6 +239,8 @@ export function buildDatabaseExecutor(options: DatabaseExecutorOptions): Workflo authSecret: options.authSecret, workflowPort: deps.workflowPort, logger: deps.logger, + mcpOAuthCredentialsStore: new McpOAuthCredentialsStore({ sequelize }), + credentialEncryption: new CredentialEncryption(), }); return createWorkflowExecutor(runner, server, deps.logger); diff --git a/packages/workflow-executor/src/crypto/credential-encryption.ts b/packages/workflow-executor/src/crypto/credential-encryption.ts new file mode 100644 index 0000000000..627cdb3b6b --- /dev/null +++ b/packages/workflow-executor/src/crypto/credential-encryption.ts @@ -0,0 +1,89 @@ +import { createCipheriv, createDecipheriv, hkdfSync, randomFillSync } from 'crypto'; + +import { ExecutorEncryptionKeyMissingError } from '../errors'; + +const ENV_KEY = 'FOREST_EXECUTOR_ENCRYPTION_KEY'; +// Fixed context label bound into the HKDF derivation — domain-separates this key from any other +// use of the same secret. Changing it would make every existing row undecryptable. +const HKDF_INFO = 'forest-executor:mcp-oauth-credentials'; +const HKDF_DIGEST = 'sha256'; +const KEY_BYTES = 32; // AES-256 +const IV_BYTES = 12; // GCM standard nonce length +const AUTH_TAG_BYTES = 16; +const ALGORITHM = 'aes-256-gcm'; +const CURRENT_ENC_KEY_VERSION = 1; + +export interface EncryptedValue { + // Packed layout: iv | authTag | ciphertext — stored as a single BLOB column. + ciphertext: Buffer; + encKeyVersion: number; +} + +// Concatenate byte arrays without going through Buffer.concat — keeps everything in the concrete +// Uint8Array domain the Node crypto types expect. +function concatBytes(parts: Uint8Array[]): Uint8Array { + const total = parts.reduce((length, part) => length + part.length, 0); + const out = new Uint8Array(total); + let offset = 0; + + for (const part of parts) { + out.set(part, offset); + offset += part.length; + } + + return out; +} + +// At-rest encryption for OAuth credentials. The key is derived in-process via HKDF from +// FOREST_EXECUTOR_ENCRYPTION_KEY and is read lazily — an executor with no OAuth in use boots and +// runs without the key ever being required. Fails closed: a missing key throws rather than +// persisting or returning an unprotected value. +export default class CredentialEncryption { + private readonly encKeyVersion: number; + + constructor(encKeyVersion: number = CURRENT_ENC_KEY_VERSION) { + this.encKeyVersion = encKeyVersion; + } + + encrypt(plaintext: string): EncryptedValue { + const iv = randomFillSync(new Uint8Array(IV_BYTES)); + const cipher = createCipheriv(ALGORITHM, this.deriveKey(), iv); + const encrypted = concatBytes([ + new Uint8Array(cipher.update(plaintext, 'utf8')), + new Uint8Array(cipher.final()), + ]); + const authTag = new Uint8Array(cipher.getAuthTag()); + + return { + ciphertext: Buffer.from(concatBytes([iv, authTag, encrypted])), + encKeyVersion: this.encKeyVersion, + }; + } + + decrypt(value: Buffer): string { + const bytes = new Uint8Array(value); + const iv = bytes.subarray(0, IV_BYTES); + const authTag = bytes.subarray(IV_BYTES, IV_BYTES + AUTH_TAG_BYTES); + const encrypted = bytes.subarray(IV_BYTES + AUTH_TAG_BYTES); + + const decipher = createDecipheriv(ALGORITHM, this.deriveKey(), iv); + decipher.setAuthTag(authTag); + + const decrypted = concatBytes([ + new Uint8Array(decipher.update(encrypted)), + new Uint8Array(decipher.final()), + ]); + + return Buffer.from(decrypted).toString('utf8'); + } + + private deriveKey(): Uint8Array { + const secret = process.env[ENV_KEY]; + + if (!secret) throw new ExecutorEncryptionKeyMissingError(); + + // hkdfSync returns an ArrayBuffer; wrap it as a concrete Uint8Array so it + // satisfies the crypto CipherKey type (Buffer's generic ArrayBufferLike backing does not). + return new Uint8Array(hkdfSync(HKDF_DIGEST, secret, new Uint8Array(0), HKDF_INFO, KEY_BYTES)); + } +} diff --git a/packages/workflow-executor/src/errors.ts b/packages/workflow-executor/src/errors.ts index 63dc104db2..dcd8880194 100644 --- a/packages/workflow-executor/src/errors.ts +++ b/packages/workflow-executor/src/errors.ts @@ -312,6 +312,17 @@ export class ConfigurationError extends Error { } } +// Boundary error — the deposit endpoint translates it into a typed HTTP response so the frontend +// can tell an operator to provision the key, rather than treating it as a generic / re-consent failure. +export class ExecutorEncryptionKeyMissingError extends Error { + readonly code = 'executor_encryption_key_missing'; + + constructor() { + super('FOREST_EXECUTOR_ENCRYPTION_KEY is not set'); + this.name = 'ExecutorEncryptionKeyMissingError'; + } +} + export class RunNotFoundError extends Error { cause?: unknown; diff --git a/packages/workflow-executor/src/http/executor-http-server.ts b/packages/workflow-executor/src/http/executor-http-server.ts index 236dbc61ba..8a04bd313e 100644 --- a/packages/workflow-executor/src/http/executor-http-server.ts +++ b/packages/workflow-executor/src/http/executor-http-server.ts @@ -1,6 +1,8 @@ +import type CredentialEncryption from '../crypto/credential-encryption'; import type { Logger } from '../ports/logger-port'; import type { WorkflowPort } from '../ports/workflow-port'; import type Runner from '../runner'; +import type McpOAuthCredentialsStore from '../stores/mcp-oauth-credentials-store'; import type { StepUser } from '../types/execution-context'; import type { Server } from 'http'; @@ -13,6 +15,7 @@ import koaJwt from 'koa-jwt'; import serializeStepForWire from './step-serializer'; import ConsoleLogger from '../adapters/console-logger'; import { + ExecutorEncryptionKeyMissingError, RunAlreadyInFlightError, RunNotFoundError, UserMismatchError, @@ -26,17 +29,32 @@ export interface ExecutorHttpServerOptions { authSecret: string; workflowPort: WorkflowPort; logger?: Logger; + mcpOAuthCredentialsStore?: McpOAuthCredentialsStore; + credentialEncryption?: CredentialEncryption; +} + +interface DepositCredentialsBody { + mcpServerId?: string; + refreshToken?: string; + clientId?: string; + clientSecret?: string; + clientSecretExpiresAt?: string; + tokenEndpoint?: string; + tokenEndpointAuthMethod?: string; + scopes?: string; } export default class ExecutorHttpServer { private readonly app: Koa; private readonly options: ExecutorHttpServerOptions; private readonly logger: Logger; + private readonly mcpOAuthCredentialsStore?: McpOAuthCredentialsStore; private server: Server | null = null; constructor(options: ExecutorHttpServerOptions) { this.options = options; this.logger = options.logger ?? new ConsoleLogger(); + this.mcpOAuthCredentialsStore = options.mcpOAuthCredentialsStore; this.app = new Koa(); // Error middleware — catches all errors (including JWT 401) and returns structured JSON @@ -96,11 +114,29 @@ export default class ExecutorHttpServer { ); router.post('/runs/:runId/trigger', this.handleTrigger.bind(this)); + // Registered only when both dependencies are wired (a real executor with a database) — keeps + // the OAuth deposit surface absent (and dormant) on in-memory / OAuth-less deployments. + const credentialsStore = this.options.mcpOAuthCredentialsStore; + const { credentialEncryption } = this.options; + + if (credentialsStore && credentialEncryption) { + router.post('/mcp-oauth-credentials', ctx => + this.handleDepositCredentials(ctx, credentialsStore, credentialEncryption), + ); + router.delete('/mcp-oauth-credentials/:mcpServerId', ctx => + this.handleDeleteCredentials(ctx, credentialsStore), + ); + } + this.app.use(router.routes()); this.app.use(router.allowedMethods()); } async start(): Promise { + if (this.mcpOAuthCredentialsStore) { + await this.mcpOAuthCredentialsStore.init(this.logger); + } + return new Promise((resolve, reject) => { this.server = http.createServer(this.app.callback()); this.server.once('error', reject); @@ -167,10 +203,9 @@ export default class ExecutorHttpServer { private async handleTrigger(ctx: Koa.Context): Promise { const { runId } = ctx.params; - const rawId = (ctx.state.user as { id?: unknown })?.id; - const bearerUserId = typeof rawId === 'number' ? rawId : Number(rawId); + const bearerUserId = this.getBearerUserId(ctx); - if (!Number.isFinite(bearerUserId)) { + if (bearerUserId === null) { ctx.status = 400; ctx.body = { error: 'Missing or invalid user id in token' }; @@ -226,4 +261,84 @@ export default class ExecutorHttpServer { ctx.status = 200; ctx.body = { triggered: true }; } + + private getBearerUserId(ctx: Koa.Context): number | null { + const rawId = (ctx.state.user as { id?: unknown })?.id; + const userId = typeof rawId === 'number' ? rawId : Number(rawId); + + return Number.isFinite(userId) ? userId : null; + } + + private async handleDepositCredentials( + ctx: Koa.Context, + store: McpOAuthCredentialsStore, + encryption: CredentialEncryption, + ): Promise { + const userId = this.getBearerUserId(ctx); + + if (userId === null) { + ctx.status = 400; + ctx.body = { error: 'Missing or invalid user id in token' }; + + return; + } + + const body = (ctx.request.body ?? {}) as DepositCredentialsBody; + + if (!body.mcpServerId || !body.refreshToken) { + ctx.status = 400; + ctx.body = { error: 'mcpServerId and refreshToken are required' }; + + return; + } + + try { + const refreshToken = encryption.encrypt(body.refreshToken); + const clientSecret = body.clientSecret ? encryption.encrypt(body.clientSecret) : null; + + await store.upsert({ + userId, + mcpServerId: body.mcpServerId, + refreshTokenEnc: refreshToken.ciphertext, + clientId: body.clientId ?? null, + clientSecretEnc: clientSecret?.ciphertext ?? null, + clientSecretExpiresAt: body.clientSecretExpiresAt + ? new Date(body.clientSecretExpiresAt) + : null, + tokenEndpoint: body.tokenEndpoint ?? null, + tokenEndpointAuthMethod: body.tokenEndpointAuthMethod ?? null, + scopes: body.scopes ?? null, + encKeyVersion: refreshToken.encKeyVersion, + }); + } catch (err) { + if (err instanceof ExecutorEncryptionKeyMissingError) { + ctx.status = 503; + ctx.body = { code: err.code }; + + return; + } + + throw err; + } + + ctx.status = 200; + ctx.body = { stored: true }; + } + + private async handleDeleteCredentials( + ctx: Koa.Context, + store: McpOAuthCredentialsStore, + ): Promise { + const userId = this.getBearerUserId(ctx); + + if (userId === null) { + ctx.status = 400; + ctx.body = { error: 'Missing or invalid user id in token' }; + + return; + } + + await store.delete(userId, ctx.params.mcpServerId); + ctx.status = 204; + } } diff --git a/packages/workflow-executor/src/index.ts b/packages/workflow-executor/src/index.ts index 7fa349a083..2bea32dc08 100644 --- a/packages/workflow-executor/src/index.ts +++ b/packages/workflow-executor/src/index.ts @@ -100,6 +100,7 @@ export { AiModelPortError, AgentProbeError, ConfigurationError, + ExecutorEncryptionKeyMissingError, InvalidPreRecordedArgsError, UnsupportedStepTypeError, UnsupportedActionFormError, @@ -126,6 +127,14 @@ export { default as SchemaResolver } from './schema-resolver'; export { default as InMemoryStore } from './stores/in-memory-store'; export { default as DatabaseStore } from './stores/database-store'; export type { DatabaseStoreOptions } from './stores/database-store'; +export { default as McpOAuthCredentialsStore } from './stores/mcp-oauth-credentials-store'; +export type { + McpOAuthCredentialInput, + StoredMcpOAuthCredential, + McpOAuthCredentialsStoreOptions, +} from './stores/mcp-oauth-credentials-store'; +export { default as CredentialEncryption } from './crypto/credential-encryption'; +export type { EncryptedValue } from './crypto/credential-encryption'; export { buildDatabaseRunStore, buildInMemoryRunStore } from './stores/build-run-store'; export { buildInMemoryExecutor, buildDatabaseExecutor } from './build-workflow-executor'; export { runCli } from './cli-core'; diff --git a/packages/workflow-executor/src/stores/mcp-oauth-credentials-store.ts b/packages/workflow-executor/src/stores/mcp-oauth-credentials-store.ts new file mode 100644 index 0000000000..0813750964 --- /dev/null +++ b/packages/workflow-executor/src/stores/mcp-oauth-credentials-store.ts @@ -0,0 +1,217 @@ +import type { Logger } from '../ports/logger-port'; +import type { QueryInterface, Sequelize } from 'sequelize'; + +import { DataTypes } from 'sequelize'; +import { SequelizeStorage, Umzug } from 'umzug'; + +import { extractErrorMessage } from '../errors'; + +const TABLE_NAME = 'ai_mcp_oauth_credentials'; + +export interface McpOAuthCredentialInput { + userId: number; + mcpServerId: string; + refreshTokenEnc: Buffer; + clientId?: string | null; + clientSecretEnc?: Buffer | null; + clientSecretExpiresAt?: Date | null; + tokenEndpoint?: string | null; + tokenEndpointAuthMethod?: string | null; + scopes?: string | null; + encKeyVersion: number; +} + +export interface StoredMcpOAuthCredential extends McpOAuthCredentialInput { + id: number; +} + +export interface McpOAuthCredentialsStoreOptions { + sequelize: Sequelize; +} + +interface CredentialRow { + id: number; + user_id: number; + mcp_server_id: string; + refresh_token_enc: Buffer; + client_id: string | null; + client_secret_enc: Buffer | null; + client_secret_expires_at: string | Date | null; + token_endpoint: string | null; + token_endpoint_auth_method: string | null; + scopes: string | null; + enc_key_version: number; +} + +export default class McpOAuthCredentialsStore { + private readonly sequelize: Sequelize; + + constructor(options: McpOAuthCredentialsStoreOptions) { + this.sequelize = options.sequelize; + } + + async init(logger?: Logger): Promise { + const umzug = new Umzug({ + migrations: [ + { + name: '002_create_mcp_oauth_credentials', + up: async ({ context }: { context: QueryInterface }) => { + await context.createTable(TABLE_NAME, { + id: { type: DataTypes.INTEGER, primaryKey: true, autoIncrement: true }, + userId: { type: DataTypes.INTEGER, allowNull: false, field: 'user_id' }, + mcpServerId: { + type: DataTypes.STRING(255), + allowNull: false, + field: 'mcp_server_id', + }, + refreshTokenEnc: { + type: DataTypes.BLOB, + allowNull: false, + field: 'refresh_token_enc', + }, + clientId: { type: DataTypes.STRING(255), allowNull: true, field: 'client_id' }, + clientSecretEnc: { + type: DataTypes.BLOB, + allowNull: true, + field: 'client_secret_enc', + }, + clientSecretExpiresAt: { + type: DataTypes.DATE, + allowNull: true, + field: 'client_secret_expires_at', + }, + tokenEndpoint: { + type: DataTypes.STRING(2048), + allowNull: true, + field: 'token_endpoint', + }, + tokenEndpointAuthMethod: { + type: DataTypes.STRING(64), + allowNull: true, + field: 'token_endpoint_auth_method', + }, + scopes: { type: DataTypes.STRING(2048), allowNull: true, field: 'scopes' }, + encKeyVersion: { + type: DataTypes.INTEGER, + allowNull: false, + field: 'enc_key_version', + }, + createdAt: { + type: DataTypes.DATE, + allowNull: false, + defaultValue: DataTypes.NOW, + field: 'created_at', + }, + updatedAt: { + type: DataTypes.DATE, + allowNull: false, + defaultValue: DataTypes.NOW, + field: 'updated_at', + }, + }); + + await context.addIndex(TABLE_NAME, ['user_id', 'mcp_server_id'], { + unique: true, + name: 'idx_user_id_mcp_server_id', + }); + }, + down: async ({ context }: { context: QueryInterface }) => { + await context.dropTable(TABLE_NAME); + }, + }, + ], + context: this.sequelize.getQueryInterface(), + storage: new SequelizeStorage({ sequelize: this.sequelize }), + logger: undefined, + }); + + try { + await umzug.up(); + } catch (error) { + logger?.error('MCP OAuth credentials migration failed', { + error: extractErrorMessage(error), + }); + throw error; + } + } + + async get(userId: number, mcpServerId: string): Promise { + const [rows] = await this.sequelize.query( + `SELECT * FROM ${TABLE_NAME} WHERE user_id = :userId AND mcp_server_id = :mcpServerId`, + { replacements: { userId, mcpServerId } }, + ); + + const row = (rows as CredentialRow[])[0]; + + return row ? McpOAuthCredentialsStore.toCredential(row) : null; + } + + async upsert(credential: McpOAuthCredentialInput): Promise { + await this.sequelize.transaction(async transaction => { + const now = new Date(); + const replacements = { + userId: credential.userId, + mcpServerId: credential.mcpServerId, + refreshTokenEnc: credential.refreshTokenEnc, + clientId: credential.clientId ?? null, + clientSecretEnc: credential.clientSecretEnc ?? null, + clientSecretExpiresAt: credential.clientSecretExpiresAt ?? null, + tokenEndpoint: credential.tokenEndpoint ?? null, + tokenEndpointAuthMethod: credential.tokenEndpointAuthMethod ?? null, + scopes: credential.scopes ?? null, + encKeyVersion: credential.encKeyVersion, + now, + }; + + // Delete + insert in transaction: dialect-agnostic upsert (avoids ON CONFLICT / ON DUPLICATE). + await this.sequelize.query( + `DELETE FROM ${TABLE_NAME} WHERE user_id = :userId AND mcp_server_id = :mcpServerId`, + { replacements, transaction }, + ); + await this.sequelize.query( + `INSERT INTO ${TABLE_NAME} ` + + '(user_id, mcp_server_id, refresh_token_enc, client_id, client_secret_enc, ' + + 'client_secret_expires_at, token_endpoint, token_endpoint_auth_method, scopes, ' + + 'enc_key_version, created_at, updated_at) VALUES ' + + '(:userId, :mcpServerId, :refreshTokenEnc, :clientId, :clientSecretEnc, ' + + ':clientSecretExpiresAt, :tokenEndpoint, :tokenEndpointAuthMethod, :scopes, ' + + ':encKeyVersion, :now, :now)', + { replacements, transaction }, + ); + }); + } + + async delete(userId: number, mcpServerId: string): Promise { + await this.sequelize.query( + `DELETE FROM ${TABLE_NAME} WHERE user_id = :userId AND mcp_server_id = :mcpServerId`, + { replacements: { userId, mcpServerId } }, + ); + } + + async close(logger?: Logger): Promise { + try { + await this.sequelize.close(); + } catch (error) { + logger?.error('Failed to close database connection', { + error: extractErrorMessage(error), + }); + } + } + + private static toCredential(row: CredentialRow): StoredMcpOAuthCredential { + return { + id: Number(row.id), + userId: Number(row.user_id), + mcpServerId: row.mcp_server_id, + refreshTokenEnc: row.refresh_token_enc, + clientId: row.client_id ?? null, + clientSecretEnc: row.client_secret_enc ?? null, + clientSecretExpiresAt: + row.client_secret_expires_at == null ? null : new Date(row.client_secret_expires_at), + tokenEndpoint: row.token_endpoint ?? null, + tokenEndpointAuthMethod: row.token_endpoint_auth_method ?? null, + scopes: row.scopes ?? null, + encKeyVersion: Number(row.enc_key_version), + }; + } +} diff --git a/packages/workflow-executor/test/crypto/credential-encryption.test.ts b/packages/workflow-executor/test/crypto/credential-encryption.test.ts new file mode 100644 index 0000000000..cfc2f58956 --- /dev/null +++ b/packages/workflow-executor/test/crypto/credential-encryption.test.ts @@ -0,0 +1,170 @@ +/** + * Spec for the at-rest credential encryption helper. + * + * Behaviour: + * - Key is derived in-process via HKDF (`crypto.hkdfSync`, fixed context label) from a dedicated + * `FOREST_EXECUTOR_ENCRYPTION_KEY` env var — separate from `FOREST_AUTH_SECRET`. + * - The key is read LAZILY (never required at construction / boot). + * - AES-GCM is used (authenticated encryption — tampering must be detected on decrypt). + * - Each encrypted value carries an `encKeyVersion` (persisted per-row by the store). + * - Fail closed: a missing key (or a failed decrypt) must throw, never return plaintext/garbage. + * + * Version-aware key selection (rotation) is not yet supported, so `decrypt` takes only the + * packed ciphertext; `encrypt` still surfaces `encKeyVersion` for the store to persist. + */ +import CredentialEncryption from '../../src/crypto/credential-encryption'; +import { ExecutorEncryptionKeyMissingError } from '../../src/errors'; + +const ENV_KEY = 'FOREST_EXECUTOR_ENCRYPTION_KEY'; +// 32-byte key as 64 hex chars (mirrors the envSecret format validated elsewhere). +const TEST_KEY = 'a'.repeat(64); +const OTHER_KEY = 'b'.repeat(64); + +describe('CredentialEncryption', () => { + const original = process.env[ENV_KEY]; + + beforeEach(() => { + process.env[ENV_KEY] = TEST_KEY; + }); + + afterEach(() => { + if (original === undefined) delete process.env[ENV_KEY]; + else process.env[ENV_KEY] = original; + }); + + describe('round-trip', () => { + it('decrypts back to the exact plaintext that was encrypted', () => { + const enc = new CredentialEncryption(); + const plaintext = 'refresh-token-abc123'; + + const { ciphertext } = enc.encrypt(plaintext); + + expect(enc.decrypt(ciphertext)).toBe(plaintext); + }); + + it('round-trips multi-byte unicode without corruption', () => { + const enc = new CredentialEncryption(); + const plaintext = 'tökén-🔐-Ω-secret'; + + const { ciphertext } = enc.encrypt(plaintext); + + expect(enc.decrypt(ciphertext)).toBe(plaintext); + }); + + it('round-trips an empty string (boundary: zero-length plaintext)', () => { + const enc = new CredentialEncryption(); + + const { ciphertext } = enc.encrypt(''); + + expect(enc.decrypt(ciphertext)).toBe(''); + }); + }); + + describe('output shape', () => { + it('returns ciphertext as a Buffer (blob-storable)', () => { + const enc = new CredentialEncryption(); + + const { ciphertext } = enc.encrypt('secret'); + + expect(Buffer.isBuffer(ciphertext)).toBe(true); + }); + + it('tags each value with a positive integer encKeyVersion', () => { + const enc = new CredentialEncryption(); + + const { encKeyVersion } = enc.encrypt('secret'); + + expect(Number.isInteger(encKeyVersion)).toBe(true); + expect(encKeyVersion).toBeGreaterThanOrEqual(1); + }); + + it('does not leak the plaintext into the ciphertext bytes', () => { + const enc = new CredentialEncryption(); + const plaintext = 'super-secret-refresh-token'; + + const { ciphertext } = enc.encrypt(plaintext); + + expect(ciphertext.toString('utf8')).not.toContain(plaintext); + expect(ciphertext.toString('latin1')).not.toContain(plaintext); + }); + }); + + describe('non-determinism (random IV per encryption)', () => { + it('produces different ciphertext for the same plaintext on repeated calls', () => { + const enc = new CredentialEncryption(); + + const a = enc.encrypt('same-plaintext'); + const b = enc.encrypt('same-plaintext'); + + expect(a.ciphertext.toString('hex')).not.toBe(b.ciphertext.toString('hex')); + }); + + it('still decrypts both independently to the same plaintext', () => { + const enc = new CredentialEncryption(); + + const a = enc.encrypt('same-plaintext'); + const b = enc.encrypt('same-plaintext'); + + expect(enc.decrypt(a.ciphertext)).toBe('same-plaintext'); + expect(enc.decrypt(b.ciphertext)).toBe('same-plaintext'); + }); + }); + + describe('authenticity (AES-GCM) — fail closed on tampering', () => { + it('throws when a ciphertext byte is flipped', () => { + const enc = new CredentialEncryption(); + const { ciphertext } = enc.encrypt('secret'); + + const tampered = Buffer.from(ciphertext.toString('hex'), 'hex'); + const last = tampered.length - 1; + tampered[last] = (tampered[last] + 1) % 256; + + expect(() => enc.decrypt(tampered)).toThrow(); + }); + + it('throws when the ciphertext is truncated', () => { + const enc = new CredentialEncryption(); + const { ciphertext } = enc.encrypt('secret'); + + const truncated = ciphertext.subarray(0, ciphertext.length - 1); + + expect(() => enc.decrypt(truncated)).toThrow(); + }); + + it('throws when decrypting under a different key (cross-key, fail closed)', () => { + const enc = new CredentialEncryption(); + const { ciphertext } = enc.encrypt('secret'); + + // Rotate the host key out from under the same payload. + process.env[ENV_KEY] = OTHER_KEY; + const other = new CredentialEncryption(); + + expect(() => other.decrypt(ciphertext)).toThrow(); + }); + }); + + describe('lazy key reading', () => { + it('does not throw at construction when the key is unset', () => { + delete process.env[ENV_KEY]; + + expect(() => new CredentialEncryption()).not.toThrow(); + }); + + it('throws ExecutorEncryptionKeyMissingError on encrypt when the key is unset', () => { + delete process.env[ENV_KEY]; + const enc = new CredentialEncryption(); + + expect(() => enc.encrypt('secret')).toThrow(ExecutorEncryptionKeyMissingError); + }); + + it('throws ExecutorEncryptionKeyMissingError on decrypt when the key is unset', () => { + const enc = new CredentialEncryption(); + const { ciphertext } = enc.encrypt('secret'); + + delete process.env[ENV_KEY]; + const cold = new CredentialEncryption(); + + expect(() => cold.decrypt(ciphertext)).toThrow(ExecutorEncryptionKeyMissingError); + }); + }); +}); 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 new file mode 100644 index 0000000000..17cecda9e6 --- /dev/null +++ b/packages/workflow-executor/test/http/mcp-oauth-credentials-route.test.ts @@ -0,0 +1,364 @@ +/** + * Spec for the OAuth credential deposit endpoint. + * + * Behaviour: + * - POST /mcp-oauth-credentials and DELETE deposit/disconnect, on the SAME HTTP server as /trigger. + * - Authenticated by the existing koaJwt middleware (forest_session_token, FOREST_AUTH_SECRET). + * - user_id is taken from the validated token, NEVER from the request body. + * - The executor encrypts the refresh token (+ client secret) and upserts one row per (user, server). + * - When FOREST_EXECUTOR_ENCRYPTION_KEY is unset, encryption fails closed and the endpoint returns + * a distinct, typed `executor_encryption_key_missing` (HTTP 503) — never a generic failure. + * - Dormant for now: nothing reads the table at runtime yet; only this deposit path writes it. + * + * Endpoint contract: + * - ExecutorHttpServer options gain: `mcpOAuthCredentialsStore` (upsert/get/delete) and + * `credentialEncryption` (encrypt/decrypt). Both injected like `runner` / `workflowPort`. + * - POST body (camelCase JSON): { mcpServerId, refreshToken, clientId?, clientSecret?, + * clientSecretExpiresAt?, tokenEndpoint?, tokenEndpointAuthMethod?, scopes? }. + * - DELETE path: /mcp-oauth-credentials/:mcpServerId. + * - Typed key-missing response: HTTP 503 with body { code: 'executor_encryption_key_missing' }. + */ +import jsonwebtoken from 'jsonwebtoken'; +import request from 'supertest'; + +import { ExecutorEncryptionKeyMissingError } from '../../src/errors'; +import ExecutorHttpServer from '../../src/http/executor-http-server'; + +const AUTH_SECRET = 'test-auth-secret'; + +function signToken(payload: object, secret = AUTH_SECRET, options?: jsonwebtoken.SignOptions) { + return jsonwebtoken.sign(payload, secret, { expiresIn: '1h', ...options }); +} + +function createMockRunner() { + return { + state: 'running', + start: jest.fn().mockResolvedValue(undefined), + stop: jest.fn().mockResolvedValue(undefined), + triggerPoll: jest.fn().mockResolvedValue(undefined), + getRunStepExecutions: jest.fn().mockResolvedValue([]), + }; +} + +function createMockWorkflowPort() { + return { + getAvailableRuns: jest.fn().mockResolvedValue({ pending: [], malformed: [] }), + getAvailableRun: jest.fn(), + updateStepExecution: jest.fn().mockResolvedValue(undefined), + getCollectionSchema: jest.fn(), + getMcpServerConfigs: jest.fn().mockResolvedValue({}), + hasRunAccess: jest.fn().mockResolvedValue(true), + }; +} + +function createMockStore() { + return { + init: jest.fn().mockResolvedValue(undefined), + upsert: jest.fn().mockResolvedValue(undefined), + get: jest.fn().mockResolvedValue(null), + delete: jest.fn().mockResolvedValue(undefined), + close: jest.fn().mockResolvedValue(undefined), + }; +} + +function createMockEncryption() { + return { + // Deterministic stub: the route under test only needs an opaque blob + version back. + encrypt: jest.fn((plaintext: string) => ({ + ciphertext: Buffer.from(`enc(${plaintext})`), + encKeyVersion: 1, + })), + decrypt: jest.fn(), + }; +} + +function createServer(overrides: Record = {}) { + return new ExecutorHttpServer({ + port: 0, + runner: createMockRunner(), + authSecret: AUTH_SECRET, + workflowPort: createMockWorkflowPort(), + mcpOAuthCredentialsStore: createMockStore(), + credentialEncryption: createMockEncryption(), + ...overrides, + } as never); +} + +const validBody = { + mcpServerId: 'mcp-server-1', + refreshToken: 'refresh-token-xyz', + clientId: 'client-abc', + clientSecret: 'client-secret-123', + tokenEndpoint: 'https://auth.example.com/token', + tokenEndpointAuthMethod: 'client_secret_post', + scopes: 'read write', +}; + +describe('POST /mcp-oauth-credentials', () => { + describe('authentication', () => { + it('returns 401 when no token is provided', async () => { + const server = createServer(); + + const response = await request(server.callback) + .post('/mcp-oauth-credentials') + .send(validBody); + + expect(response.status).toBe(401); + expect(response.body).toEqual({ error: 'Unauthorized' }); + }); + + it('returns 401 when the token is signed with the wrong secret', async () => { + const server = createServer(); + const token = signToken({ id: 1 }, 'wrong-secret'); + + const response = await request(server.callback) + .post('/mcp-oauth-credentials') + .set('Authorization', `Bearer ${token}`) + .send(validBody); + + expect(response.status).toBe(401); + }); + + it('does not write to the store when unauthenticated', async () => { + const store = createMockStore(); + const server = createServer({ mcpOAuthCredentialsStore: store }); + + await request(server.callback).post('/mcp-oauth-credentials').send(validBody); + + expect(store.upsert).not.toHaveBeenCalled(); + }); + }); + + describe('user identity from token', () => { + it('upserts using the user id from the token', async () => { + const store = createMockStore(); + const server = createServer({ mcpOAuthCredentialsStore: store }); + const token = signToken({ id: 7 }); + + await request(server.callback) + .post('/mcp-oauth-credentials') + .set('Authorization', `Bearer ${token}`) + .send(validBody); + + expect(store.upsert).toHaveBeenCalledWith( + expect.objectContaining({ userId: 7, mcpServerId: 'mcp-server-1' }), + ); + }); + + it('ignores any user id supplied in the body (token wins)', async () => { + const store = createMockStore(); + const server = createServer({ mcpOAuthCredentialsStore: store }); + const token = signToken({ id: 7 }); + + await request(server.callback) + .post('/mcp-oauth-credentials') + .set('Authorization', `Bearer ${token}`) + .send({ ...validBody, userId: 999, user_id: 999 }); + + expect(store.upsert).toHaveBeenCalledWith(expect.objectContaining({ userId: 7 })); + expect(store.upsert).not.toHaveBeenCalledWith(expect.objectContaining({ userId: 999 })); + }); + + it('returns 400 when the token carries no numeric id', async () => { + const store = createMockStore(); + const server = createServer({ mcpOAuthCredentialsStore: store }); + const token = signToken({ email: 'no-id@example.com' }); + + const response = await request(server.callback) + .post('/mcp-oauth-credentials') + .set('Authorization', `Bearer ${token}`) + .send(validBody); + + expect(response.status).toBe(400); + expect(store.upsert).not.toHaveBeenCalled(); + }); + }); + + describe('encryption before persistence', () => { + it('encrypts the refresh token and stores only the ciphertext (never plaintext)', async () => { + const store = createMockStore(); + const encryption = createMockEncryption(); + const server = createServer({ + mcpOAuthCredentialsStore: store, + credentialEncryption: encryption, + }); + const token = signToken({ id: 1 }); + + await request(server.callback) + .post('/mcp-oauth-credentials') + .set('Authorization', `Bearer ${token}`) + .send(validBody); + + expect(encryption.encrypt).toHaveBeenCalledWith('refresh-token-xyz'); + const persisted = store.upsert.mock.calls[0][0]; + expect(Buffer.isBuffer(persisted.refreshTokenEnc)).toBe(true); + expect(persisted.refreshTokenEnc.toString()).toBe('enc(refresh-token-xyz)'); + expect(persisted.encKeyVersion).toBe(1); + // The plaintext must not have been handed to the store under any field. + expect(JSON.stringify(persisted)).not.toContain('refresh-token-xyz'); + }); + + it('encrypts the client secret when one is provided', async () => { + const store = createMockStore(); + const encryption = createMockEncryption(); + const server = createServer({ + mcpOAuthCredentialsStore: store, + credentialEncryption: encryption, + }); + const token = signToken({ id: 1 }); + + await request(server.callback) + .post('/mcp-oauth-credentials') + .set('Authorization', `Bearer ${token}`) + .send(validBody); + + expect(encryption.encrypt).toHaveBeenCalledWith('client-secret-123'); + expect(store.upsert.mock.calls[0][0].clientSecretEnc.toString()).toBe( + 'enc(client-secret-123)', + ); + }); + + it('stores a null client secret for a public / PKCE client (no clientSecret in body)', async () => { + const store = createMockStore(); + const server = createServer({ mcpOAuthCredentialsStore: store }); + const token = signToken({ id: 1 }); + const { clientSecret, ...publicBody } = validBody; + + await request(server.callback) + .post('/mcp-oauth-credentials') + .set('Authorization', `Bearer ${token}`) + .send({ ...publicBody, tokenEndpointAuthMethod: 'none' }); + + expect(store.upsert).toHaveBeenCalledWith(expect.objectContaining({ clientSecretEnc: null })); + }); + }); + + describe('fail closed when the encryption key is missing', () => { + it('returns 503 with a typed executor_encryption_key_missing code', async () => { + const encryption = createMockEncryption(); + encryption.encrypt.mockImplementation(() => { + throw new ExecutorEncryptionKeyMissingError(); + }); + const server = createServer({ credentialEncryption: encryption }); + const token = signToken({ id: 1 }); + + const response = await request(server.callback) + .post('/mcp-oauth-credentials') + .set('Authorization', `Bearer ${token}`) + .send(validBody); + + expect(response.status).toBe(503); + expect(response.body).toEqual( + expect.objectContaining({ code: 'executor_encryption_key_missing' }), + ); + }); + + it('does not persist anything when the key is missing', async () => { + const store = createMockStore(); + const encryption = createMockEncryption(); + encryption.encrypt.mockImplementation(() => { + throw new ExecutorEncryptionKeyMissingError(); + }); + const server = createServer({ + mcpOAuthCredentialsStore: store, + credentialEncryption: encryption, + }); + const token = signToken({ id: 1 }); + + await request(server.callback) + .post('/mcp-oauth-credentials') + .set('Authorization', `Bearer ${token}`) + .send(validBody); + + expect(store.upsert).not.toHaveBeenCalled(); + }); + }); + + describe('body validation', () => { + it('returns 400 when the refresh token is missing', async () => { + const store = createMockStore(); + const server = createServer({ mcpOAuthCredentialsStore: store }); + const token = signToken({ id: 1 }); + const { refreshToken, ...noRefresh } = validBody; + + const response = await request(server.callback) + .post('/mcp-oauth-credentials') + .set('Authorization', `Bearer ${token}`) + .send(noRefresh); + + expect(response.status).toBe(400); + expect(store.upsert).not.toHaveBeenCalled(); + }); + + it('returns 400 when mcpServerId is missing', async () => { + const store = createMockStore(); + const server = createServer({ mcpOAuthCredentialsStore: store }); + const token = signToken({ id: 1 }); + const { mcpServerId, ...noServer } = validBody; + + const response = await request(server.callback) + .post('/mcp-oauth-credentials') + .set('Authorization', `Bearer ${token}`) + .send(noServer); + + expect(response.status).toBe(400); + expect(store.upsert).not.toHaveBeenCalled(); + }); + }); + + describe('store failure', () => { + it('returns 500 when the store rejects', async () => { + const store = createMockStore(); + store.upsert.mockRejectedValue(new Error('db down')); + 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); + + expect(response.status).toBe(500); + }); + }); +}); + +describe('DELETE /mcp-oauth-credentials/:mcpServerId', () => { + it('returns 401 when no token is provided', async () => { + const server = createServer(); + + const response = await request(server.callback).delete('/mcp-oauth-credentials/mcp-server-1'); + + expect(response.status).toBe(401); + }); + + it('deletes the credential for (token user, mcpServerId)', async () => { + const store = createMockStore(); + const server = createServer({ mcpOAuthCredentialsStore: store }); + const token = signToken({ id: 7 }); + + const response = await request(server.callback) + .delete('/mcp-oauth-credentials/mcp-server-1') + .set('Authorization', `Bearer ${token}`); + + expect(response.status).toBeGreaterThanOrEqual(200); + expect(response.status).toBeLessThan(300); + expect(store.delete).toHaveBeenCalledWith(7, 'mcp-server-1'); + }); + + it("does not delete another user's credential", async () => { + const store = createMockStore(); + const server = createServer({ mcpOAuthCredentialsStore: store }); + const token = signToken({ id: 7 }); + + await request(server.callback) + .delete('/mcp-oauth-credentials/mcp-server-1') + .set('Authorization', `Bearer ${token}`); + + expect(store.delete).not.toHaveBeenCalledWith(999, expect.anything()); + }); + + // The exact success response body (204 no-content vs 200 { deleted: true }) is unspecified in the + // ticket — left for the implementer once the response convention is settled. + it.todo('returns a no-content style success body on delete'); +}); diff --git a/packages/workflow-executor/test/stores/mcp-oauth-credentials-store.test.ts b/packages/workflow-executor/test/stores/mcp-oauth-credentials-store.test.ts new file mode 100644 index 0000000000..e82cf54b50 --- /dev/null +++ b/packages/workflow-executor/test/stores/mcp-oauth-credentials-store.test.ts @@ -0,0 +1,256 @@ +/** + * Spec for the MCP OAuth credentials store + its Umzug migration. + * + * Behaviour: + * - One row per (user_id, mcp_server_id) — UNIQUE (user_id, mcp_server_id); upsert in place. + * - Refresh token + client secret are stored as encrypted BLOBs; the store persists opaque bytes + * (encryption itself is exercised in credential-encryption.test.ts — the store does not encrypt). + * - client_id, client_secret_enc, client_secret_expires_at, scopes are nullable + * (null for public / PKCE clients). + * - enc_key_version is stored per row. + * - Deleted on disconnect / permanent refresh failure. + * - Migration `002_create_mcp_oauth_credentials` is added alongside `001_create_workflow_step_executions`. + * + * Store contract: + * import McpOAuthCredentialsStore from '../../src/stores/mcp-oauth-credentials-store'; + * const store = new McpOAuthCredentialsStore({ sequelize }); + * await store.init(); // runs the 002 migration (table exists after) + * await store.upsert(credential); // keyed by (userId, mcpServerId) + * const row = await store.get(userId, mcpServerId); // StoredCredential | null + * await store.delete(userId, mcpServerId); + * await store.close(); + * + * Field names are camelCase, mapping to the snake_case columns. + */ +import type { Sequelize as SequelizeType } from 'sequelize'; + +import { Sequelize } from 'sequelize'; + +import McpOAuthCredentialsStore from '../../src/stores/mcp-oauth-credentials-store'; + +interface CredentialInput { + userId: number; + mcpServerId: string; + refreshTokenEnc: Buffer; + clientId?: string | null; + clientSecretEnc?: Buffer | null; + clientSecretExpiresAt?: Date | null; + tokenEndpoint?: string | null; + tokenEndpointAuthMethod?: string | null; + scopes?: string | null; + encKeyVersion: number; +} + +function makeCredential(overrides: Partial = {}): CredentialInput { + return { + userId: 42, + mcpServerId: 'mcp-server-1', + refreshTokenEnc: Buffer.from('enc-refresh-token'), + clientId: 'client-abc', + clientSecretEnc: Buffer.from('enc-client-secret'), + clientSecretExpiresAt: null, + tokenEndpoint: 'https://auth.example.com/token', + tokenEndpointAuthMethod: 'client_secret_post', + scopes: 'read write', + encKeyVersion: 1, + ...overrides, + }; +} + +// Asserts presence and narrows the type — avoids non-null assertions (`!`), which the codebase avoids. +function unwrap(value: T | null | undefined): T { + if (value === null || value === undefined) { + throw new Error('expected a stored credential, got null/undefined'); + } + + return value; +} + +describe('McpOAuthCredentialsStore (SQLite)', () => { + let sequelize: SequelizeType; + let store: McpOAuthCredentialsStore; + + beforeEach(async () => { + sequelize = new Sequelize({ dialect: 'sqlite', storage: ':memory:', logging: false }); + store = new McpOAuthCredentialsStore({ sequelize }); + await store.init(); + }); + + afterEach(async () => { + await store.close(); + }); + + describe('get', () => { + it('returns null for an unknown (userId, mcpServerId)', async () => { + expect(await store.get(999, 'no-such-server')).toBeNull(); + }); + + it('returns the stored credential for a known (userId, mcpServerId)', async () => { + const credential = makeCredential(); + + await store.upsert(credential); + const row = await store.get(credential.userId, credential.mcpServerId); + + expect(row).toEqual( + expect.objectContaining({ + userId: 42, + mcpServerId: 'mcp-server-1', + clientId: 'client-abc', + tokenEndpoint: 'https://auth.example.com/token', + tokenEndpointAuthMethod: 'client_secret_post', + scopes: 'read write', + encKeyVersion: 1, + }), + ); + }); + + it('preserves the encrypted blobs byte-for-byte', async () => { + const refreshTokenEnc = Buffer.from([0x00, 0x01, 0xfe, 0xff, 0x10]); + const clientSecretEnc = Buffer.from([0xde, 0xad, 0xbe, 0xef]); + + await store.upsert(makeCredential({ refreshTokenEnc, clientSecretEnc })); + const row = unwrap(await store.get(42, 'mcp-server-1')); + + expect(row.refreshTokenEnc.toString('hex')).toBe(refreshTokenEnc.toString('hex')); + expect(unwrap(row.clientSecretEnc).toString('hex')).toBe(clientSecretEnc.toString('hex')); + }); + }); + + describe('upsert', () => { + it('updates the existing row in place for the same (userId, mcpServerId)', async () => { + await store.upsert(makeCredential({ refreshTokenEnc: Buffer.from('old'), encKeyVersion: 1 })); + await store.upsert(makeCredential({ refreshTokenEnc: Buffer.from('new'), encKeyVersion: 2 })); + + const row = unwrap(await store.get(42, 'mcp-server-1')); + + expect(row.refreshTokenEnc.toString()).toBe('new'); + expect(row.encKeyVersion).toBe(2); + }); + + it('keeps exactly one row after re-upserting the same key (UNIQUE constraint)', async () => { + await store.upsert(makeCredential({ refreshTokenEnc: Buffer.from('v1') })); + await store.upsert(makeCredential({ refreshTokenEnc: Buffer.from('v2') })); + + // Pollution-proof: count only rows for this known key, never the whole table. + const [rows] = await sequelize.query( + 'SELECT COUNT(*) AS c FROM ai_mcp_oauth_credentials WHERE user_id = 42 AND mcp_server_id = :id', + { replacements: { id: 'mcp-server-1' } }, + ); + expect(Number((rows[0] as { c: number }).c)).toBe(1); + }); + + it('stores nullable client fields as null for a public / PKCE client', async () => { + await store.upsert( + makeCredential({ + clientId: null, + clientSecretEnc: null, + clientSecretExpiresAt: null, + tokenEndpointAuthMethod: 'none', + scopes: null, + }), + ); + + const row = await store.get(42, 'mcp-server-1'); + + expect(row).toEqual( + expect.objectContaining({ + clientId: null, + clientSecretEnc: null, + clientSecretExpiresAt: null, + scopes: null, + }), + ); + }); + + it('persists client_secret_expires_at when provided', async () => { + const expiresAt = new Date('2030-01-02T03:04:05.000Z'); + + await store.upsert(makeCredential({ clientSecretExpiresAt: expiresAt })); + const row = unwrap(await store.get(42, 'mcp-server-1')); + + expect(new Date(unwrap(row.clientSecretExpiresAt)).toISOString()).toBe( + expiresAt.toISOString(), + ); + }); + }); + + describe('isolation', () => { + it('keeps credentials for the same server but different users separate', async () => { + await store.upsert(makeCredential({ userId: 1, refreshTokenEnc: Buffer.from('user-1') })); + await store.upsert(makeCredential({ userId: 2, refreshTokenEnc: Buffer.from('user-2') })); + + const rowOne = unwrap(await store.get(1, 'mcp-server-1')); + const rowTwo = unwrap(await store.get(2, 'mcp-server-1')); + + expect(rowOne.refreshTokenEnc.toString()).toBe('user-1'); + expect(rowTwo.refreshTokenEnc.toString()).toBe('user-2'); + }); + + it('keeps credentials for the same user but different servers separate', async () => { + await store.upsert( + makeCredential({ mcpServerId: 'server-a', refreshTokenEnc: Buffer.from('a') }), + ); + await store.upsert( + makeCredential({ mcpServerId: 'server-b', refreshTokenEnc: Buffer.from('b') }), + ); + + const rowA = unwrap(await store.get(42, 'server-a')); + const rowB = unwrap(await store.get(42, 'server-b')); + + expect(rowA.refreshTokenEnc.toString()).toBe('a'); + expect(rowB.refreshTokenEnc.toString()).toBe('b'); + }); + }); + + describe('delete', () => { + it('removes the credential for a (userId, mcpServerId)', async () => { + await store.upsert(makeCredential()); + + await store.delete(42, 'mcp-server-1'); + + expect(await store.get(42, 'mcp-server-1')).toBeNull(); + }); + + it('does not affect other users when deleting one user', async () => { + await store.upsert(makeCredential({ userId: 1 })); + await store.upsert(makeCredential({ userId: 2 })); + + await store.delete(1, 'mcp-server-1'); + + expect(await store.get(1, 'mcp-server-1')).toBeNull(); + expect(await store.get(2, 'mcp-server-1')).not.toBeNull(); + }); + + it('is a no-op (does not throw) when deleting a non-existent credential', async () => { + await expect(store.delete(999, 'no-such-server')).resolves.toBeUndefined(); + }); + }); + + describe('migration / init', () => { + it('creates the ai_mcp_oauth_credentials table on init', async () => { + const [rows] = await sequelize.query( + "SELECT name FROM sqlite_master WHERE type='table' AND name='ai_mcp_oauth_credentials'", + ); + + expect(rows).toHaveLength(1); + }); + + it('runs init idempotently', async () => { + await expect(store.init()).resolves.toBeUndefined(); + }); + + it('enforces the UNIQUE (user_id, mcp_server_id) constraint at the DB level', async () => { + // Direct insert bypassing upsert proves the constraint exists in the schema, not just app logic. + await store.upsert(makeCredential()); + + await expect( + sequelize.query( + 'INSERT INTO ai_mcp_oauth_credentials ' + + '(user_id, mcp_server_id, refresh_token_enc, enc_key_version, created_at, updated_at) ' + + "VALUES (42, 'mcp-server-1', :blob, 1, :now, :now)", + { replacements: { blob: Buffer.from('dup'), now: new Date() } }, + ), + ).rejects.toThrow(); + }); + }); +}); From b20ff7cfd965612f578d33369f0016761cab9a8b Mon Sep 17 00:00:00 2001 From: Brian Fox Date: Tue, 2 Jun 2026 19:06:18 +0200 Subject: [PATCH 2/7] docs(workflow-executor): note intentional empty HKDF salt Pre-empt the security-review question raised in code review: the empty salt is deliberate (domain separation comes from the fixed HKDF info label, input is a single high-entropy secret). --- packages/workflow-executor/src/crypto/credential-encryption.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/workflow-executor/src/crypto/credential-encryption.ts b/packages/workflow-executor/src/crypto/credential-encryption.ts index 627cdb3b6b..cf7e1da181 100644 --- a/packages/workflow-executor/src/crypto/credential-encryption.ts +++ b/packages/workflow-executor/src/crypto/credential-encryption.ts @@ -82,6 +82,8 @@ export default class CredentialEncryption { if (!secret) throw new ExecutorEncryptionKeyMissingError(); + // Empty salt is intentional: the fixed HKDF_INFO label provides domain separation and the + // input is a single high-entropy secret, so a salt would add no security here. // hkdfSync returns an ArrayBuffer; wrap it as a concrete Uint8Array so it // satisfies the crypto CipherKey type (Buffer's generic ArrayBufferLike backing does not). return new Uint8Array(hkdfSync(HKDF_DIGEST, secret, new Uint8Array(0), HKDF_INFO, KEY_BYTES)); From b236200a46bdb83e7979cc4a37be60cec0542f9f Mon Sep 17 00:00:00 2001 From: Brian Fox Date: Thu, 11 Jun 2026 12:21:33 +0200 Subject: [PATCH 3/7] test(workflow-executor): cover oauth wiring and delete response Cover the buildDatabaseExecutor wiring of the credentials store and encryption into the HTTP server (and its absence in-memory), and pin the DELETE success contract to 204 with no body, replacing the todo. Co-Authored-By: Claude Fable 5 --- .../test/build-workflow-executor.test.ts | 22 +++++++++++++++++++ .../http/mcp-oauth-credentials-route.test.ts | 10 +++------ 2 files changed, 25 insertions(+), 7 deletions(-) diff --git a/packages/workflow-executor/test/build-workflow-executor.test.ts b/packages/workflow-executor/test/build-workflow-executor.test.ts index 8666a691a7..2447ddc721 100644 --- a/packages/workflow-executor/test/build-workflow-executor.test.ts +++ b/packages/workflow-executor/test/build-workflow-executor.test.ts @@ -1,10 +1,13 @@ import ForestServerWorkflowPort from '../src/adapters/forest-server-workflow-port'; import { buildDatabaseExecutor, buildInMemoryExecutor } from '../src/build-workflow-executor'; +import CredentialEncryption from '../src/crypto/credential-encryption'; import { DEFAULT_SCHEMA_CACHE_TTL_MS } from '../src/defaults'; +import ExecutorHttpServer from '../src/http/executor-http-server'; import Runner from '../src/runner'; import SchemaCache from '../src/schema-cache'; import DatabaseStore from '../src/stores/database-store'; import InMemoryStore from '../src/stores/in-memory-store'; +import McpOAuthCredentialsStore from '../src/stores/mcp-oauth-credentials-store'; jest.mock('../src/runner'); jest.mock('../src/stores/in-memory-store'); @@ -56,6 +59,14 @@ describe('buildInMemoryExecutor', () => { ); }); + it('does not wire the OAuth credentials store into the HTTP server', () => { + buildInMemoryExecutor(BASE_OPTIONS); + + expect(ExecutorHttpServer).toHaveBeenCalledWith( + expect.not.objectContaining({ mcpOAuthCredentialsStore: expect.anything() }), + ); + }); + it('creates ForestServerWorkflowPort with default forestServerUrl', () => { buildInMemoryExecutor(BASE_OPTIONS); @@ -263,6 +274,17 @@ describe('buildDatabaseExecutor', () => { ); }); + it('wires the OAuth credentials store and encryption into the HTTP server', () => { + buildDatabaseExecutor(DB_OPTIONS); + + expect(ExecutorHttpServer).toHaveBeenCalledWith( + expect.objectContaining({ + mcpOAuthCredentialsStore: expect.any(McpOAuthCredentialsStore), + credentialEncryption: expect.any(CredentialEncryption), + }), + ); + }); + it('creates Sequelize with uri and passes remaining options through', () => { buildDatabaseExecutor(DB_OPTIONS); 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 17cecda9e6..d4b3cf02e4 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 @@ -332,7 +332,7 @@ describe('DELETE /mcp-oauth-credentials/:mcpServerId', () => { expect(response.status).toBe(401); }); - it('deletes the credential for (token user, mcpServerId)', async () => { + it('deletes the credential for (token user, mcpServerId) and returns 204 with no body', async () => { const store = createMockStore(); const server = createServer({ mcpOAuthCredentialsStore: store }); const token = signToken({ id: 7 }); @@ -341,8 +341,8 @@ describe('DELETE /mcp-oauth-credentials/:mcpServerId', () => { .delete('/mcp-oauth-credentials/mcp-server-1') .set('Authorization', `Bearer ${token}`); - expect(response.status).toBeGreaterThanOrEqual(200); - expect(response.status).toBeLessThan(300); + expect(response.status).toBe(204); + expect(response.body).toEqual({}); expect(store.delete).toHaveBeenCalledWith(7, 'mcp-server-1'); }); @@ -357,8 +357,4 @@ describe('DELETE /mcp-oauth-credentials/:mcpServerId', () => { expect(store.delete).not.toHaveBeenCalledWith(999, expect.anything()); }); - - // The exact success response body (204 no-content vs 200 { deleted: true }) is unspecified in the - // ticket — left for the implementer once the response convention is settled. - it.todo('returns a no-content style success body on delete'); }); From 564bb7701fcd050aa4bc5c27c6f8d0eb7a0b31b9 Mon Sep 17 00:00:00 2001 From: Brian Fox Date: Thu, 11 Jun 2026 12:31:57 +0200 Subject: [PATCH 4/7] feat(workflow-executor): validate oauth deposit body strictly The deposit body is a frontend HTTP body, which the package's boundary- validation convention requires to be zod-validated with .strict() like the /trigger pending-data payloads. Rejecting unknown keys also makes the token-only user identity explicit, and the column-bound fields now fail with a 400 at the boundary instead of a dialect-dependent insert error. Adds the missing DELETE 400 coverage for tokens without a numeric id. Co-Authored-By: Claude Fable 5 --- .../src/http/executor-http-server.ts | 23 ++++----- .../http/mcp-oauth-credentials-validators.ts | 24 +++++++++ .../http/mcp-oauth-credentials-route.test.ts | 49 +++++++++++++++++-- 3 files changed, 78 insertions(+), 18 deletions(-) create mode 100644 packages/workflow-executor/src/http/mcp-oauth-credentials-validators.ts diff --git a/packages/workflow-executor/src/http/executor-http-server.ts b/packages/workflow-executor/src/http/executor-http-server.ts index 8a04bd313e..39a12bf17b 100644 --- a/packages/workflow-executor/src/http/executor-http-server.ts +++ b/packages/workflow-executor/src/http/executor-http-server.ts @@ -12,6 +12,7 @@ import http from 'http'; import Koa from 'koa'; import koaJwt from 'koa-jwt'; +import { depositCredentialsBodySchema } from './mcp-oauth-credentials-validators'; import serializeStepForWire from './step-serializer'; import ConsoleLogger from '../adapters/console-logger'; import { @@ -33,17 +34,6 @@ export interface ExecutorHttpServerOptions { credentialEncryption?: CredentialEncryption; } -interface DepositCredentialsBody { - mcpServerId?: string; - refreshToken?: string; - clientId?: string; - clientSecret?: string; - clientSecretExpiresAt?: string; - tokenEndpoint?: string; - tokenEndpointAuthMethod?: string; - scopes?: string; -} - export default class ExecutorHttpServer { private readonly app: Koa; private readonly options: ExecutorHttpServerOptions; @@ -283,15 +273,20 @@ export default class ExecutorHttpServer { return; } - const body = (ctx.request.body ?? {}) as DepositCredentialsBody; + const parsed = depositCredentialsBodySchema.safeParse(ctx.request.body ?? {}); - if (!body.mcpServerId || !body.refreshToken) { + if (!parsed.success) { + const details = parsed.error.issues + .map(issue => `${issue.path.join('.') || 'body'}: ${issue.message}`) + .join('; '); ctx.status = 400; - ctx.body = { error: 'mcpServerId and refreshToken are required' }; + ctx.body = { error: `Invalid request body — ${details}` }; return; } + const body = parsed.data; + try { const refreshToken = encryption.encrypt(body.refreshToken); const clientSecret = body.clientSecret ? encryption.encrypt(body.clientSecret) : null; diff --git a/packages/workflow-executor/src/http/mcp-oauth-credentials-validators.ts b/packages/workflow-executor/src/http/mcp-oauth-credentials-validators.ts new file mode 100644 index 0000000000..fa22be6680 --- /dev/null +++ b/packages/workflow-executor/src/http/mcp-oauth-credentials-validators.ts @@ -0,0 +1,24 @@ +import { z } from 'zod'; + +// Boundary validation for the credential deposit body — a frontend HTTP body, so .strict() +// rejects unknown keys outright (including any attempt to smuggle a user id; the JWT is the only +// identity source). Length bounds mirror the column limits so oversized input fails here with a +// 400 instead of a dialect-dependent insert error. The refresh token and client secret are +// unbounded: they land in BLOB columns and the body parser already caps the payload size. +export const depositCredentialsBodySchema = z + .object({ + mcpServerId: z.string().min(1).max(255), + refreshToken: z.string().min(1), + clientId: z.string().max(255).optional(), + clientSecret: z.string().optional(), + clientSecretExpiresAt: z + .string() + .refine(value => !Number.isNaN(Date.parse(value)), { message: 'must be a parseable date' }) + .optional(), + tokenEndpoint: z.string().max(2048).optional(), + tokenEndpointAuthMethod: z.string().max(64).optional(), + scopes: z.string().max(2048).optional(), + }) + .strict(); + +export type DepositCredentialsBody = 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 d4b3cf02e4..c73c01ee00 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 @@ -145,18 +145,18 @@ describe('POST /mcp-oauth-credentials', () => { ); }); - it('ignores any user id supplied in the body (token wins)', async () => { + 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 }); const token = signToken({ id: 7 }); - await request(server.callback) + const response = await request(server.callback) .post('/mcp-oauth-credentials') .set('Authorization', `Bearer ${token}`) .send({ ...validBody, userId: 999, user_id: 999 }); - expect(store.upsert).toHaveBeenCalledWith(expect.objectContaining({ userId: 7 })); - expect(store.upsert).not.toHaveBeenCalledWith(expect.objectContaining({ userId: 999 })); + expect(response.status).toBe(400); + expect(store.upsert).not.toHaveBeenCalled(); }); it('returns 400 when the token carries no numeric id', async () => { @@ -304,6 +304,34 @@ describe('POST /mcp-oauth-credentials', () => { 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 }); + const token = signToken({ id: 1 }); + + const response = await request(server.callback) + .post('/mcp-oauth-credentials') + .set('Authorization', `Bearer ${token}`) + .send({ ...validBody, refreshToken: 12345 }); + + expect(response.status).toBe(400); + expect(store.upsert).not.toHaveBeenCalled(); + }); + + it('returns 400 when clientSecretExpiresAt is not a parseable date', 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, clientSecretExpiresAt: 'not-a-date' }); + + expect(response.status).toBe(400); + expect(store.upsert).not.toHaveBeenCalled(); + }); }); describe('store failure', () => { @@ -357,4 +385,17 @@ describe('DELETE /mcp-oauth-credentials/:mcpServerId', () => { expect(store.delete).not.toHaveBeenCalledWith(999, expect.anything()); }); + + it('returns 400 when the token carries no numeric id', async () => { + const store = createMockStore(); + const server = createServer({ mcpOAuthCredentialsStore: store }); + const token = signToken({ email: 'no-id@example.com' }); + + const response = await request(server.callback) + .delete('/mcp-oauth-credentials/mcp-server-1') + .set('Authorization', `Bearer ${token}`); + + expect(response.status).toBe(400); + expect(store.delete).not.toHaveBeenCalled(); + }); }); From e1838a9ee25d442e93cb17863219d8e558d07cf3 Mon Sep 17 00:00:00 2001 From: Brian Fox Date: Thu, 11 Jun 2026 16:42:57 +0200 Subject: [PATCH 5/7] refactor(workflow-executor): harden and reorganize oauth deposit endpoint Apply review feedback on PR1: - require tokenEndpoint (zod schema + NOT NULL column): the refresh grant has nowhere to go without it, so a missing one 400s at deposit rather than failing once the runtime read path lands - extract the body-to-record mapping into buildMcpOAuthCredentialInput and rename mcp-oauth-credentials-validators.ts to mcp-oauth-credentials.ts; the mapping is credential-domain logic, not transport - dedupe the bearer-user-id guard into requireBearerUserId, which also rejects non-positive / non-integer ids before they reach the store - shorten the new HKDF/boundary comments Co-Authored-By: Claude Opus 4.8 --- packages/workflow-executor/CLAUDE.md | 3 +- .../src/crypto/credential-encryption.ts | 14 ++-- packages/workflow-executor/src/errors.ts | 4 +- .../src/http/executor-http-server.ts | 73 ++++++----------- .../http/mcp-oauth-credentials-validators.ts | 24 ------ .../src/http/mcp-oauth-credentials.ts | 57 +++++++++++++ .../src/stores/mcp-oauth-credentials-store.ts | 8 +- .../http/mcp-oauth-credentials-route.test.ts | 15 ++++ .../test/http/mcp-oauth-credentials.test.ts | 82 +++++++++++++++++++ .../mcp-oauth-credentials-store.test.ts | 27 +++++- 10 files changed, 216 insertions(+), 91 deletions(-) delete mode 100644 packages/workflow-executor/src/http/mcp-oauth-credentials-validators.ts create mode 100644 packages/workflow-executor/src/http/mcp-oauth-credentials.ts create mode 100644 packages/workflow-executor/test/http/mcp-oauth-credentials.test.ts diff --git a/packages/workflow-executor/CLAUDE.md b/packages/workflow-executor/CLAUDE.md index 60ea19d91a..644466077b 100644 --- a/packages/workflow-executor/CLAUDE.md +++ b/packages/workflow-executor/CLAUDE.md @@ -73,7 +73,8 @@ src/ │ ├── load-related-record-step-executor.ts # AI-powered relation loading step (with confirmation flow) │ └── guidance-step-executor.ts # Manual guidance step (saves user input, no AI) ├── http/ # HTTP server (optional, for frontend data access) -│ └── executor-http-server.ts # Koa server: GET /runs/:runId, POST /runs/:runId/trigger, POST+DELETE /mcp-oauth-credentials +│ ├── executor-http-server.ts # Koa server: GET /runs/:runId, POST /runs/:runId/trigger, POST+DELETE /mcp-oauth-credentials +│ └── mcp-oauth-credentials.ts # Deposit-body zod schema (.strict()) + buildMcpOAuthCredentialInput mapper ├── crypto/ # At-rest encryption │ └── credential-encryption.ts # CredentialEncryption — HKDF (FOREST_EXECUTOR_ENCRYPTION_KEY) + AES-GCM, lazy key, fail-closed └── index.ts # Barrel exports diff --git a/packages/workflow-executor/src/crypto/credential-encryption.ts b/packages/workflow-executor/src/crypto/credential-encryption.ts index cf7e1da181..73c995709e 100644 --- a/packages/workflow-executor/src/crypto/credential-encryption.ts +++ b/packages/workflow-executor/src/crypto/credential-encryption.ts @@ -34,10 +34,9 @@ function concatBytes(parts: Uint8Array[]): Uint8Array { return out; } -// At-rest encryption for OAuth credentials. The key is derived in-process via HKDF from -// FOREST_EXECUTOR_ENCRYPTION_KEY and is read lazily — an executor with no OAuth in use boots and -// runs without the key ever being required. Fails closed: a missing key throws rather than -// persisting or returning an unprotected value. +// At-rest encryption for OAuth credentials. The HKDF key (from FOREST_EXECUTOR_ENCRYPTION_KEY) is +// read lazily — an OAuth-less executor boots without it — and fails closed: a missing key throws +// rather than persisting or returning an unprotected value. export default class CredentialEncryption { private readonly encKeyVersion: number; @@ -82,10 +81,9 @@ export default class CredentialEncryption { if (!secret) throw new ExecutorEncryptionKeyMissingError(); - // Empty salt is intentional: the fixed HKDF_INFO label provides domain separation and the - // input is a single high-entropy secret, so a salt would add no security here. - // hkdfSync returns an ArrayBuffer; wrap it as a concrete Uint8Array so it - // satisfies the crypto CipherKey type (Buffer's generic ArrayBufferLike backing does not). + // Empty salt is intentional: the fixed HKDF_INFO label gives domain separation and the + // single high-entropy secret needs no salt. Wrap hkdfSync's ArrayBuffer as a concrete + // Uint8Array to satisfy CipherKey (Buffer's ArrayBufferLike backing does not). return new Uint8Array(hkdfSync(HKDF_DIGEST, secret, new Uint8Array(0), HKDF_INFO, KEY_BYTES)); } } diff --git a/packages/workflow-executor/src/errors.ts b/packages/workflow-executor/src/errors.ts index dcd8880194..3563accc54 100644 --- a/packages/workflow-executor/src/errors.ts +++ b/packages/workflow-executor/src/errors.ts @@ -312,8 +312,8 @@ export class ConfigurationError extends Error { } } -// Boundary error — the deposit endpoint translates it into a typed HTTP response so the frontend -// can tell an operator to provision the key, rather than treating it as a generic / re-consent failure. +// 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 { readonly code = 'executor_encryption_key_missing'; diff --git a/packages/workflow-executor/src/http/executor-http-server.ts b/packages/workflow-executor/src/http/executor-http-server.ts index 39a12bf17b..8d0c998b62 100644 --- a/packages/workflow-executor/src/http/executor-http-server.ts +++ b/packages/workflow-executor/src/http/executor-http-server.ts @@ -12,7 +12,10 @@ import http from 'http'; import Koa from 'koa'; import koaJwt from 'koa-jwt'; -import { depositCredentialsBodySchema } from './mcp-oauth-credentials-validators'; +import { + buildMcpOAuthCredentialInput, + depositCredentialsBodySchema, +} from './mcp-oauth-credentials'; import serializeStepForWire from './step-serializer'; import ConsoleLogger from '../adapters/console-logger'; import { @@ -106,8 +109,7 @@ export default class ExecutorHttpServer { // Registered only when both dependencies are wired (a real executor with a database) — keeps // the OAuth deposit surface absent (and dormant) on in-memory / OAuth-less deployments. - const credentialsStore = this.options.mcpOAuthCredentialsStore; - const { credentialEncryption } = this.options; + const { mcpOAuthCredentialsStore: credentialsStore, credentialEncryption } = this.options; if (credentialsStore && credentialEncryption) { router.post('/mcp-oauth-credentials', ctx => @@ -193,14 +195,8 @@ export default class ExecutorHttpServer { private async handleTrigger(ctx: Koa.Context): Promise { const { runId } = ctx.params; - const bearerUserId = this.getBearerUserId(ctx); - - if (bearerUserId === null) { - ctx.status = 400; - ctx.body = { error: 'Missing or invalid user id in token' }; - - return; - } + const bearerUserId = this.requireBearerUserId(ctx); + if (bearerUserId === null) return; const pendingData = (ctx.request.body as { pendingData?: unknown })?.pendingData; @@ -252,11 +248,22 @@ export default class ExecutorHttpServer { ctx.body = { triggered: true }; } - private getBearerUserId(ctx: Koa.Context): number | null { + // Resolves the authenticated user id from the validated JWT. Writes a 400 and returns null when + // the token carries no usable id, so the three deposit/trigger handlers share one guard rather + // than repeating it. A Forest user id is always a positive integer — 0, negatives, and non-numeric + // ids are rejected here rather than reaching the store. + private requireBearerUserId(ctx: Koa.Context): number | null { const rawId = (ctx.state.user as { id?: unknown })?.id; const userId = typeof rawId === 'number' ? rawId : Number(rawId); - return Number.isFinite(userId) ? userId : null; + if (!Number.isInteger(userId) || userId <= 0) { + ctx.status = 400; + ctx.body = { error: 'Missing or invalid user id in token' }; + + return null; + } + + return userId; } private async handleDepositCredentials( @@ -264,14 +271,8 @@ export default class ExecutorHttpServer { store: McpOAuthCredentialsStore, encryption: CredentialEncryption, ): Promise { - const userId = this.getBearerUserId(ctx); - - if (userId === null) { - ctx.status = 400; - ctx.body = { error: 'Missing or invalid user id in token' }; - - return; - } + const userId = this.requireBearerUserId(ctx); + if (userId === null) return; const parsed = depositCredentialsBodySchema.safeParse(ctx.request.body ?? {}); @@ -285,26 +286,8 @@ export default class ExecutorHttpServer { return; } - const body = parsed.data; - try { - const refreshToken = encryption.encrypt(body.refreshToken); - const clientSecret = body.clientSecret ? encryption.encrypt(body.clientSecret) : null; - - await store.upsert({ - userId, - mcpServerId: body.mcpServerId, - refreshTokenEnc: refreshToken.ciphertext, - clientId: body.clientId ?? null, - clientSecretEnc: clientSecret?.ciphertext ?? null, - clientSecretExpiresAt: body.clientSecretExpiresAt - ? new Date(body.clientSecretExpiresAt) - : null, - tokenEndpoint: body.tokenEndpoint ?? null, - tokenEndpointAuthMethod: body.tokenEndpointAuthMethod ?? null, - scopes: body.scopes ?? null, - encKeyVersion: refreshToken.encKeyVersion, - }); + await store.upsert(buildMcpOAuthCredentialInput({ body: parsed.data, userId, encryption })); } catch (err) { if (err instanceof ExecutorEncryptionKeyMissingError) { ctx.status = 503; @@ -324,14 +307,8 @@ export default class ExecutorHttpServer { ctx: Koa.Context, store: McpOAuthCredentialsStore, ): Promise { - const userId = this.getBearerUserId(ctx); - - if (userId === null) { - ctx.status = 400; - ctx.body = { error: 'Missing or invalid user id in token' }; - - return; - } + const userId = this.requireBearerUserId(ctx); + if (userId === null) return; await store.delete(userId, ctx.params.mcpServerId); ctx.status = 204; diff --git a/packages/workflow-executor/src/http/mcp-oauth-credentials-validators.ts b/packages/workflow-executor/src/http/mcp-oauth-credentials-validators.ts deleted file mode 100644 index fa22be6680..0000000000 --- a/packages/workflow-executor/src/http/mcp-oauth-credentials-validators.ts +++ /dev/null @@ -1,24 +0,0 @@ -import { z } from 'zod'; - -// Boundary validation for the credential deposit body — a frontend HTTP body, so .strict() -// rejects unknown keys outright (including any attempt to smuggle a user id; the JWT is the only -// identity source). Length bounds mirror the column limits so oversized input fails here with a -// 400 instead of a dialect-dependent insert error. The refresh token and client secret are -// unbounded: they land in BLOB columns and the body parser already caps the payload size. -export const depositCredentialsBodySchema = z - .object({ - mcpServerId: z.string().min(1).max(255), - refreshToken: z.string().min(1), - clientId: z.string().max(255).optional(), - clientSecret: z.string().optional(), - clientSecretExpiresAt: z - .string() - .refine(value => !Number.isNaN(Date.parse(value)), { message: 'must be a parseable date' }) - .optional(), - tokenEndpoint: z.string().max(2048).optional(), - tokenEndpointAuthMethod: z.string().max(64).optional(), - scopes: z.string().max(2048).optional(), - }) - .strict(); - -export type DepositCredentialsBody = z.infer; diff --git a/packages/workflow-executor/src/http/mcp-oauth-credentials.ts b/packages/workflow-executor/src/http/mcp-oauth-credentials.ts new file mode 100644 index 0000000000..51ac9cf242 --- /dev/null +++ b/packages/workflow-executor/src/http/mcp-oauth-credentials.ts @@ -0,0 +1,57 @@ +import type CredentialEncryption from '../crypto/credential-encryption'; +import type { McpOAuthCredentialInput } from '../stores/mcp-oauth-credentials-store'; + +import { z } from 'zod'; + +// Frontend HTTP body: .strict() rejects unknown keys (incl. user-id smuggling — the JWT is the +// only identity source). String lengths mirror the column limits so oversized input 400s here +// rather than at insert; refreshToken/clientSecret stay unbounded (BLOB columns, parser caps size). +// tokenEndpoint is required — the refresh grant has nowhere to go without it, so a missing one is +// a 400 here rather than a runtime failure once PR2 reads the row. +export const depositCredentialsBodySchema = z + .object({ + mcpServerId: z.string().min(1).max(255), + refreshToken: z.string().min(1), + clientId: z.string().max(255).optional(), + clientSecret: z.string().optional(), + clientSecretExpiresAt: z + .string() + .refine(value => !Number.isNaN(Date.parse(value)), { message: 'must be a parseable date' }) + .optional(), + tokenEndpoint: z.string().min(1).max(2048), + tokenEndpointAuthMethod: z.string().max(64).optional(), + scopes: z.string().max(2048).optional(), + }) + .strict(); + +export type DepositCredentialsBody = z.infer; + +// Translates a validated deposit body into the at-rest record: encrypts the refresh token (and +// client secret when present) and maps optional fields to their nullable columns. Lives outside the +// HTTP layer because it is credential-domain logic, not transport. encrypt() throws +// ExecutorEncryptionKeyMissingError when the key is unset; the caller maps that to a 503. +export function buildMcpOAuthCredentialInput({ + body, + userId, + encryption, +}: { + body: DepositCredentialsBody; + userId: number; + encryption: CredentialEncryption; +}): McpOAuthCredentialInput { + const refreshToken = encryption.encrypt(body.refreshToken); + const clientSecret = body.clientSecret ? encryption.encrypt(body.clientSecret) : null; + + return { + userId, + mcpServerId: body.mcpServerId, + refreshTokenEnc: refreshToken.ciphertext, + clientId: body.clientId ?? null, + clientSecretEnc: clientSecret?.ciphertext ?? null, + clientSecretExpiresAt: body.clientSecretExpiresAt ? new Date(body.clientSecretExpiresAt) : null, + tokenEndpoint: body.tokenEndpoint, + tokenEndpointAuthMethod: body.tokenEndpointAuthMethod ?? null, + scopes: body.scopes ?? null, + encKeyVersion: refreshToken.encKeyVersion, + }; +} diff --git a/packages/workflow-executor/src/stores/mcp-oauth-credentials-store.ts b/packages/workflow-executor/src/stores/mcp-oauth-credentials-store.ts index 0813750964..357b977bda 100644 --- a/packages/workflow-executor/src/stores/mcp-oauth-credentials-store.ts +++ b/packages/workflow-executor/src/stores/mcp-oauth-credentials-store.ts @@ -15,7 +15,7 @@ export interface McpOAuthCredentialInput { clientId?: string | null; clientSecretEnc?: Buffer | null; clientSecretExpiresAt?: Date | null; - tokenEndpoint?: string | null; + tokenEndpoint: string; tokenEndpointAuthMethod?: string | null; scopes?: string | null; encKeyVersion: number; @@ -37,7 +37,7 @@ interface CredentialRow { client_id: string | null; client_secret_enc: Buffer | null; client_secret_expires_at: string | Date | null; - token_endpoint: string | null; + token_endpoint: string; token_endpoint_auth_method: string | null; scopes: string | null; enc_key_version: number; @@ -82,7 +82,7 @@ export default class McpOAuthCredentialsStore { }, tokenEndpoint: { type: DataTypes.STRING(2048), - allowNull: true, + allowNull: false, field: 'token_endpoint', }, tokenEndpointAuthMethod: { @@ -208,7 +208,7 @@ export default class McpOAuthCredentialsStore { clientSecretEnc: row.client_secret_enc ?? null, clientSecretExpiresAt: row.client_secret_expires_at == null ? null : new Date(row.client_secret_expires_at), - tokenEndpoint: row.token_endpoint ?? null, + tokenEndpoint: row.token_endpoint, tokenEndpointAuthMethod: row.token_endpoint_auth_method ?? null, scopes: row.scopes ?? null, encKeyVersion: Number(row.enc_key_version), 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 c73c01ee00..ec9d933cea 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 @@ -305,6 +305,21 @@ describe('POST /mcp-oauth-credentials', () => { expect(store.upsert).not.toHaveBeenCalled(); }); + it('returns 400 when tokenEndpoint is missing', async () => { + const store = createMockStore(); + const server = createServer({ mcpOAuthCredentialsStore: store }); + const token = signToken({ id: 1 }); + const { tokenEndpoint, ...noEndpoint } = validBody; + + const response = await request(server.callback) + .post('/mcp-oauth-credentials') + .set('Authorization', `Bearer ${token}`) + .send(noEndpoint); + + 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/http/mcp-oauth-credentials.test.ts b/packages/workflow-executor/test/http/mcp-oauth-credentials.test.ts new file mode 100644 index 0000000000..aef9726c98 --- /dev/null +++ b/packages/workflow-executor/test/http/mcp-oauth-credentials.test.ts @@ -0,0 +1,82 @@ +import type CredentialEncryption from '../../src/crypto/credential-encryption'; +import type { DepositCredentialsBody } from '../../src/http/mcp-oauth-credentials'; + +import { ExecutorEncryptionKeyMissingError } from '../../src/errors'; +import { buildMcpOAuthCredentialInput } from '../../src/http/mcp-oauth-credentials'; + +function createEncryption(): CredentialEncryption { + return { + encrypt: jest.fn((plaintext: string) => ({ + ciphertext: Buffer.from(`enc(${plaintext})`), + encKeyVersion: 1, + })), + decrypt: jest.fn(), + } as unknown as CredentialEncryption; +} + +const fullBody: DepositCredentialsBody = { + mcpServerId: 'mcp-server-1', + refreshToken: 'refresh-token-xyz', + clientId: 'client-abc', + clientSecret: 'client-secret-123', + clientSecretExpiresAt: '2030-01-02T03:04:05.000Z', + tokenEndpoint: 'https://auth.example.com/token', + tokenEndpointAuthMethod: 'client_secret_post', + scopes: 'read write', +}; + +describe('buildMcpOAuthCredentialInput', () => { + it('encrypts the refresh token and maps the record for the given user', () => { + const encryption = createEncryption(); + + const input = buildMcpOAuthCredentialInput({ body: fullBody, userId: 7, encryption }); + + expect(encryption.encrypt).toHaveBeenCalledWith('refresh-token-xyz'); + expect(input.userId).toBe(7); + expect(input.mcpServerId).toBe('mcp-server-1'); + expect(input.refreshTokenEnc.toString()).toBe('enc(refresh-token-xyz)'); + expect(input.tokenEndpoint).toBe('https://auth.example.com/token'); + expect(input.tokenEndpointAuthMethod).toBe('client_secret_post'); + expect(input.scopes).toBe('read write'); + expect(input.encKeyVersion).toBe(1); + }); + + it('encrypts the client secret and parses the expiry when both are provided', () => { + const encryption = createEncryption(); + + const input = buildMcpOAuthCredentialInput({ body: fullBody, userId: 7, encryption }); + + expect(encryption.encrypt).toHaveBeenCalledWith('client-secret-123'); + expect(input.clientSecretEnc?.toString()).toBe('enc(client-secret-123)'); + expect(input.clientSecretExpiresAt).toEqual(new Date('2030-01-02T03:04:05.000Z')); + }); + + it('leaves optional client fields null for a public / PKCE client', () => { + const encryption = createEncryption(); + const publicBody: DepositCredentialsBody = { + mcpServerId: 'mcp-server-1', + refreshToken: 'refresh-token-xyz', + tokenEndpoint: 'https://auth.example.com/token', + }; + + const input = buildMcpOAuthCredentialInput({ body: publicBody, userId: 7, encryption }); + + expect(encryption.encrypt).toHaveBeenCalledTimes(1); + expect(input.clientId).toBeNull(); + expect(input.clientSecretEnc).toBeNull(); + expect(input.clientSecretExpiresAt).toBeNull(); + expect(input.tokenEndpointAuthMethod).toBeNull(); + expect(input.scopes).toBeNull(); + }); + + it('propagates ExecutorEncryptionKeyMissingError so the caller can fail closed', () => { + const encryption = createEncryption(); + (encryption.encrypt as jest.Mock).mockImplementation(() => { + throw new ExecutorEncryptionKeyMissingError(); + }); + + expect(() => buildMcpOAuthCredentialInput({ body: fullBody, userId: 7, encryption })).toThrow( + ExecutorEncryptionKeyMissingError, + ); + }); +}); diff --git a/packages/workflow-executor/test/stores/mcp-oauth-credentials-store.test.ts b/packages/workflow-executor/test/stores/mcp-oauth-credentials-store.test.ts index e82cf54b50..1707c858e4 100644 --- a/packages/workflow-executor/test/stores/mcp-oauth-credentials-store.test.ts +++ b/packages/workflow-executor/test/stores/mcp-oauth-credentials-store.test.ts @@ -35,7 +35,7 @@ interface CredentialInput { clientId?: string | null; clientSecretEnc?: Buffer | null; clientSecretExpiresAt?: Date | null; - tokenEndpoint?: string | null; + tokenEndpoint: string; tokenEndpointAuthMethod?: string | null; scopes?: string | null; encKeyVersion: number; @@ -239,6 +239,18 @@ describe('McpOAuthCredentialsStore (SQLite)', () => { await expect(store.init()).resolves.toBeUndefined(); }); + it('rejects an insert with a null token_endpoint at the DB level', async () => { + // token_endpoint is NOT NULL: the refresh grant has nowhere to go without it. + await expect( + sequelize.query( + 'INSERT INTO ai_mcp_oauth_credentials ' + + '(user_id, mcp_server_id, refresh_token_enc, enc_key_version, created_at, updated_at) ' + + "VALUES (7, 'mcp-server-1', :blob, 1, :now, :now)", + { replacements: { blob: Buffer.from('no-endpoint'), now: new Date() } }, + ), + ).rejects.toThrow(); + }); + it('enforces the UNIQUE (user_id, mcp_server_id) constraint at the DB level', async () => { // Direct insert bypassing upsert proves the constraint exists in the schema, not just app logic. await store.upsert(makeCredential()); @@ -246,9 +258,16 @@ describe('McpOAuthCredentialsStore (SQLite)', () => { await expect( sequelize.query( 'INSERT INTO ai_mcp_oauth_credentials ' + - '(user_id, mcp_server_id, refresh_token_enc, enc_key_version, created_at, updated_at) ' + - "VALUES (42, 'mcp-server-1', :blob, 1, :now, :now)", - { replacements: { blob: Buffer.from('dup'), now: new Date() } }, + '(user_id, mcp_server_id, refresh_token_enc, token_endpoint, enc_key_version, ' + + 'created_at, updated_at) ' + + "VALUES (42, 'mcp-server-1', :blob, :tokenEndpoint, 1, :now, :now)", + { + replacements: { + blob: Buffer.from('dup'), + tokenEndpoint: 'https://auth.example.com/token', + now: new Date(), + }, + }, ), ).rejects.toThrow(); }); From 270031d9e149177cba4f000efe016ea54edee6bf Mon Sep 17 00:00:00 2001 From: Brian Fox Date: Fri, 12 Jun 2026 15:49:13 +0200 Subject: [PATCH 6/7] docs(workflow-executor): document FOREST_EXECUTOR_ENCRYPTION_KEY in example env The executor reads this key lazily for OAuth MCP credential encryption; add it to the executor's own example env (deployment-facing) and the local agent+executor demo template so it is discoverable where the var is actually consumed. Co-Authored-By: Claude Opus 4.8 (1M context) --- packages/_example/.env.example | 2 ++ packages/workflow-executor/example/.env.example | 5 +++++ 2 files changed, 7 insertions(+) diff --git a/packages/_example/.env.example b/packages/_example/.env.example index 7c4ebaf6e7..afcc920d0c 100644 --- a/packages/_example/.env.example +++ b/packages/_example/.env.example @@ -21,6 +21,8 @@ FOREST_AUTH_SECRET= EXECUTOR_AGENT_URL=http://localhost:3351 WORKFLOW_EXECUTOR_URL=http://localhost:3400 EXECUTOR_DATABASE_URL=postgresql://executor:password@localhost:5459/workflow_executor +# At-rest encryption key for OAuth MCP credentials (openssl rand -hex 32; same value across instances) +FOREST_EXECUTOR_ENCRYPTION_KEY= # when start:with-executor:multiple-instances command # EXECUTOR_AGENT_URL=http://host.docker.internal:3351 # WORKFLOW_EXECUTOR_URL=http://localhost:3400 diff --git a/packages/workflow-executor/example/.env.example b/packages/workflow-executor/example/.env.example index 76e7dfaeb6..9e831df8f4 100644 --- a/packages/workflow-executor/example/.env.example +++ b/packages/workflow-executor/example/.env.example @@ -2,6 +2,11 @@ FOREST_ENV_SECRET= FOREST_AUTH_SECRET= +# At-rest encryption key for OAuth MCP credentials (HKDF-derived, AES-256-GCM). +# Generate: openssl rand -hex 32. Set the SAME value across all executor instances. +# Read lazily — only required once OAuth-protected MCP servers are used. +FOREST_EXECUTOR_ENCRYPTION_KEY= + # Your locally running Forest Admin agent AGENT_URL=http://localhost:3351 From d7cee9a27f713b7d9b41fa8a4ce4d23f897531e9 Mon Sep 17 00:00:00 2001 From: Brian Fox Date: Fri, 12 Jun 2026 15:51:14 +0200 Subject: [PATCH 7/7] docs(workflow-executor): add FOREST_EXECUTOR_ENCRYPTION_KEY to env reference MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Document the key in the package README env-vars table — the reference the example README points to — so it's discoverable alongside the other executor env vars, not only in .env.example. Co-Authored-By: Claude Opus 4.8 (1M context) --- packages/workflow-executor/README.md | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/workflow-executor/README.md b/packages/workflow-executor/README.md index 31cdd752ac..1ff8ab25bc 100644 --- a/packages/workflow-executor/README.md +++ b/packages/workflow-executor/README.md @@ -26,6 +26,7 @@ npm install -g @forestadmin/workflow-executor | `FOREST_SERVER_URL` | — | `https://api.forestadmin.com` | Orchestrator URL | | `POLLING_INTERVAL_MS` | — | `5000` | Poll cadence for pending steps | | `STOP_TIMEOUT_MS` | — | `30000` | Graceful shutdown deadline | +| `FOREST_EXECUTOR_ENCRYPTION_KEY` | —† | — | At-rest key for OAuth MCP credentials (HKDF-derived, AES-256-GCM). Generate with `openssl rand -hex 32`; set the **same** value across all executor instances. (†Read lazily — required only once OAuth-protected MCP servers are used.) | Optional AI configuration (all-or-nothing — falls back to server AI if any is missing):