From 3a05cf16ced20d15e5c16ed65ff83f98a6af2e4c Mon Sep 17 00:00:00 2001 From: Jesse_Chen Date: Mon, 20 Jul 2026 23:54:49 +0800 Subject: [PATCH] fix: create staging backup path safely --- .superpowers/sdd/task-3-report.md | 19 ++++++++ deploy/backup-staging-postgres.sh | 34 +++++++++----- frontend/tests/database-backup.test.ts | 65 ++++++++++++++++++++++++++ 3 files changed, 106 insertions(+), 12 deletions(-) diff --git a/.superpowers/sdd/task-3-report.md b/.superpowers/sdd/task-3-report.md index b271311f..f17b809c 100644 --- a/.superpowers/sdd/task-3-report.md +++ b/.superpowers/sdd/task-3-report.md @@ -36,6 +36,25 @@ cd frontend && npm run lint exit 0 git diff --check exit 0 ``` +## Atomic component-creation follow-up (2026-07-20) + +- The sticky root-owned exception now applies only to ancestors. An existing requested backup leaf is separately required to be current-user owned, non-symlink, and non-group/world-writable before the script can chmod or write it; direct canonical `/tmp` is rejected without calling `chmod` or changing its mode. +- The script no longer uses `mkdir -p` for backup paths. It pins the deepest validated existing ancestor, creates each missing component with one plain relative `mkdir`, validates it, and `cd -P`s into it before creating the next. If a concurrent actor creates any component first, including a symlink, the script rejects it without descending through it. + +```text +RED: +cd frontend && npm run test:db +FAIL rejects a direct canonical sticky shared backup directory before chmod +FAIL fails safely when a racer inserts a symlink during nested backup path creation + +GREEN: +bash -n deploy/backup-staging-postgres.sh exit 0 +cd frontend && npm run test:db 20 passed, 0 failed +cd frontend && npm test 496 passed, 0 failed +cd frontend && npm run lint exit 0 +git diff --check exit 0 +``` + ## Secure directory-creation follow-up (2026-07-20) - `umask 077` now executes at script startup, before any possible `mkdir`. The script records the first absent component, then re-walks every component after `mkdir -p`; all newly created directories must be current-user owned, non-symlink directories without group/world write permission before `cd -P` pins the target. diff --git a/deploy/backup-staging-postgres.sh b/deploy/backup-staging-postgres.sh index 438cdd51..0e6acc14 100755 --- a/deploy/backup-staging-postgres.sh +++ b/deploy/backup-staging-postgres.sh @@ -95,6 +95,8 @@ fi backup_directory_component_path="" CURRENT_UID="$(id -u)" FIRST_CREATED_COMPONENT_INDEX=-1 +DEEPEST_EXISTING_COMPONENT_INDEX=-1 +DEEPEST_EXISTING_COMPONENT_PATH="/" for ((backup_directory_component_index = 0; backup_directory_component_index < ${#backup_directory_components[@]}; backup_directory_component_index += 1)); do backup_directory_component="${backup_directory_components[$backup_directory_component_index]}" if [ -z "$backup_directory_component" ] || [ "$backup_directory_component" = "." ] || [ "$backup_directory_component" = ".." ]; then @@ -106,6 +108,8 @@ for ((backup_directory_component_index = 0; backup_directory_component_index < $ fi if [ -e "$backup_directory_component_path" ]; then validate_backup_directory_component "$backup_directory_component_path" 0 + DEEPEST_EXISTING_COMPONENT_INDEX="$backup_directory_component_index" + DEEPEST_EXISTING_COMPONENT_PATH="$backup_directory_component_path" elif [ "$FIRST_CREATED_COMPONENT_INDEX" -eq -1 ]; then FIRST_CREATED_COMPONENT_INDEX="$backup_directory_component_index" fi @@ -130,18 +134,24 @@ POSTGRES_USER="$(read_environment_value POSTGRES_USER)" STAGING_BACKUP_ENCRYPTION_KEY="$(read_environment_value STAGING_BACKUP_ENCRYPTION_KEY)" export STAGING_BACKUP_ENCRYPTION_KEY -mkdir -p "$BACKUP_DIRECTORY_INPUT" -backup_directory_component_path="" -for ((backup_directory_component_index = 0; backup_directory_component_index < ${#backup_directory_components[@]}; backup_directory_component_index += 1)); do - backup_directory_component="${backup_directory_components[$backup_directory_component_index]}" - backup_directory_component_path="${backup_directory_component_path}/${backup_directory_component}" - if [ "$FIRST_CREATED_COMPONENT_INDEX" -ne -1 ] && [ "$backup_directory_component_index" -ge "$FIRST_CREATED_COMPONENT_INDEX" ]; then - validate_backup_directory_component "$backup_directory_component_path" 1 - else - validate_backup_directory_component "$backup_directory_component_path" 0 - fi -done -cd -P "$BACKUP_DIRECTORY_INPUT" +if [ "$FIRST_CREATED_COMPONENT_INDEX" -eq -1 ]; then + validate_backup_directory_component "$BACKUP_DIRECTORY_INPUT" 1 +fi + +cd -P "$DEEPEST_EXISTING_COMPONENT_PATH" +if [ "$FIRST_CREATED_COMPONENT_INDEX" -ne -1 ]; then + for ((backup_directory_component_index = DEEPEST_EXISTING_COMPONENT_INDEX + 1; backup_directory_component_index < ${#backup_directory_components[@]}; backup_directory_component_index += 1)); do + backup_directory_component="${backup_directory_components[$backup_directory_component_index]}" + if [ -e "$backup_directory_component" ] || [ -L "$backup_directory_component" ]; then + reject_backup_directory + fi + if ! mkdir "$backup_directory_component"; then + reject_backup_directory + fi + validate_backup_directory_component "$backup_directory_component" 1 + cd -P "$backup_directory_component" + done +fi BACKUP_DIRECTORY="$(pwd -P)" if [ "$BACKUP_DIRECTORY" != "$BACKUP_DIRECTORY_INPUT" ] || [ "$BACKUP_DIRECTORY" = "/" ]; then reject_backup_directory diff --git a/frontend/tests/database-backup.test.ts b/frontend/tests/database-backup.test.ts index 2467b377..fc75260b 100644 --- a/frontend/tests/database-backup.test.ts +++ b/frontend/tests/database-backup.test.ts @@ -305,6 +305,71 @@ test("rejects unsafe writable backup parents before creating the target", () => } }); +test("rejects a direct canonical sticky shared backup directory before chmod", () => { + const sharedDirectory = realpathSync("/tmp"); + const environmentFile = createDatabaseEnvironment(); + const commandDirectory = mkdtempSync(join(tmpdir(), "jyotisha-backup-direct-shared-command-")); + const chmodLog = join(commandDirectory, "chmod.log"); + const originalMode = statSync(sharedDirectory).mode & 0o777; + + try { + writeCommand(commandDirectory, "chmod", "printf '%s' called > \"$CHMOD_LOG\"\nexit 97"); + const result = runBackup( + environmentFile.file, + sharedDirectory, + backupEnvironment(commandDirectory, { CHMOD_LOG: chmodLog }), + ); + + 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(chmodLog), false); + assert.equal(statSync(sharedDirectory).mode & 0o777, originalMode); + } finally { + rmSync(environmentFile.directory, { force: true, recursive: true }); + rmSync(commandDirectory, { force: true, recursive: true }); + } +}); + +test("fails safely when a racer inserts a symlink during nested backup path creation", () => { + const root = canonicalSharedTemporaryDirectory("jyotisha-backup-create-race-"); + const raceTarget = canonicalTemporaryDirectory("jyotisha-backup-race-target-"); + const environmentFile = createDatabaseEnvironment(); + const commandDirectory = mkdtempSync(join(tmpdir(), "jyotisha-backup-create-race-command-")); + const target = join(root, "first", "second", "backup"); + const originalRootMode = statSync(root).mode & 0o777; + + try { + safeDiskCommand(commandDirectory); + writeCommand(commandDirectory, "docker", "printf '%s' 'race dump payload'"); + writeCommand( + commandDirectory, + "mkdir", + `if [ "$#" -eq 1 ] && [ "$1" = first ]; then + ln -s "$RACE_TARGET" first +fi +exec /bin/mkdir "$@"`, + ); + const result = runBackup( + environmentFile.file, + target, + backupEnvironment(commandDirectory, { + BACKUP_TIMESTAMP: "20260720T060101Z", + RACE_TARGET: raceTarget, + }), + ); + + assert.notEqual(result.status, 0); + assert.equal(existsSync(join(raceTarget, "second")), false); + assert.equal(existsSync(join(raceTarget, "backup")), false); + assert.equal(statSync(root).mode & 0o777, originalRootMode); + } finally { + rmSync(root, { force: true, recursive: true }); + rmSync(raceTarget, { force: true, recursive: true }); + rmSync(environmentFile.directory, { force: true, recursive: true }); + rmSync(commandDirectory, { force: true, recursive: true }); + } +}); + test("creates every absent backup path component privately despite a permissive caller umask", () => { const root = canonicalSharedTemporaryDirectory("jyotisha-backup-umask-"); const environmentFile = createDatabaseEnvironment();