Fix broken legacy script handling

README.md

@@ -101,6 +101,34 @@ For an optional full test on an isolated test machine or CI runner, execute
 `sudo sh tests/integration.sh`. It creates and removes a temporary system
 account and tests under `/tmp`, without using your live `/tilde` tree.
 
+## Broken legacy entries
+
+`sudo tilde migrate` now continues past broken links, reports each one, and returns
+an error status if it skipped an unreadable or broken target. The links are not
+removed automatically. For an affected command, inspect the link and its
+submitted file without following or deleting anything:
+
+```sh
+sudo tilde status us
+sudo ls -ld /tilde/bin/us /tilde/pending-submissions/jmjl/us
+sudo readlink -- /tilde/bin/us
+sudo ls -l /tilde/pending-submissions/jmjl/us
+```
+
+The `us` and `jmjl` names above are examples. If the linked executable was
+moved or deleted, restore it under the author's `~/bin` and review it again.
+If the source is gone permanently, the administrator can explicitly run
+`sudo tilde revoke us`, confirm, and provide a reason. A broken legacy link
+whose author cannot be identified can now be revoked, but no email will be
+sent. There is no automatic deletion during migration. Old
+submissions containing broken executable symlinks are skipped with a specific
+warning, without stopping approval of other submissions.
+
+Root-owned legacy per-user pending directories can be read during approval
+when they are not group/world writable. New submissions still require the
+user's own pending directory to be owned by that user. Do not recursively
+change ownership or permissions of `/tilde` to fix a single submission.
+
 ## Recovery and rollback
 
 Before installing, archive `/tilde` and preserve the old launcher binary and

tests/integration.sh

@@ -98,3 +98,50 @@ printf 'a\n' | "$tmp/launcher" migrate legacy >/dev/null
 printf '#!/bin/sh\necho legacy-v2\n' > "$home/bin/legacy"
 [ "$("$tmp/launcher" run legacy)" = 'legacy-v1' ]
 printf 'Integration tests passed.\n'
+# A broken legacy symlink must not stop migration of subsequent entries.
+ln -s "$home/bin/missing-us" "$root/bin/us"
+printf 'Missing script\n' > "$root/descriptions/us"
+cat > "$home/bin/zzgood" <<'EOF_INNER'
+#!/bin/sh
+echo zzgood-v1
+EOF_INNER
+chmod 0755 "$home/bin/zzgood"
+chown "$uid:$gid" "$home/bin/zzgood"
+ln -s "$home/bin/zzgood" "$root/bin/zzgood"
+printf 'Good legacy script\n' > "$root/descriptions/zzgood"
+if printf 'a\n' | "$tmp/launcher" migrate > "$tmp/migrate.out" 2> "$tmp/migrate.err"; then
+    echo 'Expected batch migration to report a broken legacy link' >&2
+    exit 1
+fi
+grep -q 'broken legacy symlink: us' "$tmp/migrate.err"
+grep -q 'migration finished with 1 unreadable or broken link' "$tmp/migrate.err"
+[ -L "$root/bin/us" ] && [ ! -e "$root/bin/us" ]
+[ "$("$tmp/launcher" run zzgood)" = zzgood-v1 ]
+"$tmp/launcher" status us | grep -q 'broken or inaccessible'
+# Root-owned legacy submission directories may be reviewed read-only.
+# Broken submitted symlinks must give actionable diagnostics, not abort the queue.
+chown root:root "$root/pending-submissions/$user"
+chmod 0755 "$root/pending-submissions/$user"
+mkdir -m 0755 "$root/pending-submissions/$user/us"
+ln -s "$home/bin/missing-us" "$root/pending-submissions/$user/us/us"
+printf 'Broken old submission\n' > "$root/pending-submissions/$user/us/description.txt"
+cat > "$home/bin/legacypending" <<'EOF_INNER'
+#!/bin/sh
+echo reviewed-legacy-pending
+EOF_INNER
+chmod 0755 "$home/bin/legacypending"
+chown "$uid:$gid" "$home/bin/legacypending"
+mkdir -m 0755 "$root/pending-submissions/$user/legacypending"
+ln -s "$home/bin/legacypending" "$root/pending-submissions/$user/legacypending/legacypending"
+printf 'An older pending submission\n' > "$root/pending-submissions/$user/legacypending/description.txt"
+printf 'a\n' | "$tmp/launcher" approve > "$tmp/approve.out" 2> "$tmp/approve.err"
+grep -q "broken submitted-script symlink: $user/us" "$tmp/approve.err"
+[ "$("$tmp/launcher" run legacypending)" = reviewed-legacy-pending ]
+[ -L "$root/bin/us" ] && [ ! -e "$root/bin/us" ]
+# A broken legacy link can be revoked with confirmation, even without its owner.
+printf 'n\n' | "$tmp/launcher" revoke us >/dev/null
+[ -L "$root/bin/us" ]
+printf 'y\nBroken and no longer available\n' | "$tmp/launcher" revoke us >/dev/null 2> "$tmp/revoke.err"
+[ ! -L "$root/bin/us" ] && [ ! -e "$root/bin/us" ]
+[ ! -e "$root/descriptions/us" ]
+grep -q 'author unknown; no notification sent' "$tmp/revoke.err"

