From 02e3d7eb9fcbfbad9799779e66a1083e629ab78d Mon Sep 17 00:00:00 2001 From: Daniel Morandini Date: Mon, 15 Jun 2026 15:28:11 +0200 Subject: [PATCH] Fix SSH sandbox remote directory args --- extensions/openshell/src/backend.ts | 26 ++++++------- .../openshell/src/openshell-core.test.ts | 38 ++++++++++++++++++- src/agents/sandbox/ssh.test.ts | 25 ++++++++++++ src/agents/sandbox/ssh.ts | 22 +++++------ 4 files changed, 85 insertions(+), 26 deletions(-) diff --git a/extensions/openshell/src/backend.ts b/extensions/openshell/src/backend.ts index 1a0a50ec05f3..4b68c9951aee 100644 --- a/extensions/openshell/src/backend.ts +++ b/extensions/openshell/src/backend.ts @@ -146,7 +146,7 @@ export const PINNED_REMOTE_PATH_MUTATION_SCRIPT = [ " ;;", "esac", ].join("\n"); -const ENSURE_REMOTE_REAL_DIRECTORY_SCRIPT = [ +export const ENSURE_OPEN_SHELL_REMOTE_REAL_DIRECTORY_SCRIPT = [ "set -e", 'target="$1"', 'root="${2:-$1}"', @@ -157,13 +157,14 @@ const ENSURE_REMOTE_REAL_DIRECTORY_SCRIPT = [ '[ -n "$target" ] || target="/"', '[ -n "$root" ] || root="/"', 'case "$target/" in "$root"/*|"$root/") ;; *) echo "remote directory must stay under root: $target" >&2; exit 1 ;; esac', - 'old_ifs="$IFS"', - 'IFS="/"', - "set -- ${target#/} ${root#/}", - 'IFS="$old_ifs"', - "for part do", - ' [ -n "$part" ] || continue', - ' case "$part" in "."|"..") echo "unsafe remote directory component: $part" >&2; exit 1 ;; esac', + 'for path_to_check in "$target" "$root"; do', + ' relative="${path_to_check#/}"', + ' while [ -n "$relative" ]; do', + ' part="${relative%%/*}"', + ' if [ "$part" = "$relative" ]; then relative=""; else relative="${relative#*/}"; fi', + ' [ -n "$part" ] || continue', + ' case "$part" in "."|"..") echo "unsafe remote directory component: $part" >&2; exit 1 ;; esac', + " done", "done", 'if [ -L "$root" ]; then echo "unsafe remote root symlink: $root" >&2; exit 1; fi', 'mkdir -p -- "$root"', @@ -171,10 +172,9 @@ const ENSURE_REMOTE_REAL_DIRECTORY_SCRIPT = [ 'relative="${target#"$root"}"', 'relative="${relative#/}"', 'current="$canonical_root"', - 'IFS="/"', - "set -- $relative", - 'IFS="$old_ifs"', - "for part do", + 'while [ -n "$relative" ]; do', + ' part="${relative%%/*}"', + ' if [ "$part" = "$relative" ]; then relative=""; else relative="${relative#*/}"; fi', ' [ -n "$part" ] || continue', ' if [ "$current" = "/" ]; then next="/$part"; else next="$current/$part"; fi', ' if [ -L "$next" ]; then echo "unsafe remote directory symlink: $next" >&2; exit 1; fi', @@ -678,7 +678,7 @@ class OpenShellSandboxBackendImpl { this.params.remoteWorkspaceDir, ); await this.runRemoteShellScriptInternal({ - script: `${ENSURE_REMOTE_REAL_DIRECTORY_SCRIPT}\nfind "$1" -mindepth 1 -maxdepth 1 -exec rm -rf -- {} +`, + script: `${ENSURE_OPEN_SHELL_REMOTE_REAL_DIRECTORY_SCRIPT}\nfind "$1" -mindepth 1 -maxdepth 1 -exec rm -rf -- {} +`, args: [remoteSkillsWorkspaceDir, this.params.remoteWorkspaceDir], }); const stats = await fs.lstat(this.params.createParams.skillsWorkspaceDir).catch(() => null); diff --git a/extensions/openshell/src/openshell-core.test.ts b/extensions/openshell/src/openshell-core.test.ts index d5ac6945154b..36a553237e1e 100644 --- a/extensions/openshell/src/openshell-core.test.ts +++ b/extensions/openshell/src/openshell-core.test.ts @@ -29,6 +29,7 @@ const cliMocks = vi.hoisted(() => ({ let createOpenShellSandboxBackendManager: typeof import("./backend.js").createOpenShellSandboxBackendManager; let createOpenShellSandboxBackendFactory: typeof import("./backend.js").createOpenShellSandboxBackendFactory; +let ensureOpenShellRemoteRealDirectoryScript: typeof import("./backend.js").ENSURE_OPEN_SHELL_REMOTE_REAL_DIRECTORY_SCRIPT; describe("openshell cli helpers", () => { const originalEnv = { ...process.env }; @@ -170,8 +171,11 @@ describe("openshell backend manager", () => { runOpenShellCli: cliMocks.runOpenShellCli, }; }); - ({ createOpenShellSandboxBackendFactory, createOpenShellSandboxBackendManager } = - await import("./backend.js")); + ({ + ENSURE_OPEN_SHELL_REMOTE_REAL_DIRECTORY_SCRIPT: ensureOpenShellRemoteRealDirectoryScript, + createOpenShellSandboxBackendFactory, + createOpenShellSandboxBackendManager, + } = await import("./backend.js")); }); afterAll(() => { @@ -183,6 +187,36 @@ describe("openshell backend manager", () => { vi.clearAllMocks(); }); + it.runIf(process.platform !== "win32")( + "preserves caller positional args after OpenShell remote directory validation", + async () => { + const realParent = await makeTempDir("openclaw-openshell-real-"); + const root = path.join(realParent, "sandbox"); + const target = path.join(root, ".openclaw", "sandbox-skills"); + + const result = spawnSync( + "/bin/sh", + [ + "-c", + [ + ensureOpenShellRemoteRealDirectoryScript, + 'printf "%s\\n%s\\n" "$1" "$2"', + 'touch "$1/proof"', + 'find "$1" -mindepth 1 -maxdepth 1 -name proof -print', + ].join("\n"), + "openclaw-openshell-dir", + target, + root, + ], + { encoding: "utf8" }, + ); + + expect(result.status).toBe(0); + expect(result.stderr).toBe(""); + expect(result.stdout.trim().split("\n")).toEqual([target, root, path.join(target, "proof")]); + }, + ); + it("checks runtime status with config override from OpenClaw config", async () => { cliMocks.runOpenShellCli.mockResolvedValue({ code: 0, diff --git a/src/agents/sandbox/ssh.test.ts b/src/agents/sandbox/ssh.test.ts index 1ceb9c26a074..2e9dae6739d5 100644 --- a/src/agents/sandbox/ssh.test.ts +++ b/src/agents/sandbox/ssh.test.ts @@ -6,6 +6,7 @@ import os from "node:os"; import path from "node:path"; import { promisify } from "node:util"; import { afterEach, describe, expect, it } from "vitest"; +import { makeTempDir } from "../../../test/helpers/temp-dir.js"; import { buildExecRemoteCommand, buildValidatedExecRemoteCommand, @@ -212,6 +213,30 @@ describe("sandbox ssh helpers", () => { }, ); + it.runIf(process.platform !== "win32")( + "preserves caller positional args for commands after remote directory validation", + async () => { + const realParent = makeTempDir(tempDirs, "openclaw-ssh-real-"); + const root = path.join(realParent, "runtime"); + const target = path.join(root, "workspace", ".openclaw", "sandbox-skills"); + + const { stdout } = await execFileAsync("/bin/sh", [ + "-c", + [ + ENSURE_REMOTE_REAL_DIRECTORY_SCRIPT, + 'printf "%s\\n%s\\n" "$1" "$2"', + 'touch "$1/proof"', + 'find "$1" -mindepth 1 -maxdepth 1 -name proof -print', + ].join("\n"), + "openclaw-remote-dir", + target, + root, + ]); + + expect(stdout.trim().split("\n")).toEqual([target, root, path.join(target, "proof")]); + }, + ); + it.runIf(process.platform !== "win32")( "rejects symlinked directories inside the trusted remote root", async () => { diff --git a/src/agents/sandbox/ssh.ts b/src/agents/sandbox/ssh.ts index 643adae511ea..95f76e8e0541 100644 --- a/src/agents/sandbox/ssh.ts +++ b/src/agents/sandbox/ssh.ts @@ -666,13 +666,14 @@ export const ENSURE_REMOTE_REAL_DIRECTORY_SCRIPT = [ '[ -n "$target" ] || target="/"', '[ -n "$root" ] || root="/"', 'case "$target/" in "$root"/*|"$root/") ;; *) echo "remote directory must stay under root: $target" >&2; exit 1 ;; esac', - 'old_ifs="$IFS"', - 'IFS="/"', - "set -- ${target#/} ${root#/}", - 'IFS="$old_ifs"', - "for part do", - ' [ -n "$part" ] || continue', - ' case "$part" in "."|"..") echo "unsafe remote directory component: $part" >&2; exit 1 ;; esac', + 'for path_to_check in "$target" "$root"; do', + ' relative="${path_to_check#/}"', + ' while [ -n "$relative" ]; do', + ' part="${relative%%/*}"', + ' if [ "$part" = "$relative" ]; then relative=""; else relative="${relative#*/}"; fi', + ' [ -n "$part" ] || continue', + ' case "$part" in "."|"..") echo "unsafe remote directory component: $part" >&2; exit 1 ;; esac', + " done", "done", 'if [ -L "$root" ]; then echo "unsafe remote root symlink: $root" >&2; exit 1; fi', 'mkdir -p -- "$root"', @@ -680,10 +681,9 @@ export const ENSURE_REMOTE_REAL_DIRECTORY_SCRIPT = [ 'relative="${target#"$root"}"', 'relative="${relative#/}"', 'current="$canonical_root"', - 'IFS="/"', - "set -- $relative", - 'IFS="$old_ifs"', - "for part do", + 'while [ -n "$relative" ]; do', + ' part="${relative%%/*}"', + ' if [ "$part" = "$relative" ]; then relative=""; else relative="${relative#*/}"; fi', ' [ -n "$part" ] || continue', ' if [ "$current" = "/" ]; then next="/$part"; else next="$current/$part"; fi', ' if [ -L "$next" ]; then echo "unsafe remote directory symlink: $next" >&2; exit 1; fi',