Skip to content

fix(restore): confine backup IDs to backup root - #3541

Open
tang-vu wants to merge 1 commit into
Osmantic:mainfrom
tang-vu:fix/restore-backup-id-confinement
Open

fix(restore): confine backup IDs to backup root#3541
tang-vu wants to merge 1 commit into
Osmantic:mainfrom
tang-vu:fix/restore-backup-id-confinement

Conversation

@tang-vu

@tang-vu tang-vu commented Aug 30, 2026

Copy link
Copy Markdown
Contributor

Summary

  • validate explicit and interactively selected restore IDs before joining them to .backups
  • reject path separators, dot segments, and control characters
  • keep generated timestamp IDs working and wire the boundary regression into make test

Why this matters

ods-restore.sh --dry-run ../external previously resolved .backups/../external, accepted a valid manifest from that sibling directory, and proceeded to the restore preview. A non-dry restore could therefore overwrite live ODS state from a directory the operator did not select from the backup store. Restore IDs must identify one direct child of $ODS_DIR/.backups.

Root cause and invariant

ensure_restore_space and extract_backup joined a raw positional argument onto BACKUP_ROOT. Archive member validation did not constrain that outer identifier. The invariant is that a backup ID is one path segment; generated IDs such as 20260830-120000 remain unchanged.

Overlap check

Searched open and closed PRs for restore backup ID, backup identifier restore, restore path traversal, ods-restore validation, and PRs changing ods/ods-restore.sh. No PR validates the outer backup ID. PRs #2720 (symlinked source entries), #2952 (empty config), #3094 (post-restore verification), #3250 (manifest member), and #3336 (status handling) address later, independent stages. Pairwise synthetic merges with all five plus #3372 were conflict-free. A synthetic head with tang-vu PR #3372 also passed both its restore safety UX suite and this boundary suite.

Validation

  • Red repro on upstream main: bash tests/test-restore-backup-id-boundary.sh failed because ../external reached dry-run preview
  • bash tests/test-restore-backup-id-boundary.sh ? 5 passed
  • bash tests/test-backup-restore-cli.sh ? 11 passed
  • synthetic merge with ci: exercise backup and restore safety contracts #3372: boundary 5/5; restore safety UX 2/2
  • make lint
  • shellcheck --rcfile=/dev/null --severity=error ods-restore.sh tests/test-restore-backup-id-boundary.sh
  • git diff --check

On the unmerged base, tests/test-restore-safety-ux.sh exits before restore because its fixture does not copy the newly required lib/rsync.sh; PR #3372 fixes that fixture, and the synthetic validation above proves this change composes with it. The batch-level make gate and all-files pre-commit limitations are the same pre-existing Windows CRLF/private-key-fixture failures recorded in PR #3540.

Tradeoffs and rollback

The validator intentionally accepts legacy IDs containing spaces or punctuation as long as they are a single segment. Backslashes are rejected on every platform to keep the identifier portable across WSL and Windows. No backup contents or manifests change. Reverting restores permissive path joining.

Generated with Codex

Batch compatibility update

A synthetic integration head built from upstream/main plus #3540, #3541, and #3542 merged without conflicts. It passed all three new boundary suites, the adjacent preset/backup/rollback suites, make lint, and git diff --check. The PRs change independent command boundaries; no merge order is required.

CI receipt

Head ccf72ed2c711310559190d550affe4a8fce69ae9 completed with 28 successful checks, 4 conditionally skipped Claude jobs, 0 pending checks, and 0 failures. GitHub reports the diff mergeable; branch protection is waiting only for required human review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

1 participant