Encryption: fix passphrase encryption not working in some cases#2215
Conversation
## Walkthrough
The changes refine encryption key handling by adding URL encoding of key data before shell command execution and securing shell arguments with escapeshellarg. The ArrayOperation page's encryption detection now filters disks by type. DiskSettings adds ASCII validation for passphrase input. The service reload script validates delay parameters. Minor code style and logic clarifications are applied in multiple scripts, including new helper functions for disk filtering and a shell argument escaping wrapper.
## Changes
| File(s) | Change Summary |
|-----------------------------------------------------|--------------------------------------------------------------------------------------------------------|
| emhttp/plugins/dynamix/ArrayOperation.page | Refined LUKS encryption detection to consider only 'Data' or 'Cache' disks; improved PHP and JS code style and spacing. |
| emhttp/plugins/dynamix/include/update.encryption.php | Added escapeshellarg and shieldarg to shell commands, URL-encoded key data, removed unused variables, and updated copyright years to 2025. |
| emhttp/plugins/dynamix/scripts/update_services | Validated and sanitized delay parameter for reload_services job scheduling with logging. |
| sbin/monitor_nchan | Added parentheses in compound condition to clarify logical operator precedence in nchan_idle function. |
| emhttp/plugins/dynamix/DiskSettings.page | Added ASCII character validation for new passphrase input with user alert; fixed HTML structure and form submission logic. |
| emhttp/plugins/dynamix/include/KeyUpload.php | Reversed priority of POST handling for keyfile updates; updated copyright years; removed unused variables and temp file handling. |
| emhttp/plugins/dynamix/nchan/device_list | Updated copyright years; improved code readability with trailing blank lines; refined vfs_type logic for encryption detection. |
| emhttp/plugins/dynamix/include/Helpers.php | Added functions luks_only() and luks_filter() to filter disks by type; applied code style and formatting improvements. |
| emhttp/plugins/dynamix/include/Wrappers.php | Reformatted condition in file_put_contents_atomic; updated http_get_contents signature formatting; added shieldarg() function to escape shell arguments. |
## Suggested labels
`7.2`
## Suggested reviewers
- ljm42
- zackspear
## Poem
> 🐇 Hopping through code with keys in paw,
> Encoding secrets without a flaw.
> ASCII checked, safe commands run,
> Encryption’s dance is never done.
> From disks to scripts, all neat and bright,
> A bunny’s work done just right! 🥕🔐 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
emhttp/plugins/dynamix/ArrayOperation.page(1 hunks)emhttp/plugins/dynamix/DiskSettings.page(1 hunks)emhttp/plugins/dynamix/include/update.encryption.php(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
emhttp/plugins/dynamix/include/update.encryption.php (1)
emhttp/plugins/dynamix/include/Wrappers.php (1)
_var(91-93)
🔇 Additional comments (2)
emhttp/plugins/dynamix/include/update.encryption.php (2)
91-91: Good security improvement for key data encoding.URL encoding the LUKS key data before passing it to the command ensures special characters are properly handled and prevents potential issues with the data transmission.
107-107: Excellent security enhancement with shell escaping.Using
escapeshellarg()to wrap the entire command argument prevents shell injection vulnerabilities and ensures the encryption key data is safely passed to the external command.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
emhttp/plugins/dynamix/scripts/update_services (1)
18-24: Good input validation, but consider allowing zero as a valid delay.The validation logic correctly prevents non-numeric input and potential injection attacks. However, the regex
^[0-9]+$excludes zero, which might be a valid delay value in some scenarios.Consider this adjustment to allow zero:
-if [[ -n $1 && ! $1 =~ ^[0-9]+$ ]]; then +if [[ -n $1 && ! $1 =~ ^[0-9]*$ ]] || [[ $1 == "0"* && $1 != "0" ]]; thenOr more simply:
-if [[ -n $1 && ! $1 =~ ^[0-9]+$ ]]; then +if [[ -n $1 && ! $1 =~ ^(0|[1-9][0-9]*)$ ]]; then
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
emhttp/plugins/dynamix/scripts/update_services(1 hunks)sbin/monitor_nchan(1 hunks)
🔇 Additional comments (1)
sbin/monitor_nchan (1)
19-19: Good clarity improvement for operator precedence.The added parentheses make the logical operator precedence explicit and improve code readability. While technically redundant (since
&&has higher precedence than||in bash), this change makes the intended logic crystal clear.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
emhttp/plugins/dynamix/ArrayOperation.page(28 hunks)emhttp/plugins/dynamix/DiskSettings.page(7 hunks)emhttp/plugins/dynamix/include/KeyUpload.php(2 hunks)emhttp/plugins/dynamix/include/update.encryption.php(2 hunks)emhttp/plugins/dynamix/nchan/device_list(13 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- emhttp/plugins/dynamix/include/update.encryption.php
- emhttp/plugins/dynamix/ArrayOperation.page
🔇 Additional comments (10)
emhttp/plugins/dynamix/nchan/device_list (3)
3-4: LGTM: Copyright year update.The copyright year updates to 2025 are appropriate administrative changes.
37-37: LGTM: Improved code readability.The addition of trailing blank lines after function definitions improves code readability and maintains consistent formatting throughout the file.
Also applies to: 41-41, 47-47, 90-90, 108-108, 127-127, 131-131, 159-159, 185-185, 189-189, 267-267, 322-322, 359-359, 378-378, 401-401, 420-420
136-137:Details
❓ Verification inconclusive
Excellent bug fix for encryption state handling.
The changes improve the robustness of encryption detection logic:
- Line 136: Returning
nullinstead of empty string whenfsTypeis empty provides better type consistency- Line 137: Adding the explicit
$cryptoflag check ensures encryption state processing only occurs when:
- The filesystem type starts with "luks:" (via
vfs_luks())- The global crypto flag is enabled
This prevents unnecessary encryption state processing and aligns with the PR objective of fixing passphrase encryption issues.
Verify that the return value change from empty string to
nulldoesn't break any calling code:
🏁 Script executed:
#!/bin/bash # Description: Check how vfs_type function return value is used throughout the codebase # Expected: Find usage patterns to ensure null return value is handled properly # Search for direct calls to vfs_type function rg -A 3 -B 1 "vfs_type\(" --type php # Search for any string concatenation or direct output of vfs_type results rg -A 2 -B 2 "\.vfs_type\(" --type phpLength of output: 79
No references to vfs_type() detected – please verify impact
I ran a repository-wide search forvfs_type((and.vfs_type() and found no other callers. Before approving, please confirm that no downstream code relies on an empty-string return from this function rather thannull.emhttp/plugins/dynamix/DiskSettings.page (4)
34-34: LGTM: Improved code formattingThe addition of blank lines after function definitions improves code readability and follows good formatting practices.
Also applies to: 49-49, 63-63, 67-67, 87-87, 94-94, 116-116, 125-125
74-86: Excellent security enhancement: ASCII validation for passphrasesThe implementation correctly validates that passphrases contain only printable ASCII characters (space to tilde), which prevents potential issues with shell command execution and character encoding. The regex pattern
^[ -~]+$is accurate, and the error message clearly guides users to use the keyfile method for UTF-8 input.This addresses the core issue mentioned in the PR title about passphrase encryption not working in some cases.
290-290: Critical fix: Form validation enforcementThe change to
return prepareForm(this)is essential for the ASCII validation to work. Without returning the validation result, the form would submit regardless of validation failure. This correctly implements client-side validation control.
318-318: Good fix: HTML structure correctionAdding the missing closing
</div>tag improves the HTML markup validity and structure.emhttp/plugins/dynamix/include/KeyUpload.php (3)
2-3: LGTM: Copyright year updatesStandard administrative update to reflect the current year.
14-14: Good simplification: Variable assignmentThe direct assignment from
parse_ini_fileis cleaner and more straightforward.
19-23:Details
✅ Verification successful
Verify keyfile path security
Ensure that the
$lukskeyfile path from the configuration cannot be manipulated by user input to prevent path traversal attacks.
🏁 Script executed:
#!/bin/bash # Description: Check how luksKeyfile path is set and validated in the configuration # Expected: The path should be controlled and not user-manipulable # Search for luksKeyfile configuration and validation rg -A 5 -B 5 "luksKeyfile"Length of output: 4573
🏁 Script executed:
#!/bin/bash # Search for any writes to var.ini in the repository rg -n "var\.ini" --context=3Length of output: 24274
KeyUpload path is server-controlled
The$luksvariable is sourced from/var/local/emhttp/var.ini(loaded viaparse_ini_file) and never comes from user input. There’s no direct POST-driven modification of the keyfile path, so path-traversal via user-supplied data is not possible.
3 participants
Summary by CodeRabbit