fix: salt persistence and padding validation in encryption - #505
fix: salt persistence and padding validation in encryption#505saurabhhhcodes wants to merge 1 commit into
Conversation
|
👋 Thank you for opening this pull request! We're excited to review your contribution. Please give us a moment, and we'll get back to you shortly! Feel free to join our community on Discord to discuss more! |
📝 WalkthroughWalkthroughAES-CBC padding validation now rejects malformed padding. Encrypted payloads can include a salt prefix, while the CLI stores salts in ChangesEncryption and salt handling
Estimated code review effort: 3 (Moderate) | ~15–30 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant Encryption
participant SaltFile
CLI->>Encryption: Encrypt data with salt
Encryption-->>CLI: Salt-prefixed ciphertext
CLI->>SaltFile: Write salt
CLI->>SaltFile: Read salt
CLI->>Encryption: Decrypt with salt length
Encryption-->>CLI: Plaintext
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@pysnippets/encryption/encryption.py`:
- Around line 75-80: Update the unpadding logic to validate that data is
non-empty and its length is AES block-aligned before accessing data[-1]. Raise
ValueError for empty or non-block-aligned input, while preserving the existing
padding-length, padding-byte, and successful unpadding behavior.
- Around line 214-221: Update the encryption flow around cipher.encrypt_file and
cipher.encrypt_folder so file and folder modes are mutually exclusive: when
file_path is provided, encrypt only that file and its salt sidecar; when
folder_path is provided, encrypt only the folder without recursively
re-encrypting the selected file or any .salt sidecars. Preserve the existing
salt-writing behavior and ensure the corresponding decryption path applies the
same exclusion rules.
- Around line 214-218: Update the encryption flow around encrypt_file so the
salt sidecar is durably written before replacing the plaintext, and handle any
failed encryption result without leaving unrecoverable ciphertext. Ensure the
ciphertext and salt updates are coordinated transactionally, preserving the
existing salt_path behavior while preventing partial completion.
🪄 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 Plus
Run ID: 56dee2dd-72c1-4e1e-8cfd-1c963bc93744
📒 Files selected for processing (1)
pysnippets/encryption/encryption.py
| pad_length = data[-1] | ||
| if pad_length < 1 or pad_length > AES.block_size: | ||
| raise ValueError("Invalid padding length") | ||
| if data[-pad_length:] != bytes([pad_length] * pad_length): | ||
| raise ValueError("Invalid padding bytes") | ||
| return data[:-pad_length] |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reject empty and non-block-aligned padded data.
Line 75 raises IndexError for empty input, and b"\x01" is accepted despite not being an AES block. Raise ValueError before indexing unless data is non-empty and block-aligned.
🤖 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 `@pysnippets/encryption/encryption.py` around lines 75 - 80, Update the
unpadding logic to validate that data is non-empty and its length is AES
block-aligned before accessing data[-1]. Raise ValueError for empty or
non-block-aligned input, while preserving the existing padding-length,
padding-byte, and successful unpadding behavior.
| cipher.encrypt_file(file_path) | ||
| # Save salt alongside encrypted data | ||
| salt_path = file_path + ".salt" | ||
| with open(salt_path, "wb") as f: | ||
| f.write(salt) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Persist recovery material before replacing plaintext.
encrypt_file() overwrites the source at Line 214 before the salt is written. A termination or write failure between these operations leaves ciphertext whose key cannot be re-derived. Make the ciphertext and sidecar update durable/transactional, at minimum persisting the salt before replacing the file and handling a failed encryption result.
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 216-216: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(salt_path, "wb")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(open-filename-from-request)
🤖 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 `@pysnippets/encryption/encryption.py` around lines 214 - 218, Update the
encryption flow around encrypt_file so the salt sidecar is durably written
before replacing the plaintext, and handle any failed encryption result without
leaving unrecoverable ciphertext. Ensure the ciphertext and salt updates are
coordinated transactionally, preserving the existing salt_path behavior while
preventing partial completion.
| cipher.encrypt_file(file_path) | ||
| # Save salt alongside encrypted data | ||
| salt_path = file_path + ".salt" | ||
| with open(salt_path, "wb") as f: | ||
| f.write(salt) | ||
| logging.info(f"Salt saved to {salt_path} — keep this file to decrypt later") | ||
| if folder_path: | ||
| cipher.encrypt_folder(folder_path) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Do not recursively encrypt the selected file or its salt sidecar.
If file_path is inside folder_path, Line 214 encrypts it, Lines 216-218 create <file>.salt, and Line 221 encrypts both again. The sidecar is then ciphertext, so decryption derives a key from invalid salt bytes and cannot recover the files. Make file and folder modes mutually exclusive, or exclude the selected file and all .salt sidecars consistently in both directions.
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 216-216: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(salt_path, "wb")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(open-filename-from-request)
🤖 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 `@pysnippets/encryption/encryption.py` around lines 214 - 221, Update the
encryption flow around cipher.encrypt_file and cipher.encrypt_folder so file and
folder modes are mutually exclusive: when file_path is provided, encrypt only
that file and its salt sidecar; when folder_path is provided, encrypt only the
folder without recursively re-encrypting the selected file or any .salt
sidecars. Preserve the existing salt-writing behavior and ensure the
corresponding decryption path applies the same exclusion rules.
1 participant
Fixed two bugs in encryption module:
Summary by CodeRabbit
New Features
.saltfile for future decryption.Bug Fixes