test_apply_pack_oserror_integrity.py
python
sha256:8860dea10c653956b613a814cc752a6d34cb3986cdf16749a49172affdabf045
fix tests
Human
minor
⚠ breaking
42 days ago
| 1 | """Tests for Bug 13: apply_pack propagates unhandled OSError from write_commit. |
| 2 | |
| 3 | Root cause: apply_pack's commit loop catches (KeyError, ValueError, TypeError) |
| 4 | but NOT OSError. write_commit raises OSError("Store integrity violation") when |
| 5 | an existing commit file contains a DIFFERENT commit_id than the incoming one — |
| 6 | i.e. an impostor file. When this condition exists in the local store, apply_pack |
| 7 | propagates the unhandled OSError and the entire pull/push/clone call stack crashes |
| 8 | with an unhandled exception rather than logging CRITICAL and continuing. |
| 9 | |
| 10 | Concrete attack / failure scenario: |
| 11 | 1. Local store has commit file abc.msgpack containing impostor bytes |
| 12 | (different commit_id inside the file than the filename implies). |
| 13 | 2. A bundle arrives containing the legitimate abc commit. |
| 14 | 3. write_commit reads the existing file, detects commit_id mismatch, raises |
| 15 | OSError("Store integrity violation"). |
| 16 | 4. apply_pack (before fix) propagates the OSError → muse pull / muse clone |
| 17 | crashes with an unhandled exception. |
| 18 | |
| 19 | Scope of tests |
| 20 | -------------- |
| 21 | - apply_pack with impostor commit file does not crash (no uncaught OSError) |
| 22 | - apply_pack logs CRITICAL when OSError is raised by write_commit |
| 23 | - apply_pack continues processing remaining commits after OSError on one |
| 24 | - apply_pack correctly reports commits_written for non-affected commits |
| 25 | - The impostor file is NOT overwritten by the bundle commit (safety) |
| 26 | - apply_pack raises ValueError (not OSError) for commits with wrong hash |
| 27 | """ |
| 28 | from __future__ import annotations |
| 29 | |
| 30 | import datetime |
| 31 | import logging |
| 32 | import pathlib |
| 33 | |
| 34 | import msgpack |
| 35 | import pytest |
| 36 | |
| 37 | from muse.core.pack import PackBundle, apply_pack |
| 38 | from muse.core.snapshot import compute_commit_id, compute_snapshot_id |
| 39 | from muse.core.store import ( |
| 40 | CommitDict, |
| 41 | CommitRecord, |
| 42 | SnapshotRecord, |
| 43 | read_commit, |
| 44 | write_commit, |
| 45 | write_snapshot, |
| 46 | ) |
| 47 | |
| 48 | _TS = datetime.datetime(2024, 6, 15, 10, 0, 0, tzinfo=datetime.timezone.utc) |
| 49 | |
| 50 | |
| 51 | def _make_repo(tmp_path: pathlib.Path) -> pathlib.Path: |
| 52 | repo = tmp_path / "repo" |
| 53 | repo.mkdir() |
| 54 | (repo / ".muse").mkdir() |
| 55 | (repo / ".muse" / "commits").mkdir() |
| 56 | (repo / ".muse" / "snapshots").mkdir() |
| 57 | return repo |
| 58 | |
| 59 | |
| 60 | def _make_commit_record(message: str, parent: str | None = None) -> CommitRecord: |
| 61 | manifest = {f"{message}.py": "a" * 64} |
| 62 | snap_id = compute_snapshot_id(manifest) |
| 63 | parent_ids = [parent] if parent else [] |
| 64 | cid = compute_commit_id(parent_ids, snap_id, message, _TS.isoformat()) |
| 65 | return CommitRecord( |
| 66 | commit_id=cid, |
| 67 | repo_id="test-repo", |
| 68 | branch="main", |
| 69 | snapshot_id=snap_id, |
| 70 | message=message, |
| 71 | committed_at=_TS, |
| 72 | author="tester", |
| 73 | parent_commit_id=parent, |
| 74 | parent2_commit_id=None, |
| 75 | ) |
| 76 | |
| 77 | |
| 78 | def _bundle_with_commits(commits: list[CommitRecord]) -> PackBundle: |
| 79 | return PackBundle(objects=[], snapshots=[], commits=[c.to_dict() for c in commits], tags=[]) |
| 80 | |
| 81 | |
| 82 | def _write_impostor_file(repo: pathlib.Path, path_commit_id: str, content_commit_id: str) -> None: |
| 83 | """Write a commit file at path_commit_id.msgpack that contains content_commit_id inside.""" |
| 84 | impostor_data = { |
| 85 | "commit_id": content_commit_id, |
| 86 | "repo_id": "impostor", |
| 87 | "branch": "main", |
| 88 | "snapshot_id": "0" * 64, |
| 89 | "message": "impostor", |
| 90 | "committed_at": _TS.isoformat(), |
| 91 | "parent_commit_id": None, |
| 92 | "parent2_commit_id": None, |
| 93 | "author": "attacker", |
| 94 | } |
| 95 | path = repo / ".muse" / "commits" / f"{path_commit_id}.msgpack" |
| 96 | path.write_bytes(msgpack.packb(impostor_data, use_bin_type=True)) |
| 97 | |
| 98 | |
| 99 | # ────────────────────────────────────────────────────────────────────────────── |
| 100 | # Bug 13: unhandled OSError from write_commit |
| 101 | # ────────────────────────────────────────────────────────────────────────────── |
| 102 | |
| 103 | class TestApplyPackOsErrorIntegrity: |
| 104 | |
| 105 | def test_apply_pack_does_not_crash_on_store_integrity_violation(self, tmp_path: pathlib.Path) -> None: |
| 106 | """Bug 13: apply_pack must not propagate OSError from write_commit.""" |
| 107 | repo = _make_repo(tmp_path) |
| 108 | c = _make_commit_record("good-commit") |
| 109 | |
| 110 | # Poison the store: write an impostor at c.commit_id's path |
| 111 | _write_impostor_file(repo, c.commit_id, "f" * 64) |
| 112 | |
| 113 | bundle = _bundle_with_commits([c]) |
| 114 | # Before the fix: apply_pack would raise OSError("Store integrity violation") |
| 115 | # After the fix: apply_pack must return normally (logs CRITICAL, skips commit) |
| 116 | result = apply_pack(repo, bundle) # must not raise |
| 117 | assert result is not None |
| 118 | |
| 119 | def test_apply_pack_logs_critical_on_store_integrity_violation( |
| 120 | self, tmp_path: pathlib.Path, caplog: pytest.LogCaptureFixture |
| 121 | ) -> None: |
| 122 | """apply_pack must log CRITICAL (not swallow silently) when OSError is raised.""" |
| 123 | repo = _make_repo(tmp_path) |
| 124 | c = _make_commit_record("critical-log-commit") |
| 125 | _write_impostor_file(repo, c.commit_id, "e" * 64) |
| 126 | |
| 127 | bundle = _bundle_with_commits([c]) |
| 128 | with caplog.at_level(logging.CRITICAL, logger="muse.core.pack"): |
| 129 | apply_pack(repo, bundle) |
| 130 | |
| 131 | crits = [r for r in caplog.records if r.levelno >= logging.CRITICAL] |
| 132 | assert crits, "apply_pack must log CRITICAL when write_commit raises OSError" |
| 133 | assert any("integrity" in r.message.lower() or "violation" in r.message.lower() |
| 134 | for r in crits) |
| 135 | |
| 136 | def test_apply_pack_continues_after_integrity_violation(self, tmp_path: pathlib.Path) -> None: |
| 137 | """Remaining commits must be processed after an integrity-violation skip.""" |
| 138 | repo = _make_repo(tmp_path) |
| 139 | bad = _make_commit_record("bad-commit") |
| 140 | good = _make_commit_record("good-commit-after") |
| 141 | |
| 142 | # Poison only the bad commit's file |
| 143 | _write_impostor_file(repo, bad.commit_id, "d" * 64) |
| 144 | |
| 145 | bundle = _bundle_with_commits([bad, good]) |
| 146 | result = apply_pack(repo, bundle) |
| 147 | |
| 148 | # good commit must be written despite the preceding integrity violation |
| 149 | assert read_commit(repo, good.commit_id) is not None, ( |
| 150 | "apply_pack must continue processing commits after an integrity violation" |
| 151 | ) |
| 152 | |
| 153 | def test_apply_pack_commits_written_excludes_integrity_violation(self, tmp_path: pathlib.Path) -> None: |
| 154 | """commits_written must not include commits that triggered OSError.""" |
| 155 | repo = _make_repo(tmp_path) |
| 156 | bad = _make_commit_record("integrity-violation") |
| 157 | good = _make_commit_record("normal-commit") |
| 158 | |
| 159 | _write_impostor_file(repo, bad.commit_id, "c" * 64) |
| 160 | |
| 161 | bundle = _bundle_with_commits([bad, good]) |
| 162 | result = apply_pack(repo, bundle) |
| 163 | |
| 164 | # Only the good commit should be counted as written |
| 165 | assert result["commits_written"] == 1, ( |
| 166 | f"commits_written should be 1 (only good), got {result['commits_written']}" |
| 167 | ) |
| 168 | |
| 169 | def test_impostor_file_not_overwritten_by_apply_pack(self, tmp_path: pathlib.Path) -> None: |
| 170 | """apply_pack must NOT replace the impostor file — that would hide the violation.""" |
| 171 | repo = _make_repo(tmp_path) |
| 172 | c = _make_commit_record("legit-commit") |
| 173 | impostor_id = "b" * 64 |
| 174 | _write_impostor_file(repo, c.commit_id, impostor_id) |
| 175 | |
| 176 | bundle = _bundle_with_commits([c]) |
| 177 | apply_pack(repo, bundle) |
| 178 | |
| 179 | # The file still contains the impostor — write_commit's OSError branch |
| 180 | # does NOT overwrite (only write_commit with a valid incoming record |
| 181 | # and a corrupt EXISTING file overwrites via the except Exception branch). |
| 182 | # apply_pack catches the OSError and skips — no write happens. |
| 183 | path = repo / ".muse" / "commits" / f"{c.commit_id}.msgpack" |
| 184 | raw = msgpack.unpackb(path.read_bytes(), raw=False) |
| 185 | assert raw["commit_id"] == impostor_id, ( |
| 186 | "apply_pack must not overwrite an impostor file — that would hide the " |
| 187 | "integrity violation from the user" |
| 188 | ) |
| 189 | |
| 190 | def test_apply_pack_valid_commit_no_integrity_violation(self, tmp_path: pathlib.Path) -> None: |
| 191 | """Regression: valid commits must still be written normally.""" |
| 192 | repo = _make_repo(tmp_path) |
| 193 | c = _make_commit_record("clean-commit") |
| 194 | bundle = _bundle_with_commits([c]) |
| 195 | result = apply_pack(repo, bundle) |
| 196 | |
| 197 | assert result["commits_written"] == 1 |
| 198 | assert read_commit(repo, c.commit_id) is not None |
| 199 | |
| 200 | def test_apply_pack_wrong_hash_raises_valuerror_not_oserror(self, tmp_path: pathlib.Path) -> None: |
| 201 | """Commits with mismatched commit_id hash raise ValueError (caught separately).""" |
| 202 | repo = _make_repo(tmp_path) |
| 203 | # Bad record: commit_id doesn't match content hash |
| 204 | bad_dict = CommitDict( |
| 205 | commit_id="f" * 64, # wrong hash |
| 206 | repo_id="test-repo", |
| 207 | branch="main", |
| 208 | snapshot_id="a" * 64, |
| 209 | message="bad", |
| 210 | committed_at=_TS.isoformat(), |
| 211 | parent_commit_id=None, |
| 212 | parent2_commit_id=None, |
| 213 | author="tester", |
| 214 | ) |
| 215 | bundle = PackBundle(objects=[], snapshots=[], commits=[bad_dict], tags=[]) |
| 216 | result = apply_pack(repo, bundle) # must not raise |
| 217 | assert result["commits_written"] == 0 |
File History
1 commit
sha256:8860dea10c653956b613a814cc752a6d34cb3986cdf16749a49172affdabf045
fix tests
Human
minor
⚠
42 days ago