[HIGH] fix(restore): fail restore when files do not match the manifest

[elhoim]

BLUF

• Impact: a corrupted or tampered backup restores "successfully" — vhir restore printed Restored case INC-2026-001 to ..., told the operator to vhir case activate it, and exited 0 while reporting 1 MISMATCH on the same screen. Any wrapper script or operator gating on the exit code proceeds, and the bad case becomes the live case.
• Root cause: the post-copy hash loop in cmd_restore counted mismatches but never acted on them — no sys.exit, and the .restore-in-progress marker was unlinked unconditionally, unlike _verify_backup (returns False) and cmd_verify_evidence (exits 2).
• Blast radius: every vhir restore — disaster recovery, legal preservation, and archival retrieval. Because the marker was removed, re-running restore to recover was also blocked by "Case directory already exists", so there was no clean retry path.
• Fix: ~25 lines in one function — keep the marker when anything mismatches or is missing, name the offending files, and sys.exit(1) before the activation banner. No signature, format, or dependency changes.
• Evidence: 4 new tests in tests/test_backup_restore.py (tamper, missing file, intact backup, repair-and-retry); 3 of them fail with DID NOT RAISE SystemExit on unmodified source. Suite goes 428 → 432 green on Python 3.10 and 3.12; ruff check and format clean.
• Re-checked on current main @ b2324ac (2026-10-05): the bug is still present — this PR's new tests, applied to main without the fix, still fail (3 failed, 1 passed). The branch now has main merged in.

Priority: HIGH

The defect

cmd_restore copies every file listed in the backup manifest into the live case directory, then re-hashes each one against the manifest. It counted the failures — and then ignored them:

    for i, entry in enumerate(files, 1):
        ...
        if not fpath.exists():
            missing_count += 1
        elif sha256_file(fpath) != expected:
            mismatch_count += 1
        else:
            ok_count += 1
        progress("Verifying", i, total)

    # Remove restore marker
    try:
        restore_marker.unlink()
    except OSError:
        pass

    # Summary
    print(f"\nRestored case {case_id} to {target_dir}")
    print(f"  Files: {ok_count} verified", end="")
    if mismatch_count:
        print(f", {mismatch_count} MISMATCH", end="")

mismatch_count and missing_count are used for nothing but a print. The function then falls through to the OpenSearch/ledger summary and the closing banner, and returns — exit status 0.

Here is the real transcript from a backup whose findings.json was altered after the manifest was written (captured from the new test against unmodified source):

Restoring...
  Copying files... 2/2
  Verifying... 2/2

Restored case INC-2026-001 to /tmp/.../cases/INC-2026-001
  Files: 1 verified, 1 MISMATCH
  OpenSearch: not included in backup
  Ledger: skipped

================================================================
CASE IS NOT ACTIVE. You must activate it before use:

  vhir case activate INC-2026-001

It says "Restored case", it instructs the operator to activate the case, and $? is 0.

Why it is wrong

This is an integrity check that does not check. In a forensic tool the whole point of carrying SHA-256 hashes in the manifest is that a restore either reproduces the case byte-for-byte or it fails loudly. Three specific consequences:

1. No machine-readable signal. Automation around vhir restore cannot distinguish a clean restore from a corrupted one. The only evidence is a substring on stdout.

2. The tampered case becomes the live case. Once restored and activated it is indistinguishable from a clean restore — the provenance question "did this case come back intact?" is unanswerable after the fact.

3. No recovery path. restore_marker.unlink() ran unconditionally, so .restore-in-progress was gone. Re-running vhir restore then hit the guard at backup.py:867:

   Error: Case directory already exists: ...
   Remove it first, then re-run restore.
   

   The marker exists precisely so an interrupted restore can be detected and shutil.rmtree'd on the next run (backup.py:845-863) — removing it after a failed verification threw away the one mechanism that makes retry possible.

Consistency with sibling code

Every other integrity check in this repo fails closed:

• _verify_backup (backup.py:383-444) prints MISMATCH: {rel_path} per file and return False on any mismatch or missing file; cmd_backup (backup.py:26-34) turns that into sys.exit(1). So vhir backup --verify <path> on the same tampered backup correctly exits 1 — while vhir restore <path> on it exits 0.
• cmd_verify_evidence (evidence.py:477-479) prints ALERT: Evidence files have been modified since registration. and sys.exit(2).

