diff --git a/docs/how-to/export-dataset-snapshot.md b/docs/how-to/export-dataset-snapshot.md index 76918812..68b0e38b 100644 --- a/docs/how-to/export-dataset-snapshot.md +++ b/docs/how-to/export-dataset-snapshot.md @@ -86,13 +86,16 @@ in the receipt is re-read and compared by size and sha256, and every finding is returned in one report built from the shared verification types in `hflow.verification` (a `VerificationReport` with status `ok`, `damaged`, or `unverifiable`). Recorded paths are joined only onto the handed -directory, so a copied delivery verifies in place. Exit `0` clean, -`1` damaged, `3` unverifiable, `2` unreadable input. A pre-#401 -`format.json` with no `integrity` key is reported as `no-receipt`, -unverifiable, not corrupt. Older HFlow overwrite checks only -`format` and `format_version`, and readers that only consume the string -`tables` map keep working; the `integrity` key is purely additive under -format version `1`. +directory, so a copied delivery verifies in place. A receipt path that +is absolute, contains `..`, or carries a Windows drive letter is refused +as unreadable input (exit `2`) before any file is read — the same family +storage keys refuse — so a tampered marker cannot hash bytes outside the +delivery. Exit `0` clean, `1` damaged, `3` unverifiable, `2` unreadable +input. A pre-#401 `format.json` with no `integrity` key is reported as +`no-receipt`, unverifiable, not corrupt. Older HFlow overwrite checks +only `format` and `format_version`, and readers that only consume the +string `tables` map keep working; the `integrity` key is purely additive +under format version `1`. The receipt travels unsigned inside the `format.json` it describes, so it catches corruption and accidental loss, not tampering: anyone who can edit a diff --git a/src/hflow/snapshot.py b/src/hflow/snapshot.py index e155bdda..d005408e 100644 --- a/src/hflow/snapshot.py +++ b/src/hflow/snapshot.py @@ -23,7 +23,7 @@ from hflow.app import ARTIFACT_MEASUREMENT_KEY_PREFIX, MEDIA_CONTACT_SHEET_STEP_NAME from hflow.catalog import episode_status_case_sql from hflow.curation import open_catalog_connection -from hflow.storage import StorageRoot, fetch_uri +from hflow.storage import StorageRoot, _validated_relative_key, fetch_uri if TYPE_CHECKING: from hflow.verification import VerificationReport @@ -912,12 +912,16 @@ def verify_dataset_snapshot( ``integrity`` key. That snapshot is unverifiable, not corrupt. Unreadable input (a missing directory, a missing or unparsable - ``format.json``) raises instead: that is not a finding about a delivered - snapshot, it is the wrong input entirely. The same applies to a receipt - that is internally inconsistent with itself: a missing or malformed - ``content_id``, or a recomputed inventory hash that disagrees with the - stored one, means the receipt no longer describes the delivered set - (tampering or a truncated write), which is exit 2, not damaged bytes. + ``format.json``, or a receipt ``path`` that is absolute / contains ``..`` + / carries a Windows drive letter) raises instead: that is not a finding + about a delivered snapshot, it is the wrong input entirely. Path + containment reuses :func:`hflow.storage._validated_relative_key` so the + verifier and storage roots share one answer to "is this key inside the + root", and the check runs before any file read (#469). The same exit-2 + class applies to a receipt that is internally inconsistent with itself: + a missing or malformed ``content_id``, or a recomputed inventory hash + that disagrees with the stored one, means the receipt no longer + describes the delivered set (tampering or a truncated write). Extra files under ``assets/`` that the receipt does not name are ignored: the receipt covers what was exported, not everything a recipient may add. @@ -1021,7 +1025,18 @@ def verify_dataset_snapshot( ) for record in receipt_records: - relative_path = record.path + # Containment (#469): refuse before any read. Absolute paths discard + # the handed root under pathlib; ``..`` climbs out of it. Both are + # malformed receipts (exit 2), not damaged bytes. Reuse the storage + # key validator so this family has one implementation. + try: + relative_path = _validated_relative_key(record.path) + except ValueError as error: + raise ValueError( + f"format.json integrity receipt path {record.path!r} must stay " + "under the handed snapshot directory (no leading '/', no '..', " + "no Windows drive letter); refused before any file read" + ) from error delivered_path = resolved_directory / relative_path if not delivered_path.is_file(): findings.append( diff --git a/tests/test_snapshot_verify.py b/tests/test_snapshot_verify.py index df490c49..ebdf50e0 100644 --- a/tests/test_snapshot_verify.py +++ b/tests/test_snapshot_verify.py @@ -519,3 +519,89 @@ def test_receipt_entry_with_numeric_sha256_is_refused_at_the_boundary() -> None: with pytest.raises(ValueError, match="sha256"): _parse_file_integrity_record({"path": "samples.parquet", "size_bytes": 10, "sha256": 123}) + + +def _hand_built_snapshot_with_receipt_path( + snap: Path, *, receipt_path: str, payload: bytes = b"secret-bytes" +) -> None: + """Minimal identity+integrity marker whose single receipt uses ``receipt_path``. + + ``content_id`` matches that one-entry inventory so the deleted-member gate + is not the thing that fires; the path containment check is. + """ + from hflow.snapshot import ( + DATASET_SNAPSHOT_FORMAT_NAME, + DATASET_SNAPSHOT_FORMAT_VERSION, + FileIntegrityRecord, + _inventory_content_id, + ) + + digest = hashlib.sha256(payload).hexdigest() + record = FileIntegrityRecord(path=receipt_path, size_bytes=len(payload), sha256=digest) + marker = { + "format": DATASET_SNAPSHOT_FORMAT_NAME, + "format_version": DATASET_SNAPSHOT_FORMAT_VERSION, + "media_mode": "references", + "media_uri_base": None, + "tables": ["samples.parquet"], + "integrity": { + "tables": { + "samples": { + "path": record.path, + "size_bytes": record.size_bytes, + "sha256": record.sha256, + } + }, + "assets": [], + "content_id": _inventory_content_id([record]), + }, + } + snap.mkdir(parents=True, exist_ok=True) + (snap / "format.json").write_text(json.dumps(marker, indent=2) + "\n") + + +def test_receipt_path_escaping_the_handed_directory_is_refused(tmp_path: Path) -> None: + """#469: relative ``..`` and absolute paths hash outside the root today; + both must raise before any read (exit 2), not report ok.""" + snap = tmp_path / "snap" + outside = tmp_path / "outside" + outside.mkdir() + secret = outside / "secret.bin" + secret.write_bytes(b"secret-bytes") + + for escape_path in (f"../outside/{secret.name}", str(secret.resolve())): + _hand_built_snapshot_with_receipt_path(snap, receipt_path=escape_path) + with pytest.raises(ValueError, match="must stay under the handed snapshot directory"): + verify_dataset_snapshot(snap) + assert cli_main(["verify", "snapshot", str(snap)]) == 2 + + +def test_normalized_parent_escape_through_an_existing_subdir_is_refused( + tmp_path: Path, +) -> None: + """Kingston's third shape: ``tables/../../outside/...`` only looks like it + failed before because ``snap/tables/`` was missing. Create it and the bare + join escapes; containment must still refuse before the read.""" + snap = tmp_path / "snap" + outside = tmp_path / "outside" + outside.mkdir() + (snap / "tables").mkdir(parents=True) + secret = outside / "secret.bin" + secret.write_bytes(b"secret-bytes") + + _hand_built_snapshot_with_receipt_path(snap, receipt_path=f"tables/../../outside/{secret.name}") + with pytest.raises(ValueError, match="must stay under the handed snapshot directory"): + verify_dataset_snapshot(snap) + assert cli_main(["verify", "snapshot", str(snap)]) == 2 + + +def test_honest_relative_receipt_path_still_verifies_after_containment_gate( + tmp_path: Path, +) -> None: + """Containment must not break a clean export: relative keys under the root + still pass size and sha256 checks.""" + output_directory, _ = _export_two_episode_snapshot(tmp_path, "references") + report = verify_dataset_snapshot(output_directory) + assert report.ok + assert report.findings == [] + assert cli_main(["verify", "snapshot", str(output_directory)]) == 0