diff --git a/apps/worker/src/commands/resume.ts b/apps/worker/src/commands/resume.ts index 017b729cf..611e8390d 100644 --- a/apps/worker/src/commands/resume.ts +++ b/apps/worker/src/commands/resume.ts @@ -41,6 +41,7 @@ export async function resume(runId: number): Promise { }) => buildWorkspaceConfig({ environmentId, repo, selectedRepositories }), runFn: async ({ jobContext, + userEnvVars, workspace, workspacePath, usesSharedWorkspaceRoot, @@ -88,6 +89,7 @@ export async function resume(runId: number): Promise { return runTask({ ...jobContext, envVars: jobContext.envVars, + userEnvVars, workspacePath, prompt: '', harnessInstructions: jobContext.harnessInstructions, diff --git a/apps/worker/src/commands/run.ts b/apps/worker/src/commands/run.ts index 13aa243de..0076a71b3 100644 --- a/apps/worker/src/commands/run.ts +++ b/apps/worker/src/commands/run.ts @@ -49,6 +49,7 @@ export async function run({ }), runFn: async ({ jobContext, + userEnvVars, workspace, workspacePath, usesSharedWorkspaceRoot, @@ -65,6 +66,7 @@ export async function run({ return runTask({ ...jobContext, envVars: jobContext.envVars, + userEnvVars, workspacePath, prompt: jobContext.prompt, harnessInstructions: jobContext.harnessInstructions, diff --git a/apps/worker/src/commands/setup/__tests__/setup-mcps.test.ts b/apps/worker/src/commands/setup/__tests__/setup-mcps.test.ts index b1811906f..aa7a37b4a 100644 --- a/apps/worker/src/commands/setup/__tests__/setup-mcps.test.ts +++ b/apps/worker/src/commands/setup/__tests__/setup-mcps.test.ts @@ -128,6 +128,192 @@ describe('resolveBuiltInMcpServers', () => { expect(docsConfig.headers['X-MCP-Region']).toBe('us-east-1'); }); + it('substitutes operator-defined vars regardless of name shape', () => { + const warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}); + + const parsed = { + mcpServers: resolveBuiltInMcpServers( + { + REDDIT_CLIENT_SECRET: 'super-secret', + }, + undefined, + { + reddit: { + command: 'npx', + args: ['-y', 'reddit-mcp-buddy'], + env: { + REDDIT_CLIENT_SECRET: '${REDDIT_CLIENT_SECRET}', + }, + }, + }, + { REDDIT_CLIENT_SECRET: 'super-secret' }, + ), + }; + + const redditConfig = parsed.mcpServers.reddit as { + env: Record; + }; + expect(redditConfig.env.REDDIT_CLIENT_SECRET).toBe('super-secret'); + expect(warnSpy).not.toHaveBeenCalled(); + }); + + it('substitutes non-reserved task env vars even with secret-like names', () => { + const parsed = { + mcpServers: resolveBuiltInMcpServers( + { + MY_APP_PRIVATE_KEY: 'pem-content', + }, + undefined, + { + internal: { + command: 'npx', + args: ['-y', '@acme/internal-mcp'], + env: { + PRIVATE_KEY: '${MY_APP_PRIVATE_KEY}', + }, + }, + }, + ), + }; + + const internalConfig = parsed.mcpServers.internal as { + env: Record; + }; + expect(internalConfig.env.PRIVATE_KEY).toBe('pem-content'); + }); + + it('refuses reserved Roomote runtime names and warns', () => { + const warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}); + + const parsed = { + mcpServers: resolveBuiltInMcpServers( + { + ROOMOTE_CLOUD_TOKEN: 'runtime-token', + }, + undefined, + { + exfil: { + url: 'https://mcp.example.com/collect', + headers: { + Authorization: 'Bearer ${ROOMOTE_CLOUD_TOKEN}', + }, + }, + }, + ), + }; + + const exfilConfig = parsed.mcpServers.exfil as { + headers: Record; + }; + expect(exfilConfig.headers.Authorization).toBe( + 'Bearer ${ROOMOTE_CLOUD_TOKEN}', + ); + expect(warnSpy).toHaveBeenCalledWith( + expect.stringContaining( + "Custom MCP 'exfil' headers: ${ROOMOTE_CLOUD_TOKEN} was NOT substituted", + ), + ); + }); + + it('never substitutes Roomote-namespaced names, even from the operator overlay', () => { + const warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}); + + // Regression: runtime code injects values like ROOMOTE_AUTH_BYPASS_VALUE + // into env maps after dequeue. Even if such an entry reaches the operator + // overlay, it must not become substitutable. + const parsed = { + mcpServers: resolveBuiltInMcpServers( + { + ROOMOTE_AUTH_BYPASS_VALUE: 'bypass-token', + }, + undefined, + { + exfil: { + url: 'https://mcp.example.com/collect', + headers: { + 'X-Bypass': '${ROOMOTE_AUTH_BYPASS_VALUE}', + }, + }, + }, + { ROOMOTE_AUTH_BYPASS_VALUE: 'bypass-token' }, + ), + }; + + const exfilConfig = parsed.mcpServers.exfil as { + headers: Record; + }; + expect(exfilConfig.headers['X-Bypass']).toBe( + '${ROOMOTE_AUTH_BYPASS_VALUE}', + ); + expect(warnSpy).toHaveBeenCalledWith( + expect.stringContaining( + "Custom MCP 'exfil' headers: ${ROOMOTE_AUTH_BYPASS_VALUE} was NOT substituted", + ), + ); + }); + + it('resolves reserved-name collisions to the operator-defined value', () => { + const parsed = { + mcpServers: resolveBuiltInMcpServers( + { + DATABASE_URL: 'postgres://internal-control-plane/db', + }, + undefined, + { + internal: { + command: 'npx', + args: ['-y', '@acme/internal-mcp'], + env: { + DATABASE_URL: '${DATABASE_URL}', + }, + }, + }, + { DATABASE_URL: 'postgres://operator-app/db' }, + ), + }; + + const internalConfig = parsed.mcpServers.internal as { + env: Record; + }; + expect(internalConfig.env.DATABASE_URL).toBe('postgres://operator-app/db'); + }); + + it('warns when a custom MCP env reference is not defined in the task env', () => { + const warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}); + + resolveBuiltInMcpServers({ MCP_REGION: 'us-east-1' }, undefined, { + internal: { + command: 'npx', + args: ['-y', '@acme/internal-mcp'], + env: { + API_KEY: '${MCP_API_KYE}', + }, + }, + }); + + expect(warnSpy).toHaveBeenCalledWith( + expect.stringContaining( + "Custom MCP 'internal' env: ${MCP_API_KYE} is not defined in the task environment", + ), + ); + }); + + it('does not warn when custom MCP references all resolve', () => { + const warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}); + + resolveBuiltInMcpServers({ MCP_API_KEY: 'secret123' }, undefined, { + internal: { + command: 'npx', + args: ['-y', '@acme/internal-mcp'], + env: { + API_KEY: '${MCP_API_KEY}', + }, + }, + }); + + expect(warnSpy).not.toHaveBeenCalled(); + }); + it('leaves unresolved custom streamable HTTP MCP headers intact', () => { const parsed = { mcpServers: resolveBuiltInMcpServers( diff --git a/apps/worker/src/commands/setup/setup-mcps.ts b/apps/worker/src/commands/setup/setup-mcps.ts index c0a326e50..bfcaf6e1f 100644 --- a/apps/worker/src/commands/setup/setup-mcps.ts +++ b/apps/worker/src/commands/setup/setup-mcps.ts @@ -6,7 +6,7 @@ import { type EnvironmentMcpServers, } from '@roomote/types'; -import { substituteEnvVars } from '../../env'; +import { collectEnvVarReferences, substituteEnvVars } from '../../env'; // The Roomote MCP server is compiled into the worker's dist directory. // Resolve its path relative to the running worker script (process.argv[1]). @@ -60,51 +60,125 @@ interface IntegrationProxyConfig { upstreamPath?: string; } -function isRestrictedMcpEnvVarName(name: string): boolean { - // Roomote runtime / control-plane secrets must not be injectable into - // operator-configured MCP server env via ${...} substitution. - if ( +/** + * Names that belong unambiguously to the Roomote runtime. These never + * substitute into operator-configured MCP config — not even from the + * operator-provided overlay — so a value injected into an env map by the + * runtime can never be reclassified as operator-owned. + */ +function isRoomoteNamespacedEnvVarName(name: string): boolean { + return ( name === 'AUTH_TOKEN' || name === 'BASH_ENV' || - name === 'DATABASE_URL' || - name === 'REDIS_URL' || name.startsWith('ROOMOTE_') || name.startsWith('JOB_AUTH_') || name.startsWith('PREVIEW_AUTH_') - ) { - return true; - } + ); +} - return /_SECRET$/i.test(name) || /_PRIVATE_KEY$/i.test(name); +/** + * Roomote runtime / control-plane values that must never be injectable into + * operator-configured MCP server config via ${...} substitution from the + * task env. This is deliberately limited to Roomote-internal names: anything + * the operator defined themselves (deployment env vars) is already present + * in the sandbox environment, so refusing to substitute it would add + * friction without protecting anything. Operator-defined names substitute + * via the overlay in buildMcpSubstitutionLookup; generic reserved names + * (DATABASE_URL, REDIS_URL) can be shadowed there by an operator's own + * value, Roomote-namespaced names cannot. + */ +function isReservedRuntimeMcpEnvVarName(name: string): boolean { + return ( + isRoomoteNamespacedEnvVarName(name) || + name === 'DATABASE_URL' || + name === 'REDIS_URL' + ); } -function filterMcpEnvLookup( - lookup: Record | undefined, -): Record | undefined { - if (!lookup) { - return undefined; - } +/** + * Build the ${...} substitution lookup for custom MCP config: the task env + * minus reserved runtime names, overlaid with operator-defined deployment + * vars. Operator values win on collision, so an operator var that happens to + * share a generic reserved name (for example their own DATABASE_URL) + * resolves to the operator's value — never to a Roomote-internal one. + * Roomote-namespaced names are dropped even from the operator overlay as + * defense in depth against runtime-injected entries. + */ +function buildMcpSubstitutionLookup( + taskEnv: Record | undefined, + operatorEnvVars: Record | undefined, +): Record { + return { + ...Object.fromEntries( + Object.entries(taskEnv ?? {}).filter( + ([name]) => !isReservedRuntimeMcpEnvVarName(name), + ), + ), + ...Object.fromEntries( + Object.entries(operatorEnvVars ?? {}).filter( + ([name]) => !isRoomoteNamespacedEnvVarName(name), + ), + ), + }; +} - return Object.fromEntries( - Object.entries(lookup).filter(([key]) => !isRestrictedMcpEnvVarName(key)), - ); +/** + * Warn about `${VAR}` references that will not be substituted, so a refused + * or misspelled reference fails loudly instead of reaching the MCP server as + * a literal `${VAR}` string with no trace anywhere. + */ +function warnUnresolvableConfigReferences(options: { + serverName: string; + field: 'env' | 'headers'; + values: Record; + lookup: Record; +}): void { + const warnedNames = new Set(); + + for (const value of Object.values(options.values)) { + for (const name of collectEnvVarReferences(value)) { + if (warnedNames.has(name) || name in options.lookup) { + continue; + } + + warnedNames.add(name); + + if (isReservedRuntimeMcpEnvVarName(name)) { + console.warn( + `[resolveBuiltInMcpServers] Custom MCP '${options.serverName}' ${options.field}: ` + + `\${${name}} was NOT substituted because the name is a reserved ` + + `Roomote runtime name. Define your own deployment environment ` + + `variable under a different name and reference that instead; ` + + `the literal text was passed through.`, + ); + } else { + console.warn( + `[resolveBuiltInMcpServers] Custom MCP '${options.serverName}' ${options.field}: ` + + `\${${name}} is not defined in the task environment; the literal ` + + `reference was passed through unchanged.`, + ); + } + } + } } function resolveConfigValues( values: Record | undefined, - lookup: Record | undefined, + lookup: Record, + context: { serverName: string; field: 'env' | 'headers' }, ): Record | undefined { if (!values) { return undefined; } - const safeLookup = filterMcpEnvLookup(lookup); - - if (!safeLookup) { - return values; - } + warnUnresolvableConfigReferences({ + serverName: context.serverName, + field: context.field, + values, + lookup, + }); - return substituteEnvVars(values, safeLookup); + return substituteEnvVars(values, lookup); } function buildIntegrationProxyMap(): Map { @@ -240,6 +314,7 @@ export function resolveBuiltInMcpServers( taskEnv?: Record, integrations?: IntegrationMcpOptions, environmentMcpServers?: EnvironmentMcpServers, + operatorEnvVars?: Record, ): Record { // The extension's StdioClientTransport only inherits a minimal set of // env vars (HOME, PATH, SHELL, TERM, USER) via getDefaultEnvironment(). @@ -359,6 +434,11 @@ export function resolveBuiltInMcpServers( // Add environment-specific MCP servers. // Built-ins and integration MCPs take precedence over custom names. if (environmentMcpServers) { + const substitutionLookup = buildMcpSubstitutionLookup( + taskEnv, + operatorEnvVars, + ); + for (const [name, config] of Object.entries(environmentMcpServers)) { if (resolvedMcps[name]) { console.warn( @@ -374,14 +454,20 @@ export function resolveBuiltInMcpServers( args: config.args, env: { ...stdioEnvExtras, - ...resolveConfigValues(config.env, taskEnv), + ...resolveConfigValues(config.env, substitutionLookup, { + serverName: name, + field: 'env', + }), }, }; } else { resolvedMcps[name] = { type: 'streamable-http', url: config.url, - headers: resolveConfigValues(config.headers, taskEnv), + headers: resolveConfigValues(config.headers, substitutionLookup, { + serverName: name, + field: 'headers', + }), }; } } diff --git a/apps/worker/src/commands/utils/execute-task-run.ts b/apps/worker/src/commands/utils/execute-task-run.ts index 0c87868ca..f08420aa7 100644 --- a/apps/worker/src/commands/utils/execute-task-run.ts +++ b/apps/worker/src/commands/utils/execute-task-run.ts @@ -72,6 +72,11 @@ interface ExecuteTaskRunConfig { workspaceConfigFn: (jobContext: TJobContext) => Promise; runFn: (params: { jobContext: TJobContext; + /** + * Snapshot of the dequeue-provided env vars taken before injectEnvVars + * adds runtime-internal entries. This is the operator-owned set. + */ + userEnvVars: Record; workspace: WorkspaceConfig; workspacePath: string; usesSharedWorkspaceRoot: boolean; @@ -661,6 +666,7 @@ export async function executeTaskRun({ const runTaskPromise = runFn({ jobContext, + userEnvVars, workspace, workspacePath, usesSharedWorkspaceRoot, diff --git a/apps/worker/src/env/index.ts b/apps/worker/src/env/index.ts index 5c05dc3e9..7f740d674 100644 --- a/apps/worker/src/env/index.ts +++ b/apps/worker/src/env/index.ts @@ -1,2 +1,2 @@ export { WorkerEnv } from './worker-env'; -export { substituteEnvVars } from './substitute'; +export { collectEnvVarReferences, substituteEnvVars } from './substitute'; diff --git a/apps/worker/src/env/substitute.ts b/apps/worker/src/env/substitute.ts index 5f5896521..d71b095d4 100644 --- a/apps/worker/src/env/substitute.ts +++ b/apps/worker/src/env/substitute.ts @@ -12,6 +12,9 @@ * - Lookup is explicitly provided, never reads process.env. * - Unresolved references are left intact for visibility. */ +const ENV_VAR_REFERENCE_PATTERN = + /\$\{([A-Za-z_][A-Za-z0-9_]*)\}|\$([A-Za-z_][A-Za-z0-9_]*)/g; + export function substituteEnvVars( vars: Record, lookup: Record, @@ -20,7 +23,7 @@ export function substituteEnvVars( for (const [key, value] of Object.entries(vars)) { result[key] = value.replace( - /\$\{([A-Za-z_][A-Za-z0-9_]*)\}|\$([A-Za-z_][A-Za-z0-9_]*)/g, + ENV_VAR_REFERENCE_PATTERN, (match, braced: string | undefined, bare: string | undefined) => { const varName = braced ?? bare!; return varName in lookup ? lookup[varName]! : match; @@ -30,3 +33,11 @@ export function substituteEnvVars( return result; } + +/** List the $VAR / ${VAR} names a value references, in order of appearance. */ +export function collectEnvVarReferences(value: string): string[] { + return Array.from( + value.matchAll(ENV_VAR_REFERENCE_PATTERN), + (match) => (match[1] ?? match[2])!, + ); +} diff --git a/apps/worker/src/run-task/create-harness.ts b/apps/worker/src/run-task/create-harness.ts index e12200d1f..aca24970a 100644 --- a/apps/worker/src/run-task/create-harness.ts +++ b/apps/worker/src/run-task/create-harness.ts @@ -32,6 +32,11 @@ interface CreateHarnessOptions { integrations: IntegrationMcpOptions; mcpTaskEnv: Record; environmentMcpServers?: EnvironmentMcpServers; + /** + * Operator-defined deployment env vars. Always eligible for ${...} + * substitution in custom MCP config, regardless of variable name. + */ + operatorEnvVars?: Record; taskRun: DequeuedTaskRun['taskRun']; developerInstructionsContent?: string; callbacks: RunTaskCallbacks; @@ -61,6 +66,7 @@ export async function createHarness({ integrations, mcpTaskEnv, environmentMcpServers, + operatorEnvVars, taskRun, developerInstructionsContent, callbacks, @@ -91,6 +97,7 @@ export async function createHarness({ mcpTaskEnv, integrations, environmentMcpServers, + operatorEnvVars, ); const modelOverride = taskRun.payload?.harnessModelOverrides ? getHarnessModelOverride( diff --git a/apps/worker/src/run-task/run-task.ts b/apps/worker/src/run-task/run-task.ts index 9f7fa7a0e..476a28078 100644 --- a/apps/worker/src/run-task/run-task.ts +++ b/apps/worker/src/run-task/run-task.ts @@ -573,6 +573,7 @@ const CODE_MODE_FLAG = FeatureFlag.CodeMode; export const runTask = async ({ taskRun, envVars, + userEnvVars, workspacePath, usesSharedWorkspaceRoot, repoPaths, @@ -1155,6 +1156,15 @@ export const runTask = async ({ integrations, mcpTaskEnv, environmentMcpServers: environmentConfig?.mcpServers, + // The pre-injection snapshot, NOT deploymentEnvVars: by harness start, + // `envVars` has been mutated with runtime-internal entries (auth bypass + // values, BASH_ENV, ...) that must not ride the operator overlay past + // the reserved-name guard. + operatorEnvVars: Object.fromEntries( + Object.entries(userEnvVars ?? {}).filter( + (entry): entry is [string, string] => entry[1] !== undefined, + ), + ), taskRun, developerInstructionsContent: harnessDeveloperInstructions, callbacks, diff --git a/apps/worker/src/run-task/types.ts b/apps/worker/src/run-task/types.ts index 2e8270279..cfd70811a 100644 --- a/apps/worker/src/run-task/types.ts +++ b/apps/worker/src/run-task/types.ts @@ -150,6 +150,14 @@ export type RunTaskCallbacks = { export type RunTaskOptions = { taskRun: DequeuedTaskRun['taskRun']; envVars: Record; + /** + * Snapshot of the dequeue-provided env vars taken before injectEnvVars adds + * runtime-internal entries (auth bypass values, BASH_ENV, ...). This is the + * operator-owned set used for custom MCP config `${...}` substitution; + * `envVars` must not be used for that because post-injection it can + * reclassify internal values as operator-provided. + */ + userEnvVars?: Record; workspacePath: string; usesSharedWorkspaceRoot?: boolean; repoPaths?: Record;