Repository navigation
fix(sdk): DSPX-4589 zip64 + manifest conformance (combined view of #397 + #398) - #396
Closed
dmihalcik-virtru wants to merge 2 commits into
Closed
dmihalcik-virtru wants to merge 2 commits into
dmihalcik-virtru wants to merge 2 commits into
Conversation
Contributor
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueThanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
dmihalcik-virtru
force-pushed
the
DSPX-4589-zip64-conformance
branch
from
September 3, 2026 20:12
9053c2c to
49689fe
Compare
Contributor
…defaults integrityInformation.segments[].segmentSize and .encryptedSegmentSize are optional in the TDF spec: manifest.schema.json marks segmentSizeDefault and encryptedSegmentSizeDefault required on integrityInformation but puts no required list on segments/items, so an absent per-segment value means "the default", not zero. Gson leaves an absent primitive at 0, so java-sdk read those segments with a zero length buffer and failed inside the integrity check. web-sdk omits a per-segment size whenever it equals the default, which is every full segment, so every web-sdk TDF larger than one default segment (1 MiB) failed to decrypt, surfacing as a confusing integrity error rather than as a manifest problem. A primitive long cannot distinguish an absent JSON key from a literal 0, so a Gson TypeAdapterFactory registered for IntegrityInformation walks the parsed segments array alongside the deserialized list and fills in the defaults only where the key is absent or JSON null. Boxing Segment.segmentSize to Long was the other option, but it breaks the public API for no added behavior. An explicit 0 in the JSON is preserved as 0. TDF.Reader.readPayload additionally rejects a segment with a non-positive encryptedSegmentSize up front -- an encrypted segment always carries at least an IV and a tag -- so a manifest that supplies neither a per-segment size nor a usable default now says so instead of failing downstream with an unrelated complaint about the payload being too small to GMAC.
…, UTF-8 entry names Three zip container conformance fixes found while auditing the TDF zip container against PKWARE APPNOTE.TXT. 1. ZipWriter only set the zip64 flag on the end of central directory record when the entry count exceeded 0xFF or the central directory offset/size exceeded 0xFFFF. Those masks do not match the field widths: the entry count is 2 bytes and the offset and size are 4 bytes each. Archives with between 256 and 65534 entries were needlessly promoted to zip64, and the offset/size checks now go through needsZip64 so they honor the same 2 GiB ceiling as the per-entry fields. 2. ZipReader treated a short read while scanning backwards for the end of central directory signature as a signature match, so a truncated archive could fall out of the scan loop and parse whatever followed as an end of central directory record. It now only breaks on a real match and throws InvalidZipException otherwise, and rejects an archive too small to hold the zip64 locator it claims to have. 3. ZipWriter computed the central directory filename length from String.length() rather than from the UTF-8 encoded byte count. The value was assigned to a field that write() never read, so the bytes on the wire were already correct, but the dead field is removed, the name is encoded once instead of twice, and a name too long for the 2 byte length field is now rejected instead of silently truncated.
dmihalcik-virtru
force-pushed
the
DSPX-4589-zip64-conformance
branch
from
September 9, 2026 12:51
49689fe to
5196409
Compare
This was referenced Sep 9, 2026
dmihalcik-virtru
changed the title
|
Contributor
dmihalcik-virtru
added a commit
to opentdf/tests
that referenced
this pull request
Sep 10, 2026
Closes [DSPX-4592](https://virtru.atlassian.net/browse/DSPX-4592). ## Why ZIP central-directory offsets and sizes are 32-bit **unsigned** on the wire. A reader that widens one with a signed read sees anything `>= 2**31` as negative; at or above `2**32` the format mandates the ZIP64 sentinel, so the 32-bit field never holds a real value. That leaves exactly one broken window, `[2**31, 2**32)`, and nothing in this suite reached it — `--large` is 5 GiB, which steps straight over. ## What lands - `xtest/sizes.py` — adds `medium` (2 254 857 830 B, ~102 MiB inside the low edge of the window — shrinking it doesn't make the test cheaper, it makes it vacuous) and the window predicates (`in_zip64_window`, `exercises_zip64_window`). - `xtest/zipinspect.py` — a raw ZIP central-directory reader that keeps the 32-bit fields alongside the resolved values. `zipfile` normalises ZIP64 away, which is exactly the encoding under test. Lets a failure name the SDK at fault instead of reporting "decrypt failed" after an hour and 6 GiB of IO. - `xtest/test_zip64.py` — the roundtrip cell, marked `zip64` and **deselected** (not skipped) unless the session's sizes reach `2**31`. Asserts an offset actually landed in the window, so a mis-sized payload fails rather than passes vacuously. Writer conformance is checked *before* the reader xfail is applied, so a writer regression can't hide behind a known reader bug. Reuses `tdfs.skip_chunky_skew` (from [#590](#590)) to keep the independent segment-defaulting defect out of the ZIP64 result. - `tdfs.zip64_reader_xfail` — `xfail(strict=True)` keyed on semver for java decryptors predating java-sdk#393. Strict, so the cell must flip to a hard failure when the fix ships and somebody deletes the predicate. - A nightly-only `zip64` job in `xtest.yml`: own 90 m timeout, matrixed over the encrypting SDK, no `--skip-released-pairs` (a released java decryptor is the point). Parses its own junit XML and fails if no cell executed. Also pins the `bench` job's platform ref through the same resolved main SHA the zip64 job uses, so both share one commit instead of resolving "main" independently. - `xtest/test_zip64_units.py` (20 tests) on the offline PR gate, since the nightly's verdict is only as good as this parser. - `spec/DSPX-4592.md` — spec and live-run findings. ## Sibling PRs | Repo | PR | Covers | |---|---|---| | java-sdk | opentdf/java-sdk#396 | DSPX-4589 — `readUnsignedInt`, `needsZip64`, segment-size defaulting | | platform (go) | opentdf/platform#3979 | DSPX-4590 — `resolveSegmentSizes`, `LoadTDF` payload size | | web-sdk | opentdf/web-sdk#1017 | DSPX-4591 — ZIP64 writer conformance | Stacked on [#590](#590) (chunky segment-defaulting), which stacks on [#589](#589) (configurable payload sizes), which stacks on [#588](#588) (XT_FORCE_SUPPORTS). This PR is scoped to ZIP64 conformance only — chunky segment-defaulting coverage split out to #590 since it's an orthogonal, independently-mergeable concern found along the way. ## Follow-ups (not in this PR) - Once the go and java fixes release, replace the `exit 1` in the `chunky)` case of `xtest/sdk/{go,java}/cli.sh` with real version gates. - Consider widening `zip64_reader_xfail` once the first nightly reports which cells actually fail. ## Verification `ruff check` / `ruff format` / `pyright` clean from `xtest/`. Full offline harness suite (177 tests) passes. `actionlint` on `xtest.yml`/`check.yml` reports the same 15 pre-existing shellcheck info findings as `main`, no new ones. Draft: the `zip64` job has not had a live `workflow_dispatch` run yet. Doing that against this branch is the last gate before marking ready. [DSPX-4592]: https://virtru.atlassian.net/browse/DSPX-4592?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Added configurable payload-size testing for small, medium, chunky, and large scenarios. - Added optional controls for forcing feature support and running ZIP64 validation workflows. - Added cross-SDK ZIP64 boundary coverage for large files and multi-segment containers. - **Bug Fixes** - Improved detection and reporting of malformed ZIP64 structures and unexpected test-support errors. - **Documentation** - Documented test-size options, environment settings, and ZIP64 validation coverage. - Documented the deprecated `--large` option alias. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
dmihalcik-virtru
added a commit
that referenced
this pull request
Sep 21, 2026
…defaults (#397) Jira: https://virtru.atlassian.net/browse/DSPX-4589 **Stack — this is 1 of 2.** Split out of #396 so the fix with actual field impact can be reviewed and land on its own. | | PR | contents | |---|---|---| | 1 | **this PR** (base `main`) | per-segment size defaults | | 2 | #398 (base this) | zip64 EOCD sentinels, truncated archive detection, UTF-8 entry names | | — | #396 (base `main`) | the combined diff of 1 + 2, as originally opened | --- `integrityInformation.segments[].segmentSize` and `.encryptedSegmentSize` are optional in the TDF spec; when absent, the reader is supposed to fall back to `segmentSizeDefault` / `encryptedSegmentSizeDefault`. `manifest.schema.json` marks the two defaults required on `integrityInformation` but puts no `required` list on `segments/items`, so the per-segment values are optional overrides and an absent one means "the default", not zero. Gson left the absent primitives at `0`, and java-sdk read the payload with a zero-length segment. **Every web SDK TDF larger than one default segment (1 MiB) failed to decrypt in java-sdk**, surfacing as a confusing integrity error rather than as a manifest problem. A primitive `long` cannot distinguish an absent JSON key from a literal `0`, so the fix consults the parse tree: a Gson `TypeAdapterFactory` registered for `IntegrityInformation` walks the parsed `segments` array alongside the deserialized list and fills in the defaults only where the key is absent or JSON `null`. Boxing `Segment.segmentSize` to `Long` would have been the other option, but it breaks the public API (`==` in `Segment.equals`, an `int` -> `Long` assignment in `TDF`, existing `assertEquals(Long, int)` in tests) for no added behavior, so the post-deserialization fixup was chosen instead. Explicit `0` in the JSON is preserved as `0`. `TDF.Reader.readPayload` additionally rejects a segment with a non-positive `encryptedSegmentSize` up front — an encrypted segment always carries at least an IV and a tag — so a manifest that supplies neither a per-segment size nor a usable default now says so instead of failing downstream with an unrelated complaint about the payload being too small to GMAC. ## Tests 4 new tests: - `ManifestTest.testAbsentSegmentSizesFallBackToTheManifestDefaults` — absent / partially overridden / fully overridden, plus a `toJson` round trip. - `ManifestTest.testExplicitZeroSegmentSizeIsNotTreatedAsAbsent`. - `TDFTest.testReadingATDFThatOmitsDefaultedSegmentSizes` — encrypts ~2 MiB + 4242 bytes at a 1 MiB segment size, strips every per-segment size equal to the default from the manifest, and asserts a byte-exact decrypt. - `TDFTest.testZeroLengthSegmentIsRejectedWithAClearError`. Confirmed to be genuine regression tests by reverting the `registerTypeAdapterFactory` line and watching them fail. ``` mvn --batch-mode verify -Dmaven.antrun.skip -P 'coverage,non-fips,!fips' -> 235 tests, 0 failures, 0 errors, 8 skipped (231 before this change) [JDK 21] ``` ## End-to-end validation Run on the `opentdf/tests` `DSPX-4592-02-chunky` branch, which adds `test_tdfs.py::test_chunky_roundtrip` — a 5 MiB round trip, versus the 128 bytes the suite has used for four years, which is what it takes for a writer to emit a segment whose size equals the manifest default. Both runs pass `force-supports=chunky`, which makes `tdfs.skip_chunky_skew` return early so the cell reports a real pass or fail instead of skipping on the unreleased version gate. | | `java-ref` | run | `js -> java` chunky cell | |---|---|---|---| | fix | `DSPX-4589-01-segment-size-defaults` | [34353420418](https://github.com/opentdf/tests/actions/runs/34353420418) ✅ | **PASSED** | | control | `main` (this PR's base) | [34355312405](https://github.com/opentdf/tests/actions/runs/34355312405) ❌ | **FAILED** | Exactly one cell flips between the two runs. Every chunky pair, side by side: | encrypt -> decrypt | control (`java@main`) | fix (`java@this-branch`) | |---|---|---| | **js -> java** | **FAILED** | **PASSED** | | go -> java | PASSED | PASSED | | java -> java | PASSED | PASSED | | java -> go | PASSED | PASSED | | java -> js | PASSED | PASSED | (The four non-java pairs report `SKIPPED` in both runs — `focus-sdk=java` deselects them, not the feature gate.) `js -> java` is precisely the reported bug: a web-SDK writer omits the per-segment sizes, and the java reader cannot default them back. The control fails with the confusing downstream symptom this PR describes, on the `main` that this branch is based on: ``` java.lang.IllegalArgumentException: tried to calculate GMAC on too small a payload. payload is 0bytes while GMAC is 16 bytes at io.opentdf.platform.sdk.TDF.calculateSignature(TDF.java:481) at io.opentdf.platform.sdk.TDF$Reader.readPayload(TDF.java:447) ``` Job totals: control js job `1 failed, 23 passed, 50 skipped`; fix java job `82 passed, 22 skipped`, no failures and no chunky skips. Both were confirmed by grepping the run logs for the cell's own `PASSED`/`FAILED`/`SKIPPED` line rather than trusting the job's colour — a green job with a skipped cell is the vacuous pass the test exists to prevent. ## Follow-up in opentdf/tests `force-supports` is a pre-release override for these runs only. `xtest/sdk/java/cli.sh` still answers `chunky unsupported: see DSPX-4589` and hard-codes exit 1; when this fix releases, that case has to become a version gate or the cell goes back to skipping. Tracked on DSPX-4592, which owns the tests repo. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved compatibility when reading manifests that omit segment-size fields by applying documented defaults. * Added validation to reject invalid, undersized, or excessively large segments before payload processing. * Prevented plaintext output when encrypted payload segments fail size validation. * Improved handling of missing, zero, and null segment-size values. * TDF files with unsupported integrity algorithms are now rejected instead of being processed with an incorrect fallback. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Member
Author
|
Closing as a duplicate of #398. This PR existed to show the combined diff of the 2-PR stack (#397 + #398). #397 has merged, so what is left of the stack is exactly #398 — and with both PRs now based on Review continues in #398. Not deleting |
dmihalcik-virtru
added a commit
that referenced
this pull request
Oct 20, 2026
…, and UTF-8 entry names (#398) Jira: https://virtru.atlassian.net/browse/DSPX-4589 This PR carries everything left of DSPX-4589 now that #397 has merged. #396 is closed as a duplicate. It started as three zip container conformance findings from an audit against PKWARE APPNOTE.TXT. Review turned up more reader hardening, which is also here. None of it has known field impact; the one finding that did is #397. ## Release note > **Behavior change:** `ZipReader` now looks for the end of central directory record only in the last 22 + 65,535 bytes of the archive, which is the most a zip comment can push it back. An archive with more than 64 KiB of data appended *after* the record (not a comment) used to read and is now rejected with `InvalidZipException`. Zip64 archives with trailing data never read correctly before and now do. Truncated archives and entries now fail with `InvalidZipException` rather than being accepted or returning short data. An archive whose central directory does not end exactly where the record after it starts (the zip64 end of central directory record if there is one, otherwise the end of central directory record) is rejected; every writer we know of, this SDK's included, writes them back to back. ## Public API note `InvalidZipException` is unchecked (it extends `RuntimeException`). Two places that used to report truncation through `IOException` now throw it instead: - `new ZipReader(channel)` and `readCentralDirectoryFileHeader()` throw `InvalidZipException` for a filename cut short, where they used to throw `EOFException`. - An entry's `InputStream` throws `InvalidZipException` when the archive ends before the entry does, where it used to return `-1` early. Callers that catch only `IOException` around these will now see an unchecked exception. Inside the SDK, `SDK.isTDF` already catches both. #418 wraps the manifest and payload cases in `TDFReader` as `SDK.MalformedTDFException`. ## Writer ### End of central directory sentinel thresholds (fixed) `ZipWriter.finish()` decided whether the archive needed a zip64 end of central directory record with masks that matched none of the field widths: ```java (numEntries & ~0xFF) != 0 || (startOfCentralDirectory & ~0xFFFF) != 0 || (sizeOfCentralDirectory & ~0xFFFF) != 0 ``` Any archive with 256 or more entries was needlessly promoted to zip64, and the offset and size checks fired three orders of magnitude too early. Now: ```java var isZip64 = hasZip64Entry || numEntries > MAX_NON_ZIP64_ENTRY_COUNT // Short.MAX_VALUE || needsZip64(startOfCentralDirectory, sizeOfCentralDirectory); ``` All three thresholds stop short of what the format allows (`0xFFFE` entries, `0xFFFFFFFE` bytes), so that a reader widening these unsigned fields with a signed read never sees a negative value. Released versions of this SDK do exactly that: an entry count above 32,767 would come back negative and read as an empty archive. So an archive goes zip64 at 32,768 entries, and at the same 2 GiB (`Integer.MAX_VALUE`) offset/size ceiling the per-entry fields already use. Routing offset and size through `needsZip64` also keeps the `ZipWriter(out, maxNonZip64Value)` test seam from #393 usable for them. Whenever the writer goes zip64 it sets the offset sentinel too, which is the only one older readers check. ### UTF-8 entry names The ticket says the central directory filename length was computed from `String.length()`. The assignment existed (`cdFileHeader.filenameLength = (short) fileInfo.filename.length();`), but `CDFileHeader.write()` never read it: it wrote the length of the encoded byte array. The local header's copy was set from the encoded bytes too. **The bytes on the wire were already correct**; no archive was ever mis-written. What changed: - The dead `filenameLength` field in `CDFileHeader` is gone, and `LocalFileHeader` now takes its length from the encoded bytes it writes, like `CDFileHeader` does. - A new `encodeFilename` helper rejects a name over `Short.MAX_VALUE` UTF-8 bytes with an `SDKException` rather than silently truncating it into the 2-byte field. The cap is `Short.MAX_VALUE` rather than `0xFFFF` for the same signed-read reason as above: older SDK readers read the name length as signed. Validation runs before anything is written, so a rejected name leaves the writer usable. - `data()` entries now set the UTF-8 flag (general purpose bit 11) in the local header as well as the central directory. It was previously set only in the central directory, so readers that go by the local header could garble non-ASCII names. `stream()` entries already set it in both. ## Reader - **End of central directory scan.** A short read used to be treated as a signature match. The scan now: - only matches a real signature, and throws `InvalidZipException` if it finds none; - is bounded to the last 22 + 0xFFFF bytes (see the release note). Previously it was unbounded, doing a positioned 4-byte read per byte back to the start of the file; - skips any candidate whose declared comment would run past the end of the archive. That rejects a record whose comment was truncated. Trailing data after an honest comment is still allowed; - skips any candidate whose central directory does not end exactly where the next record starts: the zip64 end of central directory record if there is one (including one written without sentinels, as commons-compress does), otherwise the candidate itself. This is what passes over a stray `PK\5\6` inside a comment or in trailing data whose declared comment happens to fit, such as a zero-filled one, which used to read as an empty archive. If no candidate passes, the failure of the one nearest the end is reported. - **Zip64 locator.** The locator is found relative to the end of central directory record it precedes (APPNOTE 4.3.15), not relative to the end of the file. This is what makes zip64 archives with trailing data readable. An archive too small to hold the locator it claims is rejected. - **Bounds checks.** Offsets taken from the archive (the zip64 end of central directory pointer and the central directory offset) are checked against the archive size, and so is the entry count: a negative or impossibly large zip64 count is rejected instead of reading as an empty archive. So is a zip64 entry size or central directory size that reads as a negative `long`; a negative entry size used to read as an empty entry. - **Short and empty reads.** Every fixed-width read loops until it is full. `ReadableByteChannel.read` may return fewer bytes than asked, or none at all, without being at end of stream; only `-1` is end of file. Empty reads are retried at most 16 times in a row, without waiting, and then fail with an `IOException` that names the channel class. So a channel with nothing to give can't make the reader spin, but `ZipReader` needs a blocking channel; non-blocking channels are not supported. `loadTDF` takes caller-supplied channels, so this is reachable from the public API. - **Truncation is always an error.** Entry data that runs past the end of the archive now throws `InvalidZipException` instead of ending the stream early with a short entry. A filename cut short throws `InvalidZipException` instead of `EOFException`. `InputStream.read(byte[], int, int)` no longer returns 0 for a non-empty request. - Error messages name the record, offset and bytes found. The locator signature error used to name the wrong record. A bad local file header signature now names the entry and offset. ## Tests 30 new tests: **`ZipWriterTest`** (6) - One test per end of central directory sentinel: - entry count: `Short.MAX_VALUE` stays non-zip64 and `Short.MAX_VALUE + 1` goes zip64, both round-tripped through `ZipReader`; - central directory offset; - central directory size. Each is isolated so that only the end of central directory record is zip64 and no entry is. - `filenameLengthIsMeasuredInUtf8Bytes` writes `"🔒両.txt"` (7 UTF-16 code units, 11 UTF-8 bytes). It checks the length at both header offsets and the UTF-8 flag in both headers. - `rejectsAnEntryNameTooLongToDescribe` covers the boundary: a 32,767-byte name is accepted and reads back, and 32,768 bytes is rejected on both the `data()` and `stream()` paths. It also checks that the writer still works after a rejection and that the result has exactly one entry. - `streamedEntryNameIsMeasuredInUtf8Bytes` round-trips a non-ASCII name through `stream()`, the path a TDF payload takes, with both `ZipReader` and commons-compress. The 32,768-entry archive is also opened with commons-compress. **`ZipReaderTest`** (24) Truncated or malformed trailing records: - truncated archives (four shapes) and an archive too short for its zip64 locator; - an end of central directory record pushed past the comment limit is rejected; - a zip64 archive with the longest possible comment (0xFFFF bytes) still reads; - a decoy signature inside the comment is skipped, both one whose declared comment runs past the end and one whose comment fits: zero-filled, a copy of the real record, and a copy claiming one entry, each in a roomy comment, a comment it fills exactly, and trailing data, on plain and zip64 archives; - a truncated comment is rejected. Bounds checks: - a zip64 locator pointer outside the archive; - a zip64 central directory offset outside the archive; - a plain (non-zip64) central directory offset outside the archive; - a negative or impossibly large zip64 entry count; - a zip64 entry size, and a zip64 central directory size, that read as negative; - an end of central directory record that claims zip64 with no locator in front of it; - a zip64 locator that points inside the archive, but at the wrong record. Channel behavior: - a channel returning short reads (1 to 8 bytes at a time); - a channel returning empty reads; - a channel that stops returning data, both before the archive is opened and partway through an entry (through both `read()` and `read(byte[])`); - a channel whose `size()` claims more bytes than it holds. Everything else: - a zip64 archive with trailing data reads; - an empty archive reads; - a central directory filename running past the end is rejected; - entry data running past the end is rejected, through both `read()` and `read(byte[])`; - a zero-length read returns 0, including at the end of an entry. Negative tests match error messages on stable values (entry name, offset, size) rather than wording. The writer sentinel tests were confirmed to be real regression tests by reverting the mask fix and watching them fail. Run against the previous `ZipReader`, the new tests for empty reads, a stalled channel, the decoy and truncated comments, the entry count bounds, filename truncation and entry truncation all fail. Without the fixes from earlier in this PR, the tests for short reads, trailing data and locator bounds fail too. The decoy tests with a comment that fits and the negative entry size test fail against the reader as it was before the latest commit. The plain-truncation, longest-comment, empty-archive and plain-offset tests pass on the old code; they are there to lock that behavior in. ``` mvn -o -pl sdk test -Denforcer.skip=true -Dmaven.antrun.skip=true [JDK 21, generated sources from an earlier build] -> 338 tests, 0 failures, 0 errors, 8 skipped ``` ## End-to-end validation None of this has an xtest cell of its own; it is unit-tested only. What e2e gives this PR is a no-regression signal: xtest on this PR is green across java/go/js against `main` and v0.26.0 (see the X-Test Results comments). [opentdf/tests run 34357326964](https://github.com/opentdf/tests/actions/runs/34357326964) also passed earlier with `force-supports=chunky`, including all five java `test_chunky_roundtrip` pairs. That run predates the latest round of reader changes. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * ZIP archives are read reliably when data arrives in partial chunks or channels temporarily return no data; persistently stalled reads now fail instead of hanging. * Missing, truncated, or out-of-bounds archive metadata and entry data are reported as errors. * ZIP64 archives are handled correctly, including those with large entry counts or central directories. * **Improvements** * Non-ASCII filenames use accurate UTF-8 byte lengths and flags in archive headers. * Entry names that exceed ZIP format limits are rejected with a clear error. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
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
1 participant
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.
Jira: https://virtru.atlassian.net/browse/DSPX-4589
Top of a 2-PR stack. Do not review or merge this one — review #397 and #398.
This PR keeps its base at
main, so its diff is the combined work: it exists to show what the stack adds up to and to hold the shared context. Its head branch is the same commit as #398's.mainmainMerge order: #397, then #398. This PR closes itself out once both have landed.
Four zip64 / manifest conformance findings from the container audit. Findings 1-3 were written against pre-#393
main; each was re-verified against currentmain(98c839e2) before being changed, and the honest status of each is below. Only finding 4 has field impact, which is why it was pulled to the bottom of the stack as #397.Finding 1 — end of central directory sentinel thresholds (fixed) — #398
ZipWriter.finish()decided whether the archive needed a zip64 end of central directory record with:None of those masks match the real field widths. The entry count field is 2 bytes, not 1, and the central directory offset and size fields are 4 bytes each, not 2. The practical effect was that any archive with 256 or more entries was needlessly promoted to zip64, and the offset/size checks fired three orders of magnitude too early.
Now:
The offset and size go through
needsZip64, so they honor the same deliberate 2 GiB (Integer.MAX_VALUE) ceiling as the per-entry fields — released readers widen these unsigned wire fields with signed reads, so we never write a value that would come back negative. UsingneedsZip64also keeps theZipWriter(out, maxNonZip64Value)test seam that #393 added usable for these two fields.Not touched by #393 — verified with
jj diff -r 98c839e2.Finding 2 — truncated archive detection (fixed, but the branch was latent) — #398
ZipReader.readEndOfCentralDirectory()scanned backwards for the EOCD signature and treated anullfromreadInteger()(a short read) as if it were a signature match, so a truncated archive could fall out of the loop and parse whatever bytes followed as an EOCD record. It now only breaks on a genuine match and throwsInvalidZipExceptionif the scan runs off the front, and separately rejects an archive too small to hold the zip64 locator its EOCD claims to have.Stated plainly: with the
SeekableInMemoryByteChannelthe SDK actually uses, the scan position is always<= size - 22, soreadInteger()always had four bytes available and thenullbranch was unreachable. This is a correctness/robustness fix (aFileChannelcould in principle short-read), not a bug anyone was hitting. The new tests still pass on unpatched code for the plain-truncation cases; they are there to lock the behavior in.Finding 3 — UTF-8 filename length (already correct on the wire; dead code removed) — #398
The ticket says the central directory filename length was computed from
String.length(). That assignment did exist:but
CDFileHeader.write()never read the field — it wrote(short) filename.lengthfrom the already-encoded byte array. The bytes on the wire were already correct. No archive was ever mis-written.What changed: the misleading dead
filenameLengthfield is removed from bothLocalFileHeaderandCDFileHeader, the name is encoded to UTF-8 once instead of twice, and a newencodeFilenamehelper rejects a name whose encoded length exceeds0xFFFFwith anSDKExceptionrather than silently truncating it into the 2-byte field.Finding 4 — per-segment sizes not defaulted from the manifest defaults (fixed; this is the one with field impact) — #397
integrityInformation.segments[].segmentSizeand.encryptedSegmentSizeare optional in the TDF spec; when absent, the reader is supposed to fall back tosegmentSizeDefault/encryptedSegmentSizeDefault. Gson left the absent primitives at0, and java-sdk read the payload with a zero-length segment. Every web SDK TDF larger than one default segment (1 MiB) failed to decrypt in java-sdk, surfacing as a confusing integrity error rather than as a manifest problem.A primitive
longcannot distinguish an absent JSON key from a literal0, so the fix consults the parse tree: a GsonTypeAdapterFactoryregistered forIntegrityInformationwalks the parsedsegmentsarray alongside the deserialized list and fills in the defaults only where the key is absent or JSONnull. BoxingSegment.segmentSizetoLongwould have been the other option, but it breaks the public API (==inSegment.equals, anint->Longassignment inTDF, existingassertEquals(Long, int)in tests) for no added behavior, so the post-deserialization fixup was chosen instead. Explicit0in the JSON is preserved as0.TDF.Reader.readPayloadadditionally rejects a segment with a non-positiveencryptedSegmentSizeup front — an encrypted segment always carries at least an IV and a tag — so a manifest that supplies neither a per-segment size nor a usable default now says so instead of failing downstream with an unrelated complaint about the payload being too small to GMAC.Tests
11 new tests, all in
sdk/src/test/java/io/opentdf/platform/sdk/:ZipWriterTest(fix(sdk): DSPX-4589 zip64 EOCD sentinels, truncated archive detection, and UTF-8 entry names #398) — one test per EOCD sentinel driver: entry count (0xFFFE non-zip64 vs 0xFFFF zip64, round-tripped throughZipReader), central directory offset, and central directory size, each isolated so only the EOCD is zip64 and no entry is; plusfilenameLengthIsMeasuredInUtf8Bytes("🔒両.txt",String.length()7 vs 11 UTF-8 bytes, asserted at both header offsets) andrejectsAnEntryNameTooLongToDescribe.ZipReaderTest(fix(sdk): DSPX-4589 zip64 EOCD sentinels, truncated archive detection, and UTF-8 entry names #398) —testTruncatedArchiveIsRejected(four truncation shapes) andtestArchiveTooShortForTheZip64LocatorIsRejected, both assertingInvalidZipException.ManifestTest(fix(sdk): DSPX-4589 default absent per-segment sizes to the manifest defaults #397) —testAbsentSegmentSizesFallBackToTheManifestDefaults(absent / partially overridden / fully overridden, plus atoJsonround trip) andtestExplicitZeroSegmentSizeIsNotTreatedAsAbsent.TDFTest(fix(sdk): DSPX-4589 default absent per-segment sizes to the manifest defaults #397) —testReadingATDFThatOmitsDefaultedSegmentSizesencrypts ~2 MiB + 4242 bytes at a 1 MiB segment size, strips every per-segment size equal to the default from the manifest, and asserts a byte-exact decrypt;testZeroLengthSegmentIsRejectedWithAClearError.The sentinel and default-fallback tests were confirmed to be genuine regression tests by reverting the EOCD mask fix and the
registerTypeAdapterFactoryline and watching them fail.CI-equivalent commands, on JDK 21:
End-to-end validation of finding 4
Run on the
opentdf/testsDSPX-4592-02-chunkybranch, which addstest_tdfs.py::test_chunky_roundtrip— a 5 MiB round trip, versus the 128 bytes the suite has used for four years, which is what it takes for a writer to emit a segment whose size equals the manifest default. All runs passforce-supports=chunky, which makestdfs.skip_chunky_skewreturn early so the cell reports a real pass or fail instead of skipping on the unreleased version gate.java-refjs -> javachunky cellDSPX-4589-01-segment-size-defaultsmain(the stack's base)DSPX-4589-02-zip-formatExactly one cell flips between the fix run and the control. Every chunky pair, side by side:
java@main)java@DSPX-4589-01-...)(The four non-java pairs report
SKIPPEDin both runs —focus-sdk=javadeselects them, not the feature gate.)js -> javais precisely finding 4: a web-SDK writer omits the per-segment sizes, and the java reader cannot default them back. The control fails with the confusing downstream symptom described above:Job totals: control js job
1 failed, 23 passed, 50 skipped; fix java job82 passed, 22 skipped, no failures and no chunky skips.All confirmed by grepping the run logs for the cell's own
PASSED/FAILED/SKIPPEDline rather than trusting the job's colour — a green job with a skipped cell is the vacuous pass the test exists to prevent.Findings 1-3 (#398) have no xtest cell of their own; they are covered by unit tests plus the full-stack run above showing no regression.
force-supportsis a pre-release override for the runs above only.xtest/sdk/java/cli.shstill answerschunky unsupported: see DSPX-4589and hard-codes exit 1, so by defaulttest_chunky_roundtripkeeps skipping for java. That shim lives in theopentdf/testsrepo, not in this one, so nothing in this stack changes it.When this fix releases, someone must version-gate the
chunky)case inxtest/sdk/java/cli.shso java reports support at or above the releasing version. Deliberately not done here: this stack touches onlyopentdf/java-sdk, and DSPX-4592 owns the tests repo.