Skip to content

refactor: remove inner compression method from aes options#814

Merged
Pr0methean merged 6 commits intomasterfrom
rewrite-aes-settings
May 4, 2026
Merged

refactor: remove inner compression method from aes options#814
Pr0methean merged 6 commits intomasterfrom
rewrite-aes-settings

Conversation

@Its-Just-Nans
Copy link
Copy Markdown
Member

@Its-Just-Nans Its-Just-Nans commented May 4, 2026

Little cleanup

another MR will follow this one

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.

This refactoring successfully removes redundant storage of the compression method from the AES mode tuple. The changes are consistent across all affected files and maintain the correct behavior where the actual compression method is stored in ZipFileData.compression_method while the AES metadata is stored separately in aes_mode.

The key improvements include:

  • Simplified the aes_mode field from Option<(AesMode, AesVendorVersion, CompressionMethod)> to Option<(AesMode, AesVendorVersion)>
  • Refactored AexEncryption::parse to return values instead of using mutable parameters
  • Updated all call sites to use the new structure
  • Correctly sets CompressionMethod::AES in serialization when AES encryption is present

The changes are well-implemented and don't introduce any defects that would block merging.


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 by decoupling the inner compression method from the AES mode options and updating the AexEncryption::parse method to return these values directly. It also modifies how compression methods are serialized in local and central entry blocks when AES is active. The review feedback highlights two important validation issues: first, the parse method should explicitly prevent the inner compression method from being set to AES (99) to avoid recursive definitions; second, removing the validation check for missing AES extra fields when the compression method is 99 is a regression that should be restored to maintain archive integrity.

I am having trouble creating individual review comments. Click here to see my feedback.

src/extra_fields/aex_encryption.rs (80-81)

medium

The parse method should validate that the inner compression method is not CompressionMethod::AES (99). According to the WinZip specification, the AES extra field must contain the actual compression method used for the data. If the inner method is also specified as 99, it would result in an invalid or recursive definition. Adding this check improves archive validation.

        let inner_comp_method = CompressionMethod::parse_from_u16(reader.read_u16_le()?);
        if inner_comp_method == CompressionMethod::AES {
            return Err(invalid!("AES inner compression method cannot be AES"));
        }
        Ok(((aes_mode, vendor_version), inner_comp_method))

src/read.rs (557-560)

medium

Removing this validation check is a regression in archive integrity checking. If the central directory specifies compression method 99 (AES) but the AES extra data field is missing, the archive is invalid according to the WinZip specification. Without this check, the library will proceed with CompressionMethod::Aes as the actual compression method, which will lead to less helpful error messages later (e.g., 'compression method not supported: 99') and potentially incorrect metadata such as version_needed. It is recommended to retain this check, possibly simplified to if result.compression_method == CompressionMethod::AES { ... } since parse_extra_field now handles the field processing.

Comment thread src/write.rs Fixed
@Its-Just-Nans Its-Just-Nans changed the title DRAFT refactor: remove inner compression method from aes options May 4, 2026
@Its-Just-Nans Its-Just-Nans requested a review from Pr0methean May 4, 2026 16:33
@Its-Just-Nans Its-Just-Nans self-assigned this May 4, 2026
@Pr0methean Pr0methean added this pull request to the merge queue May 4, 2026
@Pr0methean Pr0methean added this to the 9.0.0 milestone May 4, 2026
Merged via the queue into master with commit d00d5b3 May 4, 2026
133 checks passed
@Pr0methean Pr0methean deleted the rewrite-aes-settings branch May 4, 2026 22:35
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