From 22aadd686340e9e7ea2dee2629c75d20c2cbfebb Mon Sep 17 00:00:00 2001 From: jangByeongHui Date: Tue, 19 May 2026 11:31:55 +0900 Subject: [PATCH] fix(skill-mcp-manager): trust explicit skill MCP env vars (#3995) Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus --- .../skill-mcp-manager/env-cleaner.test.ts | 46 ++++++++++++++++--- src/features/skill-mcp-manager/env-cleaner.ts | 22 ++++----- 2 files changed, 50 insertions(+), 18 deletions(-) diff --git a/src/features/skill-mcp-manager/env-cleaner.test.ts b/src/features/skill-mcp-manager/env-cleaner.test.ts index 44997ad11..d4fbc1a57 100644 --- a/src/features/skill-mcp-manager/env-cleaner.test.ts +++ b/src/features/skill-mcp-manager/env-cleaner.test.ts @@ -142,24 +142,56 @@ describe("createCleanMcpEnvironment", () => { expect(cleanEnv.NODE_ENV).toBe("production") }) - it("filters secret keys from customEnv that would bypass process.env filtering", () => { - // given - customEnv tries to inject secrets that should be filtered + it("passes through secret-named keys from customEnv without filtering", () => { + // given - custom env values are explicitly declared in skill config process.env.PATH = "/usr/bin" + process.env.MCP_API_KEY = "ambient-secret-from-process-env" const customEnv = { - MCP_API_KEY: "secret-key-that-should-be-filtered", - CUSTOM_SECRET: "another-secret", + MCP_API_KEY: "skill-declared-api-key", + TELEGRAM_BOT_TOKEN: "skill-configured-bot-token", SAFE_VAR: "safe-value", } // when const cleanEnv = createCleanMcpEnvironment(customEnv) - // then - secret keys from customEnv are filtered despite not being in process.env - expect(cleanEnv.MCP_API_KEY).toBeUndefined() - expect(cleanEnv.CUSTOM_SECRET).toBeUndefined() + // then - custom env overrides ambient values and is not filtered + expect(cleanEnv.MCP_API_KEY).toBe("skill-declared-api-key") + expect(cleanEnv.TELEGRAM_BOT_TOKEN).toBe("skill-configured-bot-token") expect(cleanEnv.SAFE_VAR).toBe("safe-value") expect(cleanEnv.PATH).toBe("/usr/bin") }) + + it("passes TELEGRAM_BOT_TOKEN through when declared in skill env (issue #3995)", () => { + // given - TELEGRAM_BOT_TOKEN matches /_TOKEN$/i but is explicitly declared + process.env.TELEGRAM_BOT_TOKEN = "ambient-bot-token" + const customEnv = { + TELEGRAM_BOT_TOKEN: "skill-bot-token", + } + + // when + const cleanEnv = createCleanMcpEnvironment(customEnv) + + // then + expect(cleanEnv.TELEGRAM_BOT_TOKEN).toBe("skill-bot-token") + }) + + it("still filters secret-named vars from process.env when customEnv is provided", () => { + // given + process.env.AMBIENT_API_KEY = "ambient-secret" + process.env.PATH = "/usr/bin" + const customEnv = { + SAFE_CUSTOM_VAR: "custom-value", + } + + // when + const cleanEnv = createCleanMcpEnvironment(customEnv) + + // then + expect(cleanEnv.AMBIENT_API_KEY).toBeUndefined() + expect(cleanEnv.PATH).toBe("/usr/bin") + expect(cleanEnv.SAFE_CUSTOM_VAR).toBe("custom-value") + }) }) describe("undefined value handling", () => { diff --git a/src/features/skill-mcp-manager/env-cleaner.ts b/src/features/skill-mcp-manager/env-cleaner.ts index 643b89c18..41ca50bfa 100644 --- a/src/features/skill-mcp-manager/env-cleaner.ts +++ b/src/features/skill-mcp-manager/env-cleaner.ts @@ -35,25 +35,25 @@ export const EXCLUDED_ENV_PATTERNS: RegExp[] = [ /_API_KEY$/i, ] +function isExcludedEnvKey(key: string): boolean { + return EXCLUDED_ENV_PATTERNS.some((pattern) => pattern.test(key)) +} + export function createCleanMcpEnvironment( customEnv: Record = {} ): Record { - const mergedEnv: Record = {} + const cleanEnv: Record = {} + // Apply the blacklist only to inherited ambient environment variables. + // Skill-configured env entries are explicitly declared and must be passed + // through as-is so stdio MCP servers can receive required credentials. for (const [key, value] of Object.entries(process.env)) { if (value === undefined) continue - mergedEnv[key] = value + if (isExcludedEnvKey(key)) continue + cleanEnv[key] = value } - Object.assign(mergedEnv, customEnv) - - const cleanEnv: Record = {} - for (const [key, value] of Object.entries(mergedEnv)) { - const shouldExclude = EXCLUDED_ENV_PATTERNS.some((pattern) => pattern.test(key)) - if (!shouldExclude) { - cleanEnv[key] = value - } - } + Object.assign(cleanEnv, customEnv) return cleanEnv }