luna/oh-my-opencode
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Merge pull request #3049 from code-yeongyu/fix/p0-5-mcp-env-expansion

Browse Source 
fix(mcp): warn on blocked env expansion
This commit is contained in:
YeonGyu-Kim
2026-04-03 18:52:32 +09:00
committed by GitHub
parent ac5e507155 71b8110ff0
commit 0c6aa91fc8
14 changed files with 459 additions and 195 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/features/claude-code-mcp-loader/configure-allowed-env-vars.ts
+25 -1
View File
@@ -1,4 +1,28 @@
const BUILTIN_ALLOWED_MCP_ENV_VARS = ["PATH", "HOME", "USER", "SHELL", "TERM"]
const BUILTIN_ALLOWED_MCP_ENV_VARS = [
"PATH",
"HOME",
"USER",
"SHELL",
"TERM",
"TMPDIR",
"TMP",
"TEMP",
"PWD",
"OLDPWD",
"LANG",
"LC_ALL",
"LC_CTYPE",
"EDITOR",
"VISUAL",
"XDG_CONFIG_HOME",
"XDG_DATA_HOME",
"XDG_CACHE_HOME",
"HOSTNAME",
"LOGNAME",
"USERPROFILE",
"APPDATA",
"LOCALAPPDATA",
]
const SENSITIVE_MCP_ENV_VAR_PATTERN = /KEY|TOKEN|SECRET|PASSWORD|AUTH|CREDENTIAL/i
let additionalAllowedMcpEnvVars = new Set<string>()
src/features/claude-code-mcp-loader/env-expander.test.ts
+39
View File
@@ -42,6 +42,45 @@ describe("expandEnvVars", () => {
})
})
describe("#given a benign environment variable in the builtin allowlist", () => {
it("#when expanding the value #then it returns the env value", () => {
// given
process.env.TMPDIR = "/tmp/omo"
process.env.TEMP = "C:\\Temp"
process.env.USERPROFILE = "C:\\Users\\tester"
process.env.LANG = "en_US.UTF-8"
process.env.XDG_CONFIG_HOME = "/Users/tester/.config"
// when
const expanded = expandEnvVars(
"${TMPDIR}|${TEMP}|${USERPROFILE}|${LANG}|${XDG_CONFIG_HOME}"
)
// then
expect(expanded).toBe(
"/tmp/omo|C:\\Temp|C:\\Users\\tester|en_US.UTF-8|/Users/tester/.config"
)
})
})
describe("#given a blocked non-sensitive environment variable reference", () => {
it("#when expanding the value #then it returns an empty string and logs a warning", () => {
// given
process.env.PROJECT_ROOT = "/Users/tester/project"
const logSpy = spyOn(shared, "log").mockImplementation(() => {})
// when
const expanded = expandEnvVars("${PROJECT_ROOT}")
// then
expect(expanded).toBe("")
expect(logSpy).toHaveBeenCalledWith(
expect.stringContaining("Blocked MCP env var expansion"),
expect.objectContaining({ varName: "PROJECT_ROOT" })
)
})
})
describe("#given a blocked variable with a default value", () => {
it("#when expanding the value #then it uses the default instead of the sensitive env var", () => {
// given
src/features/claude-code-mcp-loader/env-expander.ts
+7 -5
View File
@@ -9,11 +9,13 @@ export function expandEnvVars(value: string): string {
/\$\{([^}:]+)(?::-([^}]*))?\}/g,
(_, varName: string, defaultValue?: string) => {
if (!isAllowedMcpEnvVar(varName)) {
if (isSensitiveMcpEnvVar(varName)) {
log(`Blocked MCP env var expansion for sensitive variable "${varName}"`, {
varName,
})
}
const isSensitive = isSensitiveMcpEnvVar(varName)
const reason = isSensitive ? "sensitive variable" : "not in allowlist"
log(`Blocked MCP env var expansion for ${reason} "${varName}"`, {
varName,
sensitive: isSensitive,
})
if (defaultValue !== undefined) return defaultValue
return ""
src/plugin-config.test.ts
+47 -2
View File
@@ -1,7 +1,21 @@
import { describe, expect, it } from "bun:test";
import { mergeConfigs, parseConfigPartially } from "./plugin-config";
import { afterEach, describe, expect, it, mock, spyOn } from "bun:test";
import { mkdtempSync, mkdirSync, rmSync, writeFileSync } from "node:fs"
import { tmpdir } from "node:os"
import { join } from "node:path"
import * as shared from "./shared"
import { loadPluginConfig, mergeConfigs, parseConfigPartially } from "./plugin-config";
import { OhMyOpenCodeConfigSchema, type OhMyOpenCodeConfig } from "./config";
const tempDirs: string[] = []
afterEach(() => {
mock.restore()
for (const dir of tempDirs.splice(0)) {
rmSync(dir, { recursive: true, force: true })
}
})
describe("mergeConfigs", () => {
describe("categories merging", () => {
// given base config has categories, override has different categories
@@ -277,3 +291,34 @@ describe("parseConfigPartially", () => {
});
});
});
describe("loadPluginConfig", () => {
it("should only honor mcp_env_allowlist from user config", () => {
// given
const rootDir = mkdtempSync(join(tmpdir(), "omo-plugin-config-"))
const userConfigDir = join(rootDir, "user-config")
const projectDir = join(rootDir, "project")
const projectConfigDir = join(projectDir, ".opencode")
tempDirs.push(rootDir)
mkdirSync(userConfigDir, { recursive: true })
mkdirSync(projectConfigDir, { recursive: true })
writeFileSync(
join(userConfigDir, "oh-my-openagent.jsonc"),
JSON.stringify({ mcp_env_allowlist: ["USER_ONLY_TOKEN"] })
)
writeFileSync(
join(projectConfigDir, "oh-my-openagent.jsonc"),
JSON.stringify({ mcp_env_allowlist: ["PROJECT_TOKEN"] })
)
spyOn(shared, "getOpenCodeConfigDir").mockReturnValue(userConfigDir)
// when
const config = loadPluginConfig(projectDir, {})
// then
expect(config.mcp_env_allowlist).toEqual(["USER_ONLY_TOKEN"])
})
})
src/plugin-config.ts
+3 -1
View File
@@ -210,8 +210,9 @@ export function loadPluginConfig(
}
// Load user config first (base). Parse empty config through Zod to apply field defaults.
const userConfig = loadConfigFromPath(userConfigPath, ctx)
let config: OhMyOpenCodeConfig =
loadConfigFromPath(userConfigPath, ctx) ?? OhMyOpenCodeConfigSchema.parse({});
userConfig ?? OhMyOpenCodeConfigSchema.parse({});
// Override with project config
const projectConfig = loadConfigFromPath(projectConfigPath, ctx);
@@ -221,6 +222,7 @@ export function loadPluginConfig(
config = {
...config,
mcp_env_allowlist: userConfig?.mcp_env_allowlist ?? [],
};
log("Final merged config", {
src/shared/binary-downloader.ts
-1
View File
@@ -51,7 +51,6 @@ export async function extractTarGz(
if (isTarTraversalErrorOutput(stderr)) {
throw new Error(`Unsafe archive entry: path contains path traversal (${archivePath})`)
}
throw new Error(`tar extraction failed (exit ${exitCode}): ${stderr}`);
}
}
src/shared/zip-entry-listing.ts
+13 -172
View File
@@ -1,172 +1,13 @@
import { spawn, spawnSync } from "bun"
import type { ArchiveEntry } from "./archive-entry-validator"
function parseTarListedZipEntry(line: string): ArchiveEntry | null {
const match = line.match(/^([^\s])\S*\s+\d+\s+\S+\s+\S+\s+\d+\s+\w+\s+\d+\s+(?:\d{2}:\d{2}|\d{4})\s+(.*)$/)
if (!match) {
return null
}
const [, rawType, rawEntryPath] = match
if (rawType === "l") {
const arrowIndex = rawEntryPath.lastIndexOf(" -> ")
return {
path: arrowIndex === -1 ? rawEntryPath : rawEntryPath.slice(0, arrowIndex),
type: "symlink",
linkPath: arrowIndex === -1 ? undefined : rawEntryPath.slice(arrowIndex + 4),
}
}
return {
path: rawEntryPath,
type: rawType === "d" ? "directory" : "file",
}
}
export async function listZipEntriesWithTar(archivePath: string): Promise<ArchiveEntry[]> {
const proc = spawn(["tar", "-tvf", archivePath], {
stdout: "pipe",
stderr: "pipe",
})
const [exitCode, stdout, stderr] = await Promise.all([
proc.exited,
new Response(proc.stdout).text(),
new Response(proc.stderr).text(),
])
if (exitCode !== 0) {
throw new Error(`zip entry listing failed (exit ${exitCode}): ${stderr}`)
}
return stdout
.split(/\r?\n/)
.map(line => line.trim())
.filter(Boolean)
.map(line => parseTarListedZipEntry(line))
.filter((entry): entry is ArchiveEntry => entry !== null)
}
export function isPythonZipListingAvailable(): boolean {
const proc = spawnSync(["python3", "--version"], {
stdout: "ignore",
stderr: "ignore",
})
return proc.exitCode === 0
}
export async function listZipEntriesWithPython(archivePath: string): Promise<ArchiveEntry[]> {
const script = [
"import json, stat, sys, zipfile",
"entries = []",
"with zipfile.ZipFile(sys.argv[1], 'r') as archive:",
" for info in archive.infolist():",
" mode = (info.external_attr >> 16) & 0xFFFF",
" if stat.S_ISLNK(mode):",
" entry_type = 'symlink'",
" link_path = archive.read(info).decode('utf-8', 'surrogateescape')",
" elif info.filename.endswith('/'):",
" entry_type = 'directory'",
" link_path = None",
" else:",
" entry_type = 'file'",
" link_path = None",
" entry = {'path': info.filename, 'type': entry_type}",
" if link_path is not None:",
" entry['linkPath'] = link_path",
" entries.append(entry)",
"print(json.dumps(entries))",
].join("\n")
const proc = spawn(["python3", "-c", script, archivePath], {
stdout: "pipe",
stderr: "pipe",
})
const [exitCode, stdout, stderr] = await Promise.all([
proc.exited,
new Response(proc.stdout).text(),
new Response(proc.stderr).text(),
])
if (exitCode !== 0) {
throw new Error(`zip entry listing failed (exit ${exitCode}): ${stderr}`)
}
return JSON.parse(stdout) as ArchiveEntry[]
}
export async function listZipEntriesWithPowerShell(
archivePath: string,
escapePowerShellPath: (path: string) => string,
extractor: "pwsh" | "powershell"
): Promise<ArchiveEntry[]> {
const proc = spawn(
[
extractor,
"-Command",
[
"Add-Type -AssemblyName System.IO.Compression.FileSystem",
`$archive = [System.IO.Compression.ZipFile]::OpenRead('${escapePowerShellPath(archivePath)}')`,
"try {",
" foreach ($entry in $archive.Entries) {",
" $mode = ($entry.ExternalAttributes -shr 16) -band 0xFFFF",
" $type = if (($mode -band 0xF000) -eq 0xA000) { 'symlink' } elseif ($entry.FullName.EndsWith('/')) { 'directory' } else { 'file' }",
" $target = ''",
" if ($type -eq 'symlink') {",
" $stream = $entry.Open()",
" try {",
" $reader = New-Object System.IO.StreamReader($stream)",
" try { $target = $reader.ReadToEnd() } finally { $reader.Dispose() }",
" } finally { $stream.Dispose() }",
" }",
" Write-Output ($type + \"`t\" + $entry.FullName + \"`t\" + $target)",
" }",
"} finally {",
" $archive.Dispose()",
"}",
].join("; "),
],
{
stdout: "pipe",
stderr: "pipe",
}
)
const [exitCode, stdout, stderr] = await Promise.all([
proc.exited,
new Response(proc.stdout).text(),
new Response(proc.stderr).text(),
])
if (exitCode !== 0) {
throw new Error(`zip entry listing failed (exit ${exitCode}): ${stderr}`)
}
return stdout
.split(/\r?\n/)
.map(line => line.trim())
.filter(Boolean)
.map((line): ArchiveEntry | null => {
const [type, entryPath, linkPath = ""] = line.split("\t")
if (type !== "file" && type !== "directory" && type !== "symlink") {
return null
}
if (type === "symlink") {
return {
path: entryPath,
type,
linkPath,
}
}
return {
path: entryPath,
type,
}
})
.filter((entry): entry is ArchiveEntry => entry !== null)
}
export {
isPythonZipListingAvailable,
listZipEntriesWithPython,
} from "./zip-entry-listing/python-zip-entry-listing"
export {
listZipEntriesWithPowerShell,
type PowerShellZipExtractor,
} from "./zip-entry-listing/powershell-zip-entry-listing"
export { listZipEntriesWithTar } from "./zip-entry-listing/tar-zip-entry-listing"
export {
isZipInfoZipListingAvailable,
listZipEntriesWithZipInfo,
} from "./zip-entry-listing/zipinfo-zip-entry-listing"
src/shared/zip-entry-listing/powershell-zip-entry-listing.ts
+78
View File
@@ -0,0 +1,78 @@
import { spawn } from "bun"
import type { ArchiveEntry } from "../archive-entry-validator"
export type PowerShellZipExtractor = "pwsh" | "powershell"
export async function listZipEntriesWithPowerShell(
archivePath: string,
escapePowerShellPath: (path: string) => string,
extractor: PowerShellZipExtractor
): Promise<ArchiveEntry[]> {
const proc = spawn(
[
extractor,
"-Command",
[
"Add-Type -AssemblyName System.IO.Compression.FileSystem",
`$archive = [System.IO.Compression.ZipFile]::OpenRead('${escapePowerShellPath(archivePath)}')`,
"try {",
" foreach ($entry in $archive.Entries) {",
" $mode = ($entry.ExternalAttributes -shr 16) -band 0xFFFF",
" $type = if (($mode -band 0xF000) -eq 0xA000) { 'symlink' } elseif ($entry.FullName.EndsWith('/')) { 'directory' } else { 'file' }",
" $target = ''",
" if ($type -eq 'symlink') {",
" $stream = $entry.Open()",
" try {",
" $reader = New-Object System.IO.StreamReader($stream)",
" try { $target = $reader.ReadToEnd() } finally { $reader.Dispose() }",
" } finally { $stream.Dispose() }",
" }",
" Write-Output ($type + \"`t\" + $entry.FullName + \"`t\" + $target)",
" }",
"} finally {",
" $archive.Dispose()",
"}",
].join("; "),
],
{
stdout: "pipe",
stderr: "pipe",
}
)
const [exitCode, stdout, stderr] = await Promise.all([
proc.exited,
new Response(proc.stdout).text(),
new Response(proc.stderr).text(),
])
if (exitCode !== 0) {
throw new Error(`zip entry listing failed (exit ${exitCode}): ${stderr}`)
}
return stdout
.split(/\r?\n/)
.map(line => line.trim())
.filter(Boolean)
.map((line): ArchiveEntry | null => {
const [type, entryPath, linkPath = ""] = line.split("\t")
if (type !== "file" && type !== "directory" && type !== "symlink") {
return null
}
if (type === "symlink") {
return {
path: entryPath,
type,
linkPath,
}
}
return {
path: entryPath,
type,
}
})
.filter((entry): entry is ArchiveEntry => entry !== null)
}
src/shared/zip-entry-listing/python-zip-entry-listing.ts
+55
View File
@@ -0,0 +1,55 @@
import { spawn, spawnSync } from "bun"
import type { ArchiveEntry } from "../archive-entry-validator"
export function isPythonZipListingAvailable(): boolean {
const proc = spawnSync(["python3", "--version"], {
stdout: "ignore",
stderr: "ignore",
})
return proc.exitCode === 0
}
export async function listZipEntriesWithPython(
archivePath: string
): Promise<ArchiveEntry[]> {
const script = [
"import json, stat, sys, zipfile",
"entries = []",
"with zipfile.ZipFile(sys.argv[1], 'r') as archive:",
" for info in archive.infolist():",
" mode = (info.external_attr >> 16) & 0xFFFF",
" if stat.S_ISLNK(mode):",
" entry_type = 'symlink'",
" link_path = archive.read(info).decode('utf-8', 'surrogateescape')",
" elif info.filename.endswith('/'):",
" entry_type = 'directory'",
" link_path = None",
" else:",
" entry_type = 'file'",
" link_path = None",
" entry = {'path': info.filename, 'type': entry_type}",
" if link_path is not None:",
" entry['linkPath'] = link_path",
" entries.append(entry)",
"print(json.dumps(entries))",
].join("\n")
const proc = spawn(["python3", "-c", script, archivePath], {
stdout: "pipe",
stderr: "pipe",
})
const [exitCode, stdout, stderr] = await Promise.all([
proc.exited,
new Response(proc.stdout).text(),
new Response(proc.stderr).text(),
])
if (exitCode !== 0) {
throw new Error(`zip entry listing failed (exit ${exitCode}): ${stderr}`)
}
return JSON.parse(stdout) as ArchiveEntry[]
}
src/shared/zip-entry-listing/read-zip-symlink-target.ts
+23
View File
@@ -0,0 +1,23 @@
import { spawn } from "bun"
export async function readZipSymlinkTarget(
archivePath: string,
entryPath: string
): Promise<string | undefined> {
const proc = spawn(["unzip", "-p", archivePath, "--", entryPath], {
stdout: "pipe",
stderr: "pipe",
})
const [exitCode, stdout, stderr] = await Promise.all([
proc.exited,
new Response(proc.stdout).text(),
new Response(proc.stderr).text(),
])
if (exitCode !== 0) {
throw new Error(`zip symlink target read failed (exit ${exitCode}): ${stderr}`)
}
return stdout || undefined
}
src/shared/zip-entry-listing/tar-zip-entry-listing.ts
+53
View File
@@ -0,0 +1,53 @@
import { spawn } from "bun"
import type { ArchiveEntry } from "../archive-entry-validator"
function parseTarListedZipEntry(line: string): ArchiveEntry | null {
const match = line.match(
/^([^\s])\S*\s+\d+\s+\S+\s+\S+\s+\d+\s+\w+\s+\d+\s+(?:\d{2}:\d{2}|\d{4})\s+(.*)$/
)
if (!match) {
return null
}
const [, rawType, rawEntryPath] = match
if (rawType === "l") {
const arrowIndex = rawEntryPath.lastIndexOf(" -> ")
return {
path: arrowIndex === -1 ? rawEntryPath : rawEntryPath.slice(0, arrowIndex),
type: "symlink",
linkPath: arrowIndex === -1 ? undefined : rawEntryPath.slice(arrowIndex + 4),
}
}
return {
path: rawEntryPath,
type: rawType === "d" ? "directory" : "file",
}
}
export async function listZipEntriesWithTar(
archivePath: string
): Promise<ArchiveEntry[]> {
const proc = spawn(["tar", "-tvf", archivePath], {
stdout: "pipe",
stderr: "pipe",
})
const [exitCode, stdout, stderr] = await Promise.all([
proc.exited,
new Response(proc.stdout).text(),
new Response(proc.stderr).text(),
])
if (exitCode !== 0) {
throw new Error(`zip entry listing failed (exit ${exitCode}): ${stderr}`)
}
return stdout
.split(/\r?\n/)
.map(line => line.trim())
.filter(Boolean)
.map(line => parseTarListedZipEntry(line))
.filter((entry): entry is ArchiveEntry => entry !== null)
}
src/shared/zip-entry-listing/zipinfo-zip-entry-listing.test.ts
+24
View File
@@ -0,0 +1,24 @@
/// <reference types="bun-types" />
import { describe, expect, it } from "bun:test"
import { parseZipInfoListedEntry } from "./zipinfo-zip-entry-listing"
describe("parseZipInfoListedEntry", () => {
describe("#given a zipinfo listing line with trailing filename whitespace", () => {
it("#when parsing the line #then preserves the original trailing whitespace", () => {
// given
const listedLine =
"?rw------- 2.0 unx 1 b- 1 stor 26-Apr-03 18:33 trailing-space.txt "
// when
const parsedEntry = parseZipInfoListedEntry(listedLine)
// then
expect(parsedEntry).toEqual({
path: "trailing-space.txt ",
type: "file",
})
})
})
})
src/shared/zip-entry-listing/zipinfo-zip-entry-listing.ts
+72
View File
@@ -0,0 +1,72 @@
import { spawn, spawnSync } from "bun"
import type { ArchiveEntry } from "../archive-entry-validator"
import { readZipSymlinkTarget } from "./read-zip-symlink-target"
export function parseZipInfoListedEntry(line: string): ArchiveEntry | null {
const match = line.match(
/^([-dl?])\S*\s+\S+\s+\S+\s+\d+\s+\S+\s+\d+\s+\S+\s+\S+\s+\S+\s+(.*)$/
)
if (!match) {
return null
}
const [, rawType, rawEntryPath] = match
return {
path: rawEntryPath,
type: rawType === "d" ? "directory" : rawType === "l" ? "symlink" : "file",
}
}
export function isZipInfoZipListingAvailable(): boolean {
const proc = spawnSync(["which", "zipinfo"], {
stdout: "ignore",
stderr: "ignore",
})
return proc.exitCode === 0
}
function splitZipInfoOutputLines(stdout: string): string[] {
return stdout.split(/\r?\n/).filter(line => line.length > 0)
}
export async function listZipEntriesWithZipInfo(
archivePath: string
): Promise<ArchiveEntry[]> {
if (!isZipInfoZipListingAvailable()) {
throw new Error("zip entry listing requires zipinfo, but zipinfo is not installed")
}
const proc = spawn(["zipinfo", "-l", archivePath], {
stdout: "pipe",
stderr: "pipe",
})
const [exitCode, stdout, stderr] = await Promise.all([
proc.exited,
new Response(proc.stdout).text(),
new Response(proc.stderr).text(),
])
if (exitCode !== 0) {
throw new Error(`zip entry listing failed (exit ${exitCode}): ${stderr}`)
}
const parsedEntries = splitZipInfoOutputLines(stdout)
.map(line => parseZipInfoListedEntry(line))
.filter((entry): entry is ArchiveEntry => entry !== null)
return Promise.all(
parsedEntries.map(async entry => {
if (entry.type !== "symlink") {
return entry
}
return {
...entry,
linkPath: await readZipSymlinkTarget(archivePath, entry.path),
}
})
)
}
src/shared/zip-extractor.ts
+20 -13
View File
@@ -3,10 +3,13 @@ import { release } from "os"
import { validateArchiveEntries } from "./archive-entry-validator"
import {
isPythonZipListingAvailable,
listZipEntriesWithPowerShell,
listZipEntriesWithPython,
listZipEntriesWithTar,
isPythonZipListingAvailable,
isZipInfoZipListingAvailable,
type PowerShellZipExtractor,
listZipEntriesWithPowerShell,
listZipEntriesWithPython,
listZipEntriesWithTar,
listZipEntriesWithZipInfo,
} from "./zip-entry-listing"
const WINDOWS_BUILD_WITH_TAR = 17134
@@ -32,9 +35,7 @@ function escapePowerShellPath(path: string): string {
return path.replace(/'/g, "''")
}
type WindowsZipExtractor = "tar" | "pwsh" | "powershell"
function getWindowsZipExtractor(): WindowsZipExtractor {
function getWindowsZipExtractor(): "tar" | PowerShellZipExtractor {
const buildNumber = getWindowsBuildNumber()
if (buildNumber !== null && buildNumber >= WINDOWS_BUILD_WITH_TAR) {
@@ -94,8 +95,8 @@ export async function extractZip(archivePath: string, destDir: string): Promise<
}
async function listZipEntries(archivePath: string) {
if (process.platform === "win32") {
const extractor = getWindowsZipExtractor()
if (process.platform === "win32") {
const extractor = getWindowsZipExtractor()
if (extractor === "tar") {
return listZipEntriesWithTar(archivePath)
}
@@ -103,9 +104,15 @@ async function listZipEntries(archivePath: string) {
return listZipEntriesWithPowerShell(archivePath, escapePowerShellPath, extractor)
}
if (isPythonZipListingAvailable()) {
return listZipEntriesWithPython(archivePath)
}
if (isPythonZipListingAvailable()) {
return listZipEntriesWithPython(archivePath)
}
return listZipEntriesWithTar(archivePath)
if (isZipInfoZipListingAvailable()) {
return listZipEntriesWithZipInfo(archivePath)
}
throw new Error(
"zip entry listing requires either python3 or zipinfo to inspect the archive safely"
)
}