Skip to content

Unpack only the .lift member for streaming zip reads - #17

Open
imnasnainaec wants to merge 8 commits into
mainfrom
zip-read-only-lazy-extract
Open

Unpack only the .lift member for streaming zip reads#17
imnasnainaec wants to merge 8 commits into
mainfrom
zip-read-only-lazy-extract

Conversation

@imnasnainaec

@imnasnainaec imnasnainaec commented Aug 4, 2026

Copy link
Copy Markdown
Collaborator

stats and export stream entries and resolve neither companion ranges nor media, but reading a zipped package extracted the whole archive first — writing a media-heavy package's audio and WritingSystems/ to a temp directory that nothing then read. On a real audio-carrying project that is gigabytes of pure overhead per invocation.

lift_source() now narrows the write to the single .lift member. load(), validate, and check-media still unpack the full folder, which they need to resolve companions and check media presence.

User story

A Combine or FieldWorks project backup is a .zip whose shape is lopsided: single-digit MB of .lift XML beside gigabytes of audio and images. A project manager receiving those backups runs sil-lift stats backup.zip --format json on each one — in CI, or in a triage script over a folder of them — to track entry and sense counts and catch a truncated export before it is imported. Counting reads only the XML.

Before this change, every invocation wrote the whole package to a temp directory, read a few MB of it, and discarded the rest. On a CI runner with bounded temp disk, a handful of those in sequence is a real failure mode. Now stats writes one file. export has the same shape — pulling a TSV of glosses out of a package to hand to a reviewer reads the XML and nothing else.

Guards

  • Entry-count cap and path-traversal rejection: unchanged, and still evaluated over the whole listing however much of it gets written — so a hostile archive is refused identically on both paths.
  • Per-file streaming cap: unchanged on both paths.
  • Aggregate uncompressed-size cap: full extraction only. It guards bytes written to disk, and a streaming read writes one member — so stats on a package whose media dwarfs the limit now succeeds where load() still refuses it. Deliberate, and covered by a test.

Member selection

Locating the .lift moves from globbing the extracted tree to matching archive member names, so both paths share one rule instead of the change growing a second near-duplicate; _find_lift_root() is gone.

That makes the suffix match case-insensitive on every platform. Previously it inherited Path.rglob's platform behavior, so a .LIFT member resolved on Windows but not on Linux or macOS.

Verification

python scripts/check.py green (382 passed, 94% total, _zip.py at 100%), plus mkdocs build --strict for the docs edits. New tests cover the extraction narrowing directly — asserting the temp tree holds the .lift and nothing else — along with the traversal and per-file cap paths in streaming mode and the deliberate aggregate-cap divergence.

Draft: opening for review of the guard trade-off above before marking ready.

🤖 Generated with Claude Code


Devin review: https://app.devin.ai/review/sillsdev/python-sil-lift/pull/17


This change is Reviewable

@imnasnainaec imnasnainaec self-assigned this Aug 4, 2026
imnasnainaec and others added 5 commits August 12, 2026 14:28
The stats and export commands stream entries and resolve neither companion
ranges nor media, so extracting a whole package wrote its audio and
writing-system files to a temporary directory for nothing. lift_source()
now narrows the write to the single .lift member; load(), validate, and
check-media still unpack the full folder, which they need to resolve
companions and check media presence.

The aggregate uncompressed-size cap guards bytes written to disk, so it
now applies to the full extraction only; the per-file cap and the entry
count and path-traversal checks apply to both paths, the latter two over
the whole listing however much of it gets written.

Selecting the .lift moves from globbing the extracted tree to matching
archive member names, letting both paths share one rule rather than
growing a second near-duplicate. That makes the suffix match
case-insensitive on every platform, where globbing resolved a .LIFT
member on Windows but not on Linux or macOS.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
De-duplicate archive member names before the "multiple .lift files" count,
so a package storing one path twice — some writers append a record rather
than replace it — still resolves to the single file that extraction, which
overwrites, leaves on disk.

Report which member overflowed when the streaming path's running cap
fires: there the limit bounds one file rather than the package, so the
package-wide wording misdescribes it.

