Fix installer set -e landmines, validate every prompt, verify registration#14
Conversation
…ation install.sh had two errexit bugs that killed the script silently: `( set +e; setup_self_node ); rc=$?` aborts the parent on subshell failure before the final summary (and the one-time admin password) prints, and every `VAR="$(grep '^KEY=' .env | cut ...)"` dies under pipefail when a legacy .env lacks the key. Self-node rc is now captured via `|| rc=$?` and all env reads go through an errexit-safe env_get helper. Also across both installers: - setup_firewall opens the configured PANEL_HTTP/HTTPS ports instead of hardcoded 80/443, and allows the ports sshd actually listens on so `ufw enable` can't lock out a session on a custom SSH port - preflight_ports also checks the web ports, but skips ports held by docker-proxy so re-runs over a live stack pass their own preflight - every prompt is validated (domain stripped of scheme/path, email, username, ports 1-65535, IFNAMSIZ iface names, real CIDR octets, numeric node capacity - previously interpolated raw into JSON) - PANEL_ADDR strips a pasted https:// scheme the agent would double up - the wrong-port HTML probe buffers curl output so pipefail can't swallow a positive match via grep -q's early-exit SIGPIPE - verify() waits for the agent's "registered"/"fatal" log markers instead of calling a crash-looping container a success after 3s - docker is enabled on boot even when preinstalled; apt runs noninteractive - node-entrypoint syncs ListenPort/Address on an existing wg conf so a re-install with a new port actually applies it (keys preserved) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
This pull request significantly hardens the installation and configuration scripts by adding robust input validation for domains, subnets, usernames, and ports. It also improves firewall setup to prevent SSH lockouts, ensures Docker starts on boot, and updates the node registration verification to monitor actual container log markers rather than just container status. The review feedback suggests optimizing the log-monitoring loops by using --tail 100 to limit log retrieval size, and adding fallback defaults in preflight_ports to handle legacy .env files where port variables might be missing.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| local logs deadline=$((SECONDS + 45)) | ||
| while (( SECONDS < deadline )); do | ||
| sleep 3 | ||
| logs="$(cd "$NODE_DIR" && docker compose logs --no-color 2>/dev/null || true)" |
There was a problem hiding this comment.
Fetching the entire log history of the container on every iteration of the loop can become slow and resource-intensive, especially if the container has been running for a while or is crash-looping rapidly.
Limiting the log retrieval to the most recent entries (e.g., using --tail 100) improves efficiency and reduces memory usage.
| logs="$(cd "$NODE_DIR" && docker compose logs --no-color 2>/dev/null || true)" | |
| logs="$(cd "$NODE_DIR" && docker compose logs --no-color --tail 100 2>/dev/null || true)" |
| log "Waiting for the node to register with the control plane..." | ||
| while (( SECONDS < deadline )); do | ||
| sleep 3 | ||
| logs="$(cd "$NODE_DIR" && docker compose logs --no-color 2>/dev/null || true)" |
There was a problem hiding this comment.
Fetching the entire log history of the container on every iteration of the loop can become slow and resource-intensive, especially if the container has been running for a while or is crash-looping rapidly.
Limiting the log retrieval to the most recent entries (e.g., using --tail 100) improves efficiency and reduces memory usage.
| logs="$(cd "$NODE_DIR" && docker compose logs --no-color 2>/dev/null || true)" | |
| logs="$(cd "$NODE_DIR" && docker compose logs --no-color --tail 100 2>/dev/null || true)" |
| local port pid | ||
| port="$(grep '^NODE_AGENT_PORT=' "$ENV_FILE" | cut -d= -f2)" | ||
| [[ -n "$port" ]] || return 0 | ||
| if command -v ss >/dev/null 2>&1 && ss -tlnH "sport = :${port}" 2>/dev/null | grep -q .; then | ||
| command -v ss >/dev/null 2>&1 || return 0 | ||
| for port in "$(env_get NODE_AGENT_PORT)" "$(env_get PANEL_HTTP_PORT)" "$(env_get PANEL_HTTPS_PORT)"; do |
There was a problem hiding this comment.
If NODE_AGENT_PORT, PANEL_HTTP_PORT, or PANEL_HTTPS_PORT are not explicitly defined in a legacy .env file, env_get will return an empty string, causing preflight_ports to skip checking those ports.
Providing fallback defaults (matching those used in setup_firewall) ensures that the preflight check always validates the ports the stack will attempt to bind to.
| local port pid | |
| port="$(grep '^NODE_AGENT_PORT=' "$ENV_FILE" | cut -d= -f2)" | |
| [[ -n "$port" ]] || return 0 | |
| if command -v ss >/dev/null 2>&1 && ss -tlnH "sport = :${port}" 2>/dev/null | grep -q .; then | |
| command -v ss >/dev/null 2>&1 || return 0 | |
| for port in "$(env_get NODE_AGENT_PORT)" "$(env_get PANEL_HTTP_PORT)" "$(env_get PANEL_HTTPS_PORT)"; do | |
| local port pid agent_port http_port https_port | |
| command -v ss >/dev/null 2>&1 || return 0 | |
| agent_port="$(env_get NODE_AGENT_PORT)" | |
| http_port="$(env_get PANEL_HTTP_PORT)" | |
| https_port="$(env_get PANEL_HTTPS_PORT)" | |
| for port in "${agent_port:-48443}" "${http_port:-80}" "${https_port:-443}"; do |
Summary
Audit of
deploy/install.sh,deploy/install-node.sh, anddeploy/node-entrypoint.shfound two silent-deathset -ebugs, a firewall misconfiguration, and a set of unvalidated prompts whose bad values only surfaced much later with unrelated-looking errors. All fixed.Critical (install.sh) — both reproduced before fixing
( set +e; setup_self_node ); rc=$?still trips the parent's errexit on the subshell's non-zero exit, so the held-until-the-end summary — including the unrecoverable one-time admin password — never displayed. Now captured with( ... ) || rc=$?..envmissing a newer key killed the script with no output. EveryVAR="$(grep '^KEY=' .env | cut ...)"dies underpipefailwhen the key is absent (e.g.NODE_AGENT_PORTon an env predating it — the same legacy-env scenario thePANEL_HTTP_PORTbackfill exists for). All env reads now use an errexit-safeenv_gethelper with fallbacks.Misconfigurations
setup_firewallhardcoded80/tcp 443/tcp— customPANEL_HTTP/HTTPS_PORTinstalls got the wrong ports opened. Now reads the configured ports.ufw allow OpenSSH || truefails silently without the app profile;ufw enablethen default-denies a custom-port SSH session. Both installers now also allow the ports sshd actually listens on.preflight_portsaborted re-runs over a live stack (docker-proxy holdsNODE_AGENT_PORT). Now skips docker-proxy-held ports, and also checks the web ports — a taken 443 was the documented live incident.Validation & verification
https://itself, so a pasted URL previously producedhttps://https://…), email, username, ports 1–65535, IFNAMSIZ interface names, real CIDR octet/prefix ranges, numeric node capacity (previously interpolated raw into the bootstrap JSON body).grep -q's early-exit SIGPIPE can't swallow a positive match underpipefail.verify()(both installers) waits for the agent's"registered"/"fatal"JSON log markers instead of declaring a crash-looping container a success after 3 seconds, and prints the actual failure.node-entrypoint.shsyncsListenPort/Addresson an existing persisted wg conf, so a re-install with a changed port no longer publishes one port while the interface listens on another (keys preserved).Test plan
bash -non all three scriptsset -ebugs reproduced in isolation; fixes verified to surviveenv_gettested against present / missing /=-containing keysset -euo pipefail(CIDR incl. leading-zero octal trap, ports, domain/email/username sanitization, iface regex, sshd-port extraction, conf-sync sed)🤖 Generated with Claude Code