fix: harden against confirmed review findings

[conorbronsdon]

From an automated multi-agent review (personal-context#62); implemented + verified by Claude Code.

Each fix was reproduced against the current source, then re-checked after the fix. Every finding has a new regression test in test/test_tar.mojo; all 8 new tests fail on the pre-fix source and pass after (verified by stashing the src changes and re-running). Full suite: 37/37 pass; pixi run fuzz and pixi run demo both clean.

1. (security, headline) Bad-checksum "resync" reinterpreted member data as headers

_parse_archive skipped only a single block on a checksum mismatch, so a file member's data was reparsed as top-level headers. A file whose contents are themselves a valid tar surfaced its inner members as top-level entries — a content-smuggling / scanner-evasion differential (a scanner sees one opaque corrupt file; mojo-tar surfaces the hidden member).

Fix: skip the whole member (header + padded declared size). If the corrupt block's size field is implausible (<0 or > archive), resync is impossible → raise. Pending GNU long-name/long-link/pax overrides are cleared so a corrupt member can't leak them onto a later member.

Before / after (repro: outer archive readme.txt + innocent.dat, where innocent.dat's content is an inner tar carrying SMUGGLED.txt; corrupt innocent.dat's header name byte):

before:  top-level members surfaced: 2   ->  readme.txt, SMUGGLED.txt   # smuggled member leaked
after:   top-level members surfaced: 1   ->  readme.txt                 # corrupt member skipped as a unit

Regression tests: test_bad_checksum_data_not_reinterpreted, test_bad_checksum_implausible_size_raises.

2. (security) Base-256 size decode wrapped a 95-bit field into 64 bits

_get_num shifted an up-to-95-bit base-256 field into a signed 64-bit Int, silently wrapping a huge size to a small/negative value. Fix: raise if the running value would overflow on the next << 8 (v > Int.MAX >> 8). Regression test: test_base256_oversized_size_raises.

3. (correctness) add_symlink silently truncated long names/targets

add_symlink passed name/target straight into the 100-byte ustar fields, silently truncating anything longer; over-length uname/gname (32-byte field) were also truncated silently. Fix: add_symlink now emits pax path/linkpath records for >100-byte name/target (mirroring add), so they round-trip; _set_str now raises on any field overflow instead of truncating. Regression tests: test_writer_symlink_long_name_roundtrips, test_writer_symlink_long_target_roundtrips, test_writer_oversized_uname_raises.

4. (security/doc) GNU sparse members desynced the parse

A GNU sparse member (typeflag S, or GNU.sparse.* pax records) stores a hole map plus only non-hole bytes, so its archived length differs from its logical size — parsing it as a normal member desyncs the rest of the archive. Fix: detect and raise on both forms. Documented in the module docstring and SECURITY.md. Regression tests: test_gnu_sparse_typeflag_raises, test_gnu_sparse_pax_raises.

Build prerequisite (bundled)

The pixi.toml mojo pin >=1.0.0b3 sorts below dev nightlies, so pixi install failed to solve on the nightly toolchain. Fixed to >=1.0.0b3.dev0,<2 (same class of fix confirmed in mojo-redis). Verified pixi install now solves.

Notes on scope / parallel work

• Per task scope I did not touch README.md or CHANGELOG.md (a separate agent is editing those in parallel PRs). Overlap to flag: finding 4 asks for a "Sparse files" bullet in the README "What it deliberately does NOT do" list — I documented sparse in the module docstring + SECURITY.md instead; the README bullet should be added by the docs PR.
• The security note in SECURITY.md (findings 1 & 4) is the one file-doc change that belongs with these fixes.

Draft — not for merge without Conor's review.

[conorbronsdon]

🤖 Independent Claude review: Ready to mark for review.

Second-look pass on all four fixes + the pixi pin. No blocking correctness/security issue found.

Verified:

• Bad-checksum smuggling (headline): honoring the declared size and skipping the whole padded member closes the skip-one-block differential. Checked the obvious re-open — an attacker declaring a smaller size does not resurface smuggled content, because any size-honoring reader treats the trailing bytes as the next member too, so mojo-tar stays internally consistent. skip_end/bad_size bounds keep pos strictly monotonic (skip_end >= pos+512). Pending have_long_name/have_long_link/pax correctly dropped on skip so a corrupt member cannot leak overrides forward. Headline test genuinely fails pre-fix.
• base-256 overflow (_get_num): guard is placed before the << 8; tight at the boundary (v == Int.MAX>>8 shifts to Int.MAX-255, | byte cannot overflow). Correct.
• add_symlink long name/target: pax path/linkpath emitted and re-applied on read; _set_str raising (vs silent truncate) is the right hardening. Round-trip tests are real.
• GNU sparse: both typeflag S and pax GNU.sparse.* forms covered — the pax key set spans sparse 0.0/0.1/1.0 (offset/numbytes, map, major/minor/name/realsize). Good.
• pixi pin: >=1.0.0b3.dev0 correctly admits dev nightlies (.dev0 sorts below b3 in PEP440/conda ordering).

Non-blocking notes (polish only):

1. _set_str now raises instead of truncating an oversized uname/gname — a behavior change for writer callers. Worth a CHANGELOG/README note when the docs PR lands.
2. The bad_size > n and skip_end > n resync-abort branches are not directly exercised: test_bad_checksum_implausible_size_raises uses a base-256 size that trips the _get_num overflow guard first (byte 0x01 at offset 125 → ~88-bit value → raises before those branches). An octal oversized size (e.g. eleven 7s) would reach them.
3. No test covers the defensive "drop pending pax/longname overrides on corrupt-member skip" path.

Draft is sound; the three notes are optional follow-ups, not merge blockers.

[conorbronsdon]

Code review (opus, static — no toolchain, verify CI): CHANGES-NEEDED

Findings 2/3/4 are correct and well-executed (base-256 overflow guard, _set_str raise-on-overflow, pax symlink round-trip). But the headline anti-smuggling fix is bypassable:

• src/tar/tar.mojo:276-294 — on a bad-checksum member the resync uses bad_size = _get_num(block, 124, 12) from the corrupt, attacker-controlled header. A crafted size=0 passes the plausibility gate → skip_end = pos + BLOCKSIZE, degrading to the old single-block skip, so the member's data is reinterpreted as top-level headers. The exact smuggling vector this PR claims to close is re-opened with one crafted byte. (HIGH)
• SECURITY.md addition asserts the member "is skipped as a whole unit … never one block at a time" — false for size=0. (docs defect)
• test/test_tar.mojo::test_bad_checksum_data_not_reinterpreted only flips a name byte (honest size), so it never exercises the crafted path. (test gap)

Fix: on a non-first bad-checksum block, raise (mirror tarfile's ReadError, consistent with the first-block branch) instead of resyncing on an untrusted size. A fix + a size=0 regression test is being applied to this branch now.