Fix #2488: Handle special characters in filenames #2491
Conversation
- Properly escape backslashes, quotes, and ${ in INI files (Control.php)
- Use find -exec stat --printf with null-terminated output (Browse.php)
- Decode getfattr octal escapes for LOCATION column lookup
- Add nl2br() for newline display in filename cells
Fixes unraid#2488
WalkthroughReplaces stat-based directory enumeration with a NULL-delimited find/getfattr pipeline that decodes octal escapes, preserves newlines in displayed names, detects broken symlinks, derives device/location from resolved targets, and switches file-manager job storage and parsing from INI-style blocks to JSON-per-line. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
🔧 PR Test Plugin AvailableA test plugin has been generated for this PR that includes the modified files. Version: 📥 Installation Instructions:Install via Unraid Web UI:
Alternative: Direct Download
|
![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: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
emhttp/plugins/dynamix/include/Browse.phpemhttp/plugins/dynamix/include/Control.php
🔇 Additional comments (5)
emhttp/plugins/dynamix/include/Control.php (1)
167-169: Escaping logic looks correct.The escape order (backslashes first, then quotes, then
${) is correct to prevent double-escaping and INI constant interpolation. Good defensive handling for special characters in filenames.emhttp/plugins/dynamix/include/Browse.php (4)
152-158: Good fix for octal escape decoding.The
preg_replace_callbackcorrectly decodes octal escapes (e.g.,\012for newline) fromgetfattroutput to match the actual filenames returned byfind. This ensures the lookup in$setworks correctly for filenames containing special characters.
161-166: Robust approach using NULL-delimited output.Using
find -exec stat --printf ... '\0'withstream_get_line(..., "\0")correctly handles filenames containing newlines. Theexplode('|', $row, 6)with limit 6 ensures any|characters within the filename (in%n) are preserved in the$namevariable.
174-175: Correct escaping order for XSS safety.Applying
htmlspecialchars()beforenl2br()ensures HTML entities are escaped before<br>tags are inserted. This safely renders filenames with embedded newlines.
188-188: Consistent with folder handling.Same safe pattern applied for file rows.
- Added htmlspecialchars_decode() to target (PR feedback) - Optimized: replaced stat with find -printf only - Use \0 as single delimiter, process in 7-field groups - Improved code comments for better readability
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
emhttp/plugins/dynamix/include/Control.php (1)
172-176: Critical: Apply the same newline/CR escaping fix here.The
$dsthandling has the same missing newline and carriage return escaping as$src. Apply the fix described in the previous comment to lines 173-175.
🧹 Nitpick comments (1)
emhttp/plugins/dynamix/include/Browse.php (1)
178-178: Consider clearer loop condition.The loop condition
$i < count($fields_array) - 7is correct but$i + 7 <= count($fields_array)would more clearly express the intent of ensuring 7 fields are available.🔎 Alternative formulation
-for ($i = 0; $i < count($fields_array) - 7; $i += 7) { +for ($i = 0; $i + 7 <= count($fields_array); $i += 7) {
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
emhttp/plugins/dynamix/include/Browse.phpemhttp/plugins/dynamix/include/Control.php
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-03-27T22:04:00.594Z
Learnt from: zackspear
Repo: unraid/webgui PR: 2099
File: emhttp/plugins/dynamix.my.servers/include/activation-code-extractor.php:58-74
Timestamp: 2025-03-27T22:04:00.594Z
Learning: The file `emhttp/plugins/dynamix.my.servers/include/activation-code-extractor.php` is synced from a different repository, and modifications should not be suggested in this repository's context. Changes should be proposed in the source repository instead.
Applied to files:
emhttp/plugins/dynamix/include/Control.php
🧬 Code graph analysis (1)
emhttp/plugins/dynamix/include/Browse.php (1)
emhttp/plugins/dynamix/include/Helpers.php (1)
my_scale(19-39)
🔇 Additional comments (7)
emhttp/plugins/dynamix/include/Browse.php (5)
164-170: Clever two-pass approach for handling broken symlinks.The separate find commands for working entries and broken symlinks ensure that all entries are captured with proper metadata. The NULL-separated format correctly handles filenames containing newlines and other special characters.
173-176: LGTM: Correct handling of NULL-delimited output.Reading the entire output and splitting by NULL bytes is the appropriate approach for handling filenames with special characters including newlines.
187-221: Verify device resolution for absolute symlinks in user shares.For absolute symlinks in user shares (lines 205-206), the code uses
$set[basename($name)]which retrieves the xattr from the symlink's source location. This shows where the symlink is stored rather than where it points. Confirm this is the intended behavior for the LOCATION column.If the LOCATION column should instead show where an absolute symlink points to, consider using the target's device for user shares as well:
if ($user) { // For absolute symlinks in user shares, consider using target device if ($target && $target[0] == '/') { $devs_value = $dev_name; // dev_name from target path (lines 190-192) } else { $devs_value = $set[basename($name)] ?? $shares[$dev_name]['cachePool'] ?? ''; } }
229-229: Excellent use of nl2br() for preserving newlines.Using
nl2br(htmlspecialchars(basename($name)))correctly displays filenames containing newlines while maintaining XSS protection. This addresses the PR objective of handling special characters safely.Also applies to: 243-243
240-246: Broken symlink handling addresses previous review concern.The explicit broken symlink detection (type == 'broken') and visual indicator (red chain icon) properly address the past review comment about broken symlink handling. The two-pass find approach ensures broken symlinks are captured with a distinct type marker, eliminating the risk of malformed records.
emhttp/plugins/dynamix/include/Control.php (2)
172-172: Previous inconsistency resolved.The addition of
htmlspecialchars_decode()to$dstmakes it consistent with$src(line 166) and other target path handling in this file (e.g., line 100). This addresses the concern raised in the previous review.
169-169: The\${escaping correctly prevents INI constant substitution inparse_ini_string().The code at lines 169 and 175 properly escapes
${to\${usingstr_replace(), which is the standard PHP approach documented in the official PHP manual. This escaping works as intended with the defaultINI_SCANNER_NORMALmode used at line 113. The implementation is correct and the defensive comment about PHP version behavior is reasonable, though documented behavior is consistent across supported versions.
…rovements
- Switch from INI to JSON format for file.manager.active/jobs communication
- Handles all special characters (newlines, CR, quotes, backslashes, ${) natively
- Simplifies code by removing manual escaping logic
- Updates Control.php and file_manager to use json_encode/json_decode
- Add bounds checking for getfattr output parsing
- Prevents undefined index errors if getfattr fails for individual files
- Safely handles incomplete output with isset() check
- Move broken symlink icon to icon_class() function
- Better code semantics with dedicated 'broken-symlink' extension type
- Removes hardcoded icon logic from table generation
- Disable onclick for broken symlinks
- Prevents text editor from opening when clicking broken symlinks
- Avoids accidentally creating target files through the editor
- Maintains context menu functionality for operations like delete
Fixes unraid#2488
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
emhttp/plugins/dynamix/include/Browse.phpemhttp/plugins/dynamix/include/Control.phpemhttp/plugins/dynamix/nchan/file_manager
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-03-27T22:04:00.594Z
Learnt from: zackspear
Repo: unraid/webgui PR: 2099
File: emhttp/plugins/dynamix.my.servers/include/activation-code-extractor.php:58-74
Timestamp: 2025-03-27T22:04:00.594Z
Learning: The file `emhttp/plugins/dynamix.my.servers/include/activation-code-extractor.php` is synced from a different repository, and modifications should not be suggested in this repository's context. Changes should be proposed in the source repository instead.
Applied to files:
emhttp/plugins/dynamix/nchan/file_manageremhttp/plugins/dynamix/include/Control.php
📚 Learning: 2025-03-27T22:04:34.550Z
Learnt from: zackspear
Repo: unraid/webgui PR: 2099
File: emhttp/plugins/dynamix.my.servers/include/web-components-extractor.php:13-19
Timestamp: 2025-03-27T22:04:34.550Z
Learning: The file emhttp/plugins/dynamix.my.servers/include/web-components-extractor.php is synced from another repository and should not be modified directly in the webgui repository.
Applied to files:
emhttp/plugins/dynamix/nchan/file_manager
🧬 Code graph analysis (1)
emhttp/plugins/dynamix/include/Browse.php (4)
emhttp/plugins/dynamix/include/SmartInfo.php (1)
size(30-32)emhttp/plugins/dynamix/include/FileTree.php (1)
path(30-32)emhttp/plugins/dynamix/include/Helpers.php (2)
my_time(47-51)my_scale(19-39)emhttp/plugins/dynamix/include/Control.php (1)
my_age(25-35)
🔇 Additional comments (11)
emhttp/plugins/dynamix/include/Control.php (5)
112-121: LGTM: JSON parsing with proper validation.The code correctly validates the
json_decode()result before accessing the data, and uses null coalescing operators for optional fields.
139-153: LGTM: Clean implementation of JSON-based job queue.The first-line extraction and queue management logic is correct and handles the transition from INI to JSON format appropriately.
185-202: LGTM: JSON encoding properly handles special characters.The switch to
json_encode()eliminates the need for manual escaping and natively handles special characters (newlines, quotes, backslashes,${}), addressing the core issue in the PR objectives.
159-176: The INI-to-JSON row conversion formula is correct and properly documented. The code explicitly states that the INI format had 9 lines per job entry, and the formulaintdiv($row - 1, 9)correctly converts INI row numbers (1, 10, 19, ...) to JSON line indices (0, 1, 2, ...). No changes needed.
178-181: No consumers of the 'read' endpoint were found in the codebase. The only modes used in the file manager are 'edit' and 'jobs'. This endpoint exists but appears to be unused or dead code. Verification of JSON format handling by consumers is not applicable since no callers exist.emhttp/plugins/dynamix/include/Browse.php (6)
154-161: LGTM: Proper bounds checking and octal decoding.The added bounds check prevents undefined index errors when getfattr output is incomplete, and the octal escape decoding correctly handles special characters in filenames.
165-179: LGTM: Robust NULL-delimited pipeline for special character handling.The two-pass
findapproach with NULL-delimited output correctly handles filenames containing newlines and distinguishes broken symlinks. The 7-field format is clearly documented.
85-86: LGTM: Clear visual indication for broken symlinks.The broken chain icon provides an appropriate visual cue for broken symlinks, consistent with the detection logic in the pipeline.
191-225: LGTM: Correct device resolution for symlinks.The logic properly distinguishes between absolute and relative symlinks when determining the LOCATION column value, showing the target's device for absolute symlinks as expected.
233-233: LGTM: Proper newline handling in displayed names.The use of
nl2br()correctly preserves newlines in directory names for display whilehtmlspecialchars()prevents XSS.
242-247: LGTM: Proper handling of broken symlinks in UI.Disabling
onclickfor broken symlinks prevents accidental file creation while preserving context menu functionality for other operations. Theis_brokenflag correctly controls this behavior.
- Browse.php: Fix loop condition to prevent skipping last entry - Changed from: $i < count($fields_array) - 7 - Changed to: $i + 7 <= count($fields_array) - Bug: With 7 fields (1 entry), loop wouldn't run at all (0 < 0) - Bug: With 14 fields (2 entries), last entry was skipped (7 < 7) - file_manager: Add JSON validation before extract() - Prevents PHP warning when json_decode() returns null - Check is_array($data) before calling extract()
- Remove legacy INI calculation from Browse.page line 316 - Frontend now sends direct job numbers (1,2,3...) to backend - Backend converts to array indices (0,1,2...) in Control.php - Consistent 1-based UI numbering for users (Job [0001], [0002]...) - Eliminates confusion between old INI format and new JSON format
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
emhttp/plugins/dynamix/include/Browse.php (1)
165-206: Excellent refactoring to handle special characters safely.The NULL-delimited pipeline with two separate finds (working vs. broken symlinks) correctly handles filenames containing quotes, backslashes, newlines, and other special characters—directly addressing issue #2488. The 7-field format is well-structured.
The loop condition at line 182 (
$i + 7 <= count($fields_array)) correctly processes only complete 7-field groups and skips incomplete trailing data.The device name derivation logic for symlinks (lines 191-205) works correctly:
- Absolute symlink targets: device extracted from target path (e.g.,
/mnt/disk2/foo→disk2)- Regular files/relative symlinks: device from source path
Optional: Add field validation for extra robustness
Consider adding validation after
array_sliceto handle potential edge cases where find output might be malformed:for ($i = 0; $i + 7 <= count($fields_array); $i += 7) { $fields = array_slice($fields_array, $i, 7); + // Skip if any required fields are empty (except target which may be empty for non-symlinks) + if (empty($fields[0]) || empty($fields[1]) || empty($fields[5])) continue; [$type,$owner,$perm,$size,$time,$name,$target] = $fields;This would skip entries where type, owner, or name is unexpectedly empty, preventing potential rendering issues.
emhttp/plugins/dynamix/include/Control.php (1)
112-121: LGTM! JSON format correctly replaces INI parsing.The job listing now reads JSON lines instead of INI blocks. The basic validation at lines 113-115 handles empty lines and invalid JSON gracefully. Missing keys are handled by the
??operator with appropriate defaults.Optional: Add explicit JSON structure validation
Consider validating that required keys exist after decoding to make debugging easier if the JSON format is ever corrupted:
foreach ($rows as $row) { if (empty($row)) continue; $data = json_decode($row, true); - if (!$data) continue; + if (!$data || !isset($data['source'])) continue; $task = $data['task'] ?? '';This would skip malformed entries more explicitly, though the current approach handles them gracefully via
??defaults.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
emhttp/plugins/dynamix/Browse.pageemhttp/plugins/dynamix/include/Browse.phpemhttp/plugins/dynamix/include/Control.phpemhttp/plugins/dynamix/nchan/file_manager
🚧 Files skipped from review as they are similar to previous changes (1)
- emhttp/plugins/dynamix/nchan/file_manager
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-03-27T22:04:00.594Z
Learnt from: zackspear
Repo: unraid/webgui PR: 2099
File: emhttp/plugins/dynamix.my.servers/include/activation-code-extractor.php:58-74
Timestamp: 2025-03-27T22:04:00.594Z
Learning: The file `emhttp/plugins/dynamix.my.servers/include/activation-code-extractor.php` is synced from a different repository, and modifications should not be suggested in this repository's context. Changes should be proposed in the source repository instead.
Applied to files:
emhttp/plugins/dynamix/include/Control.php
🔇 Additional comments (7)
emhttp/plugins/dynamix/Browse.page (1)
315-315: LGTM! Job numbering simplified.The change from offset-based calculation to direct ID extraction correctly aligns with the backend's new JSON-based job handling. The frontend now sends 1-based job numbers directly, and Control.php converts them to 0-based array indices (line 162:
$line_number = $row - 1).emhttp/plugins/dynamix/include/Browse.php (3)
85-86: LGTM! Broken symlink icon added.The new icon class for broken symlinks provides clear visual feedback to users.
154-161: Well done addressing the bounds checking issue.The added
if (!isset($tmp[$i+1])) break;guard at line 158 correctly handles cases where getfattr output is incomplete, preventing undefined index access. The octal escape decoding ensures filenames with newlines are correctly matched.
229-256: Perfect handling of newlines and broken symlinks.The use of
nl2br()at lines 233 and 247 correctly preserves newline characters in filenames for display without breaking the WebGUI—a key fix for issue #2488.The broken symlink handling is well-implemented:
- Line 242: Detects broken symlinks (
type == 'broken')- Line 243: Uses 'broken-symlink' extension for icon
- Line 247: Disables editor for broken symlinks (conditional onclick)
- Shows red chain icon and symlink target device in location column
emhttp/plugins/dynamix/include/Control.php (3)
139-153: Start operation correctly queues first JSON job.The logic properly reads the first JSON line (line 142), writes it to the active file, removes it from the queue (line 144), and handles the empty queue case (lines 149-150).
160-175: Undo operation correctly converts job numbering.Line 162's conversion
$line_number = $row - 1correctly maps from 1-based job numbers (sent by frontend) to 0-based array indices. Theunset()at line 164 safely removes entries, andimplode()at line 168 correctly reassembles the remaining lines.
183-200: File operation correctly builds and stores JSON data.The data array construction (lines 183-192) cleanly maps POST parameters to JSON fields. The conditional logic at lines 193-200 properly handles both queuing (append JSON to jobs file) and immediate execution (write JSON to active file).
1 participant
Fixes #2488
New features:
Example:
Summary by CodeRabbit
Bug Fixes
Refactor
Chores
✏️ Tip: You can customize this high-level summary in your review settings.