✨ Add Reserved/Private variants to Compression, Encryption, CipherMode, DataKind enums#3061
Merged
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughEnums for Compression, Encryption, CipherMode, and DataKind gain Reserved/Private variants and helpers; headers serialize via to_byte(); read/write paths and writer build now return explicit Unsupported/panic for unsupported variants; CLI list/diff/chmod updated to render and handle unknown DataKind values consistently. ChangesArchive Format Extensibility
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces Reserved and Private variants to the Compression, Encryption, CipherMode, and DataKind enums to support future PNA specifications and application-specific values. It replaces direct u8 casts with a to_byte() method and updates TryFrom<u8> implementations to handle these new variants. Additionally, error handling for unsupported variants has been added to the reading and writing logic. A logic error was identified in decrypt_reader where unknown cipher modes could incorrectly default to CtrCamellia for Aes encryption; it is recommended to explicitly handle valid combinations and return io::ErrorKind::Unsupported for others to ensure semantic clarity.
github-actions
Bot
added
cli
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
lib/src/entry/read.rs (1)
38-58:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winUnsupported cipher modes are still decrypted via Camellia-CTR fallback.
The wildcard arm currently treats non-CTR/CBC modes as
CtrCamellia. With extensibleCipherMode, this can silently use the wrong algorithm/mode instead of returningUnsupported.Proposed fix
match (encryption, cipher_mode) { (Encryption::Aes, CipherMode::CBC) => { let mut iv = vec![0; Aes256::block_size()]; reader.read_exact(&mut iv)?; DecryptReader::CbcAes(DecryptCbcAes256Reader::new(reader, key, &iv)?) } (Encryption::Aes, CipherMode::CTR) => { let mut iv = vec![0u8; Aes256::block_size()]; reader.read_exact(&mut iv)?; DecryptReader::CtrAes(Ctr128BEReader::new(reader, key, &iv)?) } (Encryption::Camellia, CipherMode::CBC) => { let mut iv = vec![0; Camellia256::block_size()]; reader.read_exact(&mut iv)?; DecryptReader::CbcCamellia(DecryptCbcCamellia256Reader::new(reader, key, &iv)?) } - _ => { + (Encryption::Camellia, CipherMode::CTR) => { let mut iv = vec![0u8; Camellia256::block_size()]; reader.read_exact(&mut iv)?; DecryptReader::CtrCamellia(Ctr128BEReader::new(reader, key, &iv)?) } + (_, mode) => { + return Err(io::Error::new( + io::ErrorKind::Unsupported, + format!("unsupported cipher mode for {encryption:?}: {mode:?}"), + )); + } }🤖 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 `@lib/src/entry/read.rs` around lines 38 - 58, The match on (encryption, cipher_mode) incorrectly uses a wildcard arm to create DecryptReader::CtrCamellia for any unknown combination; change it to explicitly match only the supported (Encryption::Aes, CipherMode::CBC|CTR) and (Encryption::Camellia, CipherMode::CBC|CTR) arms and replace the trailing `_` arm with an error return (e.g. Err(UnsupportedCipherMode) or propagate a descriptive error) so unsupported CipherMode/Encryption combinations do not silently fall back to CtrCamellia; update the function that constructs the reader (the code creating DecryptReader variants and using Ctr128BEReader, DecryptCbcAes256Reader, DecryptCbcCamellia256Reader) to return that error for any other tuple.lib/src/entry/options.rs (1)
888-901:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUnsupported encryption can be masked by the password panic.
On Line 890–893, password validation runs before unsupported encryption is checked on Line 898.
Encryption::Reserved(_)/Encryption::Private(_)without a password will panic with “Password was not provided.” instead of the intended unsupported-encryption panic.Proposed fix
- let cipher = if self.encryption != Encryption::No { - Some(Cipher::new( - self.password - .as_deref() - .expect("Password was not provided.") - .into(), - self.hash_algorithm, - match self.encryption { - Encryption::Aes => CipherAlgorithm::Aes, - Encryption::Camellia => CipherAlgorithm::Camellia, - other => panic!( - "unsupported encryption method for writing: byte={}", - other.to_byte() - ), - }, - self.cipher_mode, - )) - } else { - None - }; + let cipher = match self.encryption { + Encryption::No => None, + Encryption::Aes | Encryption::Camellia => { + let password = self + .password + .as_deref() + .expect("Password was not provided.") + .into(); + let algorithm = match self.encryption { + Encryption::Aes => CipherAlgorithm::Aes, + Encryption::Camellia => CipherAlgorithm::Camellia, + _ => unreachable!(), + }; + Some(Cipher::new(password, self.hash_algorithm, algorithm, self.cipher_mode)) + } + other => panic!( + "unsupported encryption method for writing: byte={}", + other.to_byte() + ), + };🤖 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 `@lib/src/entry/options.rs` around lines 888 - 901, The current code calls self.password.as_deref().expect(...) before validating self.encryption, so an unsupported variant (e.g., Encryption::Reserved/_Private) will trigger the password panic instead of the intended unsupported-encryption panic; fix by validating the encryption variant first (the match on self.encryption that maps to CipherAlgorithm in the Cipher::new call) and only call expect("Password was not provided.") when the chosen encryption actually requires a password (i.e., after confirming Encryption::Aes or Encryption::Camellia), adjusting the order in the block that constructs cipher (referencing self.encryption, self.password, Cipher::new, and CipherAlgorithm).
🤖 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 `@cli/src/command/list.rs`:
- Line 454: The match arm that maps unknown DataKind variants to EntryType::File
causes misleading output; instead change the wildcard arm in the DataKind ->
EntryType match (the arm producing EntryType::File(entry.name().to_string())) to
return EntryType::Unsupported (or
EntryType::Unsupported(entry.name().to_string()) if that variant carries a name)
so unknown/future DataKind values are explicitly marked unsupported; update any
downstream code expecting a File to handle Unsupported accordingly and keep
references to the symbols EntryType and DataKind when locating the change.
---
Outside diff comments:
In `@lib/src/entry/options.rs`:
- Around line 888-901: The current code calls
self.password.as_deref().expect(...) before validating self.encryption, so an
unsupported variant (e.g., Encryption::Reserved/_Private) will trigger the
password panic instead of the intended unsupported-encryption panic; fix by
validating the encryption variant first (the match on self.encryption that maps
to CipherAlgorithm in the Cipher::new call) and only call expect("Password was
not provided.") when the chosen encryption actually requires a password (i.e.,
after confirming Encryption::Aes or Encryption::Camellia), adjusting the order
in the block that constructs cipher (referencing self.encryption, self.password,
Cipher::new, and CipherAlgorithm).
In `@lib/src/entry/read.rs`:
- Around line 38-58: The match on (encryption, cipher_mode) incorrectly uses a
wildcard arm to create DecryptReader::CtrCamellia for any unknown combination;
change it to explicitly match only the supported (Encryption::Aes,
CipherMode::CBC|CTR) and (Encryption::Camellia, CipherMode::CBC|CTR) arms and
replace the trailing `_` arm with an error return (e.g.
Err(UnsupportedCipherMode) or propagate a descriptive error) so unsupported
CipherMode/Encryption combinations do not silently fall back to CtrCamellia;
update the function that constructs the reader (the code creating DecryptReader
variants and using Ctr128BEReader, DecryptCbcAes256Reader,
DecryptCbcCamellia256Reader) to return that error for any other tuple.
🪄 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: a1c810e3-8f4e-41f4-afb8-848be2466213
📒 Files selected for processing (6)
cli/src/command/diff.rscli/src/command/list.rslib/src/entry/header.rslib/src/entry/options.rslib/src/entry/read.rslib/src/entry/write.rs
ChanTsune
force-pushed
the
feat/enum-reserved-private
branch
from
d7b25c8 to
799c2ea
Compare
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
lib/src/entry/options.rs (1)
861-901:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winValidate unsupported encryption before password extraction.
On Line 890–893, password is required before Line 895–901 validates unsupported encryption.
ForEncryption::Reserved(_)/Encryption::Private(_)with no password, this panics with the wrong reason ("Password was not provided.") and skips the intended unsupported-variant panic.Also, the
# Panicsdocs in this block should mention unsupportedEncryption/Compressionvariants.Proposed fix
pub fn build(&self) -> WriteOptions { - let cipher = if self.encryption != Encryption::No { - Some(Cipher::new( - self.password - .as_deref() - .expect("Password was not provided.") - .into(), - self.hash_algorithm, - match self.encryption { - Encryption::Aes => CipherAlgorithm::Aes, - Encryption::Camellia => CipherAlgorithm::Camellia, - other => panic!( - "unsupported encryption method for writing: byte={}", - other.to_byte() - ), - }, - self.cipher_mode, - )) - } else { - None - }; + let cipher_algorithm = match self.encryption { + Encryption::No => None, + Encryption::Aes => Some(CipherAlgorithm::Aes), + Encryption::Camellia => Some(CipherAlgorithm::Camellia), + other => panic!( + "unsupported encryption method for writing: byte={}", + other.to_byte() + ), + }; + let cipher = cipher_algorithm.map(|cipher_algorithm| { + Cipher::new( + self.password + .as_deref() + .expect("Password was not provided.") + .into(), + self.hash_algorithm, + cipher_algorithm, + self.cipher_mode, + ) + });🤖 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 `@lib/src/entry/options.rs` around lines 861 - 901, In WriteOptions::build(), avoid extracting the password before validating the encryption variant: first match on self.encryption and panic for unsupported variants (Encryption::Reserved(_), Encryption::Private(_), etc.) with an explicit "unsupported encryption" message, then only call self.password.as_deref().expect(...) when the variant is a supported encrypted type (Encryption::Aes or Encryption::Camellia) prior to calling Cipher::new; also update the function's "# Panics" doc to state it may panic for both missing passwords and for unsupported Encryption/Compression variants to make the failure modes explicit.lib/src/entry/read.rs (1)
38-58:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject unsupported cipher modes instead of falling back to Camellia-CTR.
Line 54 currently catches unsupported/new cipher modes and decrypts as Camellia-CTR.
WithCipherMode::Reserved(_)/CipherMode::Private(_), this can mis-decrypt data instead of returningUnsupported.Proposed fix
match (encryption, cipher_mode) { (Encryption::Aes, CipherMode::CBC) => { let mut iv = vec![0; Aes256::block_size()]; reader.read_exact(&mut iv)?; DecryptReader::CbcAes(DecryptCbcAes256Reader::new(reader, key, &iv)?) } (Encryption::Aes, CipherMode::CTR) => { let mut iv = vec![0u8; Aes256::block_size()]; reader.read_exact(&mut iv)?; DecryptReader::CtrAes(Ctr128BEReader::new(reader, key, &iv)?) } (Encryption::Camellia, CipherMode::CBC) => { let mut iv = vec![0; Camellia256::block_size()]; reader.read_exact(&mut iv)?; DecryptReader::CbcCamellia(DecryptCbcCamellia256Reader::new(reader, key, &iv)?) } - _ => { + (Encryption::Camellia, CipherMode::CTR) => { let mut iv = vec![0u8; Camellia256::block_size()]; reader.read_exact(&mut iv)?; DecryptReader::CtrCamellia(Ctr128BEReader::new(reader, key, &iv)?) } + (_, mode) => { + return Err(io::Error::new( + io::ErrorKind::Unsupported, + format!("unsupported cipher mode: {mode:?}"), + )); + } }Also applies to: 61-66
🤖 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 `@lib/src/entry/read.rs` around lines 38 - 58, The current match on (encryption, cipher_mode) uses a wildcard arm that treats any unsupported/new CipherMode as Camellia-CTR, which can cause silent mis-decryption; update the match in the function building DecryptReader so it only accepts the explicit supported tuples (Encryption::Aes, CipherMode::CBC), (Encryption::Aes, CipherMode::CTR), (Encryption::Camellia, CipherMode::CBC), and (Encryption::Camellia, CipherMode::CTR) and construct the corresponding DecryptReader variants (DecryptReader::CbcAes, DecryptReader::CtrAes, DecryptReader::CbcCamellia, DecryptReader::CtrCamellia) after reading the IV, and for any other (e.g., CipherMode::Reserved/_Private) return an Err variant (e.g., Unsupported) rather than falling back to CtrCamellia; ensure the same explicit handling is applied to the later analogous block as well.
🤖 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.
Outside diff comments:
In `@lib/src/entry/options.rs`:
- Around line 861-901: In WriteOptions::build(), avoid extracting the password
before validating the encryption variant: first match on self.encryption and
panic for unsupported variants (Encryption::Reserved(_), Encryption::Private(_),
etc.) with an explicit "unsupported encryption" message, then only call
self.password.as_deref().expect(...) when the variant is a supported encrypted
type (Encryption::Aes or Encryption::Camellia) prior to calling Cipher::new;
also update the function's "# Panics" doc to state it may panic for both missing
passwords and for unsupported Encryption/Compression variants to make the
failure modes explicit.
In `@lib/src/entry/read.rs`:
- Around line 38-58: The current match on (encryption, cipher_mode) uses a
wildcard arm that treats any unsupported/new CipherMode as Camellia-CTR, which
can cause silent mis-decryption; update the match in the function building
DecryptReader so it only accepts the explicit supported tuples (Encryption::Aes,
CipherMode::CBC), (Encryption::Aes, CipherMode::CTR), (Encryption::Camellia,
CipherMode::CBC), and (Encryption::Camellia, CipherMode::CTR) and construct the
corresponding DecryptReader variants (DecryptReader::CbcAes,
DecryptReader::CtrAes, DecryptReader::CbcCamellia, DecryptReader::CtrCamellia)
after reading the IV, and for any other (e.g., CipherMode::Reserved/_Private)
return an Err variant (e.g., Unsupported) rather than falling back to
CtrCamellia; ensure the same explicit handling is applied to the later analogous
block as well.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fc2f8d1f-1bd7-4d55-84fb-065944accbb5
📒 Files selected for processing (7)
cli/src/command/chmod.rscli/src/command/diff.rscli/src/command/list.rslib/src/entry/header.rslib/src/entry/options.rslib/src/entry/read.rslib/src/entry/write.rs
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
2 participants
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.
Summary by CodeRabbit
Bug Fixes
Improvements