fix(drive): treat unrecognised-mime "package" downloads as single-file bundles instead of failures

[epheterson]

Summary

Stops the false "0 successful, N failed" log noise and
per-sync-cycle re-download for iCloud Drive package files that
libmagic can't identify as archives (Apple iWork .key/.pages/.numbers,
third-party .jmb, etc).

Problem

iCloud Drive serves "package" files (Finder bundle types: .band, .key,
.jmb, etc.) via /packageDownload? URLs as opaque archive bytes.
process_package() only recognises application/zip and
application/gzip mime types. Anything else falls through to
return None, which the caller at drive_file_download.py:64
interprets as a hard download failure — even though the bytes have
ALREADY been written to disk and are perfectly usable.

Result: parallel-download log says 0 successful, N failed, and
because the file is marked as "not-done" in bookkeeping, every
subsequent sync cycle re-downloads it.

Fix

• process_package() returns the local_file path on unrecognised
  mime instead of None. The bytes ARE the canonical local
  representation for these bundle types — the file opens in
  Keynote/Pages/etc directly.
• Log level drops from ERROR to INFO: "Package format not recognised
  for unpacking; keeping as single-file bundle: {path}"
• download_file() only treats explicit None as failure (was
  treating any falsy value as failure).

Validation

• New test test_download_file_keeps_unrecognised_package_as_single_file
  exercises the new success path.
• Renamed test test_process_package_unrecognised_mime_preserves_file
  documents the new contract.
• Fixed test_download_file_returns_none_on_processing_failure to
  patch the correct module-level binding (was patching wrong path
  but happened to pass on the old contract).
• Real-world: Eric's 111K-photo library + 40 .jmb / .key files —
  before this PR every sync re-downloaded ~300 MB of these files
  and logged ~50 ERROR lines per cycle. After: files preserved,
  logs clean.

Known follow-up

The next-sync dedup may still trigger a re-download because
file_exists() compares getsize(local_file) to item.size, and
for flat bundles those differ (item.size is the unpacked contents
total). A sidecar-marker or mtime-fallback comparator would close
the loop. Out of scope for this focused fix.

[Copilot]

Pull request overview

This PR adjusts iCloud Drive “package” handling so that package downloads with unrecognised MIME types are preserved as usable single-file bundles (instead of being counted as failed downloads and repeatedly re-downloaded). It also adds a drive.flatten_packages configuration option and new tests to cover package extraction layout and flattening behavior.

Changes:

• Update process_package() to treat unknown MIME types as “keep bytes as-is” (return the existing local path) and support an opt-in flatten mode to skip unpacking.
• Update drive download flow to only treat explicit None from process_package() as a processing failure, and plumb flatten_packages through the parallel-download task pipeline.
• Add/adjust tests to codify the new contract and validate ZIP extraction layout behavior + config getter.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
src/drive_package_processing.py	Extends package processing contract; adds flatten option; changes ZIP extraction layout heuristic.
src/drive_file_download.py	Updates download flow to pass flatten flag and interpret process_package() results correctly.
src/drive_parallel_download.py	Threads flatten_packages through download task info and into download_file().
src/config_parser.py	Adds get_drive_flatten_packages() config getter and related formatting updates.
tests/test_sync_drive.py	Updates/extends tests for new “unknown MIME preserved” contract and correct patching location.
tests/test_drive_package_bundle_layout.py	New tests covering ZIP layout extraction behavior and the flatten-packages config knob.

[mandarons]

/opencode review this code.

[opencode-agent]

Here's my review of the PR:

Overall Assessment

The PR addresses a real problem (false failures on unrecognised mime types) and the core fix is correct. However, there are several issues worth addressing.

---

Critical Issues

1. Zip Slip protection is incomplete — CWE-22 still exploitable

_safe_extractall (drive_package_processing.py:135-180) validates paths but still delegates to extractall. A zip member with a symlink entry can bypass the realpath check at extraction time, then be followed by a subsequent write to overwrite arbitrary files. The safer pattern is to extract per-member with explicit file writes, or use zipfile.Path.

Additionally, the traversal layout handler (line 214) widens safety_boundary to os.path.dirname(parent_dir), which means a crafted zip with ../../etc/passwd entries would pass the check and write to grandparent's grandparent. The test at test_drive_package_bundle_layout.py:351-384 asserts the bypass works (blocked == []), treating it as correct behavior. This is dangerous — the legitimate ../<bundle>/ pattern should be handled by renormalizing paths, not by widening the safety net.

