feat: add secure streaming archive extraction#2
Conversation
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe PR adds streaming and buffered zip reads, extraction options with path and limit validation, path-aware writing, platform file-removal handling, and expanded tests and documentation. ChangesChunked reads and extraction validation
Estimated code review effort🎯 5 (Critical) | ⏱️ ~90 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Sources/ZipArchive/ZipArchiveReader.swift`:
- Around line 133-142: The buffered inflate path in ZipArchiveReader’s
entry-reading logic only validates CRC32 after calling
compressor.inflate(from:uncompressedSize:) and can accept a size-mismatched
stored entry; add an explicit uncompressed byte count check against
preparedEntry.uncompressedSize before returning uncompressedBytes, mirroring the
streaming path’s validation, and keep the CRC32 check in place for the existing
integrity guard.
- Around line 102-123: The encrypted-entry read path in ZipArchiveReader should
fail before loading the payload when no password is available. In
ZipArchiveReader.readEntry, after detecting
preparedEntry.localHeader.flags.contains(.encrypted) and consuming the 12-byte
encryption header, check for the missing password state immediately and throw
the same password-required error before calling storage.readBytes for the
compressed data. Keep the fix localized around the encryptionKeys handling and
the subsequent fileBytes read so encrypted entries never buffer the full payload
unnecessarily.
In `@Sources/ZipArchive/ZipArchiveReader`+extract.swift:
- Around line 319-324: The validation in validatedExtractionPath(_:) is running
after entry.filename has already been converted into FilePath, which can
normalize separators and incorrectly reject valid nested ZIP entries. Move the
safety checks to the original filename string/bytes before constructing
FilePath, then let validatedExtractionPath(_:) operate on the already-approved
path; use the entry.filename handling in ZipArchiveReader+extract as the place
to apply the raw-name policy.
In `@Sources/ZipArchive/ZipCompression.swift`:
- Around line 215-218: In the inflate loop inside ZipCompression, the
Z_STREAM_END path currently accepts decompression even if zlib leaves unread
input in stream.avail_in. Update the status handling around the switch on status
so that CZIPARCHIVE_Z_STREAM_END is only accepted when no compressed bytes
remain buffered; if stream.avail_in is still greater than zero, treat the entry
as malformed and fail the read. Make the check in the same flow that updates
reachedEnd and remainingCompressedSize so junk bytes after a valid deflate
stream cannot pass validation.
In `@Tests/ZipArchiveTests/ZipArchiveExtractionTests.swift`:
- Around line 204-209: The test assertions in ZipArchiveExtractionTests are
using String(decoding:as:), which SwiftLint flags because it can silently
replace invalid UTF-8. Update the affected assertions in the extraction tests to
use failable UTF-8 decoding via String(bytes:encoding:) instead, and assert on
the optional result so invalid data does not pass unnoticed. Apply this change
in the Current symlink/content checks and the other matching assertions in
ZipArchiveExtractionTests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 24213043-8c9c-4090-acf4-7f4c82e8ea7a
📒 Files selected for processing (6)
README.mdSources/ZipArchive/ZipArchiveReader+extract.swiftSources/ZipArchive/ZipArchiveReader.swiftSources/ZipArchive/ZipCompression.swiftSources/ZipArchive/ZipTypes.swiftTests/ZipArchiveTests/ZipArchiveExtractionTests.swift
|
@coderabbitai review |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Sources/ZipArchive/ZipArchiveReader.swift (1)
187-190: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReturn
invalidFileHeaderfor truncated encrypted entries.Line 188 conflates missing passwords with
compressedSize < 12; with a password supplied, a malformed encrypted entry is reported asencryptedFilesRequirePassword, unlike the buffered path.🐛 Proposed fix
var cryptKey: CryptKey? if preparedEntry.localHeader.flags.contains(.encrypted) { - guard let password, compressedSize >= 12 else { + guard compressedSize >= 12 else { + throw ZipArchiveReaderError.invalidFileHeader + } + guard let password else { throw ZipArchiveReaderError.encryptedFilesRequirePassword } var key = CryptKey(password: password)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Sources/ZipArchive/ZipArchiveReader.swift` around lines 187 - 190, The encrypted-entry check in ZipArchiveReader should distinguish between a missing password and a truncated header: in the branch using preparedEntry.localHeader.flags.contains(.encrypted), keep encryptedFilesRequirePassword only for the no-password case, but when a password is present and compressedSize is less than the required encrypted header size, throw invalidFileHeader instead. Update the logic around preparedEntry and the encryptedFilesRequirePassword guard so it matches the buffered path’s behavior for malformed encrypted entries.Sources/ZipArchive/ZipTypes.swift (1)
208-213: 🔒 Security & Privacy | 🟠 MajorAccept Darwin host values in
isSymbolicLink. The current.unix-only check lets OS X/Darwin symlink entries bypasssymbolicLinkPolicyand get extracted as regular files. Add a Unix-like host helper or a Darwin case here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Sources/ZipArchive/ZipTypes.swift` around lines 208 - 213, The isSymbolicLink check in ZipEntry currently only accepts versionMadeBy.system == .unix, which causes Darwin-created symlink entries to be missed and handled like regular files. Update ZipEntry.isSymbolicLink to also treat Darwin/OS X host values as valid Unix-like hosts, either by adding a dedicated helper or extending the system check, while still using externalAttributes.unixAttributes.contains(.isSymbolicLink) for the actual symlink detection.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Sources/ZipArchive/System`+directory.swift:
- Around line 91-97: The failed-extraction cleanup on Windows is using the wrong
delete path; update the Windows branch in the directory removal helper to
perform a file-only delete for the extracted file rather than calling the shared
FileManager-based removal. Fix the specific cleanup path by hardening the try?
FileDescriptor.remove(fullFilePath) usage in the relevant extraction/removal
flow, and keep DirectoryDescriptor.recursiveDelete(_:) and
FileDescriptor.remove(_:) behavior unchanged so directory cleanup still works
correctly.
In `@Sources/ZipArchive/ZipArchiveWriter.swift`:
- Around line 140-143: The String-based archive write path in
ZipArchiveWriter.writeFile is forwarding filename directly into pathInArchive,
which bypasses the same archive-path validation used by the FilePath overloads.
Update the String overload(s) to either normalize the incoming filename through
the existing FilePath archive-path rules or explicitly reject
unsafe/non-portable separators such as backslashes before calling writeFile, so
the paths created here remain extractable by the reader.
In `@Tests/ZipArchiveTests/ZipArchiveExtractionTests.swift`:
- Around line 970-973: The `read(_:)` helper in the test double is counting
requested bytes instead of actual bytes read, so `bytesRead` can be inflated by
failed or short reads. Update `read(_ count: Int)` to increment `bytesRead` only
after `storage.read(count)` succeeds, using the returned slice’s length rather
than `count`. Keep the fix localized to the `read(_:)` method so the read-bound
assertions remain accurate.
---
Outside diff comments:
In `@Sources/ZipArchive/ZipArchiveReader.swift`:
- Around line 187-190: The encrypted-entry check in ZipArchiveReader should
distinguish between a missing password and a truncated header: in the branch
using preparedEntry.localHeader.flags.contains(.encrypted), keep
encryptedFilesRequirePassword only for the no-password case, but when a password
is present and compressedSize is less than the required encrypted header size,
throw invalidFileHeader instead. Update the logic around preparedEntry and the
encryptedFilesRequirePassword guard so it matches the buffered path’s behavior
for malformed encrypted entries.
In `@Sources/ZipArchive/ZipTypes.swift`:
- Around line 208-213: The isSymbolicLink check in ZipEntry currently only
accepts versionMadeBy.system == .unix, which causes Darwin-created symlink
entries to be missed and handled like regular files. Update
ZipEntry.isSymbolicLink to also treat Darwin/OS X host values as valid Unix-like
hosts, either by adding a dedicated helper or extending the system check, while
still using externalAttributes.unixAttributes.contains(.isSymbolicLink) for the
actual symlink detection.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5d36a111-84bf-49a7-bca7-1b6e622e9d73
📒 Files selected for processing (7)
Sources/ZipArchive/System+directory.swiftSources/ZipArchive/ZipArchiveReader+extract.swiftSources/ZipArchive/ZipArchiveReader.swiftSources/ZipArchive/ZipArchiveWriter.swiftSources/ZipArchive/ZipCompression.swiftSources/ZipArchive/ZipTypes.swiftTests/ZipArchiveTests/ZipArchiveExtractionTests.swift
🚧 Files skipped from review as they are similar to previous changes (2)
- Sources/ZipArchive/ZipCompression.swift
- Sources/ZipArchive/ZipArchiveReader+extract.swift
|
Addressed all five findings in
Validated with 91 tests, strict swift-format, release and WASI builds, and the API breakage check. Windows CI will confirm the @coderabbitai review |
|
✏️ Learnings added
✅ Action performedReview finished.
|
|
Restored compatibility for existing extraction calls: symbolic-link entries now default to materializing their stored target as regular file contents, matching the package's historical behavior without creating filesystem links. Strict rejection remains available through Validated with the full 91-test suite, release and WASI builds, strict Swift formatting, and @coderabbitai review |
|
✏️ Learnings added
✅ Action performedReview finished.
|
1 participant
Summary
ZipStreamingCompressionextract(to:password:)API while adding policy-aware extraction optionsExtraction flow
flowchart LR A[Read central directory incrementally] --> B[Validate limits and normalized paths] B --> C[Reject conflicts and symbolic links] C --> D[Verify local header matches central metadata] D --> E[Inflate bounded chunks] E --> F[Write with exclusive create and no-follow checks] F --> G[Validate size and CRC]Validation
swift test(82 tests)swift build -c releaseswift build --swift-sdk swift-6.3.2-RELEASE_wasmswift package diagnose-api-breaking-changes f43a4dbswift format lintSummary by CodeRabbit