fix(viewer): stream ZIP fallback in playground - #90
Merged
Conversation
Add a test for the S3/object-storage case that motivated widening the readEntry() guard: getLocalFile() returning an empty string rather than false/null. Extract the archive/fake-File setup shared with the existing playground test into two small helpers, and drop the class doc comment's now-stale claim that the reading methods are too heavy to fake here.
Contributor
Preview this PR in the Nextcloud PlaygroundA fresh Nextcloud boots in your browser with this branch's eXeLearning editor: |
OCP\Files\File is an interface in all supported Nextcloud versions, so the previous class_exists() guard never detected it and the eval()'d class would collide with the real API outside the standalone bootstrap. Stub the interface in bootstrap-standalone.php (guarded by interface_exists) and build the fake file with createMock() instead.
The File::fopen() fallback returned entries without applying MAX_UNCOMPRESSED_SIZE_BYTES, and widening the fallback to every ZipArchive::open() failure widened that gap too. Extract the entry read into a shared extractEntry() so the local-path and stream branches enforce identical limits and agree on missing entries. The read is bounded by the declared entry size (validated against the limit first) rather than the limit itself: getFromName() preallocates its whole $len buffer, so passing the 500MB limit would allocate 500MB per request. The extra byte past the declared size exposes archives whose central directory understates the real entry size. The limits are constructor-injectable (defaulting to the class constants) so tests can exercise them with small archives.
The real OCP\Files\File declares fopen($mode) with the type only in the docblock, and the stub's own comment already promised untyped methods.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
ZipEntryService::readEntry()falls back to the portable stream reader(
readEntryFromStream()) only whengetLocalFile()returnsfalseornull. In practice, several storage backends return neither of those:return an empty string instead of
false/null.that native
ZipArchive::open()still refuses to open.path backed by a virtual filesystem;
ZipArchivecannot open it eventhough the path "looks" local.
In all three cases,
readEntry()returnednull, andAssetControlleranswered with a 404 "Entry not found" — the viewer stayed blank with no
indication of what went wrong, even though the file was perfectly
readable through
File::fopen().Fix
=== false || === nullto!is_string() || === "", so any non-usable "local path" value routesthrough the stream fallback.
ZipArchive::open()still fails toopen it, retry through
readEntryFromStream()instead of returningnull.readEntryFromStream()already existed and is exercised bythe plain no-local-path case; it was simply unreachable once a
(bogus) local path was reported.
Hardening (from review)
MAX_UNCOMPRESSED_SIZE_BYTES, agap this PR would have widened to every
ZipArchive::open()failure.Entry extraction now lives in a shared
extractEntry()that both thelocal-path branch and the stream fallback call, so they enforce
identical limits (
MAX_ENTRIES,MAX_UNCOMPRESSED_SIZE_BYTES) andagree on returning
nullfor missing entries.strlen()afterloading the whole entry: the declared size from
statName()isvalidated against the limit first, then
getFromName()reads at mostdeclared size + 1bytes. The bound is the declared size rather thanthe 500MB limit because
getFromName()preallocates its entire$lenbuffer up front — bounding by the limit would allocate 500MBper request regardless of entry size. The extra byte exposes archives
whose central directory understates the real entry size (rejected
with a
RuntimeException).constants, so tests can exercise them with small archives. Nextcloud's
DI autowires scalar parameters with defaults, so production wiring is
unchanged.
Testing
Fileused by the tests was rebuilt (review finding):OCP\Files\Fileis an interface in all supported Nextcloudversions, so the previous
class_exists()guard never detected it andthe eval'd class would collide with the real API outside the
standalone bootstrap.
bootstrap-standalone.phpnow stubs a minimalFileinterface (guarded byinterface_exists(), methods untyped asin the real API) and the tests build the double with
createMock().testReadEntryFallsBackToStreamWhenLocalPathCannotBeOpened()simulatesa storage whose
getLocalFile()returns a pathZipArchivecannotopen (the php-wasm Playground case), and asserts
readEntry()stillreturns the correct entry contents via the stream path.
testReadEntryFallsBackToStreamWhenLocalFileIsEmptyString()covers theS3/object-storage case that originally motivated this fix:
getLocalFile()returning""rather thanfalse/null.branches (stream fallback and local path — the latter previously had
no coverage at all), the entry-count limit is asserted through the
fallback, and a zero-byte entry still reads as
''(the bounded-readedge where
getFromName()is asked for a single byte).Full unit suite (15 tests) plus php-cs-fixer are clean locally; CI is
green across stable31–33 / PHP 8.2–8.5.
Follow-up
listEntries()still uses the old=== false || === nullguard and hasno stream fallback at all. It has no callers today, so this PR leaves it
untouched rather than changing behavior nothing exercises — but the same
class of storage backend could hit the same blank-result failure mode if
a caller shows up. Worth a follow-up issue if/when
listEntries()gets areal caller.