diff --git a/scripts/download_pixlet.sh b/scripts/download_pixlet.sh index 53a38d22..80caeb74 100755 --- a/scripts/download_pixlet.sh +++ b/scripts/download_pixlet.sh @@ -24,9 +24,29 @@ echo "========================================" # Auto-detect latest version if needed if [ "$PIXLET_VERSION" = "latest" ]; then echo "Detecting latest version..." - PIXLET_VERSION=$(curl -s "https://api.github.com/repos/${REPO}/releases/latest" | grep '"tag_name"' | sed -E 's/.*"([^"]+)".*/\1/') - if [ -z "$PIXLET_VERSION" ]; then - echo "Failed to detect latest version, using fallback" + # When this response arrives on a single line -- as it did on the device + # where Starlark apps were failing -- `grep '"tag_name"'` matches the whole + # document and a greedy `sed 's/.*"([^"]+)".*/\1/'` captures the LAST + # quoted token in it rather than the tag. That resolved to + # "mentions_count", which built a download URL for a release that does not + # exist. (The API is pretty-printed by default, which is why the old + # command looks correct when you try it by hand -- but the formatting is + # not something to depend on.) Match the field itself and take the value + # after it, which is right for either shape. + PIXLET_VERSION=$(curl -fsSL "https://api.github.com/repos/${REPO}/releases/latest" \ + | grep -o '"tag_name"[[:space:]]*:[[:space:]]*"[^"]*"' \ + | head -n1 \ + | sed -E 's/.*:[[:space:]]*"([^"]*)".*/\1/') + + # A wrong-but-non-empty value is what made the old bug silent, so check the + # shape rather than just that something came back. Anchored at both ends: a + # partial match would accept "v0.53garbage" or "0.53" and build a URL for a + # release that cannot exist, which is the failure this check is here to + # stop. Every tronbyt/pixlet release to date is vX.Y.Z; the optional suffix + # leaves room for a future -rc.1 or +build tag. + if ! printf '%s' "$PIXLET_VERSION" \ + | grep -qE '^v[0-9]+\.[0-9]+\.[0-9]+([-+][0-9A-Za-z.-]+)?$'; then + echo "Could not detect the latest version (got: '${PIXLET_VERSION:-}'), using fallback" PIXLET_VERSION="v0.50.2" fi fi @@ -67,8 +87,26 @@ download_binary() { temp_dir=$(mktemp -d -p "$PROJECT_ROOT" -t pixlet_download.XXXXXXXXXX) local temp_file="$temp_dir/$archive_name" - if ! curl -L -o "$temp_file" "$url" 2>/dev/null; then - echo "✗ Failed to download $arch" + # -f so an HTTP error is a failure. Without it curl writes the 404 body + # to the file and exits 0, and the first sign of trouble is tar saying + # "not in gzip format" about what is actually a page of HTML. + if ! curl -fL -o "$temp_file" "$url" 2>/dev/null; then + echo "✗ Failed to download $arch from $url" + rm -rf "$temp_dir" + return 1 + fi + + # Belt and braces: a mirror or proxy can return 200 with an error page. + if ! gzip -t "$temp_file" 2>/dev/null; then + echo "✗ Downloaded file is not a gzip archive: $url" + # These bytes come from whatever answered the request, so strip + # everything non-printable before echoing them: an error page carrying + # terminal escapes would otherwise be able to rewrite this output or + # bury it in a CI log. Printable characters are kept rather than + # hex-encoding the lot, because "" is the diagnostic. + local first_bytes + first_bytes=$(head -c 60 "$temp_file" | tr -cd '[:print:]') + printf ' (first bytes: %s)\n' "$first_bytes" rm -rf "$temp_dir" return 1 fi diff --git a/test/test_pixlet_download.py b/test/test_pixlet_download.py new file mode 100644 index 00000000..e6fd4993 --- /dev/null +++ b/test/test_pixlet_download.py @@ -0,0 +1,180 @@ +""" +Tests for scripts/download_pixlet.sh -- release-tag resolution and download guards. + +Background: Starlark apps render through the pixlet binary, and the installer +that fetches it failed silently. It resolved the release tag by grepping the +GitHub API response for '"tag_name"' and taking the last quoted token on the +match with a greedy sed. When the response arrives on one line that token is +"mentions_count", not the tag, so the script built a URL for a release that +cannot exist -- and `curl -L -o` without -f wrote the 404 body to the file and +exited 0, so the first sign of trouble was tar reporting "not in gzip format" +about a page of HTML. + +The API is pretty-printed by default, which is exactly why this needs a test: +by hand the old command looks correct, and the failure only appears when the +formatting changes. These drive the real script with a stubbed curl on PATH, so +both response shapes are covered without touching the network. +""" + +import re +import shutil +import subprocess +from pathlib import Path + +import pytest + +SCRIPT = Path(__file__).resolve().parent.parent / "scripts" / "download_pixlet.sh" + +PRETTY = """{ + "url": "https://api.github.com/repos/tronbyt/pixlet/releases/12345", + "id": 12345, + "tag_name": "v0.53.1", + "name": "v0.53.1", + "draft": false, + "prerelease": false, + "mentions_count": 3 +} +""" + +# The shape that broke it: one line, and the last quoted token is not the tag. +MINIFIED = ( + '{"url":"https://api.github.com/repos/tronbyt/pixlet/releases/12345",' + '"id":12345,"tag_name":"v0.53.1","name":"v0.53.1","draft":false,' + '"prerelease":false,"mentions_count":3}' +) + + +def run_script(tmp_path, api_body, download=None): + """Run the real script against a stubbed curl. + + Args: + api_body: what the stub returns for the api.github.com request. + download: bytes to write for a release-asset request, or None to make + that request fail the way `curl -f` does on an HTTP error. + """ + root = tmp_path / "project" + (root / "scripts").mkdir(parents=True) + shutil.copy(SCRIPT, root / "scripts" / "download_pixlet.sh") + + api_file = tmp_path / "api.json" + api_file.write_text(api_body) + + stub_dir = tmp_path / "stub" + stub_dir.mkdir() + asset_file = tmp_path / "asset.bin" + if download is not None: + asset_file.write_bytes(download) + + # Stands in for curl, including the -f semantics the fix turns on: without + # -f, real curl writes the error body to the output file and exits 0, which + # is what let a 404 masquerade as a successful download. The stub has to + # honour that or a test of the fix would pass against the old script too. + (stub_dir / "curl").write_text(f"""#!/bin/bash +out="" +url="" +fail_on_error=0 +while [ $# -gt 0 ]; do + case "$1" in + -o) out="$2"; shift 2 ;; + -*f*) fail_on_error=1; shift ;; + -*) shift ;; + *) url="$1"; shift ;; + esac +done +if [[ "$url" == *api.github.com* ]]; then + cat {api_file} + exit 0 +fi +if [ -f "{asset_file}" ]; then + cp "{asset_file}" "$out" + exit 0 +fi +# No asset: stand in for an HTTP 404. +if [ "$fail_on_error" = "1" ]; then + exit 22 +fi +printf '404 Not Found' > "$out" +exit 0 +""") + (stub_dir / "curl").chmod(0o755) + + return subprocess.run( + ["bash", str(root / "scripts" / "download_pixlet.sh")], + capture_output=True, text=True, + env={"PATH": f"{stub_dir}:/usr/bin:/bin:/usr/sbin:/sbin", + "PIXLET_VERSION": "latest"}, + ) + + +def resolved_version(result): + match = re.search(r"^Version: (.+)$", result.stdout, re.M) + assert match, f"no version line in output:\n{result.stdout}" + return match.group(1).strip() + + +def test_script_is_syntactically_valid(): + result = subprocess.run(["bash", "-n", str(SCRIPT)], capture_output=True, text=True) + assert result.returncode == 0, result.stderr + + +@pytest.mark.parametrize("body,label", [(PRETTY, "pretty"), (MINIFIED, "minified")]) +def test_tag_is_resolved_from_either_response_shape(tmp_path, body, label): + """The minified case is the regression: the last quoted token there is + "mentions_count", which is what the old greedy sed captured.""" + result = run_script(tmp_path, body) + assert resolved_version(result) == "v0.53.1", f"{label}: {result.stdout}" + assert "mentions_count" not in result.stdout + + +@pytest.mark.parametrize( + "tag", + ["mentions_count", "v0.53garbage", "0.53", "v0.5", "", "v0.53.1 ; echo pwned"], +) +def test_a_tag_that_is_not_a_release_falls_back(tmp_path, tag): + """A wrong-but-non-empty value is what made the original bug silent, so the + check is on the shape. Partial matches must not pass: "v0.53garbage" and + "0.53" would build a URL for a release that cannot exist.""" + result = run_script(tmp_path, '{"tag_name": "%s"}' % tag) + assert resolved_version(result) == "v0.50.2", result.stdout + assert "using fallback" in result.stdout + + +@pytest.mark.parametrize("tag", ["v0.53.1", "v1.0.0", "v0.54.0-rc.1", "v1.2.3+build.4"]) +def test_real_release_tag_shapes_are_accepted(tmp_path, tag): + assert resolved_version(run_script(tmp_path, '{"tag_name": "%s"}' % tag)) == tag + + +def test_an_http_error_is_reported_as_a_failed_download(tmp_path): + """Without curl -f the 404 body lands in the file and curl exits 0, so the + failure surfaced two steps later as tar complaining about gzip -- about + what was really a page of HTML. It has to be reported where it happened. + + Both versions end at 0/1, so asserting only on the count would pass against + the old script; the discriminating part is which layer reports it. + """ + result = run_script(tmp_path, PRETTY, download=None) + assert "Download complete: 0/1 succeeded" in result.stdout + assert "✓ Downloaded" not in result.stdout + assert "Failed to download" in result.stdout + assert "Failed to extract" not in result.stdout, ( + "an HTTP error should not surface as an extraction failure") + + +def test_a_non_archive_response_is_rejected_before_extraction(tmp_path): + result = run_script(tmp_path, PRETTY, download=b"502 Bad Gateway") + assert "not a gzip archive" in result.stdout + assert "Download complete: 0/1 succeeded" in result.stdout + + +def test_the_diagnostic_cannot_smuggle_terminal_escapes(tmp_path): + """Those bytes come from whatever answered the request. An error page + carrying escapes must not be able to rewrite the output or bury it.""" + hostile = b"\x1b[2J\x1b[31mgone\x1b[0m\rHTTP 200 OK\x08\x08" + result = run_script(tmp_path, PRETTY, download=hostile) + assert "not a gzip archive" in result.stdout + printed = re.search(r"^\s*\(first bytes: (.*)\)$", result.stdout, re.M) + assert printed, f"no diagnostic line:\n{result.stdout}" + assert "DOCTYPE" in printed.group(1), "the useful part of the page was dropped" + for forbidden in ("\x1b", "\r", "\x08", "\x00"): + assert forbidden not in printed.group(1), ( + f"control byte {forbidden!r} reached the terminal")