From 1381b052ae0bb64af94c584e72020158ccd6a791 Mon Sep 17 00:00:00 2001 From: librelad Date: Thu, 27 Aug 2026 11:33:52 +0100 Subject: [PATCH] =?UTF-8?q?fix(app):=20make=20export/import=20actually=20w?= =?UTF-8?q?ork=20=E2=80=94=20found=20by=20a=20real=20round=20trip?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Ran export -> uninstall -> import on trivy against the live install. It worked end to end (1.3G app, marker file byte-identical afterwards, container running, database status correct, tree owned by the container user) but only after three real bugs, none of which syntax checks or isolated tests would have caught. Export wrote the tarball as the CONTAINER user, because tar has to read app data holding sub-UIDs the manager cannot. That meant the container user also had to be able to create the destination file, which fails for any normal destination. Now tar writes to stdout and the caller's shell creates the file: reading uses the privileges that need it, writing uses the caller's. Import had the mirror-image bug — tar extracted as the container user and so could not READ a manager-owned .lpapp; the caller now opens it and tar reads stdin. Export also failed at tar time with no hint that the destination was the problem, so it checks the directory exists and is writable up front. The third one was quiet and worse. The manifest is pretty-printed, so it reads `"size_bytes": 1324973614` — with a space that a `"key":[0-9]*` pattern does not match. Both size_bytes and storage.location came back empty everywhere they were read, which turned "will it fit" and "does that location still exist" into checks that always passed. That is the failure mode preflight exists to prevent, hiding inside preflight itself. Fixed in app_portable.sh and restore_preflight.sh. Verified afterwards with crafted manifests: an app claiming 8 TB is now refused on an 800 GB disk ("Needs 8192G, 806G free"), and one naming a location this machine lacks warns and names the fallback. Co-Authored-By: Claude Opus 5 --- scripts/app/app_portable.sh | 44 +++++++++++++++++++++++----- scripts/restore/restore_preflight.sh | 6 +++- 2 files changed, 41 insertions(+), 9 deletions(-) diff --git a/scripts/app/app_portable.sh b/scripts/app/app_portable.sh index 454fae9..540148e 100644 --- a/scripts/app/app_portable.sh +++ b/scripts/app/app_portable.sh @@ -57,6 +57,17 @@ appExport() isError "'$out' already exists — refusing to overwrite it." return 1 fi + # Check up front rather than failing at tar time: the error there is a bare + # non-zero exit with no hint that the destination was the problem. + local out_dir="${out%/*}"; [[ -z "$out_dir" ]] && out_dir="." + if [[ ! -d "$out_dir" ]]; then + isError "'$out_dir' does not exist." + return 1 + fi + if [[ ! -w "$out_dir" ]]; then + isError "Cannot write to '$out_dir' — pick somewhere $(id -un) can write." + return 1 + fi isHeader "Export $app" @@ -74,15 +85,21 @@ appExport() fi isNotice "Writing $out" - # Run as the owner: app data holds container sub-UIDs the manager cannot - # read. --numeric-owner so those uids survive the round trip rather than - # being remapped through this machine's /etc/passwd. - if ! runFileOp tar --numeric-owner -C "${dir%/*}" -czf "$out" "$app" 2>/dev/null; then + # Split the privileges deliberately: tar READS as the container user, because + # app data holds sub-UIDs the manager cannot read — but it writes to STDOUT, + # and the destination file is created by this shell, as whoever ran the + # command. Writing directly from tar meant the container user had to be able + # to create the file too, which fails for any normal destination. + # + # --numeric-owner so container uids survive the round trip rather than being + # remapped through this machine's /etc/passwd. + if ! runFileOp tar --numeric-owner -C "${dir%/*}" -czf - "$app" > "$out" 2>/dev/null; then isError "Export failed." + rm -f "$out" (( was_running )) && dockerComposeUp "$app" >/dev/null 2>&1 return 1 fi - runFileOp chmod 0640 "$out" 2>/dev/null + chmod 0640 "$out" 2>/dev/null if (( was_running )); then isNotice "Starting $app again." @@ -156,7 +173,12 @@ appImportCheck() continue fi - manifest=$(appImportManifest "$f") + # Squeeze whitespace first. The manifest is PRETTY-PRINTED, so the real + # text is `"size_bytes": 1324973614` — with a space that a + # `"key":[0-9]*` pattern does not match. Both the size and the location + # silently came back empty, which turned the "will it fit" and "does + # that location exist" checks into no-ops that always passed. + manifest=$(appImportManifest "$f" | tr -d ' \n\t') size_bytes=$(printf '%s' "$manifest" | grep -o '"size_bytes":[0-9]*' | head -1 | cut -d: -f2) loc=$(printf '%s' "$manifest" | grep -o '"location":"[^"]*"' | head -1 | cut -d'"' -f4) [[ "$size_bytes" =~ ^[0-9]+$ ]] || size_bytes=0 @@ -264,7 +286,8 @@ appImport() # --- the same checks the restore preflight makes ------------------------- local manifest size_bytes loc - manifest=$(appImportManifest "$file") + # Whitespace-squeezed: the manifest is pretty-printed (see appImportCheck). + manifest=$(appImportManifest "$file" | tr -d ' \n\t') size_bytes=$(printf '%s' "$manifest" | grep -o '"size_bytes":[0-9]*' | head -1 | cut -d: -f2) loc=$(printf '%s' "$manifest" | grep -o '"location":"[^"]*"' | head -1 | cut -d'"' -f4) @@ -300,7 +323,12 @@ appImport() # --- unpack --------------------------------------------------------------- isNotice "Unpacking into $dir" runFileOp mkdir -p "${dir%/*}" - if ! runFileOp tar --numeric-owner -xzf "$file" -C "${dir%/*}" 2>/dev/null; then + # Mirror of the export split: the caller's shell opens the .lpapp (it may sit + # anywhere the user can read), and tar EXTRACTS as the container user so the + # unpacked tree lands with the right ownership. Passing the path to tar + # instead required the container user to be able to read the file, which + # fails for any file the user created themselves. + if ! runFileOp tar --numeric-owner -xzf - -C "${dir%/*}" < "$file" 2>/dev/null; then isError "Unpack failed — removing the partial directory." runFileOp rm -rf "$dir" return 1 diff --git a/scripts/restore/restore_preflight.sh b/scripts/restore/restore_preflight.sh index b21bb12..5483fa2 100644 --- a/scripts/restore/restore_preflight.sh +++ b/scripts/restore/restore_preflight.sh @@ -61,7 +61,11 @@ restorePreflightApp() local idx="$1" app="$2" host="$3" local manifest size_bytes size_h loc want_dir avail_kb need_kb - manifest=$(restorePreflightManifest "$idx" "$app" "$host" 2>/dev/null) + # Squeezed: the manifest is pretty-printed, so `"size_bytes": 123` carries a + # space that a `"key":[0-9]*` pattern misses — which silently emptied both + # the size and the location, turning the fit and location checks into + # no-ops that always passed. + manifest=$(restorePreflightManifest "$idx" "$app" "$host" 2>/dev/null | tr -d ' \n\t') size_bytes=$(printf '%s' "$manifest" | grep -o '"size_bytes":[0-9]*' | head -1 | cut -d: -f2) loc=$(printf '%s' "$manifest" | grep -o '"location":"[^"]*"' | head -1 | cut -d'"' -f4) size_h=$(_restorePfSize "${size_bytes:-0}")