From 17708770a12e2894daf7d7442b0963645cac8517 Mon Sep 17 00:00:00 2001 From: redshift <213178690+1ftredsh@users.noreply.github.com> Date: Thu, 10 Sep 2026 20:22:30 +0200 Subject: [PATCH] fix(install): harden install.sh after review Review follow-ups on the installer: - --print-asset no longer requires curl/wget or HOME: the HTTP client is resolved lazily and the install directory only after the early exit, so printing the asset name works on a machine with neither, which is the point of that flag. - Trap INT/TERM with `exit 1` instead of `cleanup` alone. A trapped signal previously resumed execution after the handler, which (with the temp dir already removed by the handler) surfaced as a misleading follow-on download error. The installer now reports the interrupt and exits. - Bound every HTTP transfer: 10s connect timeout, 60s for the API, 15min for the archive, with retries. A stalled connection previously hung the installer forever; the standalone updater already caps this at 30s. - Prefer the native arm64 build on Apple Silicon when a Rosetta shell reports x86_64, instead of installing the x64 build under emulation. - Validate the staged binary against the requested version before swapping it in, as installStandaloneRelease already does, and refuse the install on a mismatch instead of replacing a working install and warning afterwards. Skipped when --platform/--arch are overridden, where the binary may not be able to run on this machine at all. - Cap SHA256SUMS at 1MB, matching MAX_CHECKSUM_BYTES in the updater. - Use tr '[:upper:]' '[:lower:]' (shellcheck SC2018/SC2019) and document the --install-dir alias in --help. Tests cover each fix, and the four new failure-path tests fail against the previous script. --- README.md | 3 +- install.sh | 95 ++++++++++++++++------ tests/install-script.test.ts | 153 ++++++++++++++++++++++++++++++++++- 3 files changed, 222 insertions(+), 29 deletions(-) diff --git a/README.md b/README.md index 4d8e92a..9276eff 100644 --- a/README.md +++ b/README.md @@ -44,7 +44,8 @@ curl -fsSL https://github.com/Routstr/routstrd/releases/latest/download/install. ``` The installer downloads the release archive, verifies it against the release -`SHA256SUMS`, and only replaces `routstrd` after the checksum matches. +`SHA256SUMS`, and only replaces an existing `routstrd` once the checksum matches +and the extracted binary reports the expected version.
Manual install diff --git a/install.sh b/install.sh index 3612cac..cdf7df9 100755 --- a/install.sh +++ b/install.sh @@ -21,7 +21,13 @@ PLATFORM="${ROUTSTRD_PLATFORM:-}" ARCH="${ROUTSTRD_ARCH:-}" INSTALL_DIR="${ROUTSTRD_INSTALL_DIR:-}" PRINT_ASSET=0 +PLATFORM_DETECTED=0 +ARCH_DETECTED=0 MAX_ARCHIVE_BYTES=262144000 +MAX_CHECKSUMS_BYTES=1048576 +CONNECT_TIMEOUT_SECS=10 +API_TIMEOUT_SECS=60 +DOWNLOAD_TIMEOUT_SECS=900 say() { printf '%s\n' "$*" >&2; } die() { printf 'error: %s\n' "$*" >&2; exit 1; } @@ -35,6 +41,7 @@ Usage: sh install.sh [options] Options: --version Install a specific version (default: latest) --dir Install directory (default: $HOME/.local/bin) + --install-dir Alias for --dir --platform Override platform detection (linux|darwin) --arch Override architecture detection (x64|arm64) --repo GitHub repository (default: Routstr/routstrd) @@ -70,25 +77,29 @@ else DOWNLOAD_BASE_URL="https://github.com/${REPO}/releases/download" fi -if [ -z "$INSTALL_DIR" ]; then - [ -n "${HOME:-}" ] || die "HOME is not set; pass --dir or set ROUTSTRD_INSTALL_DIR." - INSTALL_DIR="$HOME/.local/bin" -fi - if [ -z "$PLATFORM" ]; then case "$(uname -s)" in Linux) PLATFORM=linux ;; Darwin) PLATFORM=darwin ;; *) die "unsupported operating system '$(uname -s)'; releases cover Linux and macOS." ;; esac + PLATFORM_DETECTED=1 fi if [ -z "$ARCH" ]; then case "$(uname -m)" in - x86_64|amd64) ARCH=x64 ;; + x86_64|amd64) + ARCH=x64 + # Under Rosetta, uname reports x86_64 on arm64 Macs; prefer the native build. + if [ "$PLATFORM" = darwin ] \ + && [ "$(sysctl -n hw.optional.arm64 2>/dev/null)" = "1" ]; then + ARCH=arm64 + fi + ;; arm64|aarch64) ARCH=arm64 ;; *) die "unsupported architecture '$(uname -m)'; releases cover x64 and arm64." ;; esac + ARCH_DETECTED=1 fi case "$PLATFORM" in @@ -101,29 +112,43 @@ case "$ARCH" in *) die "unsupported architecture '$ARCH'; expected x64 or arm64." ;; esac -if command -v curl >/dev/null 2>&1; then - HTTP_CLIENT=curl -elif command -v wget >/dev/null 2>&1; then - HTTP_CLIENT=wget -else - die "curl or wget is required to download routstrd." -fi +# Resolved lazily by require_http_client, so --print-asset does not need either. +HTTP_CLIENT="" +require_http_client() { + if [ -n "$HTTP_CLIENT" ]; then + return 0 + fi + if command -v curl >/dev/null 2>&1; then + HTTP_CLIENT=curl + elif command -v wget >/dev/null 2>&1; then + HTTP_CLIENT=wget + else + die "curl or wget is required to download routstrd." + fi +} fetch_stdout() { + require_http_client if [ "$HTTP_CLIENT" = curl ]; then - curl -fsSL -H "Accept: application/vnd.github+json" \ + curl -fsSL --connect-timeout "$CONNECT_TIMEOUT_SECS" \ + --max-time "$API_TIMEOUT_SECS" --retry 3 \ + -H "Accept: application/vnd.github+json" \ ${GITHUB_TOKEN:+-H "Authorization: Bearer ${GITHUB_TOKEN}"} "$1" else - wget -qO- --header="Accept: application/vnd.github+json" \ + wget -qO- --timeout="$API_TIMEOUT_SECS" --tries=3 \ + --header="Accept: application/vnd.github+json" \ ${GITHUB_TOKEN:+--header="Authorization: Bearer ${GITHUB_TOKEN}"} "$1" fi } fetch_file() { + require_http_client if [ "$HTTP_CLIENT" = curl ]; then - curl -fsSL -o "$2" "$1" + curl -fsSL --connect-timeout "$CONNECT_TIMEOUT_SECS" \ + --max-time "$DOWNLOAD_TIMEOUT_SECS" --retry 3 \ + -o "$2" "$1" else - wget -qO "$2" "$1" + wget -qO "$2" --timeout="$DOWNLOAD_TIMEOUT_SECS" --tries=3 "$1" fi } @@ -155,6 +180,11 @@ if [ "$PRINT_ASSET" = 1 ]; then exit 0 fi +if [ -z "$INSTALL_DIR" ]; then + [ -n "${HOME:-}" ] || die "HOME is not set; pass --dir or set ROUTSTRD_INSTALL_DIR." + INSTALL_DIR="$HOME/.local/bin" +fi + if command -v sha256sum >/dev/null 2>&1; then SHA256_CMD="sha256sum" elif command -v shasum >/dev/null 2>&1; then @@ -167,10 +197,10 @@ fi hash_file() { if [ "$SHA256_CMD" = openssl_sha256 ]; then - openssl dgst -sha256 -r "$1" | awk '{print $1}' | tr 'A-Z' 'a-z' + openssl dgst -sha256 -r "$1" | awk '{print $1}' | tr '[:upper:]' '[:lower:]' else # shellcheck disable=SC2086 - $SHA256_CMD "$1" | awk '{print $1}' | tr 'A-Z' 'a-z' + $SHA256_CMD "$1" | awk '{print $1}' | tr '[:upper:]' '[:lower:]' fi } @@ -181,7 +211,8 @@ cleanup() { if [ -n "$STAGED" ]; then rm -f "$STAGED" 2>/dev/null || true; fi if [ -n "$WORKDIR" ]; then rm -rf "$WORKDIR" 2>/dev/null || true; fi } -trap cleanup EXIT INT TERM +trap cleanup EXIT +trap 'say "Interrupted."; exit 1' INT TERM RELEASE_BASE_URL="${DOWNLOAD_BASE_URL}/v${VERSION}" ARCHIVE="${WORKDIR}/${ASSET}" @@ -200,10 +231,14 @@ archive_bytes="$(wc -c < "$ARCHIVE" | tr -d '[:space:]')" [ "$archive_bytes" -le "$MAX_ARCHIVE_BYTES" ] \ || die "${ASSET} is unexpectedly large (${archive_bytes} bytes)." +checksums_bytes="$(wc -c < "$CHECKSUMS" | tr -d '[:space:]')" +[ "$checksums_bytes" -le "$MAX_CHECKSUMS_BYTES" ] \ + || die "SHA256SUMS is unexpectedly large (${checksums_bytes} bytes)." + expected="$(grep -F "$ASSET" "$CHECKSUMS" 2>/dev/null \ | awk -v name="$ASSET" '$2 == name || $2 == "*" name { print $1 }' \ | head -n 1 \ - | tr 'A-Z' 'a-z')" + | tr '[:upper:]' '[:lower:]')" [ -n "$expected" ] || die "SHA256SUMS does not contain ${ASSET}." actual="$(hash_file "$ARCHIVE")" @@ -220,15 +255,23 @@ STAGED="${INSTALL_DIR}/.routstrd.tmp.$$" cp "${WORKDIR}/routstrd" "$STAGED" || die "could not write to ${INSTALL_DIR}." chmod 755 "$STAGED" + +# Validate the staged binary before replacing the target, mirroring +# installStandaloneRelease: refuse a release whose binary reports the wrong +# version rather than replacing a working install and warning about it. +# Skipped for cross-installs (--platform/--arch overrides), where the binary may +# not be able to run on this machine at all. +if [ "$PLATFORM_DETECTED" = 1 ] && [ "$ARCH_DETECTED" = 1 ]; then + staged_version="$("$STAGED" --version 2>/dev/null || true)" + if [ "$staged_version" != "$VERSION" ] && [ "$staged_version" != "v$VERSION" ]; then + die "staged routstrd reported version '${staged_version:-unknown}' instead of '${VERSION}'." + fi +fi + # Rename over the target so a running daemon keeps its old inode. mv -f "$STAGED" "$TARGET" || die "could not install to ${TARGET}." STAGED="" -installed_version="$("$TARGET" --version 2>/dev/null || true)" -if [ "$installed_version" != "$VERSION" ]; then - say "warning: ${TARGET} reported version '${installed_version:-unknown}' instead of '${VERSION}'." -fi - say "Installed routstrd v${VERSION} to ${TARGET}" case ":${PATH:-}:" in diff --git a/tests/install-script.test.ts b/tests/install-script.test.ts index 95cad40..44dc187 100644 --- a/tests/install-script.test.ts +++ b/tests/install-script.test.ts @@ -1,6 +1,6 @@ import { afterEach, describe, expect, test } from "bun:test"; import { createHash } from "crypto"; -import { mkdtempSync, readFileSync, rmSync, statSync, writeFileSync } from "fs"; +import { mkdirSync, mkdtempSync, readdirSync, readFileSync, rmSync, statSync, writeFileSync } from "fs"; import { tmpdir } from "os"; import { join } from "path"; import { releaseArchiveName } from "../src/utils/standalone-update"; @@ -51,6 +51,8 @@ type FakeReleaseOptions = { archive?: Uint8Array; checksums?: string; omitAsset?: boolean; + /** Delays the archive response so a test can signal while the download is in flight. */ + assetDelayMs?: number; }; /** @@ -62,7 +64,7 @@ function serveFakeRelease(options: FakeReleaseOptions = {}): string { const asset = options.asset ?? releaseArchiveName(version, "linux", "x64"); const server = Bun.serve({ port: 0, - fetch(request) { + async fetch(request) { const { pathname } = new URL(request.url); if (pathname === `/repos/${REPO}/releases/latest`) { return Response.json({ @@ -75,6 +77,7 @@ function serveFakeRelease(options: FakeReleaseOptions = {}): string { } if (pathname === `/dl/v${version}/${asset}`) { if (options.omitAsset) return new Response("not found", { status: 404 }); + if (options.assetDelayMs) await Bun.sleep(options.assetDelayMs); return new Response(options.archive ?? new Uint8Array()); } return new Response("not found", { status: 404 }); @@ -308,4 +311,150 @@ describe("install.sh", () => { expect(statSync(installed).isFile()).toBe(true); expect(statSync(installed).mode & 0o111).not.toBe(0); }); + + test("prints the asset without curl, wget or HOME", () => { + // --print-asset performs no install and, with --version, no network access, + // so it must not require an HTTP client or an install directory. /bin/sh is + // addressed absolutely because PATH is deliberately stripped. + const result = Bun.spawnSync( + [ + "/bin/sh", + INSTALL_SCRIPT, + "--print-asset", + "--version", + VERSION, + "--platform", + "linux", + "--arch", + "x64", + ], + { env: { PATH: "/nonexistent" }, stdout: "pipe", stderr: "pipe" }, + ); + + expect(result.exitCode).toBe(0); + expect(result.stdout.toString().trim()).toBe(releaseArchiveName(VERSION, "linux", "x64")); + }); + + test("refuses a release whose binary reports the wrong version", async () => { + const dir = tempDir("routstrd-install-wrong-version-"); + const installDir = join(dir, "bin"); + mkdirSync(installDir, { recursive: true }); + writeFileSync(join(installDir, "routstrd"), "#!/bin/sh\necho old\n"); + const asset = releaseArchiveName(VERSION, process.platform, process.arch); + const archive = buildArchive(dir, "0.0.1"); + const origin = serveFakeRelease({ + asset, + archive, + checksums: `${sha256Hex(archive)} ${asset}\n`, + }); + + const result = await runInstallerAsync([ + "--dir", + installDir, + "--api-base-url", + origin, + "--download-base-url", + `${origin}/dl`, + ]); + + expect(result.exitCode).not.toBe(0); + expect(result.stderr).toContain("reported version"); + // The previous install must survive a rejected update. + expect(readFileSync(join(installDir, "routstrd"), "utf8")).toContain("old"); + }); + + test("accepts a staged binary that reports a v-prefixed version", async () => { + const dir = tempDir("routstrd-install-vprefixed-binary-"); + const installDir = join(dir, "bin"); + const asset = releaseArchiveName(VERSION, process.platform, process.arch); + const archive = buildArchive(dir, `v${VERSION}`); + const origin = serveFakeRelease({ + asset, + archive, + checksums: `${sha256Hex(archive)} ${asset}\n`, + }); + + const result = await runInstallerAsync([ + "--dir", + installDir, + "--api-base-url", + origin, + "--download-base-url", + `${origin}/dl`, + ]); + + expect(result.exitCode).toBe(0); + expect(statSync(join(installDir, "routstrd")).isFile()).toBe(true); + }); + + test("rejects an oversized SHA256SUMS", async () => { + const dir = tempDir("routstrd-install-big-sums-"); + const asset = releaseArchiveName(VERSION, process.platform, process.arch); + const archive = buildArchive(dir, VERSION); + const origin = serveFakeRelease({ + asset, + archive, + checksums: `${sha256Hex(archive)} ${asset}\n${"a".repeat(2 * 1024 * 1024)}\n`, + }); + + const result = await runInstallerAsync([ + "--dir", + join(dir, "bin"), + "--api-base-url", + origin, + "--download-base-url", + `${origin}/dl`, + ]); + + expect(result.exitCode).not.toBe(0); + expect(result.stderr).toContain("SHA256SUMS is unexpectedly large"); + }); + + test( + "exits without installing when interrupted mid-download", + async () => { + const dir = tempDir("routstrd-install-interrupt-"); + const installDir = join(dir, "bin"); + const tmpParent = join(dir, "tmp"); + mkdirSync(tmpParent, { recursive: true }); + const asset = releaseArchiveName(VERSION, process.platform, process.arch); + const archive = buildArchive(dir, VERSION); + const origin = serveFakeRelease({ + asset, + archive, + checksums: `${sha256Hex(archive)} ${asset}\n`, + assetDelayMs: 2000, + }); + + const proc = Bun.spawn( + [ + "sh", + INSTALL_SCRIPT, + "--dir", + installDir, + "--api-base-url", + origin, + "--download-base-url", + `${origin}/dl`, + ], + { env: { ...process.env, TMPDIR: tmpParent }, stdout: "pipe", stderr: "pipe" }, + ); + + // Signal while the archive download is still in flight. The installer must + // report the interrupt, exit non-zero and clean up after itself, rather + // than resuming into a misleading follow-on failure (or an install). + await Bun.sleep(300); + proc.kill(); + + const [exitCode, stderr] = await Promise.all([ + Promise.race([proc.exited, Bun.sleep(8000).then(() => -1)]), + new Response(proc.stderr).text(), + ]); + expect(exitCode).toBe(1); + expect(stderr).toContain("Interrupted."); + expect(readdirSync(tmpParent)).toEqual([]); + expect(() => statSync(join(installDir, "routstrd"))).toThrow(); + }, + 15000, + ); });