Skip to content

fix: Document feature unreserved and make the mapping of extra fields public#616

Merged
Pr0methean merged 9 commits intomasterfrom
document-feature
Feb 4, 2026
Merged

fix: Document feature unreserved and make the mapping of extra fields public#616
Pr0methean merged 9 commits intomasterfrom
document-feature

Conversation

@Its-Just-Nans
Copy link
Member

Document feature unreserved and make public the mapping

Also remove the _all-features in cargo_hack.yml since it was deleted in #612

Linked to

@Its-Just-Nans Its-Just-Nans self-assigned this Feb 3, 2026
@gemini-code-assist
Copy link
Contributor

Summary of Changes

Hello @Its-Just-Nans, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request enhances the zip2 crate by clarifying the purpose of the unreserved feature and centralizing the definition of known ZIP extra field IDs. By making the EXTRA_FIELD_MAPPING publicly accessible and expanding its contents, these changes improve the discoverability and utility of extra field definitions for custom data headers.

Highlights

  • Documented 'unreserved' feature: A comment has been added to Cargo.toml to clarify that the unreserved feature allows the usage of custom Extra data header IDs.
  • Public Extra Field Mapping: The EXTRA_FIELD_MAPPING constant, which lists known ZIP extra field IDs, has been moved to src/extra_fields/mod.rs and made public for broader accessibility.
  • Expanded Extra Field Definitions: The EXTRA_FIELD_MAPPING list has been significantly expanded to include a more comprehensive set of PKWARE and third-party extra field IDs.
Changelog
  • Cargo.toml
    • Added a comment explaining the unreserved feature, clarifying its use for custom Extra data header IDs.
  • src/extra_fields/mod.rs
    • Updated the module-level documentation from 'types for extra fields' to 'Types for extra fields'.
    • Added a 're-export' comment above the pub use statements.
    • Introduced pub const EXTRA_FIELD_MAPPING: [u16; 57], providing a comprehensive list of known PKWARE and Third-party Extra field IDs.
  • src/write.rs
    • Modified the import statement to use the newly public EXTRA_FIELD_MAPPING from crate::extra_fields.
    • Removed the previously private EXTRA_FIELD_MAPPING constant definition, as it is now centralized.
Ignored Files
  • Ignored by pattern: .github/workflows/** (1)
    • .github/workflows/cargo_hack.yml
Activity
  • The pull request is linked to issue Checking the crate #605, indicating it addresses or is related to a previously identified concern.
Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

@Its-Just-Nans
Copy link
Member Author

Note: some of the third party were missing !

Copy link
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 PR successfully documents the unreserved feature and makes the extra field mapping public as intended. The changes are well-structured:

Good changes:

  • Added clear documentation for the unreserved feature in Cargo.toml
  • Made EXTRA_FIELD_MAPPING public and moved it to the appropriate module
  • Updated imports to use the new public constant
  • Cleaned up the CI workflow by removing the deleted _all-features flag
  • Improved module documentation capitalization

⚠️ Please verify:

  • The expansion from 43 to 57 extra field entries - ensure all new mappings are accurate and properly sourced

The implementation correctly follows the user guidelines by making the feature flag functionality more discoverable and properly documented.


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
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 aims to document the unreserved feature and centralize the EXTRA_FIELD_MAPPING constant. However, the expansion of EXTRA_FIELD_MAPPING to include IDs like 0x9901 (AES encryption) and 0xa11e (data alignment) introduces a critical security regression. The library's own validation logic in src/write.rs now prevents the creation of ZIP files with these core features by default, forcing users to enable the unreserved feature and bypass intended security controls. Furthermore, the add_extra_data method in src/write.rs modifies internal state before validation, which can lead to an inconsistent state if validation fails, posing a security risk. For improved performance and maintainability, the EXTRA_FIELD_MAPPING constant should also be sorted to allow for more efficient binary_search usage.

@Its-Just-Nans
Copy link
Member Author

Thanks for the review, I answered or fixed some of them

@Pr0methean Pr0methean dismissed their stale review February 3, 2026 23:33

Applied requested change

Pr0methean
Pr0methean previously approved these changes Feb 3, 2026
@Pr0methean Pr0methean enabled auto-merge February 3, 2026 23:33
@Pr0methean Pr0methean changed the title Document feature unreserved and make public the mapping fix: Document feature unreserved and make the mapping of extra fields public Feb 3, 2026
@Pr0methean Pr0methean added this to the 7.3.0 milestone Feb 3, 2026
@Its-Just-Nans
Copy link
Member Author

I think your change will not work

The UsedExtraField need to stay private, it's not in a spec or something official

My test was only used to see if we could access a private enum, the answer is yes (I think because it's a constant compile)

@Pr0methean
Copy link
Member

Before commit 7f578a0, your change contained assert_eq!(EXTRA_FIELD_MAPPING[0], 0x0001); byte-for-byte twice. What did you intend to compare it to instead the second time?

@Its-Just-Nans
Copy link
Member Author

Its-Just-Nans commented Feb 3, 2026

Before commit 7f578a0, your change contained assert_eq!(EXTRA_FIELD_MAPPING[0], 0x0001); byte-for-byte twice. What did you intend to compare it to instead the second time?

Oh okay

I wanted to compare with 1 as u16 and then with 0x0001

I can change later or you can do the change
(Also need to remove the import)

@Pr0methean
Copy link
Member

I've now done it.

@Pr0methean Pr0methean added this pull request to the merge queue Feb 4, 2026
Merged via the queue into master with commit 57b5ecc Feb 4, 2026
129 checks passed
@Pr0methean Pr0methean deleted the document-feature branch February 4, 2026 05:01
@Pr0methean Pr0methean mentioned this pull request Feb 4, 2026
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.

2 participants