From 1fa1507fc4f95bc90c1ea9e8dd5bb4a343ea1186 Mon Sep 17 00:00:00 2001 From: Sebastien Tardif Date: Sat, 11 Jul 2026 02:00:53 -0400 Subject: [PATCH] fix(install): harden stdin consumers to prevent pipe corruption in curl | bash (#87799) * fix(install): harden stdin consumers to prevent pipe corruption in curl | bash Redirect stdin from /dev/null for non-interactive subprocesses (npm install, openclaw daemon restart, openclaw plugins update, openclaw dashboard) and from /dev/tty for interactive ones (openclaw onboard in bootstrap). Also protect the fallback paths in run_with_spinner and run_quiet_step. This prevents subprocesses from consuming the script stream when the installer is piped via curl | bash, which causes truncated function names and hangs (reported in #73814). Unlike the global pipe guard in #82918 (closed as too broad), this approach has no detection heuristics and no re-execution. Each subprocess simply gets the correct stdin for its purpose. Fixes #73814 Signed-off-by: Sebastien Tardif * chore: retrigger proof evaluation Signed-off-by: Sebastien Tardif * fix: redirect stdin in run_with_spinner raw-mode fallback The success-path raw-mode fallback in run_with_spinner invoked the command with inherited stdin. In a curl | bash scenario, this allowed the child process to consume bytes from the script stream, causing truncation. Add < /dev/null to match the other two fallback paths. * retrigger proof check * fix(test): update gum fallback assertion for stdin redirect * fix: redirect gum-wrapped stdin and preserve TTY for interactive commands Address ClawSweeper P1 and P2 findings: - P1: Redirect stdin from /dev/null on the normal gum spin path so child commands cannot consume the piped script stream (gum v0.17.0 passes os.Stdin to wrapped commands). - P2: Use is_non_interactive_shell to conditionally redirect stdin in fallback paths. When running interactively (bash install.sh), commands that need user input (e.g. Homebrew prompts) keep terminal stdin. When piped (curl | bash), stdin is redirected from /dev/null. - P2: Revert labeler.yml changes to keep the PR focused on installer stdin safety. Size-label best-effort handling belongs in a separate PR. Signed-off-by: Sebastien Tardif * chore: retrigger proof evaluation Signed-off-by: Sebastien Tardif * retrigger proof check * fix: base stdin isolation on stdin TTY check, not stdout Replace is_non_interactive_shell with needs_stdin_isolation for stdin redirection decisions. The new function checks stdin directly (! -t 0) and NO_PROMPT, without checking stdout (-t 1). This ensures that stdout redirection (e.g. install.sh > log.txt) does not suppress interactive prompts when stdin is a terminal. * test(install): add stdin isolation and NO_PROMPT coverage Three focused tests for the needs_stdin_isolation function: 1. Verify needs_stdin_isolation returns true when stdin is piped (the core curl|bash scenario). 2. Verify needs_stdin_isolation returns true when NO_PROMPT=1 is set (explicit non-interactive override). 3. Verify run_quiet_step redirects subprocess stdin to /dev/null when running in a piped context, preventing script consumption. * test(install): strengthen stdin isolation test with sentinel data Replace the weak TTY check (which passes even without the fix since the test pipe is also non-TTY) with a sentinel-based test: pipe SENTINEL_DATA on stdin and verify the child process reads nothing, proving run_quiet_step actually redirects stdin to /dev/null. * test(install): add counterproof and cat-based stdin isolation tests Add two new tests to prove the stdin isolation fix is necessary and works correctly: - counterproof: demonstrates that pipe data DOES leak to the child when stdin is not redirected through run_quiet_step, proving the /dev/null redirect is the isolation barrier - cat-based test: uses cat (reads all of stdin) instead of read -t 1 (timeout-based) for a deterministic assertion that run_quiet_step produces empty stdin * fix: gate gum spin stdin redirect on needs_stdin_isolation The gum spin command unconditionally redirected stdin from /dev/null, which also killed interactive prompts for direct installs since gum v0.17 passes os.Stdin to the wrapped command. Now only redirect stdin when needs_stdin_isolation returns true (piped install context). Adds focused tests verifying both paths: piped installs get /dev/null redirect, direct interactive installs preserve terminal stdin. * test: assert gum stdin is not /dev/null for direct interactive installs The direct gum runtime test now reads the child command's stdin-source log file and asserts stdin was NOT /dev/null, using device:inode comparison (stat -f on macOS, stat -c on Linux) for reliable detection across both platforms. * retrigger proof check * retrigger proof check with real installer evidence Signed-off-by: Sebastien Tardif * fix(install): preserve interactive post-install stdin * fix(install): route piped prompts through controlling tty * fix(install): keep quiet piped steps noninteractive * fix(install): avoid hidden prompts with redirected output * fix(install): preserve visible prompt output state --------- Signed-off-by: Sebastien Tardif Co-authored-by: Vincent Koc <25068+vincentkoc@users.noreply.github.com> --- scripts/install.sh | 86 ++++++++-- test/scripts/install-sh.test.ts | 271 +++++++++++++++++++++++++++++++- 2 files changed, 343 insertions(+), 14 deletions(-) diff --git a/scripts/install.sh b/scripts/install.sh index 99d901d76a06..12e7651a5692 100755 --- a/scripts/install.sh +++ b/scripts/install.sh @@ -140,6 +140,14 @@ is_non_interactive_shell() { return 1 } +# Returns true when stdin should be isolated from the script stream. +# Checks stdin directly (not stdout) and respects NO_PROMPT so that +# stdout redirection (e.g. install.sh > log.txt) does not suppress +# interactive prompts. +needs_stdin_isolation() { + [[ ! -t 0 ]] || [[ "${NO_PROMPT:-0}" == "1" ]] +} + has_controlling_tty() { if [[ ! -r /dev/tty || ! -w /dev/tty ]]; then return 1 @@ -150,6 +158,39 @@ has_controlling_tty() { return 0 } +has_visible_prompt_output() { + [[ -t 1 ]] +} + +resolve_subprocess_stdin_path() { + local prompt_output_visible="${1:-0}" + if [[ "${NO_PROMPT:-0}" == "1" ]]; then + echo "/dev/null" + return 0 + fi + if ! needs_stdin_isolation; then + return 1 + fi + if has_controlling_tty && [[ "$prompt_output_visible" == "1" ]]; then + echo "/dev/tty" + else + echo "/dev/null" + fi +} + +run_with_safe_stdin() { + local stdin_path="" + local prompt_output_visible=0 + if has_visible_prompt_output; then + prompt_output_visible=1 + fi + if stdin_path="$(resolve_subprocess_stdin_path "$prompt_output_visible")"; then + "$@" < "$stdin_path" + else + "$@" + fi +} + gum_is_tty() { if [[ -n "${NO_COLOR:-}" ]]; then return 1 @@ -483,13 +524,23 @@ run_with_spinner() { local gum_err gum_out mktempfile gum_err mktempfile gum_out - if "$GUM" spin --spinner dot --title "$title" -- "$@" >"$gum_out" 2>"$gum_err"; then + local gum_status=0 + if needs_stdin_isolation; then + "$GUM" spin --spinner dot --title "$title" -- "$@" < /dev/null >"$gum_out" 2>"$gum_err" || gum_status=$? + else + "$GUM" spin --spinner dot --title "$title" -- "$@" >"$gum_out" 2>"$gum_err" || gum_status=$? + fi + if [[ "$gum_status" -eq 0 ]]; then if is_gum_raw_mode_failure "$gum_out" || is_gum_raw_mode_failure "$gum_err"; then GUM="" GUM_STATUS="skipped" GUM_REASON="gum raw mode unavailable" ui_warn "Spinner unavailable in this terminal; continuing without spinner" - "$@" + if needs_stdin_isolation; then + "$@" < /dev/null + else + "$@" + fi return $? fi if [[ -s "$gum_out" ]]; then @@ -497,13 +548,16 @@ run_with_spinner() { fi return 0 fi - local gum_status=$? if is_gum_raw_mode_failure "$gum_err" || is_gum_raw_mode_failure "$gum_out"; then GUM="" GUM_STATUS="skipped" GUM_REASON="gum raw mode unavailable" ui_warn "Spinner unavailable in this terminal; continuing without spinner" - "$@" + if needs_stdin_isolation; then + "$@" < /dev/null + else + "$@" + fi return $? fi if [[ -s "$gum_err" ]]; then @@ -512,7 +566,11 @@ run_with_spinner() { return "$gum_status" fi - "$@" + if needs_stdin_isolation; then + "$@" < /dev/null + else + "$@" + fi } run_quiet_step() { @@ -544,7 +602,11 @@ run_quiet_step() { # Keep users informed even when gum spinner cannot run (for example shell functions). ui_info "${title}" showed_progress=true - "$@" >"$log" 2>&1 || cmd_exit=$? + if needs_stdin_isolation; then + "$@" < /dev/null >"$log" 2>&1 || cmd_exit=$? + else + "$@" >"$log" 2>&1 || cmd_exit=$? + fi if (( cmd_exit == 0 )); then return 0 fi @@ -915,7 +977,7 @@ run_npm_global_install() { LAST_NPM_INSTALL_CMD="${cmd_display% }" if [[ "$VERBOSE" == "1" ]]; then - "${cmd[@]}" 2>&1 | tee "$log" + "${cmd[@]}" < /dev/null 2>&1 | tee "$log" return $? fi @@ -929,7 +991,7 @@ run_npm_global_install() { fi ui_info "Installing OpenClaw package" - "${cmd[@]}" >"$log" 2>&1 + "${cmd[@]}" < /dev/null >"$log" 2>&1 } extract_npm_debug_log_path() { @@ -1997,7 +2059,7 @@ fix_npm_permissions() { ui_warn "The installer will switch npm's user prefix to ${HOME}/.npm-global; npm normally writes that setting to ~/.npmrc." ui_info "Configuring npm for user-local installs" mkdir -p "$HOME/.npm-global" - npm config set prefix "$HOME/.npm-global" + npm config set prefix "$HOME/.npm-global" < /dev/null ui_warn "Avoid sudo npm i -g for future OpenClaw updates; use npm i -g openclaw@latest so npm keeps using this user prefix instead of a different global prefix." persist_shell_path_prepend "$HOME/.npm-global/bin" "\$HOME/.npm-global/bin" || true @@ -2913,7 +2975,7 @@ maybe_open_dashboard() { if ! "$claw" dashboard --help >/dev/null 2>&1; then return 0 fi - "$claw" dashboard || true + run_with_safe_stdin "$claw" dashboard || true } has_openclaw_config() { @@ -3364,7 +3426,7 @@ main() { if (( doctor_ok )); then should_open_dashboard=true ui_info "Updating plugins" - OPENCLAW_UPDATE_IN_PROGRESS=1 "$claw" plugins update --all || true + OPENCLAW_UPDATE_IN_PROGRESS=1 run_with_safe_stdin "$claw" plugins update --all || true else ui_warn "Doctor failed; skipping plugin updates" fi @@ -3396,7 +3458,7 @@ main() { ui_info "Gateway daemon detected; would restart (${user_claw} daemon restart)" else ui_info "Gateway daemon detected; restarting" - if OPENCLAW_UPDATE_IN_PROGRESS=1 "$claw" daemon restart >/dev/null 2>&1; then + if OPENCLAW_UPDATE_IN_PROGRESS=1 "$claw" daemon restart < /dev/null >/dev/null 2>&1; then ui_success "Gateway restarted" else ui_warn "Gateway restart failed; try: ${user_claw} daemon restart" diff --git a/test/scripts/install-sh.test.ts b/test/scripts/install-sh.test.ts index e463b74448eb..356ad77a4bc3 100644 --- a/test/scripts/install-sh.test.ts +++ b/test/scripts/install-sh.test.ts @@ -1972,7 +1972,10 @@ describe("install.sh macOS Homebrew Node behavior", () => { it("falls back when gum reports raw-mode ioctl failures", () => { expect(script).toContain("setrawmode|inappropriate ioctl"); expect(script).toContain( - 'if "$GUM" spin --spinner dot --title "$title" -- "$@" >"$gum_out" 2>"$gum_err"; then', + '"$GUM" spin --spinner dot --title "$title" -- "$@" < /dev/null >"$gum_out" 2>"$gum_err" || gum_status=$?', + ); + expect(script).toContain( + '"$GUM" spin --spinner dot --title "$title" -- "$@" >"$gum_out" 2>"$gum_err" || gum_status=$?', ); expect(script).toContain( 'if is_gum_raw_mode_failure "$gum_out" || is_gum_raw_mode_failure "$gum_err"; then', @@ -1980,7 +1983,9 @@ describe("install.sh macOS Homebrew Node behavior", () => { expect(script).toContain( 'ui_warn "Spinner unavailable in this terminal; continuing without spinner"', ); - expect(script).toContain('"$@"\n return $?'); + expect(script).toContain( + 'if needs_stdin_isolation; then\n "$@" < /dev/null\n else\n "$@"\n fi\n return $?', + ); }); it("reruns spinner-wrapped commands when gum reports ioctl failure", () => { @@ -2016,6 +2021,65 @@ describe("install.sh macOS Homebrew Node behavior", () => { rmSync(dir, { recursive: true, force: true }); } }); + + it("gum spin preserves terminal stdin for direct interactive installs", () => { + // When needs_stdin_isolation returns false (direct interactive run), + // gum spin should NOT redirect stdin from /dev/null so that wrapped + // commands like Homebrew can still prompt the user via stdin. + const dir = mkdtempSync(join(tmpdir(), "openclaw-install-sh-gum-stdin-")); + try { + const gumPath = join(dir, "gum"); + const commandPath = join(dir, "command"); + const stdinLog = join(dir, "stdin-source"); + // Gum stub: skip args up to and including "--", then run the rest + writeFileSync( + gumPath, + '#!/usr/bin/env bash\nwhile [[ "$#" -gt 0 && "$1" != "--" ]]; do shift; done\nshift\n"$@"\n', + { mode: 0o755 }, + ); + // Command: detects whether stdin is literally /dev/null by comparing + // device:inode of fd 0 against /dev/null (reliable across macOS/Linux) + writeFileSync( + commandPath, + `#!/usr/bin/env bash +stdin_dev=$(stat -f '%d:%i' /dev/fd/0 2>/dev/null || stat -c '%d:%i' /dev/fd/0 2>/dev/null) +null_dev=$(stat -f '%d:%i' /dev/null 2>/dev/null || stat -c '%d:%i' /dev/null 2>/dev/null) +if [ "$stdin_dev" = "$null_dev" ]; then echo "devnull" > "${stdinLog}"; else echo "other" > "${stdinLog}"; fi +exit 0 +`, + { mode: 0o755 }, + ); + + const result = runInstallShell(` + set -euo pipefail + source "${SCRIPT_PATH}" + # Override needs_stdin_isolation to return false (direct interactive) + needs_stdin_isolation() { return 1; } + gum_is_tty() { return 0; } + GUM="${gumPath}" + run_with_spinner "Installing node" "${commandPath}" + `); + + // The gum spin command should NOT have redirected stdin from /dev/null + expect(result.status).toBe(0); + // Assert the child command's stdin was NOT /dev/null + const observed = readFileSync(stdinLog, "utf8").trim(); + expect(observed).toBe("other"); + expect(script).toContain("needs_stdin_isolation; then"); + expect(script).toContain( + '"$GUM" spin --spinner dot --title "$title" -- "$@" >"$gum_out" 2>"$gum_err" || gum_status=$?', + ); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }); + + it("gum spin redirects stdin from /dev/null for piped installs", () => { + expect(script).toContain("needs_stdin_isolation; then"); + expect(script).toContain( + '"$GUM" spin --spinner dot --title "$title" -- "$@" < /dev/null >"$gum_out" 2>"$gum_err" || gum_status=$?', + ); + }); }); describe("install.sh duplicate OpenClaw install detection", () => { @@ -2060,11 +2124,214 @@ describe("install.sh duplicate OpenClaw install detection", () => { expect(result.status).toBe(0); expect(result.stdout).not.toContain("Multiple OpenClaw global installs detected"); }); + + it("needs_stdin_isolation returns true when stdin is piped", () => { + const result = spawnSync( + "bash", + [ + "-c", + `source "${SCRIPT_PATH}" && needs_stdin_isolation && echo "ISOLATED" || echo "INTERACTIVE"`, + ], + { + encoding: "utf8", + stdio: ["pipe", "pipe", "pipe"], + env: { + ...process.env, + HOME: tmpdir(), + OPENCLAW_INSTALL_SH_NO_RUN: "1", + BASH_ENV: "", + ENV: "", + }, + input: "", + }, + ); + expect(result.stdout.trim()).toBe("ISOLATED"); + }); + + it("needs_stdin_isolation returns true when NO_PROMPT is set", () => { + const result = runInstallShell(` + set -euo pipefail + source "${SCRIPT_PATH}" + NO_PROMPT=1 + needs_stdin_isolation && echo "ISOLATED" || echo "INTERACTIVE" + `); + expect(result.status).toBe(0); + expect(result.stdout.trim()).toBe("ISOLATED"); + }); + + it("routes piped interactive subprocesses through the controlling TTY", () => { + const result = runInstallShell(` + set -euo pipefail + source "${SCRIPT_PATH}" + NO_PROMPT=0 + needs_stdin_isolation() { return 0; } + has_controlling_tty() { return 0; } + resolve_subprocess_stdin_path 1 + `); + expect(result.status).toBe(0); + expect(result.stdout.trim()).toBe("/dev/tty"); + }); + + it("keeps piped subprocesses nonblocking when prompt output is redirected", () => { + const result = runInstallShell(` + set -euo pipefail + source "${SCRIPT_PATH}" + NO_PROMPT=0 + needs_stdin_isolation() { return 0; } + has_controlling_tty() { return 0; } + resolve_subprocess_stdin_path 0 + `); + expect(result.status).toBe(0); + expect(result.stdout.trim()).toBe("/dev/null"); + }); + + it("captures visible prompt output before resolving subprocess stdin", () => { + const result = runInstallShell(` + set -euo pipefail + source "${SCRIPT_PATH}" + marker="$(mktemp)" + trap 'rm -f "$marker"' EXIT + has_visible_prompt_output() { return 0; } + resolve_subprocess_stdin_path() { + echo "visible=$1" > "$marker" + return 1 + } + run_with_safe_stdin true + cat "$marker" + `); + expect(result.status).toBe(0); + expect(result.stdout.trim()).toBe("visible=1"); + }); + + it("routes non-promptable subprocesses through /dev/null", () => { + const result = runInstallShell(` + set -euo pipefail + source "${SCRIPT_PATH}" + NO_PROMPT=1 + has_controlling_tty() { return 0; } + resolve_subprocess_stdin_path + `); + expect(result.status).toBe(0); + expect(result.stdout.trim()).toBe("/dev/null"); + }); + + it("run_quiet_step redirects stdin to /dev/null in piped context", () => { + const dir = mkdtempSync(join(tmpdir(), "openclaw-stdin-test-")); + const marker = join(dir, "stdin-state"); + try { + const result = spawnSync( + "bash", + [ + "-c", + `source "${SCRIPT_PATH}" && GUM="" && run_quiet_step "test-step" bash -c 'if read -t 1 line 2>/dev/null && [ -n "$line" ]; then echo "LEAKED:$line" > ${JSON.stringify(marker)}; else echo ISOLATED > ${JSON.stringify(marker)}; fi'`, + ], + { + encoding: "utf8", + stdio: ["pipe", "pipe", "pipe"], + env: { + ...process.env, + HOME: tmpdir(), + NO_PROMPT: "1", + OPENCLAW_INSTALL_SH_NO_RUN: "1", + BASH_ENV: "", + ENV: "", + }, + input: "SENTINEL_DATA_SHOULD_NOT_LEAK\n", + }, + ); + expect(result.status).toBe(0); + const stdinState = readFileSync(marker, "utf8").trim(); + expect(stdinState).toBe("ISOLATED"); + } finally { + rmSync(dir, { force: true, recursive: true }); + } + }); + + it("pipe data leaks to child when stdin is not isolated (counterproof)", () => { + // This test proves the fix is necessary: without /dev/null redirect, + // pipe data from the installer invocation reaches the child process. + // If this test ever fails, the isolation in run_quiet_step is no longer + // the only barrier protecting child processes from pipe consumption. + const dir = mkdtempSync(join(tmpdir(), "openclaw-stdin-leak-")); + const marker = join(dir, "stdin-state"); + try { + const result = spawnSync( + "bash", + [ + "-c", + // Bypass run_quiet_step: call the child directly with inherited stdin + `source "${SCRIPT_PATH}" && bash -c 'output=$(cat); if [ -n "$output" ]; then echo "LEAKED" > ${JSON.stringify(marker)}; else echo "EMPTY" > ${JSON.stringify(marker)}; fi'`, + ], + { + encoding: "utf8", + stdio: ["pipe", "pipe", "pipe"], + env: { + ...process.env, + HOME: tmpdir(), + OPENCLAW_INSTALL_SH_NO_RUN: "1", + BASH_ENV: "", + ENV: "", + }, + input: "SENTINEL_DATA_SHOULD_LEAK\n", + }, + ); + expect(result.status).toBe(0); + const stdinState = readFileSync(marker, "utf8").trim(); + // Without /dev/null redirect, cat reads the sentinel from the pipe. + expect(stdinState).toBe("LEAKED"); + } finally { + rmSync(dir, { force: true, recursive: true }); + } + }); + + it("run_quiet_step blocks cat from reading pipe data", () => { + // Stronger version of the isolation test: uses cat to consume all of + // stdin and verifies it reads nothing (empty output from /dev/null). + const dir = mkdtempSync(join(tmpdir(), "openclaw-stdin-cat-")); + const marker = join(dir, "stdin-state"); + try { + const result = spawnSync( + "bash", + [ + "-c", + `source "${SCRIPT_PATH}" && GUM="" && run_quiet_step "test-step" bash -c 'output=$(cat); if [ -n "$output" ]; then echo "LEAKED" > ${JSON.stringify(marker)}; else echo "ISOLATED" > ${JSON.stringify(marker)}; fi'`, + ], + { + encoding: "utf8", + stdio: ["pipe", "pipe", "pipe"], + env: { + ...process.env, + HOME: tmpdir(), + NO_PROMPT: "1", + OPENCLAW_INSTALL_SH_NO_RUN: "1", + BASH_ENV: "", + ENV: "", + }, + input: "SENTINEL_DATA_SHOULD_NOT_LEAK\n", + }, + ); + expect(result.status).toBe(0); + const stdinState = readFileSync(marker, "utf8").trim(); + expect(stdinState).toBe("ISOLATED"); + } finally { + rmSync(dir, { force: true, recursive: true }); + } + }); }); describe("install.sh doctor cancellation and dashboard guard", () => { const script = readFileSync(SCRIPT_PATH, "utf8"); + it("preserves dashboard stdin for direct interactive installs", () => { + expect(script).toContain('run_with_safe_stdin "$claw" dashboard || true'); + }); + + it("preserves plugin update stdin for direct interactive upgrades", () => { + expect(script).toContain( + 'OPENCLAW_UPDATE_IN_PROGRESS=1 run_with_safe_stdin "$claw" plugins update --all || true', + ); + }); + it("guards every run_doctor caller against failure", () => { // A failed or cancelled doctor must not launch the dashboard. expect(script).toContain("if run_doctor; then");