From c743974d7cca691be24688b5e155ac60f92d4302 Mon Sep 17 00:00:00 2001 From: Tinson Lai Date: Thu, 4 Jun 2026 17:01:05 +0000 Subject: [PATCH] fix(scripts): extract step-down function source verbatim instead of declare -f Signed-off-by: Tinson Lai --- scripts/nemoclaw-start.sh | 122 ++++++++++++++++++++++++++++++++---- test/nemoclaw-start.test.ts | 84 +++++++++++++++++++++++-- 2 files changed, 191 insertions(+), 15 deletions(-) diff --git a/scripts/nemoclaw-start.sh b/scripts/nemoclaw-start.sh index 114320b4bdf..8adeed3def0 100755 --- a/scripts/nemoclaw-start.sh +++ b/scripts/nemoclaw-start.sh @@ -2713,21 +2713,113 @@ NODE fi } +# Extract the literal source of a bash function from its defining file. +# +# Uses `shopt -s extdebug` + `declare -F` to look up the function's +# source location, then prints the function definition byte-exact from +# disk. The opener line MUST match ^\(\) \{$ and the body MUST +# end with a single `}` at column 0; every function dispatched through +# run_step_down_as_sandbox follows that style. +# +# This bypasses `declare -f`'s serialiser, which mis-orders the body of +# functions whose `if`/`while`/`until` condition is a here-doc command: +# `declare -f` places the indented `then`-body command immediately after +# the `</dev/null; then + return 1 + fi + info="$(declare -F "$fn" 2>/dev/null)" + shopt -u extdebug 2>/dev/null || true + if [ -z "$info" ]; then + return 1 + fi + src_lineno="${info#* }" + src_lineno="${src_lineno%% *}" + src_path="${info#* * }" + if [ -z "$src_lineno" ] || [ -z "$src_path" ] || [ ! -r "$src_path" ]; then + return 1 + fi + awk -v start="$src_lineno" -v fn="$fn" ' + NR == start { + # One-liner shape: `name() { body; }` — entire definition on one line. + # No heredoc is possible in this shape, so emit and stop. + if ($0 ~ "^"fn"[[:space:]]*\\(\\)[[:space:]]*\\{.*\\}[[:space:]]*$") { + print + exit 0 + } + # Multi-line shape: `name() {` opener, with the matching `}` on its + # own line at column 0 at the end of the body. Both production + # call sites and the test stubs that exercise here-docs follow + # this convention. + if ($0 !~ "^"fn"[[:space:]]*\\(\\)[[:space:]]*\\{[[:space:]]*$") { + exit 1 + } + in_fn = 1 + print + next + } + !in_fn { next } + in_heredoc { + print + if ($0 == heredoc_tag) in_heredoc = 0 + next + } + { + print + if (match($0, /<<-?[[:space:]]*['"'"'"]?[A-Za-z_][A-Za-z0-9_]*['"'"'"]?/)) { + tag = substr($0, RSTART, RLENGTH) + sub(/^<<-?[[:space:]]*/, "", tag) + sub(/^['"'"'"]/, "", tag) + sub(/['"'"'"]$/, "", tag) + in_heredoc = 1 + heredoc_tag = tag + next + } + if ($0 == "}") exit + } + END { if (in_fn && in_heredoc) exit 1 } + ' "$src_path" +} + # Run one or more locally-defined bash functions as the sandbox user -# without round-tripping through `bash -c "$(declare -f ...) ..."`. +# without round-tripping through `bash -c "$(declare -f ...) ..."` and +# without going through `declare -f`'s serialiser at all. # -# The interpolated form is fragile under restricted runtimes: the -# step-down shell cannot always re-parse a heredoc-bearing function -# body carried through `bash -c`'s argv. Writing the declarations plus -# the trailing invocation to a temp script and invoking `bash ` -# instead lets the step-down shell read the literal source bytes from -# disk so the argv/quoting round-trip is gone. +# The interpolated argv form was fragile because the step-down shell +# could not always re-parse a here-doc-bearing function body carried +# through `bash -c`'s argv. The earlier in-house fix routed function +# bodies through `declare -f` plus a temp file, which removed the argv +# round-trip but kept `declare -f`'s body-reordering bug for here-doc +# `if` conditions. This helper now copies each named function's source +# verbatim from `${BASH_SOURCE[0]}` (resolved per function via the +# extdebug machinery), so every here-doc shape — condition, body, +# trailing — survives the dispatch unchanged. # # The temp script lives directly under /tmp (sticky-bit, world-writable # but unlink-protected) with an unguessable mktemp suffix, so an # attacker cannot swap the file between mktemp and the step-down bash # invocation. The directory is intentionally not configurable. # +# A `bash -n` syntax check runs on the assembled script before the +# step-down invocation. It is a fail-closed guard: if a future change +# ever produces a malformed temp script (for example, a dispatched +# function that violates the opener/closer style assumption), we abort +# before handing the broken script to step-down, surfacing a clean +# error instead of the obscure `unexpected token 'fi'` failure that +# this helper exists to prevent. +# # Usage: run_step_down_as_sandbox ... # # SECURITY CONTRACT: is appended verbatim to the @@ -2747,14 +2839,22 @@ run_step_down_as_sandbox() { rm -f "$script" 2>/dev/null || true return 1 fi - { + if ! ( printf 'set -euo pipefail\n' - declare -f "$@" + for fn in "$@"; do + _step_down_extract_function "$fn" || exit 1 + done printf '%s\n' "$invocation" - } >"$script" || { + ) >"$script"; then rm -f "$script" 2>/dev/null || true + printf '[step-down] failed to assemble dispatch script\n' >&2 return 1 - } + fi + if ! bash -n "$script" 2>/dev/null; then + rm -f "$script" 2>/dev/null || true + printf '[step-down] generated dispatch script failed bash -n syntax check\n' >&2 + return 1 + fi local rc=0 "${STEP_DOWN_PREFIX_SANDBOX[@]}" bash "$script" || rc=$? rm -f "$script" 2>/dev/null || true diff --git a/test/nemoclaw-start.test.ts b/test/nemoclaw-start.test.ts index 3ac91aad5ca..a33b76a9d1e 100644 --- a/test/nemoclaw-start.test.ts +++ b/test/nemoclaw-start.test.ts @@ -2701,7 +2701,10 @@ describe("seed_default_workspace_templates (#3240)", () => { const configPath = path.join(tmpDir, "openclaw.json"); fs.writeFileSync(configPath, JSON.stringify({ agents: { defaults: { skipBootstrap: true } } })); const scriptPath = path.join(tmpDir, "seed-as-sandbox.sh"); - const runStepDown = extractShellFunctionFromSource(src, "run_step_down_as_sandbox"); + const runStepDown = [ + extractShellFunctionFromSource(src, "_step_down_extract_function"), + extractShellFunctionFromSource(src, "run_step_down_as_sandbox"), + ].join("\n"); const seedAsSandbox = extractShellFunctionFromSource( src, "seed_default_workspace_templates_as_sandbox", @@ -4111,7 +4114,10 @@ describe("openclaw.json baseline + recovery (#3118)", () => { describe("run_step_down_as_sandbox", () => { const src = fs.readFileSync(START_SCRIPT, "utf-8"); - const helper = extractShellFunctionFromSource(src, "run_step_down_as_sandbox"); + const helper = [ + extractShellFunctionFromSource(src, "_step_down_extract_function"), + extractShellFunctionFromSource(src, "run_step_down_as_sandbox"), + ].join("\n"); it("dispatches via a temp script and cleans up after success", () => { const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), "nemoclaw-step-down-helper-")); @@ -4171,6 +4177,70 @@ describe("run_step_down_as_sandbox", () => { } }); + it("survives a heredoc used as an if-condition's command without bash declare -f reordering the then-body into the heredoc", () => { + // Regression: bash `declare -f` serialises a function whose `if` + // condition is a heredoc-bearing command by placing the indented + // `then`-body command BEFORE the heredoc closer. When the + // step-down shell re-parses that output, it consumes the displaced + // command as part of the heredoc body, leaves the `then` block + // empty, and aborts on the closing `fi` with + // syntax error near unexpected token `fi' + // (the exact text NV QA reported on v0.0.58 after the earlier fix + // that handled only the heredoc-as-last-statement shape). The new + // helper bypasses `declare -f` and reads the function source + // verbatim from disk via `shopt -s extdebug` + `declare -F`, so + // every here-doc placement survives intact. + const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), "nemoclaw-step-down-heredoc-if-")); + const stepDownLog = path.join(tmpDir, "step-down.log"); + const sentinel = path.join(tmpDir, "ran.txt"); + const scriptPath = path.join(tmpDir, "run.sh"); + fs.writeFileSync( + scriptPath, + [ + "#!/usr/bin/env bash", + "set -euo pipefail", + `STEP_DOWN_PREFIX_SANDBOX=(bash -c 'printf "%s\\n" "$2" >${JSON.stringify(stepDownLog)}; exec "$@"' sandbox-step-down)`, + `SENTINEL=${JSON.stringify(sentinel)}`, + // Mirror seed_default_workspace_templates' broken shape exactly: + // a heredoc-bearing `node` invocation as the `if` condition, + // with a `then`-body command, followed by `fi`. This is the + // shape `declare -f` mangles in bash 5.x. + "heredoc_in_if_condition() {", + " local marker=\"$1\"", + " if ! node - \"$marker\" <<'NODE' >/dev/null 2>&1; then", + "const fs = require(\"fs\");", + "const target = process.argv[2];", + "fs.writeFileSync(target, \"ran-via-heredoc-if\\n\");", + "process.exit(0);", + "NODE", + " return 0", + " fi", + "}", + helper, + "run_step_down_as_sandbox 'heredoc_in_if_condition \"$SENTINEL\"' heredoc_in_if_condition", + ].join("\n"), + { mode: 0o700 }, + ); + try { + const result = spawnSync("bash", [scriptPath], { + encoding: "utf-8", + env: { ...process.env, SENTINEL: sentinel }, + timeout: 5000, + }); + expect(result.status, result.stderr || result.stdout).toBe(0); + expect(result.stderr).not.toContain("syntax error near unexpected token `fi'"); + expect(result.stderr).not.toContain("bash -n syntax check"); + // The heredoc body ran in the step-down shell: it wrote the sentinel. + expect(fs.existsSync(sentinel)).toBe(true); + expect(fs.readFileSync(sentinel, "utf-8")).toBe("ran-via-heredoc-if\n"); + const tempScriptPath = fs.readFileSync(stepDownLog, "utf-8").trim(); + expect(tempScriptPath).toMatch(/^\/tmp\/nemoclaw-step-down-[A-Za-z0-9]{6}\.sh$/); + expect(fs.existsSync(tempScriptPath)).toBe(false); + } finally { + fs.rmSync(tmpDir, { recursive: true, force: true }); + } + }); + it("survives heredoc-bearing function bodies through the temp-script round-trip", () => { // The production caller passes functions whose bodies contain a // `<<'TAG'` heredoc (e.g. `python3 - <<'PYAUTH' ...`). This test @@ -4239,7 +4309,10 @@ describe("run_step_down_as_sandbox", () => { describe("setup_auth_profile_as_sandbox", () => { const src = fs.readFileSync(START_SCRIPT, "utf-8"); - const helper = extractShellFunctionFromSource(src, "run_step_down_as_sandbox"); + const helper = [ + extractShellFunctionFromSource(src, "_step_down_extract_function"), + extractShellFunctionFromSource(src, "run_step_down_as_sandbox"), + ].join("\n"); const setup = extractShellFunctionFromSource(src, "setup_auth_profile_as_sandbox"); it("runs the auth-profile setup under HOME=/sandbox even when the parent env has HOME=/root", () => { @@ -4506,7 +4579,10 @@ describe("direct-root entrypoint composition under CAP_DAC_OVERRIDE drop", () => const writeRuntimeEnv = src .slice(writeRuntimeStart, writeRuntimeEnd) .replaceAll("/tmp/nemoclaw-proxy-env.sh", proxyEnvFile); - const helper = extractShellFunctionFromSource(src, "run_step_down_as_sandbox"); + const helper = [ + extractShellFunctionFromSource(src, "_step_down_extract_function"), + extractShellFunctionFromSource(src, "run_step_down_as_sandbox"), + ].join("\n"); const setupAuth = extractShellFunctionFromSource(src, "setup_auth_profile_as_sandbox"); fs.writeFileSync( scriptPath,