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 <clio-agent@sisyphuslabs.ai>
This commit is contained in:
@@ -142,24 +142,56 @@ describe("createCleanMcpEnvironment", () => {
|
|||||||
expect(cleanEnv.NODE_ENV).toBe("production")
|
expect(cleanEnv.NODE_ENV).toBe("production")
|
||||||
})
|
})
|
||||||
|
|
||||||
it("filters secret keys from customEnv that would bypass process.env filtering", () => {
|
it("passes through secret-named keys from customEnv without filtering", () => {
|
||||||
// given - customEnv tries to inject secrets that should be filtered
|
// given - custom env values are explicitly declared in skill config
|
||||||
process.env.PATH = "/usr/bin"
|
process.env.PATH = "/usr/bin"
|
||||||
|
process.env.MCP_API_KEY = "ambient-secret-from-process-env"
|
||||||
const customEnv = {
|
const customEnv = {
|
||||||
MCP_API_KEY: "secret-key-that-should-be-filtered",
|
MCP_API_KEY: "skill-declared-api-key",
|
||||||
CUSTOM_SECRET: "another-secret",
|
TELEGRAM_BOT_TOKEN: "skill-configured-bot-token",
|
||||||
SAFE_VAR: "safe-value",
|
SAFE_VAR: "safe-value",
|
||||||
}
|
}
|
||||||
|
|
||||||
// when
|
// when
|
||||||
const cleanEnv = createCleanMcpEnvironment(customEnv)
|
const cleanEnv = createCleanMcpEnvironment(customEnv)
|
||||||
|
|
||||||
// then - secret keys from customEnv are filtered despite not being in process.env
|
// then - custom env overrides ambient values and is not filtered
|
||||||
expect(cleanEnv.MCP_API_KEY).toBeUndefined()
|
expect(cleanEnv.MCP_API_KEY).toBe("skill-declared-api-key")
|
||||||
expect(cleanEnv.CUSTOM_SECRET).toBeUndefined()
|
expect(cleanEnv.TELEGRAM_BOT_TOKEN).toBe("skill-configured-bot-token")
|
||||||
expect(cleanEnv.SAFE_VAR).toBe("safe-value")
|
expect(cleanEnv.SAFE_VAR).toBe("safe-value")
|
||||||
expect(cleanEnv.PATH).toBe("/usr/bin")
|
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", () => {
|
describe("undefined value handling", () => {
|
||||||
|
|||||||
@@ -35,25 +35,25 @@ export const EXCLUDED_ENV_PATTERNS: RegExp[] = [
|
|||||||
/_API_KEY$/i,
|
/_API_KEY$/i,
|
||||||
]
|
]
|
||||||
|
|
||||||
|
function isExcludedEnvKey(key: string): boolean {
|
||||||
|
return EXCLUDED_ENV_PATTERNS.some((pattern) => pattern.test(key))
|
||||||
|
}
|
||||||
|
|
||||||
export function createCleanMcpEnvironment(
|
export function createCleanMcpEnvironment(
|
||||||
customEnv: Record<string, string> = {}
|
customEnv: Record<string, string> = {}
|
||||||
): Record<string, string> {
|
): Record<string, string> {
|
||||||
const mergedEnv: Record<string, string> = {}
|
const cleanEnv: Record<string, string> = {}
|
||||||
|
|
||||||
|
// 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)) {
|
for (const [key, value] of Object.entries(process.env)) {
|
||||||
if (value === undefined) continue
|
if (value === undefined) continue
|
||||||
mergedEnv[key] = value
|
if (isExcludedEnvKey(key)) continue
|
||||||
|
cleanEnv[key] = value
|
||||||
}
|
}
|
||||||
|
|
||||||
Object.assign(mergedEnv, customEnv)
|
Object.assign(cleanEnv, customEnv)
|
||||||
|
|
||||||
const cleanEnv: Record<string, string> = {}
|
|
||||||
for (const [key, value] of Object.entries(mergedEnv)) {
|
|
||||||
const shouldExclude = EXCLUDED_ENV_PATTERNS.some((pattern) => pattern.test(key))
|
|
||||||
if (!shouldExclude) {
|
|
||||||
cleanEnv[key] = value
|
|
||||||
}
|
|
||||||
}
|
|
||||||
|
|
||||||
return cleanEnv
|
return cleanEnv
|
||||||
}
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user