cmd_restore was the outlier.

The fix

Confined to the tail of cmd_restore:

    integrity_ok = not (mismatch_count or missing_count)

    # Remove restore marker — only once every file verified, so a bad restore
    # stays flagged and is cleaned up when restore is re-run.
    if integrity_ok:
        try:
            restore_marker.unlink()
        except OSError:
            pass

    # Summary
    if integrity_ok:
        print(f"\nRestored case {case_id} to {target_dir}")
    else:
        print(f"\nRestore of case {case_id} to {target_dir} FAILED verification")
    ...
    if not integrity_ok:
        print(
            f"\nError: Restored case does not match the backup manifest.\n"
            f"  {target_dir} is marked .restore-in-progress — do not use it.\n"
            f"  Verify the backup first: vhir backup --verify {backup_path}",
            file=sys.stderr,
        )
        sys.exit(1)

Three properties, in order of importance:

• Exit 1 on any mismatch or missing file, placed before the vhir case activate banner — the tool no longer instructs the operator to activate a case that failed verification.
• The marker stays. The directory remains flagged as a partial restore, which both warns anyone who looks and re-arms the existing cleanup path so the next vhir restore can rmtree it and retry. This is what makes repair-and-retry work.
• The header no longer lies ("Restore of case ... FAILED verification"), and the loop now names the offending files (MISSING: {rel} / MISMATCH: {rel}) exactly as _verify_backup does, so the non-zero exit is actionable.

A deliberate non-change: the finding suggested also calling _verify_backup(backup_path) as a pre-flight. That was not added — it hashes every file a second time and catches strictly less than the post-copy loop, which also detects copy-time corruption and files the copy loop silently skipped. See the note below on what remains out of scope.

Known limitation, unchanged by this PR

The verification ledger copy, password-hash copies and OpenSearch snapshot restore all run before the verify loop, so they are not rolled back by the new sys.exit(1). A retry after a failed restore will hit Error: Verification ledger already exists unless run with --skip-ledger. Fixing that means moving the verify block up to run immediately after the file copy — a larger change to restore's ordering, kept separate so this one stays cherry-pickable.

Testing

New file tests/test_backup_restore.py, 4 tests driving cmd_restore end-to-end against a synthesised backup in tmp_path:

Test	What it pins
test_tampered_file_exits_nonzero	backup file altered after the manifest → SystemExit(1), marker retained
test_missing_file_exits_nonzero	manifest entry with no file in the backup (the copy loop's if src.exists() silently skips it) → SystemExit(1), marker retained
test_intact_backup_restores	clean backup still completes, marker removed, contents match — no regression
test_retry_after_repair_succeeds	tamper → exit 1 → repair the backup → re-run → succeeds, proving the retained marker restores the recovery path

Against unmodified source, three of the four fail for exactly the reported reason:

FAILED tests/test_backup_restore.py::TestRestoreIntegrity::test_tampered_file_exits_nonzero - Failed: DID NOT RAISE SystemExit
FAILED tests/test_backup_restore.py::TestRestoreIntegrity::test_missing_file_exits_nonzero - Failed: DID NOT RAISE SystemExit
FAILED tests/test_backup_restore.py::TestRestoreIntegrity::test_retry_after_repair_succeeds - Failed: DID NOT RAISE SystemExit
3 failed, 1 passed in 0.22s

With the fix: 4 passed. Full suite 432 passed on Python 3.10 and 3.12 (428 baseline + 4). ruff check . → All checks passed; ruff format --check . → 64 files already formatted.

---

CI status

The checks on this PR will show action_required: GitHub gates workflow runs on pull requests from
first-time fork contributors until a maintainer approves them. They need an Approve and run click.

The CI job was reproduced locally after merging main @ b2324ac (761 tests on main), using the same steps as .github/workflows/ci.yml:

Step	Result
pip install -e ".[dev]" && pytest tests/ — Python 3.10	765 passed
pip install -e ".[dev]" && pytest tests/ — Python 3.11	765 passed
pip install -e ".[dev]" && pytest tests/ — Python 3.12	765 passed
ruff check . + ruff format --check . (latest ruff, unpinned as in CI)	check + format clean

🤖 Generated with Claude Code