Repository navigation
Security hardening, test suite, and quality-of-life features from the review - #12
Merged
Merged
Conversation
Three fixes from a security/robustness review, plus the test suite that proves them. Backup archives were matched by prefix glob: with apps vault and vault2, BACKUP_KEEP pruning for vault deleted vault2's archives (silent data loss in the backup tool) and restore vault picked vault2's newer archive. Archive lookups now go through list_archives, which validates the full name - app, separator or legacy no-separator, exact date - so archives can never cross app boundaries. New archives are written as app_YYYY-MM-DD.tar.bz2; the old form still restores. The step log was one mktemp'd file that the interactive menu's per-command cleanup deleted, freeing a /tmp name another local user had already seen - a symlink planted there would have the next root-run step truncate whatever it pointed at. All scratch files now live in a mktemp -d 0700 directory that survives menu commands (the subshell keeps it; the owning shell removes it). The run lock was a predictable name in world-writable /tmp: any local user could pre-create it and permanently lock the tool (sticky /tmp means the victim can't remove it). It now lives in the managed folder itself, records the owning pid, and clears itself when that process is gone - a crashed run no longer demands manual lock removal. tests/run-tests.sh drives manage.sh against a stub docker - no daemon - asserting on exit codes, output, and the exact STOP/UP/pull actions taken: the stopped-policy matrix, carrying on past failures, backup/ restore roundtrips, the collision case above, legacy archive names and both lock behaviours. Run against the pre-fix script it fails 6 assertions; CI runs it on every push and PR. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
notify() interpolated app names and error text straight into its JSON payload, so a quote or backslash in either broke - or quietly rewrote - what reached the webhook. Content now goes through a small escaper (backslash, quote, tab, newline) and the payload stays valid JSON whatever flows in. The release workflow publishes manage.sh.sha256 beside the notes, and update-self verifies the downloaded script against it before swapping it in, with whichever of sha256sum/shasum/openssl the host has. Releases without the asset (everything before 0.4.1) install as before. Since the tag is the distribution channel this mostly guards against corrupted or truncated downloads, but it costs nothing at install time. update-self also now reads the API error body, so an exhausted GitHub rate limit says so instead of the misleading "Are you online?". Both workflows pin actions/checkout to a commit SHA instead of a floating tag. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Updating swaps every app onto a new image and leaves the old ones behind as dangling layers - across 30 apps updating weekly that's a disk quietly filling until docker's own "no space left" failure, which this script then hints about. --prune (or PRUNE_AFTER_UPDATE=1 in manage.conf) runs docker image prune -f after the update tally and reports the space reclaimed; a standalone `prune` command (also in the menu) does the same on demand, confirming first on a terminal. Only dangling images are touched. restore can now reach every archive BACKUP_KEEP retains instead of only the newest: a date-shaped argument picks that day's archive (restore linkace 2026-08-01, with _HHMMSS for same-day backups), a wrong date lists what exists, and a bare restore still takes the newest while mentioning how many older archives are available. Both come with harness coverage: prune firing with the flag and not without, and dated/undated/missing-date restores against seeded archives with distinct contents. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
An app that started but never reported healthy within HEALTH_TIMEOUT warned inline and then counted as a success - the closing tally said "Updated all 30 apps" and the exit status was 0, so a cron job had no way to hear about a service that came up broken. wait_healthy now files such apps in an unhealthy bucket of their own: the summary row says unhealthy, the tally reads "Updated 27 of 30 apps - 1 failed, 1 unhealthy, 1 skipped" with the names beneath, and the run exits non-zero. It stays a separate bucket from failed because the distinction matters when deciding what to do next: failed apps didn't complete their command; unhealthy apps are running on the new image but their healthcheck disagrees that they're well. The stub docker learned to answer health inspections from an 'unhealthy' list, and the harness covers the bucket, the exit status and that the app genuinely was started. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
--backup-first on update archives every app on its way to the new image. The backup flow already ends each app on the freshly pulled image, so this is the two commands composed rather than a third path through stop/start - update honestly announces the delegation. doctor now reports free disk space where backups are written (an archive that hits a full disk fails late and messily) and warns when manage.conf is writable by group or others - the file is sourced as shell, so write access to it is command execution as the user running the script. manage.conf.example documents the settings added since it was written: STOPPED_POLICY and PRUNE_AFTER_UPDATE. README covers --prune, --backup-first, restore-by-date, PRUNE_AFTER_UPDATE and the unhealthy tally bucket. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implements everything from the security/functionality/management review, in order of severity. Five commits, each self-contained; a test suite lands in the first and every later change ships with coverage. 83 assertions, all green, run under macOS
sh, Debiandashand Alpine busyboxash.1. Data loss: backup archives matched by prefix (
c045cc0)Archive names had no separator between app and date (
vault2026-08-04.tar.bz2), and both restore andBACKUP_KEEPpruning used the glob${app}[0-9]*.tar.bz2. With appsvaultandvault2:restore vaultpicked vault2's newer archive (it then failed the layout check, but with a baffling error).All archive lookups now go through
list_archives, which validates the complete filename — exact app name, then a separator or the legacy no-separator form, then exactly a date with optional_HHMMSS. New archives are writtenvault_2026-08-04.tar.bz2; old names still restore.2. Security: /tmp hardening + lock self-healing (
c045cc0)/tmpname another local user had already seen. A symlink planted there would have the next root-run step truncate whatever it pointed at. All scratch files now live in amktemp -d0700 directory that survives menu commands./tmp, so any local user could pre-create it and (thanks to sticky/tmp) permanently lock you out of your own tool. It now lives in the managed folder itself (.dockerdance.lock), records the owning pid, and a lock whose process is gone clears itself — a crashed run or reboot no longer demands manual lock removal.doctorreports whether a held lock is live or stale.3. Test suite in the repo, run by CI (
c045cc0)tests/run-tests.sh+ a stubdocker(no daemon needed): each case builds a sandbox of fake apps and asserts on exit codes, output, and the exactSTOP/UP/pull actions taken. Covers the stopped-policy matrix, carrying on past failures, backup/restore roundtrips (real tar), the vault/vault2 collision, legacy archive names, both lock behaviours, prune, restore-by-date,--backup-first, doctor, and the unhealthy bucket. Run against the pre-fix script it fails 6 assertions — it detects the bugs it guards. CI runs it on every push/PR alongside shellcheck (which now also lints the tests).4. Webhook/JSON + supply chain (
8534dc1)notify()escaped nothing, so a quote or backslash in an app name or error broke (or rewrote) the JSON payload. Content is escaped now; payload validated as JSON in test.manage.sh.sha256;update-selfverifies the download against it before installing (sha256sum/shasum/openssl, whichever exists; pre-0.4.1 releases without the asset install as before).update-selfreads the API error body: an exhausted GitHub rate limit now says so instead of "Are you online?".actions/checkoutpinned to a commit SHA in both workflows.5. Features (
4acf7f1,566b7aa,2f511dc)prunecommand +--prune/PRUNE_AFTER_UPDATE=1: reclaims the dangling images updates leave behind (30 apps updating weekly is a disk quietly filling); reports space freed. Dangling only — tagged/shared images untouched../manage.sh restore linkace 2026-08-01reaches any archiveBACKUP_KEEPretains; a wrong date lists what exists; barerestorestill takes the newest and mentions how many older archives are available.HEALTH_TIMEOUTused to count as a success — the tally now readsUpdated 27 of 30 apps - 1 failed, 1 unhealthy, 1 skipped, names listed, non-zero exit. Kept separate fromfailedbecause the remedy differs: failed apps didn't complete the command; unhealthy ones run on the new image but their healthcheck disagrees.update --backup-first: archives every app on the way through. The backup flow already ends each app on the freshly pulled image, so this composes the two existing paths rather than adding a third.doctor: reports free disk space where backups are written, and warns whenmanage.confis group/other-writable (it's sourced as shell — write access is code execution).manage.conf.exampledocumentsSTOPPED_POLICYandPRUNE_AFTER_UPDATE; README covers all of the above; completions (bash/zsh/fish) knowprune.Review corrections
Two things I claimed in the review that turned out wrong, for the record:
manage.conf.exampledoes exist (it just lacked the new settings), and the rate-limit fix landed in commit 4 rather than as a standalone.Verification
shellcheckclean acrossmanage.sh, completions, tests and stub (CI's own image).sh -n(macOS) anddash -n(Debian) pass.sh, Debiandash(with bzip2), and the harness itself fails fast with a clear message where bzip2 is missing.run:block passessh -n.🤖 Generated with Claude Code