From 0471078006cab06a6e489d6444eac1d3dd14da32 Mon Sep 17 00:00:00 2001 From: YeonGyu-Kim Date: Tue, 17 Mar 2026 15:16:54 +0900 Subject: [PATCH] fix(tmux): escape serverUrl in pane shell commands --- .../add-plugin-to-opencode-config.ts | 24 +- .../config-manager/detect-current-config.ts | 10 +- .../config-manager/plugin-detection.test.ts | 68 +++++ src/cli/doctor/checks/system-plugin.ts | 32 ++- src/features/boulder-state/storage.ts | 3 + .../git-master-template-injection.test.ts | 22 ++ .../git-master-template-injection.ts | 12 +- .../skill-mcp-manager/env-cleaner.test.ts | 233 ++++++++++++++++++ src/features/skill-mcp-manager/env-cleaner.ts | 18 ++ src/shared/plugin-identity.ts | 1 + src/shared/shell-env.ts | 41 +++ src/shared/tmux/tmux-utils/pane-replace.ts | 5 +- src/shared/tmux/tmux-utils/pane-spawn.test.ts | 96 ++++++++ src/shared/tmux/tmux-utils/pane-spawn.ts | 4 +- 14 files changed, 546 insertions(+), 23 deletions(-) create mode 100644 src/shared/tmux/tmux-utils/pane-spawn.test.ts diff --git a/src/cli/config-manager/add-plugin-to-opencode-config.ts b/src/cli/config-manager/add-plugin-to-opencode-config.ts index 53af77a82..90a78f0ae 100644 --- a/src/cli/config-manager/add-plugin-to-opencode-config.ts +++ b/src/cli/config-manager/add-plugin-to-opencode-config.ts @@ -1,5 +1,6 @@ import { readFileSync, writeFileSync } from "node:fs" import type { ConfigMergeResult } from "../types" +import { PLUGIN_NAME, LEGACY_PLUGIN_NAME } from "../../shared" import { getConfigDir } from "./config-context" import { ensureConfigDirectoryExists } from "./ensure-config-directory-exists" import { formatErrorWithSuggestion } from "./format-error-with-suggestion" @@ -7,8 +8,6 @@ import { detectConfigFormat } from "./opencode-config-format" import { parseOpenCodeConfigFileWithError, type OpenCodeConfig } from "./parse-opencode-config-file" import { getPluginNameWithVersion } from "./plugin-name-with-version" -const PACKAGE_NAME = "oh-my-opencode" - export async function addPluginToOpenCodeConfig(currentVersion: string): Promise { try { ensureConfigDirectoryExists() @@ -21,7 +20,7 @@ export async function addPluginToOpenCodeConfig(currentVersion: string): Promise } const { format, path } = detectConfigFormat() - const pluginEntry = await getPluginNameWithVersion(currentVersion, PACKAGE_NAME) + const pluginEntry = await getPluginNameWithVersion(currentVersion, PLUGIN_NAME) try { if (format === "none") { @@ -41,13 +40,24 @@ export async function addPluginToOpenCodeConfig(currentVersion: string): Promise const config = parseResult.config const plugins = config.plugin ?? [] - const existingIndex = plugins.findIndex((plugin) => plugin === PACKAGE_NAME || plugin.startsWith(`${PACKAGE_NAME}@`)) - if (existingIndex !== -1) { - if (plugins[existingIndex] === pluginEntry) { + // Check for existing plugin (either current or legacy name) + const currentNameIndex = plugins.findIndex( + (plugin) => plugin === PLUGIN_NAME || plugin.startsWith(`${PLUGIN_NAME}@`) + ) + const legacyNameIndex = plugins.findIndex( + (plugin) => plugin === LEGACY_PLUGIN_NAME || plugin.startsWith(`${LEGACY_PLUGIN_NAME}@`) + ) + + // If either name exists, update to new name + if (currentNameIndex !== -1) { + if (plugins[currentNameIndex] === pluginEntry) { return { success: true, configPath: path } } - plugins[existingIndex] = pluginEntry + plugins[currentNameIndex] = pluginEntry + } else if (legacyNameIndex !== -1) { + // Upgrade legacy name to new name + plugins[legacyNameIndex] = pluginEntry } else { plugins.push(pluginEntry) } diff --git a/src/cli/config-manager/detect-current-config.ts b/src/cli/config-manager/detect-current-config.ts index 5900f2777..3679d5bd6 100644 --- a/src/cli/config-manager/detect-current-config.ts +++ b/src/cli/config-manager/detect-current-config.ts @@ -1,5 +1,5 @@ import { existsSync, readFileSync } from "node:fs" -import { parseJsonc } from "../../shared" +import { parseJsonc, LEGACY_PLUGIN_NAME, PLUGIN_NAME } from "../../shared" import type { DetectedConfig } from "../types" import { getOmoConfigPath } from "./config-context" import { detectConfigFormat } from "./opencode-config-format" @@ -55,8 +55,12 @@ function detectProvidersFromOmoConfig(): { } } +function isOurPlugin(plugin: string): boolean { + return plugin === PLUGIN_NAME || plugin.startsWith(`${PLUGIN_NAME}@`) || + plugin === LEGACY_PLUGIN_NAME || plugin.startsWith(`${LEGACY_PLUGIN_NAME}@`) +} + export function detectCurrentConfig(): DetectedConfig { - const PACKAGE_NAME = "oh-my-opencode" const result: DetectedConfig = { isInstalled: false, hasClaude: true, @@ -82,7 +86,7 @@ export function detectCurrentConfig(): DetectedConfig { const openCodeConfig = parseResult.config const plugins = openCodeConfig.plugin ?? [] - result.isInstalled = plugins.some((plugin) => plugin.startsWith(PACKAGE_NAME)) + result.isInstalled = plugins.some(isOurPlugin) if (!result.isInstalled) { return result diff --git a/src/cli/config-manager/plugin-detection.test.ts b/src/cli/config-manager/plugin-detection.test.ts index 6d489b385..2d4d69945 100644 --- a/src/cli/config-manager/plugin-detection.test.ts +++ b/src/cli/config-manager/plugin-detection.test.ts @@ -52,6 +52,30 @@ describe("detectCurrentConfig - single package detection", () => { expect(result.isInstalled).toBe(true) }) + it("detects oh-my-openagent as installed (legacy name)", () => { + // given + const config = { plugin: ["oh-my-openagent"] } + writeFileSync(testConfigPath, JSON.stringify(config, null, 2) + "\n", "utf-8") + + // when + const result = detectCurrentConfig() + + // then + expect(result.isInstalled).toBe(true) + }) + + it("detects oh-my-openagent with version pin as installed (legacy name)", () => { + // given + const config = { plugin: ["oh-my-openagent@3.11.0"] } + writeFileSync(testConfigPath, JSON.stringify(config, null, 2) + "\n", "utf-8") + + // when + const result = detectCurrentConfig() + + // then + expect(result.isInstalled).toBe(true) + }) + it("returns false when plugin not present", () => { // given const config = { plugin: ["some-other-plugin"] } @@ -64,6 +88,18 @@ describe("detectCurrentConfig - single package detection", () => { expect(result.isInstalled).toBe(false) }) + it("returns false when plugin not present (even with similar name)", () => { + // given - not exactly oh-my-openagent + const config = { plugin: ["oh-my-openagent-extra"] } + writeFileSync(testConfigPath, JSON.stringify(config, null, 2) + "\n", "utf-8") + + // when + const result = detectCurrentConfig() + + // then + expect(result.isInstalled).toBe(false) + }) + it("detects OpenCode Go from the existing omo config", () => { // given writeFileSync(testConfigPath, JSON.stringify({ plugin: ["oh-my-opencode"] }, null, 2) + "\n", "utf-8") @@ -130,6 +166,38 @@ describe("addPluginToOpenCodeConfig - single package writes", () => { expect(savedConfig.plugin).not.toContain("oh-my-opencode@3.10.0") }) + it("recognizes oh-my-openagent as already installed (legacy name)", async () => { + // given + const config = { plugin: ["oh-my-openagent"] } + writeFileSync(testConfigPath, JSON.stringify(config, null, 2) + "\n", "utf-8") + + // when + const result = await addPluginToOpenCodeConfig("3.11.0") + + // then + expect(result.success).toBe(true) + const savedConfig = JSON.parse(readFileSync(testConfigPath, "utf-8")) + // Should upgrade to new name + expect(savedConfig.plugin).toContain("oh-my-opencode") + expect(savedConfig.plugin).not.toContain("oh-my-openagent") + }) + + it("replaces version-pinned oh-my-openagent@X.Y.Z with new name", async () => { + // given + const config = { plugin: ["oh-my-openagent@3.10.0"] } + writeFileSync(testConfigPath, JSON.stringify(config, null, 2) + "\n", "utf-8") + + // when + const result = await addPluginToOpenCodeConfig("3.11.0") + + // then + expect(result.success).toBe(true) + const savedConfig = JSON.parse(readFileSync(testConfigPath, "utf-8")) + // Legacy should be replaced with new name + expect(savedConfig.plugin).toContain("oh-my-opencode") + expect(savedConfig.plugin).not.toContain("oh-my-openagent") + }) + it("adds new plugin when none exists", async () => { // given const config = {} diff --git a/src/cli/doctor/checks/system-plugin.ts b/src/cli/doctor/checks/system-plugin.ts index cd4969247..6abe089a5 100644 --- a/src/cli/doctor/checks/system-plugin.ts +++ b/src/cli/doctor/checks/system-plugin.ts @@ -1,7 +1,6 @@ import { existsSync, readFileSync } from "node:fs" -import { PACKAGE_NAME } from "../constants" -import { getOpenCodeConfigPaths, parseJsonc } from "../../../shared" +import { LEGACY_PLUGIN_NAME, PLUGIN_NAME, getOpenCodeConfigPaths, parseJsonc } from "../../../shared" export interface PluginInfo { registered: boolean @@ -24,18 +23,33 @@ function detectConfigPath(): string | null { } function parsePluginVersion(entry: string): string | null { - if (!entry.startsWith(`${PACKAGE_NAME}@`)) return null - const value = entry.slice(PACKAGE_NAME.length + 1) - if (!value || value === "latest") return null - return value + // Check for current package name + if (entry.startsWith(`${PLUGIN_NAME}@`)) { + const value = entry.slice(PLUGIN_NAME.length + 1) + if (!value || value === "latest") return null + return value + } + // Check for legacy package name + if (entry.startsWith(`${LEGACY_PLUGIN_NAME}@`)) { + const value = entry.slice(LEGACY_PLUGIN_NAME.length + 1) + if (!value || value === "latest") return null + return value + } + return null } function findPluginEntry(entries: string[]): { entry: string; isLocalDev: boolean } | null { for (const entry of entries) { - if (entry === PACKAGE_NAME || entry.startsWith(`${PACKAGE_NAME}@`)) { + // Check for current package name + if (entry === PLUGIN_NAME || entry.startsWith(`${PLUGIN_NAME}@`)) { return { entry, isLocalDev: false } } - if (entry.startsWith("file://") && entry.includes(PACKAGE_NAME)) { + // Check for legacy package name + if (entry === LEGACY_PLUGIN_NAME || entry.startsWith(`${LEGACY_PLUGIN_NAME}@`)) { + return { entry, isLocalDev: false } + } + // Check for file:// paths that include either name + if (entry.startsWith("file://") && (entry.includes(PLUGIN_NAME) || entry.includes(LEGACY_PLUGIN_NAME))) { return { entry, isLocalDev: true } } } @@ -76,7 +90,7 @@ export function getPluginInfo(): PluginInfo { registered: true, configPath, entry: pluginEntry.entry, - isPinned: pinnedVersion !== null && /^\d+\.\d+\.\d+/.test(pinnedVersion), + isPinned: pinnedVersion !== null && /^\d+\.\d+\.\d+/.test(pinnedVersion ?? ""), pinnedVersion, isLocalDev: pluginEntry.isLocalDev, } diff --git a/src/features/boulder-state/storage.ts b/src/features/boulder-state/storage.ts index ab84368b7..c9ac83993 100644 --- a/src/features/boulder-state/storage.ts +++ b/src/features/boulder-state/storage.ts @@ -59,10 +59,13 @@ export function appendSessionId(directory: string, sessionId: string): BoulderSt if (!Array.isArray(state.session_ids)) { state.session_ids = [] } + const originalSessionIds = [...state.session_ids] state.session_ids.push(sessionId) if (writeBoulderState(directory, state)) { return state } + state.session_ids = originalSessionIds + return null } return state diff --git a/src/features/opencode-skill-loader/git-master-template-injection.test.ts b/src/features/opencode-skill-loader/git-master-template-injection.test.ts index 60ea0f0b3..bbe38645e 100644 --- a/src/features/opencode-skill-loader/git-master-template-injection.test.ts +++ b/src/features/opencode-skill-loader/git-master-template-injection.test.ts @@ -153,3 +153,25 @@ describe("#given git_env_prefix with commit footer", () => { }) }) }) + +describe("#given idempotency of prefixGitCommandsInBashCodeBlocks", () => { + describe("#when git_env_prefix is provided and template already has prefixed commands in env prefix section", () => { + it("#then does NOT double-prefix the already-prefixed commands", () => { + const result = injectGitMasterConfig(SAMPLE_TEMPLATE, { + commit_footer: false, + include_co_authored_by: false, + git_env_prefix: "GIT_MASTER=1", + }) + + expect(result).not.toContain("GIT_MASTER=1 GIT_MASTER=1 git status") + expect(result).not.toContain("GIT_MASTER=1 GIT_MASTER=1 git add") + expect(result).not.toContain("GIT_MASTER=1 GIT_MASTER=1 git commit") + expect(result).not.toContain("GIT_MASTER=1 GIT_MASTER=1 git push") + + expect(result).toContain("GIT_MASTER=1 git status") + expect(result).toContain("GIT_MASTER=1 git add") + expect(result).toContain("GIT_MASTER=1 git commit") + expect(result).toContain("GIT_MASTER=1 git push") + }) + }) +}) diff --git a/src/features/opencode-skill-loader/git-master-template-injection.ts b/src/features/opencode-skill-loader/git-master-template-injection.ts index 3b8e9630a..fc1ba3e4a 100644 --- a/src/features/opencode-skill-loader/git-master-template-injection.ts +++ b/src/features/opencode-skill-loader/git-master-template-injection.ts @@ -72,8 +72,16 @@ function prefixGitCommandsInBashCodeBlocks(template: string, prefix: string): st function prefixGitCommandsInCodeBlock(codeBlock: string, prefix: string): string { return codeBlock - .replace(LEADING_GIT_COMMAND_PATTERN, `$1${prefix} git`) - .replace(INLINE_GIT_COMMAND_PATTERN, `$1${prefix} git`) + .split("\n") + .map((line) => { + if (line.includes(prefix)) { + return line + } + return line + .replace(LEADING_GIT_COMMAND_PATTERN, `$1${prefix} git`) + .replace(INLINE_GIT_COMMAND_PATTERN, `$1${prefix} git`) + }) + .join("\n") } function buildCommitFooterInjection( diff --git a/src/features/skill-mcp-manager/env-cleaner.test.ts b/src/features/skill-mcp-manager/env-cleaner.test.ts index 08da63388..75cfe348e 100644 --- a/src/features/skill-mcp-manager/env-cleaner.test.ts +++ b/src/features/skill-mcp-manager/env-cleaner.test.ts @@ -199,3 +199,236 @@ describe("EXCLUDED_ENV_PATTERNS", () => { } }) }) +describe("secret env var filtering", () => { + it("filters out ANTHROPIC_API_KEY", () => { + // given + process.env.ANTHROPIC_API_KEY = "sk-ant-api03-secret" + process.env.PATH = "/usr/bin" + + // when + const cleanEnv = createCleanMcpEnvironment() + + // then + expect(cleanEnv.ANTHROPIC_API_KEY).toBeUndefined() + expect(cleanEnv.PATH).toBe("/usr/bin") + }) + + it("filters out AWS_SECRET_ACCESS_KEY", () => { + // given + process.env.AWS_SECRET_ACCESS_KEY = "wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY" + process.env.AWS_ACCESS_KEY_ID = "AKIAIOSFODNN7EXAMPLE" + process.env.HOME = "/home/user" + + // when + const cleanEnv = createCleanMcpEnvironment() + + // then + expect(cleanEnv.AWS_SECRET_ACCESS_KEY).toBeUndefined() + expect(cleanEnv.AWS_ACCESS_KEY_ID).toBeUndefined() + expect(cleanEnv.HOME).toBe("/home/user") + }) + + it("filters out GITHUB_TOKEN", () => { + // given + process.env.GITHUB_TOKEN = "ghp_secrettoken123456789" + process.env.GITHUB_API_TOKEN = "another_secret_token" + process.env.SHELL = "/bin/bash" + + // when + const cleanEnv = createCleanMcpEnvironment() + + // then + expect(cleanEnv.GITHUB_TOKEN).toBeUndefined() + expect(cleanEnv.GITHUB_API_TOKEN).toBeUndefined() + expect(cleanEnv.SHELL).toBe("/bin/bash") + }) + + it("filters out OPENAI_API_KEY", () => { + // given + process.env.OPENAI_API_KEY = "sk-secret123456789" + process.env.LANG = "en_US.UTF-8" + + // when + const cleanEnv = createCleanMcpEnvironment() + + // then + expect(cleanEnv.OPENAI_API_KEY).toBeUndefined() + expect(cleanEnv.LANG).toBe("en_US.UTF-8") + }) + + it("filters out DATABASE_URL with credentials", () => { + // given + process.env.DATABASE_URL = "postgresql://user:password@localhost:5432/db" + process.env.DB_PASSWORD = "supersecretpassword" + process.env.TERM = "xterm-256color" + + // when + const cleanEnv = createCleanMcpEnvironment() + + // then + expect(cleanEnv.DATABASE_URL).toBeUndefined() + expect(cleanEnv.DB_PASSWORD).toBeUndefined() + expect(cleanEnv.TERM).toBe("xterm-256color") + }) +}) + +describe("suffix-based secret filtering", () => { + it("filters variables ending with _KEY", () => { + // given + process.env.MY_API_KEY = "secret-value" + process.env.SOME_KEY = "another-secret" + process.env.TMPDIR = "/tmp" + + // when + const cleanEnv = createCleanMcpEnvironment() + + // then + expect(cleanEnv.MY_API_KEY).toBeUndefined() + expect(cleanEnv.SOME_KEY).toBeUndefined() + expect(cleanEnv.TMPDIR).toBe("/tmp") + }) + + it("filters variables ending with _SECRET", () => { + // given + process.env.AWS_SECRET = "secret-value" + process.env.JWT_SECRET = "jwt-secret-token" + process.env.USER = "testuser" + + // when + const cleanEnv = createCleanMcpEnvironment() + + // then + expect(cleanEnv.AWS_SECRET).toBeUndefined() + expect(cleanEnv.JWT_SECRET).toBeUndefined() + expect(cleanEnv.USER).toBe("testuser") + }) + + it("filters variables ending with _TOKEN", () => { + // given + process.env.ACCESS_TOKEN = "token-value" + process.env.BEARER_TOKEN = "bearer-token" + process.env.HOME = "/home/user" + + // when + const cleanEnv = createCleanMcpEnvironment() + + // then + expect(cleanEnv.ACCESS_TOKEN).toBeUndefined() + expect(cleanEnv.BEARER_TOKEN).toBeUndefined() + expect(cleanEnv.HOME).toBe("/home/user") + }) + + it("filters variables ending with _PASSWORD", () => { + // given + process.env.DB_PASSWORD = "db-password" + process.env.APP_PASSWORD = "app-secret" + process.env.NODE_ENV = "production" + + // when + const cleanEnv = createCleanMcpEnvironment() + + // then + expect(cleanEnv.DB_PASSWORD).toBeUndefined() + expect(cleanEnv.APP_PASSWORD).toBeUndefined() + expect(cleanEnv.NODE_ENV).toBe("production") + }) + + it("filters variables ending with _CREDENTIAL", () => { + // given + process.env.GCP_CREDENTIAL = "json-credential" + process.env.AZURE_CREDENTIAL = "azure-creds" + process.env.PWD = "/current/dir" + + // when + const cleanEnv = createCleanMcpEnvironment() + + // then + expect(cleanEnv.GCP_CREDENTIAL).toBeUndefined() + expect(cleanEnv.AZURE_CREDENTIAL).toBeUndefined() + expect(cleanEnv.PWD).toBe("/current/dir") + }) + + it("filters variables ending with _API_KEY", () => { + // given + // given + process.env.STRIPE_API_KEY = "sk_live_secret" + process.env.SENDGRID_API_KEY = "SG.secret" + process.env.SHELL = "/bin/zsh" + + // when + const cleanEnv = createCleanMcpEnvironment() + + // then + expect(cleanEnv.STRIPE_API_KEY).toBeUndefined() + expect(cleanEnv.SENDGRID_API_KEY).toBeUndefined() + expect(cleanEnv.SHELL).toBe("/bin/zsh") + }) +}) + +describe("safe environment variables preserved", () => { + it("preserves PATH", () => { + // given + process.env.PATH = "/usr/bin:/usr/local/bin" + + // when + const cleanEnv = createCleanMcpEnvironment() + + // then + expect(cleanEnv.PATH).toBe("/usr/bin:/usr/local/bin") + }) + + it("preserves HOME", () => { + // given + process.env.HOME = "/home/testuser" + + // when + const cleanEnv = createCleanMcpEnvironment() + + // then + expect(cleanEnv.HOME).toBe("/home/testuser") + }) + + it("preserves SHELL", () => { + // given + process.env.SHELL = "/bin/bash" + + // when + const cleanEnv = createCleanMcpEnvironment() + + // then + expect(cleanEnv.SHELL).toBe("/bin/bash") + }) + + it("preserves LANG", () => { + // given + process.env.LANG = "en_US.UTF-8" + + // when + const cleanEnv = createCleanMcpEnvironment() + + // then + expect(cleanEnv.LANG).toBe("en_US.UTF-8") + }) + + it("preserves TERM", () => { + // given + process.env.TERM = "xterm-256color" + + // when + const cleanEnv = createCleanMcpEnvironment() + + // then + expect(cleanEnv.TERM).toBe("xterm-256color") + }) + + it("preserves TMPDIR", () => { + // given + process.env.TMPDIR = "/tmp" + + // when + const cleanEnv = createCleanMcpEnvironment() + + // then + expect(cleanEnv.TMPDIR).toBe("/tmp") +}) +}) diff --git a/src/features/skill-mcp-manager/env-cleaner.ts b/src/features/skill-mcp-manager/env-cleaner.ts index 9a3faba79..9c6ebe1aa 100644 --- a/src/features/skill-mcp-manager/env-cleaner.ts +++ b/src/features/skill-mcp-manager/env-cleaner.ts @@ -1,10 +1,28 @@ // Filters npm/pnpm/yarn config env vars that break MCP servers in pnpm projects (#456) +// Also filters secret-containing env vars to prevent exposure to malicious stdio MCP servers (#B-02) export const EXCLUDED_ENV_PATTERNS: RegExp[] = [ + // npm/pnpm/yarn config patterns (original) /^NPM_CONFIG_/i, /^npm_config_/, /^YARN_/, /^PNPM_/, /^NO_UPDATE_NOTIFIER$/, + + // Specific high-risk secret env vars (explicit blocks) + /^ANTHROPIC_API_KEY$/i, + /^AWS_ACCESS_KEY_ID$/i, + /^AWS_SECRET_ACCESS_KEY$/i, + /^GITHUB_TOKEN$/i, + /^DATABASE_URL$/i, + /^OPENAI_API_KEY$/i, + + // Suffix-based patterns for common secret naming conventions + /_KEY$/i, + /_SECRET$/i, + /_TOKEN$/i, + /_PASSWORD$/i, + /_CREDENTIAL$/i, + /_API_KEY$/i, ] export function createCleanMcpEnvironment( diff --git a/src/shared/plugin-identity.ts b/src/shared/plugin-identity.ts index 65b190fe6..3de14c328 100644 --- a/src/shared/plugin-identity.ts +++ b/src/shared/plugin-identity.ts @@ -1,4 +1,5 @@ export const PLUGIN_NAME = "oh-my-opencode" +export const LEGACY_PLUGIN_NAME = "oh-my-openagent" export const CONFIG_BASENAME = "oh-my-opencode" export const LOG_FILENAME = "oh-my-opencode.log" export const CACHE_DIR_NAME = "oh-my-opencode" diff --git a/src/shared/shell-env.ts b/src/shared/shell-env.ts index b074baf51..bec4b8336 100644 --- a/src/shared/shell-env.ts +++ b/src/shared/shell-env.ts @@ -109,3 +109,44 @@ export function buildEnvPrefix( return "" } } + +/** + * Escape a value for use in a double-quoted shell -c command argument. + * + * In shell -c "..." strings, these characters have special meaning and must be escaped: + * - $ - variable expansion, command substitution $(...) + * - ` - command substitution `...` + * - \\ - escape character + * - " - end quote + * - ; | & - command separators + * - # - comment + * - () - grouping operators + * + * @param value - The value to escape + * @returns Escaped value safe for double-quoted shell -c argument + * + * @example + * ```ts + * // For malicious input + * const url = "http://localhost:3000'; cat /etc/passwd; echo '" + * const escaped = shellEscapeForDoubleQuotedCommand(url) + * // => "http://localhost:3000'\''; cat /etc/passwd; echo '" + * + * // Usage in command: + * const cmd = `/bin/sh -c "opencode attach ${escaped} --session ${sessionId}"` + * ``` + */ +export function shellEscapeForDoubleQuotedCommand(value: string): string { + // Order matters: escape backslash FIRST, then other characters + return value + .replace(/\\/g, "\\\\") // escape backslash first + .replace(/\$/g, "\\$") // escape dollar sign + .replace(/`/g, "\\`") // escape backticks + .replace(/"/g, "\\\"") // escape double quotes + .replace(/;/g, "\\;") // escape semicolon (command separator) + .replace(/\|/g, "\\|") // escape pipe (command separator) + .replace(/&/g, "\\&") // escape ampersand (command separator) + .replace(/#/g, "\\#") // escape hash (comment) + .replace(/\(/g, "\\(") // escape parentheses + .replace(/\)/g, "\\)") // escape parentheses +} diff --git a/src/shared/tmux/tmux-utils/pane-replace.ts b/src/shared/tmux/tmux-utils/pane-replace.ts index 53520439a..271ad79eb 100644 --- a/src/shared/tmux/tmux-utils/pane-replace.ts +++ b/src/shared/tmux/tmux-utils/pane-replace.ts @@ -3,6 +3,7 @@ import type { TmuxConfig } from "../../../config/schema" import { getTmuxPath } from "../../../tools/interactive-bash/tmux-path-resolver" import type { SpawnPaneResult } from "../types" import { isInsideTmux } from "./environment" +import { shellEscapeForDoubleQuotedCommand } from "../../shell-env" export async function replaceTmuxPane( paneId: string, @@ -35,7 +36,8 @@ export async function replaceTmuxPane( await ctrlCProc.exited const shell = process.env.SHELL || "/bin/sh" - const opencodeCmd = `${shell} -c 'opencode attach ${serverUrl} --session ${sessionId}'` + const escapedUrl = shellEscapeForDoubleQuotedCommand(serverUrl) + const opencodeCmd = `${shell} -c "opencode attach ${escapedUrl} --session ${sessionId}"` const proc = spawn([tmux, "respawn-pane", "-k", "-t", paneId, opencodeCmd], { stdout: "pipe", @@ -60,6 +62,7 @@ export async function replaceTmuxPane( const titleStderr = await stderrPromise log("[replaceTmuxPane] WARNING: failed to set pane title", { paneId, + title, exitCode: titleExitCode, stderr: titleStderr.trim(), }) diff --git a/src/shared/tmux/tmux-utils/pane-spawn.test.ts b/src/shared/tmux/tmux-utils/pane-spawn.test.ts new file mode 100644 index 000000000..a4ca40cfe --- /dev/null +++ b/src/shared/tmux/tmux-utils/pane-spawn.test.ts @@ -0,0 +1,96 @@ +import { describe, expect, it } from "bun:test" +import { shellEscapeForDoubleQuotedCommand } from "../../shell-env" + +describe("given a serverUrl with shell metacharacters", () => { + describe("when building tmux spawn command with double quotes", () => { + it("then serverUrl is escaped to prevent shell injection", () => { + const serverUrl = "http://localhost:3000'; cat /etc/passwd; echo '" + const sessionId = "test-session" + const shell = "/bin/sh" + + // Use double quotes for outer shell -c command, escape dangerous chars in URL + const escapedUrl = shellEscapeForDoubleQuotedCommand(serverUrl) + const opencodeCmd = `${shell} -c "opencode attach ${escapedUrl} --session ${sessionId}"` + + // The semicolon should be escaped so it's treated as literal, not separator + expect(opencodeCmd).toContain("\\;") + // The malicious content should be escaped - semicolons are now \\; + expect(opencodeCmd).not.toMatch(/[^\\];\s*cat/) + }) + }) + + describe("when building tmux replace command", () => { + it("then serverUrl is escaped to prevent shell injection", () => { + const serverUrl = "http://localhost:3000'; rm -rf /; '" + const sessionId = "test-session" + const shell = "/bin/sh" + + const escapedUrl = shellEscapeForDoubleQuotedCommand(serverUrl) + const opencodeCmd = `${shell} -c "opencode attach ${escapedUrl} --session ${sessionId}"` + + expect(opencodeCmd).toContain("\\;") + expect(opencodeCmd).not.toMatch(/[^\\];\s*rm/) + }) + }) +}) + +describe("given a normal serverUrl without shell metacharacters", () => { + describe("when building tmux spawn command", () => { + it("then serverUrl works correctly", () => { + const serverUrl = "http://localhost:3000" + const sessionId = "test-session" + const shell = "/bin/sh" + + const escapedUrl = shellEscapeForDoubleQuotedCommand(serverUrl) + const opencodeCmd = `${shell} -c "opencode attach ${escapedUrl} --session ${sessionId}"` + + expect(opencodeCmd).toContain(serverUrl) + }) + }) +}) + +describe("given a serverUrl with dollar sign (command injection)", () => { + describe("when building tmux command", () => { + it("then dollar sign is escaped properly", () => { + const serverUrl = "http://localhost:3000$(whoami)" + const sessionId = "test-session" + const shell = "/bin/sh" + + const escapedUrl = shellEscapeForDoubleQuotedCommand(serverUrl) + const opencodeCmd = `${shell} -c "opencode attach ${escapedUrl} --session ${sessionId}"` + + // The $ should be escaped to literal $ + expect(opencodeCmd).toContain("\\$") + }) + }) +}) + +describe("given a serverUrl with backticks (command injection)", () => { + describe("when building tmux command", () => { + it("then backticks are escaped properly", () => { + const serverUrl = "http://localhost:3000`whoami`" + const sessionId = "test-session" + const shell = "/bin/sh" + + const escapedUrl = shellEscapeForDoubleQuotedCommand(serverUrl) + const opencodeCmd = `${shell} -c "opencode attach ${escapedUrl} --session ${sessionId}"` + + expect(opencodeCmd).toContain("\\`") + }) + }) +}) + +describe("given a serverUrl with pipe operator", () => { + describe("when building tmux command", () => { + it("then pipe is escaped properly", () => { + const serverUrl = "http://localhost:3000 | ls" + const sessionId = "test-session" + const shell = "/bin/sh" + + const escapedUrl = shellEscapeForDoubleQuotedCommand(serverUrl) + const opencodeCmd = `${shell} -c "opencode attach ${escapedUrl} --session ${sessionId}"` + + expect(opencodeCmd).toContain("\\|") + }) + }) +}) diff --git a/src/shared/tmux/tmux-utils/pane-spawn.ts b/src/shared/tmux/tmux-utils/pane-spawn.ts index 18af7c536..2713eafbc 100644 --- a/src/shared/tmux/tmux-utils/pane-spawn.ts +++ b/src/shared/tmux/tmux-utils/pane-spawn.ts @@ -5,6 +5,7 @@ import type { SpawnPaneResult } from "../types" import type { SplitDirection } from "./environment" import { isInsideTmux } from "./environment" import { isServerRunning } from "./server-health" +import { shellEscapeForDoubleQuotedCommand } from "../../shell-env" export async function spawnTmuxPane( sessionId: string, @@ -49,7 +50,8 @@ export async function spawnTmuxPane( log("[spawnTmuxPane] all checks passed, spawning...") const shell = process.env.SHELL || "/bin/sh" - const opencodeCmd = `${shell} -c 'opencode attach ${serverUrl} --session ${sessionId}'` + const escapedUrl = shellEscapeForDoubleQuotedCommand(serverUrl) + const opencodeCmd = `${shell} -c "opencode attach ${escapedUrl} --session ${sessionId}"` const args = [ "split-window",