Skip to content

refactor: allow AES in zip file stream#820

Open
Its-Just-Nans wants to merge 31 commits intomasterfrom
rewrite-aes-settings-4
Open

refactor: allow AES in zip file stream#820
Its-Just-Nans wants to merge 31 commits intomasterfrom
rewrite-aes-settings-4

Conversation

@Its-Just-Nans
Copy link
Copy Markdown
Member

Following MR

  • allow AES in zip file stream

Diff with previous MR https://github.com/zip-rs/zip2/compare/rewrite-aes-settings-3...rewrite-aes-settings-4?expand=1

Copy link
Copy Markdown
Contributor

@amazon-q-developer amazon-q-developer Bot left a comment

Choose a reason for hiding this comment

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

The refactoring improves the codebase by moving AES vendor version information handling to where it's needed, eliminating redundant storage in CryptoReader. The changes correctly extract vendor_version from ZipFileData.aes_mode and pass it to make_reader, maintaining all functionality while simplifying the architecture. No defects found that would block the merge.


You can now have the agent implement changes and create commits directly on your pull request's source branch. Simply comment with /q followed by your request in natural language to ask the agent to make changes.

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request refactors the handling of AES encryption metadata and enables support for encrypted files in streaming mode. Key changes include moving the is_ae2_encrypted logic to AesVendorVersion, updating the signatures of make_crypto_reader and make_reader for better consistency, and introducing read_zipfile_from_stream_with_compressed_size_and_options to handle passwords during stream reading. A review comment suggests passing the actual AES vendor version in read_zipfile_from_stream to ensure correct CRC check behavior if AES support is expanded in that specific function.

Comment thread src/read/stream.rs Outdated
compression_method,
uncompressed_size,
Some(crc32),
None,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

In read_zipfile_from_stream, the vendor_version is passed as None to make_reader. Although this function currently only supports plaintext (as it calls make_crypto_reader with a None password, which fails for AES), it is more robust and consistent to pass the actual vendor version if it was successfully parsed from the extra fields. This ensures that if password support is added to this function in the future, or if the make_crypto_reader logic changes, the CRC check behavior (which depends on whether the file is AE-2 encrypted) remains correct.

Suggested change
None,
result.aes_mode.map(|aes| aes.1),

Comment thread src/read/stream.rs Fixed
Signed-off-by: n4n5 <its.just.n4n5@gmail.com>
Comment thread src/read/stream.rs Fixed
Comment thread src/read/stream.rs Fixed
@Its-Just-Nans Its-Just-Nans requested a review from Pr0methean May 5, 2026 22:19
@Its-Just-Nans Its-Just-Nans self-assigned this May 5, 2026
Comment thread src/read/stream.rs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants