From 8b875a21cdf409c34bef3965ff2aa5e3f3739825 Mon Sep 17 00:00:00 2001 From: Trefor Southwell Date: Tue, 4 Aug 2026 20:40:17 +0100 Subject: [PATCH 1/3] feat(update): verify downloaded files against GitHub checksums before installing Predbat's self-update trusted whatever came back from raw.githubusercontent.com. The GitHub directory listing already publishes a Git blob SHA per file, and the downloader already used it to decide which files it could skip, but the bytes it actually fetched were never checked against it. A truncated or corrupted download was written straight to disk and moved into place, and check_install() only warned about the mismatch afterwards, by which point Predbat had already restarted into the broken copy. Every file is now verified against its published checksum before it is written, and the staged files are re-verified immediately before install. Any mismatch aborts the whole update, so nothing is installed and Predbat keeps running the version it already has. Released versions are now fetched as a single source archive rather than one request per file, which cuts an 84 request update down to one. Only the files named in the directory listing are extracted, so tests, docs and the rest of the repository are never installed, and the staged path is built from the listed filename rather than the archive member path so a hostile member cannot escape the install directory. Updates from main keep the per-file path, which lets unchanged files continue to be skipped. - Verify downloads in memory before writing, retrying a mismatch up to 3 times - Fetch releases via the GitHub source archive, verifying each extracted file against its expected size and SHA - Fall back to per-file downloads only when the archive cannot be fetched, never when it fails verification - Re-verify staged files against the staged manifest before predbat_update_move() runs, installing nothing if any file fails - Remove staged files when an update is aborted - Write the temporary archive into the install directory rather than /tmp, which under Home Assistant is a small tmpfs the add-on may not have room in - Fix check_install() returning a bare bool on two error paths, which crashed the startup self-check that unpacks a (passed, modified) pair Adds 31 sub-tests covering checksum verification, retry, the archive path, traversal and size guards, fallback routing, staged cleanup and the pre-move gate, including one pinning that the live apps.yaml is never overwritten. Co-Authored-By: Claude Opus 5 (1M context) --- .cspell/custom-dictionary-workspace.txt | 1 + apps/predbat/download.py | 508 +++++++++++++-- apps/predbat/tests/test_download.py | 805 +++++++++++++++++++++++- docs/install.md | 13 + 4 files changed, 1264 insertions(+), 63 deletions(-) diff --git a/.cspell/custom-dictionary-workspace.txt b/.cspell/custom-dictionary-workspace.txt index c83a3324b..8e26c0c9e 100644 --- a/.cspell/custom-dictionary-workspace.txt +++ b/.cspell/custom-dictionary-workspace.txt @@ -489,6 +489,7 @@ timeobj timestep timestr timezone +tmpfs tmrw tojson tottime diff --git a/apps/predbat/download.py b/apps/predbat/download.py index 704b13e70..7469f8ec4 100644 --- a/apps/predbat/download.py +++ b/apps/predbat/download.py @@ -13,15 +13,36 @@ Downloads PredBat source files from GitHub releases, validates installed file integrity via SHA1 hashing, and manages the update/rollback process. + +Every downloaded file is verified against the Git blob SHA published by the +GitHub directory listing before it is written to disk, and the staged files are +re-verified immediately before being installed. A file that fails verification +is never installed and the update is aborted instead. """ import os import requests import yaml import hashlib +import tarfile +import tempfile DEFAULT_PREDBAT_REPOSITORY = "springfall2008/batpred" +# Branch name used for development updates, which are downloaded file by file rather +# than from a release archive so unchanged files can be skipped +MAIN_BRANCH = "main" + +# Number of attempts made for a download that fails integrity verification +DOWNLOAD_MAX_ATTEMPTS = 3 + +# Hard cap on the size of the downloaded release archive, insurance against a corrupt +# or hostile response exhausting memory or disk. The real archive is around 7MB. +MAX_ARCHIVE_BYTES = 200 * 1024 * 1024 + +# Chunk size used when streaming the release archive to disk +ARCHIVE_CHUNK_BYTES = 256 * 1024 + def resolve_predbat_repository(repository=None): """Resolve the GitHub repository used for Predbat self-update operations. @@ -80,10 +101,23 @@ def get_github_directory_listing(tag, repository=None): return None +def compute_data_sha1(data): + """ + Compute the Git blob SHA1 hash of in-memory data (matches GitHub's SHA) + Git computes SHA as: sha1("blob " + filesize + "\0" + contents) + + Args: + data (bytes): The raw file contents + Returns: + str: Git blob SHA1 hash as hex string + """ + header = "blob {}\0".format(len(data)).encode("utf-8") + return hashlib.sha1(header + data).hexdigest() + + def compute_file_sha1(filepath): """ Compute Git blob SHA1 hash of a file (matches GitHub's SHA) - Git computes SHA as: sha1("blob " + filesize + "\0" + contents) Args: filepath (str): Path to the file @@ -91,23 +125,54 @@ def compute_file_sha1(filepath): str: Git blob SHA1 hash as hex string, or None on error """ try: - sha1 = hashlib.sha1() with open(filepath, "rb") as f: data = f.read() - - # Compute Git blob SHA: sha1("blob " + size + "\0" + contents) - header = "blob {}\0".format(len(data)).encode("utf-8") - sha1.update(header + data) - return sha1.hexdigest() + return compute_data_sha1(data) except Exception as e: print("Error: Failed to compute SHA1 for {}: {}".format(filepath, e)) return None -def download_predbat_file_from_github(tag, filename, new_filename, repository=None): +def remove_file_quietly(filepath): + """ + Delete a file, reporting but not raising on failure. + + Used to clean up temporary downloads and staged files after a failed update. + + Args: + filepath (str): Path to the file to remove + """ + try: + if filepath and os.path.exists(filepath): + os.remove(filepath) + except Exception as e: + print("Warn: Failed to remove {}: {}".format(filepath, e)) + + +def remove_staged_files(this_path, files, tag): + """ + Remove staged files, used to clean up after an aborted update. + + Staged files are the downloaded copies named "." which sit alongside + the installed files until the update is committed by ``predbat_update_move()``. + + Args: + this_path (str): The Predbat install directory + files (list): Bare filenames whose staged copies should be removed + tag (str): The version tag used as the staged filename suffix + """ + for filename in files: + remove_file_quietly(os.path.join(this_path, filename + "." + tag)) + + +def download_predbat_file_from_github(tag, filename, new_filename, repository=None, expected_sha=None, attempts=DOWNLOAD_MAX_ATTEMPTS): """ Download a Predbat source file from GitHub and return the contents. + When *expected_sha* is supplied the downloaded data is verified against it before + anything is written, so a corrupt or truncated download never reaches disk. A + mismatch is retried up to *attempts* times before the download is abandoned. + Args: tag (str): The tag to download from (e.g. v1.0.0). filename (str): The filename to download (e.g. predbat.py). @@ -115,32 +180,380 @@ def download_predbat_file_from_github(tag, filename, new_filename, repository=No repository (str, optional): GitHub repository override in "owner/repo" format (e.g. "springfall2008/batpred"). If not provided, the value is resolved via ``resolve_predbat_repository()``. + expected_sha (str, optional): Expected Git blob SHA of the file contents. When + omitted the download is not verified. + attempts (int, optional): Number of attempts made when verification fails. Returns: - bytes: The raw contents of the file. + bytes: The raw contents of the file, or None on failure. """ repository = resolve_predbat_repository(repository) url = "https://raw.githubusercontent.com/{}/".format(repository) + tag + "/apps/predbat/{}".format(filename) - print("Downloading {}".format(url)) - r = requests.get(url, headers={}) - if r.ok: + + for attempt in range(1, attempts + 1): + if attempt == 1: + print("Downloading {}".format(url)) + else: + print("Downloading {} (attempt {} of {})".format(url, attempt, attempts)) + + try: + r = requests.get(url, headers={}) + except Exception as e: + print("Warn: Exception while downloading {}: {}".format(url, e)) + continue + + if not r.ok: + print("Error: Failed to download {}".format(url)) + return None + # Use raw bytes (not r.text, which decodes through a guessed text encoding and would # corrupt binary files such as the prediction kernel .so binaries) and write in binary # mode so both source files and binaries round-trip byte-for-byte. data = r.content + + if expected_sha: + actual_sha = compute_data_sha1(data) + if actual_sha != expected_sha: + print("Warn: Checksum mismatch for {}: expected {}, got {}".format(filename, expected_sha, actual_sha)) + continue + print("Got data, writing to {}".format(new_filename)) if new_filename: with open(new_filename, "wb") as han: han.write(data) return data - else: - print("Error: Failed to download {}".format(url)) + + print("Error: Failed to download {} after {} attempts".format(url, attempts)) + return None + + +def download_predbat_release_archive(tag, repository=None, target_dir=None): + """ + Download the GitHub source archive for a tag to a temporary file. + + The archive is streamed to disk rather than held in memory, and is capped at + ``MAX_ARCHIVE_BYTES`` as insurance against a corrupt or hostile response. + + The temporary file is written into the install directory rather than the system + temporary directory, because under Home Assistant /tmp is a small tmpfs that the + add-on may not have room in. Writing it alongside the staged files means anywhere + with space for the update has space for the archive. + + Args: + tag (str): The tag to download (e.g. v1.0.0). + repository (str, optional): GitHub repository override in "owner/repo" format. + If not provided, the value is resolved via ``resolve_predbat_repository()``. + target_dir (str, optional): Directory to write the temporary file into. Defaults + to the Predbat install directory. + + Returns: + str: Path to the downloaded temporary file, which the caller is responsible for + deleting, or None if the archive could not be fetched. + """ + repository = resolve_predbat_repository(repository) + if target_dir is None: + target_dir = os.path.dirname(__file__) + url = "https://github.com/{}/archive/{}.tar.gz".format(repository, tag) + print("Downloading release archive {}".format(url)) + + temp_path = None + try: + response = requests.get(url, headers={}, stream=True) + if not response.ok: + print("Error: Failed to download release archive {}, status code: {}".format(url, getattr(response, "status_code", "unknown"))) + return None + + handle, temp_path = tempfile.mkstemp(prefix="predbat-archive-", suffix=".tar.gz", dir=target_dir) + total = 0 + with os.fdopen(handle, "wb") as archive: + for chunk in response.iter_content(chunk_size=ARCHIVE_CHUNK_BYTES): + if not chunk: + continue + total += len(chunk) + if total > MAX_ARCHIVE_BYTES: + raise ValueError("Release archive exceeds the maximum allowed size of {} bytes".format(MAX_ARCHIVE_BYTES)) + archive.write(chunk) + + print("Downloaded release archive, {} bytes".format(total)) + return temp_path + except Exception as e: + print("Error: Exception while downloading release archive {}: {}".format(url, e)) + remove_file_quietly(temp_path) + return None + + +def match_archive_member(member_name): + """ + Match a release archive member against the apps/predbat install directory. + + Archive members are named "-/apps/predbat/". Only files sitting + directly inside apps/predbat are install candidates, so subdirectories such as tests/ + and config/, and anything elsewhere in the repository, return None. + + Args: + member_name (str): The member path from the archive + + Returns: + str: The bare filename, or None if the member is not an install candidate + """ + parts = member_name.split("/") + if len(parts) != 4: + return None + if parts[1] != "apps" or parts[2] != "predbat": + return None + filename = parts[3] + if not filename or filename in (".", ".."): + return None + return filename + + +def extract_predbat_files_from_archive(archive_path, tag, file_list, this_path): + """ + Extract and verify the Predbat install files from a downloaded release archive. + + Only the files named in *file_list* (the GitHub directory listing) are extracted; + everything else in the archive is ignored, so documentation, tests and the rest of + the repository are never installed. Each file is checked against its expected size + and Git blob SHA before being written, and the staged path is built from the listed + filename rather than the archive member path so a hostile member cannot escape the + install directory. + + Args: + archive_path (str): Path to the downloaded .tar.gz archive + tag (str): The version tag used as the staged filename suffix + file_list (list): GitHub directory listing entries with name/size/sha + this_path (str): The Predbat install directory + + Returns: + list: Bare filenames of the staged files, or None if verification failed. + """ + wanted = {} + for file_info in file_list: + name = file_info.get("name") + if name: + wanted[name] = file_info + + staged_files = [] + try: + with tarfile.open(archive_path, "r:gz") as archive: + # Single forward pass over the archive, members are matched against the directory + # listing rather than extracted wholesale, so seeking back through the compressed + # stream is never needed and only the wanted files are ever read + for member in archive: + filename = match_archive_member(member.name) + if filename is None or filename not in wanted or filename in staged_files: + continue + + file_info = wanted[filename] + if not member.isfile(): + raise ValueError("Archive member {} is not a regular file".format(member.name)) + + expected_size = file_info.get("size") + if expected_size is not None and member.size != expected_size: + raise ValueError("Archive member {} size mismatch: expected {}, got {}".format(filename, expected_size, member.size)) + + handle = archive.extractfile(member) + if handle is None: + raise ValueError("Failed to read archive member {}".format(filename)) + data = handle.read() + + expected_sha = file_info.get("sha") + if expected_sha: + actual_sha = compute_data_sha1(data) + if actual_sha != expected_sha: + raise ValueError("Checksum mismatch for {}: expected {}, got {}".format(filename, expected_sha, actual_sha)) + + with open(os.path.join(this_path, filename + "." + tag), "wb") as staged: + staged.write(data) + staged_files.append(filename) + + missing = sorted(set(wanted) - set(staged_files)) + if missing: + raise ValueError("Release archive is missing {} expected file(s): {}".format(len(missing), ", ".join(missing))) + except Exception as e: + print("Error: Failed to extract release archive: {}".format(e)) + remove_staged_files(this_path, staged_files, tag) return None + print("Extracted and verified {} file(s) from the release archive".format(len(staged_files))) + return staged_files + + +def download_predbat_files_from_archive(tag, file_list, this_path, repository=None, attempts=DOWNLOAD_MAX_ATTEMPTS): + """ + Download and stage the Predbat files for a tag using the GitHub source archive. + + A single archive download replaces one request per file. If the archive downloads but + fails verification the whole download is retried, and after *attempts* failures the + update is abandoned rather than falling back, because content that does not match the + published checksums should never be installed by another route. + + Args: + tag (str): The version tag to download + file_list (list): GitHub directory listing entries with name/size/sha + this_path (str): The Predbat install directory + repository (str, optional): GitHub repository override in "owner/repo" format + attempts (int, optional): Number of attempts made when verification fails + + Returns: + tuple: (staged filenames or None, bool indicating whether the caller may fall back + to downloading the files individually) + """ + for attempt in range(1, attempts + 1): + archive_path = download_predbat_release_archive(tag, repository=repository, target_dir=this_path) + if not archive_path: + # The archive could not be fetched at all, which can happen when the archive + # host is blocked, so the caller is allowed to try the per-file download + print("Warn: Release archive could not be downloaded") + return None, True + + try: + staged_files = extract_predbat_files_from_archive(archive_path, tag, file_list, this_path) + finally: + remove_file_quietly(archive_path) + + if staged_files is not None: + return staged_files, False + + print("Warn: Release archive failed verification (attempt {} of {})".format(attempt, attempts)) + + print("Error: Release archive failed verification after {} attempts, aborting update".format(attempts)) + return None, False + + +def download_predbat_files_individually(tag, file_list, this_path, repository=None): + """ + Download and stage the Predbat files one at a time from raw.githubusercontent.com. + + Files whose installed copy already matches the expected SHA are copied rather than + re-downloaded, which keeps development updates from the main branch cheap. Every + downloaded file is verified against its expected Git blob SHA before being written, + and any failure aborts the update and removes the files staged so far. + + Args: + tag (str): The version tag to download + file_list (list): GitHub directory listing entries with name/size/sha + this_path (str): The Predbat install directory + repository (str, optional): GitHub repository override in "owner/repo" format + + Returns: + list: Bare filenames of the staged files, or None on failure. + """ + staged_files = [] + skipped_files = [] + + for file_info in file_list: + filename = file_info["name"] + expected_sha = file_info.get("sha") + local_filepath = os.path.join(this_path, filename) + download_filepath = os.path.join(this_path, filename + "." + tag) + + # Check if local file exists and has matching SHA + skip_download = False + if expected_sha and os.path.exists(local_filepath): + local_sha = compute_file_sha1(local_filepath) + if local_sha == expected_sha: + print("Skipping {}: local file matches remote SHA ({})".format(filename, expected_sha[:8])) + # Copy local file to download location so move operation works + try: + with open(local_filepath, "rb") as src: + with open(download_filepath, "wb") as dst: + dst.write(src.read()) + skip_download = True + skipped_files.append(filename) + except Exception as e: + print("Warn: Failed to copy local file {}: {}".format(filename, e)) + skip_download = False + + if not skip_download: + if not download_predbat_file_from_github(tag, filename, download_filepath, repository=repository, expected_sha=expected_sha): + print("Error: Failed to download {}".format(filename)) + if tag == MAIN_BRANCH: + print("Info: When updating from the main branch a checksum mismatch can happen if a commit lands part way through the download, please retry the update") + remove_file_quietly(download_filepath) + remove_staged_files(this_path, staged_files, tag) + return None + + staged_files.append(filename) + + if skipped_files: + print("\nSkipped downloading {} file(s) (already up to date): {}".format(len(skipped_files), ", ".join(skipped_files))) + + return staged_files + + +def verify_staged_files(this_path, files, tag): + """ + Verify the staged files against the staged manifest before they are installed. + + This is the final gate before the update is committed, catching a file that was + corrupted between download and install, for example by a failed write or a full disk. + + The manifest itself is not verified because it is generated locally and is the source + of the expected checksums. If no staged manifest is present the files cannot be + checked, which is reported but not treated as a failure since the files were already + verified at download time. + + Args: + this_path (str): The Predbat install directory + files (list): Bare filenames whose staged copies should be verified + tag (str): The version tag used as the staged filename suffix + + Returns: + bool: True if every staged file is present and matches, False otherwise + """ + manifest_file = os.path.join(this_path, "manifest.yaml." + tag) + if not os.path.exists(manifest_file): + print("Warn: Staged manifest {} is missing, unable to verify the staged files".format(manifest_file)) + return True + + try: + with open(manifest_file, "r") as han: + manifest = yaml.safe_load(han) + except Exception as e: + print("Error: Failed to load staged manifest {}: {}".format(manifest_file, e)) + return False + + if not manifest or not isinstance(manifest, list): + print("Error: Staged manifest {} is not valid".format(manifest_file)) + return False + + expected = {} + for file_info in manifest: + if isinstance(file_info, dict) and file_info.get("name"): + expected[file_info["name"]] = file_info + + verified = True + for filename in files: + staged_path = os.path.join(this_path, filename + "." + tag) + if not os.path.exists(staged_path): + print("Error: Staged file {} is missing".format(staged_path)) + verified = False + continue + + file_info = expected.get(filename) + if not file_info: + # The manifest has no entry for itself as it is generated locally + continue + + expected_sha = file_info.get("sha") + if not expected_sha: + continue + + actual_sha = compute_file_sha1(staged_path) + if actual_sha != expected_sha: + print("Error: Staged file {} checksum mismatch: expected {}, got {}".format(staged_path, expected_sha, actual_sha)) + verified = False + + return verified + def predbat_update_move(version, files): """ Move the updated files into place + + The staged files are re-verified against the staged manifest first, and nothing is + moved unless every one of them matches, so a corrupted file is never installed. """ if not files: return False @@ -148,6 +561,11 @@ def predbat_update_move(version, files): if tag_split: tag = tag_split[0] this_path = os.path.dirname(__file__) + + if not verify_staged_files(this_path, files, tag): + print("Error: Staged files failed verification, no files have been installed") + return False + cmd = "" for file in files: cmd += "mv -f {} {} && ".format(os.path.join(this_path, file + "." + tag), os.path.join(this_path, file)) @@ -203,7 +621,7 @@ def check_install(version, repository=None): if not files: print("Error: Manifest is empty") - return False + return False, False validation_passed = True validation_modified = False @@ -242,65 +660,50 @@ def check_install(version, repository=None): except Exception as e: print("Error: Failed to load manifest: {}".format(e)) - return False + return False, False def predbat_update_download(version, repository=None): """ Download the defined version of Predbat from GitHub. + Released versions are fetched as a single source archive, which is one request rather + than one per file, while updates from the main branch are fetched file by file so that + unchanged files can be skipped. Either way every file is verified against the Git blob + SHA published in the GitHub directory listing, and a file that fails verification + aborts the whole update so nothing is installed. + Args: version (str): The version string (e.g. v8.30.8). repository (str, optional): GitHub repository override in "owner/repo" format (e.g. "springfall2008/batpred"). If not provided, the value is resolved via ``resolve_predbat_repository()``. + + Returns: + list: Bare filenames staged for install including the manifest, or None on failure. """ this_path = os.path.dirname(__file__) tag_split = version.split(" ") if tag_split: tag = tag_split[0] - # Get the list of files from GitHub API + # Get the list of files from GitHub API, this is the authority for both which files + # are installed and what each of them must hash to file_list = get_github_directory_listing(tag, repository=repository) if not file_list: print("Error: Failed to get file list from GitHub") return None - # Download all files (skip if local file has matching SHA) - downloaded_files = [] - skipped_files = [] - for file_info in file_list: - filename = file_info["name"] - expected_sha = file_info.get("sha") - local_filepath = os.path.join(this_path, filename) - download_filepath = os.path.join(this_path, filename + "." + tag) - - # Check if local file exists and has matching SHA - skip_download = False - if expected_sha and os.path.exists(local_filepath): - local_sha = compute_file_sha1(local_filepath) - if local_sha == expected_sha: - print("Skipping {}: local file matches remote SHA ({})".format(filename, expected_sha[:8])) - # Copy local file to download location so move operation works - try: - with open(local_filepath, "rb") as src: - with open(download_filepath, "wb") as dst: - dst.write(src.read()) - skip_download = True - skipped_files.append(filename) - except Exception as e: - print("Warn: Failed to copy local file {}: {}".format(filename, e)) - skip_download = False - - if not skip_download: - if not download_predbat_file_from_github(tag, filename, download_filepath, repository=repository): - print("Error: Failed to download {}".format(filename)) - return None - - downloaded_files.append(filename) - - if skipped_files: - print("\nSkipped downloading {} file(s) (already up to date): {}".format(len(skipped_files), ", ".join(skipped_files))) + if tag == MAIN_BRANCH: + downloaded_files = download_predbat_files_individually(tag, file_list, this_path, repository=repository) + else: + downloaded_files, allow_fallback = download_predbat_files_from_archive(tag, file_list, this_path, repository=repository) + if downloaded_files is None and allow_fallback: + print("Warn: Falling back to downloading the files individually") + downloaded_files = download_predbat_files_individually(tag, file_list, this_path, repository=repository) + + if downloaded_files is None: + return None # Sort files alphabetically file_list_sorted = sorted(file_list, key=lambda x: x["name"]) @@ -313,6 +716,7 @@ def predbat_update_download(version, repository=None): print("Generated manifest: {}".format(manifest_filename)) except Exception as e: print("Error: Failed to write manifest: {}".format(e)) + remove_staged_files(this_path, downloaded_files, tag) return None # Return list of files including manifest diff --git a/apps/predbat/tests/test_download.py b/apps/predbat/tests/test_download.py index fe05a2730..364ff9f7b 100644 --- a/apps/predbat/tests/test_download.py +++ b/apps/predbat/tests/test_download.py @@ -9,8 +9,10 @@ # pylint: disable=attribute-defined-outside-init import os +import io import sys import importlib +import tarfile import tempfile import shutil from unittest.mock import patch @@ -25,11 +27,17 @@ get_github_directory_listing, check_install, predbat_update_download, + compute_data_sha1, compute_file_sha1, download_predbat_file_from_github, + download_predbat_release_archive, + extract_predbat_files_from_archive, + match_archive_member, predbat_update_move, resolve_predbat_repository, + verify_staged_files, DEFAULT_PREDBAT_REPOSITORY, + DOWNLOAD_MAX_ATTEMPTS, ) @@ -77,6 +85,31 @@ def test_download(my_predbat): ("update_move_invalid_version", _test_predbat_update_move_invalid_version, "Move files invalid version"), ("update_download_skip_matching", _test_predbat_update_download_skip_matching_sha, "Update download skips files with matching SHA"), ("update_download_skip_mixed", _test_predbat_update_download_skip_mixed, "Update download skips some files, downloads others"), + ("compute_data_sha1", _test_compute_data_sha1, "Compute Git blob SHA1 of in-memory data"), + ("download_verify_match", _test_download_file_verifies_sha, "Download writes file when checksum matches"), + ("download_verify_mismatch", _test_download_file_sha_mismatch_aborts, "Download aborts and writes nothing when checksum never matches"), + ("download_verify_retry", _test_download_file_sha_retry_succeeds, "Download retries a checksum mismatch and keeps the good copy"), + ("archive_download_success", _test_download_release_archive_success, "Release archive streams to a temporary file"), + ("archive_download_failure", _test_download_release_archive_failure, "Release archive download failure returns None"), + ("archive_download_too_large", _test_download_release_archive_too_large, "Release archive over the size cap is rejected and cleaned up"), + ("archive_member_match", _test_match_archive_member, "Archive member matching accepts only apps/predbat files"), + ("archive_extract_success", _test_extract_archive_success, "Archive extract stages only the listed files"), + ("archive_extract_apps_yaml", _test_extract_archive_preserves_live_apps_yaml, "Archive extract never overwrites the live apps.yaml"), + ("archive_extract_mismatch", _test_extract_archive_checksum_mismatch, "Archive extract aborts and cleans up on checksum mismatch"), + ("archive_extract_size", _test_extract_archive_size_mismatch, "Archive extract aborts on member size mismatch"), + ("archive_extract_missing", _test_extract_archive_missing_file, "Archive extract aborts when a listed file is absent"), + ("archive_extract_traversal", _test_extract_archive_rejects_traversal, "Archive extract ignores path traversal members"), + ("update_download_release_archive", _test_update_download_release_uses_archive, "Release download uses the archive, not per-file requests"), + ("update_download_main_individual", _test_update_download_main_uses_individual, "Main branch download uses per-file requests, not the archive"), + ("update_download_archive_fallback", _test_update_download_archive_fetch_failure_falls_back, "Archive fetch failure falls back to per-file download"), + ("update_download_archive_abort", _test_update_download_archive_verify_failure_aborts, "Archive verification failure aborts without falling back"), + ("update_download_cleanup", _test_update_download_cleans_staged_on_failure, "Failed download removes the files staged so far"), + ("update_move_verifies", _test_update_move_verifies_staged_files, "Move verifies staged files against the staged manifest"), + ("update_move_mismatch", _test_update_move_blocks_on_staged_mismatch, "Move installs nothing when a staged file is corrupt"), + ("update_move_missing", _test_update_move_blocks_on_missing_staged_file, "Move installs nothing when a staged file is missing"), + ("update_move_no_manifest", _test_update_move_without_manifest_proceeds, "Move proceeds with a warning when no staged manifest exists"), + ("verify_staged_bad_manifest", _test_verify_staged_files_invalid_manifest, "Staged verification fails on an invalid manifest"), + ("check_install_empty_manifest", _test_check_install_empty_manifest, "Check install returns a pair when the manifest is empty"), ("predbat_main_repo_override", _test_predbat_download_main_uses_configured_repository, "Predbat main update uses configured repository override"), ("predbat_tag_repo_upstream", _test_predbat_download_tag_uses_default_repository, "Predbat tagged update always uses upstream repository"), ("predbat_startup_pins_upstream", _test_predbat_startup_self_check_uses_default_repository, "Predbat startup self-check pins repository to upstream"), @@ -454,7 +487,7 @@ def _test_check_install_no_manifest_downloads(my_predbat): def _test_predbat_update_download_success(my_predbat): """ - Test successful download of all files + Test successful download of all files (main branch uses the per-file path) """ temp_dir = tempfile.mkdtemp() @@ -465,14 +498,14 @@ def _test_predbat_update_download_success(my_predbat): with patch("download.os.path.dirname", return_value=temp_dir): with patch("download.get_github_directory_listing", return_value=mock_files): with patch("download.download_predbat_file_from_github", return_value="file content"): - result = predbat_update_download("v8.30.8") + result = predbat_update_download("main") assert result is not None assert "manifest.yaml" in result assert "predbat.py" in result assert "config.py" in result # Check manifest file was created - assert os.path.exists(os.path.join(temp_dir, "manifest.yaml.v8.30.8")) + assert os.path.exists(os.path.join(temp_dir, "manifest.yaml.main")) finally: shutil.rmtree(temp_dir) @@ -508,7 +541,7 @@ def _test_predbat_update_download_file_failure(my_predbat): with patch("download.os.path.dirname", return_value=temp_dir): with patch("download.get_github_directory_listing", return_value=mock_files): with patch("download.download_predbat_file_from_github", return_value=None): - result = predbat_update_download("v8.30.8") + result = predbat_update_download("main") assert result is None finally: @@ -625,6 +658,9 @@ def _test_predbat_update_move_success(my_predbat): with open(tagged_file, "w") as f: f.write("content of {}\n".format(filename)) + # The staged manifest is re-checked before anything is moved, so it has to match + _write_staged_manifest(temp_dir, tag, ["predbat.py", "config.py"]) + # Mock os.system and os.path.dirname with patch("download.os.path.dirname", return_value=temp_dir): with patch("download.os.system") as mock_system: @@ -703,7 +739,7 @@ def _test_predbat_update_download_skip_matching_sha(my_predbat): download_called = False - def mock_download(tag, filename, output_path, repository=None): + def mock_download(tag, filename, output_path, repository=None, expected_sha=None): nonlocal download_called download_called = True return "downloaded content" @@ -711,7 +747,7 @@ def mock_download(tag, filename, output_path, repository=None): with patch("download.os.path.dirname", return_value=temp_dir): with patch("download.get_github_directory_listing", return_value=mock_files): with patch("download.download_predbat_file_from_github", side_effect=mock_download): - result = predbat_update_download("v8.30.8") + result = predbat_update_download("main") # Verify download was skipped (not called) assert download_called is False, "download_predbat_file_from_github should not be called when SHA matches" @@ -722,7 +758,7 @@ def mock_download(tag, filename, output_path, repository=None): assert "manifest.yaml" in result # Verify staged file exists (copied from local) - staged_file = os.path.join(temp_dir, "predbat.py.v8.30.8") + staged_file = os.path.join(temp_dir, "predbat.py.main") assert os.path.exists(staged_file), "Staged file should exist after copy" with open(staged_file, "r") as f: staged_content = f.read() @@ -760,7 +796,7 @@ def _test_predbat_update_download_skip_mixed(my_predbat): download_calls = [] - def mock_download(tag, filename, output_path, repository=None): + def mock_download(tag, filename, output_path, repository=None, expected_sha=None): download_calls.append(filename) with open(output_path, "w") as f: f.write("downloaded content for {}\n".format(filename)) @@ -769,7 +805,7 @@ def mock_download(tag, filename, output_path, repository=None): with patch("download.os.path.dirname", return_value=temp_dir): with patch("download.get_github_directory_listing", return_value=mock_files): with patch("download.download_predbat_file_from_github", side_effect=mock_download): - result = predbat_update_download("v8.30.8") + result = predbat_update_download("main") # Verify only config.py was downloaded (not predbat.py) assert len(download_calls) == 1, "Should download only 1 file" @@ -782,14 +818,14 @@ def mock_download(tag, filename, output_path, repository=None): assert "config.py" in result # Verify predbat.py was copied (not downloaded) - staged_file1 = os.path.join(temp_dir, "predbat.py.v8.30.8") + staged_file1 = os.path.join(temp_dir, "predbat.py.main") assert os.path.exists(staged_file1) with open(staged_file1, "r") as f: content = f.read() assert content == file1_content, "Staged predbat.py should be copied from local" # Verify config.py was downloaded (new content) - staged_file2 = os.path.join(temp_dir, "config.py.v8.30.8") + staged_file2 = os.path.join(temp_dir, "config.py.main") assert os.path.exists(staged_file2) with open(staged_file2, "r") as f: content = f.read() @@ -800,6 +836,753 @@ def mock_download(tag, filename, output_path, repository=None): return 0 +class _MockStreamResponse: + """Minimal stand-in for a streamed requests response used by the archive tests.""" + + def __init__(self, chunks, ok=True, status_code=200): + """Store the chunks the fake response will yield.""" + self.chunks = chunks + self.ok = ok + self.status_code = status_code + + def iter_content(self, chunk_size=None): + """Yield the configured chunks, ignoring the requested chunk size.""" + for chunk in self.chunks: + yield chunk + + +def _build_test_archive(archive_path, prefix, predbat_files, extra_members=None): + """ + Build a GitHub style source archive for testing. + + Args: + archive_path (str): Where to write the .tar.gz + prefix (str): The top level directory name GitHub adds (e.g. batpred-8.47.5) + predbat_files (dict): Bare filename to bytes, placed in /apps/predbat/ + extra_members (dict, optional): Full member path to bytes, added verbatim + """ + with tarfile.open(archive_path, "w:gz") as archive: + for name, data in predbat_files.items(): + info = tarfile.TarInfo(name="{}/apps/predbat/{}".format(prefix, name)) + info.size = len(data) + archive.addfile(info, io.BytesIO(data)) + for member_name, data in (extra_members or {}).items(): + info = tarfile.TarInfo(name=member_name) + info.size = len(data) + archive.addfile(info, io.BytesIO(data)) + + +def _listing_for(predbat_files): + """ + Build a GitHub directory listing for the supplied files. + + Args: + predbat_files (dict): Bare filename to bytes + Returns: + list: Listing entries with name/size/sha/type + """ + return [{"name": name, "size": len(data), "sha": compute_data_sha1(data), "type": "file"} for name, data in predbat_files.items()] + + +def _write_staged_manifest(this_path, tag, filenames): + """ + Write a staged manifest describing the staged copies of *filenames*. + + Args: + this_path (str): Directory holding the staged files + tag (str): The version tag used as the staged filename suffix + filenames (list): Bare filenames to describe + """ + manifest = [] + for filename in filenames: + staged_path = os.path.join(this_path, filename + "." + tag) + manifest.append({"name": filename, "size": os.path.getsize(staged_path), "sha": compute_file_sha1(staged_path)}) + with open(os.path.join(this_path, "manifest.yaml." + tag), "w") as han: + yaml.dump(manifest, han) + + +def _test_compute_data_sha1(my_predbat): + """ + Test Git blob SHA1 computation over in-memory data matches the on-disk helper + """ + data = b"test content\n" + assert compute_data_sha1(data) == "d670460b4b4aece5915caf5c68d12f560a9fe3e4" + + temp_dir = tempfile.mkdtemp() + try: + path = os.path.join(temp_dir, "test.txt") + with open(path, "wb") as han: + han.write(data) + assert compute_file_sha1(path) == compute_data_sha1(data) + finally: + shutil.rmtree(temp_dir) + return 0 + + +def _test_download_file_verifies_sha(my_predbat): + """ + Test a download whose checksum matches is written to disk + """ + temp_dir = tempfile.mkdtemp() + + try: + content = b'print("verified content")\n' + output_file = os.path.join(temp_dir, "test.py.v8.30.8") + mock_response = type("MockResponse", (), {"ok": True, "content": content})() + + with patch("download.requests.get", return_value=mock_response) as mock_get: + result = download_predbat_file_from_github("v8.30.8", "test.py", output_file, expected_sha=compute_data_sha1(content)) + + assert result == content + assert os.path.exists(output_file) + with open(output_file, "rb") as han: + assert han.read() == content + assert mock_get.call_count == 1, "A matching checksum should not be retried" + + finally: + shutil.rmtree(temp_dir) + return 0 + + +def _test_download_file_sha_mismatch_aborts(my_predbat): + """ + Test a download whose checksum never matches is retried, then abandoned without writing + """ + temp_dir = tempfile.mkdtemp() + + try: + output_file = os.path.join(temp_dir, "test.py.v8.30.8") + mock_response = type("MockResponse", (), {"ok": True, "content": b"corrupted content\n"})() + + with patch("download.requests.get", return_value=mock_response) as mock_get: + result = download_predbat_file_from_github("v8.30.8", "test.py", output_file, expected_sha=compute_data_sha1(b"the real content\n")) + + assert result is None, "A file that fails verification must not be returned" + assert not os.path.exists(output_file), "A file that fails verification must never be written" + assert mock_get.call_count == DOWNLOAD_MAX_ATTEMPTS, "The download should be retried before giving up" + + finally: + shutil.rmtree(temp_dir) + return 0 + + +def _test_download_file_sha_retry_succeeds(my_predbat): + """ + Test a transient checksum mismatch is retried and the good copy is kept + """ + temp_dir = tempfile.mkdtemp() + + try: + good = b'print("the real content")\n' + output_file = os.path.join(temp_dir, "test.py.v8.30.8") + responses = [ + type("MockResponse", (), {"ok": True, "content": b"truncated"})(), + type("MockResponse", (), {"ok": True, "content": good})(), + ] + + with patch("download.requests.get", side_effect=responses) as mock_get: + result = download_predbat_file_from_github("v8.30.8", "test.py", output_file, expected_sha=compute_data_sha1(good)) + + assert result == good + assert mock_get.call_count == 2 + with open(output_file, "rb") as han: + assert han.read() == good, "Only the verified copy should reach disk" + + finally: + shutil.rmtree(temp_dir) + return 0 + + +def _test_download_release_archive_success(my_predbat): + """ + Test the release archive is streamed to a temporary file + """ + payload = b"archive-bytes" * 100 + response = _MockStreamResponse([payload[:500], payload[500:]]) + temp_dir = tempfile.mkdtemp() + + try: + with patch("download.requests.get", return_value=response) as mock_get: + archive_path = download_predbat_release_archive("v8.30.8", repository="owner/repo", target_dir=temp_dir) + + assert archive_path is not None + # The archive is written beside the staged files, not into the system temporary + # directory, which under Home Assistant is a small tmpfs + assert os.path.dirname(archive_path) == temp_dir + with open(archive_path, "rb") as han: + assert han.read() == payload + called_url = mock_get.call_args[0][0] + assert called_url == "https://github.com/owner/repo/archive/v8.30.8.tar.gz" + assert mock_get.call_args.kwargs.get("stream") is True, "The archive must be streamed, not buffered in memory" + + finally: + shutil.rmtree(temp_dir) + return 0 + + +def _test_download_release_archive_failure(my_predbat): + """ + Test a failed release archive request returns None + """ + temp_dir = tempfile.mkdtemp() + + try: + response = _MockStreamResponse([], ok=False, status_code=404) + + with patch("download.requests.get", return_value=response): + assert download_predbat_release_archive("v8.30.8", target_dir=temp_dir) is None + + with patch("download.requests.get", side_effect=Exception("Network error")): + assert download_predbat_release_archive("v8.30.8", target_dir=temp_dir) is None + + assert os.listdir(temp_dir) == [], "A failed archive download must not leave a partial file behind" + + finally: + shutil.rmtree(temp_dir) + return 0 + + +def _test_download_release_archive_too_large(my_predbat): + """ + Test an oversized release archive is rejected and its temporary file removed + """ + temp_dir = tempfile.mkdtemp() + + try: + handle, temp_path = tempfile.mkstemp(prefix="oversized-", suffix=".tar.gz", dir=temp_dir) + + # One chunk larger than the cap, so the very first write trips the limit + response = _MockStreamResponse([b"x" * 16]) + + with patch("download.requests.get", return_value=response): + with patch("download.tempfile.mkstemp", return_value=(handle, temp_path)): + with patch("download.MAX_ARCHIVE_BYTES", 8): + result = download_predbat_release_archive("v8.30.8", target_dir=temp_dir) + + assert result is None + assert not os.path.exists(temp_path), "The partial archive should be cleaned up" + + finally: + shutil.rmtree(temp_dir) + return 0 + + +def _test_match_archive_member(my_predbat): + """ + Test archive member matching accepts only files directly inside apps/predbat + """ + assert match_archive_member("batpred-8.47.5/apps/predbat/predbat.py") == "predbat.py" + # Subdirectories are not part of the install set + assert match_archive_member("batpred-8.47.5/apps/predbat/tests/test_download.py") is None + assert match_archive_member("batpred-8.47.5/apps/predbat/config/inverter.yaml") is None + # Anything outside apps/predbat is ignored + assert match_archive_member("batpred-8.47.5/docs/install.md") is None + assert match_archive_member("batpred-8.47.5/apps/other/predbat.py") is None + # Traversal attempts never resolve to a bare filename + assert match_archive_member("batpred-8.47.5/apps/predbat/../../../evil.py") is None + assert match_archive_member("batpred-8.47.5/apps/predbat/..") is None + return 0 + + +def _test_extract_archive_success(my_predbat): + """ + Test extraction stages exactly the listed files and ignores the rest of the archive + """ + temp_dir = tempfile.mkdtemp() + + try: + predbat_files = {"predbat.py": b'print("predbat")\n', "config.py": b'print("config")\n'} + extras = { + "batpred-8.30.8/apps/predbat/tests/test_download.py": b"tests are not installed\n", + "batpred-8.30.8/apps/predbat/config/inverter.yaml": b"config: true\n", + "batpred-8.30.8/docs/install.md": b"# docs are not installed\n", + "batpred-8.30.8/apps/predbat/unlisted.py": b"not in the listing\n", + } + archive_path = os.path.join(temp_dir, "release.tar.gz") + _build_test_archive(archive_path, "batpred-8.30.8", predbat_files, extras) + + staged = extract_predbat_files_from_archive(archive_path, "v8.30.8", _listing_for(predbat_files), temp_dir) + + assert staged is not None + assert sorted(staged) == ["config.py", "predbat.py"] + for name, data in predbat_files.items(): + with open(os.path.join(temp_dir, name + ".v8.30.8"), "rb") as han: + assert han.read() == data + + # Nothing outside the listing should have been written anywhere + written = sorted(os.listdir(temp_dir)) + assert written == ["config.py.v8.30.8", "predbat.py.v8.30.8", "release.tar.gz"], written + + finally: + shutil.rmtree(temp_dir) + return 0 + + +def _test_extract_archive_preserves_live_apps_yaml(my_predbat): + """ + Test the live apps.yaml is never overwritten by an update + + For the Predbat app the install directory is also the add-on config directory, so the + user's live apps.yaml sits right next to the installed files. The template ships as + apps/predbat/config/apps.yaml, inside a subdirectory, so it is neither in the GitHub + directory listing nor an install candidate, and the user's configuration survives. + """ + temp_dir = tempfile.mkdtemp() + + try: + # The user's live configuration, sitting in the install directory + live_apps_yaml = os.path.join(temp_dir, "apps.yaml") + live_content = b"pred_bat:\n my_precious_settings: true\n" + with open(live_apps_yaml, "wb") as han: + han.write(live_content) + + predbat_files = {"predbat.py": b'print("predbat")\n'} + extras = { + # The shipped template, in the config subdirectory as it is in the real repository + "batpred-8.30.8/apps/predbat/config/apps.yaml": b"pred_bat:\n template: true\n", + "batpred-8.30.8/coverage/apps.yaml": b"pred_bat:\n test_fixture: true\n", + } + archive_path = os.path.join(temp_dir, "release.tar.gz") + _build_test_archive(archive_path, "batpred-8.30.8", predbat_files, extras) + + staged = extract_predbat_files_from_archive(archive_path, "v8.30.8", _listing_for(predbat_files), temp_dir) + + assert staged == ["predbat.py"] + assert not os.path.exists(os.path.join(temp_dir, "apps.yaml.v8.30.8")), "apps.yaml must never be staged for install" + with open(live_apps_yaml, "rb") as han: + assert han.read() == live_content, "The live apps.yaml must be left untouched" + + finally: + shutil.rmtree(temp_dir) + return 0 + + +def _test_extract_archive_checksum_mismatch(my_predbat): + """ + Test a tampered archive file aborts extraction and leaves nothing staged + """ + temp_dir = tempfile.mkdtemp() + + try: + predbat_files = {"aaa_first.py": b'print("staged before the bad one")\n', "zzz_tampered.py": b'print("original")\n'} + archive_path = os.path.join(temp_dir, "release.tar.gz") + _build_test_archive(archive_path, "batpred-8.30.8", predbat_files) + + listing = _listing_for(predbat_files) + for entry in listing: + if entry["name"] == "zzz_tampered.py": + entry["sha"] = compute_data_sha1(b'print("something else entirely")\n') + + staged = extract_predbat_files_from_archive(archive_path, "v8.30.8", listing, temp_dir) + + assert staged is None, "A checksum mismatch must abort the extraction" + assert os.listdir(temp_dir) == ["release.tar.gz"], "Files staged before the failure should be cleaned up" + + finally: + shutil.rmtree(temp_dir) + return 0 + + +def _test_extract_archive_size_mismatch(my_predbat): + """ + Test a member whose size disagrees with the listing aborts extraction + """ + temp_dir = tempfile.mkdtemp() + + try: + predbat_files = {"predbat.py": b'print("predbat")\n'} + archive_path = os.path.join(temp_dir, "release.tar.gz") + _build_test_archive(archive_path, "batpred-8.30.8", predbat_files) + + listing = _listing_for(predbat_files) + listing[0]["size"] = 999999 + + staged = extract_predbat_files_from_archive(archive_path, "v8.30.8", listing, temp_dir) + + assert staged is None + assert os.listdir(temp_dir) == ["release.tar.gz"] + + finally: + shutil.rmtree(temp_dir) + return 0 + + +def _test_extract_archive_missing_file(my_predbat): + """ + Test extraction aborts when a listed file is absent from the archive + """ + temp_dir = tempfile.mkdtemp() + + try: + predbat_files = {"predbat.py": b'print("predbat")\n'} + archive_path = os.path.join(temp_dir, "release.tar.gz") + _build_test_archive(archive_path, "batpred-8.30.8", predbat_files) + + listing = _listing_for(predbat_files) + listing.append({"name": "missing.py", "size": 10, "sha": compute_data_sha1(b"missing\n"), "type": "file"}) + + staged = extract_predbat_files_from_archive(archive_path, "v8.30.8", listing, temp_dir) + + assert staged is None, "An incomplete archive must abort the extraction" + assert os.listdir(temp_dir) == ["release.tar.gz"] + + finally: + shutil.rmtree(temp_dir) + return 0 + + +def _test_extract_archive_rejects_traversal(my_predbat): + """ + Test a path traversal member cannot write outside the install directory + + Staged paths are built from the listed filename rather than the archive member path, + so a hostile member has no route out of the install directory even if the listing + itself names something outside it. + """ + temp_dir = tempfile.mkdtemp() + + try: + install_dir = os.path.join(temp_dir, "install") + os.makedirs(install_dir) + + predbat_files = {"predbat.py": b'print("predbat")\n'} + evil = b'print("pwned")\n' + extras = { + "batpred-8.30.8/apps/predbat/../../../evil.py": evil, + "../../../../evil_absolute.py": evil, + } + archive_path = os.path.join(temp_dir, "release.tar.gz") + _build_test_archive(archive_path, "batpred-8.30.8", predbat_files, extras) + + staged = extract_predbat_files_from_archive(archive_path, "v8.30.8", _listing_for(predbat_files), install_dir) + + assert staged == ["predbat.py"] + assert sorted(os.listdir(install_dir)) == ["predbat.py.v8.30.8"] + assert sorted(os.listdir(temp_dir)) == ["install", "release.tar.gz"], "Nothing may be written outside the install directory" + assert not os.path.exists(os.path.join(os.path.dirname(temp_dir), "evil.py")) + + finally: + shutil.rmtree(temp_dir) + return 0 + + +def _test_update_download_release_uses_archive(my_predbat): + """ + Test a release version is downloaded via the archive rather than one request per file + """ + temp_dir = tempfile.mkdtemp() + + try: + predbat_files = {"predbat.py": b'print("predbat")\n', "config.py": b'print("config")\n'} + archive_path = os.path.join(temp_dir, "release.tar.gz") + _build_test_archive(archive_path, "batpred-8.30.8", predbat_files) + + # download_predbat_release_archive hands ownership of the file to its caller, which + # deletes it, so give it a throwaway copy + def fake_archive_download(tag, repository=None, target_dir=None): + """Hand out a throwaway copy of the test archive, as the real downloader would.""" + copy_path = os.path.join(temp_dir, "release-copy.tar.gz") + shutil.copyfile(archive_path, copy_path) + return copy_path + + with patch("download.os.path.dirname", return_value=temp_dir): + with patch("download.get_github_directory_listing", return_value=_listing_for(predbat_files)): + with patch("download.download_predbat_release_archive", side_effect=fake_archive_download) as mock_archive: + with patch("download.download_predbat_file_from_github") as mock_per_file: + result = predbat_update_download("v8.30.8") + + assert result is not None + assert sorted(result) == ["config.py", "manifest.yaml", "predbat.py"] + assert mock_archive.call_count == 1 + assert not mock_per_file.called, "The archive path must not fall back to per-file downloads" + assert os.path.exists(os.path.join(temp_dir, "manifest.yaml.v8.30.8")) + with open(os.path.join(temp_dir, "predbat.py.v8.30.8"), "rb") as han: + assert han.read() == predbat_files["predbat.py"] + assert not os.path.exists(os.path.join(temp_dir, "release-copy.tar.gz")), "The temporary archive should be deleted" + + finally: + shutil.rmtree(temp_dir) + return 0 + + +def _test_update_download_main_uses_individual(my_predbat): + """ + Test a main branch update downloads file by file so unchanged files can be skipped + """ + temp_dir = tempfile.mkdtemp() + + try: + mock_files = [{"name": "predbat.py", "size": 10, "sha": "abc123", "type": "file"}] + + with patch("download.os.path.dirname", return_value=temp_dir): + with patch("download.get_github_directory_listing", return_value=mock_files): + with patch("download.download_predbat_release_archive") as mock_archive: + with patch("download.download_predbat_file_from_github", return_value=b"content") as mock_per_file: + result = predbat_update_download("main") + + assert result is not None + assert not mock_archive.called, "The main branch must not use the release archive" + assert mock_per_file.called + assert mock_per_file.call_args.kwargs.get("expected_sha") == "abc123", "The expected SHA must be passed through for verification" + + finally: + shutil.rmtree(temp_dir) + return 0 + + +def _test_update_download_archive_fetch_failure_falls_back(my_predbat): + """ + Test an archive that cannot be fetched falls back to downloading files individually + """ + temp_dir = tempfile.mkdtemp() + + try: + mock_files = [{"name": "predbat.py", "size": 10, "sha": "abc123", "type": "file"}] + + with patch("download.os.path.dirname", return_value=temp_dir): + with patch("download.get_github_directory_listing", return_value=mock_files): + with patch("download.download_predbat_release_archive", return_value=None) as mock_archive: + with patch("download.download_predbat_file_from_github", return_value=b"content") as mock_per_file: + result = predbat_update_download("v8.30.8") + + assert result is not None, "A blocked archive host should not break the update" + assert mock_archive.call_count == 1, "A fetch failure should fall back rather than retry the archive" + assert mock_per_file.called, "The per-file download should be used as the fallback" + + finally: + shutil.rmtree(temp_dir) + return 0 + + +def _test_update_download_archive_verify_failure_aborts(my_predbat): + """ + Test an archive that fails verification aborts instead of falling back + + Content that does not match the published checksums must not be installed by another + route, so verification failure is retried and then abandoned. + """ + temp_dir = tempfile.mkdtemp() + + try: + predbat_files = {"predbat.py": b'print("predbat")\n'} + archive_path = os.path.join(temp_dir, "release.tar.gz") + _build_test_archive(archive_path, "batpred-8.30.8", predbat_files) + + listing = _listing_for(predbat_files) + listing[0]["sha"] = compute_data_sha1(b'print("not what was published")\n') + + def fake_archive_download(tag, repository=None, target_dir=None): + """Hand out a throwaway copy of the test archive, as the real downloader would.""" + copy_path = os.path.join(temp_dir, "release-copy.tar.gz") + shutil.copyfile(archive_path, copy_path) + return copy_path + + with patch("download.os.path.dirname", return_value=temp_dir): + with patch("download.get_github_directory_listing", return_value=listing): + with patch("download.download_predbat_release_archive", side_effect=fake_archive_download) as mock_archive: + with patch("download.download_predbat_file_from_github") as mock_per_file: + result = predbat_update_download("v8.30.8") + + assert result is None, "A tampered archive must abort the update" + assert mock_archive.call_count == DOWNLOAD_MAX_ATTEMPTS, "The archive should be retried before giving up" + assert not mock_per_file.called, "Verification failure must not fall back to another download route" + assert not os.path.exists(os.path.join(temp_dir, "predbat.py.v8.30.8")) + assert not os.path.exists(os.path.join(temp_dir, "manifest.yaml.v8.30.8")) + + finally: + shutil.rmtree(temp_dir) + return 0 + + +def _test_update_download_cleans_staged_on_failure(my_predbat): + """ + Test an aborted download removes the files it had already staged + """ + temp_dir = tempfile.mkdtemp() + + try: + mock_files = [ + {"name": "first.py", "size": 10, "sha": "aaa111", "type": "file"}, + {"name": "second.py", "size": 10, "sha": "bbb222", "type": "file"}, + ] + + def mock_download(tag, filename, output_path, repository=None, expected_sha=None): + if filename == "second.py": + return None + with open(output_path, "wb") as han: + han.write(b"staged\n") + return b"staged\n" + + with patch("download.os.path.dirname", return_value=temp_dir): + with patch("download.get_github_directory_listing", return_value=mock_files): + with patch("download.download_predbat_file_from_github", side_effect=mock_download): + result = predbat_update_download("main") + + assert result is None + assert os.listdir(temp_dir) == [], "A failed update must not leave staged files behind" + + finally: + shutil.rmtree(temp_dir) + return 0 + + +def _test_update_move_verifies_staged_files(my_predbat): + """ + Test the move verifies the staged files against the staged manifest before installing + """ + temp_dir = tempfile.mkdtemp() + + try: + files = ["predbat.py", "config.py"] + tag = "v8.30.8" + for filename in files: + with open(os.path.join(temp_dir, filename + "." + tag), "wb") as han: + han.write("content of {}\n".format(filename).encode("utf-8")) + _write_staged_manifest(temp_dir, tag, files) + + assert verify_staged_files(temp_dir, files, tag) is True + + with patch("download.os.path.dirname", return_value=temp_dir): + with patch("download.os.system") as mock_system: + assert predbat_update_move(tag, files + ["manifest.yaml"]) is True + assert mock_system.called + + finally: + shutil.rmtree(temp_dir) + return 0 + + +def _test_update_move_blocks_on_staged_mismatch(my_predbat): + """ + Test a staged file corrupted after download is never installed + """ + temp_dir = tempfile.mkdtemp() + + try: + files = ["predbat.py", "config.py"] + tag = "v8.30.8" + for filename in files: + with open(os.path.join(temp_dir, filename + "." + tag), "wb") as han: + han.write("content of {}\n".format(filename).encode("utf-8")) + _write_staged_manifest(temp_dir, tag, files) + + # Corrupt a staged file after the manifest was written, as a bad write would + with open(os.path.join(temp_dir, "config.py." + tag), "wb") as han: + han.write(b"truncated") + + assert verify_staged_files(temp_dir, files, tag) is False + + with patch("download.os.path.dirname", return_value=temp_dir): + with patch("download.os.system") as mock_system: + assert predbat_update_move(tag, files + ["manifest.yaml"]) is False + assert not mock_system.called, "Nothing may be installed when a staged file fails verification" + + # The installed files must be untouched + assert not os.path.exists(os.path.join(temp_dir, "predbat.py")) + assert not os.path.exists(os.path.join(temp_dir, "config.py")) + + finally: + shutil.rmtree(temp_dir) + return 0 + + +def _test_update_move_blocks_on_missing_staged_file(my_predbat): + """ + Test a missing staged file blocks the install + """ + temp_dir = tempfile.mkdtemp() + + try: + files = ["predbat.py", "config.py"] + tag = "v8.30.8" + for filename in files: + with open(os.path.join(temp_dir, filename + "." + tag), "wb") as han: + han.write("content of {}\n".format(filename).encode("utf-8")) + _write_staged_manifest(temp_dir, tag, files) + os.remove(os.path.join(temp_dir, "config.py." + tag)) + + with patch("download.os.path.dirname", return_value=temp_dir): + with patch("download.os.system") as mock_system: + assert predbat_update_move(tag, files) is False + assert not mock_system.called + + finally: + shutil.rmtree(temp_dir) + return 0 + + +def _test_update_move_without_manifest_proceeds(my_predbat): + """ + Test the move proceeds with a warning when there is no staged manifest to check against + """ + temp_dir = tempfile.mkdtemp() + + try: + tag = "v8.30.8" + with open(os.path.join(temp_dir, "predbat.py." + tag), "wb") as han: + han.write(b"content\n") + + assert verify_staged_files(temp_dir, ["predbat.py"], tag) is True + + with patch("download.os.path.dirname", return_value=temp_dir): + with patch("download.os.system") as mock_system: + assert predbat_update_move(tag, ["predbat.py"]) is True + assert mock_system.called + + finally: + shutil.rmtree(temp_dir) + return 0 + + +def _test_verify_staged_files_invalid_manifest(my_predbat): + """ + Test an unreadable or malformed staged manifest fails verification + """ + temp_dir = tempfile.mkdtemp() + + try: + tag = "v8.30.8" + with open(os.path.join(temp_dir, "predbat.py." + tag), "wb") as han: + han.write(b"content\n") + + # Not a list of file entries + with open(os.path.join(temp_dir, "manifest.yaml." + tag), "w") as han: + han.write("this is not a manifest\n") + assert verify_staged_files(temp_dir, ["predbat.py"], tag) is False + + # Empty manifest + with open(os.path.join(temp_dir, "manifest.yaml." + tag), "w") as han: + han.write("") + assert verify_staged_files(temp_dir, ["predbat.py"], tag) is False + + finally: + shutil.rmtree(temp_dir) + return 0 + + +def _test_check_install_empty_manifest(my_predbat): + """ + Test check_install returns a (passed, modified) pair when the manifest is empty + + Callers unpack two values, so returning a bare bool here used to crash the startup + self-check rather than reporting a failed install. + """ + temp_dir = tempfile.mkdtemp() + + try: + with open(os.path.join(temp_dir, "manifest.yaml"), "w") as han: + han.write("") + + with patch("download.os.path.dirname", return_value=temp_dir): + result, modified = check_install("v8.30.8") + assert result is False + assert modified is False + + finally: + shutil.rmtree(temp_dir) + return 0 + + def _test_predbat_download_main_uses_configured_repository(my_predbat): """Test download_predbat_version('main') uses get_predbat_repository() value.""" predbat_module = _import_predbat_module_for_tests() diff --git a/docs/install.md b/docs/install.md index ef1dee2fd..2535e8e15 100644 --- a/docs/install.md +++ b/docs/install.md @@ -416,6 +416,19 @@ You can also set environment variable `PREDBAT_REPOSITORY` (same `owner/repo` fo Once Predbat has been installed and configured you should update Predbat to the latest version by selecting the latest version in the **select.predbat_update** selector, or by turning on the **switch.predbat_auto_update** to auto-update Predbat. +### Update integrity checking + +Every file Predbat downloads during an update is checked against the checksum GitHub publishes for it, +both when it is downloaded and again immediately before it is installed. +If any file does not match, the update is abandoned and **nothing is installed**, so a corrupted or truncated download can never leave you with a broken Predbat. +Predbat carries on running the version you already have, and you can simply retry the update. + +Released versions are fetched as a single compressed archive rather than one request per file, which makes updating considerably quicker. +Only the files that make up Predbat are installed from it; your `apps.yaml` and the rest of your configuration are never touched by an update. + +If you are updating from the `main` branch rather than a release, an occasional checksum mismatch is possible +when a change is merged part way through your download. Retrying the update resolves it. + ## Manually installing a Predbat release The Predbat version selector **select.predbat_update** contains the last 25 Predbat releases, but sometimes if Predbat has stopped working, From e2737dfd0e166e0b7875318e810f054c0b0093d3 Mon Sep 17 00:00:00 2001 From: Trefor Southwell Date: Tue, 4 Aug 2026 20:49:29 +0100 Subject: [PATCH 2/3] refactor(update): use the archive for any ref instead of hard coding a branch name Routing on `tag == "main"` assumed the only mutable ref anyone would update from is called main. That is not a safe assumption: predbat_repository exists so a fork can be used, and a fork's default branch may be named something else, so the per-file path was selected by name rather than by what is actually available. GitHub builds a source archive for any ref it knows about, so the archive is now tried for every version and the per-file download is what happens when no archive comes back, not what happens when the ref has a particular name. Measured on v8.47.5 the archive is around 25x faster than 84 individual requests, so there is no case where the per-file path is the better first choice. Downloading from a mutable ref can race with a commit landing part way through, leaving the directory listing describing one commit and the archive another. The retry now re-fetches the listing rather than repeatedly checking new content against a stale one, so the update settles on the newer commit instead of failing, and the manifest is written from the listing the files were actually verified against so the pre-install check stays consistent. - Try the archive for every ref, fall back to per-file only when none is returned - Re-fetch the directory listing on each retry so a moved ref self-heals - Drop the MAIN_BRANCH constant and its name based routing entirely - Replace the "main uses per-file" test with one asserting the archive is tried for main, master, an arbitrary branch name and a release tag - Add a test that a mismatch caused by a ref moving mid-update recovers on retry and produces a manifest consistent with the installed files Co-Authored-By: Claude Opus 5 (1M context) --- apps/predbat/download.py | 109 ++++++++++++------------- apps/predbat/tests/test_download.py | 119 ++++++++++++++++++++++------ docs/install.md | 9 ++- 3 files changed, 156 insertions(+), 81 deletions(-) diff --git a/apps/predbat/download.py b/apps/predbat/download.py index 7469f8ec4..a822ce584 100644 --- a/apps/predbat/download.py +++ b/apps/predbat/download.py @@ -29,10 +29,6 @@ DEFAULT_PREDBAT_REPOSITORY = "springfall2008/batpred" -# Branch name used for development updates, which are downloaded file by file rather -# than from a release archive so unchanged files can be skipped -MAIN_BRANCH = "main" - # Number of attempts made for a download that fails integrity verification DOWNLOAD_MAX_ATTEMPTS = 3 @@ -260,7 +256,8 @@ def download_predbat_release_archive(tag, repository=None, target_dir=None): try: response = requests.get(url, headers={}, stream=True) if not response.ok: - print("Error: Failed to download release archive {}, status code: {}".format(url, getattr(response, "status_code", "unknown"))) + # Not an error in itself, the caller downloads the files individually instead + print("Warn: No release archive at {}, status code: {}".format(url, getattr(response, "status_code", "unknown"))) return None handle, temp_path = tempfile.mkstemp(prefix="predbat-archive-", suffix=".tar.gz", dir=target_dir) @@ -379,46 +376,35 @@ def extract_predbat_files_from_archive(archive_path, tag, file_list, this_path): return staged_files -def download_predbat_files_from_archive(tag, file_list, this_path, repository=None, attempts=DOWNLOAD_MAX_ATTEMPTS): +def download_predbat_files_from_archive(tag, file_list, this_path, repository=None): """ - Download and stage the Predbat files for a tag using the GitHub source archive. + Download and stage the Predbat files for a ref using the GitHub source archive. - A single archive download replaces one request per file. If the archive downloads but - fails verification the whole download is retried, and after *attempts* failures the - update is abandoned rather than falling back, because content that does not match the - published checksums should never be installed by another route. + A single archive download replaces one request per file, which is around 25 times + faster for a full update. GitHub builds an archive for any ref it knows about, so this + is tried for every version rather than for particular ref names, and the caller falls + back to the per-file download only if no archive comes back. Args: - tag (str): The version tag to download + tag (str): The version tag or branch to download file_list (list): GitHub directory listing entries with name/size/sha this_path (str): The Predbat install directory repository (str, optional): GitHub repository override in "owner/repo" format - attempts (int, optional): Number of attempts made when verification fails Returns: - tuple: (staged filenames or None, bool indicating whether the caller may fall back - to downloading the files individually) + tuple: (staged filenames or None, bool indicating whether an archive was obtained). + The second element distinguishes "there is no archive to use here" from "the + archive did not match the directory listing", which the caller handles + differently. """ - for attempt in range(1, attempts + 1): - archive_path = download_predbat_release_archive(tag, repository=repository, target_dir=this_path) - if not archive_path: - # The archive could not be fetched at all, which can happen when the archive - # host is blocked, so the caller is allowed to try the per-file download - print("Warn: Release archive could not be downloaded") - return None, True - - try: - staged_files = extract_predbat_files_from_archive(archive_path, tag, file_list, this_path) - finally: - remove_file_quietly(archive_path) + archive_path = download_predbat_release_archive(tag, repository=repository, target_dir=this_path) + if not archive_path: + return None, False - if staged_files is not None: - return staged_files, False - - print("Warn: Release archive failed verification (attempt {} of {})".format(attempt, attempts)) - - print("Error: Release archive failed verification after {} attempts, aborting update".format(attempts)) - return None, False + try: + return extract_predbat_files_from_archive(archive_path, tag, file_list, this_path), True + finally: + remove_file_quietly(archive_path) def download_predbat_files_individually(tag, file_list, this_path, repository=None): @@ -468,8 +454,7 @@ def download_predbat_files_individually(tag, file_list, this_path, repository=No if not skip_download: if not download_predbat_file_from_github(tag, filename, download_filepath, repository=repository, expected_sha=expected_sha): print("Error: Failed to download {}".format(filename)) - if tag == MAIN_BRANCH: - print("Info: When updating from the main branch a checksum mismatch can happen if a commit lands part way through the download, please retry the update") + print("Info: When updating from a branch rather than a release, a checksum mismatch can happen if a commit lands part way through the download, please retry the update") remove_file_quietly(download_filepath) remove_staged_files(this_path, staged_files, tag) return None @@ -667,11 +652,16 @@ def predbat_update_download(version, repository=None): """ Download the defined version of Predbat from GitHub. - Released versions are fetched as a single source archive, which is one request rather - than one per file, while updates from the main branch are fetched file by file so that - unchanged files can be skipped. Either way every file is verified against the Git blob - SHA published in the GitHub directory listing, and a file that fails verification - aborts the whole update so nothing is installed. + The GitHub source archive is used whenever one is available, which is one request + instead of one per file and around 25 times faster for a full update. GitHub builds an + archive for any ref it knows about, so no assumption is made about which versions have + one; if none comes back the files are downloaded individually instead, which also lets + unchanged files be skipped. + + Every file is verified against the Git blob SHA published in the GitHub directory + listing, and a file that fails verification aborts the whole update so nothing is + installed. A mismatch is retried against a freshly fetched listing, because updating + from a branch rather than a release can race with a commit landing part way through. Args: version (str): The version string (e.g. v8.30.8). @@ -684,31 +674,40 @@ def predbat_update_download(version, repository=None): """ this_path = os.path.dirname(__file__) tag_split = version.split(" ") - if tag_split: - tag = tag_split[0] + if not tag_split: + return None + tag = tag_split[0] - # Get the list of files from GitHub API, this is the authority for both which files - # are installed and what each of them must hash to + for attempt in range(1, DOWNLOAD_MAX_ATTEMPTS + 1): + # The listing is the authority for both which files are installed and what each of + # them must hash to. It is fetched again on each attempt so that a ref which moved + # under us is picked up rather than repeatedly checked against a stale listing. file_list = get_github_directory_listing(tag, repository=repository) if not file_list: print("Error: Failed to get file list from GitHub") return None - if tag == MAIN_BRANCH: - downloaded_files = download_predbat_files_individually(tag, file_list, this_path, repository=repository) - else: - downloaded_files, allow_fallback = download_predbat_files_from_archive(tag, file_list, this_path, repository=repository) - if downloaded_files is None and allow_fallback: - print("Warn: Falling back to downloading the files individually") - downloaded_files = download_predbat_files_individually(tag, file_list, this_path, repository=repository) + downloaded_files, archive_available = download_predbat_files_from_archive(tag, file_list, this_path, repository=repository) if downloaded_files is None: - return None + if not archive_available: + # No archive came back for this ref, so download the files one at a time + print("Warn: No release archive available, downloading the files individually") + downloaded_files = download_predbat_files_individually(tag, file_list, this_path, repository=repository) + if downloaded_files is None: + return None + else: + # The archive was fetched but did not match the listing. Rather than install + # it by another route, try again with a fresh listing, which also resolves a + # branch that moved between the listing and the archive being fetched. + print("Warn: Downloaded files did not match the directory listing (attempt {} of {})".format(attempt, DOWNLOAD_MAX_ATTEMPTS)) + continue # Sort files alphabetically file_list_sorted = sorted(file_list, key=lambda x: x["name"]) - # Generate manifest.yaml (just the sorted file list from GitHub API) + # Generate manifest.yaml (the sorted file list the downloaded files were verified + # against, so the pre-install check uses the same checksums) manifest_filename = os.path.join(this_path, "manifest.yaml." + tag) try: with open(manifest_filename, "w") as f: @@ -722,6 +721,8 @@ def predbat_update_download(version, repository=None): # Return list of files including manifest downloaded_files.append("manifest.yaml") return downloaded_files + + print("Error: Downloaded files failed verification after {} attempts, aborting update".format(DOWNLOAD_MAX_ATTEMPTS)) return None diff --git a/apps/predbat/tests/test_download.py b/apps/predbat/tests/test_download.py index 364ff9f7b..7289bbfbe 100644 --- a/apps/predbat/tests/test_download.py +++ b/apps/predbat/tests/test_download.py @@ -100,8 +100,9 @@ def test_download(my_predbat): ("archive_extract_missing", _test_extract_archive_missing_file, "Archive extract aborts when a listed file is absent"), ("archive_extract_traversal", _test_extract_archive_rejects_traversal, "Archive extract ignores path traversal members"), ("update_download_release_archive", _test_update_download_release_uses_archive, "Release download uses the archive, not per-file requests"), - ("update_download_main_individual", _test_update_download_main_uses_individual, "Main branch download uses per-file requests, not the archive"), - ("update_download_archive_fallback", _test_update_download_archive_fetch_failure_falls_back, "Archive fetch failure falls back to per-file download"), + ("update_download_any_ref", _test_update_download_archive_tried_for_every_ref, "Archive is tried for any ref, with no hard coded branch name"), + ("update_download_fresh_listing", _test_update_download_retries_with_fresh_listing, "A mismatch is retried against a freshly fetched listing"), + ("update_download_archive_fallback", _test_update_download_archive_fetch_failure_falls_back, "No archive available falls back to per-file download"), ("update_download_archive_abort", _test_update_download_archive_verify_failure_aborts, "Archive verification failure aborts without falling back"), ("update_download_cleanup", _test_update_download_cleans_staged_on_failure, "Failed download removes the files staged so far"), ("update_move_verifies", _test_update_move_verifies_staged_files, "Move verifies staged files against the staged manifest"), @@ -497,15 +498,16 @@ def _test_predbat_update_download_success(my_predbat): with patch("download.os.path.dirname", return_value=temp_dir): with patch("download.get_github_directory_listing", return_value=mock_files): - with patch("download.download_predbat_file_from_github", return_value="file content"): - result = predbat_update_download("main") + with patch("download.download_predbat_release_archive", return_value=None): + with patch("download.download_predbat_file_from_github", return_value="file content"): + result = predbat_update_download("v8.30.8") assert result is not None assert "manifest.yaml" in result assert "predbat.py" in result assert "config.py" in result # Check manifest file was created - assert os.path.exists(os.path.join(temp_dir, "manifest.yaml.main")) + assert os.path.exists(os.path.join(temp_dir, "manifest.yaml.v8.30.8")) finally: shutil.rmtree(temp_dir) @@ -540,9 +542,10 @@ def _test_predbat_update_download_file_failure(my_predbat): with patch("download.os.path.dirname", return_value=temp_dir): with patch("download.get_github_directory_listing", return_value=mock_files): - with patch("download.download_predbat_file_from_github", return_value=None): - result = predbat_update_download("main") - assert result is None + with patch("download.download_predbat_release_archive", return_value=None): + with patch("download.download_predbat_file_from_github", return_value=None): + result = predbat_update_download("v8.30.8") + assert result is None finally: shutil.rmtree(temp_dir) @@ -746,7 +749,8 @@ def mock_download(tag, filename, output_path, repository=None, expected_sha=None with patch("download.os.path.dirname", return_value=temp_dir): with patch("download.get_github_directory_listing", return_value=mock_files): - with patch("download.download_predbat_file_from_github", side_effect=mock_download): + # The per-file path is now reached only when no archive is available + with patch("download.download_predbat_release_archive", return_value=None), patch("download.download_predbat_file_from_github", side_effect=mock_download): result = predbat_update_download("main") # Verify download was skipped (not called) @@ -804,7 +808,8 @@ def mock_download(tag, filename, output_path, repository=None, expected_sha=None with patch("download.os.path.dirname", return_value=temp_dir): with patch("download.get_github_directory_listing", return_value=mock_files): - with patch("download.download_predbat_file_from_github", side_effect=mock_download): + # The per-file path is now reached only when no archive is available + with patch("download.download_predbat_release_archive", return_value=None), patch("download.download_predbat_file_from_github", side_effect=mock_download): result = predbat_update_download("main") # Verify only config.py was downloaded (not predbat.py) @@ -1305,25 +1310,91 @@ def fake_archive_download(tag, repository=None, target_dir=None): return 0 -def _test_update_download_main_uses_individual(my_predbat): +def _test_update_download_archive_tried_for_every_ref(my_predbat): """ - Test a main branch update downloads file by file so unchanged files can be skipped + Test the archive is tried for every ref, not only for ref names that look like releases + + GitHub builds an archive for any ref it knows about, so branches get the same single + request treatment as releases and nothing is keyed off a hard coded branch name. """ temp_dir = tempfile.mkdtemp() try: - mock_files = [{"name": "predbat.py", "size": 10, "sha": "abc123", "type": "file"}] + predbat_files = {"predbat.py": b'print("predbat")\n'} + archive_path = os.path.join(temp_dir, "release.tar.gz") + _build_test_archive(archive_path, "batpred-branch", predbat_files) + + for ref in ["main", "master", "some-feature-branch", "v8.30.8"]: + staged_dir = os.path.join(temp_dir, "install-" + ref) + os.makedirs(staged_dir) + + def fake_archive_download(tag, repository=None, target_dir=None): + """Hand out a throwaway copy of the test archive, as the real downloader would.""" + copy_path = os.path.join(target_dir, "release-copy.tar.gz") + shutil.copyfile(archive_path, copy_path) + return copy_path + + with patch("download.os.path.dirname", return_value=staged_dir): + with patch("download.get_github_directory_listing", return_value=_listing_for(predbat_files)): + with patch("download.download_predbat_release_archive", side_effect=fake_archive_download) as mock_archive: + with patch("download.download_predbat_file_from_github") as mock_per_file: + result = predbat_update_download(ref) + + assert result is not None, ref + assert mock_archive.called, "The archive should be tried for ref {}".format(ref) + assert mock_archive.call_args[0][0] == ref + assert not mock_per_file.called, "No per-file requests are needed when the archive works for ref {}".format(ref) + + finally: + shutil.rmtree(temp_dir) + return 0 + + +def _test_update_download_retries_with_fresh_listing(my_predbat): + """ + Test a mismatch is retried against a freshly fetched listing + + Updating from a branch can race with a commit landing part way through, leaving the + listing describing one commit and the archive another. Re-fetching the listing lets + the update settle on the newer commit rather than failing. + """ + temp_dir = tempfile.mkdtemp() + + try: + moved_files = {"predbat.py": b'print("the commit that landed mid-update")\n'} + archive_path = os.path.join(temp_dir, "release.tar.gz") + _build_test_archive(archive_path, "batpred-branch", moved_files) + + # The first listing describes the old commit, so it will not match the archive + stale_listing = [{"name": "predbat.py", "size": len(moved_files["predbat.py"]), "sha": compute_data_sha1(b'print("the commit we started from")\n'), "type": "file"}] + listings = [stale_listing, _listing_for(moved_files)] + + def fake_listing(tag, repository=None): + """Return the stale listing first, then the one matching the archive.""" + return listings.pop(0) if listings else _listing_for(moved_files) + + def fake_archive_download(tag, repository=None, target_dir=None): + """Hand out a throwaway copy of the test archive, as the real downloader would.""" + copy_path = os.path.join(target_dir, "release-copy.tar.gz") + shutil.copyfile(archive_path, copy_path) + return copy_path with patch("download.os.path.dirname", return_value=temp_dir): - with patch("download.get_github_directory_listing", return_value=mock_files): - with patch("download.download_predbat_release_archive") as mock_archive: - with patch("download.download_predbat_file_from_github", return_value=b"content") as mock_per_file: - result = predbat_update_download("main") + with patch("download.get_github_directory_listing", side_effect=fake_listing) as mock_listing: + with patch("download.download_predbat_release_archive", side_effect=fake_archive_download): + with patch("download.download_predbat_file_from_github") as mock_per_file: + result = predbat_update_download("some-branch") - assert result is not None - assert not mock_archive.called, "The main branch must not use the release archive" - assert mock_per_file.called - assert mock_per_file.call_args.kwargs.get("expected_sha") == "abc123", "The expected SHA must be passed through for verification" + assert result is not None, "The update should settle once the listing catches up" + assert mock_listing.call_count == 2, "The listing should be re-fetched after a mismatch" + assert not mock_per_file.called, "A mismatch must not be resolved by downloading from elsewhere" + + # The manifest must describe the listing the files were verified + # against, or the pre-install check would reject them + with open(os.path.join(temp_dir, "manifest.yaml.some-branch"), "r") as han: + manifest = yaml.safe_load(han) + assert manifest[0]["sha"] == compute_data_sha1(moved_files["predbat.py"]) + assert verify_staged_files(temp_dir, result, "some-branch") is True finally: shutil.rmtree(temp_dir) @@ -1378,13 +1449,14 @@ def fake_archive_download(tag, repository=None, target_dir=None): return copy_path with patch("download.os.path.dirname", return_value=temp_dir): - with patch("download.get_github_directory_listing", return_value=listing): + with patch("download.get_github_directory_listing", return_value=listing) as mock_listing: with patch("download.download_predbat_release_archive", side_effect=fake_archive_download) as mock_archive: with patch("download.download_predbat_file_from_github") as mock_per_file: result = predbat_update_download("v8.30.8") assert result is None, "A tampered archive must abort the update" assert mock_archive.call_count == DOWNLOAD_MAX_ATTEMPTS, "The archive should be retried before giving up" + assert mock_listing.call_count == DOWNLOAD_MAX_ATTEMPTS, "Each retry should re-fetch the listing" assert not mock_per_file.called, "Verification failure must not fall back to another download route" assert not os.path.exists(os.path.join(temp_dir, "predbat.py.v8.30.8")) assert not os.path.exists(os.path.join(temp_dir, "manifest.yaml.v8.30.8")) @@ -1415,7 +1487,8 @@ def mock_download(tag, filename, output_path, repository=None, expected_sha=None with patch("download.os.path.dirname", return_value=temp_dir): with patch("download.get_github_directory_listing", return_value=mock_files): - with patch("download.download_predbat_file_from_github", side_effect=mock_download): + # The per-file path is now reached only when no archive is available + with patch("download.download_predbat_release_archive", return_value=None), patch("download.download_predbat_file_from_github", side_effect=mock_download): result = predbat_update_download("main") assert result is None diff --git a/docs/install.md b/docs/install.md index 2535e8e15..633b3ec05 100644 --- a/docs/install.md +++ b/docs/install.md @@ -423,11 +423,12 @@ both when it is downloaded and again immediately before it is installed. If any file does not match, the update is abandoned and **nothing is installed**, so a corrupted or truncated download can never leave you with a broken Predbat. Predbat carries on running the version you already have, and you can simply retry the update. -Released versions are fetched as a single compressed archive rather than one request per file, which makes updating considerably quicker. -Only the files that make up Predbat are installed from it; your `apps.yaml` and the rest of your configuration are never touched by an update. +Updates are fetched as a single compressed archive rather than one request per file, which makes updating considerably quicker. +This applies to branches such as `main` as well as to releases; if no archive is available Predbat falls back to downloading the files one at a time. +Only the files that make up Predbat are installed from the archive; your `apps.yaml` and the rest of your configuration are never touched by an update. -If you are updating from the `main` branch rather than a release, an occasional checksum mismatch is possible -when a change is merged part way through your download. Retrying the update resolves it. +If you are updating from a branch rather than a release, a change merged part way through your download can cause a checksum mismatch. +Predbat retries automatically against a fresh file listing, so this normally resolves itself. ## Manually installing a Predbat release From 83204406d79f400f47cf6e8134bcecc37a59fbf2 Mon Sep 17 00:00:00 2001 From: Trefor Southwell Date: Sat, 8 Aug 2026 08:42:25 +0100 Subject: [PATCH 3/3] fix(update): close the streamed archive response on every exit path With stream=True the connection is only returned to the pool once the body has been consumed, so the paths that bail out early held the socket open until garbage collection. Those are precisely the paths an update retries: a non-OK status never reads the body at all, and the size cap tripping abandons it part way through, both repeated up to three times per update. Closing now happens in a finally block covering the success, non-OK and exception paths, guarded by hasattr() so a response object without close() still works. The mock records closes and three tests assert it, one per exit path; with the finally block disabled all three fail, so the assertions verify the fix rather than just describing it. Co-Authored-By: Claude Opus 5 (1M context) --- apps/predbat/download.py | 11 ++++++++++ apps/predbat/tests/test_download.py | 32 ++++++++++++++++++++++++++++- 2 files changed, 42 insertions(+), 1 deletion(-) diff --git a/apps/predbat/download.py b/apps/predbat/download.py index a822ce584..c808bfb6d 100644 --- a/apps/predbat/download.py +++ b/apps/predbat/download.py @@ -253,6 +253,7 @@ def download_predbat_release_archive(tag, repository=None, target_dir=None): print("Downloading release archive {}".format(url)) temp_path = None + response = None try: response = requests.get(url, headers={}, stream=True) if not response.ok: @@ -277,6 +278,16 @@ def download_predbat_release_archive(tag, repository=None, target_dir=None): print("Error: Exception while downloading release archive {}: {}".format(url, e)) remove_file_quietly(temp_path) return None + finally: + # A streamed response only returns its connection to the pool once the body has + # been consumed, so the paths that bail out early (a non-OK status, or the size cap + # tripping part way through) would hold the socket open until garbage collection. + # Those are exactly the paths an update retries, so close on every exit instead. + if response is not None and hasattr(response, "close"): + try: + response.close() + except Exception as e: + print("Warn: Failed to close the release archive response: {}".format(e)) def match_archive_member(member_name): diff --git a/apps/predbat/tests/test_download.py b/apps/predbat/tests/test_download.py index 7289bbfbe..5ba19ef1b 100644 --- a/apps/predbat/tests/test_download.py +++ b/apps/predbat/tests/test_download.py @@ -844,6 +844,26 @@ def mock_download(tag, filename, output_path, repository=None, expected_sha=None class _MockStreamResponse: """Minimal stand-in for a streamed requests response used by the archive tests.""" + def __init__(self, chunks, ok=True, status_code=200): + """Store the chunks the fake response will yield.""" + self.chunks = chunks + self.ok = ok + self.status_code = status_code + self.closed = False + + def iter_content(self, chunk_size=None): + """Yield the configured chunks, ignoring the requested chunk size.""" + for chunk in self.chunks: + yield chunk + + def close(self): + """Record that the connection was released, as a real streamed response requires.""" + self.closed = True + + +class _MockUnclosableResponse: + """A streamed response with no close() at all, standing in for a simpler test mock.""" + def __init__(self, chunks, ok=True, status_code=200): """Store the chunks the fake response will yield.""" self.chunks = chunks @@ -1019,6 +1039,7 @@ def _test_download_release_archive_success(my_predbat): called_url = mock_get.call_args[0][0] assert called_url == "https://github.com/owner/repo/archive/v8.30.8.tar.gz" assert mock_get.call_args.kwargs.get("stream") is True, "The archive must be streamed, not buffered in memory" + assert response.closed is True, "The streamed response must be closed to release the connection" finally: shutil.rmtree(temp_dir) @@ -1027,7 +1048,7 @@ def _test_download_release_archive_success(my_predbat): def _test_download_release_archive_failure(my_predbat): """ - Test a failed release archive request returns None + Test a failed release archive request returns None and releases the connection """ temp_dir = tempfile.mkdtemp() @@ -1036,10 +1057,18 @@ def _test_download_release_archive_failure(my_predbat): with patch("download.requests.get", return_value=response): assert download_predbat_release_archive("v8.30.8", target_dir=temp_dir) is None + assert response.closed is True, "A non-OK response must still be closed, its body is never read" with patch("download.requests.get", side_effect=Exception("Network error")): assert download_predbat_release_archive("v8.30.8", target_dir=temp_dir) is None + # A response object that has no close() at all must not break the download + unclosable = _MockUnclosableResponse([b"archive-bytes"]) + with patch("download.requests.get", return_value=unclosable): + archive_path = download_predbat_release_archive("v8.30.8", target_dir=temp_dir) + assert archive_path is not None + os.remove(archive_path) + assert os.listdir(temp_dir) == [], "A failed archive download must not leave a partial file behind" finally: @@ -1066,6 +1095,7 @@ def _test_download_release_archive_too_large(my_predbat): assert result is None assert not os.path.exists(temp_path), "The partial archive should be cleaned up" + assert response.closed is True, "A response abandoned part way through must still be closed" finally: shutil.rmtree(temp_dir)