fix(restore): confine backup IDs to backup root - #3541
Open
tang-vu wants to merge 1 commit into
Open
Conversation
This was referenced Aug 30, 2026
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.
Summary
.backupsmake testWhy this matters
ods-restore.sh --dry-run ../externalpreviously 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_spaceandextract_backupjoined a raw positional argument ontoBACKUP_ROOT. Archive member validation did not constrain that outer identifier. The invariant is that a backup ID is one path segment; generated IDs such as20260830-120000remain unchanged.Overlap check
Searched open and closed PRs for
restore backup ID,backup identifier restore,restore path traversal,ods-restore validation, and PRs changingods/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
bash tests/test-restore-backup-id-boundary.shfailed because../externalreached dry-run previewbash tests/test-restore-backup-id-boundary.sh? 5 passedbash tests/test-backup-restore-cli.sh? 11 passedmake lintshellcheck --rcfile=/dev/null --severity=error ods-restore.sh tests/test-restore-backup-id-boundary.shgit diff --checkOn the unmerged base,
tests/test-restore-safety-ux.shexits before restore because its fixture does not copy the newly requiredlib/rsync.sh; PR #3372 fixes that fixture, and the synthetic validation above proves this change composes with it. The batch-levelmake gateand 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/mainplus #3540, #3541, and #3542 merged without conflicts. It passed all three new boundary suites, the adjacent preset/backup/rollback suites,make lint, andgit diff --check. The PRs change independent command boundaries; no merge order is required.CI receipt
Head
ccf72ed2c711310559190d550affe4a8fce69ae9completed 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.