diff --git a/.superpowers/sdd/task-3-report.md b/.superpowers/sdd/task-3-report.md index 97193e56..4cdced1a 100644 --- a/.superpowers/sdd/task-3-report.md +++ b/.superpowers/sdd/task-3-report.md @@ -5,7 +5,7 @@ - Added `deploy/backup-staging-postgres.sh DATABASE_ENV_FILE BACKUP_DIRECTORY`. - Validates the private database environment before reading required values without sourcing or executing the env file. - Refuses backup destinations with disk usage at or above 70%, creates the explicit destination with mode `0700`, writes encrypted custom PostgreSQL dumps with mode `0600`, and keeps only the newest three exact staging dump names. -- Uses a PID-scoped `.partial` file, `pipefail`, an exit cleanup trap, and same-directory rename so failed dump/encryption pipelines leave no completed partial archive and successful archives are published atomically. +- Uses a PID-scoped `.partial` file, `pipefail`, an exit cleanup trap, and same-directory hard-link/no-clobber publication so failed dump/encryption pipelines leave no completed partial archive and successful archives are published atomically. - Emits only final path/count on success; credentials use environment variables and never command-line arguments. ## TDD evidence @@ -36,6 +36,25 @@ cd frontend && npm run lint exit 0 git diff --check exit 0 ``` +## TOCTOU follow-up (2026-07-20) + +- Before `mkdir`/`cd`, the script walks all existing absolute-target ancestors, rejects symlinks, requires current-user-or-root ownership, and rejects group/world-writable modes. The unsafe-parent regression uses `realpathSync` for the macOS temporary root, so its nested symlink and `0777` parent are the components actually reached by validation. +- After creation it enters the directory with `cd -P`, verifies the canonical path and directory identity, and keeps the working directory pinned. Disk check, lock, partial/final publication, inventory, `find`, and rotation all use `.` or relative filenames; success still prints the canonical absolute archive path. + +```text +RED: +cd frontend && npm run test:db +FAIL rejects unsafe writable backup parents before creating the target +The old script created the target and then failed only at the disk check, rather than rejecting the unsafe ancestor. + +GREEN: +bash -n deploy/backup-staging-postgres.sh exit 0 +cd frontend && npm run test:db 17 passed, 0 failed +cd frontend && npm test 493 passed, 0 failed +cd frontend && npm run lint exit 0 +git diff --check exit 0 +``` + ## Self-review and caveats - Reviewed the final diff for secret exposure, output scope, filename filtering, rotation boundaries, portable shell options, and atomic/cleanup behavior; no task-scope finding remained. diff --git a/deploy/backup-staging-postgres.sh b/deploy/backup-staging-postgres.sh index 93d3abb1..3473b6ea 100755 --- a/deploy/backup-staging-postgres.sh +++ b/deploy/backup-staging-postgres.sh @@ -20,6 +20,31 @@ reject_backup_directory() { exit 1 } +reject_unsafe_backup_directory_ancestor() { + echo "backup directory ancestor must be owned by the current user or root and not group/world-writable" >&2 + exit 1 +} + +stat_owner_and_mode() { + local path="$1" + + if stat -f '%u %Lp' "$path" >/dev/null 2>&1; then + stat -f '%u %Lp' "$path" + else + stat -c '%u %a' "$path" + fi +} + +directory_identity() { + local path="$1" + + if stat -f '%d:%i' "$path" >/dev/null 2>&1; then + stat -f '%d:%i' "$path" + else + stat -c '%d:%i' "$path" + fi +} + if [ "$BACKUP_DIRECTORY_INPUT" = "/" ] || [[ "$BACKUP_DIRECTORY_INPUT" != /* ]] || [[ "$BACKUP_DIRECTORY_INPUT" == */ ]] || [[ "$BACKUP_DIRECTORY_INPUT" == *"//"* ]]; then reject_backup_directory fi @@ -30,6 +55,7 @@ if [ "${#backup_directory_components[@]}" -eq 0 ]; then fi backup_directory_component_path="" +CURRENT_UID="$(id -u)" for backup_directory_component in "${backup_directory_components[@]}"; do if [ -z "$backup_directory_component" ] || [ "$backup_directory_component" = "." ] || [ "$backup_directory_component" = ".." ]; then reject_backup_directory @@ -38,6 +64,18 @@ for backup_directory_component in "${backup_directory_components[@]}"; do if [ -L "$backup_directory_component_path" ]; then reject_backup_directory fi + if [ -e "$backup_directory_component_path" ]; then + if [ ! -d "$backup_directory_component_path" ]; then + reject_backup_directory + fi + read -r backup_directory_owner backup_directory_mode <<< "$(stat_owner_and_mode "$backup_directory_component_path")" + if [ "$backup_directory_owner" != "$CURRENT_UID" ] && [ "$backup_directory_owner" != "0" ]; then + reject_unsafe_backup_directory_ancestor + fi + if (( (10#${backup_directory_mode: -2:1} & 2) != 0 || (10#${backup_directory_mode: -1} & 2) != 0 )); then + reject_unsafe_backup_directory_ancestor + fi + fi done "$VALIDATOR" "$DATABASE_ENV_FILE" >/dev/null @@ -60,13 +98,18 @@ STAGING_BACKUP_ENCRYPTION_KEY="$(read_environment_value STAGING_BACKUP_ENCRYPTIO export STAGING_BACKUP_ENCRYPTION_KEY mkdir -p "$BACKUP_DIRECTORY_INPUT" -BACKUP_DIRECTORY="$(cd "$BACKUP_DIRECTORY_INPUT" && pwd -P)" +cd -P "$BACKUP_DIRECTORY_INPUT" +BACKUP_DIRECTORY="$(pwd -P)" if [ "$BACKUP_DIRECTORY" != "$BACKUP_DIRECTORY_INPUT" ] || [ "$BACKUP_DIRECTORY" = "/" ]; then reject_backup_directory fi -chmod 0700 "$BACKUP_DIRECTORY" +BACKUP_DIRECTORY_IDENTITY="$(directory_identity .)" +if [ "$(directory_identity "$BACKUP_DIRECTORY_INPUT")" != "$BACKUP_DIRECTORY_IDENTITY" ]; then + reject_backup_directory +fi +chmod 0700 . -DISK_USAGE="$(df -Pk "$BACKUP_DIRECTORY" | awk 'NR == 2 { gsub(/%/, "", $5); print $5 }')" +DISK_USAGE="$(df -Pk . | awk 'NR == 2 { gsub(/%/, "", $5); print $5 }')" if ! [[ "$DISK_USAGE" =~ ^[0-9]+$ ]] || [ "$DISK_USAGE" -ge 70 ]; then echo "backup directory disk usage must be below 70 percent" >&2 exit 1 @@ -79,9 +122,9 @@ if ! [[ "$BACKUP_TIMESTAMP" =~ ^[0-9]{8}T[0-9]{6}Z$ ]]; then fi FILE_NAME="jyotisha-staging-${BACKUP_TIMESTAMP}.dump.enc" -FINAL_FILE="$BACKUP_DIRECTORY/$FILE_NAME" -PARTIAL_FILE="$BACKUP_DIRECTORY/.${FILE_NAME}.$$.partial" -LOCK_DIRECTORY="$BACKUP_DIRECTORY/.${FILE_NAME}.lock" +FINAL_FILE="$FILE_NAME" +PARTIAL_FILE=".${FILE_NAME}.$$.partial" +LOCK_DIRECTORY=".${FILE_NAME}.lock" LOCK_ACQUIRED=0 if [ -e "$FINAL_FILE" ] || [ -L "$FINAL_FILE" ]; then @@ -110,10 +153,12 @@ LOCK_ACQUIRED=1 : > "$PARTIAL_FILE" chmod 0600 "$PARTIAL_FILE" -cd "$REPOSITORY_ROOT" -docker compose -p "${COMPOSE_PROJECT_NAME:-jyotisha-staging}" \ - -f deploy/docker-compose.postgres.yml exec -T postgres \ - pg_dump -U "$POSTGRES_USER" -d "$POSTGRES_DB" --format=custom --no-owner | +( + cd "$REPOSITORY_ROOT" + docker compose -p "${COMPOSE_PROJECT_NAME:-jyotisha-staging}" \ + -f deploy/docker-compose.postgres.yml exec -T postgres \ + pg_dump -U "$POSTGRES_USER" -d "$POSTGRES_DB" --format=custom --no-owner +) | openssl enc -aes-256-cbc -salt -pbkdf2 \ -pass env:STAGING_BACKUP_ENCRYPTION_KEY > "$PARTIAL_FILE" @@ -126,7 +171,7 @@ rm -f "$PARTIAL_FILE" PARTIAL_FILE="" completed=() -if ! completed_paths="$(find "$BACKUP_DIRECTORY" -maxdepth 1 -type f -name 'jyotisha-staging-*.dump.enc' -print | LC_ALL=C sort)"; then +if ! completed_paths="$(find . -maxdepth 1 -type f -name 'jyotisha-staging-*.dump.enc' -print | LC_ALL=C sort)"; then echo "failed to enumerate completed backups" >&2 exit 1 fi @@ -139,8 +184,8 @@ done <<< "$completed_paths" if [ "${#completed[@]}" -gt 3 ]; then for ((index = 0; index < ${#completed[@]} - 3; index += 1)); do - rm -f "$BACKUP_DIRECTORY/${completed[$index]}" + rm -f "${completed[$index]}" done fi -printf 'path=%s count=%s\n' "$FINAL_FILE" "$(( ${#completed[@]} > 3 ? 3 : ${#completed[@]} ))" +printf 'path=%s count=%s\n' "$BACKUP_DIRECTORY/$FINAL_FILE" "$(( ${#completed[@]} > 3 ? 3 : ${#completed[@]} ))" diff --git a/frontend/tests/database-backup.test.ts b/frontend/tests/database-backup.test.ts index afde4410..19afa628 100644 --- a/frontend/tests/database-backup.test.ts +++ b/frontend/tests/database-backup.test.ts @@ -2,6 +2,7 @@ import assert from "node:assert/strict"; import { spawn, spawnSync } from "node:child_process"; import { chmodSync, + existsSync, mkdirSync, mkdtempSync, readdirSync, @@ -227,7 +228,7 @@ test("staging backups are encrypted, atomic, private, and retain the newest thre }); test("rejects destructive backup directory aliases and symlink components before mutation", () => { - const root = mkdtempSync(join(tmpdir(), "jyotisha-backup-boundary-")); + const root = canonicalTemporaryDirectory("jyotisha-backup-boundary-"); const environmentFile = createDatabaseEnvironment(); const target = join(root, "target"); const sentinel = join(target, "sentinel.txt"); @@ -271,6 +272,31 @@ test("rejects destructive backup directory aliases and symlink components before } }); +test("rejects unsafe writable backup parents before creating the target", () => { + const root = canonicalTemporaryDirectory("jyotisha-backup-unsafe-parent-"); + const environmentFile = createDatabaseEnvironment(); + const unsafeParent = join(root, "unsafe-parent"); + const target = join(unsafeParent, "backup"); + const sentinel = join(unsafeParent, "sentinel.txt"); + + try { + mkdirSync(unsafeParent, { mode: 0o700 }); + writeFileSync(sentinel, "must remain untouched"); + chmodSync(unsafeParent, 0o777); + + const result = runBackup(environmentFile.file, target, process.env, root); + + assert.notEqual(result.status, 0); + assert.match(result.stderr, /backup directory ancestor must be owned by the current user or root and not group\/world-writable/); + assert.equal(existsSync(target), false); + assert.equal(statSync(unsafeParent).mode & 0o777, 0o777); + assert.equal(readFileSync(sentinel, "utf8"), "must remain untouched"); + } finally { + rmSync(root, { force: true, recursive: true }); + rmSync(environmentFile.directory, { force: true, recursive: true }); + } +}); + test("same-second backups publish once without overwriting the completed archive", async () => { const environmentFile = createDatabaseEnvironment(); const backupDirectory = canonicalTemporaryDirectory("jyotisha-backup-collision-");