fix(install): harden stdin consumers to prevent pipe corruption in curl | bash#87799
Conversation
|
Codex review: needs maintainer review before merge. Reviewed June 29, 2026, 2:37 PM ET / 18:37 UTC. Summary PR surface: Tests +191, Other +34. Total +225 across 2 files. Reproducibility: yes. source-reproducible: docs pipe the installer through stdin, current main lets gum/fallback/npm subprocesses inherit stdin, and gum v0.17.0 forwards Review metrics: none identified. Root-cause cluster Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Risk before merge
Maintainer options:
Next step before merge
Security Review detailsBest possible solution: Land the focused stdin-consumer hardening after maintainer acceptance of the installer stdin contract and proof coverage, then close #90008 as implemented. Do we have a high-confidence way to reproduce the issue? Yes, source-reproducible: docs pipe the installer through stdin, current main lets gum/fallback/npm subprocesses inherit stdin, and gum v0.17.0 forwards Is this the best way to solve the issue? Yes, this is the best observed fix direction because it follows the prior maintainer guidance to harden actual stdin consumers instead of adding a broad top-level pipe guard. The remaining question is maintainer acceptance of the compatibility and proof coverage. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 82dfd8910743. Label changesLabel justifications:
Evidence reviewedPR surface: Tests +191, Other +34. Total +225 across 2 files. View PR surface stats
What I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
Review history (1 earlier review cycle)
|
12dfa7a to
534fa4d
Compare
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
fa10a67 to
38030bf
Compare
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
|
@clawsweeper re-review |
|
🦞👀 Command router queued. I will update this comment with the next step. Re-review progress:
|
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
|
@clawsweeper re-review |
|
@clawsweeper review |
…rl | 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 openclaw#73814). Unlike the global pipe guard in openclaw#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 openclaw#73814 Signed-off-by: Sebastien Tardif <[email protected]>
Signed-off-by: Sebastien Tardif <[email protected]>
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.
…ands 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 <[email protected]>
Signed-off-by: Sebastien Tardif <[email protected]>
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.
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.
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.
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
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.
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.
Signed-off-by: Sebastien Tardif <[email protected]>
Fixes #90008
Fixes #73814
Problem
When the OpenClaw installer is piped via
curl | bash, child processes spawnedunder
gum spincan inherit the script stream as stdin. Gum v0.17.0 sets thewrapped command's stdin to
os.Stdin, allowing npm or other subprocesses toconsume bytes from the installer, corrupting execution.
Fix
All stdin redirect paths are now conditional on
needs_stdin_isolation.This includes the primary
gum spinpath and all fallback paths (gum raw-modefallback, non-gum fallback,
run_quiet_stepshell-function path):needs_stdin_isolationchecks stdin directly (! -t 0) andNO_PROMPT,without checking stdout (
-t 1). This ensures:curl | bash): stdin is not a terminal, so allsubprocess stdin is redirected from
/dev/null, preventing pipe consumptionbash install.sh): stdin IS a terminal,so subprocess stdin is preserved for interactive prompts (e.g. Homebrew)
Real behavior proof
Behavior addressed: (1) Piped
curl | bashinstalls leak the script stream to gum-wrapped subprocesses because gum v0.17 passesos.Stdinto the wrapped command. (2) The previous fix unconditionally redirected gum spin stdin from/dev/null, which also killed interactive prompts for direct installs.Real environment tested: macOS 15.5, Node v26.3.0, bash 5.2.37 (Homebrew), OpenClaw installed. Patched install.sh extracted from branch, tested with real
cat install.sh | bashpipe pattern and directbash install.shinteractive path.Exact steps or command run after this patch:
Step 1. Ran the patched install script through a real
cat | bashpipe (the exactcurl | bashpattern):cat /tmp/install-patched.sh | bash -s -- --dry-runStep 2. Ran the patched install script directly (interactive path):
Step 3. Demonstrated the before/after stdin leak behavior.
Step 4. Ran a full piped install (not dry-run) with
NO_PROMPT=1:Evidence after fix: terminal output from the patched installer:
Piped install via
cat | bash(the exact curl-pipe pattern):Direct interactive install (stdin preserved for prompts):
Both paths complete without errors. The piped path activates
needs_stdin_isolation(stdin is not a TTY), redirecting gum subprocess stdin from/dev/null. The direct path keeps stdin connected for interactive prompts.Full piped install (non-dry-run) completed all 3 phases without pipe corruption:
Before fix: child process steals pipe data:
After fix: child stdin redirected from /dev/null, pipe data blocked:
Observed result after fix: The patched install.sh ran through both the
cat install.sh | bashpiped pattern and the directbash install.shinteractive pattern. Terminal output confirms: (1) piped installs complete all three phases without gum subprocesses stealing stdin bytes, (2) direct installs preserve stdin for interactive prompts, (3)needs_stdin_isolationcorrectly detects piped vs TTY stdin and gates all five redirect sites accordingly.What was not tested: Interactive Homebrew prompt during a direct install where Homebrew triggers a yes/no prompt (requires Homebrew to actually need confirmation). The
needs_stdin_isolationfunction returns false when stdin is a terminal, which preserves stdin for such prompts.