tests/smoke.sh

@@ -2,7 +2,7 @@
 set -eu
 cd "$(dirname "$0")/.."
 sh -n tilde
-[ "$(./tilde --version)" = 0.2.0 ]
+[ "$(./tilde --version)" = 0.2.1 ]
 ./tilde --help | grep -q 'migrate'
 if ./tilde run '../etc/passwd' >/dev/null 2>&1; then
     printf 'Path traversal was accepted\n' >&2

tilde

@@ -15,7 +15,7 @@ if [ "$(/usr/bin/id -u)" -eq 0 ]; then
 fi
 
 PROGNAME=${0##*/}
-VERSION=0.2.0
+VERSION=0.2.1
 ROOT=/tilde
 BIN=$ROOT/bin
 DESCRIPTIONS=$ROOT/descriptions
@@ -196,6 +196,32 @@ user_pending_dir() {
     printf '%s\n' "$dir"
 }
 
+# Older launcher versions sometimes left root-owned submission directories.
+# Admins may review those directories if root alone can change their entries.
+# Ordinary users still need user-owned directories for new submissions.
+review_user_dir() {
+    review_owner=$1
+    valid_user "$review_owner" || { warn 'invalid submission account'; return 1; }
+    review_dir=$PENDING/$review_owner
+    if [ ! -d "$review_dir" ] || [ -L "$review_dir" ]; then
+        warn "missing or unsafe submission directory: $review_dir"
+        return 1
+    fi
+    review_dir_owner=$(stat -c %U -- "$review_dir") || return 1
+    if [ "$review_dir_owner" != "$review_owner" ]; then
+        if [ "$review_dir_owner" != root ]; then
+            warn "unexpected owner of $review_dir: $review_dir_owner"
+            return 1
+        fi
+        review_mode=$(stat -c %a -- "$review_dir") || return 1
+        if [ $((0$review_mode & 0022)) -ne 0 ]; then
+            warn "root-owned legacy directory is writable by other accounts: $review_dir"
+            return 1
+        fi
+    fi
+    printf '%s\n' "$review_dir"
+}
+
 ensure_user_dir() {
     owner=$1
     valid_user "$owner" || fail 'unsupported account name'
@@ -300,15 +326,50 @@ capture() {
 capture_submission() {
     author=$1
     name=$2
-    folder=$(user_pending_dir "$author") || return 1
+    if ! getent passwd "$author" >/dev/null; then
+        warn "submission account no longer exists: $author"
+        return 1
+    fi
+    folder=$(review_user_dir "$author") || return 1
     submission=$folder/$name
-    [ -d "$submission" ] && [ ! -L "$submission" ] || return 1
+    if [ ! -d "$submission" ] || [ -L "$submission" ]; then
+        warn "missing or unsafe submission folder: $submission"
+        return 1
+    fi
+    if [ -L "$submission/$name" ] && [ ! -e "$submission/$name" ]; then
+        warn "broken submitted-script symlink: $author/$name; ask the author to restore or resubmit"
+        return 1
+    fi
+    if [ ! -f "$submission/$name" ]; then
+        warn "submitted executable is missing or not a regular file: $author/$name"
+        return 1
+    fi
+    if [ ! -f "$submission/description.txt" ]; then
+        warn "missing description: $author/$name"
+        return 1
+    fi
+    if ! runuser -u "$author" -- test -r "$submission/$name" ||
+       ! runuser -u "$author" -- test -x "$submission/$name"; then
+        warn "author cannot read or execute submitted script: $author/$name"
+        return 1
+    fi
+    if ! runuser -u "$author" -- test -r "$submission/description.txt"; then
+        warn "author cannot read submission description: $author/$name"
+        return 1
+    fi
     WORK=$(mktemp -d "$REVIEW/.review.XXXXXXXX") || fail 'cannot stage review'
-    capture "$author" "$submission/$name" "$WORK/script" "$MAX_SCRIPT" || return 1
-    capture "$author" "$submission/description.txt" "$WORK/description" "$MAX_DESCRIPTION" || return 1
-    [ -s "$WORK/script" ] && [ -s "$WORK/description" ] || return 1
-    # A submitted binary or script must be runnable by the submitter.
-    runuser -u "$author" -- test -x "$submission/$name" || return 1
+    if ! capture "$author" "$submission/$name" "$WORK/script" "$MAX_SCRIPT"; then
+        warn "cannot capture script (unreadable, changed or larger than 50 MiB): $author/$name"
+        return 1
+    fi
+    if ! capture "$author" "$submission/description.txt" "$WORK/description" "$MAX_DESCRIPTION"; then
+        warn "cannot capture description (unreadable or larger than 16 KiB): $author/$name"
+        return 1
+    fi
+    if [ ! -s "$WORK/script" ] || [ ! -s "$WORK/description" ]; then
+        warn "submitted script or description is empty: $author/$name"
+        return 1
+    fi
 }
 
 log_event() {
@@ -479,8 +540,7 @@ approve_queue() {
     for ownerdir in "$PENDING"/*; do
         [ -d "$ownerdir" ] && [ ! -L "$ownerdir" ] || continue
         author=${ownerdir##*/}
-        if ! valid_user "$author" || [ "$(stat -c %U -- "$ownerdir")" != "$author" ]; then
-            warn "skipping unsafe submission directory: $ownerdir"
+        if ! review_user_dir "$author" >/dev/null; then
             continue
         fi
         for item in "$ownerdir"/*; do
@@ -506,8 +566,14 @@ migrate_one() {
         warn "$name has already been migrated"
         return 0
     fi
-    source=$(readlink -f -- "$link") || fail "broken legacy symlink: $name"
-    [ -f "$source" ] || fail "legacy target not a file: $name"
+    if ! source=$(readlink -e -- "$link" 2>/dev/null); then
+        warn "broken legacy symlink: $name; leaving it unchanged and skipping"
+        return 1
+    fi
+    if [ ! -f "$source" ]; then
+        warn "legacy target is not a regular file: $name; skipping"
+        return 1
+    fi
     author=$(stat -c %U -- "$source") || fail 'cannot inspect author'
     valid_user "$author" || fail 'cannot determine legacy author'
     home=$(getent passwd "$author" | cut -d: -f6)
@@ -544,10 +610,15 @@ migrate_legacy() {
     if [ -n "${1-}" ]; then
         migrate_one "$1"
     else
+        skipped=0
         for item in "$BIN"/*; do
             [ -L "$item" ] || continue
-            migrate_one "${item##*/}"
+            migrate_one "${item##*/}" || skipped=$((skipped + 1))
         done
+        if [ "$skipped" -gt 0 ]; then
+            warn "migration finished with $skipped unreadable or broken link(s); no affected links were removed"
+            return 1
+        fi
     fi
 }
 
@@ -559,11 +630,20 @@ revoke_script() {
     [ -L "$link" ] || fail "$name is not an approved symlink"
     author=$(approved_author "$name" 2>/dev/null || :)
     if [ -z "$author" ]; then
-        source=$(readlink -f -- "$link") || fail 'cannot find legacy script author'
-        author=$(stat -c %U -- "$source") || fail 'cannot find script owner'
+        source=$(readlink -e -- "$link" 2>/dev/null || :)
+        if [ -n "$source" ]; then
+            author=$(stat -c %U -- "$source" 2>/dev/null || :)
         fi
-    valid_user "$author" || fail 'invalid script owner'
+    fi
+    if ! valid_user "$author"; then
+        author=
+    fi
+    if [ -z "$author" ]; then
+        printf 'No author could be identified for %s. This may be a broken legacy link.\n' "$name"
+        prompt "Revoke $name (author unknown)?" || { printf 'Cancelled.\n'; return 0; }
+    else
         prompt "Revoke $name by $author?" || { printf 'Cancelled.\n'; return 0; }
+    fi
     ask_line 'Reason: '
     reason=$INPUT
     [ -n "$reason" ] || fail 'a reason is required'
@@ -581,8 +661,12 @@ revoke_script() {
     if [ -e "$META/$name" ] || [ -L "$META/$name" ]; then
         rm -- "$META/$name" || warn 'could not remove metadata'
     fi
-    log_event "revoked $name by $author"
+    log_event "revoked $name by ${author:-unknown}"
+    if [ -n "$author" ]; then
         send_notice "$author" 'tilde script revoked' "$name was revoked. Reason: $reason. You may submit a revised version." || :
+    else
+        warn 'author unknown; no notification sent'
+    fi
     printf '%s revoked. Older reviewed copies were retained in %s.\n' "$name" "$APPROVED/$name"
 }
 
@@ -646,7 +730,11 @@ status_script() {
     if [ -n "$author" ] && [ -L "$BIN/$name" ]; then
         printf '%s: approved (author: %s)\n' "$name" "$author"
     elif [ -L "$BIN/$name" ]; then
+        if [ -f "$BIN/$name" ] && [ -x "$BIN/$name" ]; then
             printf '%s: approved (legacy symlink; needs migration)\n' "$name"
+        else
+            printf '%s: broken or inaccessible approved symlink; requires admin review\n' "$name"
+        fi
     else
         printf '%s: not approved\n' "$name"
     fi