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 listing with a NULL-delimited find/getfattr pipeline, decodes octal escapes, enumerates and flags broken symlinks, resolves device names from deviceIDs, preserves newlines in names, and migrates file-manager job storage/parsing from INI blocks to JSON-per-line. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Browser as Client (Browser UI)
participant Web as emhttp PHP
participant FS as System (find / getfattr)
participant Jobs as Jobs file (JSON lines)
Note over Browser,Web: Directory listing request
Browser->>Web: GET listing for path
Web->>FS: run `find -L ... -print0` + `getfattr` (NULL-delimited 8-field stream)
FS-->>Web: NULL-delimited stream (type,linktype,owner,perms,size,ts,name,devID)
Web->>Web: parse 8-field groups, unescape octal, map symlinks, detect broken links, resolve devID→dev_name
Web-->>Browser: Render rows (icons incl. broken, nl2br names, symlink tooltips, LOCATION)
Note over Browser,Jobs: Job queue operations
Browser->>Web: POST start / undo (row id or JSON)
Web->>Jobs: read/modify/write JSON lines (consume/append/remove)
Jobs-->>Web: updated queue file
Web-->>Browser: operation result
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
📜 Recent review detailsConfiguration used: Repository UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (3)📚 Learning: 2025-12-30T20:27:59.455ZApplied to files:
📚 Learning: 2025-02-01T15:12:16.914ZApplied to files:
📚 Learning: 2025-12-28T15:54:58.673ZApplied to files:
🧬 Code graph analysis (1)emhttp/plugins/dynamix/include/Browse.php (4)
🔇 Additional comments (8)
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).
This PR addresses multiple issues and feature requests from unraid#2500: 1. Fix PR unraid#2402 Markdown Parser regression (literal colon bug) - Changed dialog templates from <div> to <span> for inline processing - Cleaned up unused dfm_info CSS rules 2. Show total size of running transfer - Enhanced rsync parser to calculate total transfer size - Displays 'Transferring X of Y' format 3. Show last N used destination paths in FileTree - New PopularDestinations.php with frequency-based scoring - FUSE conflict prevention for /mnt/user vs /mnt/diskX 4. Manually typing destination path updates FileTree - Keyboard navigation with arrow keys and Enter/Escape - Automatic FileTree open/close on input interaction 5. Add 'Open Terminal here' button - OpenTerminal.php accepts path parameter - File Manager can open terminal at selected folder Note: This PR depends on unraid#2491 and unraid#2490 being merged first.
…all icons - Use find -L for both user share and disk paths - Build device ID map from all /mnt/* directories - Add 'symlink' case to icon_class() function - Set ext='symlink' for user share symlinks, use icon_class() consistently
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
emhttp/plugins/dynamix/include/Browse.php (1)
186-188: Consider memory usage for large directories (optional).Reading the entire
findoutput into memory withstream_get_contents()could consume significant memory for directories with thousands of files. However, this is typical for file manager UIs and not a regression from the previous implementation.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
emhttp/plugins/dynamix/include/Browse.php
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-28T15:54:58.673Z
Learnt from: mgutt
Repo: unraid/webgui PR: 2496
File: emhttp/plugins/dynamix/Browse.page:901-906
Timestamp: 2025-12-28T15:54:58.673Z
Learning: In the unraid/webgui codebase, CSRF validation is centralized in the global auto_prepend_file (local_prepend.php) which runs before every PHP request. Do not add per-script CSRF checks in individual files like Browse.page or Control.php. If a script relies on global validation, ensure it does not duplicate CSRF logic; otherwise extend the central preface to cover the needed checks.
Applied to files:
emhttp/plugins/dynamix/include/Browse.php
🔇 Additional comments (4)
emhttp/plugins/dynamix/include/Browse.php (4)
85-88: LGTM!The new icon mappings for broken symlinks and regular symlinks are clear and appropriate.
154-161: Device mapping approach looks good.The device ID to name mapping is well-implemented and includes a sensible fallback (line 209:
$device_map[$device_id] ?? $lock) when the map is incomplete.
165-172: Octal escape decoding is correctly implemented.The bounds checking and octal decoding logic properly handles getfattr output variations and matches the actual newline characters from the find command.
216-252: Rendering logic correctly handles special characters.The use of
nl2br(htmlspecialchars(basename($name)))on lines 220 and 242 properly preserves newlines while preventing XSS. Disabling the editor for broken symlinks (line 242) is a good UX decision.Note: Due to the symlink detection issue flagged above, lines 234-235 (
elseif ($is_symlink && $user)) are effectively unreachable since$is_symlinkis only true for broken symlinks, which are already handled by the earlierif ($is_broken)condition.
- Always display target file icon, size, and permissions for symlinks in both user share and disk views - Add fa-external-link icon after filename to visually indicate symlinks - Include Unraid-style tooltip on external-link icon showing symlink target path - Support broken symlinks with tooltip showing their invalid target path - Move file edit onclick from table cell to filename span to prevent tooltip click interference - Remove unused 'symlink' case from icon_class function - Fix LOCATION column to properly display current disk in disk view
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
emhttp/plugins/dynamix/include/Browse.php (1)
160-160: Add bounds check for explode result.The direct array access
explode('"',$tmp[$i+1])[1]will cause an undefined offset error if the getfattr attribute line is malformed (missing quotes or only one quote).🔎 Proposed fix
if (!isset($tmp[$i+1])) break; $filename = preg_replace_callback('/\\\\([0-7]{3})/', function($m) { return chr(octdec($m[1])); }, $tmp[$i]); - $set[basename($filename)] = explode('"',$tmp[$i+1])[1]; + $parts = explode('"', $tmp[$i+1]); + if (count($parts) < 2) continue; // Skip if attribute value is malformed + $set[basename($filename)] = $parts[1]; }
🧹 Nitpick comments (1)
emhttp/plugins/dynamix/include/Browse.php (1)
231-250: Solid file rendering with proper broken-symlink handling.The conditional editing (disabled for broken symlinks), consistent
nl2br()usage for newlines, and proper XSS prevention withhtmlspecialchars()are all correct.💡 Optional: Extract duplicate symlink tooltip construction
The symlink tooltip construction on lines 221 and 240 is identical. Consider extracting it into a helper function to reduce duplication:
function symlink_tooltip($is_symlink, $target) { return $is_symlink ? '<a class="info" href="#" onclick="return false;"><i class="fa fa-external-link" style="margin-left:4px;"></i><span>'.htmlspecialchars($target).'</span></a>' : ''; }Then use:
$symlink_tooltip = symlink_tooltip($is_symlink, $symlinks[basename($name)]['target'] ?? '');
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
emhttp/plugins/dynamix/include/Browse.php
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-28T15:54:58.673Z
Learnt from: mgutt
Repo: unraid/webgui PR: 2496
File: emhttp/plugins/dynamix/Browse.page:901-906
Timestamp: 2025-12-28T15:54:58.673Z
Learning: In the unraid/webgui codebase, CSRF validation is centralized in the global auto_prepend_file (local_prepend.php) which runs before every PHP request. Do not add per-script CSRF checks in individual files like Browse.page or Control.php. If a script relies on global validation, ensure it does not duplicate CSRF logic; otherwise extend the central preface to cover the needed checks.
Applied to files:
emhttp/plugins/dynamix/include/Browse.php
🧬 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 (1)
my_scale(19-39)emhttp/plugins/dynamix/include/Control.php (1)
my_age(25-35)
🔇 Additional comments (3)
emhttp/plugins/dynamix/include/Browse.php (3)
85-86: LGTM! Clear visual indicator for broken symlinks.The red chain icon appropriately signals broken symlinks to users.
165-175: Excellent symlink detection approach.Using
findwithout-Lcorrectly identifies all symlinks (both working and broken), addressing the limitation mentioned in past reviews wherefind -Lwould only detect broken symlinks as type 'l'. The tab-delimited format and bounds checking are appropriate.
177-212: Well-designed NULL-delimited processing.The 8-field NULL-delimited approach correctly handles filenames with newlines and special characters. The loop bounds are correct (addressing past review concerns), and the symlink detection using the pre-built map properly identifies both working and broken symlinks.
Remove htmlspecialchars() from rawurlencode() calls in href attributes. rawurlencode() already produces HTML-safe output, so htmlspecialchars() causes double-encoding which breaks navigation for paths with &, <, >, ", or '. Example: - Before: a&b → htmlspecialchars → a&b → rawurlencode → a%26amp%3Bb ❌ - After: a&b → rawurlencode → a%26b ✓ Fixes: parent_link() and folder navigation hrefs
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
emhttp/plugins/dynamix/include/Browse.php (1)
136-136: Critical: Line 136 must be updated to match URL encoding changes.The URL encoding fixes on lines 58 and 222 removed
htmlspecialchars()wrapping, but line 136 still applieshtmlspecialchars_decode(). This creates an inconsistency that will break navigation for paths containing&,<,>,", or'.Current flow:
- Line 222 generates href:
rawurlencode($name)→ URL containsa%26bfor filenamea&b- PHP auto-decodes
$_GET['dir']→ containsa&b- Line 136 applies
htmlspecialchars_decode()→ stilla&b(nothing to decode)- Navigation works by accident, but logic is wrong
Broken scenario:
If a filename contains&(literal text), line 136 will incorrectly decode it to&, changing the filename.🔎 Proposed fix
-$dir = validdir(htmlspecialchars_decode(rawurldecode($_GET['dir']))); +$dir = validdir($_GET['dir'] ?? '');Rationale: PHP automatically URL-decodes
$_GETparameters, and the hrefs no longer contain HTML entities, so bothhtmlspecialchars_decode()andrawurldecode()should be removed.
♻️ Duplicate comments (1)
emhttp/plugins/dynamix/include/Browse.php (1)
156-161: Add bounds checking for explode on line 160.While the bounds check on line 158 prevents accessing missing lines, line 160's
explode('"',$tmp[$i+1])[1]could still fail if the attribute line is malformed (e.g., missing quotes). This would cause an undefined array index error.🔎 Proposed fix
for ($i = 0; $i < count($tmp); $i+=3) { // Check bounds: if getfattr fails for a file, we might not have all 3 lines if (!isset($tmp[$i+1])) break; $filename = preg_replace_callback('/\\\\([0-7]{3})/', function($m) { return chr(octdec($m[1])); }, $tmp[$i]); - $set[basename($filename)] = explode('"',$tmp[$i+1])[1]; + $parts = explode('"', $tmp[$i+1]); + if (count($parts) >= 2) { + $set[basename($filename)] = $parts[1]; + } }
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
emhttp/plugins/dynamix/include/Browse.php
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-12-30T20:27:59.455Z
Learnt from: mgutt
Repo: unraid/webgui PR: 2501
File: emhttp/plugins/dynamix/Browse.page:1282-1339
Timestamp: 2025-12-30T20:27:59.455Z
Learning: In emhttp/plugins/dynamix/Browse.page, the xlink() function's clipboard functionality is intentionally designed to silently fail on HTTP connections without showing warnings. The path text is pre-selected to allow manual copying, which is preferred over showing repetitive error messages that would annoy HTTP users.
Applied to files:
emhttp/plugins/dynamix/include/Browse.php
📚 Learning: 2025-02-01T15:12:16.914Z
Learnt from: SimonFair
Repo: unraid/webgui PR: 1992
File: emhttp/plugins/dynamix/include/CPUset.php:0-0
Timestamp: 2025-02-01T15:12:16.914Z
Learning: In the unRAID WebGUI, when both translation and HTML escaping are needed, always apply translation (_()) first, then escape the translated text with htmlspecialchars() to ensure proper handling of translation placeholders.
Applied to files:
emhttp/plugins/dynamix/include/Browse.php
📚 Learning: 2025-12-28T15:54:58.673Z
Learnt from: mgutt
Repo: unraid/webgui PR: 2496
File: emhttp/plugins/dynamix/Browse.page:901-906
Timestamp: 2025-12-28T15:54:58.673Z
Learning: In the unraid/webgui codebase, CSRF validation is centralized in the global auto_prepend_file (local_prepend.php) which runs before every PHP request. Do not add per-script CSRF checks in individual files like Browse.page or Control.php. If a script relies on global validation, ensure it does not duplicate CSRF logic; otherwise extend the central preface to cover the needed checks.
Applied to files:
emhttp/plugins/dynamix/include/Browse.php
🧬 Code graph analysis (1)
emhttp/plugins/dynamix/include/Browse.php (4)
emhttp/plugins/dynamix/include/FileTree.php (1)
path(30-32)emhttp/plugins/dynamix/include/SmartInfo.php (1)
size(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 (7)
emhttp/plugins/dynamix/include/Browse.php (7)
58-58: LGTM! URL encoding fix applied correctly.The change to use
rawurlencode($parent)withouthtmlspecialchars()is correct. URL-encoded output is already HTML-safe for href attributes.
85-86: LGTM! Broken symlink icon case added.The new
'broken-symlink'case with a red chain-broken icon provides clear visual feedback for broken symlinks.
165-175: LGTM! Symlink detection correctly implemented.The approach of using
findwithout-Lto detect all symlinks (working and broken) and building a lookup map is correct and addresses the previous issue wherefind -Lcouldn't identify working symlinks.The bounds checking on line 172 ensures malformed output doesn't corrupt the map.
177-194: LGTM! Null-delimited 8-field parsing correctly handles special characters.The implementation properly addresses special character handling in filenames:
- NULL delimiters support newlines, quotes, and other special characters
- Loop condition
$i + 8 <= count($fields_array)correctly ensures complete 8-field records- Command injection protection via
escapeshellarg($dir)- Field format (type, linktype, owner, perms, size, timestamp, name, device_id) provides all needed data
Note: Using
stream_get_contents()to read all output at once may use significant memory for very large directories, but this is acceptable for a file manager UI.
195-213: LGTM! Entry processing and device resolution logic is sound.The implementation correctly:
- Identifies broken symlinks via
$link_type == 'N'- Detects all symlinks (working and broken) via the pre-built map
- Resolves device names appropriately for user shares (via xattr/share config) vs disk paths (via
$lock)- Handles multi-device scenarios with warning tags
217-229: LGTM! Directory rendering properly handles special characters and symlinks.The implementation correctly:
- Uses
nl2br()to preserve newlines in filenames without breaking HTML- Applies
rawurlencode($name)for hrefs (correct encoding order per past review)- Uses
htmlspecialchars()for display text (XSS protection)- Shows symlink targets in tooltips with proper escaping
- Uses
escapeQuote()for data attributes
231-249: LGTM! File rendering correctly handles broken symlinks and special characters.The implementation properly:
- Sets
ext = 'broken-symlink'to trigger the red chain icon for broken symlinks- Disables editing for broken symlinks while allowing it for working files/symlinks
- Applies
nl2br()andhtmlspecialchars()to preserve newlines and prevent XSS- Shows symlink targets in tooltips with safe escaping
Design note: Working symlinks are editable, which will edit the target file (since
find -Lfollows the link). This appears intentional, as the symlink icon with tooltip informs users they're working with a symlink.
1. Remove htmlspecialchars_decode() from line 136: - href now uses rawurlencode() without htmlspecialchars() - PHP auto-decodes $_GET['dir'], so only rawurldecode() is needed - Prevents incorrect decoding of literal HTML entities in filenames 2. Add bounds checking for explode() on getfattr output: - Prevents undefined array index error if attribute line is malformed - Check count(parts) >= 2 before accessing parts[1]
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.