diff --git a/lib/server/gateway.js b/lib/server/gateway.js index c2262415..0b2bf777 100644 --- a/lib/server/gateway.js +++ b/lib/server/gateway.js @@ -1,5 +1,5 @@ const path = require("path"); -const { spawn, execSync } = require("child_process"); +const { spawn, execSync, execFileSync } = require("child_process"); const fs = require("fs"); const net = require("net"); const { @@ -709,8 +709,21 @@ const syncChannelConfig = (savedVars, mode = "all") => { if (ch === "slack") { const appToken = savedMap[def.extraEnvKeys?.[0]]; if (!appToken) continue; - execSync( - `openclaw channels add --channel slack --bot-token "${token}" --app-token "${appToken}"`, + // execFileSync passes token/appToken as argv entries, never through a + // shell, so a value containing `"`, `$`, or backticks can't break out + // of the command (unlike the old execSync + string-interpolation form). + execFileSync( + "openclaw", + [ + "channels", + "add", + "--channel", + "slack", + "--bot-token", + token, + "--app-token", + appToken, + ], { env, timeout: 15000, encoding: "utf8" }, ); let raw = fs.readFileSync(configPath, "utf8"); @@ -722,11 +735,11 @@ const syncChannelConfig = (savedVars, mode = "all") => { } fs.writeFileSync(configPath, raw); } else { - execSync(`openclaw channels add --channel ${ch} --token "${token}"`, { - env, - timeout: 15000, - encoding: "utf8", - }); + execFileSync( + "openclaw", + ["channels", "add", "--channel", ch, "--token", token], + { env, timeout: 15000, encoding: "utf8" }, + ); const raw = fs.readFileSync(configPath, "utf8"); if (raw.includes(token)) { fs.writeFileSync( @@ -748,11 +761,11 @@ const syncChannelConfig = (savedVars, mode = "all") => { ) { console.log(`[alphaclaw] Removing channel: ${ch}`); try { - execSync(`openclaw channels remove --channel ${ch} --delete`, { - env, - timeout: 15000, - encoding: "utf8", - }); + execFileSync( + "openclaw", + ["channels", "remove", "--channel", ch, "--delete"], + { env, timeout: 15000, encoding: "utf8" }, + ); console.log(`[alphaclaw] Channel ${ch} removed`); } catch (e) { console.error( diff --git a/tests/server/gateway.test.js b/tests/server/gateway.test.js index bc42b6d4..7d68dafc 100644 --- a/tests/server/gateway.test.js +++ b/tests/server/gateway.test.js @@ -17,6 +17,7 @@ const kAlphaclawConfigPath = path.join(OPENCLAW_DIR, "alphaclaw.json"); const modulePath = require.resolve("../../lib/server/gateway"); const originalSpawn = childProcess.spawn; const originalExecSync = childProcess.execSync; +const originalExecFileSync = childProcess.execFileSync; const originalExistsSync = fs.existsSync; const originalMkdirSync = fs.mkdirSync; const originalReaddirSync = fs.readdirSync; @@ -57,6 +58,7 @@ describe("server/gateway restart behavior", () => { afterEach(() => { childProcess.spawn = originalSpawn; childProcess.execSync = originalExecSync; + childProcess.execFileSync = originalExecFileSync; fs.existsSync = originalExistsSync; fs.mkdirSync = originalMkdirSync; fs.readdirSync = originalReaddirSync; @@ -1275,4 +1277,74 @@ describe("server/gateway restart behavior", () => { } } }); + + it("passes a channel bot token containing shell metacharacters as a single argv entry, never through a shell", () => { + const maliciousToken = '123:ABC" ; touch /tmp/pwned ; echo "'; + const execFileSyncMock = vi.fn(() => ""); + childProcess.execFileSync = execFileSyncMock; + fs.readFileSync = vi.fn((targetPath, ...args) => { + if (String(targetPath) === `${OPENCLAW_DIR}/openclaw.json`) { + return JSON.stringify({ channels: {} }); + } + return originalReadFileSync(targetPath, ...args); + }); + fs.writeFileSync = vi.fn(); + delete require.cache[modulePath]; + const gateway = require(modulePath); + + gateway.syncChannelConfig( + [{ key: "TELEGRAM_BOT_TOKEN", value: maliciousToken }], + "add", + ); + + expect(execFileSyncMock).toHaveBeenCalledTimes(1); + const [command, args] = execFileSyncMock.mock.calls[0]; + expect(command).toBe("openclaw"); + expect(args).toEqual([ + "channels", + "add", + "--channel", + "telegram", + "--token", + maliciousToken, + ]); + }); + + it("passes slack bot/app tokens as separate argv entries, never concatenated into a shell string", () => { + const maliciousBotToken = 'xoxb-1" ; touch /tmp/pwned ; echo "'; + const maliciousAppToken = 'xapp-1" ; touch /tmp/pwned2 ; echo "'; + const execFileSyncMock = vi.fn(() => ""); + childProcess.execFileSync = execFileSyncMock; + fs.readFileSync = vi.fn((targetPath, ...args) => { + if (String(targetPath) === `${OPENCLAW_DIR}/openclaw.json`) { + return JSON.stringify({ channels: {} }); + } + return originalReadFileSync(targetPath, ...args); + }); + fs.writeFileSync = vi.fn(); + delete require.cache[modulePath]; + const gateway = require(modulePath); + + gateway.syncChannelConfig( + [ + { key: "SLACK_BOT_TOKEN", value: maliciousBotToken }, + { key: "SLACK_APP_TOKEN", value: maliciousAppToken }, + ], + "add", + ); + + expect(execFileSyncMock).toHaveBeenCalledTimes(1); + const [command, args] = execFileSyncMock.mock.calls[0]; + expect(command).toBe("openclaw"); + expect(args).toEqual([ + "channels", + "add", + "--channel", + "slack", + "--bot-token", + maliciousBotToken, + "--app-token", + maliciousAppToken, + ]); + }); });