feat: enhanced notification settings and translation cache fix #2503
Conversation
…ration, and maximum count
…ttings, update related help text, and refresh translation cache when source files are newer. Fix translation cache invalidation logic Previously, the translation system only checked for the existence of the cache file (.dot) and did not verify if the source file (.txt) had been modified. This caused updates to help text and translations to be ignored until the cache was manually cleared. This commit updates Translations.php to compare the `filemtime` of the source file against the cache file. If the source is newer, the cache is now automatically regenerated.
…ate `safe_filename` to allow dots and enforce length limits.
WalkthroughThis PR enhances notification management by introducing new customization options for display behavior, adding configuration defaults, updating help documentation, and extending the notify command script with custom filename support while improving cache validation for translations. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
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
|
Ajit-Mehrotra
changed the title
Ajit-Mehrotra
changed the title
![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
🧹 Nitpick comments (4)
emhttp/languages/en_US/helptext.txt (1)
2158-2159: Minor grammar refinement suggestion.The phrase "position of where" could be simplified to "position where" for better readability:
-Choose the position of where notification popups appear on screen. +Choose the position where notification popups appear on screen.emhttp/plugins/dynamix/Notifications.page (1)
143-146: Add maximum limit to duration field to establish reasonable bounds.The notification popup duration field currently has
min="1000"but no maximum limit. Consider addingmax="60000"for 60 seconds to match the pattern used in other number inputs on this page (e.g., the "Max notification popups" field at line 149 includesmax="10").Server-side validation does not currently exist for this field. The form data is processed through
/update.php, a generic configuration writer that does not perform field-specific validation, and thenotifyscript called afterward does not validate duration bounds. Adding client-side constraints via the max attribute will improve UX but should be paired with server-side validation to prevent invalid values if the client-side validation is bypassed.Suggested addition
_(Notification popup duration)_: -: <input type="number" name="duration" value="<?=$notify['duration'] ?? 5000?>" min="1000" step="500"> +: <input type="number" name="duration" value="<?=$notify['duration'] ?? 5000?>" min="1000" max="60000" step="500">emhttp/plugins/dynamix/scripts/notify (2)
91-97: Consider restricting dot placement to prevent unusual filenames.While the removal of path separators (
/,\) on line 93 prevents directory traversal, allowing dots anywhere in the filename (line 94) can create unusual or problematic names:
- Input like
"....file"or"..hidden"would create files with multiple leading dots- Hidden files (starting with
.) might not be intended- Multiple consecutive dots could cause confusion
Since the goal is to support file extensions (like
.notify), consider restricting dots to a single occurrence followed by alphanumeric characters at the end of the filename, or document that custom filenames should include their own extension.🔎 Example: Restrict dots to extension-only pattern
function safe_filename($string, $maxLength=255) { $special_chars = ["?", "[", "]", "/", "\\", "=", "<", ">", ":", ";", ",", "'", "\"", "&", "$", "#", "*", "(", ")", "|", "~", "`", "!", "{", "}"]; $string = trim(str_replace($special_chars, "", $string)); - $string = preg_replace('~[^0-9a-z -_.]~i', '', $string); + // Allow dots only in extension position (e.g., "filename.ext") + $string = preg_replace('~[^0-9a-z -_]~i', '', $string); + // If caller wants to add extension, they can append it after sanitization $string = preg_replace('~[- ]~i', '_', $string); // limit filename length to $maxLength characters return substr(trim($string), 0, $maxLength); }Then callers would explicitly add extensions:
$filename = safe_filename($customFilename) . '.notify';
275-297: Refactor: Reuse$cleanSubjectvariable to avoid redundant function calls.The
clean_subject($subject)is called on line 275 and stored in$cleanSubject, which is correctly used in the data arrays (lines 279, 289). However, lines 296 and 297 callclean_subject($subject)again instead of reusing the$cleanSubjectvariable.🔎 Proposed refactor to eliminate redundant calls
$cleanSubject = clean_subject($subject); $archiveData = [ 'timestamp' => $timestamp, 'event' => $event, 'subject' => $cleanSubject, 'description' => $description, 'importance' => $importance, ]; if ($message) $archiveData['message'] = str_replace('\n','<br>',$message); if (!$mailtest) file_put_contents($archive, build_ini_string($archiveData)); if (($entity & 1)==1 && !$mailtest && !$noBrowser) { $unreadData = [ 'timestamp' => $timestamp, 'event' => $event, 'subject' => $cleanSubject, 'description' => $description, 'importance' => $importance, 'link' => $link, ]; file_put_contents($unread, build_ini_string($unreadData)); } - if (($entity & 2)==2 || $mailtest) generate_email($event, clean_subject($subject), str_replace('<br>','. ',$description), $importance, $message, $recipients, $fqdnlink); + if (($entity & 2)==2 || $mailtest) generate_email($event, $cleanSubject, str_replace('<br>','. ',$description), $importance, $message, $recipients, $fqdnlink); - if (($entity & 4)==4 && !$mailtest) { if (is_array($agents)) {foreach ($agents as $agent) {exec("TIMESTAMP='$timestamp' EVENT=".escapeshellarg($event)." SUBJECT=".escapeshellarg(clean_subject($subject))." DESCRIPTION=".escapeshellarg($description)." IMPORTANCE=".escapeshellarg($importance)." CONTENT=".escapeshellarg($message)." LINK=".escapeshellarg($fqdnlink)." bash ".$agent);};}}; + if (($entity & 4)==4 && !$mailtest) { if (is_array($agents)) {foreach ($agents as $agent) {exec("TIMESTAMP='$timestamp' EVENT=".escapeshellarg($event)." SUBJECT=".escapeshellarg($cleanSubject)." DESCRIPTION=".escapeshellarg($description)." IMPORTANCE=".escapeshellarg($importance)." CONTENT=".escapeshellarg($message)." LINK=".escapeshellarg($fqdnlink)." bash ".$agent);};}};
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
emhttp/languages/en_US/helptext.txtemhttp/plugins/dynamix/Notifications.pageemhttp/plugins/dynamix/default.cfgemhttp/plugins/dynamix/include/Notify.phpemhttp/plugins/dynamix/include/Translations.phpemhttp/plugins/dynamix/scripts/notify
🧰 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/Translations.phpemhttp/plugins/dynamix/include/Notify.php
🔇 Additional comments (11)
emhttp/plugins/dynamix/default.cfg (1)
48-50: LGTM! New notification configuration defaults are well-chosen.The three new settings provide sensible defaults:
expand="true"enables stacking by default, which is user-friendlyduration="5000"(5 seconds) is a reasonable auto-close durationmax="3"prevents notification overload while still being informativeThese values align with the UI controls added in Notifications.page and are documented in the help text.
emhttp/plugins/dynamix/include/Translations.php (3)
76-78: Excellent cache invalidation fix for plugin translations.The addition of
clearstatcache()andfilemtime()comparison properly addresses the issue where translation caches were not regenerated when source files were updated. The logic is safe because thefile_exists($text)check on line 76 ensuresfilemtime($text)won't be called on a non-existent file.This pattern is consistently applied across all three translation loading locations (plugin, global, and per-URI), which ensures uniform behavior.
136-136: LGTM! Cache invalidation applied to global translations.Same cache validation improvement as in the plugin translation section, ensuring global translations are refreshed when the source file is newer than the cached version.
166-166: LGTM! Cache invalidation applied to per-URI translations.Consistent cache validation logic ensures per-URI translations (more.txt files) are also regenerated when source files are updated.
emhttp/languages/en_US/helptext.txt (1)
2161-2171: LGTM! Clear and concise help text for new notification features.The new help sections accurately describe the stack, duration, and max notification features. The explanations are user-friendly and provide the right level of technical detail (e.g., specifying milliseconds for duration).
emhttp/plugins/dynamix/Notifications.page (3)
115-122: LGTM! Flash storage control properly implemented.The "Store notifications to flash" control provides clear Yes/No options mapped to appropriate paths. The help text integration is correct.
129-130: LGTM! New center position options added.The addition of
bottom-centerandtop-centerdisplay positions is consistent with existing position options and expands user choice appropriately.
135-141: LGTM! Stack control properly implemented with sensible default.The stack notification popups control correctly uses the
expandconfiguration key with a default of "true" via the null coalescing operator, matching the default.cfg value.emhttp/plugins/dynamix/scripts/notify (3)
34-34: LGTM: Clear documentation for the new API option.The help text clearly indicates the purpose and scope of the
-uoption.
117-117: LGTM: Cleaner escaping implementation.Using
strtrwith an array is more efficient and readable than multiplestr_replacecalls. The escaping logic correctly handles backslashes and quotes.
215-217: LGTM: Proper option handling for custom filename.The
-uoption is correctly integrated into the getopt processing with appropriate initialization and assignment.Also applies to: 253-255
| case 'u': | ||
| $notify .= " -{$option} ".escapeshellarg($value); | ||
| break; | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check the notify script for validation of the -u parameter and safe_filename function
# Find the notify script
NOTIFY_SCRIPT="emhttp/plugins/dynamix/scripts/notify"
if [ -f "$NOTIFY_SCRIPT" ]; then
echo "=== Checking for -u option handling ==="
rg -A 10 -B 2 'u\)' "$NOTIFY_SCRIPT" | head -50
echo ""
echo "=== Checking for safe_filename function ==="
rg -A 20 'safe_filename|function.*filename' "$NOTIFY_SCRIPT"
echo ""
echo "=== Checking for filename validation patterns ==="
rg -C 3 'basename|realpath|sanitize|validate.*filename' "$NOTIFY_SCRIPT"
else
echo "Notify script not found at $NOTIFY_SCRIPT"
fiRepository: unraid/webgui
Length of output: 3165
🏁 Script executed:
#!/bin/bash
NOTIFY_SCRIPT="emhttp/plugins/dynamix/scripts/notify"
# Search for case statement handling options
echo "=== Searching for all case statement handling ==="
rg -B 5 -A 10 "case '-" "$NOTIFY_SCRIPT" | head -80
echo ""
echo "=== Full context around customFilename usage ==="
rg -B 10 -A 10 "customFilename" "$NOTIFY_SCRIPT"
echo ""
echo "=== Check if .notify extension is enforced for custom filenames ==="
rg -B 5 -A 5 '\.notify' "$NOTIFY_SCRIPT"Repository: unraid/webgui
Length of output: 2637
Extend -u parameter validation to enforce .notify extension.
The safe_filename() function in the notify script provides comprehensive protection against injection attacks and path traversal:
- Removes dangerous special characters and enforces alphanumeric, dots, dashes, and underscores
- Prevents path traversal by removing
/and\characters - Enforces reasonable length limits (maxLength parameter)
However, there is a validation gap: the custom filename passed via the -u option is not required to end with .notify. The default notification path enforces this via suffix construction (_{$ticket}.notify), but custom filenames bypass this requirement. Since the notification retrieval logic expects *.notify files (glob("$unread/*.notify", GLOB_NOSORT)), custom filenames without the .notify extension will not be retrievable by the notification system.
Add extension validation for custom filenames to either append or enforce the .notify extension.
🤖 Prompt for AI Agents
In emhttp/plugins/dynamix/include/Notify.php around lines 39 to 42, the -u
option accepts a custom filename but does not enforce the .notify extension so
such files won't be picked up by the notification glob; after sanitizing the
provided filename with safe_filename(), ensure it ends with ".notify"
(case-insensitive) and if not, append ".notify" to the sanitized name, then use
that resulting filename when building $notify (i.e. escapeshellarg the
validated/normalized filename) so custom names are always retrievable by the
notification system.
| _(Max notification popups)_: | ||
| : <input type="number" name="max" value="<?=$notify['max'] ?? 3?>" min="1" max="10"> | ||
|
|
||
| :notifications_max_help: |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the Notifications.page file and check for server-side validation
echo "=== Checking file structure ==="
# First, find the Notifications.page file
find . -name "Notifications.page" -type f
echo ""
echo "=== Checking lines 148-151 in Notifications.page ==="
# Read the specific lines mentioned in the review
sed -n '145,155p' emhttp/plugins/dynamix/Notifications.page
echo ""
echo "=== Searching for max popups validation in backend ==="
# Search for validation logic related to max popups
rg -i 'max.*notif|notify.*max' emhttp/plugins/dynamix/include/ -A 3 -B 1
echo ""
echo "=== Searching for validation patterns in notification handling ==="
# Look for any validation patterns in the notification files
rg '\$notify\[.*max|validate.*max|bounds.*max' emhttp/plugins/dynamix/ -A 2
echo ""
echo "=== Checking for POST/input validation in backend ==="
# Search for where the notification settings are processed/validated
rg -i 'post.*notif|input.*notif' emhttp/plugins/dynamix/include/ -l | head -10Repository: unraid/webgui
Length of output: 50371
🏁 Script executed:
#!/bin/bash
# More targeted search for notification settings validation
echo "=== Looking for notification settings handler files ==="
find emhttp/plugins/dynamix/include/ -type f -name "*.php" -o -name "*.sh" | head -20
echo ""
echo "=== Searching for where notify array is processed ==="
rg '\$notify.*=.*\$_' emhttp/plugins/dynamix/include/ -A 2
echo ""
echo "=== Searching for notification settings save/update logic ==="
rg -l 'notify.*POST|POST.*notify' emhttp/plugins/dynamix/
echo ""
echo "=== Checking the main notification handler ==="
rg -l 'notifications' emhttp/plugins/dynamix/include/ | head -5Repository: unraid/webgui
Length of output: 1762
🏁 Script executed:
#!/bin/bash
# Check the full Notifications.page file structure
echo "=== File size and line count ==="
wc -l emhttp/plugins/dynamix/Notifications.page
echo ""
echo "=== Checking for form submission handlers ==="
rg -i 'submit|POST|save.*notif' emhttp/plugins/dynamix/Notifications.page -B 2 -A 2
echo ""
echo "=== Checking for JavaScript validation ==="
rg -i 'validate.*max|max.*validate|function.*notif' emhttp/plugins/dynamix/Notifications.page
echo ""
echo "=== Checking entire file for validation logic ==="
# Read the full file to look for any server-side validation
cat emhttp/plugins/dynamix/Notifications.page | head -100
echo ""
echo "=== Search for where the form is saved/submitted ==="
rg 'apply|update' emhttp/plugins/dynamix/Notifications.page -i -B 1 -A 1Repository: unraid/webgui
Length of output: 7884
🏁 Script executed:
#!/bin/bash
# Find and examine the update handler
echo "=== Looking for update.php ==="
find . -name "update.php" -type f | grep -v node_modules | grep -v ".js"
echo ""
echo "=== Checking update.php for validation logic ==="
if [ -f "emhttp/update.php" ]; then
wc -l emhttp/update.php
echo "---"
# Look for validation patterns
rg -i 'validate|sanitize|intval|bounds|range|min.*max' emhttp/update.php | head -20
fi
echo ""
echo "=== Searching for numeric field validation ==="
rg -i 'intval|is_numeric|\[0-9\]' emhttp/update.php | head -10
echo ""
echo "=== Check for specific max/min validation ==="
rg -C 3 '\$max|\bmax\b' emhttp/update.php | head -30Repository: unraid/webgui
Length of output: 278
🏁 Script executed:
#!/bin/bash
# Read the full update.php file
echo "=== Full update.php content ==="
cat -n emhttp/update.php
echo ""
echo "=== Check if there's any validation in dynamix.cfg parsing ==="
rg -i 'notify|max' emhttp/plugins/dynamix/ --include="*.php" | grep -i "validate\|sanitize\|intval\|min\|max" | head -20Repository: unraid/webgui
Length of output: 7555
🏁 Script executed:
#!/bin/bash
# Check for validation logic related to notifications
echo "=== Searching for notification-related includes or validation ==="
rg 'notify' emhttp/plugins/dynamix/ -t php | grep -i "valid\|sanitize\|intval\|bounds\|include" | head -20
echo ""
echo "=== Check if update.php uses an include for notify section ==="
grep -r "#include" emhttp/plugins/dynamix/Notifications.page
echo ""
echo "=== Look for dynamix.cfg default values ==="
find emhttp/plugins/dynamix -name "default.cfg" -o -name "dynamix.cfg" | xargs cat 2>/dev/null | head -40
echo ""
echo "=== Check for notify validation in wrapper functions ==="
rg 'function.*notify\|notify.*valid' emhttp/plugins/dynamix/include/ -t phpRepository: unraid/webgui
Length of output: 2791
🏁 Script executed:
#!/bin/bash
# Find and examine the notify script
echo "=== Looking for notify script ==="
find . -path "*/webGui/scripts/notify" -type f
echo ""
echo "=== Check for max validation in notify script ==="
if [ -f "emhttp/webGui/scripts/notify" ]; then
file emhttp/webGui/scripts/notify
echo "---"
rg -i 'max.*popup|max.*notif|validate.*max' emhttp/webGui/scripts/notify | head -10
fi
echo ""
echo "=== Look for the full notify section in default.cfg ==="
find emhttp/plugins/dynamix -name "default.cfg" -exec grep -A 20 "\[notify\]" {} \;
echo ""
echo "=== Search for any bounds checking on numeric values ==="
rg -C 3 '\$notify\[.max.\]' emhttp/ -t phpRepository: unraid/webgui
Length of output: 615
Add server-side validation to enforce max popup bounds.
The max notification popups field has client-side HTML5 constraints (min=1, max=10) but update.php writes form values directly to the configuration file without any bounds checking. Add validation to reject values outside the 1-10 range before saving to prevent invalid configurations from direct POST requests or manual config editing.
🤖 Prompt for AI Agents
In emhttp/plugins/dynamix/Notifications.page around lines 148-151, the "max"
input value is only constrained client-side; add server-side validation when
processing the form (e.g., in update.php or the handler invoked on POST) to
enforce an integer between 1 and 10. Specifically, retrieve the posted value,
cast to int, verify it is >=1 and <=10, and if not, reject the request (do not
write the invalid value to the config) and return a clear error message to the
user; if the value is missing or invalid type, treat it as an error as well.
Ensure you do not silently override invalid values—fail validation and prompt
correction before saving the configuration.
| if ($customFilename) { | ||
| $filename = safe_filename($customFilename); | ||
| } else { | ||
| // suffix length: _{timestamp}.notify = 1+10+7 = 18 chars. | ||
| $suffix = "_{$ticket}.notify"; | ||
| $max_name_len = 255 - strlen($suffix); | ||
| // sanitize event, truncating it to leave room for suffix | ||
| $clean_name = safe_filename($event, $max_name_len); | ||
| // construct filename with suffix (underscore separator matches safe_filename behavior) | ||
| $filename = "{$clean_name}{$suffix}"; | ||
| } | ||
|
|
||
| $unread = "{$unread}/{$filename}"; | ||
| $archive = "{$archive}/{$filename}"; | ||
| if (file_exists($archive)) break; |
There was a problem hiding this comment.
Custom filenames lack the .notify extension required for notification discovery.
When a custom filename is provided (line 260), it's sanitized without enforcing the .notify extension. However, the get operation uses glob("$unread/*.notify", GLOB_NOSORT) to discover notifications. Custom filenames without this extension won't be found by the notification retrieval system, creating an inconsistency between custom and auto-generated notification filenames.
The auto-generated path (lines 264-268) explicitly includes the .notify extension as a suffix, while the custom filename path does not. This means a notification created with a custom filename would be stored but unretrievable via the get operation.
Recommend either:
- Appending
.notifyto custom filenames, or - Documenting that custom filenames must explicitly include the
.notifyextension
Regarding the file_exists($archive) check (line 273): This appears to be an intentional loop exit and is not an issue.
🤖 Prompt for AI Agents
In emhttp/plugins/dynamix/scripts/notify around lines 259-273, the branch that
handles $customFilename currently sanitizes the name but does not ensure it ends
with the required ".notify" extension, so custom-named notifications won't be
discovered by glob("$unread/*.notify"). After calling
safe_filename($customFilename) ensure the returned name ends with ".notify"
(append ".notify" if missing) before assigning to $filename; alternatively
sanitize the base name and then append the ".notify" suffix to guarantee
consistent discovery by the existing get logic.
1 participant
Summary
This PR enhances the Notification Settings page with new customization options for notification toasts/popups and resolves a core caching bug that prevented help text updates from being displayed.
Key Changes
Notifications.page:default.cfgto reflect these changes.Translations.php):.dot) existed. It ignored updates to the source.txtfiles, meaning new help text strings were never loaded until the cache was manually cleared or the server restarted.filemtime(modification time). The cache is now automatically regenerated if the source text file is newer than the cache file.Added corresponding help text descriptions in
helptext.txtfor the new notification controls (stacking, display position, duration, etc.).Please Note
These changes are not ready to be merged yet. Please coordinate with the API team. @elibosley and @zackspear are aware of both the API and Webgui changes.
Related PRs
api:
- PR (#1860)
Summary by CodeRabbit
Release Notes
New Features
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.