From 5eac4686aae954d4409d4d3593fb47ba800f6634 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Sun, 3 May 2026 14:52:07 +0100 Subject: [PATCH] fix: preserve env split-string payloads --- src/agents/bash-tools.exec.path.test.ts | 6 + .../bash-tools.exec.script-preflight.test.ts | 15 ++ src/infra/command-analysis/risks.test.ts | 44 ++++ src/infra/command-analysis/risks.ts | 198 ++++++++++++------ src/infra/dispatch-wrapper-resolution.ts | 34 ++- src/infra/exec-wrapper-resolution.test.ts | 12 ++ 6 files changed, 246 insertions(+), 63 deletions(-) diff --git a/src/agents/bash-tools.exec.path.test.ts b/src/agents/bash-tools.exec.path.test.ts index ed44778525f..70f2a432c91 100644 --- a/src/agents/bash-tools.exec.path.test.ts +++ b/src/agents/bash-tools.exec.path.test.ts @@ -337,13 +337,19 @@ describe("exec host env validation", () => { "env --ignore-environment /approve abc123 allow-once", "env -i FOO=1 /approve abc123 allow-once", "env -S '/approve abc123 deny'", + "env -iS'/approve abc123 deny'", + "env -S '/approve abc123' deny", + "env -iS'/approve abc123' deny", "command /approve abc123 deny", "command -p /approve abc123 deny", "exec -a openclaw /approve abc123 deny", "sudo /approve abc123 allow-once", "sudo -E /approve abc123 allow-once", + "sudo -EH /approve abc123 allow-once", "sudo -uroot bash -lc '/approve abc123 allow-once'", + "sudo -EH bash -lc '/approve abc123 allow-once'", "doas -uroot bash -lc '/approve abc123 deny'", + "env env env env env env /approve abc123 allow-once", "bash -lc '/approve abc123 deny'", "bash -c 'sudo /approve abc123 allow-once'", "sh -c '/approve abc123 allow-once'", diff --git a/src/agents/bash-tools.exec.script-preflight.test.ts b/src/agents/bash-tools.exec.script-preflight.test.ts index a6bdc32b417..d55ce752fbb 100644 --- a/src/agents/bash-tools.exec.script-preflight.test.ts +++ b/src/agents/bash-tools.exec.script-preflight.test.ts @@ -93,6 +93,21 @@ describe("exec interactive OpenClaw channel login guard", () => { command: "sudo -u openclaw bash -lc 'openclaw channels login --channel whatsapp'", }), ).rejects.toThrow(/exec cannot run interactive OpenClaw channel login commands/); + await expect( + tool.execute("call-clustered-sudo-channel-login", { + command: "sudo -EH bash -lc 'openclaw channels login --channel whatsapp'", + }), + ).rejects.toThrow(/exec cannot run interactive OpenClaw channel login commands/); + await expect( + tool.execute("call-deep-env-channel-login", { + command: "env env env env env env openclaw channels login --channel whatsapp", + }), + ).rejects.toThrow(/exec cannot run interactive OpenClaw channel login commands/); + await expect( + tool.execute("call-env-s-trailing-channel-login", { + command: "env -S 'openclaw channels' login --channel whatsapp", + }), + ).rejects.toThrow(/exec cannot run interactive OpenClaw channel login commands/); }); }); diff --git a/src/infra/command-analysis/risks.test.ts b/src/infra/command-analysis/risks.test.ts index e312728f805..748830100b4 100644 --- a/src/infra/command-analysis/risks.test.ts +++ b/src/infra/command-analysis/risks.test.ts @@ -17,8 +17,20 @@ describe("command-analysis risks", () => { "-c", ); expect(detectInlineEvalArgv(["sudo", "-uroot", "python3", "-c", "print(1)"])?.flag).toBe("-c"); + expect(detectInlineEvalArgv(["sudo", "-EH", "python3", "-c", "print(1)"])?.flag).toBe("-c"); + expect(detectInlineEvalArgv(["sudo", "-Eu", "root", "python3", "-c", "print(1)"])?.flag).toBe( + "-c", + ); expect(detectInlineEvalArgv(["doas", "-uroot", "python3", "-c", "print(1)"])?.flag).toBe("-c"); expect(detectInlineEvalArgv(["env", "sudo", "python3", "-c", "print(1)"])?.flag).toBe("-c"); + expect( + detectInlineEvalArgv(["env", "env", "env", "env", "env", "env", "python3", "-c", "print(1)"]) + ?.flag, + ).toBe("-c"); + expect(detectInlineEvalArgv(["env", "-iSpython3 -c 'print(1)'"])?.flag).toBe("-c"); + expect(detectInlineEvalArgv(["env", "-iS", "python3 -c 'print(1)'"])?.flag).toBe("-c"); + expect(detectInlineEvalArgv(["env", "-S", "python3 -c", "print(1)"])?.flag).toBe("-c"); + expect(detectInlineEvalArgv(["env", "-iSpython3 -c", "print(1)"])?.flag).toBe("-c"); expect(detectInlineEvalArgv(["command", "node", "--eval", "1"])?.flag).toBe("--eval"); expect(detectInlineEvalArgv(["env", "-S", 'python3 -c "print(1)"'])?.flag).toBe("-c"); expect(detectInlineEvalArgv(["python3", "script.py"])).toBeNull(); @@ -62,6 +74,12 @@ describe("command-analysis risks", () => { (argv, startIndex) => argv[startIndex] === "-lc", ), ).toBe("sudo"); + expect( + detectShellWrapperThroughCarrierArgv( + ["sudo", "-EH", "bash", "-lc", "id"], + (argv, startIndex) => argv[startIndex] === "-lc", + ), + ).toBe("sudo"); expect( detectShellWrapperThroughCarrierArgv( ["sudo", "echo", "bash", "-lc", "id"], @@ -85,6 +103,9 @@ describe("command-analysis risks", () => { expect(buildCommandPayloadCandidates(["FOO=1", "sudo", "-E", "/approve", "abc"])).toEqual([ "/approve abc", ]); + expect(buildCommandPayloadCandidates(["sudo", "-EH", "/approve", "abc"])).toEqual([ + "/approve abc", + ]); expect( buildCommandPayloadCandidates(["sudo", "-uroot", "bash", "-lc", "/approve req allow-once"]), ).toEqual(["bash -lc /approve req allow-once", "/approve req allow-once"]); @@ -95,12 +116,35 @@ describe("command-analysis risks", () => { "bash -lc /approve abc deny", "/approve abc deny", ]); + expect(buildCommandPayloadCandidates(["env", "-S", "bash -lc", "/approve abc deny"])).toEqual([ + "bash -lc /approve abc deny", + "/approve abc deny", + ]); + expect(buildCommandPayloadCandidates(["env", "-iSbash -lc", "/approve abc deny"])).toEqual([ + "bash -lc /approve abc deny", + "/approve abc deny", + ]); expect(buildCommandPayloadCandidates(["exec", "-a", "openclaw", "/approve", "abc"])).toEqual([ "/approve abc", ]); expect(buildCommandPayloadCandidates(["command", "-v", "/approve"])).toEqual([ "command -v /approve", ]); + expect( + buildCommandPayloadCandidates([ + "env", + "env", + "env", + "env", + "env", + "env", + "openclaw", + "channels", + "login", + "--channel", + "whatsapp", + ]), + ).toContain("openclaw channels login --channel whatsapp"); }); it("checks both effective and original argv for segment inline eval", () => { diff --git a/src/infra/command-analysis/risks.ts b/src/infra/command-analysis/risks.ts index 34284eea36b..d33446223fb 100644 --- a/src/infra/command-analysis/risks.ts +++ b/src/infra/command-analysis/risks.ts @@ -19,7 +19,7 @@ export type CommandCarrierHit = { export type CarriedShellBuiltinHit = { kind: "eval" } | { kind: "source"; command: string }; -const MAX_INLINE_EVAL_CARRIER_DEPTH = 4; +const MAX_ENV_SPLIT_PAYLOAD_DEPTH = 32; const COMMAND_EXECUTING_OPTIONS = new Set(["-p"]); const COMMAND_QUERY_OPTIONS = new Set(["-v", "-V"]); @@ -35,6 +35,7 @@ const ENV_OPTIONS_WITH_VALUE = new Set([ "--split-string", "--unset", ]); +const ENV_SPLIT_STRING_OPTIONS = new Set(["-S", "--split-string"]); const ENV_STANDALONE_OPTIONS = new Set(["-0", "-i", "--ignore-environment", "--null"]); const SUDO_OPTIONS_WITH_VALUE = new Set([ "-C", @@ -103,46 +104,97 @@ function optionName(token: string): string { type ParsedCarrierOption = { name: string; hasInlineValue: boolean; + inlineValue?: string; }; function parseCarrierOptionToken( token: string, + standaloneOptions: ReadonlySet, optionsWithValue: ReadonlySet, -): ParsedCarrierOption { + nonExecutingOptions: ReadonlySet = new Set(), +): ParsedCarrierOption[] | null { if (token.startsWith("--")) { - return { name: optionName(token), hasInlineValue: token.includes("=") }; + const name = optionName(token); + if ( + standaloneOptions.has(name) || + optionsWithValue.has(name) || + nonExecutingOptions.has(name) + ) { + const valueDelimiter = token.indexOf("="); + return [ + { + name, + hasInlineValue: valueDelimiter >= 0, + inlineValue: valueDelimiter >= 0 ? token.slice(valueDelimiter + 1) : undefined, + }, + ]; + } + return null; } - const shortName = token.slice(0, 2); - if (/^-[A-Za-z]/u.test(shortName) && token.length > 2 && optionsWithValue.has(shortName)) { - return { name: shortName, hasInlineValue: true }; + + if (!/^-[A-Za-z0-9]/u.test(token)) { + return null; } - return { name: token, hasInlineValue: false }; + + const options: ParsedCarrierOption[] = []; + for (let index = 1; index < token.length; index += 1) { + const name = `-${token[index] ?? ""}`; + if (optionsWithValue.has(name)) { + options.push({ + name, + hasInlineValue: index < token.length - 1, + inlineValue: index < token.length - 1 ? token.slice(index + 1) : undefined, + }); + return options; + } + if (standaloneOptions.has(name) || nonExecutingOptions.has(name)) { + options.push({ name, hasInlineValue: false }); + continue; + } + return null; + } + return options.length > 0 ? options : null; } function knownCarrierOptionConsumesNextValue( - option: ParsedCarrierOption, - standaloneOptions: ReadonlySet, + options: readonly ParsedCarrierOption[], optionsWithValue: ReadonlySet, + nonExecutingOptions: ReadonlySet = new Set(), ): boolean | null { - if (standaloneOptions.has(option.name)) { - return false; + let consumesNextValue = false; + for (const option of options) { + if (nonExecutingOptions.has(option.name)) { + return null; + } + if (optionsWithValue.has(option.name)) { + consumesNextValue = !option.hasInlineValue; + } } - if (optionsWithValue.has(option.name)) { - return !option.hasInlineValue; - } - return null; + return consumesNextValue; } -function resolveEnvSplitPayload(payload: string, depth: number): string[] | null { +function findParsedCarrierOption( + options: readonly ParsedCarrierOption[], + names: ReadonlySet, +): ParsedCarrierOption | undefined { + return options.find((option) => names.has(option.name)); +} + +function resolveEnvSplitPayload( + payload: string, + trailingArgv: string[], + depth: number, +): string[] | null { const innerArgv = splitShellArgs(payload); if (!innerArgv || innerArgv.length === 0) { return null; } - return resolveEnvCarriedArgv(["env", ...innerArgv], depth + 1) ?? innerArgv; + const carriedArgv = [...innerArgv, ...trailingArgv]; + return resolveEnvCarriedArgv(["env", ...carriedArgv], depth + 1) ?? carriedArgv; } function resolveEnvCarriedArgv(argv: string[], depth = 0): string[] | null { - if (depth > MAX_INLINE_EVAL_CARRIER_DEPTH || normalizeExecutableToken(argv[0] ?? "") !== "env") { + if (depth > MAX_ENV_SPLIT_PAYLOAD_DEPTH || normalizeExecutableToken(argv[0] ?? "") !== "env") { return null; } for (let index = 1; index < argv.length; index += 1) { @@ -156,25 +208,20 @@ function resolveEnvCarriedArgv(argv: string[], depth = 0): string[] | null { if (token === "--") { return argv.slice(index + 1); } - if (token === "-S" || token === "--split-string") { - const payload = argv[index + 1]; - return typeof payload === "string" ? resolveEnvSplitPayload(payload, depth) : null; - } - if (token.startsWith("--split-string=")) { - return resolveEnvSplitPayload(token.slice("--split-string=".length), depth); - } - if (token.startsWith("-S") && token.length > 2) { - return resolveEnvSplitPayload(token.slice(2), depth); - } if (token.startsWith("-")) { - const consumeNextValue = knownCarrierOptionConsumesNextValue( - parseCarrierOptionToken(token, ENV_OPTIONS_WITH_VALUE), - ENV_STANDALONE_OPTIONS, - ENV_OPTIONS_WITH_VALUE, - ); - if (consumeNextValue === null) { + const option = parseCarrierOptionToken(token, ENV_STANDALONE_OPTIONS, ENV_OPTIONS_WITH_VALUE); + if (!option) { return null; } + const splitStringOption = findParsedCarrierOption(option, ENV_SPLIT_STRING_OPTIONS); + if (splitStringOption) { + const payloadIndex = splitStringOption.inlineValue === undefined ? index + 1 : index; + const payload = splitStringOption.inlineValue ?? argv[payloadIndex]; + return typeof payload === "string" + ? resolveEnvSplitPayload(payload, argv.slice(payloadIndex + 1), depth) + : null; + } + const consumeNextValue = knownCarrierOptionConsumesNextValue(option, ENV_OPTIONS_WITH_VALUE); if (consumeNextValue) { index += 1; } @@ -234,14 +281,19 @@ function resolveSudoLikeCarriedArgv(argv: string[]): string[] | null { if (!token.startsWith("-")) { return argv.slice(index); } - const option = parseCarrierOptionToken(token, optionsWithValue); - if (executable === "sudo" && SUDO_NON_EXEC_OPTIONS.has(option.name)) { + const option = parseCarrierOptionToken( + token, + standaloneOptions, + optionsWithValue, + executable === "sudo" ? SUDO_NON_EXEC_OPTIONS : undefined, + ); + if (!option) { return null; } const consumeNextValue = knownCarrierOptionConsumesNextValue( option, - standaloneOptions, optionsWithValue, + executable === "sudo" ? SUDO_NON_EXEC_OPTIONS : undefined, ); if (consumeNextValue === null) { return null; @@ -259,9 +311,6 @@ export function resolveCarrierCommandArgv( depth = 0, options?: { includeExec?: boolean }, ): string[] | null { - if (depth > MAX_INLINE_EVAL_CARRIER_DEPTH) { - return null; - } const executable = normalizeExecutableToken(argv[0] ?? ""); switch (executable) { case "env": @@ -291,14 +340,11 @@ function resolveExecCarriedArgv(argv: string[]): string[] | null { if (!token.startsWith("-")) { return argv.slice(index); } - const consumeNextValue = knownCarrierOptionConsumesNextValue( - parseCarrierOptionToken(token, EXEC_OPTIONS_WITH_VALUE), - EXEC_STANDALONE_OPTIONS, - EXEC_OPTIONS_WITH_VALUE, - ); - if (consumeNextValue === null) { + const option = parseCarrierOptionToken(token, EXEC_STANDALONE_OPTIONS, EXEC_OPTIONS_WITH_VALUE); + if (!option) { return null; } + const consumeNextValue = knownCarrierOptionConsumesNextValue(option, EXEC_OPTIONS_WITH_VALUE); if (consumeNextValue) { index += 1; } @@ -307,24 +353,31 @@ function resolveExecCarriedArgv(argv: string[]): string[] | null { return null; } -export function buildCommandPayloadCandidates(argv: string[], depth = 0): string[] { - if (depth > MAX_INLINE_EVAL_CARRIER_DEPTH) { +function commandArgvKey(argv: readonly string[]): string { + return argv.join("\0"); +} + +export function buildCommandPayloadCandidates( + argv: string[], + seenArgv = new Set(), +): string[] { + const key = commandArgvKey(argv); + if (seenArgv.has(key)) { return argv.length > 0 ? [argv.join(" ")] : []; } + seenArgv.add(key); const assignmentStrippedArgv = stripLeadingEnvAssignments(argv); - const carriedArgv = resolveCarrierCommandArgv(assignmentStrippedArgv, depth, { + const carriedArgv = resolveCarrierCommandArgv(assignmentStrippedArgv, 0, { includeExec: true, }); const executableArgv = carriedArgv ?? assignmentStrippedArgv; - const carriedCandidates = carriedArgv - ? buildCommandPayloadCandidates(carriedArgv, depth + 1) - : []; + const carriedCandidates = carriedArgv ? buildCommandPayloadCandidates(carriedArgv, seenArgv) : []; const shellWrapperPayload = extractShellWrapperInlineCommand(executableArgv); const shellWrapperCandidates = shellWrapperPayload ? (() => { const innerArgv = splitShellArgs(shellWrapperPayload); return innerArgv - ? buildCommandPayloadCandidates(innerArgv, depth + 1) + ? buildCommandPayloadCandidates(innerArgv, seenArgv) : [shellWrapperPayload]; })() : []; @@ -347,34 +400,55 @@ function uniqueCommandPayloadCandidates(candidates: string[]): string[] { return [...new Set(candidates.filter((candidate) => candidate.trim().length > 0))]; } -export function detectCarrierInlineEvalArgv( +function detectCarrierInlineEvalArgvInternal( argv: string[], - depth = 0, + seenArgv: Set, ): InterpreterInlineEvalHit | null { - if (depth > MAX_INLINE_EVAL_CARRIER_DEPTH) { + const key = commandArgvKey(argv); + if (seenArgv.has(key)) { return null; } + seenArgv.add(key); + const dispatchUnwrap = unwrapKnownDispatchWrapperInvocation(argv); if (dispatchUnwrap.kind === "unwrapped") { - return detectInlineEvalArgv(dispatchUnwrap.argv, depth + 1); + return detectInlineEvalArgvInternal(dispatchUnwrap.argv, seenArgv); } const executable = normalizeExecutableToken(argv[0] ?? ""); if (!COMMAND_CARRIER_EXECUTABLES.has(executable)) { return null; } - const carriedArgv = resolveCarrierCommandArgv(argv, depth); - return carriedArgv ? detectInlineEvalArgv(carriedArgv, depth + 1) : null; + const carriedArgv = resolveCarrierCommandArgv(argv); + if (!carriedArgv) { + return null; + } + return ( + detectInterpreterInlineEvalArgv(carriedArgv) ?? + detectCarrierInlineEvalArgvInternal(carriedArgv, seenArgv) + ); } -export function detectInlineEvalArgv( +export function detectCarrierInlineEvalArgv(argv: string[]): InterpreterInlineEvalHit | null { + return detectCarrierInlineEvalArgvInternal(argv, new Set()); +} + +function detectInlineEvalArgvInternal( argv: string[] | undefined | null, - depth = 0, + seenArgv: Set, ): InterpreterInlineEvalHit | null { if (!Array.isArray(argv)) { return null; } - return detectInterpreterInlineEvalArgv(argv) ?? detectCarrierInlineEvalArgv(argv, depth); + return ( + detectInterpreterInlineEvalArgv(argv) ?? detectCarrierInlineEvalArgvInternal(argv, seenArgv) + ); +} + +export function detectInlineEvalArgv( + argv: string[] | undefined | null, +): InterpreterInlineEvalHit | null { + return detectInlineEvalArgvInternal(argv, new Set()); } export function detectInlineEvalInSegments( diff --git a/src/infra/dispatch-wrapper-resolution.ts b/src/infra/dispatch-wrapper-resolution.ts index 5f21f7ce0da..c85fca596d6 100644 --- a/src/infra/dispatch-wrapper-resolution.ts +++ b/src/infra/dispatch-wrapper-resolution.ts @@ -1,4 +1,5 @@ import { normalizeLowercaseStringOrEmpty } from "../shared/string-coerce.js"; +import { splitShellArgs } from "../utils/shell-argv.js"; import { normalizeExecutableToken } from "./exec-wrapper-tokens.js"; export const MAX_DISPATCH_WRAPPER_DEPTH = 4; @@ -144,14 +145,20 @@ function scanWrapperInvocation( export function unwrapEnvInvocation(argv: string[]): string[] | null { const parsed = parseEnvInvocationPrelude(argv); - return parsed ? argv.slice(parsed.commandIndex) : null; + return parsed ? (parsed.splitArgv ?? argv.slice(parsed.commandIndex)) : null; } type ParsedEnvInvocationPrelude = { assignmentKeys: string[]; commandIndex: number; + splitArgv?: string[]; }; +function splitEnvSplitStringPayload(payload: string, trailingArgv: string[]): string[] | null { + const splitArgv = splitShellArgs(payload); + return splitArgv && splitArgv.length > 0 ? [...splitArgv, ...trailingArgv] : null; +} + function parseEnvInvocationPrelude(argv: string[]): ParsedEnvInvocationPrelude | null { let idx = 1; let expectsOptionValue = false; @@ -184,6 +191,31 @@ function parseEnvInvocationPrelude(argv: string[]): ParsedEnvInvocationPrelude | } const lower = normalizeLowercaseStringOrEmpty(token); const [flag] = lower.split("=", 2); + if (flag === "-s" || flag === "--split-string") { + const payload = lower.includes("=") ? token.slice(token.indexOf("=") + 1) : argv[idx + 1]; + if (typeof payload !== "string") { + return null; + } + const trailingIndex = lower.includes("=") ? idx + 1 : idx + 2; + const splitArgv = splitEnvSplitStringPayload(payload, argv.slice(trailingIndex)); + return splitArgv + ? { + assignmentKeys, + commandIndex: trailingIndex, + splitArgv, + } + : null; + } + if (lower.startsWith("-s") && lower.length > 2) { + const splitArgv = splitEnvSplitStringPayload(token.slice(2), argv.slice(idx + 1)); + return splitArgv + ? { + assignmentKeys, + commandIndex: idx + 1, + splitArgv, + } + : null; + } if (ENV_FLAG_OPTIONS.has(flag)) { idx += 1; continue; diff --git a/src/infra/exec-wrapper-resolution.test.ts b/src/infra/exec-wrapper-resolution.test.ts index dc7bfed74de..4679580d7eb 100644 --- a/src/infra/exec-wrapper-resolution.test.ts +++ b/src/infra/exec-wrapper-resolution.test.ts @@ -116,6 +116,18 @@ describe("unwrapEnvInvocation", () => { argv: ["env", "--chdir=/tmp", "pwsh", "-Command", "Get-Date"], expected: ["pwsh", "-Command", "Get-Date"], }, + { + argv: ["env", "-S", "python3 -c", "print(1)"], + expected: ["python3", "-c", "print(1)"], + }, + { + argv: ["env", "--split-string=python3 -c", "print(1)"], + expected: ["python3", "-c", "print(1)"], + }, + { + argv: ["env", "-Spython3 -c", "print(1)"], + expected: ["python3", "-c", "print(1)"], + }, { argv: ["env", "-", "bash", "-lc", "echo hi"], expected: ["bash", "-lc", "echo hi"],