mirror of
https://github.com/Heretek-Games/flatpark.git
synced 2026-10-12 02:05:52 +00:00
ci: exercise apply_extra as an uncapable root for newly added apps
Nothing in CI reached the code path that broke six apps for every user who installed system-wide. build-app.sh only builds, and extra-data is fetched at install time, so a green build says nothing about whether an app installs. A --user install misses it too: there apply_extra runs as the invoking user, and the ownership restore that fails under root never happens. scripts/check-apply-extra.sh closes that gap. It downloads the pinned artifact, verifies its sha256, and runs the app's real apply_extra.sh in a sandbox mirroring what flatpak-system-helper imposes: uid 0, all capabilities dropped, no network, no /proc, runtime at /usr, extra-data bound at /app/extra. bwrap --unshare-user gives us that without needing root: a chown to an unmapped uid fails there just as a dropped CAP_CHOWN does on the real thing. pr-checks runs it for apps a PR *adds* -- the moment an unpack script and an upstream builder's uid first meet review. Scoping it there keeps the artifact download (hundreds of MB for some apps) off every pin bump and off refactors like the one that prompted this. The playbook asks contributors to run it by hand whenever they touch an unpack script or re-pin to an artifact from a new builder, which is the gap that remains. tests/test_check_apply_extra.sh is hermetic: it builds a zip carrying an Info-ZIP 0x7875 field with uid/gid 1001 -- what a release archive built on CI without fakeroot looks like -- serves it over file://, and asserts the checker rejects an unpack without --no-same-owner and accepts one with it. Verified it fails against a mutant checker that ignores the exit code, so it is testing what it claims to. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
This commit is contained in:
Komh
@@ -148,6 +148,16 @@ jobs:
|
||||
ids="$(./scripts/changed-apps.sh "$base" HEAD | tr '\n' ' ')"
|
||||
echo "ids=$ids" >> "$GITHUB_OUTPUT"
|
||||
echo "changed apps: ${ids:-<none>}"
|
||||
# Apps this PR introduces: nothing existed under registry/<id>/ 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:-<none>}"
|
||||
- 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: |
|
||||
|
||||
@@ -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 <dir-with-the-artifact> /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 <id>` — 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/<id>/…`). 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.
|
||||
|
||||
Executable
+113
@@ -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 <app-id>...
|
||||
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 <app-id>..."
|
||||
|
||||
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"
|
||||
@@ -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<TAB><id><TAB><version>
|
||||
// extra<TAB><filename><TAB><url><TAB><sha256>
|
||||
//
|
||||
// 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 <manifest.yml> <arch>
|
||||
import { readFileSync } from 'node:fs';
|
||||
|
||||
const [file, arch] = process.argv.slice(2);
|
||||
if (!file || !arch) {
|
||||
process.stderr.write('usage: read-extra-data.mjs <manifest.yml> <arch>\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');
|
||||
Executable
+108
@@ -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('<H', uid) + b'\x02' + struct.pack('<H', gid)
|
||||
return struct.pack('<HH', 0x7875, len(body)) + body
|
||||
with zipfile.ZipFile(sys.argv[1], 'w') as z:
|
||||
for name in ('payload.txt', 'nested/other.txt'):
|
||||
zi = zipfile.ZipInfo(name)
|
||||
zi.external_attr = 0o100644 << 16
|
||||
zi.extra = unix_uid_gid(1001, 1001)
|
||||
z.writestr(zi, 'x')
|
||||
PY
|
||||
sha="$(sha256sum "$tmp/payload.zip" | cut -d' ' -f1)"
|
||||
|
||||
cat >"$app/flatpark.yml" <<EOF
|
||||
id: $id
|
||||
name: Apply Extra
|
||||
summary: Synthetic apply_extra fixture
|
||||
build:
|
||||
manifest: $id.yml
|
||||
branch: stable
|
||||
mode: extra-data
|
||||
catalog:
|
||||
category: Utilities
|
||||
EOF
|
||||
|
||||
cat >"$app/$id.yml" <<EOF
|
||||
id: $id
|
||||
runtime: org.freedesktop.Platform
|
||||
runtime-version: '25.08'
|
||||
sdk: org.freedesktop.Sdk
|
||||
command: apply-extra
|
||||
modules:
|
||||
- name: apply-extra
|
||||
buildsystem: simple
|
||||
build-commands:
|
||||
- install -Dm755 apply_extra.sh /app/bin/apply_extra
|
||||
sources:
|
||||
- type: extra-data
|
||||
filename: payload.zip
|
||||
only-arches:
|
||||
- $(flatpak --default-arch)
|
||||
url: file://$tmp/payload.zip
|
||||
sha256: $sha
|
||||
size: $(stat -c%s "$tmp/payload.zip")
|
||||
EOF
|
||||
|
||||
write_apply() { # $1 = extra bsdtar flags
|
||||
cat >"$app/apply_extra.sh" <<EOF
|
||||
#!/bin/sh
|
||||
set -eu
|
||||
cd "\${EXTRA_ROOT:-/app/extra}"
|
||||
rm -rf out; mkdir out
|
||||
bsdtar $1 -xf payload.zip -C out
|
||||
rm -f payload.zip
|
||||
EOF
|
||||
}
|
||||
|
||||
run_check() { env REGISTRY_DIR="$tmp/registry" "$ROOT/scripts/check-apply-extra.sh" "$id" >"$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"
|
||||
Reference in New Issue
Block a user