Recommendation: Replace _safe_extractall + extractall with per-member extraction that normalizes each path and writes explicitly. For the traversal layout, strip the ../ prefix rather than widening the boundary.

2. _zip_entries_self_prefixed trusts the zip's stated filenames

The heuristic at drive_package_processing.py:122-132 checks all(n.startswith(prefix) for n in names) where names comes from zf.namelist(). A malicious zip can contain entries with arbitrary prefixes while still having a bundle_basename/ entry. The check should also verify the bundle directory entry itself exists.

3. Recursive process_package call on gzip has no depth limit

_process_gzip_package (line 257) recursively calls process_package(local_file=local_file). If a gzip stream contains another gzip stream (or a zip), this recurses. There's no depth guard — a crafted chain could cause a RecursionError. Add a max_depth parameter or iteration limit.

---

Medium Issues

4. flatten early-return should come before magic.Magic() instantiation

The Copilot review noted this — when flatten=True, libmagic detection at line 64-65 is wasted work. The current code already handles this correctly (the early return is at line 53, before line 64), but the magic_object instantiation on line 64 is still executed when flatten=False for the non-zip/gzip path. This is fine but worth noting for clarity.

5. getattr pattern in drive_parallel_download.py:68-72 is fragile

flatten_packages = (
    bool(config_parser.get_drive_flatten_packages(config))
    if config
    else False
)

The Copilot review suggests calling get_drive_flatten_packages(config) directly since it handles None. The current code is functionally correct but unnecessarily verbose — get_drive_flatten_packages(None) returns False, so the if config guard is redundant.

6. Unicode normalization inconsistency

_process_zip_package normalizes to NFD (line 226), while collect_file_for_download normalizes to NFC (line 59). This means a file collected as NFC could be renamed to NFD during unpacking. Downstream file_exists checks may then fail to match. The PR description acknowledges a known follow-up for dedup, but this NFD/NFC mismatch should be resolved within the PR — it's a regression vector.

7. _safe_extractall uses os.rename for NFD but os.remove for cleanup

Line 228-229 renames for normalization, line 231 removes the archive. If the rename fails (e.g., target exists), the archive is still removed. The os.rename should have error handling or use os.replace.

---

Low Issues / Nits

8. Docstring on process_package mentions JMG but the mime comment says JMB

Lines 46-47 mention .jmb but the comment on line 74 says "JMG .jmb" — pick one. Minor inconsistency.

9. TestProcessPackageNeverTouchesRegularUserZips uses source scanning for invariant

The test at line 430-463 scans src/*.py for callers of process_package. This is brittle — a refactor that moves the URL gate to a helper function would break the test without breaking the invariant. Consider documenting this as a known fragility, or switch to a runtime assertion.

10. Tests use assert statements outside test methods

test_drive_package_bundle_layout.py:199-205 uses bare assert inside with blocks rather than self.assertEqual. While functionally equivalent, it breaks the unittest convention and provides worse failure messages.

---

Test Quality

The test suite is thorough. The following are well-done:

• _safe_extractall Zip Slip tests cover absolute paths, .. traversal, and boundary widening
• The sibling iWork collision test (test_two_sibling_iwork_zips_dont_collide) directly validates the real-world bug
• test_url_gate_is_the_only_caller locks down the process_package call site invariant
• test_download_file_returns_none_on_processing_failure now patches the correct module binding

One gap: there's no test for the recursive gzip→zip case (gzip wrapping a zip, which recurses). Consider adding one to guard against stack overflow.

opencode session  |  github run

[epheterson]

Went through the opencode findings:

Zip Slip / CWE-22 (1) — verified false positive, left extraction as-is. Python's ZipFile.extractall sanitises entries: a ../../../tmp/x entry lands inside the extract dir, not /tmp, and a symlink-mode entry extracts as a regular file, not a symlink — so neither the traversal nor symlink-through vector applies. _safe_extractall is redundant defence-in-depth, not a hole. (Can simplify the boundary logic in a follow-up since Python's sanitisation makes it moot, but it's not a vuln.)

Recursion depth (3) — fixed. gzip carries no depth signal, so a gzip-of-gzip chain could hit RecursionError. Added a _MAX_PACKAGE_DEPTH ceiling threaded through the gzip path, with a test that a >ceiling chain returns instead of crashing.

Config guard (5) — fixed. get_drive_flatten_packages(None) already returns False, so dropped the redundant if config.

2 and 6 are backstopped / known follow-ups. Full suite green, 100%.