[Bug]: Snapshot verify can read outside the handed directory via receipt paths

[VARUN3WARE]

Version or commit

7df224a

Environment

Ubuntu 24.04, Python 3.11, x86_64

Minimal reproduction

from pathlib import Path
import json, tempfile, hashlib
from hflow.snapshot import verify_dataset_snapshot

def sha256_hex(path: Path) -> str:
    digest = hashlib.sha256()
    digest.update(path.read_bytes())
    return digest.hexdigest()

with tempfile.TemporaryDirectory() as temporary_directory:
    root = Path(temporary_directory)
    snap = root / "snap"
    outside = root / "outside"
    snap.mkdir()
    outside.mkdir()
    secret = outside / "secret.bin"
    secret.write_bytes(b"secret-bytes")

    (snap / "format.json").write_text(
        json.dumps(
            {
                "format": "hflow-dataset-snapshot",
                "format_version": 1,
                "media_mode": "references",
                "media_uri_base": None,
                "tables": ["samples.parquet"],
                "integrity": {
                    "tables": {
                        "samples": {
                            "path": "../outside/secret.bin",
                            "size_bytes": secret.stat().st_size,
                            "sha256": sha256_hex(secret),
                        }
                    },
                    "assets": [],
                    "content_id": "00" * 32,
                },
            }
        )
    )

    report = verify_dataset_snapshot(snap)
    print(report.status, report.ok, report.findings)

### Expected behavior

Verify is scoped to the snapshot directory handed in. Docs say recorded paths are “joined only onto the handed directory” (`docs/how-to/export-dataset-snapshot.md`).
A receipt `path` with `..`, an absolute filesystem path, or a drive/UNC form should be refused as unreadable input (exit `2` / `ValueError`) — same spirit as `_validated_relative_key` in `src/hflow/storage.py` — not accepted by hashing bytes outside the delivery.
A normal relative path under that root should still pass when size and sha256 match.

### Actual behavior

Implementation is a bare join in `verify_dataset_snapshot`:
```python
delivered_path = resolved_directory / relative_path

(src/hflow/snapshot.py, integrity tables/assets loop)

On the reproduction above:

status=ok  ok=True  findings=[]

The verifier hashed the sibling file outside snap/ and reported the delivery clean. Absolute paths to the same object also pass. Moved-root tests from #457 do not cover this escape.

Additional context

Same trap Kingston called out on #454 for LeRobot verify: verify what is under the root you were handed, never what a receipt says about where bytes used to live (or live next door).
Honest export already writes POSIX-relative keys under the snapshot; the gap is the verifier accepting out-of-root paths from a tampered or hand-built format.json.
Related: #397 / #401 (integrity writer), #428 / #457 (snapshot verify). Not a duplicate of those — they never scoped receipt path resolution.
Happy to implement after scope is agreed (refuse as exit 2 vs finding; reuse storage key validation vs a local helper).

[chiruu12]

Can I take this one?

Reproduced on current main. The reported case returns ok True [], and a control confirms verify is really reading the file rather than skipping it: recording a wrong sha256 or a wrong size for the outside file turns the report damaged, so the bytes outside the handed directory are what got hashed.

An absolute path works the same way. Pointing path straight at the file outside the snapshot also verifies ok, so .. is not the only spelling.

Planning to resolve each recorded path and refuse anything that does not stay under the snapshot root, raised before any read rather than after, in the spirit of _validated_relative_key in storage.py. Tests for the parent-directory case, the absolute path, and a normal relative path that must still pass.

[VARUN3WARE]

@chiruu12 Thanks for reproducing, that control (wrong size/sha → damaged) is a good check.

I'd like to keep this one: I filed it as a follow-up to the verify path we landed in #454 / #457 and offered to implement in the issue. Happy to take assignment when @kstonekuan is ready.

If you've already started a branch, no worries, we can coordinate; otherwise I'll pick it up once it's assigned.

[kstonekuan]

Reproduced. There's a second shape in it that fails for a different reason:

Path('/a/snap') / '../outside/x'   -> /a/snap/../outside/x
Path('/a/snap') / '/etc/hostname'  -> /etc/hostname

relative escape    ../outside/secret.bin       status=ok ok=True findings=0
absolute path      /tmp/.../outside/secret.bin status=ok ok=True findings=0
normalized escape  tables/../../outside/...    status=damaged findings=1
control (honest)   samples.parquet             status=ok ok=True

With an absolute path, pathlib discards the left operand, so resolved_directory is never consulted at all. Your third row only failed because snap/tables/ doesn't exist to traverse through; create it and that escapes too.

I merged #457 and checked the moved-root property without checking that receipts can't climb out of the root. That one's on me.

On scope, since you asked. Refuse as exit 2. A receipt naming ../ or an absolute path is a malformed receipt, the same class as unparseable JSON, which verify_dataset_snapshot already raises for. A finding would be saying something about the delivered bytes.

_validated_relative_key in storage.py already refuses this family and has tests behind it (test_escaping_keys_are_refused). Reuse it if the contract fits. If it doesn't, tell me what doesn't fit, because two implementations of "is this key inside the root" will drift.

One thing to put in the PR body: anyone who can write format.json can usually write the delivery's files too, so this isn't privilege escalation on a local CLI. What it breaks is the documented promise that recorded paths are "joined only onto the handed directory", and it makes a clean verdict meaningless for any marker we didn't write. That changes once verify runs near tenant-supplied snapshots, which is where #397 and #428 were heading.

Lands with #472 and #473; sequencing note is on #472. Yours.

[VARUN3WARE]

@Sagar-024 Same pattern as #428 / #432 — happy to sequence.

Proposal: you land the identity gate (#472) first on verify_dataset_snapshot (format / format_version, strict like the exporter, plus the int-vs-string "1" decision Kingston called out). I’ll rebase #469 on top and add the path-under-root refusal (exit 2, reuse _validated_relative_key if it fits). Then #473 (content_id) can ride your PR or follow immediately after, since it’s the same function.

If you’d rather one combined PR with both of us, say so — otherwise I’ll wait for #472 to hit main (or your open PR) before pushing #469.

@kstonekuan Works for me either as sequenced PRs or one shared boundary PR; preference above.