From 4d51a87a04ff627452ccbb3cb1d25c69f0b5969c Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 30 Jun 2026 18:35:46 +0000 Subject: [PATCH 1/2] fix: config validation, download hardening, builder robustness & isolated bug sweep Second sweep batch (27 fixes) across config/ports validation, the download path, the redroid/vm builder, and several isolated correctness bugs. config & ports: - reject well-known ports mapping with a non-canonical guest port; require frida_control alongside frida (#195) - dedup the legacy ports-migration note per resolved path (#202) - accept Docker's documented memswap_limit: -1 (#215) - flag a non-empty modules: list as inert under binder: vm (#200) - validate frida.sha256 as 64-char hex at load time (#194) - warn when a pinned gapps_vendor overrides the minimal/full intent (#220) - remove the dead _MIGRATION_REQUIRED_VERSIONS constant (#242) - magisk-config.sh: observe the magisk --sqlite exit status under set -eu (#239) downloads: - stage into a process-unique temp file before the atomic rename so concurrent fetches can't poison the cache (#185) - stream-decompress frida/modules instead of buffering full payloads (#227) - bound frida .xz decompression output to avoid OOM (#228) - strip query/fragment from module URL basenames so the flash glob matches (#168) builder & rootfs: - pinned-sha256 verify the kernel source tarball before compiling (#184) - write the rootfs .android-version marker before the image rename, and record the actually-baked image version (#234, #187) - validate REDROID_TAR exists in preflight (#186) - docker-daemon preflight for beetroot build (#193) - shell-quote build-context paths in the kernel compile (#208) - force BuildKit for COPY --chmod (#229) - euid preflight for the local rootfs bake (#231) - fcntl.flock the shared clone dir against concurrent builds (#232) - add a repo-root .dockerignore (#207) isolated bugs: - adb mid-batch offline-abort row is stage-neutral and retains the adb error (#223) - render ls/modes tables losslessly off-TTY (#204) - compose.logs() raises on a non-zero exit in non-follow mode (#218) - absolute-path contract for user_config/registry/cache dirs on relative XDG (#225) - atomic write for the materialised compose/vm-asset cache (#226) - snapshot/restore: backend reconcile (#171), overlap guard on non-existent target (#172), self-inclusion guard at any depth (#173) All covered by new tests; full gate green (ruff, mypy --strict src+tests, pytest 100% line+branch (1828 passed), shellcheck/shfmt, yamllint, actionlint, zizmor, codespell, deptry, uv lock, changelog lint). Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01P1YsNMpGizhSDDPCBu5cut --- .dockerignore | 10 + .github/workflows/beetroot-ci.yml | 11 +- .github/workflows/e2e.yml | 6 + .github/workflows/vm-kernel-release.yml | 7 + CHANGELOG.md | 30 ++ docker/magisk-config.sh | 20 +- docs/reference/config.md | 10 +- src/beetroot/backends/adb.py | 15 +- src/beetroot/builder.py | 279 ++++++++++++++---- src/beetroot/compose.py | 9 +- src/beetroot/config.py | 147 ++++++++-- src/beetroot/console.py | 49 +++- src/beetroot/frida_download.py | 53 +++- src/beetroot/kernel_download.py | 18 +- src/beetroot/modules_download.py | 40 ++- src/beetroot/paths.py | 44 ++- src/beetroot/rootfs_download.py | 8 +- src/beetroot/snapshot.py | 44 ++- tests/test_adb_device.py | 9 +- tests/test_bugfix_config_validation.py | 218 ++++++++++++++ tests/test_bugfix_download_temp_streaming.py | 258 +++++++++++++++++ tests/test_bugfix_module_filename.py | 63 +++++ tests/test_bugfix_snapshot_self_inclusion.py | 26 ++ tests/test_builder.py | 281 ++++++++++++++++++- tests/test_compose.py | 11 + tests/test_console.py | 42 +++ tests/test_dockerignore.py | 22 ++ tests/test_magisk_config_helper.py | 43 +++ tests/test_module_auto_install.py | 7 +- tests/test_paths.py | 148 ++++++++++ tests/test_rootfs_download.py | 43 +++ tests/test_snapshot.py | 90 ++++++ 32 files changed, 1919 insertions(+), 142 deletions(-) create mode 100644 .dockerignore create mode 100644 tests/test_bugfix_config_validation.py create mode 100644 tests/test_bugfix_download_temp_streaming.py create mode 100644 tests/test_bugfix_module_filename.py create mode 100644 tests/test_dockerignore.py diff --git a/.dockerignore b/.dockerignore new file mode 100644 index 0000000..b9b6780 --- /dev/null +++ b/.dockerignore @@ -0,0 +1,10 @@ +# Beetroot build context allowlist. +# +# `beetroot build` resolves the build context to the repo root (compose.yaml's +# `context: ${BEETROOT_BUILD_CONTEXT:-.}`), but docker/Dockerfile only COPYs +# docker/*.sh + docker/stealth.rc. Without this file Docker would tar the whole +# root (.venv, .git, instance dirs) into the build context. Exclude everything, +# then re-include only the docker/ tree the build actually consumes. +* +!docker/ +!docker/** diff --git a/.github/workflows/beetroot-ci.yml b/.github/workflows/beetroot-ci.yml index 1c5393a..8df7937 100644 --- a/.github/workflows/beetroot-ci.yml +++ b/.github/workflows/beetroot-ci.yml @@ -237,6 +237,10 @@ jobs: working-directory: .beetroot env: KERNEL_VERSION: "6.12.9" + # sha256 of linux-${KERNEL_VERSION}.tar.xz; verified before extract so + # a tampered CDN tarball can't be compiled. Keep in sync with + # KERNEL_SOURCE_SHA256 in src/beetroot/builder.py (issue #184). + KERNEL_SOURCE_SHA256: "87be0360df0931b340d2bac35161a548070fbc3a8c352c49e21e96666c26aeb4" VM_OUT: ${{ github.workspace }}/.beetroot-vm run: | set -euxo pipefail @@ -251,9 +255,14 @@ jobs: mkdir -p "$VM_OUT" work="$RUNNER_TEMP/linux" mkdir -p "$work" + # Download to a file and verify the pinned source digest BEFORE extract + # (split the curl | tar pipe so the bytes can be hashed first) (#184). + tarball="$RUNNER_TEMP/linux-${KERNEL_VERSION}.tar.xz" curl -fsSL \ "https://cdn.kernel.org/pub/linux/kernel/v6.x/linux-${KERNEL_VERSION}.tar.xz" \ - | tar -xJ -C "$work" --strip-components=1 + -o "$tarball" + echo "${KERNEL_SOURCE_SHA256} ${tarball}" | sha256sum -c - + tar -xJ -C "$work" --strip-components=1 -f "$tarball" cd "$work" make defconfig ./scripts/kconfig/merge_config.sh -m .config \ diff --git a/.github/workflows/e2e.yml b/.github/workflows/e2e.yml index 55d98fa..8023166 100644 --- a/.github/workflows/e2e.yml +++ b/.github/workflows/e2e.yml @@ -338,6 +338,10 @@ jobs: env: # The binder-enabled guest kernel pinned by docs/design/vm-rnd-log.md. KERNEL_VERSION: "6.12.9" + # sha256 of linux-${KERNEL_VERSION}.tar.xz; verified before extract so a + # tampered CDN tarball can't be compiled. Keep in sync with + # KERNEL_SOURCE_SHA256 in src/beetroot/builder.py (issue #184). + KERNEL_SOURCE_SHA256: "87be0360df0931b340d2bac35161a548070fbc3a8c352c49e21e96666c26aeb4" VM_OUT: ${{ github.workspace }}/vm-artifacts BEETROOT_VM_KERNEL: ${{ github.workspace }}/vm-artifacts/bzImage BEETROOT_VM_ROOTFS: ${{ github.workspace }}/vm-artifacts/rootdisk.img @@ -377,6 +381,8 @@ jobs: mkdir -p "$VM_OUT" cd "$RUNNER_TEMP" curl -fsSLO "https://cdn.kernel.org/pub/linux/kernel/v6.x/linux-${KERNEL_VERSION}.tar.xz" + # Verify the pinned source digest BEFORE extract (issue #184). + echo "${KERNEL_SOURCE_SHA256} linux-${KERNEL_VERSION}.tar.xz" | sha256sum -c - tar -xf "linux-${KERNEL_VERSION}.tar.xz" cd "linux-${KERNEL_VERSION}" make defconfig diff --git a/.github/workflows/vm-kernel-release.yml b/.github/workflows/vm-kernel-release.yml index 6ef7acf..7a9ee2a 100644 --- a/.github/workflows/vm-kernel-release.yml +++ b/.github/workflows/vm-kernel-release.yml @@ -33,6 +33,11 @@ jobs: env: # Keep in sync with KERNEL_VERSION in src/beetroot/builder.py and e2e.yml. KERNEL_VERSION: "6.12.9" + # sha256 of linux-${KERNEL_VERSION}.tar.xz from cdn.kernel.org's signed + # sha256sums.asc; verified before extract so a tampered CDN tarball can't + # be compiled into a published bzImage. Keep in sync with + # KERNEL_SOURCE_SHA256 in src/beetroot/builder.py (issue #184). + KERNEL_SOURCE_SHA256: "87be0360df0931b340d2bac35161a548070fbc3a8c352c49e21e96666c26aeb4" steps: - uses: actions/checkout@df4cb1c069e1874edd31b4311f1884172cec0e10 # v6.0.3 with: @@ -62,6 +67,8 @@ jobs: asset="bzImage-${KERNEL_VERSION}-${fp}" cd "$RUNNER_TEMP" curl -fsSLO "https://cdn.kernel.org/pub/linux/kernel/v6.x/linux-${KERNEL_VERSION}.tar.xz" + # Verify the pinned source digest BEFORE extract (issue #184). + echo "${KERNEL_SOURCE_SHA256} linux-${KERNEL_VERSION}.tar.xz" | sha256sum -c - tar -xf "linux-${KERNEL_VERSION}.tar.xz" cd "linux-${KERNEL_VERSION}" make defconfig diff --git a/CHANGELOG.md b/CHANGELOG.md index 4ca0034..f9ac6ca 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,8 @@ ### Security +- **The guest-kernel source tarball is verified against a pinned sha256 before compiling (#184).** The `binder: vm` kernel build (and the release/CI lanes) now hash the downloaded `cdn.kernel.org` tarball and fold the digest into the published prebuilt fingerprint, so a tampered tarball can't be compiled into a trusted `bzImage`. +- **`frida-server` downloads decompress incrementally with a bounded output ceiling (#228).** A corrupt or zip-bomb `.xz` now raises `FridaFetchError` past the ceiling instead of OOM-ing the host. - **Relative `path:` module entries are now contained to the instance directory (#152).** A relative `path:` that resolves outside the instance dir (e.g. `path: ../../../etc/shadow`) is rejected at staging time — the path-traversal analogue of the existing `file://` URL block. **Absolute** `path:` entries (e.g. `path: /tmp/mod.zip`) remain permitted and are read as-is (an unchanged, tested feature). No schema/api_version change. ### Breaking changes @@ -500,6 +502,11 @@ pre-abort rows in its `results` attribute). ### Quality & internals +- **`frida-server` and module downloads stream chunk-by-chunk to disk (#227).** Frida decompresses incrementally instead of buffering the whole compressed and decompressed payload in RAM — matching the rootfs streaming idiom and easing the memory posture on constrained TCG/CI hosts. +- **Added a repo-root `.dockerignore` (#207).** `beetroot build` no longer uploads `.venv/`, `.git/`, or instance dirs as build context — only `docker/` is sent. +- **Warn when a pinned `gapps_vendor` overrides the `minimal`/`full` intent (#220).** The silently-collapsed image/flag is no longer a surprise. +- **Removed the dead, never-read `_MIGRATION_REQUIRED_VERSIONS` constant (#242).** Its comment falsely claimed to enforce the 3→4 stealth migration, which is actually handled by `_reject_stealth_key`. +- **`frida.sha256` is validated as 64-char hex at config-load time (#194).** A fat-fingered digest now fails immediately with a clear error instead of late with a misleading hostile-mirror message. (`module.sha256` validation is tracked as a follow-up — bogus-digest test fixtures need updating first.) - **Fixed self-contradicting `vm.py` comments that claimed `adb connect` succeeds at QEMU `hostfwd`-bind (#241).** It only attaches once the post-boot in-guest relay is listening; the comments and the `_wait_for_boot_completed` docstring @@ -610,6 +617,29 @@ are absent), so shell regressions are caught locally before the push. ### Bug fixes +- **Module URLs carrying a query string or fragment now stage a clean `.zip` basename (#168).** The redroid flash glob (`*.zip`) matches it, so a `m.zip?v=2` URL's module is actually flashed instead of silently skipped. +- **Frida, module, and kernel downloads stage into a process-unique temp file before the atomic rename (#185).** Concurrent fetches of the same artifact can no longer poison the shared user-global cache with a torn file. +- **`beetroot build --vm-kernel [--check]` validates that a set `REDROID_TAR` points at an existing file (#186).** A typo'd tarball fails preflight instead of aborting mid-bake at `docker load`. +- **The rootfs `.android-version` marker records the major version of the actually-baked `REDROID_IMAGE` (#187).** The `binder: vm` skew check no longer trusts a marker that lies when `REDROID_IMAGE` overrides the version. +- **`beetroot build` runs a Docker-daemon preflight (#193).** A daemonless host gets a friendly "start the daemon" message instead of a bare `command failed (exit 1)`. +- **Shell-quote the build-context-derived kernel paths in the `--vm-kernel --from-source` compile (#208).** A checkout under a path with spaces/metacharacters no longer breaks the `merge_config`/`cp` step. +- **Force BuildKit when `beetroot build` runs `docker compose build` (#229).** The BuildKit-only `COPY --chmod` in the Dockerfile no longer aborts on a BuildKit-disabled host. +- **`beetroot build --vm-kernel` fails fast with a root-privilege preflight (#231).** An unprivileged local rootfs bake no longer masquerades as a generic ~60s `staging dockerd did not become ready` timeout. +- **`beetroot build` serializes concurrent runs with an `fcntl.flock` around the shared clone dir (#232).** Two builds can no longer corrupt each other's `rm -rf`/`git clone`/patch tree. +- **The rootfs `.android-version` marker is written before the image is renamed into place (#234).** An interrupted download can't leave a marker-less image that silently skips the skew check. +- **`beetroot ls`/`modes` tables render losslessly when stdout is not a TTY (#204).** No cell truncation or box-drawing borders off-TTY; the overclaiming console docstrings are corrected. +- **`docker compose logs` raises `ComposeError` on a non-zero exit in non-follow mode (#218).** It no longer silently swallows the failure, while still tolerating the Ctrl-C exit under `--follow`. +- **`user_config_dir`/`user_registry_file`/`user_cache_dir` ignore a relative `$XDG_*_HOME` and fall back to `~/.config`/`~/.cache` (#225).** This honors their absolute-path contract instead of fragmenting the registry across working directories. +- **Bundled compose / vm-asset cache materialisation writes via a temp file + `os.replace` (#226).** Concurrent wheel-installed invocations can no longer hand `docker compose` a truncated file. +- **`beetroot restore` rejects a snapshot whose archived `beetroot.yaml` sets `binder: vm` instead of silently restoring it as a redroid instance (#171).** +- **snapshot restore runs the cross-instance directory-overlap guard even when the target doesn't yet exist (#172).** It refuses a restore into a non-existent descendant/ancestor of a registered instance instead of registering a nested one. +- **`beetroot snapshot` excludes its own output archive by resolved path at any directory depth (#173).** Running it from a subdirectory of the instance no longer packs the partially-written archive into itself. +- **Reject a well-known `ports:` mapping whose guest port isn't canonical, and require `frida_control` alongside `frida` (#195).** A mistyped guest port no longer publishes a host port forwarding to a dead guest port. +- **Dedup the legacy ports-mapping migration note by resolved path (#202).** It no longer re-prints on every `load_yaml` across a fleet scan. +- **Accept Docker's documented `memswap_limit: -1` (unlimited swap) (#215).** It's no longer rejected as an invalid size, while `-1` stays rejected for the other size fields. +- **The `binder: vm` inert-config advisory also flags a non-empty `modules:` list (#200).** The Magisk-less guest can never flash them. +- **The mid-batch offline-abort module row is stage-neutral and retains the underlying adb error (#223).** It no longer hardcodes a "mid-install" detail or drops the adb error text. +- **Restructure the magisk-config.sh zygisk read so the `magisk --sqlite` exit status is observed under `set -eu` (#239).** A post-liveness daemon flap no longer triggers a needless zygote restart or a misleading boot abort. - **`binder: vm` `down`/`restart`/`destroy` verify the recorded PID still names this instance's QEMU before signalling (#162).** A stale or reused pidfile can no longer SIGTERM/SIGKILL an unrelated process; `is_running()` and the diff --git a/docker/magisk-config.sh b/docker/magisk-config.sh index 4e97938..6d26b7a 100755 --- a/docker/magisk-config.sh +++ b/docker/magisk-config.sh @@ -50,7 +50,16 @@ echo "[*] Enabling Zygisk + denylist" # is the one that flips it on. Zygisk only injects zygote at zygote start, so a # 0/missing → 1 transition this boot means the running zygote predates Zygisk # and needs a one-shot restart to activate it (and any flashed Zygisk module). -PREV_ZYGISK="$(magisk --sqlite "SELECT value FROM settings WHERE key='zygisk';" | awk -F'=' '{print $NF}')" +# ``magisk --sqlite`` prints each row as ``column=value``; we want the value. +# The prior ``magisk ... | awk -F= '{print $NF}'`` did the right extraction but +# put magisk *inside a pipeline*, so under ``set -eu`` the substitution's exit +# status was awk's (always 0) — a magisk failure after the liveness probe was +# silently masked, leaving PREV_ZYGISK empty and spuriously flagging Zygisk as +# newly enabled. Capture magisk's raw output in its own command substitution +# (so a magisk failure aborts the script), then strip the ``value=`` prefix +# with shell parameter expansion — no pipe (issue #239). +PREV_ZYGISK_ROW="$(magisk --sqlite "SELECT value FROM settings WHERE key='zygisk';")" +PREV_ZYGISK="${PREV_ZYGISK_ROW##*=}" magisk --sqlite "REPLACE INTO settings (key, value) VALUES ('zygisk', 1);" magisk --sqlite "REPLACE INTO settings (key, value) VALUES ('denylist', 1);" if [ "$PREV_ZYGISK" != "1" ]; then @@ -65,9 +74,14 @@ fi # container with denylist=1 but zygisk=0 and no root hiding — a # user-visible behaviour change that v0.3 had no detection for. # (T2 Agent 1 / Agent 2 F-9 / Agent 3 1.2.) -ZYGISK_VALUE="$(magisk --sqlite "SELECT value FROM settings WHERE key='zygisk';" | awk -F'=' '{print $NF}')" +# Same structure as PREV_ZYGISK above: read into its own command substitution +# (so a magisk failure aborts under ``set -eu`` rather than being masked into +# an empty value that misreports as "setting did not take"), then strip the +# ``value=`` prefix without a pipe (issue #239). +ZYGISK_ROW="$(magisk --sqlite "SELECT value FROM settings WHERE key='zygisk';")" +ZYGISK_VALUE="${ZYGISK_ROW##*=}" if [ "$ZYGISK_VALUE" != "1" ]; then - echo "[!] Magisk Zygisk setting did not take (got: '$ZYGISK_VALUE'). Aborting." + echo "[!] Magisk reports zygisk='$ZYGISK_VALUE' after the REPLACE INTO (expected '1'); the setting did not persist. Aborting." >&2 exit 1 fi diff --git a/docs/reference/config.md b/docs/reference/config.md index 7b710cc..7e9d711 100644 --- a/docs/reference/config.md +++ b/docs/reference/config.md @@ -110,7 +110,7 @@ GApps is split across two axes (issue #107): an **intent** that says *what you g |-------|------|---------|-------------| | `version` | int | `14` | Android version. Valid: `11`, `12`, `13`, `14`. | | `gapps` | enum | `minimal` | GApps **intent**: `none` (no Play Services), `minimal` (a slim Play Services), or `full` (the full suite). | -| `gapps_vendor` | enum | _(unset)_ | Optional **vendor** override: `litegapps`, `opengapps`, or `mindthegapps`. Unset lets the intent pick the vendor (`minimal` → LiteGApps, `full` → OpenGApps). Cannot be combined with `gapps: none`. | +| `gapps_vendor` | enum | _(unset)_ | Optional **vendor** override: `litegapps`, `opengapps`, or `mindthegapps`. Unset lets the intent pick the vendor (`minimal` → LiteGApps, `full` → OpenGApps). When set alongside `minimal`/`full`, the vendor wins and the intent is overridden (a one-line note is printed). Cannot be combined with `gapps: none`. | ```yaml android: @@ -169,7 +169,7 @@ Docker resource limits for the container. | `cpus` | float | `2.0` | CPU limit in fractional cores. | | `shared_mem` | string | `256m` | Shared memory size (`/dev/shm`, Docker `shm_size`). Docker size format. | | `mem_reservation` | string | none | Soft memory floor. Docker size format. Docker scheduler reserves this for the container but allows it to use more up to `mem`. | -| `memswap_limit` | string | none | Combined memory + swap cap. Docker size format. Unset → Docker's normal swap allowance applies; set it equal to `mem` to disable swap entirely and prevent swap storms. | +| `memswap_limit` | string | none | Combined memory + swap cap. Docker size format, plus Docker's documented sentinel `-1` for unlimited swap. Unset → Docker's normal swap allowance applies; set it equal to `mem` to disable swap entirely and prevent swap storms. | | `pids_limit` | int | `4096` | Maximum number of PIDs the container can spawn. | ```yaml @@ -183,7 +183,7 @@ resources: The old `resources.shm` field was renamed to `resources.shared_mem` for clarity. Loading a YAML with the legacy field raises a `ValidationError` pointing at this migration — see `CHANGELOG.md`. !!! note "Docker size format" - All string memory fields (`mem`, `shared_mem`, `mem_reservation`, `memswap_limit`) must use Docker's size format: a number optionally followed by a single suffix — `b`, `k`, `m`, `g`, or `t` (case-insensitive). Examples: `3g`, `512m`, `1.5G`. Values like `3gb` (two-letter suffix) fail at load time with a clear error rather than being silently misinterpreted at `docker compose up`. + All string memory fields (`mem`, `shared_mem`, `mem_reservation`, `memswap_limit`) must use Docker's size format: a number optionally followed by a single suffix — `b`, `k`, `m`, `g`, or `t` (case-insensitive). Examples: `3g`, `512m`, `1.5G`. Values like `3gb` (two-letter suffix) fail at load time with a clear error rather than being silently misinterpreted at `docker compose up`. `memswap_limit` additionally accepts Docker's documented `-1` sentinel (unlimited swap); the other size fields reject `-1` as a malformed size. !!! note "`resources.mem` vs `vm.memory_mib`" `resources.mem` is the Docker container memory cap, **authoritative for `binder: auto` / `host`** (redroid). For `binder: vm` the guest RAM is `vm.memory_mib` (the QEMU `-m`). Both knobs are deliberately kept — collapsing them into one field is deferred. Decision recorded in [#104](https://github.com/Xiddoc/Beetroot/issues/104). @@ -197,7 +197,7 @@ Frida server configuration. **Opt-in starting in v0.3** — omit the block entir | Field | Type | Default | Description | |-------|------|---------|-------------| | `version` | string | `"auto"` | Which `frida-server` release to stage. One of: **`auto`** (default) — match your host's installed `frida-tools` version so the staged server and the client you attach with agree on major+minor, falling back to `latest` when `frida-tools` isn't installed; **`latest`** — the current upstream release, resolved at download time; or a pinned **`major.minor.patch`** tag (e.g. `"16.4.10"`) for reproducibility. `auto` / `latest` are resolved to a concrete tag at staging time; a malformed pinned tag fails at config-load. | -| `sha256` | string \| null | `null` | Optional expected hex digest of the decompressed `frida-server` binary. When set, it's verified against the downloaded binary at download time (case-insensitive); a mismatch raises an error rather than staging a tampered binary. **Requires a pinned `version`** — a digest can't match the moving target `auto` / `latest` resolve to, so that combination is rejected at load. | +| `sha256` | string \| null | `null` | Optional expected hex digest of the decompressed `frida-server` binary. Validated at config-load time as a 64-character hex digest (case-insensitive) so a truncated or fat-fingered value fails fast instead of after a full download; when set, it's also verified against the downloaded binary at download time, and a mismatch raises an error rather than staging a tampered binary. **Requires a pinned `version`** — a digest can't match the moving target `auto` / `latest` resolve to, so that combination is rejected at load. | ```yaml # Opt in, tracking your host frida-tools (recommended): @@ -271,7 +271,7 @@ ports: - {service: metrics, guest: 9100} # arbitrary, auto-allocated host ``` -If you omit the block entirely (the default), the three well-known services are stride-allocated. The list is validated: duplicate `service` names, duplicate `guest` ports, and duplicate explicit `host` ports are all rejected at load time. If a resolved host port collides with another instance, `beetroot create` and `beetroot apply` exit with a clear error before staging: +If you omit the block entirely (the default), the three well-known services are stride-allocated. The list is validated: duplicate `service` names, duplicate `guest` ports, and duplicate explicit `host` ports are all rejected at load time. A well-known service (`adb` / `frida` / `frida_control`) must use its **canonical** guest port (`5555` / `27042` / `27043` — fixed by the redroid image); a mistyped guest there is rejected, since the published host port is derived from the service *name*, not the guest, and a wrong guest would forward to a dead port. A `frida:` block additionally requires both a `service: frida` **and** a `service: frida_control` mapping. If a resolved host port collides with another instance, `beetroot create` and `beetroot apply` exit with a clear error before staging: ``` error: port 5555 (adb) collides with instance 'alpha' (which also uses 5555). Pin or remove one. diff --git a/src/beetroot/backends/adb.py b/src/beetroot/backends/adb.py index 21d0701..37cce97 100644 --- a/src/beetroot/backends/adb.py +++ b/src/beetroot/backends/adb.py @@ -501,16 +501,23 @@ def auto_install_modules( # unavailable device aborts the batch. if not serial_is_available(self._config.serial): # The current module failed *because* the device went - # offline mid-install — record its failed row before - # aborting so it is accounted for in the report. + # offline during this module — record its failed row + # before aborting so it is accounted for in the report. # Otherwise it vanishes: its row was never appended to # ``results``, and ``skipped`` counts only the - # truly-un-attempted modules AFTER this position. + # truly-un-attempted modules AFTER this position. Keep the + # detail stage-neutral (the drop can happen at any point of + # the install, not specifically "mid-install") and retain + # the underlying adb error so the row is diagnosable. + adb_error = str(e).strip() + detail = "device went offline during this module" + if adb_error: + detail = f"{detail} — last adb error: {adb_error}" results.append( ModuleInstallResult( source=source, ok=False, - detail="device went offline mid-install", + detail=detail, ), ) skipped = len(sources) - index - 1 diff --git a/src/beetroot/builder.py b/src/beetroot/builder.py index 437f505..093d3e2 100644 --- a/src/beetroot/builder.py +++ b/src/beetroot/builder.py @@ -22,12 +22,15 @@ from __future__ import annotations import contextlib +import fcntl +import hashlib import os +import shlex import shutil import subprocess import tempfile import time -from collections.abc import Sequence +from collections.abc import Iterator, Sequence from pathlib import Path from typing import IO, Final, Protocol @@ -68,6 +71,38 @@ def _default_work_dir() -> Path: return paths.user_cache_dir("redroid-script") +@contextlib.contextmanager +def _clone_dir_lock(work: Path) -> Iterator[None]: + """ + Hold an exclusive ``fcntl.flock`` on a lockfile beside the clone dir. + + ``build_image`` mutates the shared per-user redroid-script clone dir in + place (``rm -rf`` + ``git clone`` + patch); without a lock two concurrent + ``beetroot build``s race on the same tree and corrupt each other (issue + #232). The lockfile sits at ``.lock`` (a sibling, so it survives the + ``rm -rf`` of ``work`` itself) and the exclusive lock serializes the + clone+patch steps while still letting the second build reuse the artifacts + once the first releases. + + Args: + work: The clone directory whose mutation is being serialized. + + Yields: + ``None`` while the lock is held; released (and the fd closed) on exit. + """ + lockfile = work.with_name(work.name + ".lock") + lockfile.parent.mkdir(parents=True, exist_ok=True) + fd = os.open(lockfile, os.O_CREAT | os.O_RDWR, 0o644) + try: + fcntl.flock(fd, fcntl.LOCK_EX) + try: + yield + finally: + fcntl.flock(fd, fcntl.LOCK_UN) + finally: + os.close(fd) + + def _default_build_context() -> Path: """ Return the default Docker build context for the Beetroot layer. @@ -266,44 +301,64 @@ def build_image( # noqa: PLR0913 # 7 keyword-only params; each is a distinct i ctx = _default_build_context() run = runner if runner is not None else DefaultRunner() + # Validate the (version, gapps, vendor) triple first — a contradictory + # config (e.g. ``none`` + a vendor) is a pure input error and must surface + # before any daemon/clone work. android = _android_for(android_version, gapps, gapps_vendor) tag = config.base_image_tag(android) vendor = config.resolve_gapps_vendor(android) gapps_flags = [] if vendor is None else GAPPS_VENDOR_FLAGS[vendor] - # Step 1: clone the patcher — skip when an identical clone already exists - # so a re-run doesn't discard already-downloaded Houdini / Magisk / - # GApps artifacts. Wipe and re-clone when the URL differs (different fork - # or a corrupted work dir). - with console.progress("Cloning redroid-script"): - if _clone_url_matches(work, redroid_script_url): - console.info(f"reusing existing clone at {work}") - else: - run.run(["rm", "-rf", str(work)]) - run.run( - ["git", "clone", "--depth", "1", redroid_script_url, str(work)], - ) + # Daemon preflight: the patcher and ``docker compose build`` both touch the + # Docker daemon, so fail fast with the same actionable remedy the bake path + # uses instead of a generic ``command failed (exit 1)`` mid-build (issue + # #193). + if not _docker_daemon_responsive(): + raise BootstrapError( + f"Docker daemon: `{settings.docker_bin} info` failed (daemon not running?) — " + "start the daemon (e.g. `sudo systemctl start docker`)" + ) - # Step 2: patch. ``-i`` installs Houdini, ``-m`` installs Magisk. - patcher_cmd: list[str] = [ - "uv", - "run", - "--with", - "requests", - "--with", - "tqdm", - "python", - "-W", - "ignore", - "redroid.py", - "-a", - f"{android_version}.0.0", - *gapps_flags, - "-i", - "-m", - ] - with console.progress("Patching base image (Magisk + Houdini + GApps)"): - run.run(patcher_cmd, cwd=work) + # Steps 1-2 (clone + patch) mutate the shared per-user clone dir in place + # (``rm -rf`` + ``git clone`` + patcher). Serialize concurrent builds on the + # same host under an exclusive flock so two ``beetroot build``s can't + # ``rm -rf``/clone over each other's tree (issue #232). The reuse path + # (clone URL matches) holds the lock too, so the ``.git/config`` read is + # synchronized with any racing clone. + with _clone_dir_lock(work): + # Step 1: clone the patcher — skip when an identical clone already + # exists so a re-run doesn't discard already-downloaded Houdini / + # Magisk / GApps artifacts. Wipe and re-clone when the URL differs + # (different fork or a corrupted work dir). + with console.progress("Cloning redroid-script"): + if _clone_url_matches(work, redroid_script_url): + console.info(f"reusing existing clone at {work}") + else: + run.run(["rm", "-rf", str(work)]) + run.run( + ["git", "clone", "--depth", "1", redroid_script_url, str(work)], + ) + + # Step 2: patch. ``-i`` installs Houdini, ``-m`` installs Magisk. + patcher_cmd: list[str] = [ + "uv", + "run", + "--with", + "requests", + "--with", + "tqdm", + "python", + "-W", + "ignore", + "redroid.py", + "-a", + f"{android_version}.0.0", + *gapps_flags, + "-i", + "-m", + ] + with console.progress("Patching base image (Magisk + Houdini + GApps)"): + run.run(patcher_cmd, cwd=work) # Step 3: build the Beetroot layer on top via the bundled compose template. # ``build_context`` is the directory Docker uses as the build context — it @@ -334,6 +389,12 @@ def build_image( # noqa: PLR0913 # 7 keyword-only params; each is a distinct i "BASE_IMAGE": tag, "BEETROOT_BUILD_CONTEXT": str(ctx), "INSTANCE_NAME": _BUILD_INSTANCE_NAME, + # docker/Dockerfile uses the BuildKit-only ``COPY --chmod``; force + # BuildKit so the build doesn't abort with "the --chmod option + # requires BuildKit" on a host whose default builder is the legacy + # one (issue #229). + "DOCKER_BUILDKIT": "1", + "COMPOSE_DOCKER_CLI_BUILD": "1", }, ) @@ -737,6 +798,36 @@ def from_env( ) +def _major_version_from_image(image: str) -> int: + """ + Parse the Android major version that leads a redroid image tag. + + The tag follows the last ``:`` and starts with the version (e.g. + ``redroid/redroid:13.0.0-latest`` → ``13``). Used so the rootfs version + marker records the version of the *actually-baked* image rather than the + requested ``android_version`` arg, which a ``REDROID_IMAGE`` override can + diverge from (issue #187). + + Args: + image: The redroid image reference (``repo:tag``). + + Returns: + The integer Android major version leading the tag. + + Raises: + BootstrapError: If the tag has no leading integer (malformed). + """ + tag = image.partition(":")[2] + leading = tag.split(".")[0] + try: + return int(leading) + except ValueError: + raise BootstrapError( + f"cannot parse an Android major version from redroid image {image!r}; " + "expected a tag like 'redroid/redroid:13.0.0-latest'" + ) from None + + class _RootfsAssembly: """Drives one rootfs build inside a scratch ``work`` directory.""" @@ -759,12 +850,16 @@ def __init__( self.dbin = self.docker_extract / "docker" def build(self) -> Path: - """Fetch the static bundle, assemble the tree, pack the image, write the marker.""" + """Fetch the static bundle, assemble the tree, write the marker, pack the image.""" self._fetch_static_bundle() self._build_tree() self._verify_guest_image_marker() - self._pack_image() + # Write the host-side version marker BEFORE packing the image, so a crash + # mid-pack can't leave a present-but-marker-less image that silently + # skips the #82 skew check (issue #234). A stale marker without an image + # is harmless — the next bake re-drives both halves. self._write_version_marker() + self._pack_image() return self.cfg.out_image def _guest_image_marker(self) -> Path: @@ -798,8 +893,15 @@ def _verify_guest_image_marker(self) -> None: def _write_version_marker(self) -> None: """Record the baked Android version beside the image for up/apply skew checks.""" marker = rootfs_version_marker(self.cfg.out_image) - console.info(f"recording baked Android version {self.cfg.android_version} → {marker}") - marker.write_text(f"{self.cfg.android_version}\n", encoding="utf-8") + # Source the recorded version from the image that was actually baked + # (``redroid_image``), not the requested ``android_version`` arg — a + # ``REDROID_IMAGE`` override can pin a different version, and the VM + # backend's #82 skew check trusts this marker as the guest's real + # version, so it must agree with the guest-side /etc/beetroot/redroid- + # image marker (issue #187). + baked = _major_version_from_image(self.cfg.redroid_image) + console.info(f"recording baked Android version {baked} → {marker}") + marker.write_text(f"{baked}\n", encoding="utf-8") def _fetch_static_bundle(self) -> None: console.info(f"fetching Docker static bundle {self.cfg.docker_version}") @@ -1082,6 +1184,13 @@ def _resolve_vm_dir(build_context: Path | None) -> Path: # matching prebuilt bzImage exists for the new version. KERNEL_VERSION: Final = "6.12.9" +# sha256 of the pinned ``linux-.tar.xz`` source tarball, taken +# from cdn.kernel.org's signed ``sha256sums.asc``. Verified before ``tar -xf`` +# so a tampered/MITM'd CDN tarball can't be compiled into a trusted bzImage +# (issue #184). Bump in lockstep with KERNEL_VERSION (and the workflows that +# fetch + verify the same tarball). +KERNEL_SOURCE_SHA256: Final = "87be0360df0931b340d2bac35161a548070fbc3a8c352c49e21e96666c26aeb4" + # Where the pinned kernel source tarball is fetched from when the prebuilt # fetch misses and the build falls back to a source compile (issue #74). The # major-version directory (``v6.x`` for 6.12.9) is derived from KERNEL_VERSION, @@ -1135,10 +1244,33 @@ def _fetch_kernel_source(run: SubprocessRunner, work: Path) -> Path: tarball = work / f"linux-{KERNEL_VERSION}.tar.xz" console.info(f"fetching kernel source {url}") run.run(["curl", "-fsSL", url, "-o", str(tarball)]) + # Verify the downloaded bytes against the pinned digest BEFORE extracting, + # so a tampered/MITM'd CDN tarball can't be unpacked + compiled into a + # trusted bzImage (issue #184). + _verify_kernel_source_digest(tarball) run.run(["tar", "-xf", str(tarball), "-C", str(work)]) return work / f"linux-{KERNEL_VERSION}" +def _verify_kernel_source_digest(tarball: Path) -> None: + """ + Verify a downloaded kernel source tarball against the pinned sha256. + + Args: + tarball: The on-disk ``linux-.tar.xz`` to hash. + + Raises: + BootstrapError: If the bytes' sha256 doesn't match + :data:`KERNEL_SOURCE_SHA256` (a tampered/MITM'd tarball). + """ + actual = hashlib.sha256(tarball.read_bytes()).hexdigest() + if actual != KERNEL_SOURCE_SHA256: + raise BootstrapError( + f"kernel source sha256 mismatch for {tarball.name}: expected " + f"{KERNEL_SOURCE_SHA256}, got {actual}; refusing to compile a tampered kernel" + ) + + class VmArtifacts(BaseModel): """ Host paths to the guest kernel + rootfs produced by ``beetroot build --vm-kernel``. @@ -1286,30 +1418,58 @@ def vm_bake_preflight(*, redroid_tar: Path | None = None) -> list[PreflightProbl requirement=tool, detail="not found on PATH", fix=f"apt-get install {pkg}" ) ) + # The local bake always spawns a staging dockerd (--data-root/--exec-root, + # cgroups, mounts, namespaces) even with REDROID_TAR set, so it needs root. + # Surface the privilege gap up front instead of letting the dockerd + # readiness loop time out after ~60s on an unprivileged host (issue #231). + if os.geteuid() != 0: + problems.append( + PreflightProblem( + requirement="root privilege", + detail=( + "the local rootfs bake spawns a staging dockerd, which requires " + "root (cgroups, mounts, namespaces)" + ), + fix=( + "re-run as root (or in a --privileged container), or fetch the " + "prebuilt rootfs instead of forcing a local bake" + ), + ) + ) # The host Docker CLI + a running daemon bake the redroid image — unless a - # pre-saved REDROID_TAR is supplied, which loads without pulling. - if redroid_tar is None: - if shutil.which(settings.docker_bin) is None: + # pre-saved REDROID_TAR is supplied, which loads without pulling. A set + # REDROID_TAR must still point at an existing file, else the bake passes + # --check then aborts mid-bake at ``docker load`` (issue #186). + if redroid_tar is not None: + if not redroid_tar.is_file(): problems.append( PreflightProblem( - requirement=settings.docker_bin, - detail="Docker CLI not found on PATH", - fix="install Docker Engine (apt-get install docker.io)", + requirement="REDROID_TAR", + detail=f"tarball not found at {redroid_tar}", + fix="point REDROID_TAR at an existing `docker save`d image tarball", ) ) - elif not _docker_daemon_responsive(): - problems.append( - PreflightProblem( - requirement="Docker daemon", - detail=f"`{settings.docker_bin} info` failed (daemon not running?)", - fix=( - "start the daemon (e.g. `sudo systemctl start docker`). If the " - "redroid pull then hits a Docker Hub rate limit, point REDROID_TAR " - "at a `docker save`d image tarball or use a registry mirror " - "(e.g. mirror.gcr.io)." - ), - ) + elif shutil.which(settings.docker_bin) is None: + problems.append( + PreflightProblem( + requirement=settings.docker_bin, + detail="Docker CLI not found on PATH", + fix="install Docker Engine (apt-get install docker.io)", ) + ) + elif not _docker_daemon_responsive(): + problems.append( + PreflightProblem( + requirement="Docker daemon", + detail=f"`{settings.docker_bin} info` failed (daemon not running?)", + fix=( + "start the daemon (e.g. `sudo systemctl start docker`). If the " + "redroid pull then hits a Docker Hub rate limit, point REDROID_TAR " + "at a `docker save`d image tarball or use a registry mirror " + "(e.g. mirror.gcr.io)." + ), + ) + ) return problems @@ -1504,6 +1664,11 @@ def build_vm_kernel( # noqa: PLR0913 # 9 keyword-only params; each is a distin tempfile.TemporaryDirectory(prefix="beetroot-kernel-src-") as src_work, ): source_tree = _fetch_kernel_source(run, Path(src_work)) + # Quote the build-context-derived paths so a checkout under a path + # with spaces/metacharacters doesn't word-split inside the sh -c + # (issue #208). + config_arg = shlex.quote(str(kernel_config)) + out_arg = shlex.quote(str(kernel_out)) run.run( [ "sh", @@ -1511,9 +1676,9 @@ def build_vm_kernel( # noqa: PLR0913 # 9 keyword-only params; each is a distin # merge the defconfig base with the vendored fragment, then # build, then drop the bzImage where the launcher expects it. "make defconfig && " - f"./scripts/kconfig/merge_config.sh -m .config {kernel_config} && " + f"./scripts/kconfig/merge_config.sh -m .config {config_arg} && " f'make olddefconfig && make {cc_prefix}-j"$(nproc)" bzImage && ' - f"cp arch/x86/boot/bzImage {kernel_out}", + f"cp arch/x86/boot/bzImage {out_arg}", ], cwd=source_tree, ) diff --git a/src/beetroot/compose.py b/src/beetroot/compose.py index 783f457..9388fc2 100644 --- a/src/beetroot/compose.py +++ b/src/beetroot/compose.py @@ -180,11 +180,18 @@ def logs(name: str, instance_root: Path, follow: bool = False) -> None: name: Instance name. instance_root: The instance directory. follow: If ``True``, stream logs continuously (``-f``). + + Raises: + ComposeError: If compose exits with a non-zero status in non-follow + mode. In follow mode a non-zero exit is tolerated, because + ``Ctrl-C``-ing out of the stream is the expected way to stop it. """ args = ["logs"] if follow: args.append("-f") - run(name, instance_root, args) + res = run(name, instance_root, args) + if not follow and res.returncode != 0: + raise ComposeError(f"`compose logs` failed for {name} (exit {res.returncode})") def ps_status(name: str, instance_root: Path) -> ComposeStatus: diff --git a/src/beetroot/config.py b/src/beetroot/config.py index 9bce832..0102bb8 100644 --- a/src/beetroot/config.py +++ b/src/beetroot/config.py @@ -61,18 +61,6 @@ # additive; old YAMLs without a frida block default to frida=None). _AUTO_BUMPABLE_API_VERSIONS: Final = frozenset({1, 2, 3, 4, 5, 6, 7}) -# Non-additive versions that require an explicit migration rather than a -# silent auto-bump. If a YAML pins one of these and a migration path exists, -# load_yaml raises a clear, actionable migration error naming the renamed / -# removed fields. -# -# api_version 3 → 4: ``stealth.denylist`` moved to ``magisk.denylist``. -# The ``stealth:`` key is now rejected with a migration hint pointing at -# CHANGELOG.md. YAMLs that merely omit ``api_version`` (default=current) -# are unaffected — only those that explicitly wrote ``api_version: 3`` and -# also used ``stealth:`` are covered by this path. -_MIGRATION_REQUIRED_VERSIONS: Final = frozenset[int]() # none yet beyond auto-bumpable - # The single source of truth for the supported Android major versions. To add a # new version, see "Adding a new Android version" in AGENTS.md — the human- # readable enumerations elsewhere ("11, 12, 13, or 14") are kept in sync by @@ -141,6 +129,40 @@ def validate_android_version(v: int) -> int: FRIDA_LATEST: Final = "latest" _FRIDA_SYMBOLIC_VERSIONS: Final = frozenset({FRIDA_AUTO, FRIDA_LATEST}) +# A SHA-256 digest is exactly 64 hex characters. Pre-validated at config-load +# time so a truncated / non-hex / fat-fingered ``frida.sha256`` fails fast +# instead of after a full download + decompress (the download-time compare in +# frida_download stays as defence-in-depth). Case-insensitive — Frida release +# manifests publish lowercase, but a pasted mixed-case digest is equally valid. +_SHA256_RE: Final = re.compile(r"^[0-9a-fA-F]{64}$") + + +def _check_sha256_shape(field_name: str, v: str | None) -> str | None: + """ + Reject a non-``None`` ``sha256`` that isn't a 64-character hex digest. + + ``None`` passes through unchanged (the field is optional). + + Args: + field_name: The dotted field name for the error message (e.g. + ``frida.sha256``). + v: The candidate digest, or ``None``. + + Returns: + ``v`` unchanged when it is ``None`` or a valid 64-char hex digest. + + Raises: + ValueError: If ``v`` is a non-``None`` value that doesn't match the + 64-character hex grammar. + """ + if v is None or _SHA256_RE.match(v): + return v + raise ValueError( + f"{field_name} {v!r} must be a 64-character hex SHA-256 digest " + "(e.g. a 64-char string of 0-9a-f). A truncated or non-hex digest " + "can never match the downloaded artifact." + ) + def is_pinned_frida_version(v: str) -> bool: """ @@ -173,6 +195,13 @@ def is_pinned_frida_version(v: str) -> bool: # cascades into the same load twice. CR #2 finding A2. _API_VERSION_BUMP_WARNED: set[Path] = set() +# Companion dedup set for the legacy ports-mapping migration note. The note is +# emitted from ``load_yaml`` (where the path is known), not from the mode-before +# validator (which has no path), so a fleet scan that calls ``load_yaml`` once +# per instance prints the note once per path instead of on every load. Cleared +# alongside ``_API_VERSION_BUMP_WARNED`` in the conftest autouse fixture. +_PORTS_MIGRATION_WARNED: set[Path] = set() + # Where DRM render nodes live; their presence means the host has a GPU the # privileged container can render through. Used by ``rendering: auto`` to pick @@ -274,7 +303,8 @@ class Resources(BaseModel): cpus: CPU cap as a float. shared_mem: Shared-memory size (Docker ``shm_size``). Docker size format. mem_reservation: Optional soft memory floor. Docker size format. - memswap_limit: Optional total memory + swap cap. Docker size format. + memswap_limit: Optional total memory + swap cap. Docker size format, + plus the documented sentinel ``-1`` (unlimited swap). pids_limit: Maximum number of PIDs the container can spawn. """ @@ -297,6 +327,12 @@ def _check_size_optional(cls, v: str | None, info: object) -> str | None: if v is None: return v field_name = getattr(info, "field_name", "field") + # Docker documents ``memswap_limit: -1`` as "unlimited swap" — a + # sentinel, not a size. Accept it only for memswap_limit; the other + # size fields (mem_reservation, and the required mem/shared_mem) have + # no such sentinel and still reject -1 as a malformed size. + if field_name == "memswap_limit" and v == "-1": + return v return _check_docker_size(f"resources.{field_name}", v) @model_validator(mode="before") @@ -351,6 +387,11 @@ def _check_version_shape(cls, v: str) -> str: "typos surface 404s at download time otherwise." ) + @field_validator("sha256") + @classmethod + def _check_sha256_shape(cls, v: str | None) -> str | None: + return _check_sha256_shape("frida.sha256", v) + @model_validator(mode="after") def _reject_sha256_with_symbolic_version(self) -> Self: if self.sha256 is not None and self.version in _FRIDA_SYMBOLIC_VERSIONS: @@ -550,6 +591,21 @@ def _reject_vendor_with_none(self) -> Self: ) return self + @model_validator(mode="after") + def _note_vendor_overrides_intent(self) -> Self: + # A pinned vendor wins outright in resolve_gapps_vendor, so the + # minimal/full intent's own default-vendor pick is silently discarded + # and gapps: minimal vs gapps: full collapse to the same base image. + # Warn so the override isn't a surprise (the gapps: none case is the + # contradiction handled by _reject_vendor_with_none above, not here). + if self.gapps_vendor is not None and self.gapps != "none": + console.note( + f"android.gapps_vendor: {self.gapps_vendor} is pinned, so it " + f"overrides the android.gapps: {self.gapps} intent — the base " + "image is selected by the vendor, not the intent." + ) + return self + def resolve_gapps_vendor(android: Android) -> GappsVendor | None: """ @@ -878,15 +934,14 @@ def _migrate_legacy_ports_mapping(cls, data: object) -> object: f"api_version: {SUPPORTED_API_VERSION}. See CHANGELOG.md." ) # Translate the well-known host overrides into the seeded list form. + # The user-facing migration note is emitted once-per-path from + # ``load_yaml`` (which has the path); this validator has no path and + # runs on every model_validate, so noting here would re-fire on every + # fleet-scan load (issue #202). migrated = [ {"service": service, "guest": guest, "host": raw_ports.get(service)} for service, guest in WELL_KNOWN_SERVICES.items() ] - console.note( - "migrated legacy ports mapping to the api_version " - f"{SUPPORTED_API_VERSION} list form; run 'beetroot apply' to rewrite " - "the YAML." - ) return {**data, "ports": migrated} @model_validator(mode="after") @@ -908,6 +963,22 @@ def _check_ports_distinct(self) -> Self: @model_validator(mode="after") def _check_required_addressing_services(self) -> Self: services = {m.service for m in self.ports if m.service is not None} + # A well-known service's guest port is fixed by the redroid image — + # the stride allocator derives the published host port from the + # *service name* (ports.resolve_ports), not the guest port, so a + # mistyped guest port would publish a host port forwarding to a dead + # guest port. Reject any well-known mapping whose guest isn't canonical + # (arbitrary/unlabelled mappings stay unconstrained — they name their + # own guest port). + for m in self.ports: + if m.service in WELL_KNOWN_SERVICES and m.guest != WELL_KNOWN_SERVICES[m.service]: + raise ValueError( + f"ports mapping for well-known service {m.service!r} has guest " + f"{m.guest}, but its canonical guest port is " + f"{WELL_KNOWN_SERVICES[m.service]} (fixed by the redroid image). " + "Drop the guest override, or use a non-well-known service name " + "for an arbitrary guest port." + ) if "adb" not in services: raise ValueError( "ports must include a mapping with service: adb — every backend " @@ -921,6 +992,14 @@ def _check_required_addressing_services(self) -> Self: "`- {service: frida, guest: 27042}` (host optional), or drop the " "frida: block." ) + if self.frida is not None and "frida_control" not in services: + raise ValueError( + "ports must include a mapping with service: frida_control when a " + "frida: block is configured — Frida's control channel (guest " + "27043) must be published alongside the data channel. Add " + "`- {service: frida_control, guest: 27043}` (host optional), or " + "drop the frida: block." + ) return self @model_validator(mode="after") @@ -947,9 +1026,9 @@ def inert_fields(cfg: InstanceConfig) -> list[str]: Only ``binder: vm`` has inert fields today: it boots an UNMODIFIED upstream redroid image (:func:`vm_redroid_image`) with no GApps / Magisk / Houdini / Frida layer, so the layered-image knobs (``android.gapps``, - ``magisk.denylist``) and the whole ``frida:`` block are inert, and only - adb is forwarded so arbitrary ``ports:`` mappings are dropped (issue #44/ - #108). ``binder: auto``/``host`` honour all of these → empty list. + ``magisk.denylist``, ``modules``) and the whole ``frida:`` block are inert, + and only adb is forwarded so arbitrary ``ports:`` mappings are dropped + (issue #44/#108). ``binder: auto``/``host`` honour all of these → empty list. Args: cfg: The fully-loaded instance config to inspect. @@ -976,6 +1055,11 @@ def inert_fields(cfg: InstanceConfig) -> list[str]: "magisk.denylist (the guest runs plain redroid with no Magisk, " "so the denylist is never applied)" ) + if cfg.modules: + inert.append( + "modules (the guest runs plain redroid with no Magisk, so flashed " + "modules are never applied)" + ) arbitrary = [m for m in cfg.ports if m.service not in WELL_KNOWN_SERVICES] if arbitrary: inert.append( @@ -1098,6 +1182,27 @@ def load_yaml(path: Path) -> InstanceConfig: ) _API_VERSION_BUMP_WARNED.add(resolved) raw["api_version"] = SUPPORTED_API_VERSION + # Emit the legacy ports-mapping migration note here (where the path is + # known) rather than in the mode-before validator (which has no path and + # re-runs on every model_validate), deduped per resolved path so a fleet + # scan that loads each YAML repeatedly only notes once (issue #202). A + # populated well-known mapping is what _migrate_legacy_ports_mapping + # translates; a non-well-known key raises a migration error there instead, + # so it must not also print this note. + if ( + isinstance(raw, dict) + and isinstance(raw.get("ports"), dict) + and raw["ports"] + and all(k in WELL_KNOWN_SERVICES for k in raw["ports"]) + ): + resolved_ports_path = path.resolve() + if resolved_ports_path not in _PORTS_MIGRATION_WARNED: + console.note( + "migrated legacy ports mapping to the api_version " + f"{SUPPORTED_API_VERSION} list form; run 'beetroot apply' to " + "rewrite the YAML." + ) + _PORTS_MIGRATION_WARNED.add(resolved_ports_path) return InstanceConfig.model_validate(raw) diff --git a/src/beetroot/console.py b/src/beetroot/console.py index 1fa18bc..d86392a 100644 --- a/src/beetroot/console.py +++ b/src/beetroot/console.py @@ -12,12 +12,17 @@ and break downstream parsers. TTY degradation is handled automatically by rich: when the underlying file is -not a TTY (e.g. a pipe), ``Console`` strips ANSI codes and renders plain text -so log files and CI output remain readable. Progress bars still render a single -summary line in non-TTY mode — they do not spam the log with carriage-return -sequences because rich's ``Progress``/``Console`` detects a non-interactive -(non-TTY) console and disables the ``Live`` refresh loop, emitting plain line -output instead. +not a TTY (e.g. a pipe), ``Console`` strips ANSI color codes. Stripping color +is *all* it does by default — box-drawing borders are plain UTF-8 (not ANSI) +and rich still clips wide table cells to its 80-column default width — so +piped output is not automatically machine-parseable. Callers that need a stable, +parseable stream should use a verb's ``--json`` mode, not scrape the human +table. The :func:`table` helper does render losslessly off-TTY (no borders, no +cell truncation), but that is a courtesy for log readability, not a contract. +Progress bars still render a single summary line in non-TTY mode — they do not +spam the log with carriage-return sequences because rich's ``Progress``/ +``Console`` detects a non-interactive (non-TTY) console and disables the +``Live`` refresh loop, emitting plain line output instead. """ from __future__ import annotations @@ -244,19 +249,41 @@ def table(columns: Sequence[str], rows: Sequence[Sequence[str]]) -> None: """ Render a rich ``Table`` with the given column headers and rows to stdout. - Primary results (like ``beetroot ls`` or ``beetroot status``) go to - stdout via the stdout console so they can be captured by shell pipelines. - Rich strips decoration automatically when stdout is not a TTY. + Primary results (like ``beetroot ls`` or ``beetroot modes``) go to stdout + via the stdout console. On a TTY the table is fully decorated (borders, + color). Off a TTY, rich's defaults would clip wide cells to 80 columns and + still draw UTF-8 box borders, so a piped ``ls`` could silently truncate an + ADB endpoint or an instance path. To keep piped output lossless this branch + drops the borders, folds (never truncates) cells, and prints at a width wide + enough for the longest line — so every cell survives verbatim. This is a + readability courtesy, not a parsing contract: machine consumers should use a + verb's ``--json`` mode. Args: columns: Sequence of column header strings. rows: Sequence of rows; each row is a sequence of cell strings whose length must match ``columns``. """ - t = Table(*columns) + if _stdout_console.is_terminal: + t = Table(*columns) + for row in rows: + t.add_row(*row) + _stdout_console.print(t) + return + + t = Table(box=None, pad_edge=False) + for col in columns: + t.add_column(col, overflow="fold", no_wrap=False) for row in rows: t.add_row(*row) - _stdout_console.print(t) + # Width the render to the longest content line so rich never falls back to + # its 80-column default and clips a cell with an ellipsis. + max_cell = max( + (len(cell) for line in (columns, *rows) for cell in line), + default=0, + ) + width = max(max_cell * len(columns) + len(columns), 80) + _stdout_console.print(t, width=width, crop=False) # --------------------------------------------------------------------------- diff --git a/src/beetroot/frida_download.py b/src/beetroot/frida_download.py index c0a9764..0fed7ca 100644 --- a/src/beetroot/frida_download.py +++ b/src/beetroot/frida_download.py @@ -13,11 +13,14 @@ import hashlib import lzma +import os import shutil import subprocess +import tempfile import urllib.error import urllib.request from pathlib import Path +from typing import Final from . import config, console, paths from .settings import settings @@ -30,6 +33,11 @@ _CHUNK_SIZE = 1 << 16 # 64 KiB per read; balances memory and progress granularity +# Ceiling on the *decompressed* frida-server output. A real frida-server is +# tens of MB; this generous 512 MiB cap turns a corrupt or zip-bomb ``.xz`` into +# a clean ``FridaFetchError`` instead of an OOM kill of the whole process (#228). +_MAX_DECOMPRESSED_BYTES: Final[int] = 512 * 1024 * 1024 + class FridaFetchError(RuntimeError): """ @@ -202,36 +210,61 @@ def download(version: str, *, expected_sha256: str | None = None) -> Path: out.parent.mkdir(parents=True, exist_ok=True) url = release_url(version) + # Stage into a process-unique temp on the cache filesystem so two concurrent + # fetches of the same version can't write a shared fixed ``.tmp`` and publish + # a cross-contaminated binary via the atomic rename (#185). The compressed + # payload is fed chunk-by-chunk into an incremental LZMA decompressor whose + # output streams straight to the temp file — neither the whole compressed nor + # the whole decompressed payload is ever resident (#227) — and the running + # decompressed total is capped to guard against a zip-bomb ``.xz`` (#228). + fd, tmp_name = tempfile.mkstemp(dir=out.parent, suffix=".tmp") + tmp = Path(tmp_name) try: - with urllib.request.urlopen(url, timeout=settings.http_timeout) as resp: # noqa: S310 # URL built from a pinned GitHub release path; scheme is https + decompressor = lzma.LZMADecompressor() + decompressed_total = 0 + with ( + os.fdopen(fd, "wb") as handle, + urllib.request.urlopen(url, timeout=settings.http_timeout) as resp, # noqa: S310 # URL built from a pinned GitHub release path; scheme is https + ): raw_length = resp.headers.get("Content-Length") total: float | None = float(raw_length) if raw_length else None - chunks: list[bytes] = [] with console.progress(f"Fetching frida-server {version}", total=total) as bar: while True: chunk = resp.read(_CHUNK_SIZE) if not chunk: break - chunks.append(chunk) + piece = decompressor.decompress(chunk) + decompressed_total += len(piece) + if decompressed_total > _MAX_DECOMPRESSED_BYTES: + raise FridaFetchError( + f"frida-server {url} decompressed past the " + f"{_MAX_DECOMPRESSED_BYTES}-byte ceiling — the download may be " + "corrupt or a zip bomb; refusing to continue" + ) + handle.write(piece) bar.advance(len(chunk)) - compressed = b"".join(chunks) + tmp.chmod(0o755) + tmp.replace(out) except urllib.error.HTTPError as e: + tmp.unlink(missing_ok=True) raise FridaFetchError(f"download failed: HTTP {e.code} fetching {url}") from e except TimeoutError as e: + tmp.unlink(missing_ok=True) raise FridaFetchError(f"download timed out after {settings.http_timeout}s: {url}") from e except urllib.error.URLError as e: + tmp.unlink(missing_ok=True) raise FridaFetchError(f"download failed: cannot reach {url}: {e.reason}") from e - try: - decompressed = lzma.decompress(compressed) except lzma.LZMAError as e: + tmp.unlink(missing_ok=True) raise FridaFetchError( f"decompression failed for frida-server {url}: the download may be " "corrupt or truncated — delete the partial cache and retry" ) from e - tmp = out.with_suffix(".tmp") - tmp.write_bytes(decompressed) - tmp.chmod(0o755) - tmp.replace(out) + except BaseException: + # Any other failure (the ceiling guard above, an interrupt, a disk-full + # write) must not orphan the temp in the user-global cache. + tmp.unlink(missing_ok=True) + raise _check_sha256(out, expected_sha256) return out diff --git a/src/beetroot/kernel_download.py b/src/beetroot/kernel_download.py index 9283fa5..dc19763 100644 --- a/src/beetroot/kernel_download.py +++ b/src/beetroot/kernel_download.py @@ -34,6 +34,8 @@ from __future__ import annotations import hashlib +import os +import tempfile import urllib.error import urllib.request from pathlib import Path @@ -189,7 +191,17 @@ def fetch_prebuilt(*, version: str, fingerprint: str, out_path: Path) -> Path: f"sha256 mismatch for {url}: expected {expected.lower()}, got {actual.lower()}" ) out_path.parent.mkdir(parents=True, exist_ok=True) - tmp = out_path.with_suffix(".tmp") - tmp.write_bytes(payload) - tmp.replace(out_path) + # Stage into a process-unique temp on the destination filesystem so two + # concurrent fetches of the same kernel can't write a shared fixed ``.tmp`` + # and publish a cross-contaminated bzImage via the atomic rename (#185). + fd, tmp_name = tempfile.mkstemp(dir=out_path.parent, suffix=".tmp") + tmp = Path(tmp_name) + try: + with os.fdopen(fd, "wb") as handle: + handle.write(payload) + tmp.replace(out_path) + except BaseException: + # A mid-write crash must not orphan the temp beside the destination. + tmp.unlink(missing_ok=True) + raise return out_path diff --git a/src/beetroot/modules_download.py b/src/beetroot/modules_download.py index 1369df0..eae0230 100644 --- a/src/beetroot/modules_download.py +++ b/src/beetroot/modules_download.py @@ -16,8 +16,11 @@ from __future__ import annotations import hashlib +import os import shutil +import tempfile import urllib.error +import urllib.parse import urllib.request from pathlib import Path @@ -67,9 +70,14 @@ def _module_cache_dir() -> Path: def _filename_from_url(url: str) -> str: """ - Return the basename of the URL path, or ``module.zip`` if empty. + Return the basename of the URL *path*, or ``module.zip`` if empty. + + Derived from the path component only (via :func:`urllib.parse.urlsplit`) so a + trailing ``?query`` or ``#fragment`` never leaks into the staged filename — + otherwise a staged ``m.zip?v=2`` would never match the ``*.zip`` flash glob + in ``flash-modules.sh`` and the module would be silently skipped (#168). """ - return url.rsplit("/", 1)[-1] or "module.zip" + return urllib.parse.urlsplit(url).path.rsplit("/", 1)[-1] or "module.zip" def _cache_path_for_url(url: str) -> Path: @@ -106,31 +114,45 @@ def _fetch_url(url: str) -> Path: return cache cache.parent.mkdir(parents=True, exist_ok=True) filename = _filename_from_url(url) + # Stage into a process-unique temp on the cache filesystem so two concurrent + # fetches of the same URL can't write the same fixed ``.tmp`` and publish a + # cross-contaminated zip via the atomic rename (#185). The payload streams + # straight to disk chunk-by-chunk rather than buffering the whole zip in RAM + # (#227), mirroring ``rootfs_download``. + fd, tmp_name = tempfile.mkstemp(dir=cache.parent, suffix=".tmp") + tmp = Path(tmp_name) try: - with urllib.request.urlopen(url, timeout=settings.http_timeout) as resp: # noqa: S310 # scheme validated by Module pydantic model + _fetch_url allowlist + with ( + os.fdopen(fd, "wb") as out, + urllib.request.urlopen(url, timeout=settings.http_timeout) as resp, # noqa: S310 # scheme validated by Module pydantic model + _fetch_url allowlist + ): raw_length = resp.headers.get("Content-Length") total: float | None = float(raw_length) if raw_length else None - chunks: list[bytes] = [] with console.progress(f"Fetching module {filename}", total=total) as bar: while True: chunk = resp.read(_CHUNK_SIZE) if not chunk: break - chunks.append(chunk) + out.write(chunk) bar.advance(len(chunk)) - data = b"".join(chunks) + tmp.replace(cache) except urllib.error.HTTPError as e: + tmp.unlink(missing_ok=True) raise ModuleFetchError( f"download failed: HTTP {e.code} fetching {url}; " "verify the URL is current (the upstream release may have moved)" ) from e except TimeoutError as e: + tmp.unlink(missing_ok=True) raise ModuleFetchError(f"download timed out after {settings.http_timeout}s: {url}") from e except urllib.error.URLError as e: + tmp.unlink(missing_ok=True) raise ModuleFetchError(f"download failed: cannot reach {url}: {e.reason}") from e - tmp = cache.with_suffix(".tmp") - tmp.write_bytes(data) - tmp.replace(cache) + except BaseException: + # A mid-write crash (interrupt, disk-full) must not orphan the temp in + # the user-global cache; the successful path already renamed it away. + tmp.unlink(missing_ok=True) + raise return cache diff --git a/src/beetroot/paths.py b/src/beetroot/paths.py index 527e799..18c7fc2 100644 --- a/src/beetroot/paths.py +++ b/src/beetroot/paths.py @@ -18,6 +18,8 @@ from __future__ import annotations import importlib.resources +import os +import tempfile from pathlib import Path from platformdirs import user_cache_path, user_config_path @@ -164,6 +166,31 @@ def bundled_vm_dir() -> Path: return _materialise_bundled_vm_dir() +def _atomic_write_bytes(target: Path, contents: bytes) -> None: + """ + Write ``contents`` to ``target`` via a same-directory temp file + rename. + + The cache these helpers write to is process-global and shared with + concurrent wheel-installed invocations that hand the same path to + ``docker compose -f``. An in-place ``write_bytes`` truncates first, so a + concurrent reader can observe an empty / half-written file. Staging to a + sibling temp file and ``os.replace``-ing it in is atomic on the same + filesystem, so a reader always sees either the old bytes or the full new + bytes — never a truncated view. + """ + fd, tmp_name = tempfile.mkstemp(dir=target.parent, prefix=target.name, suffix=".tmp") + tmp = Path(tmp_name) + try: + with os.fdopen(fd, "wb") as fh: + fh.write(contents) + # ``os.replace`` is the atomic-rename primitive; ``Path.replace`` wraps + # it but obscures the intent (and the spy in the tests targets it here). + os.replace(tmp, target) # noqa: PTH105 + except BaseException: + tmp.unlink(missing_ok=True) + raise + + _BUNDLED_VM_DIR_CACHE: Path | None = None @@ -195,7 +222,7 @@ def _materialise_bundled_vm_dir() -> Path: contents = pkg.joinpath(name).read_bytes() dst = target / name if not dst.exists() or dst.read_bytes() != contents: - dst.write_bytes(contents) + _atomic_write_bytes(dst, contents) _BUNDLED_VM_DIR_CACHE = target return _BUNDLED_VM_DIR_CACHE @@ -235,7 +262,7 @@ def _materialise_bundled_compose() -> Path: cache_target = user_cache_dir("templates") / "compose.yaml" cache_target.parent.mkdir(parents=True, exist_ok=True) if not cache_target.exists() or cache_target.read_bytes() != contents: - cache_target.write_bytes(contents) + _atomic_write_bytes(cache_target, contents) _BUNDLED_COMPOSE_CACHE = cache_target return _BUNDLED_COMPOSE_CACHE @@ -252,7 +279,12 @@ def user_config_dir() -> Path: Returns: Absolute path to the per-user Beetroot config dir. """ - return user_config_path(_APP_NAME) + # platformdirs honours a relative ``$XDG_CONFIG_HOME`` literally, but the + # XDG spec says relative values are invalid and must be ignored — and a + # cwd-relative registry would fragment the user-global registry across + # working directories. Fall back to the spec default when not absolute. + p = user_config_path(_APP_NAME) + return p if p.is_absolute() else (Path.home() / ".config" / _APP_NAME) def user_registry_file() -> Path: @@ -288,4 +320,8 @@ def user_cache_dir(subdir: str) -> Path: Returns: Absolute path to the requested cache subdirectory. """ - return user_cache_path(_APP_NAME) / subdir + # As with ``user_config_dir``: a relative ``$XDG_CACHE_HOME`` is invalid + # per the XDG spec, so ignore it and fall back to ``~/.cache``. + c = user_cache_path(_APP_NAME) + base = c if c.is_absolute() else (Path.home() / ".cache" / _APP_NAME) + return base / subdir diff --git a/src/beetroot/rootfs_download.py b/src/beetroot/rootfs_download.py index f28799e..422e2fa 100644 --- a/src/beetroot/rootfs_download.py +++ b/src/beetroot/rootfs_download.py @@ -325,7 +325,11 @@ def fetch_prebuilt( zstandard.ZstdDecompressor().copy_stream(src, dst) except zstandard.ZstdError as e: raise RootfsFetchError(f"corrupt/truncated zstd payload for {url}: {e}") from e + # Write the version marker BEFORE the image is renamed into place, so an + # interrupted fetch can never leave a present-but-marker-less image that + # silently skips the #82 skew check. A stale marker without an image is + # harmless — the next fetch re-drives both halves (issue #234). + marker = out_image.with_name(out_image.name + _ROOTFS_VERSION_MARKER_SUFFIX) + marker.write_text(f"{android_version}\n", encoding="utf-8") tmp.replace(out_image) - marker = out_image.with_name(out_image.name + _ROOTFS_VERSION_MARKER_SUFFIX) - marker.write_text(f"{android_version}\n", encoding="utf-8") return out_image diff --git a/src/beetroot/snapshot.py b/src/beetroot/snapshot.py index 856408b..646e9cc 100644 --- a/src/beetroot/snapshot.py +++ b/src/beetroot/snapshot.py @@ -319,9 +319,13 @@ def _add_instance_tree(tar: tarfile.TarFile, instance_root: Path, final_dest: Pa into the cwd, which is normally the instance dir, so the just-created (still-open, partially-flushed) archive would otherwise be packed into itself as a phantom ``./.tar.zst`` member and re-extracted on - restore. The match is by resolved absolute path, so a destination - outside ``instance_root`` is unaffected and a same-named file elsewhere - in the tree is not wrongly excluded. + restore. The exclusion applies at ANY directory depth (via the + ``tar.add`` ``filter`` callback), so a dest nested in a subdir such as + ``data/.tar.zst`` — the CLI default resolved against a cwd inside + ``data/`` (#173) — is dropped too, not just a top-level dest. The match + is by resolved absolute path, so a destination outside ``instance_root`` + is unaffected and a same-named file elsewhere in the tree is not wrongly + excluded. Args: tar: The open tar stream to append members to. @@ -330,12 +334,21 @@ def _add_instance_tree(tar: tarfile.TarFile, instance_root: Path, final_dest: Pa falls inside ``instance_root``. """ dest_resolved = final_dest.resolve() + + def _skip_dest(info: tarfile.TarInfo) -> tarfile.TarInfo | None: + # ``info.name`` is the arcname (e.g. ``./data/alpha.tar.zst``); + # resolving it against ``instance_root`` recovers the on-disk path so + # the still-open destination archive is dropped at any depth (#173). + if (instance_root / info.name).resolve() == dest_resolved: + return None + return info + for entry in sorted(instance_root.iterdir()): if entry.name in _EXCLUDED_TOP_LEVEL: continue if entry.resolve() == dest_resolved: continue - tar.add(entry, arcname=f"./{entry.name}", recursive=True) + tar.add(entry, arcname=f"./{entry.name}", recursive=True, filter=_skip_dest) def _add_manifest(tar: tarfile.TarFile, manifest: Manifest) -> None: @@ -435,8 +448,12 @@ def _prepare_destination(target: Path, *, force: bool) -> None: # ``target`` is an existing regular file; iterdir() blows up. # Re-raise as a SnapshotError so callers see a consistent type. raise SnapshotError(f"{target} exists and is a file, not a directory") from None - if not occupied: - return + # The cross-instance overlap loop runs UNCONDITIONALLY — before the + # ``occupied`` early-return — because the predicate below is a pure + # path comparison that is just as valid for a not-yet-existent target. + # Gating it behind ``occupied`` (#172) let a restore into a brand-new + # nested path like ``/sub`` slip past the guard and + # register a nested instance inside a registered peer with no --force. for other_name, meta in registry.list_instances().items(): # Both redroid and vm backends are directory-backed and carry an # ``absolute_path``; an adb row carries a serial, not a path, so @@ -458,6 +475,10 @@ def _prepare_destination(target: Path, *, force: bool) -> None: f"with --force). 'beetroot destroy {other_name}' first, " "or pick a different --path." ) + # Overlap guard cleared: a non-existent or empty target needs no + # further clearing, so return before the force/overwrite tail. + if not occupied: + return if not force: raise SnapshotError( f"{target} already exists and is non-empty; " @@ -583,6 +604,17 @@ def restore( # the try block, so a corrupt member left a partially-extracted # tree behind that the user had to clean up manually. _extract_archive_into(archive, target) + # #171: reconcile the EXTRACTED beetroot.yaml's binder mode. The + # registry row is always written as RedroidBackendConfig above, but + # the archived config is the source of truth for the backend — a + # ``binder: vm`` archive (e.g. an unapplied edit on the source) + # would otherwise restore as a redroid row and dispatch the wrong + # backend silently. Snapshots are redroid-only (#128), so refuse it + # here, inside the rollback try/except so the half-registered row + + # extracted tree are torn down. Mirrors the source-side gate in + # ``_find_registry_entry``. + if config.load_yaml(paths.instance_yaml(target)).binder == "vm": + raise SnapshotError(unsupported_backend_message("restore", dest_name, "vm")) # T4: replay the snapshot's path_layout into the new registry # entry's stealth_paths slot. v0.4 manifests carry ``{}`` so # this is a structural no-op today; a v0.6 snapshot carrying diff --git a/tests/test_adb_device.py b/tests/test_adb_device.py index 2469e76..a956eb4 100644 --- a/tests/test_adb_device.py +++ b/tests/test_adb_device.py @@ -912,7 +912,7 @@ def test_mid_batch_offline_aborts_with_partial_results( # longer lists the serial → the batch aborts with the friendly # offline error carrying the first module's ok row AND the second # module's failed row (it failed *because* the device went - # offline mid-install, so it must be accounted for). The third is + # offline mid-batch, so it must be accounted for). The third is # never pushed and is reflected only in the skipped count. monkeypatch.setattr(shutil, "which", lambda name: f"/usr/bin/{name}") first = tmp_path / "First.zip" @@ -935,6 +935,13 @@ def test_mid_batch_offline_aborts_with_partial_results( (str(first), True), (str(second), False), ] + # #223: the failed row's detail is stage-neutral (not the old hardcoded + # "mid-install") and retains the underlying adb error for diagnosis. + failed_detail = exc_info.value.results[1].detail + assert "mid-install" not in failed_detail + assert failed_detail.startswith("device went offline during this module") + assert "last adb error:" in failed_detail + assert "device offline" in failed_detail # The failed second push, then the connectivity re-probe, are the # last two adb calls — the third module is never pushed. assert captured[-2:] == [ diff --git a/tests/test_bugfix_config_validation.py b/tests/test_bugfix_config_validation.py new file mode 100644 index 0000000..8443d95 --- /dev/null +++ b/tests/test_bugfix_config_validation.py @@ -0,0 +1,218 @@ +"""Bugfix regression tests for the config-denylist sweep. + +Covers the config.py / load_yaml fixes shipped in this sweep: + +* #194 — ``frida.sha256`` is validated as 64-char hex at config-load time. +* #195 — a well-known ``ports:`` mapping with a non-canonical guest port is + rejected, and ``frida_control`` is required alongside ``frida``. +* #202 — the legacy ports-mapping migration note is deduped per resolved path + so a fleet scan that re-loads a YAML only notes once. +* #215 — Docker's documented ``memswap_limit: -1`` (unlimited swap) is accepted + while ``-1`` is still rejected for the other size fields. +* #220 — a pinned ``gapps_vendor`` that overrides the ``minimal``/``full`` + intent emits a one-line note. +* #242 — the dead ``_MIGRATION_REQUIRED_VERSIONS`` constant stays removed. +* #200 — the ``binder: vm`` inert-config advisory flags a non-empty + ``modules:`` list the Magisk-less guest can never flash. +""" + +from __future__ import annotations + +from collections.abc import Iterator +from pathlib import Path + +import pytest +import yaml +from pydantic import ValidationError + +from beetroot import config +from beetroot.config import ( + Android, + Frida, + InstanceConfig, + Module, + PortMapping, + Resources, + inert_fields, + load_yaml, + render_env, +) + + +@pytest.fixture(autouse=True) +def _reset_ports_migration_dedup() -> Iterator[None]: + """Clear the per-path ports-migration dedup set around each test. + + Mirrors conftest's ``_reset_api_version_warning_dedup`` for the companion + ``_PORTS_MIGRATION_WARNED`` set so an order-shuffled run can't carry a + populated path between tests (issue #202). + """ + config._PORTS_MIGRATION_WARNED.clear() + yield + config._PORTS_MIGRATION_WARNED.clear() + + +class TestFridaSha256HexValidation: + """#194: ``frida.sha256`` must be a 64-character hex digest.""" + + @pytest.mark.parametrize( + "bad", + ["abc", "g" * 64, "not a hash", "", "a" * 63, "a" * 65], + ) + def test_non_hex_or_wrong_length_rejected(self, bad: str) -> None: + with pytest.raises(ValidationError, match="64-character hex SHA-256"): + Frida(version="16.4.10", sha256=bad) + + def test_lowercase_64_hex_accepted(self) -> None: + digest = "0123456789abcdef" * 4 + assert Frida(version="16.4.10", sha256=digest).sha256 == digest + + def test_mixed_case_64_hex_accepted(self) -> None: + digest = "0123456789ABCDEFabcdef0123456789ABCDEFab0123456789abcdef01234567" + assert len(digest) == 64 + assert Frida(version="16.4.10", sha256=digest).sha256 == digest + + def test_none_passes_through(self) -> None: + assert Frida(version="16.4.10").sha256 is None + + +class TestMemswapLimitUnlimited: + """#215: ``memswap_limit: -1`` (unlimited swap) is accepted.""" + + def test_minus_one_accepted_for_memswap(self) -> None: + assert Resources(memswap_limit="-1").memswap_limit == "-1" + + def test_minus_one_rendered_into_env(self) -> None: + cfg = InstanceConfig(resources=Resources(memswap_limit="-1")) + assert "MEMSWAP_LIMIT=-1\n" in render_env("alpha", cfg) + + def test_minus_one_rejected_for_mem_reservation(self) -> None: + # The -1 sentinel is memswap-only; mem_reservation has no such + # documented sentinel and must still reject it as a malformed size. + with pytest.raises(ValidationError, match="Docker size format"): + Resources(mem_reservation="-1") + + def test_minus_one_rejected_for_mem(self) -> None: + with pytest.raises(ValidationError, match="Docker size format"): + Resources(mem="-1") + + +class TestGappsVendorOverrideNote: + """#220: a pinned vendor overriding the intent emits a note.""" + + @pytest.mark.parametrize("intent", ["minimal", "full"]) + def test_note_fires_when_vendor_overrides_intent( + self, intent: str, capsys: pytest.CaptureFixture[str] + ) -> None: + Android(gapps=intent, gapps_vendor="opengapps") # type: ignore[arg-type] + err = capsys.readouterr().err + assert "overrides the android.gapps" in err + + def test_no_note_when_vendor_unset(self, capsys: pytest.CaptureFixture[str]) -> None: + Android(gapps="minimal") + assert "overrides the android.gapps" not in capsys.readouterr().err + + def test_no_note_for_gapps_none(self, capsys: pytest.CaptureFixture[str]) -> None: + # gapps: none + a vendor is the contradiction rejected outright, so it + # never reaches the override note; gapps: none with no vendor is clean. + Android(gapps="none") + assert "overrides the android.gapps" not in capsys.readouterr().err + + +class TestWellKnownGuestPortValidation: + """#195: well-known mappings must use their canonical guest port.""" + + def test_non_canonical_adb_guest_rejected(self) -> None: + with pytest.raises(ValidationError, match="canonical guest port is 5555"): + InstanceConfig( + ports=[ + PortMapping(service="adb", guest=9999), + PortMapping(service="frida", guest=27042), + PortMapping(service="frida_control", guest=27043), + ] + ) + + def test_canonical_adb_guest_accepted(self) -> None: + cfg = InstanceConfig(ports=[PortMapping(service="adb", guest=5555)]) + assert any(m.service == "adb" and m.guest == 5555 for m in cfg.ports) + + def test_arbitrary_service_guest_unconstrained(self) -> None: + # A non-well-known mapping names its own guest port — no canonical + # constraint applies. + cfg = InstanceConfig( + ports=[ + PortMapping(service="adb", guest=5555), + PortMapping(service="telemetry", guest=9999), + ] + ) + assert any(m.service == "telemetry" and m.guest == 9999 for m in cfg.ports) + + def test_frida_control_required_with_frida_block(self) -> None: + with pytest.raises(ValidationError, match="service: frida_control"): + InstanceConfig( + frida=Frida(version="16.4.10"), + ports=[ + PortMapping(service="adb", guest=5555), + PortMapping(service="frida", guest=27042), + ], + ) + + def test_default_config_validates_clean(self) -> None: + # The default seeds all three well-known services at canonical ports. + InstanceConfig(frida=Frida(version="16.4.10")) + + +class TestLegacyPortsMigrationNoteDedup: + """#202: the migration note fires once per resolved path.""" + + def test_note_fires_once_across_two_loads( + self, tmp_path: Path, capsys: pytest.CaptureFixture[str] + ) -> None: + p = tmp_path / "cfg.yaml" + p.write_text(yaml.safe_dump({"api_version": 7, "ports": {"adb": 9000}})) + load_yaml(p) + load_yaml(p) + note_lines = [ + line + for line in capsys.readouterr().err.splitlines() + if "migrated legacy ports mapping" in line + ] + assert len(note_lines) == 1, note_lines + + def test_unknown_key_does_not_emit_migration_note( + self, tmp_path: Path, capsys: pytest.CaptureFixture[str] + ) -> None: + # A non-well-known key raises the migration error instead of the note. + p = tmp_path / "cfg.yaml" + p.write_text(yaml.safe_dump({"api_version": 7, "ports": {"telemetry": 9000}})) + with pytest.raises(ValidationError, match="non-well-known"): + load_yaml(p) + assert "migrated legacy ports mapping" not in capsys.readouterr().err + + +class TestDeadMigrationConstantRemoved: + """#242: the never-read constant must not silently return.""" + + def test_constant_is_gone(self) -> None: + assert not hasattr(config, "_MIGRATION_REQUIRED_VERSIONS") + + +class TestVmInertModules: + """#200: a non-empty modules list is inert under binder: vm.""" + + def test_modules_flagged_inert_on_vm(self) -> None: + cfg = InstanceConfig( + binder="vm", + modules=[Module(url="https://example.com/mod.zip")], + ) + entries = inert_fields(cfg) + assert any(e.startswith("modules") for e in entries), entries + + def test_no_modules_no_entry_on_vm(self) -> None: + cfg = InstanceConfig(binder="vm") + assert not any(e.startswith("modules") for e in inert_fields(cfg)) + + def test_modules_honoured_on_redroid(self) -> None: + # binder != vm honours modules → no inert entry at all. + cfg = InstanceConfig(modules=[Module(url="https://example.com/mod.zip")]) + assert inert_fields(cfg) == [] diff --git a/tests/test_bugfix_download_temp_streaming.py b/tests/test_bugfix_download_temp_streaming.py new file mode 100644 index 0000000..f80ef00 --- /dev/null +++ b/tests/test_bugfix_download_temp_streaming.py @@ -0,0 +1,258 @@ +"""Regression tests for #185, #227, #228 — download staging, streaming, and bounds. + +#185: frida/module/kernel downloads stage into a *process-unique* temp file (via +``tempfile.mkstemp``) before the atomic rename, so two concurrent fetches of the +same artifact can't write a shared fixed ``.tmp`` and publish a corrupt, +cross-contaminated file into the user-global cache. + +#227: frida and module downloads stream chunk-by-chunk to the open temp handle +(frida decompresses incrementally) instead of buffering the whole payload in RAM. + +#228: frida ``.xz`` decompression is bounded by a generous output ceiling and +raises ``FridaFetchError`` instead of OOM-ing on a corrupt / zip-bomb payload. +""" + +from __future__ import annotations + +import hashlib +import lzma +import stat +import tempfile +import urllib.error +from pathlib import Path +from typing import Protocol +from unittest.mock import MagicMock, patch + +import pytest + +from beetroot import frida_download, kernel_download, modules_download +from beetroot.config import InstanceConfig, Module + +# Capture the real mkstemp before any test patches the name, so the spy below +# delegates to the genuine function instead of recursing into itself. +_REAL_MKSTEMP = tempfile.mkstemp + + +class _MkstempSpy(Protocol): + def __call__(self, *, dir: Path, suffix: str) -> tuple[int, str]: # noqa: A002 + ... + + +def _mkstemp_spy(seen: list[str]) -> _MkstempSpy: + """Wrap ``tempfile.mkstemp`` to record the temp names it hands out.""" + + def _spy(*, dir: Path, suffix: str) -> tuple[int, str]: # noqa: A002 # mirrors mkstemp's kw + fd, name = _REAL_MKSTEMP(dir=dir, suffix=suffix) + seen.append(name) + return fd, name + + return _spy + +VERSION = "16.4.10" +FAKE_BINARY = b"ELF\x7f fake frida binary content" * 64 +FAKE_COMPRESSED = lzma.compress(FAKE_BINARY) +FAKE_ZIP = b"PK\x03\x04 fake module zip payload" * 64 + + +def _chunked_resp(*chunks: bytes) -> MagicMock: + """A mock urlopen response yielding each chunk in turn, then EOF.""" + resp = MagicMock() + resp.read.side_effect = [*chunks, b""] + resp.headers.get.side_effect = lambda key, *args: None + resp.__enter__ = lambda s: s + resp.__exit__ = MagicMock(return_value=False) + return resp + + +@pytest.fixture +def instance_root(isolated_registry: Path, tmp_path: Path) -> Path: + root = tmp_path / "alpha" + root.mkdir() + return root + + +def _split(data: bytes, n: int) -> list[bytes]: + step = max(1, len(data) // n) + return [data[i : i + step] for i in range(0, len(data), step)] + + +class TestFridaProcessUniqueTemp: + def test_does_not_use_fixed_dotted_tmp(self, isolated_registry: Path) -> None: + # The staging path must be a mkstemp name, never the deterministic + # ``.tmp`` that two processes would collide on. + out = frida_download.cached_binary(VERSION) + seen: list[str] = [] + with ( + patch("urllib.request.urlopen", return_value=_chunked_resp(FAKE_COMPRESSED)), + patch("beetroot.frida_download.tempfile.mkstemp", side_effect=_mkstemp_spy(seen)), + ): + frida_download.download(VERSION) + assert seen, "download must stage via tempfile.mkstemp" + assert seen[0] != str(out.with_suffix(".tmp")) + assert Path(seen[0]).parent == out.parent + + def test_staged_binary_keeps_executable_bit(self, isolated_registry: Path) -> None: + with patch("urllib.request.urlopen", return_value=_chunked_resp(FAKE_COMPRESSED)): + out = frida_download.download(VERSION) + assert out.stat().st_mode & stat.S_IXUSR + + +class TestFridaIncrementalStreaming: + def test_multi_chunk_decompresses_correctly(self, isolated_registry: Path) -> None: + # Multiple chunks exercise the incremental decompressor's + # intermediate-vs-final output; the file must be byte-identical. + chunks = _split(FAKE_COMPRESSED, 5) + with patch("urllib.request.urlopen", return_value=_chunked_resp(*chunks)): + out = frida_download.download(VERSION) + assert out.read_bytes() == FAKE_BINARY + + def test_corrupt_xz_raises_frida_fetch_error(self, isolated_registry: Path) -> None: + chunks = _split(b"this is not valid lzma data at all" * 8, 4) + with patch("urllib.request.urlopen", return_value=_chunked_resp(*chunks)): + with pytest.raises(frida_download.FridaFetchError, match="decompression failed"): + frida_download.download(VERSION) + assert not frida_download.cached_binary(VERSION).exists() + + +class TestFridaDecompressionCeiling: + def test_exceeding_ceiling_raises_and_leaves_no_output( + self, isolated_registry: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + # Shrink the ceiling so the (small) happy-path payload trips it, + # proving the bound is enforced without needing a real zip bomb. + monkeypatch.setattr(frida_download, "_MAX_DECOMPRESSED_BYTES", 8) + with patch("urllib.request.urlopen", return_value=_chunked_resp(FAKE_COMPRESSED)): + with pytest.raises(frida_download.FridaFetchError, match="ceiling"): + frida_download.download(VERSION) + assert not frida_download.cached_binary(VERSION).exists() + + def test_under_ceiling_succeeds(self, isolated_registry: Path) -> None: + with patch("urllib.request.urlopen", return_value=_chunked_resp(FAKE_COMPRESSED)): + out = frida_download.download(VERSION) + assert out.read_bytes() == FAKE_BINARY + + +class TestFridaConcurrentDoesNotPoison: + def test_interleaved_writers_publish_one_complete_payload( + self, isolated_registry: Path + ) -> None: + # Two back-to-back downloads to the same cache target (the second a cache + # hit) must publish exactly one complete, valid binary — never a + # concatenation of two writers' bytes. + with patch("urllib.request.urlopen", return_value=_chunked_resp(FAKE_COMPRESSED)): + first = frida_download.download(VERSION) + with patch("urllib.request.urlopen", return_value=_chunked_resp(FAKE_COMPRESSED)): + second = frida_download.download(VERSION) + assert first == second + assert first.read_bytes() == FAKE_BINARY + + +class TestModuleProcessUniqueTempAndStreaming: + def test_does_not_use_fixed_dotted_tmp(self, instance_root: Path) -> None: + url = "https://example.com/mod.zip" + cache = modules_download._cache_path_for_url(url) + cfg = InstanceConfig(modules=[Module(url=url)]) + seen: list[str] = [] + with ( + patch("urllib.request.urlopen", return_value=_chunked_resp(FAKE_ZIP)), + patch("beetroot.modules_download.tempfile.mkstemp", side_effect=_mkstemp_spy(seen)), + ): + modules_download.stage_for_instance(instance_root, cfg) + assert seen + assert seen[0] != str(cache.with_suffix(".tmp")) + assert Path(seen[0]).parent == cache.parent + + def test_multi_chunk_streams_to_disk(self, instance_root: Path) -> None: + chunks = _split(FAKE_ZIP, 4) + url = "https://example.com/mod.zip" + cfg = InstanceConfig(modules=[Module(url=url)]) + with patch("urllib.request.urlopen", return_value=_chunked_resp(*chunks)): + staged = modules_download.stage_for_instance(instance_root, cfg) + assert staged[0].read_bytes() == FAKE_ZIP + + def test_http_error_leaves_no_temp_behind(self, instance_root: Path) -> None: + url = "https://example.com/mod.zip" + cfg = InstanceConfig(modules=[Module(url=url)]) + + def _raise(*args: object, **kwargs: object) -> MagicMock: + raise urllib.error.HTTPError(url, 404, "Not Found", {}, None) # type: ignore[arg-type] + + with patch("urllib.request.urlopen", side_effect=_raise): + with pytest.raises(modules_download.ModuleFetchError, match="HTTP 404"): + modules_download.stage_for_instance(instance_root, cfg) + cache_dir = modules_download._cache_path_for_url(url).parent + leftovers = list(cache_dir.glob("*.tmp")) if cache_dir.exists() else [] + assert leftovers == [] + + def test_unmapped_crash_cleans_up_temp(self, instance_root: Path) -> None: + # A failure that isn't one of the mapped HTTP/timeout/URL errors (e.g. a + # KeyboardInterrupt mid-write) must still unlink the staged temp so it + # doesn't orphan in the user-global cache. + url = "https://example.com/mod.zip" + cfg = InstanceConfig(modules=[Module(url=url)]) + + def _close_and_crash(fd: int, mode: str) -> object: + import os as _os + + _os.close(fd) # avoid leaking the mkstemp fd / a ResourceWarning + raise KeyboardInterrupt + + with ( + patch("urllib.request.urlopen", return_value=_chunked_resp(FAKE_ZIP)), + patch("beetroot.modules_download.os.fdopen", side_effect=_close_and_crash), + ): + with pytest.raises(KeyboardInterrupt): + modules_download.stage_for_instance(instance_root, cfg) + cache_dir = modules_download._cache_path_for_url(url).parent + leftovers = list(cache_dir.glob("*.tmp")) if cache_dir.exists() else [] + assert leftovers == [] + + +class TestKernelProcessUniqueTemp: + def test_does_not_use_fixed_dotted_tmp(self, isolated_registry: Path, tmp_path: Path) -> None: + out = tmp_path / "kernels" / "bzImage" + digest = hashlib.sha256(b"kernel-bytes").hexdigest() + + def _fake_fetch(url: str, description: str) -> bytes: + return digest.encode() if url.endswith(".sha256") else b"kernel-bytes" + + seen: list[str] = [] + with ( + patch("beetroot.kernel_download._fetch_bytes", side_effect=_fake_fetch), + patch("beetroot.kernel_download.tempfile.mkstemp", side_effect=_mkstemp_spy(seen)), + ): + result = kernel_download.fetch_prebuilt( + version="6.12.9", fingerprint="abc123def456", out_path=out + ) + assert result == out + assert out.read_bytes() == b"kernel-bytes" + assert seen + assert seen[0] != str(out.with_suffix(".tmp")) + assert Path(seen[0]).parent == out.parent + + def test_unmapped_crash_cleans_up_temp( + self, isolated_registry: Path, tmp_path: Path + ) -> None: + out = tmp_path / "kernels" / "bzImage" + digest = hashlib.sha256(b"kernel-bytes").hexdigest() + + def _fake_fetch(url: str, description: str) -> bytes: + return digest.encode() if url.endswith(".sha256") else b"kernel-bytes" + + def _close_and_crash(fd: int, mode: str) -> object: + import os as _os + + _os.close(fd) + raise KeyboardInterrupt + + with ( + patch("beetroot.kernel_download._fetch_bytes", side_effect=_fake_fetch), + patch("beetroot.kernel_download.os.fdopen", side_effect=_close_and_crash), + ): + with pytest.raises(KeyboardInterrupt): + kernel_download.fetch_prebuilt( + version="6.12.9", fingerprint="abc123def456", out_path=out + ) + leftovers = list(out.parent.glob("*.tmp")) if out.parent.exists() else [] + assert leftovers == [] + assert not out.exists() diff --git a/tests/test_bugfix_module_filename.py b/tests/test_bugfix_module_filename.py new file mode 100644 index 0000000..4ed4fdb --- /dev/null +++ b/tests/test_bugfix_module_filename.py @@ -0,0 +1,63 @@ +"""Regression tests for #168 — module URL query/fragment poisons the staged basename. + +A module URL carrying a ``?query`` or ``#fragment`` used to stage a filename +that retained the suffix (e.g. ``m.zip?v=2``), which the ``*.zip`` flash glob in +``flash-modules.sh`` never matched, so the module was silently skipped. The +basename is now derived from the URL *path* only. +""" + +from __future__ import annotations + +import fnmatch +from pathlib import Path +from unittest.mock import MagicMock, patch + +import pytest + +from beetroot import modules_download, paths +from beetroot.config import InstanceConfig, Module + +FAKE_ZIP_CONTENT = b"PK\x03\x04 fake zip content" + + +def _make_url_resp() -> MagicMock: + resp = MagicMock() + resp.read.side_effect = [FAKE_ZIP_CONTENT, b""] + resp.headers.get.side_effect = lambda key, *args: None + resp.__enter__ = lambda s: s + resp.__exit__ = MagicMock(return_value=False) + return resp + + +@pytest.fixture +def instance_root(isolated_registry: Path, tmp_path: Path) -> Path: + root = tmp_path / "alpha" + root.mkdir() + return root + + +class TestFilenameFromUrl: + def test_query_and_fragment_are_stripped(self) -> None: + assert modules_download._filename_from_url("https://h/m.zip?token=abc#frag") == "m.zip" + + def test_query_only_is_stripped(self) -> None: + assert modules_download._filename_from_url("https://h/path/m.zip?v=2") == "m.zip" + + def test_empty_path_falls_back(self) -> None: + assert modules_download._filename_from_url("https://h") == "module.zip" + + def test_trailing_slash_falls_back(self) -> None: + assert modules_download._filename_from_url("https://h/dir/") == "module.zip" + + +class TestStagedNameMatchesFlashGlob: + def test_query_string_url_stages_clean_zip(self, instance_root: Path) -> None: + cfg = InstanceConfig(modules=[Module(url="https://example.com/magisk-mod.zip?v=2")]) + with patch("urllib.request.urlopen", return_value=_make_url_resp()): + staged = modules_download.stage_for_instance(instance_root, cfg) + assert len(staged) == 1 + assert staged[0].parent == paths.instance_modules(instance_root) + assert staged[0].name.endswith(".zip") + # The redroid boot helper globs ``*.zip``; the staged name must match it. + assert fnmatch.fnmatch(staged[0].name, "*.zip") + assert "?" not in staged[0].name diff --git a/tests/test_bugfix_snapshot_self_inclusion.py b/tests/test_bugfix_snapshot_self_inclusion.py index dcfa0c6..77f20a2 100644 --- a/tests/test_bugfix_snapshot_self_inclusion.py +++ b/tests/test_bugfix_snapshot_self_inclusion.py @@ -78,3 +78,29 @@ def test_snapshot_keeps_same_named_archive_outside_instance_dir( members = _list_archive_members(archive) assert "./data/alpha.tar.zst" in members + + +def test_snapshot_excludes_nested_dest_archive_at_any_depth( + isolated_registry: Path, tmp_path: Path +) -> None: + """A dest nested in a subdir (e.g. data/) is excluded, not just top-level. + + The CLI default ``.tar.zst`` resolves against the cwd; run from + inside ``data/`` it lands at ``data/.tar.zst``. The top-level + ``entry.resolve()`` check never sees that path — only the depth- + independent filter callback drops it (#173). Asserting on both the + dropped and the kept member exercises both branches of the callback. + """ + src = _make_instance(tmp_path / "alpha") + registry.add_allocating("alpha", src) + + dest = src / "data" / "alpha.tar.zst" + archive = snapshot.snapshot(src, dest) + assert archive == dest + + members = _list_archive_members(archive) + # The still-open output archive is dropped (filter returns None) even + # though it lives a directory deep, not at the instance root. + assert "./data/alpha.tar.zst" not in members + # A sibling under the same subdir is kept (filter returns the info). + assert "./data/marker.txt" in members diff --git a/tests/test_builder.py b/tests/test_builder.py index a880fc5..cf66d0b 100644 --- a/tests/test_builder.py +++ b/tests/test_builder.py @@ -2,7 +2,10 @@ from __future__ import annotations +import fcntl +import hashlib import re +import shlex import shutil import subprocess from collections.abc import Sequence @@ -50,6 +53,25 @@ def run( raise BootstrapError(f"fake failure on {self.fail_on} (exit {self.fail_exit})") +# The real daemon-preflight callable, captured before the autouse fixture +# below stubs the module attribute — so the few tests that exercise the real +# implementation can restore it. +_REAL_DAEMON_RESPONSIVE = builder._docker_daemon_responsive + + +@pytest.fixture(autouse=True) +def _daemon_up(monkeypatch: pytest.MonkeyPatch) -> None: + """ + Default the Docker-daemon preflight to "up" for every builder test. + + ``build_image`` grew a daemon preflight (#193); without this the daemonless + test host would short-circuit every existing build_image test. Tests that + exercise the daemon-down branch re-patch it to ``False`` themselves; tests + of the real probe restore :data:`_REAL_DAEMON_RESPONSIVE`. + """ + monkeypatch.setattr(builder, "_docker_daemon_responsive", lambda: True) + + class TestGappsVendorFlags: def test_litegapps_uses_lg(self) -> None: assert GAPPS_VENDOR_FLAGS["litegapps"] == ["-lg"] @@ -588,6 +610,15 @@ def _ready_bake_host(self, monkeypatch: pytest.MonkeyPatch) -> None: # failing bake_preflight explicitly. monkeypatch.setattr(builder, "vm_bake_preflight", lambda **_k: []) + # ``_fetch_kernel_source`` now sha256-verifies the downloaded tarball + # against the pinned digest (#184); the FakeRunner doesn't materialise + # the tarball on its ``curl`` call, so the real read+hash would raise. + # Neutralise just the verify (a no-op) so the genuine curl/tar/return + # flow — including the real cdn.kernel.org URL the dispatch tests assert + # on — still runs; the verify-before-extract branch itself is covered by + # TestFetchKernelSource with a side-effecting runner. + monkeypatch.setattr(builder, "_verify_kernel_source_digest", lambda _tarball: None) + def test_runs_kernel_then_rootfs_steps(self, tmp_path: Path) -> None: runner = FakeRunner() rec = _RootfsBuildRecorder() @@ -1610,6 +1641,7 @@ def test_sleep_invokes_time_sleep(self, monkeypatch: pytest.MonkeyPatch) -> None class TestDockerDaemonResponsive: def test_true_when_info_succeeds(self, monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setattr(builder, "_docker_daemon_responsive", _REAL_DAEMON_RESPONSIVE) monkeypatch.setattr( "beetroot.builder.subprocess.run", lambda *_a, **_k: subprocess.CompletedProcess(args=[], returncode=0), @@ -1617,6 +1649,7 @@ def test_true_when_info_succeeds(self, monkeypatch: pytest.MonkeyPatch) -> None: assert builder._docker_daemon_responsive() is True def test_false_when_info_nonzero(self, monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setattr(builder, "_docker_daemon_responsive", _REAL_DAEMON_RESPONSIVE) monkeypatch.setattr( "beetroot.builder.subprocess.run", lambda *_a, **_k: subprocess.CompletedProcess(args=[], returncode=1), @@ -1624,6 +1657,8 @@ def test_false_when_info_nonzero(self, monkeypatch: pytest.MonkeyPatch) -> None: assert builder._docker_daemon_responsive() is False def test_false_when_docker_missing(self, monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setattr(builder, "_docker_daemon_responsive", _REAL_DAEMON_RESPONSIVE) + def _boom(*_a: object, **_k: object) -> object: raise FileNotFoundError @@ -1647,7 +1682,7 @@ def _cfg(self, tmp_path: Path, *, present: tuple[str, ...]) -> builder._RootfsCo xtables_multi=paths["xtables-legacy-multi"], ) - def _ready( + def _ready( # noqa: PLR0913 # test helper; each kwarg toggles one preflight branch self, monkeypatch: pytest.MonkeyPatch, tmp_path: Path, @@ -1655,11 +1690,15 @@ def _ready( present: tuple[str, ...] = ("busybox", "socat", "xtables-legacy-multi"), which: object = None, daemon: bool = True, + euid: int = 0, ) -> None: cfg = self._cfg(tmp_path, present=present) monkeypatch.setattr(builder._RootfsConfig, "from_env", lambda **_k: cfg) monkeypatch.setattr("beetroot.builder.shutil.which", which or (lambda _n: "/usr/bin/found")) monkeypatch.setattr(builder, "_docker_daemon_responsive", lambda: daemon) + # The bake's root-privilege preflight (#231) — default to root so the + # other branches stay isolated; root-specific tests override ``euid``. + monkeypatch.setattr("beetroot.builder.os.geteuid", lambda: euid) def test_ready_host_has_no_problems( self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch @@ -1704,8 +1743,11 @@ def test_redroid_tar_skips_daemon_check( self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch ) -> None: self._ready(monkeypatch, tmp_path, daemon=False) - # With a pre-saved tarball the bake never pulls, so a down daemon is fine. - assert builder.vm_build_preflight(redroid_tar=tmp_path / "redroid.tar") == [] + # With a pre-saved tarball the bake never pulls, so a down daemon is fine + # — but the tarball must actually exist (issue #186). + tar = tmp_path / "redroid.tar" + tar.write_bytes(b"tar") + assert builder.vm_build_preflight(redroid_tar=tar) == [] def test_reports_everything_missing_in_one_pass( self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch @@ -1714,3 +1756,236 @@ def test_reports_everything_missing_in_one_pass( self._ready(monkeypatch, tmp_path, present=(), which=lambda _n: None, daemon=False) names = {p.requirement for p in builder.vm_build_preflight()} assert {"busybox", "socat", "xtables-legacy-multi", "curl", "tar"} <= names + + def test_missing_redroid_tar_reported( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + # issue #186: a set-but-missing REDROID_TAR must be a preflight problem, + # not a mid-bake `docker load` 404 after --check passed. + self._ready(monkeypatch, tmp_path, daemon=False) + problems = builder.vm_bake_preflight(redroid_tar=tmp_path / "nope.tar") + assert [p.requirement for p in problems] == ["REDROID_TAR"] + assert "not found" in problems[0].detail + + def test_existing_redroid_tar_skips_daemon_even_when_down( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + # A genuinely-present tarball: no REDROID_TAR problem AND no daemon + # problem, even with the daemon probe forced down (#186 skip branch). + self._ready(monkeypatch, tmp_path, daemon=False) + tar = tmp_path / "redroid.tar" + tar.write_bytes(b"tar") + problems = builder.vm_bake_preflight(redroid_tar=tar) + assert all(p.requirement not in {"REDROID_TAR", "Docker daemon"} for p in problems) + + def test_unprivileged_euid_reported_without_redroid_tar( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + # issue #231: the staging dockerd needs root; surface it up front. + self._ready(monkeypatch, tmp_path, euid=1000) + names = [p.requirement for p in builder.vm_bake_preflight()] + assert "root privilege" in names + + def test_unprivileged_euid_reported_with_redroid_tar( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + # The root check is NOT gated on REDROID_TAR — the staging dockerd is + # spawned even when loading from a tarball (#231). + self._ready(monkeypatch, tmp_path, euid=1000) + tar = tmp_path / "redroid.tar" + tar.write_bytes(b"tar") + names = [p.requirement for p in builder.vm_bake_preflight(redroid_tar=tar)] + assert "root privilege" in names + + def test_root_euid_has_no_privilege_problem( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + self._ready(monkeypatch, tmp_path, euid=0) + names = [p.requirement for p in builder.vm_bake_preflight()] + assert "root privilege" not in names + + +class TestBuildImageDaemonPreflight: + """issue #193: ``beetroot build`` runs a Docker-daemon preflight.""" + + def test_daemon_down_raises_friendly_error(self, monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setattr(builder, "_docker_daemon_responsive", lambda: False) + runner = FakeRunner() + with pytest.raises(BootstrapError, match="Docker daemon") as exc: + build_image(runner=runner) + assert "start the daemon" in str(exc.value) + # Fails before any clone/patch/build runner call. + assert runner.calls == [] + + def test_daemon_up_proceeds_to_build(self, monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setattr(builder, "_docker_daemon_responsive", lambda: True) + runner = FakeRunner() + build_image(runner=runner) + # rm, clone, patch, build all run when the daemon is up. + assert [c.cmd[0] for c in runner.calls] == ["rm", "git", "uv", "docker"] + + +class TestBuildImageBuildKit: + """issue #229: force BuildKit for the BuildKit-only ``COPY --chmod``.""" + + def test_compose_build_env_forces_buildkit(self) -> None: + runner = FakeRunner() + build_image(runner=runner) + build_call = runner.calls[-1] + assert build_call.cmd[1] == "compose" + assert build_call.env is not None + assert build_call.env["DOCKER_BUILDKIT"] == "1" + assert build_call.env["COMPOSE_DOCKER_CLI_BUILD"] == "1" + + +class TestBuildImageCloneLock: + """issue #232: serialize concurrent builds with an fcntl.flock.""" + + def test_lock_acquired_before_clone_released_after_patch( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + events: list[str] = [] + real_flock = fcntl.flock + + def recording_flock(fd: int, op: int) -> None: + if op == fcntl.LOCK_EX: + events.append("lock") + elif op == fcntl.LOCK_UN: + events.append("unlock") + real_flock(fd, op) + + monkeypatch.setattr("beetroot.builder.fcntl.flock", recording_flock) + + @dataclass + class _RecordingRunner: + def run( + self, + cmd: Sequence[str], + *, + cwd: Path | None = None, + check: bool = True, + env: dict[str, str] | None = None, + ) -> None: + events.append(cmd[0]) + + build_image(work_dir=tmp_path / "work", runner=_RecordingRunner()) + # Lock is acquired before the rm/clone, released after the patch (uv), + # and the docker build runs after release. + assert events.index("lock") < events.index("rm") + assert events.index("lock") < events.index("git") + assert events.index("uv") < events.index("unlock") + assert events.index("unlock") < events.index("docker") + + def test_reuse_path_holds_lock_across_clone_url_check( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + # An existing matching clone reuses artifacts but must still hold the + # lock while reading .git/config (synchronizing with a racing clone). + work = tmp_path / "work" + held: list[bool] = [] + + def check_lock_held(work_dir: Path, url: str) -> bool: + # The lockfile exists while the reuse branch runs under the held lock. + held.append(work.with_name("work.lock").exists()) + return True + + monkeypatch.setattr(builder, "_clone_url_matches", check_lock_held) + build_image(work_dir=work, runner=FakeRunner()) + assert held == [True] + + +class TestKernelConfigShellQuoting: + """issue #208: build-context-derived paths are shell-quoted in the compile.""" + + def test_paths_with_spaces_are_quoted( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + ctx = tmp_path / "My Checkout" + _make_vm_context(ctx) + out = tmp_path / "My Out" + runner = FakeRunner() + # build_vm_kernel's autouse fixture lives on TestBuildVmKernel; here we + # need the source-digest verify stub too (the FakeRunner doesn't + # materialise the tarball), so patch it locally. + monkeypatch.setattr(builder, "_verify_kernel_source_digest", lambda _t: None) + builder.build_vm_kernel( + out_dir=out, + build_context=ctx, + runner=runner, + rootfs_build=_RootfsBuildRecorder(), + from_source=True, + bake_preflight=lambda **_k: [], + ) + compile_cmd = runner.calls[-1].cmd[2] + kernel_config = (ctx / "docker" / "vm" / "kernel.config").resolve() + bzimage = (out / "bzImage").resolve() + # Both interpolated paths appear as single shlex-quoted tokens. + assert shlex.quote(str(kernel_config)) in compile_cmd + assert shlex.quote(str(bzimage)) in compile_cmd + # The bare unquoted (space-splitting) forms are absent. + assert f"-m .config {kernel_config} " not in compile_cmd + + +class TestFetchKernelSource: + """issue #184: verify the kernel source tarball against a pinned sha256.""" + + def _runner_writing(self, contents: bytes) -> FakeRunner: + # A FakeRunner whose curl call materialises the tarball bytes, so the + # real ``_fetch_kernel_source`` can read + hash them. + runner = FakeRunner() + real_run = runner.run + + def writing_run( + cmd: Sequence[str], + *, + cwd: Path | None = None, + check: bool = True, + env: dict[str, str] | None = None, + ) -> None: + real_run(cmd, cwd=cwd, check=check, env=env) + if cmd[0] == "curl": + Path(cmd[cmd.index("-o") + 1]).write_bytes(contents) + + runner.run = writing_run # type: ignore[method-assign] + return runner + + def test_mismatch_raises_and_skips_tar(self, tmp_path: Path) -> None: + runner = self._runner_writing(b"tampered-bytes") + with pytest.raises(BootstrapError, match="sha256 mismatch"): + builder._fetch_kernel_source(runner, tmp_path) + # tar must NOT have run — extraction is gated behind verification. + assert not any(c.cmd[0] == "tar" for c in runner.calls) + + def test_match_proceeds_to_extract( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + good = b"genuine-kernel-source" + monkeypatch.setattr( + builder, "KERNEL_SOURCE_SHA256", hashlib.sha256(good).hexdigest() + ) + runner = self._runner_writing(good) + tree = builder._fetch_kernel_source(runner, tmp_path) + assert any(c.cmd[0] == "tar" for c in runner.calls) + assert tree == tmp_path / f"linux-{builder.KERNEL_VERSION}" + + +@pytest.mark.usefixtures("_no_sleep") +class TestMajorVersionFromImage: + """issue #187: the rootfs marker records the baked REDROID_IMAGE version.""" + + def test_parses_leading_major(self) -> None: + assert builder._major_version_from_image("redroid/redroid:13.0.0-latest") == 13 + + def test_malformed_tag_raises(self) -> None: + with pytest.raises(BootstrapError, match="Android major version"): + builder._major_version_from_image("redroid/redroid:latest") + + def test_marker_records_baked_image_version_not_arg(self, tmp_path: Path) -> None: + # android_version arg says 14 but REDROID_IMAGE bakes 13 — the marker + # must follow the actually-baked image (#187). + cfg = _make_rootfs_config( + tmp_path, android_version=14, redroid_image="redroid/redroid:13.0.0-latest" + ) + runner = FakeRootfsRunner(applets=("sh",)) + out = _run_assembly(tmp_path, runner, cfg) + assert builder.read_rootfs_version(out) == 13 diff --git a/tests/test_compose.py b/tests/test_compose.py index 161e946..81acf6b 100644 --- a/tests/test_compose.py +++ b/tests/test_compose.py @@ -268,6 +268,17 @@ def test_logs_without_follow_no_f_flag(self, tmp_path: Path) -> None: logs_idx = cmd.index("logs") assert "-f" not in cmd[logs_idx:] + def test_logs_raises_on_nonzero_non_follow(self, tmp_path: Path) -> None: + with patch("subprocess.run", return_value=_fail_result(1)): + with pytest.raises(ComposeError, match="compose logs"): + compose.logs("alpha", tmp_path, follow=False) + + def test_logs_tolerates_nonzero_in_follow(self, tmp_path: Path) -> None: + # Ctrl-C out of a ``logs -f`` stream surfaces as a non-zero (SIGINT) + # exit; that is the expected way to stop it, so it must not raise. + with patch("subprocess.run", return_value=_fail_result(-2)): + compose.logs("alpha", tmp_path, follow=True) + class TestBuild: def test_build_includes_build_subcommand(self, tmp_path: Path) -> None: diff --git a/tests/test_console.py b/tests/test_console.py index e60a3c0..c0cdf02 100644 --- a/tests/test_console.py +++ b/tests/test_console.py @@ -277,6 +277,48 @@ def test_table_writes_to_stdout_not_stderr(monkeypatch: pytest.MonkeyPatch) -> N assert "alpha" not in stderr_buf.getvalue() +def test_table_non_tty_renders_long_cell_verbatim(monkeypatch: pytest.MonkeyPatch) -> None: + # Off-TTY, rich's 80-col default would clip a wide ls/modes row with an + # ellipsis. The lossless branch must emit the ADB endpoint and the full + # path verbatim, with no "…" truncation marker anywhere (#204). + c, buf = _make_console(tty=False) + monkeypatch.setattr(console, "_stdout_console", c) + long_path = "/home/user/very/long/path/to/instance" + console.table( + ["NAME", "ADB", "PATH"], + [["alpha-research-phone", "localhost:5555", long_path]], + ) + out = buf.getvalue() + assert "localhost:5555" in out + assert long_path in out + assert "alpha-research-phone" in out + assert "…" not in out # the ellipsis rich inserts when it truncates + + +def test_table_non_tty_has_no_box_drawing(monkeypatch: pytest.MonkeyPatch) -> None: + # Box-drawing borders are plain UTF-8 (not ANSI), so they survive the + # non-TTY color strip and would pollute piped output. The lossless branch + # drops them entirely (#204). + c, buf = _make_console(tty=False) + monkeypatch.setattr(console, "_stdout_console", c) + console.table(["NAME", "ADB"], [["alpha", "localhost:5555"]]) + out = buf.getvalue() + for glyph in "┏┳┓┃│┗┻┛┡╇┩─": + assert glyph not in out + + +def test_table_tty_keeps_decorated_box(monkeypatch: pytest.MonkeyPatch) -> None: + # The TTY branch is unchanged: interactive output stays a decorated rich + # Table with box-drawing borders. + c, buf = _make_console(tty=True) + monkeypatch.setattr(console, "_stdout_console", c) + console.table(["NAME", "ADB"], [["alpha", "localhost:5555"]]) + out = buf.getvalue() + assert "alpha" in out + assert "localhost:5555" in out + assert any(glyph in out for glyph in "┏┓│─") + + # --------------------------------------------------------------------------- # ProgressContext — direct use # --------------------------------------------------------------------------- diff --git a/tests/test_dockerignore.py b/tests/test_dockerignore.py new file mode 100644 index 0000000..8784c2e --- /dev/null +++ b/tests/test_dockerignore.py @@ -0,0 +1,22 @@ +"""Tests for the repo-root ``.dockerignore`` build-context allowlist (#207).""" + +from __future__ import annotations + +from beetroot import builder + + +def test_dockerignore_exists_at_build_context_root() -> None: + dockerignore = builder._default_build_context() / ".dockerignore" + assert dockerignore.is_file() + + +def test_dockerignore_excludes_all_then_reincludes_docker() -> None: + dockerignore = builder._default_build_context() / ".dockerignore" + lines = { + line.strip() + for line in dockerignore.read_text().splitlines() + if line.strip() and not line.lstrip().startswith("#") + } + assert "*" in lines + assert "!docker/" in lines + assert "!docker/**" in lines diff --git a/tests/test_magisk_config_helper.py b/tests/test_magisk_config_helper.py index 5e20feb..c395557 100644 --- a/tests/test_magisk_config_helper.py +++ b/tests/test_magisk_config_helper.py @@ -195,6 +195,49 @@ def test_daemon_wait_succeeds_within_budget(tmp_path: Path) -> None: assert any("REPLACE INTO settings" in q for q in queries) +def test_magisk_value_select_failure_aborts_not_masked(tmp_path: Path) -> None: + # Issue #239: the prior ``magisk ... | awk`` ran magisk inside a pipeline, + # so under ``set -eu`` the substitution's exit status was awk's (always 0) + # — a ``magisk --sqlite`` failure after the liveness probe was silently + # masked. With the pipe removed, a value-SELECT failure must abort the + # helper (non-zero) instead of proceeding with an empty/garbage value. + fake_bin = tmp_path / "bin" + fake_bin.mkdir() + magisk = fake_bin / "magisk" + # SELECT 1 (liveness) succeeds; the REPLACE INTO settings writes succeed; + # the value-SELECT exits non-zero with no output (a daemon-unreachable + # read failure after the probe). The pre-#239 pipe would have masked this. + magisk.write_text( + """#!/bin/sh +echo "$@" >> "$MAGISK_LOG" +case "$2" in + "SELECT 1") exit 0 ;; + "SELECT value FROM settings WHERE key='zygisk';") exit 7 ;; +esac +exit 0 +""" + ) + magisk.chmod(0o755) + log = tmp_path / "magisk.log" + log.write_text("") + res = subprocess.run( # noqa: S603 # controlled fake-magisk shim; argv is fixed + ["sh", str(HELPER)], # noqa: S607 # `sh` is universal POSIX, matching Android init's invocation + check=False, + capture_output=True, + text=True, + env={ + "BEETROOT_DENYLIST_PACKAGES": "", + "MAGISK_LOG": str(log), + "PATH": f"{fake_bin}:/usr/bin:/bin", + }, + timeout=10, + ) + assert res.returncode != 0, "value-SELECT failure was masked instead of aborting" + # The helper must not have reached the denylist INSERTs after the failed read. + queries = [line for line in log.read_text().splitlines() if line] + assert not [q for q in queries if "INSERT OR IGNORE INTO denylist" in q] + + @pytest.mark.parametrize("packages", [",,", ",com.app,", " "]) def test_malformed_csv_does_not_inject_empty_rows(tmp_path: Path, packages: str) -> None: # CSVs with extra commas (``,,``, leading/trailing) must not produce diff --git a/tests/test_module_auto_install.py b/tests/test_module_auto_install.py index f8a1b73..297f5bd 100644 --- a/tests/test_module_auto_install.py +++ b/tests/test_module_auto_install.py @@ -391,7 +391,12 @@ def test_mid_batch_offline_reports_completed_rows_then_error( "(reconnect it, accept its USB-debugging authorization prompt " "if one is shown, and check `adb devices`)" in result.stderr ) - assert f"[beetroot] failed: {second} — device went offline mid-install" in result.stderr + # The row is stage-neutral and retains the underlying adb error + # (#223), instead of the old hardcoded "mid-install" detail. + assert f"[beetroot] failed: {second} — device went offline during this module" in ( + result.stderr + ) + assert "last adb error:" in result.stderr class TestCapabilityGating: diff --git a/tests/test_paths.py b/tests/test_paths.py index 0ca5d55..7ce555d 100644 --- a/tests/test_paths.py +++ b/tests/test_paths.py @@ -3,6 +3,7 @@ from __future__ import annotations import importlib.resources +import os from pathlib import Path import pytest @@ -279,6 +280,110 @@ def _fake_as_file(resource: _ZipResource) -> Iterator[_ZipResource]: assert (result / name).stat().st_mtime_ns == mtimes[name], name +class TestAtomicCacheMaterialisation: + """Issue #226: cache writes go through a temp file + ``os.replace``. + + A concurrent wheel-installed reader handing the same path to + ``docker compose -f`` must never observe a truncated file, and the helpers + must not leave ``.tmp`` debris behind. + """ + + @staticmethod + def _patch_zip_install( + monkeypatch: pytest.MonkeyPatch, + tmp_path: Path, + payloads: dict[str, bytes], + ) -> None: + monkeypatch.setenv("XDG_CACHE_HOME", str(tmp_path / "cache")) + + import contextlib + from collections.abc import Iterator + + class _ZipResource: + def __init__(self, name: str) -> None: + self._name = name + + def is_file(self) -> bool: + return False + + def read_bytes(self) -> bytes: + return payloads[self._name] + + class _Files: + def joinpath(self, name: str) -> _ZipResource: + return _ZipResource(name) + + @contextlib.contextmanager + def _fake_as_file(resource: _ZipResource) -> Iterator[_ZipResource]: + yield resource + + monkeypatch.setattr(importlib.resources, "files", lambda _pkg: _Files()) + monkeypatch.setattr(importlib.resources, "as_file", _fake_as_file) + + def test_compose_leaves_no_tmp_artifact( + self, monkeypatch: pytest.MonkeyPatch, tmp_path: Path + ) -> None: + self._patch_zip_install(monkeypatch, tmp_path, {"compose.yaml": b"compose: v1\n"}) + monkeypatch.setattr(paths, "_BUNDLED_COMPOSE_CACHE", None) + result = paths.bundled_compose_file() + assert result.read_bytes() == b"compose: v1\n" + leftovers = list(result.parent.glob("*.tmp")) + assert leftovers == [], f"left temp files behind: {leftovers}" + + def test_compose_uses_os_replace( + self, monkeypatch: pytest.MonkeyPatch, tmp_path: Path + ) -> None: + self._patch_zip_install(monkeypatch, tmp_path, {"compose.yaml": b"compose: v2\n"}) + monkeypatch.setattr(paths, "_BUNDLED_COMPOSE_CACHE", None) + calls: list[tuple[str, str]] = [] + real_replace = os.replace + + def _spy(src: object, dst: object, /, **kw: object) -> None: + calls.append((str(src), str(dst))) + real_replace(src, dst) # type: ignore[arg-type] + + monkeypatch.setattr(os, "replace", _spy) + result = paths.bundled_compose_file() + assert len(calls) == 1, "atomic write must replace exactly once" + # The rename lands on the final cache path, not a temp name. + assert calls[0][1] == str(result) + + def test_vm_assets_leave_no_tmp_artifact( + self, monkeypatch: pytest.MonkeyPatch, tmp_path: Path + ) -> None: + payloads = { + "kernel.config": b"CONFIG_FOO=y\n", + "guest-init.sh": b"#!/bin/sh\n", + "adbprobe.c": b"int main(){}\n", + } + self._patch_zip_install(monkeypatch, tmp_path, payloads) + monkeypatch.setattr(paths, "_BUNDLED_VM_DIR_CACHE", None) + result = paths.bundled_vm_dir() + for name, content in payloads.items(): + assert (result / name).read_bytes() == content + leftovers = list(result.glob("*.tmp")) + assert leftovers == [], f"left temp files behind: {leftovers}" + + def test_atomic_write_unlinks_temp_on_failure( + self, monkeypatch: pytest.MonkeyPatch, tmp_path: Path + ) -> None: + # The except-branch: if the rename fails, the staged temp file must be + # cleaned up rather than leaked into the cache dir. + target = tmp_path / "out.bin" + + class _BoomError(RuntimeError): + pass + + def _bad_replace(_src: object, _dst: object, /, **_kw: object) -> None: + raise _BoomError + + monkeypatch.setattr(os, "replace", _bad_replace) + with pytest.raises(_BoomError): + paths._atomic_write_bytes(target, b"data") + assert not target.exists() + assert list(tmp_path.glob("*.tmp")) == [], "temp file leaked on failure" + + class TestUserRegistryFile: def test_default_under_home_config( self, monkeypatch: pytest.MonkeyPatch, tmp_path: Path @@ -294,6 +399,49 @@ def test_respects_xdg_config_home( assert paths.user_registry_file() == (tmp_path / "myxdg" / "beetroot" / "instances.json") +class TestRelativeXdgIsIgnored: + """Issue #225: a relative ``$XDG_*_HOME`` must not yield a relative path. + + The XDG spec treats relative env values as invalid; honouring one would + fragment the user-global registry across working directories. The accessors + fall back to the spec default (``~/.config`` / ``~/.cache``) instead. + """ + + def test_relative_xdg_config_home_falls_back_to_home( + self, monkeypatch: pytest.MonkeyPatch, tmp_path: Path + ) -> None: + monkeypatch.setenv("HOME", str(tmp_path)) + monkeypatch.setenv("XDG_CONFIG_HOME", "relconfig") + config_dir = paths.user_config_dir() + registry = paths.user_registry_file() + assert config_dir.is_absolute() + assert config_dir == tmp_path / ".config" / "beetroot" + assert registry.is_absolute() + assert registry == tmp_path / ".config" / "beetroot" / "instances.json" + + def test_relative_xdg_cache_home_falls_back_to_home( + self, monkeypatch: pytest.MonkeyPatch, tmp_path: Path + ) -> None: + monkeypatch.setenv("HOME", str(tmp_path)) + monkeypatch.setenv("XDG_CACHE_HOME", "relcache") + cache = paths.user_cache_dir("frida") + assert cache.is_absolute() + assert cache == tmp_path / ".cache" / "beetroot" / "frida" + + def test_absolute_xdg_config_home_used_verbatim( + self, monkeypatch: pytest.MonkeyPatch, tmp_path: Path + ) -> None: + # The ``is_absolute()`` true-branch: an absolute XDG dir is honoured. + monkeypatch.setenv("XDG_CONFIG_HOME", str(tmp_path / "abs")) + assert paths.user_config_dir() == tmp_path / "abs" / "beetroot" + + def test_absolute_xdg_cache_home_used_verbatim( + self, monkeypatch: pytest.MonkeyPatch, tmp_path: Path + ) -> None: + monkeypatch.setenv("XDG_CACHE_HOME", str(tmp_path / "abs")) + assert paths.user_cache_dir("modules") == tmp_path / "abs" / "beetroot" / "modules" + + class TestUserCacheDir: def test_default_under_home_cache( self, monkeypatch: pytest.MonkeyPatch, tmp_path: Path diff --git a/tests/test_rootfs_download.py b/tests/test_rootfs_download.py index 28dd71e..74a3726 100644 --- a/tests/test_rootfs_download.py +++ b/tests/test_rootfs_download.py @@ -223,3 +223,46 @@ def fake_urlopen(req_url: str, timeout: float) -> _FakeResp: out_image=tmp_path / "rootdisk.img", docker_version="27.5.1", ) + + +def test_fetch_prebuilt_marker_written_before_image_rename( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + # issue #234: if the process dies in the window between renaming the image + # into place and writing the marker, read_rootfs_version() would return None + # and silently skip the #82 skew check. The marker must be written BEFORE the + # image is renamed, so an installed image is always accompanied by a marker. + # Simulate a crash at the marker write and assert no marker-less image + # survives. + image = b"ext4-image-bytes" + payload = zstandard.ZstdCompressor().compress(image) + digest = hashlib.sha256(payload).hexdigest() + url = rootfs_download.release_url("14", "abc123def456") + + def fake_urlopen(req_url: str, timeout: float) -> _FakeResp: + if req_url == url: + return _FakeResp(payload) + return _FakeResp(f"{digest} rootfs-14-abc123def456.img.zst\n".encode()) + + monkeypatch.setattr("urllib.request.urlopen", fake_urlopen) + + out = tmp_path / "rootdisk.img" + marker_path = out.with_name(out.name + ".android-version") + real_write_text = Path.write_text + + def boom_on_marker(self: Path, *args: object, **kwargs: object) -> int: + if self == marker_path: + raise OSError("disk full") + return real_write_text(self, *args, **kwargs) # type: ignore[arg-type] + + monkeypatch.setattr(Path, "write_text", boom_on_marker) + with pytest.raises(OSError, match="disk full"): + rootfs_download.fetch_prebuilt( + android_version=14, + fingerprint="abc123def456", + out_image=out, + docker_version="27.5.1", + ) + # Because the marker is written first, the crash happens before the rename: + # no marker-less image is left installed. + assert not out.exists() diff --git a/tests/test_snapshot.py b/tests/test_snapshot.py index 8ec8794..b6dd169 100644 --- a/tests/test_snapshot.py +++ b/tests/test_snapshot.py @@ -314,6 +314,46 @@ def test_force_refuses_descendant_of_registered_vm_instance_dir( assert (vm_dir / "data" / "marker.txt").read_bytes() == (b"vm nested data") assert registry.get("beta") is None + def test_refuses_nonexistent_descendant_of_registered_redroid_dir( + self, isolated_registry: Path, tmp_path: Path + ) -> None: + # #172: the overlap guard used to be gated behind ``occupied``, so + # a restore into a NON-EXISTENT subdir of a registered instance + # (no --force) slipped past it and registered a nested instance. + # The guard now runs unconditionally, so the brand-new nested + # path is refused before any mkdir / registry write. + src = _make_instance(tmp_path / "alpha") + registry.add_allocating("alpha", src) + archive = snapshot.snapshot(src, tmp_path / "out") + + phone = _make_instance(tmp_path / "phone", data_bytes=b"redroid precious data") + registry.add_allocating("phone", phone) + + with pytest.raises(snapshot.SnapshotError, match="phone"): + snapshot.restore(archive, dest_name="beta", dest_path=phone / "sub") + assert not (phone / "sub").exists() + assert registry.get("beta") is None + + def test_refuses_nonexistent_descendant_of_registered_vm_dir( + self, isolated_registry: Path, tmp_path: Path + ) -> None: + # #172 for the vm backend: the unconditional overlap loop fires on + # the VmBackendConfig arm too, for a not-yet-existent nested path. + src = _make_instance(tmp_path / "alpha") + registry.add_allocating("alpha", src) + archive = snapshot.snapshot(src, tmp_path / "out") + + vm_dir = _make_instance(tmp_path / "vmphone", data_bytes=b"vm nested data") + registry.add_allocating( + "vmphone", + backend=registry.VmBackendConfig(absolute_path=str(vm_dir)), + ) + + with pytest.raises(snapshot.SnapshotError, match="vmphone"): + snapshot.restore(archive, dest_name="beta", dest_path=vm_dir / "sub") + assert not (vm_dir / "sub").exists() + assert registry.get("beta") is None + def test_empty_existing_dir_is_allowed_without_force( self, isolated_registry: Path, tmp_path: Path ) -> None: @@ -364,6 +404,56 @@ def test_force_corrupted_archive_does_not_destroy_target( assert registry.get("beta") is None +_VM_YAML = "api_version: 3\nandroid:\n version: 14\nbinder: vm\n" + + +class TestRestoreReconcilesArchivedBinder: + def test_restore_rejects_archived_binder_vm_and_rolls_back( + self, isolated_registry: Path, tmp_path: Path + ) -> None: + # #171: restore() always registers a RedroidBackendConfig row, but + # the EXTRACTED beetroot.yaml is the source of truth for the + # backend. An archive whose config sets ``binder: vm`` (e.g. an + # unapplied edit on the source — the registry row stays redroid) + # would otherwise restore as a redroid row and dispatch the wrong + # backend silently. Restore must refuse it (snapshots are + # redroid-only, #128) and roll back the half-registered row + + # extracted directory. + src = tmp_path / "alpha" + src.mkdir(parents=True) + (src / "beetroot.yaml").write_text(_VM_YAML) + (src / "data").mkdir() + (src / "data" / "marker.txt").write_bytes(b"hello") + # Registered as redroid, simulating the unapplied-edit state where + # the YAML already says vm but the registry row is still redroid. + registry.add_allocating("alpha", src) + archive = snapshot.snapshot(src, tmp_path / "out") + registry.remove("alpha") + + target = tmp_path / "beta" + with pytest.raises(snapshot.SnapshotError, match="vm"): + snapshot.restore(archive, dest_name="beta", dest_path=target) + + # The half-registered row was torn down and the created directory + # removed — no nested redroid row masquerading over a vm config. + assert registry.get("beta") is None + assert not target.exists() + + def test_restore_succeeds_for_redroid_archive( + self, isolated_registry: Path, tmp_path: Path + ) -> None: + # Control: a normal redroid archive still restores cleanly through + # the new binder-reconciliation guard. + src = _make_instance(tmp_path / "alpha") + registry.add_allocating("alpha", src) + archive = snapshot.snapshot(src, tmp_path / "out") + registry.remove("alpha") + + restored = snapshot.restore(archive, dest_name="beta", dest_path=tmp_path / "beta") + assert restored == (tmp_path / "beta").resolve() + assert registry.get("beta") is not None + + class TestRestoreErrors: def test_dest_name_already_registered(self, isolated_registry: Path, tmp_path: Path) -> None: src = _make_instance(tmp_path / "alpha") From e2e7823f76c6b83d99deaed5bf9f28aa43cd98a4 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 30 Jun 2026 19:04:16 +0000 Subject: [PATCH 2/2] test(adb): cover the empty-adb-error branch of the #223 offline-abort row MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The #223 fix added an `if adb_error:` branch; the falsy branch (empty underlying adb error → bare stage-neutral detail) was unexercised, dropping coverage to 99.98% and failing the CI 100% gate. Add a focused unit test. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01P1YsNMpGizhSDDPCBu5cut --- tests/test_bugfix_adb_skip_count.py | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/tests/test_bugfix_adb_skip_count.py b/tests/test_bugfix_adb_skip_count.py index 973fbbb..469d897 100644 --- a/tests/test_bugfix_adb_skip_count.py +++ b/tests/test_bugfix_adb_skip_count.py @@ -69,3 +69,28 @@ def _fake_install_one(self: adb_backend.AdbDevice, source: str, sha256: object, # skipped covers 2 → len(results) + skipped == len(sources). skipped = 1 assert len(results) + skipped == len(sources) + + +def test_offline_abort_detail_without_adb_error_text( + monkeypatch: pytest.MonkeyPatch, +) -> None: + # #223: when the underlying adb error text is empty, the offline-abort + # row's detail is the bare stage-neutral message with no "last adb error" + # suffix (covers the falsy-``adb_error`` branch). + monkeypatch.setattr(shutil, "which", lambda name: f"/usr/bin/{name}") + monkeypatch.setattr(adb_backend.AdbDevice, "_preflight_root_and_magisk", lambda self: None) + + def _raise_empty(self: adb_backend.AdbDevice, source: str, sha256: object, index: int) -> str: + del self, source, sha256, index + raise RuntimeError("") + + monkeypatch.setattr(adb_backend.AdbDevice, "_auto_install_one", _raise_empty) + monkeypatch.setattr(adb_backend, "serial_is_available", lambda serial: False) + + with pytest.raises(api.DevicePreflightError) as exc_info: + _make_device().auto_install_modules(["a.zip"]) + + row = exc_info.value.results[0] + assert row.ok is False + assert row.detail == "device went offline during this module" + assert "last adb error" not in row.detail