diff --git a/.github/workflows/pr-checks.yml b/.github/workflows/pr-checks.yml index afd0118..17bf00a 100644 --- a/.github/workflows/pr-checks.yml +++ b/.github/workflows/pr-checks.yml @@ -148,6 +148,16 @@ jobs: ids="$(./scripts/changed-apps.sh "$base" HEAD | tr '\n' ' ')" echo "ids=$ids" >> "$GITHUB_OUTPUT" echo "changed apps: ${ids:-}" + # Apps this PR introduces: nothing existed under registry// at base. + # The apply_extra check downloads the full pinned artifact (hundreds of + # MB for some apps), so it is scoped to new submissions — the moment a + # never-before-installed unpack script first meets review. + new_ids="" + for id in $ids; do + git cat-file -e "$base:registry/$id" 2>/dev/null || new_ids="$new_ids $id" + done + echo "new_ids=${new_ids# }" >> "$GITHUB_OUTPUT" + echo "new apps: ${new_ids:-}" - uses: actions/setup-node@v6 if: steps.changed.outputs.ids != '' with: @@ -170,6 +180,14 @@ jobs: ./scripts/build-app.sh "$id" echo "::endgroup::" done + # Building proves the manifest is sound; it says nothing about whether the + # app can be *installed*. extra-data is fetched on the user's machine and + # unpacked by apply_extra, which a system-wide install runs as root with + # --cap-drop ALL — a path neither the build nor a --user install exercises. + # Runs after the build so the runtime is already installed. + - name: Check apply_extra under a system-wide install (new apps only) + if: steps.changed.outputs.new_ids != '' + run: ./scripts/check-apply-extra.sh ${{ steps.changed.outputs.new_ids }} - name: Bundle changed apps into installable .flatpak files if: steps.changed.outputs.ids != '' run: | diff --git a/docs/packaging-playbook.md b/docs/packaging-playbook.md index d1e53f2..c5c0aac 100644 --- a/docs/packaging-playbook.md +++ b/docs/packaging-playbook.md @@ -147,17 +147,12 @@ Detail + schema in the [contributing guide](https://flatpark.org/contributing/). 3. Install from the signed local repo into an **isolated** `--installation=test` so it never pollutes your everyday Flatpak state (recipe in the contributing guide). Installing exercises `apply_extra` (the extra-data download + unpack). - A custom installation runs `apply_extra` as **you**, so it never exercises the root path a - system-wide install takes. To cover that too, run the script under the same constraints - Flatpak imposes there — uid 0, no capabilities: - ```sh - RT=$(ls -d ~/.local/share/flatpak/runtime/org.freedesktop.Platform/x86_64/25.08/*/files) - bwrap --unshare-user --uid 0 --gid 0 --cap-drop ALL \ - --ro-bind "$RT" /usr --symlink usr/bin /bin --symlink usr/lib /lib --symlink usr/lib64 /lib64 \ - --bind /app/extra --chdir /app/extra --dev /dev --tmpfs /tmp \ - /bin/sh -c 'sh /app/extra/apply_extra.sh; echo rc=$?' - ``` -4. **Smoke-test the actual launch:** the app must start, and its data must land inside the +4. `scripts/check-apply-extra.sh ` — runs `apply_extra` as **root with every capability + dropped**, which is what a *system-wide* install does and what step 3 never reaches (a + custom installation runs it as you). It downloads the pinned artifact, checks its sha256, + and unpacks it in that sandbox. `pr-checks` runs this for apps a PR **adds**; run it + yourself whenever you touch an unpack script or re-pin to an artifact from a new builder. +5. **Smoke-test the actual launch:** the app must start, and its data must land inside the sandbox (`~/.var/app//…`). Confirm no missing libs (`LD_TRACE_LOADED_OBJECTS=1` → 0 "not found"). Note in the PR anything you *couldn't* verify (GUI render on a real session, login flows, hardware paths) — coverage is always partial and honesty about that matters. diff --git a/scripts/check-apply-extra.sh b/scripts/check-apply-extra.sh new file mode 100755 index 0000000..dd194c8 --- /dev/null +++ b/scripts/check-apply-extra.sh @@ -0,0 +1,113 @@ +#!/usr/bin/env bash +# Run an app's apply_extra.sh the way a *system-wide* install runs it: as root, +# with every capability dropped, offline, against the real pinned artifact. +# +# Why this exists. `flatpak install` (system) executes apply_extra under +# flatpak-system-helper as root with `--cap-drop ALL`, so the script cannot +# chown to whatever uid the upstream archive happens to record. bsdtar and tar +# both restore ownership by default under uid 0; the resulting EPERM is fatal +# under `set -e` even though every member extracted correctly, and the user sees +# only `apply_extra script failed, exit status 256` (512 for GNU tar) with the +# real message buried in the system helper's journal. +# +# Nothing else in CI reaches this code path: build-app.sh only *builds*, and +# extra-data is fetched at install time, so a green build says nothing about +# whether the app can actually be installed system-wide. A `--user` install +# won't catch it either — there the script runs as the invoking user and +# ownership is never restored. +# +# This does not need root itself: `bwrap --unshare-user --uid 0` gives an +# unprivileged user namespace whose root has no capabilities over the host, and +# a chown to an unmapped uid fails there just as a dropped CAP_CHOWN does under +# the real system helper (EINVAL rather than EPERM; same failure branch in +# libarchive and tar). +# +# Usage: check-apply-extra.sh ... +ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)" +. "$ROOT/scripts/lib/common.sh" +load_config "$ROOT" +need flatpak; need bwrap; need curl; need sha256sum; need node + +[ "$#" -gt 0 ] || die "usage: check-apply-extra.sh ..." + +arch="$(flatpak --default-arch)" + +check_one() { + load_app "$1" + local apply="$APP_SRC/apply_extra.sh" + if [ ! -f "$apply" ]; then + log "$APP_ID: no apply_extra.sh, nothing to check" + return 0 + fi + + local runtime="" runtime_version="" sources=() kind a b c + while IFS=$'\t' read -r kind a b c; do + case "$kind" in + runtime) runtime="$a"; runtime_version="$b" ;; + extra) sources+=("$a"$'\t'"$b"$'\t'"$c") ;; + esac + done < <(node "$ROOT/scripts/read-extra-data.mjs" "$MANIFEST" "$arch") + + if [ "${#sources[@]}" -eq 0 ]; then + log "$APP_ID: no extra-data for $arch, nothing to check" + return 0 + fi + + # The runtime must already be installed; apply_extra runs against its /usr. + local loc files + loc="$(flatpak info --show-location "$runtime//$runtime_version" 2>/dev/null)" \ + || die "$APP_ID: runtime not installed: $runtime//$runtime_version" + files="$loc/files" + [ -d "$files" ] || die "$APP_ID: runtime has no files dir: $files" + + # check_one always runs in a subshell, so an EXIT trap is scoped to this app + # and still fires on the `die` paths below. The log lives outside $work: + # apply_extra deletes the extra-data it consumed, and would take the log too. + local work log_file + work="$(mktemp -d)" + log_file="$(mktemp)" + # shellcheck disable=SC2064 + trap "rm -rf '$work' '$log_file'" EXIT + + local src filename url want got + for src in "${sources[@]}"; do + IFS=$'\t' read -r filename url want <<<"$src" + log "$APP_ID: fetching $filename" + curl -fsSL --retry 3 --max-time 900 -o "$work/$filename" "$url" \ + || die "$APP_ID: download failed: $url" + got="$(sha256sum "$work/$filename" | cut -d' ' -f1)" + [ "$got" = "$want" ] || die "$APP_ID: sha256 mismatch for $filename (pinned $want, got $got)" + done + + cp "$apply" "$work/apply_extra.sh" + + # Mirror flatpak's apply_extra sandbox: uid 0, no caps, no network, no /proc + # (flatpak passes FLATPAK_RUN_FLAG_NO_PROC there), runtime at /usr, the + # extra-data dir bound read-write at /app/extra. + local rc=0 + bwrap \ + --unshare-user --unshare-pid --unshare-net --die-with-parent \ + --uid 0 --gid 0 --cap-drop ALL \ + --ro-bind "$files" /usr \ + --symlink usr/bin /bin --symlink usr/sbin /sbin \ + --symlink usr/lib /lib --symlink usr/lib64 /lib64 \ + --bind "$work" /app/extra --chdir /app/extra \ + --dev /dev --tmpfs /tmp \ + /bin/sh -c 'sh /app/extra/apply_extra.sh' >"$log_file" 2>&1 || rc=$? + + if [ "$rc" -ne 0 ]; then + # A chown failure names every member; the tail is enough to identify it. + warn "$APP_ID: last lines of apply_extra output:" + tail -n 20 "$log_file" | sed 's/^/ /' >&2 + die "$APP_ID: apply_extra failed under a system-wide install (exit $rc; flatpak would report 'exit status $((rc * 256))'). If it unpacks an archive, it must do so with --no-same-owner." + fi + log "$APP_ID: apply_extra OK as root with no capabilities" +} + +# Each app in its own subshell so load_app's globals and the tmpdir trap don't +# leak between them — and so one failure still reports the rest. +fail=0 +for id in "$@"; do + ( check_one "$id" ) || fail=1 +done +exit "$fail" diff --git a/scripts/read-extra-data.mjs b/scripts/read-extra-data.mjs new file mode 100644 index 0000000..5153dbe --- /dev/null +++ b/scripts/read-extra-data.mjs @@ -0,0 +1,85 @@ +#!/usr/bin/env node +// Read a flatpak manifest and print the runtime plus the extra-data sources +// that apply to one arch, as tab-separated lines a shell can read: +// +// runtime +// extra +// +// Like read-descriptor.mjs this is NOT a general YAML parser. Manifests here +// are generated and reviewed against a fixed shape, so we extract the few keys +// check-apply-extra.sh needs and stay dependency-free. +// +// Usage: read-extra-data.mjs +import { readFileSync } from 'node:fs'; + +const [file, arch] = process.argv.slice(2); +if (!file || !arch) { + process.stderr.write('usage: read-extra-data.mjs \n'); + process.exit(2); +} + +let lines; +try { + lines = readFileSync(file, 'utf8').split('\n'); +} catch (e) { + process.stderr.write(`cannot read ${file}: ${e.message || e}\n`); + process.exit(1); +} + +const strip = (v) => v.trim().replace(/^["'](.*)["']$/, '$1'); + +let runtime = ''; +let runtimeVersion = ''; +const sources = []; +let cur = null; +let inArches = false; + +for (const raw of lines) { + const t = raw.trim(); + if (!t || t.startsWith('#')) continue; + + let m; + // Top-level keys only: a nested `runtime:` under build-options must not win. + if (!/^\s/.test(raw)) { + if ((m = raw.match(/^runtime:\s*(.+)$/))) runtime = strip(m[1]); + else if ((m = raw.match(/^runtime-version:\s*(.+)$/))) runtimeVersion = strip(m[1]); + } + + if (t === '- type: extra-data') { + cur = { arches: [] }; + sources.push(cur); + inArches = false; + continue; + } + // Any other list entry ends the record we were filling. + if (t.startsWith('- type:')) { + cur = null; + inArches = false; + continue; + } + if (!cur) continue; + + if (t === 'only-arches:') { inArches = true; continue; } + if (inArches && (m = t.match(/^-\s*(\S+)$/))) { cur.arches.push(strip(m[1])); continue; } + inArches = false; + + if ((m = t.match(/^filename:\s*(.+)$/))) cur.filename = strip(m[1]); + else if ((m = t.match(/^url:\s*(.+)$/))) cur.url = strip(m[1]); + else if ((m = t.match(/^sha256:\s*(.+)$/))) cur.sha256 = strip(m[1]); +} + +if (!runtime || !runtimeVersion) { + process.stderr.write(`${file}: no top-level runtime / runtime-version\n`); + process.exit(1); +} + +const out = [`runtime\t${runtime}\t${runtimeVersion}`]; +for (const s of sources) { + if (s.arches.length && !s.arches.includes(arch)) continue; + if (!s.filename || !s.url || !s.sha256) { + process.stderr.write(`${file}: extra-data source missing filename/url/sha256\n`); + process.exit(1); + } + out.push(`extra\t${s.filename}\t${s.url}\t${s.sha256}`); +} +process.stdout.write(out.join('\n') + '\n'); diff --git a/tests/test_check_apply_extra.sh b/tests/test_check_apply_extra.sh new file mode 100755 index 0000000..7939469 --- /dev/null +++ b/tests/test_check_apply_extra.sh @@ -0,0 +1,108 @@ +#!/usr/bin/env bash +# check-apply-extra.sh must reject an apply_extra.sh that unpacks without +# --no-same-owner, and accept the same script once the flag is there. +# +# Hermetic: the payload is a zip we build here carrying an Info-ZIP 0x7875 +# extra field with uid/gid 1001 — exactly what a CI-built release archive looks +# like — and it is served over file://, so the test never hits the network. +set -euo pipefail +ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)" +. "$ROOT/tests/lib/assert.sh" + +command -v flatpak >/dev/null || { echo "test_check_apply_extra: SKIP (no flatpak)"; exit 0; } +command -v bwrap >/dev/null || { echo "test_check_apply_extra: SKIP (no bwrap)"; exit 0; } +command -v python3 >/dev/null || { echo "test_check_apply_extra: SKIP (no python3)"; exit 0; } +flatpak info --show-location org.freedesktop.Platform//25.08 >/dev/null 2>&1 \ + || { echo "test_check_apply_extra: SKIP (no org.freedesktop.Platform//25.08)"; exit 0; } + +tmp="$(mktemp -d)"; trap 'rm -rf "$tmp"' EXIT +id="io.flatpark.ApplyExtra" +app="$tmp/registry/$id" +mkdir -p "$app" + +# A zip whose members record uid=gid=1001, like any archive built on CI without +# fakeroot. Under uid 0 an unpacker restores that ownership unless told not to. +python3 - "$tmp/payload.zip" <<'PY' +import struct, sys, zipfile +def unix_uid_gid(uid, gid): + body = b'\x01' + b'\x02' + struct.pack('"$app/flatpark.yml" <"$app/$id.yml" <"$app/apply_extra.sh" <"$tmp/log" 2>&1; } + +# 1. Without the flag the unpack must be rejected: bsdtar cannot chown to 1001 +# as an uncapable root, and exits nonzero after extracting everything. +write_apply "" +if run_check; then + echo "FAIL: check-apply-extra accepted an unpack without --no-same-owner" + cat "$tmp/log"; exit 1 +fi +assert_contains "$tmp/log" "--no-same-owner" + +# 2. With the flag it must pass. +write_apply "--no-same-owner" +if ! run_check; then + echo "FAIL: check-apply-extra rejected a correct unpack" + cat "$tmp/log"; exit 1 +fi +assert_contains "$tmp/log" "apply_extra OK" + +# 3. An app with no apply_extra.sh is skipped, not failed. +rm "$app/apply_extra.sh" +sed -i 's#install -Dm755 apply_extra.sh /app/bin/apply_extra#true#' "$app/$id.yml" +assert_ok run_check +assert_contains "$tmp/log" "nothing to check" + +echo "test_check_apply_extra: PASS"