Document two consequences of matching member names: a .LIFT member's
conventional sibling companion does not resolve on a case-sensitive
filesystem, since that candidate is derived from the suffix, and narrowing
the write leaves one member able to reach the full cap before refusal.

Derive the extraction-scope test's temp root from the member's own depth
and assert it is the temp directory, so the check cannot silently walk
above it if the fixture layout changes.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The note read as though matching member names case-insensitively created
the companion-resolution question. It does not: a case-variant .lift
filename raises the same question loaded from a plain folder, and
_resolve_ranges is what answers it. Point there instead of describing an
outcome this function does not control.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
0.1.0 has not shipped, so there is no released behavior for an Unreleased
entry to be changing — unpacking only the .lift for streaming reads is
simply what the first release does. Fold it into the zipped-packages
bullet and drop the note about how the member is located, an
implementation detail with no released counterpart.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The prose around the streaming read used a second word for concepts the
package already names: "unpack" beside "extract", "decompression bomb"
beside the "zip bomb" the changelog and interop guide use, and "record"
for a zip listing entry, which collides with the writer's own records.
Settle on one term for each.

Replace the shorthand where it stands in for the mechanism: the
"aggregate declared-size check" is the up-front check of the whole
listing's declared sizes, "the streaming two" are the streaming
commands, and media that "dwarfs" the limit is media far larger than it.
The test named after the aggregate cap follows.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@imnasnainaec
imnasnainaec force-pushed the zip-read-only-lazy-extract branch from 7340c01 to be718c8 Compare August 12, 2026 19:11
A streaming read skips the whole-listing size pre-check, since it writes
one member rather than the package. That left an honestly declared zip
bomb in the .lift to stream up to the 10 GiB cap into the temp directory
before being refused; checking that one member's declared size instead
refuses it with nothing written. The count as bytes stream still guards
a declared size that lies.

Resolving the member through getinfo() also collapses a path stored
twice in the listing to the single entry extraction would leave on disk,
so it is no longer written -- and counted against the cap -- twice.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@imnasnainaec
imnasnainaec marked this pull request as ready for review August 12, 2026 20:07
@imnasnainaec imnasnainaec added the 🟨Medium Medium-priority PR label Aug 12, 2026
@imnasnainaec
imnasnainaec marked this pull request as draft August 13, 2026 18:16

@jasonleenaylor jasonleenaylor left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider the api surface and user documentation detail level with regards to security hardening (probably less is more)

@jasonleenaylor reviewed 5 files and all commit messages, and made 1 comment.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on imnasnainaec).

imnasnainaec and others added 2 commits August 13, 2026 15:30
The changelog and interop guide explained why the guards exist -- hostile
archives, zip bombs, a declared size that can lie -- and how the check is
split between the declared sizes and the bytes as they stream. None of
that changes what a reader does: the limits and the refusal do. Give the
two numbers, say a package over either is refused, and say the same of
one whose member paths escape the extraction directory. The reasoning
stays in _zip.py, beside the constants and the extraction it governs.

Keep the streaming narrowing's user-visible consequence, which the cap
text carried: extracting only the .lift is what makes stats and export
cheap on a media-heavy package, and it is why their limit applies to
that one file rather than to the media beside it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The path a zip read hands back is built from the archive member name
rather than found on disk, so the write and the yield share a derivation
that nothing checks still agrees. A backslash separator is where the two
would come apart: POSIX takes it as one filename and Windows as a
directory split, so normalizing it on either side alone leaves the yield
pointing at a path nothing wrote.

Extract a member stored with that separator and read the .lift's bytes
back through the yielded path.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@imnasnainaec
imnasnainaec force-pushed the zip-read-only-lazy-extract branch from 8288db4 to bf34666 Compare August 13, 2026 20:36
@imnasnainaec
imnasnainaec marked this pull request as ready for review August 13, 2026 20:41

@imnasnainaec imnasnainaec left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

User doc details adjusted/reduced.

User story added to pr body to justify api surface.

@imnasnainaec reviewed all commit messages and made 1 comment.
Reviewable status: 2 of 5 files reviewed, all discussions resolved (waiting on jasonleenaylor).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🟨Medium Medium-priority PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants