fix: resolve install binary verification, uninstall, and version prefix bugs#535
fix: resolve install binary verification, uninstall, and version prefix bugs#535khaliqgant merged 3 commits intomainfrom
Conversation
…on prefix issues - Strip macOS Gatekeeper quarantine attribute before binary verification to prevent "Killed: 9" errors on downloaded binaries - Add binary and npm package removal to uninstall command (previously only cleaned up data/config files, leaving binaries in place) - Fix SDK version tag parsing to strip "openclaw-" prefix, preventing double-prefix download URLs like "vopenclaw-v3.1.18" (404) - Warn when an older npm-installed binary shadows newly installed version Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
| timeout: 10_000, | ||
| stdio: ['pipe', 'pipe', 'pipe'], | ||
| }); | ||
| } catch { |
Check warning
Code scanning / CodeQL
Indirect uncontrolled command line Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 2 months ago
In general, the safest fix is to avoid exec/execSync with a single shell command string when any part of that string comes from untrusted or tainted data. Instead, use execFile/execFileSync (or spawn/spawnSync) and pass arguments as an array of strings; this avoids shell parsing, redirection, and environment-variable expansion. For cases where you truly need shell features like redirection (2>/dev/null) or || true, wrap that in a small, fixed shell snippet and keep all untrusted data passed as separate arguments, or emulate the behavior in JavaScript (e.g., ignore errors instead of || true, discard stderr instead of 2>/dev/null).
Here, we only need to (a) run xattr -d com.apple.quarantine <targetPath> and ignore any errors, and (b) run codesign --force --sign - <targetPath> and ignore errors. Both can be implemented with execFileSync without any shell metacharacters, and the “ignore failure” behavior can be preserved by wrapping each call in a try/catch as already done. So the single best fix is:
- Add
execFileSyncto the existing import fromnode:child_process. - Replace:
execSync(\xattr -d com.apple.quarantine "${targetPath}" 2>/dev/null || true`, ...)withexecFileSync('xattr', ['-d', 'com.apple.quarantine', targetPath], ...). The previous2>/dev/null || true` is unnecessary because:stdio: ['pipe','pipe','pipe']already prevents stderr from cluttering the console.- The
try/catcharound the call already makes errors non-fatal.
- Replace:
execSync(\codesign --force --sign - "${targetPath}"`, ...)withexecFileSync('codesign', ['--force', '--sign', '-', targetPath], ...)`.
This preserves all semantics (timeout, stdio behavior, non-fatal nature of failures), removes any use of an interpolated shell command string, and thus addresses all variants of the alert at this location.
| @@ -1,5 +1,5 @@ | ||
| import { once } from 'node:events'; | ||
| import { execSync, spawn, type ChildProcessWithoutNullStreams } from 'node:child_process'; | ||
| import { execFileSync, execSync, spawn, type ChildProcessWithoutNullStreams } from 'node:child_process'; | ||
| import { createInterface, type Interface as ReadlineInterface } from 'node:readline'; | ||
| import fs from 'node:fs'; | ||
| import os from 'node:os'; | ||
| @@ -793,7 +793,7 @@ | ||
| // macOS: strip quarantine attribute and re-sign to avoid Gatekeeper issues | ||
| if (process.platform === 'darwin') { | ||
| try { | ||
| execSync(`xattr -d com.apple.quarantine "${targetPath}" 2>/dev/null || true`, { | ||
| execFileSync('xattr', ['-d', 'com.apple.quarantine', targetPath], { | ||
| timeout: 10_000, | ||
| stdio: ['pipe', 'pipe', 'pipe'], | ||
| }); | ||
| @@ -801,7 +801,7 @@ | ||
| // Non-fatal | ||
| } | ||
| try { | ||
| execSync(`codesign --force --sign - "${targetPath}"`, { | ||
| execFileSync('codesign', ['--force', '--sign', '-', targetPath], { | ||
| timeout: 10_000, | ||
| stdio: ['pipe', 'pipe', 'pipe'], | ||
| }); |
The try/catch already handles errors, and the shell redirection is fragile if execCommand ever changes from exec to execFile. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The ~/.agent-relay directory is the global data dir (GLOBAL_BASE_DIR) which stores telemetry preferences, dashboard files, and legacy project data. Only the bin/ subdirectory contains installer-managed binaries. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ix bugs (#535) * fix: resolve install script binary verification, uninstall, and version prefix issues - Strip macOS Gatekeeper quarantine attribute before binary verification to prevent "Killed: 9" errors on downloaded binaries - Add binary and npm package removal to uninstall command (previously only cleaned up data/config files, leaving binaries in place) - Fix SDK version tag parsing to strip "openclaw-" prefix, preventing double-prefix download URLs like "vopenclaw-v3.1.18" (404) - Warn when an older npm-installed binary shadows newly installed version Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: remove shell-dependent 2>/dev/null from npm uninstall execCommand The try/catch already handles errors, and the shell redirection is fragile if execCommand ever changes from exec to execFile. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: only remove ~/.agent-relay/bin/ not entire ~/.agent-relay directory The ~/.agent-relay directory is the global data dir (GLOBAL_BASE_DIR) which stores telemetry preferences, dashboard files, and legacy project data. Only the bin/ subdirectory contains installer-managed binaries. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
Summary
com.apple.quarantinexattr from all downloaded binaries before verification. Addedstrip_quarantine()helper used at all 7 download points ininstall.sh, plusxattr -din SDK'sinstallBrokerBinary().agent-relay uninstallnow removes standalone binaries from~/.local/bin/, the~/.agent-relay/directory, and runsnpm uninstall -gfor all three packages.vopenclaw-v3.1.18404: SDK'sgetLatestVersionSync()now strips bothopenclaw-andvprefixes from GitHub release tags before constructing download URLs.verify_installation()now detects when an older npm-installed binary shadows the newly installed standalone binary and advises the user.Test plan
curl -fsSL .../install.sh | bashon macOS — broker binary should pass verification (no "Killed: 9")agent-relay uninstall— verifywhich agent-relayreturns nothing afterwardvopenclaw-prefix)🤖 Generated with Claude Code