feat: hardware sensor probe temp source + stable USB hwmon path resolution#3
Open
mrjacobarussell wants to merge 19 commits into
Open
feat: hardware sensor probe temp source + stable USB hwmon path resolution#3mrjacobarussell wants to merge 19 commits into
mrjacobarussell wants to merge 19 commits into
Conversation
- autofan: add -s <sensor_path> flag to read temp from hwmon probe instead of scanning drives via smartctl - rc.autofan: resolve_hwmon_in_string() finds current hwmonN dir by kernel device name at start/stop time, fixing USB controllers that get a different hwmon number each reboot - FanSettings.page: add Temperature source dropdown (HDD / sensor), sensor picker populated via AJAX, and Stable device name field - SystemFan.php: add list_temp() + op=temps endpoint for sensor picker - update.autofan.php: skip temp_source/hwmon_name from options string; temp_sensor maps to -s flag via prefix - default.cfg: add temp_source/temp_sensor/hwmon_name defaults Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds hwmon temperature-sensor support and stable hwmon-name resolution across UI, PHP API, runtime scripts, and rc wiring; introduces autopair PWM↔fan pairing, drive include/exclude mode, new config keys, updated option assembly, and plugin/README metadata for the Enhanced Fork. ChangesHardware Sensor Support and Stable Device Naming
Sequence Diagram(s)sequenceDiagram
participant FanUI as FanSettings.page
participant API as SystemFan.php
participant RC as rc.autofan
participant Autofan as autofan
participant Sysfs as /sys/devices
FanUI->>API: POST op=autopair
API->>RC: stop rc.autofan
API->>Sysfs: scan pwm* and fan*_input per hwmon chip
loop per pwm
API->>Autofan: set pwm value
Autofan->>Sysfs: sample fan*_input rpm
Sysfs-->>Autofan: rpm sample
Autofan-->>API: rpm delta
end
API->>RC: start rc.autofan
API-->>FanUI: return JSON pairs
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
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 |
Cross-chip RPM fluctuation caused detect to return a motherboard fan when a USB controller (Corsair Commander Pro) PWM was selected. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
unRAIDv6/dynamix.system.autofan.plg (2)
8-8: ⚡ Quick winConsider using a tagged release for the pluginURL.
The
pluginURLpoints to a specific branch (corsair-commander-pro) rather than a tagged release ormaster. Branch-based URLs are mutable and can change or be deleted, which reduces installation stability and reproducibility.For production deployments, consider:
- Creating a tagged release (e.g.,
v2025.06.09) and updating the URL to point to that tag- Or, if this is intentional for a development fork, document this in the plugin metadata or comments
🤖 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 `@unRAIDv6/dynamix.system.autofan.plg` at line 8, The pluginURL ENTITY currently points to a branch URL ("corsair-commander-pro") which is mutable; update the <!ENTITY pluginURL ...> value to reference a specific git tag/release (e.g., replace "corsair-commander-pro" with a release tag like "vYYYY.MM.DD" or a commit SHA) to ensure reproducible installs, or if you intend to track the branch, add a comment or metadata near the pluginURL explaining this is a development branch and may change.
16-23: ⚡ Quick winClarify the version timeline in the changelog.
The changelog lists
2025.06.09before2025.11.14 (upstream), which creates confusion:
- Date-wise, June comes before November, suggesting 2025.06.09 is older
- But the placement suggests 2025.06.09 is the "newer" enhanced fork entry
Consider adding clarifying text such as:
- If this fork diverged from 2025.11.14 upstream: "Forked from upstream 2025.11.14"
- If these are parallel timelines: Explain the relationship between the fork and upstream versions
- Or adjust the versioning to avoid date-based comparison issues (see earlier comment about version numbers)
This will help users understand the lineage and which version to expect.
🤖 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 `@unRAIDv6/dynamix.system.autofan.plg` around lines 16 - 23, Update the changelog header block under "##Dynamix System Autofan (Enhanced Fork)" to clarify lineage between the two entries: explicitly state whether "2025.06.09" is the enhanced fork (e.g., "2025.06.09 — Enhanced fork, changes since upstream") and whether "2025.11.14 (upstream)" is the upstream baseline (e.g., "Forked from upstream 2025.11.14"), or reorder the entries so chronological order is clear; modify the text lines containing "2025.06.09" and "2025.11.14 (upstream)" to include a short clarifying phrase such as "Forked from upstream" or "Enhanced fork" to remove ambiguity.source/system-autofan/include/SystemFan.php (1)
36-56: ⚡ Quick winConsider refactoring
end(explode(...))for PHP 7.4+ compatibility.Lines 44 and 48 use
end(explode('/', ...))which passes a temporary value toend(). While functional, this generates strict standards warnings in PHP 7.4+ sinceend()expects a reference.♻️ Proposed refactor
- $label = is_file($label_file) ? trim(file_get_contents($label_file)) : end(explode('/',$temp)); + $label = is_file($label_file) ? trim(file_get_contents($label_file)) : basename($temp);- 'name' => end(explode('/',$temp)), + 'name' => basename($temp),This also applies to line 32 in
list_fan():- if ($name) foreach (preg_grep("/fan\d+_input/", scan_dir($chip)) as $fan) $out[] = ['chip'=>trim($name), 'name'=>end(explode('/',$fan)), 'sensor'=>$fan , 'rpm'=>intval(file_get_contents($fan))]; + if ($name) foreach (preg_grep("/fan\d+_input/", scan_dir($chip)) as $fan) $out[] = ['chip'=>trim($name), 'name'=>basename($fan), 'sensor'=>$fan , 'rpm'=>intval(file_get_contents($fan))];🤖 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 `@source/system-autofan/include/SystemFan.php` around lines 36 - 56, Refactor occurrences of end(explode('/', ...)) in list_temp (and list_fan) to avoid passing a temporary to end(): split the string into a variable first (e.g. $parts = explode('/', $temp) or $parts = explode('/', $somePath)) then use end($parts) or array_pop($parts) to get the last element, or simply use basename($temp) where appropriate; update the 'name' and 'label' assignments in function list_temp (and the analogous place in list_fan) to use the temporary array or basename instead of end(explode(...)).
🤖 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 `@README.md`:
- Line 1: Update README.md to remove the "archived" message and instead document
this repo as an enhanced active fork of the upstream Dynamix project: mention
it's an enhanced fork of https://github.com/unraid/dynamix, summarize the
enhanced features (hardware sensor probe temperature sources, stable hwmon
naming for USB controllers, UI improvements), add installation instructions for
this fork, and include an "Upstream" link to https://github.com/unraid/dynamix;
also ensure the README's installation/link text matches the .plg file reference
to mrjacobarussell/dynamix so users aren't confused by inconsistent install
sources.
In `@source/system-autofan/FanSettings.page`:
- Around line 122-139: The dropdown construction in loadTempSensors concatenates
item.chip, item.label, item.temp and item.sensor directly into HTML; to fix,
build options using safe DOM APIs instead of string concatenation (e.g., create
option elements with jQuery: $('<option>') and set .val(...) and .text(...) and
.prop('selected', ...) or otherwise HTML-escape values) for the selector
'`#temp_sensor_select`' so that item.chip, item.label, item.temp and item.sensor
are not injected raw; keep the existing logic that sets
$('input[name=temp_sensor]').val(sel.val() || '') and the change handler intact.
In `@source/system-autofan/scripts/autofan`:
- Around line 221-231: In function_get_sensor_temp, validate TEMP_SENSOR before
reading: ensure the path is absolute and resides under /sys (e.g., starts with
/sys/), confirm it is a regular file and readable (and optionally not a symlink
or resolves to a path under /sys via realpath), and only then cat it; if
validation fails, set HIGHEST_TEMP=0 and log the same warning via program_name.
Reference TEMP_SENSOR, HIGHEST_TEMP, program_name and function_get_sensor_temp
when adding these checks.
In `@source/system-autofan/scripts/rc.autofan`:
- Around line 31-39: In resolve_hwmon(), grep is currently invoked with a regex
match (grep -rlx "$name") which can misinterpret device names containing regex
metacharacters; change the grep invocation to use fixed-string matching (add the
-F flag) so it treats "$name" literally (e.g., use grep with -F -r -l -x) to
reliably locate the matching /sys/class/hwmon/*/name file.
- Around line 87-95: In autofan.speed(), guard reads of $fan_path by checking
the file exists (and/or is readable) before using cat; update the conditional
that currently uses [[ -f $rc && $(cat $fan_path) -gt 0 ]] to first test [[ -f
$rc && -f $fan_path ]] (or -r for readability), then only perform the numeric
cat on $fan_path when that check passes, otherwise fall back to echoing "0";
ensure you reference the autofan.speed function and the variables rc and
fan_path so the change is applied to the correct conditional.
In `@unRAIDv6/dynamix.system.autofan.plg`:
- Line 6: The version entity value was lowered from "2025.11.14" to
"2025.06.09", which will cause downgrade/rejection issues; update the <!ENTITY
version ...> value in dynamix.system.autofan.plg to preserve monotonic ordering
by either bumping from the upstream base (e.g., change to "2025.11.14.1") or
append a fork suffix to the actual date (e.g., "2025.06.09-fork.1"), so the XML
entity named version reflects a higher/ordered version string that plugin
managers will accept.
---
Nitpick comments:
In `@source/system-autofan/include/SystemFan.php`:
- Around line 36-56: Refactor occurrences of end(explode('/', ...)) in list_temp
(and list_fan) to avoid passing a temporary to end(): split the string into a
variable first (e.g. $parts = explode('/', $temp) or $parts = explode('/',
$somePath)) then use end($parts) or array_pop($parts) to get the last element,
or simply use basename($temp) where appropriate; update the 'name' and 'label'
assignments in function list_temp (and the analogous place in list_fan) to use
the temporary array or basename instead of end(explode(...)).
In `@unRAIDv6/dynamix.system.autofan.plg`:
- Line 8: The pluginURL ENTITY currently points to a branch URL
("corsair-commander-pro") which is mutable; update the <!ENTITY pluginURL ...>
value to reference a specific git tag/release (e.g., replace
"corsair-commander-pro" with a release tag like "vYYYY.MM.DD" or a commit SHA)
to ensure reproducible installs, or if you intend to track the branch, add a
comment or metadata near the pluginURL explaining this is a development branch
and may change.
- Around line 16-23: Update the changelog header block under "##Dynamix System
Autofan (Enhanced Fork)" to clarify lineage between the two entries: explicitly
state whether "2025.06.09" is the enhanced fork (e.g., "2025.06.09 — Enhanced
fork, changes since upstream") and whether "2025.11.14 (upstream)" is the
upstream baseline (e.g., "Forked from upstream 2025.11.14"), or reorder the
entries so chronological order is clear; modify the text lines containing
"2025.06.09" and "2025.11.14 (upstream)" to include a short clarifying phrase
such as "Forked from upstream" or "Enhanced fork" to remove ambiguity.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fa820481-4b98-4cd2-b6cd-fa471f6f1159
⛔ Files ignored due to path filters (1)
source/system-autofan/icons/dynamix.system.autofan.pngis excluded by!**/*.png
📒 Files selected for processing (9)
README.mdarchive/dynamix.system.autofan.txzsource/system-autofan/FanSettings.pagesource/system-autofan/default.cfgsource/system-autofan/include/SystemFan.phpsource/system-autofan/include/update.autofan.phpsource/system-autofan/scripts/autofansource/system-autofan/scripts/rc.autofanunRAIDv6/dynamix.system.autofan.plg
Comment on lines
+221
to
+231
| function_get_sensor_temp() { | ||
| # Read temperature from hwmon sensor file (values are in millidegrees Celsius) | ||
| local raw | ||
| raw=$(cat "$TEMP_SENSOR" 2>/dev/null) | ||
| if [[ -n $raw ]]; then | ||
| HIGHEST_TEMP=$((raw/1000)) | ||
| else | ||
| HIGHEST_TEMP=0 | ||
| echo "$program_name: warning: could not read temp sensor $TEMP_SENSOR" | logger -t$program_name | ||
| fi | ||
| } |
There was a problem hiding this comment.
Add path validation for TEMP_SENSOR to prevent potential path traversal.
The function reads from $TEMP_SENSOR without validating that the path is under /sys. While the configuration file is root-controlled, defense-in-depth principles suggest validating that sensor paths are legitimate hwmon sensor files.
🛡️ Proposed validation
function_get_sensor_temp() {
# Read temperature from hwmon sensor file (values are in millidegrees Celsius)
+ # Validate that sensor path is under /sys/devices
+ case "$TEMP_SENSOR" in
+ /sys/devices/*) ;;
+ *) echo "$program_name: error: sensor path must be under /sys/devices" | logger -t$program_name; HIGHEST_TEMP=0; return ;;
+ esac
local raw
raw=$(cat "$TEMP_SENSOR" 2>/dev/null)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| function_get_sensor_temp() { | |
| # Read temperature from hwmon sensor file (values are in millidegrees Celsius) | |
| local raw | |
| raw=$(cat "$TEMP_SENSOR" 2>/dev/null) | |
| if [[ -n $raw ]]; then | |
| HIGHEST_TEMP=$((raw/1000)) | |
| else | |
| HIGHEST_TEMP=0 | |
| echo "$program_name: warning: could not read temp sensor $TEMP_SENSOR" | logger -t$program_name | |
| fi | |
| } | |
| function_get_sensor_temp() { | |
| # Read temperature from hwmon sensor file (values are in millidegrees Celsius) | |
| # Validate that sensor path is under /sys/devices | |
| case "$TEMP_SENSOR" in | |
| /sys/devices/*) ;; | |
| *) echo "$program_name: error: sensor path must be under /sys/devices" | logger -t$program_name; HIGHEST_TEMP=0; return ;; | |
| esac | |
| local raw | |
| raw=$(cat "$TEMP_SENSOR" 2>/dev/null) | |
| if [[ -n $raw ]]; then | |
| HIGHEST_TEMP=$((raw/1000)) | |
| else | |
| HIGHEST_TEMP=0 | |
| echo "$program_name: warning: could not read temp sensor $TEMP_SENSOR" | logger -t$program_name | |
| fi | |
| } |
🤖 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 `@source/system-autofan/scripts/autofan` around lines 221 - 231, In
function_get_sensor_temp, validate TEMP_SENSOR before reading: ensure the path
is absolute and resides under /sys (e.g., starts with /sys/), confirm it is a
regular file and readable (and optionally not a symlink or resolves to a path
under /sys via realpath), and only then cat it; if validation fails, set
HIGHEST_TEMP=0 and log the same warning via program_name. Reference TEMP_SENSOR,
HIGHEST_TEMP, program_name and function_get_sensor_temp when adding these
checks.
Comment on lines
+31
to
+39
| # Find the current hwmon directory for a device by its kernel name. | ||
| # hwmon numbers (hwmon0, hwmon1, ...) can change each reboot for USB devices. | ||
| # If hwmon_name is set in cfg, this resolves the correct current path. | ||
| resolve_hwmon() { | ||
| local name="$1" | ||
| local match | ||
| match=$(grep -rlx "$name" /sys/class/hwmon/*/name 2>/dev/null | head -1) | ||
| [[ -n $match ]] && dirname "$match" | ||
| } |
There was a problem hiding this comment.
Use grep -F for literal string matching to prevent regex interpretation.
Line 37 uses grep -rlx "$name" which interprets $name as a regex pattern. If the hwmon device name contains regex metacharacters (e.g., corsair.cpro with a dot), it could match unintended files.
🔧 Proposed fix
resolve_hwmon() {
local name="$1"
local match
- match=$(grep -rlx "$name" /sys/class/hwmon/*/name 2>/dev/null | head -1)
+ match=$(grep -rFx "$name" /sys/class/hwmon/*/name 2>/dev/null | head -1)
[[ -n $match ]] && dirname "$match"
}The -F flag treats the pattern as a fixed string rather than a regex.
🤖 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 `@source/system-autofan/scripts/rc.autofan` around lines 31 - 39, In
resolve_hwmon(), grep is currently invoked with a regex match (grep -rlx
"$name") which can misinterpret device names containing regex metacharacters;
change the grep invocation to use fixed-string matching (add the -F flag) so it
treats "$name" literally (e.g., use grep with -F -r -l -x) to reliably locate
the matching /sys/class/hwmon/*/name file.
Comment on lines
87
to
95
| autofan.speed() { | ||
| for cfg in $cfg_path/$plugin*.cfg; do | ||
| source $cfg | ||
| [[ -f $controller && $(cat $fan) -gt 0 ]] && echo -n "$(($(($(cat $controller)*100))/255)) " || echo -n "0 " | ||
| local rc fan_path | ||
| rc=$(resolve_hwmon_in_string "$controller" "$hwmon_name" "$controller") | ||
| fan_path=$(resolve_hwmon_in_string "$fan" "$hwmon_name" "$controller") | ||
| [[ -f $rc && $(cat $fan_path) -gt 0 ]] && echo -n "$(($(($(cat $rc)*100))/255)) " || echo -n "0 " | ||
| done | ||
| } |
There was a problem hiding this comment.
Add file existence check for $fan_path before reading.
Line 93 reads from $fan_path without checking if the file exists. If the resolved fan path is invalid or the device has been unplugged, cat $fan_path will fail and produce an error.
🔧 Proposed fix
autofan.speed() {
for cfg in $cfg_path/$plugin*.cfg; do
source $cfg
local rc fan_path
rc=$(resolve_hwmon_in_string "$controller" "$hwmon_name" "$controller")
fan_path=$(resolve_hwmon_in_string "$fan" "$hwmon_name" "$controller")
- [[ -f $rc && $(cat $fan_path) -gt 0 ]] && echo -n "$(($(($(cat $rc)*100))/255)) " || echo -n "0 "
+ [[ -f $rc && -f $fan_path && $(cat $fan_path) -gt 0 ]] && echo -n "$(($(($(cat $rc)*100))/255)) " || echo -n "0 "
done
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| autofan.speed() { | |
| for cfg in $cfg_path/$plugin*.cfg; do | |
| source $cfg | |
| [[ -f $controller && $(cat $fan) -gt 0 ]] && echo -n "$(($(($(cat $controller)*100))/255)) " || echo -n "0 " | |
| local rc fan_path | |
| rc=$(resolve_hwmon_in_string "$controller" "$hwmon_name" "$controller") | |
| fan_path=$(resolve_hwmon_in_string "$fan" "$hwmon_name" "$controller") | |
| [[ -f $rc && $(cat $fan_path) -gt 0 ]] && echo -n "$(($(($(cat $rc)*100))/255)) " || echo -n "0 " | |
| done | |
| } | |
| autofan.speed() { | |
| for cfg in $cfg_path/$plugin*.cfg; do | |
| source $cfg | |
| local rc fan_path | |
| rc=$(resolve_hwmon_in_string "$controller" "$hwmon_name" "$controller") | |
| fan_path=$(resolve_hwmon_in_string "$fan" "$hwmon_name" "$controller") | |
| [[ -f $rc && -f $fan_path && $(cat $fan_path) -gt 0 ]] && echo -n "$(($(($(cat $rc)*100))/255)) " || echo -n "0 " | |
| done | |
| } |
🤖 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 `@source/system-autofan/scripts/rc.autofan` around lines 87 - 95, In
autofan.speed(), guard reads of $fan_path by checking the file exists (and/or is
readable) before using cat; update the conditional that currently uses [[ -f $rc
&& $(cat $fan_path) -gt 0 ]] to first test [[ -f $rc && -f $fan_path ]] (or -r
for readability), then only perform the numeric cat on $fan_path when that check
passes, otherwise fall back to echoing "0"; ensure you reference the
autofan.speed function and the variables rc and fan_path so the change is
applied to the correct conditional.
| <!ENTITY author "Bergware/gfjardim"> | ||
| <!ENTITY version "2025.11.14"> | ||
| <!ENTITY author "Bergware/gfjardim/mrjacobarussell"> | ||
| <!ENTITY version "2025.06.09"> |
There was a problem hiding this comment.
Critical: Version number appears to downgrade.
The version changes from 2025.11.14 to 2025.06.09, which represents a backward movement in date-based versioning. Plugin management systems typically compare version numbers lexicographically or numerically to determine update eligibility. A lower version number will:
- Prevent automatic updates on systems running
2025.11.14(if it exists) - Potentially be rejected by plugin installers expecting monotonically increasing versions
- Confuse users about which version is newer
If 2025.06.09 represents the actual release date of this enhanced fork and 2025.11.14 was the upstream version, consider using a different versioning scheme that maintains ordering, such as 2025.06.09-fork.1 or 2025.11.14.1 (incrementing from the upstream base).
🤖 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 `@unRAIDv6/dynamix.system.autofan.plg` at line 6, The version entity value was
lowered from "2025.11.14" to "2025.06.09", which will cause downgrade/rejection
issues; update the <!ENTITY version ...> value in dynamix.system.autofan.plg to
preserve monotonic ordering by either bumping from the upstream base (e.g.,
change to "2025.11.14.1") or append a fork suffix to the actual date (e.g.,
"2025.06.09-fork.1"), so the XML entity named version reflects a higher/ordered
version string that plugin managers will accept.
- autofan: add -I <drives> flag for include-only mode - FanSettings.page: Drive filter mode dropdown (Exclude/Include) with dynamic label update - update.autofan.php: build -e or -I flag based on filter_mode - default.cfg: add filter_mode="exclude" default Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
source/system-autofan/scripts/autofan (1)
227-245:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSensor-read failure currently collapses to 0°C instead of falling back to disk temperature.
If the sensor file exists but returns unreadable/non-numeric content,
HIGHEST_TEMPbecomes0, which can incorrectly reduce fan speed. Also, log labels useTEMP_SENSORpresence instead of actual source used. Make sensor read return a status and fallback tofunction_get_highest_hd_tempon failure, while tracking the real source for logs.💡 Suggested patch
function_get_sensor_temp() { # Read temperature from hwmon sensor file (values are in millidegrees Celsius) local raw raw=$(cat "$TEMP_SENSOR" 2>/dev/null) - if [[ -n $raw ]]; then - HIGHEST_TEMP=$((raw/1000)) - else - HIGHEST_TEMP=0 - echo "$program_name: warning: could not read temp sensor $TEMP_SENSOR" | logger -t$program_name - fi + if [[ $raw =~ ^[0-9]+$ ]]; then + HIGHEST_TEMP=$((raw/1000)) + return 0 + fi + echo "$program_name: warning: could not read temp sensor $TEMP_SENSOR; falling back to disk temps" | logger -t$program_name + return 1 } function_get_temp() { + TEMP_SOURCE_LABEL="disk" if [[ -n $TEMP_SENSOR && -f $TEMP_SENSOR ]]; then - function_get_sensor_temp - else - function_get_highest_hd_temp + if function_get_sensor_temp; then + TEMP_SOURCE_LABEL="sensor (${TEMP_SENSOR##*/})" + return + fi fi + function_get_highest_hd_temp } @@ - if [[ -n $TEMP_SENSOR ]]; then - TEMP_SOURCE_LABEL="sensor (${TEMP_SENSOR##*/})" - else - TEMP_SOURCE_LABEL="disk" - fi echo "$program_name: Highest $TEMP_SOURCE_LABEL temp is ${REPORTED_TEMP}${TEMP_UNIT}, adjusting fan speed from: $CURRENT_OUTPUT to: $ADJUSTED_OUTPUT" echo "Highest $TEMP_SOURCE_LABEL temp is ${REPORTED_TEMP}${TEMP_UNIT}, adjusting fan speed from: $CURRENT_OUTPUT to: $ADJUSTED_OUTPUT" | logger -t$program_name @@ -if [[ -n $TEMP_SENSOR ]]; then -echo "$program_name using sensor probe $TEMP_SENSOR for temperature source" | logger -t$program_name +if [[ -n $TEMP_SENSOR ]]; then +echo "$program_name sensor probe configured: $TEMP_SENSOR" | logger -t$program_name fiAlso applies to: 314-318, 377-379
🤖 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 `@source/system-autofan/scripts/autofan` around lines 227 - 245, function_get_sensor_temp should not silently set HIGHEST_TEMP=0 on unreadable/non-numeric sensor contents; instead make it return a failure status and avoid modifying HIGHEST_TEMP so caller can fall back to disk temp. Update function_get_sensor_temp to validate that raw is numeric (after trimming), set HIGHEST_TEMP only when valid, and return 0 on success / non-zero on failure; update function_get_temp to call function_get_sensor_temp and on non-zero return call function_get_highest_hd_temp to populate HIGHEST_TEMP. Also change log messages to reference the actual source used (TEMP_SENSOR vs disk) and include program_name in the log. Reference symbols: TEMP_SENSOR, HIGHEST_TEMP, function_get_sensor_temp, function_get_temp, function_get_highest_hd_temp, program_name.source/system-autofan/include/SystemFan.php (1)
29-29:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix find patterns to match multi-digit sensor numbers.
Both
list_fan()(line 29) andlist_temp()(line 38) usefindpatterns that only match single-digit sensor numbers:fan[0-9]_inputandtemp[0-9]_input. These patterns will missfan10_input,temp10_input, and higher-numbered sensors. However, the subsequentpreg_grepfilters on lines 32 and 42 use\d+, which does match multi-digit numbers.The result: if a hwmon chip has only
temp10_input(orfan10_input) with no lower-numbered sensors, thefindcommand will not include that chip's directory in$chips, and those sensors will never appear in the UI dropdown.🔧 Proposed fix
- exec("find /sys/devices -type f -iname 'fan[0-9]_input' -exec dirname \"{}\" +|uniq", $chips); + exec("find /sys/devices -type f -iname 'fan[0-9]*_input' -exec dirname \"{}\" +|uniq", $chips);- exec("find /sys/devices -type f -name 'temp[0-9]_input' -exec dirname \"{}\" +|uniq", $chips); + exec("find /sys/devices -type f -name 'temp[0-9]*_input' -exec dirname \"{}\" +|uniq", $chips);Also applies to: 38-38
🤖 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 `@source/system-autofan/include/SystemFan.php` at line 29, The find patterns in list_fan() and list_temp() only match single-digit sensors; update the exec() calls that search for "fan[0-9]_input" and "temp[0-9]_input" so they match multi-digit sensor names (e.g. use glob patterns like "fan[0-9][0-9]*_input" and "temp[0-9][0-9]*_input"); change the pattern passed to exec() in the list_fan and list_temp functions so chips containing only fan10_input/temp10_input are included and the existing preg_grep(\d+) filtering continues to work as before.
🤖 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.
Outside diff comments:
In `@source/system-autofan/include/SystemFan.php`:
- Line 29: The find patterns in list_fan() and list_temp() only match
single-digit sensors; update the exec() calls that search for "fan[0-9]_input"
and "temp[0-9]_input" so they match multi-digit sensor names (e.g. use glob
patterns like "fan[0-9][0-9]*_input" and "temp[0-9][0-9]*_input"); change the
pattern passed to exec() in the list_fan and list_temp functions so chips
containing only fan10_input/temp10_input are included and the existing
preg_grep(\d+) filtering continues to work as before.
In `@source/system-autofan/scripts/autofan`:
- Around line 227-245: function_get_sensor_temp should not silently set
HIGHEST_TEMP=0 on unreadable/non-numeric sensor contents; instead make it return
a failure status and avoid modifying HIGHEST_TEMP so caller can fall back to
disk temp. Update function_get_sensor_temp to validate that raw is numeric
(after trimming), set HIGHEST_TEMP only when valid, and return 0 on success /
non-zero on failure; update function_get_temp to call function_get_sensor_temp
and on non-zero return call function_get_highest_hd_temp to populate
HIGHEST_TEMP. Also change log messages to reference the actual source used
(TEMP_SENSOR vs disk) and include program_name in the log. Reference symbols:
TEMP_SENSOR, HIGHEST_TEMP, function_get_sensor_temp, function_get_temp,
function_get_highest_hd_temp, program_name.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4d01909e-cf46-4001-8ab8-0937c847c582
📒 Files selected for processing (7)
archive/dynamix.system.autofan.txzsource/system-autofan/FanSettings.pagesource/system-autofan/default.cfgsource/system-autofan/include/SystemFan.phpsource/system-autofan/include/update.autofan.phpsource/system-autofan/scripts/autofanunRAIDv6/dynamix.system.autofan.plg
🚧 Files skipped from review as they are similar to previous changes (3)
- source/system-autofan/default.cfg
- source/system-autofan/include/update.autofan.php
- unRAIDv6/dynamix.system.autofan.plg
Fan picker now shows only fans from the same hwmon chip as the selected PWM controller. Detect button auto-selects the matched fan in the dropdown instead of filling a text field. Falls back to all fans if chip has no fan sensors. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… setup Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ixes Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- autofan: rename program_name to "MRJAR autofan" - SystemFan.php: add op=autopair — cycles all PWMs on selected chip, finds matching fan by RPM response, returns JSON array of pairs - FanSettings.page: Auto Pair button next to PWM controller dropdown, shows results table with Apply button per pair Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Set icon attribute to fan icon (was showing generic plugin manager icon) - Package now includes images/dynamix.system.autofan.png and README.md (Plugins page renders README.md as the description) - Author field now credits mrjacobarussell as fork maintainer - Added support link to GitHub issues Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Avoid creating a new GitHub release; reuse the existing release's asset slot for the rebuilt txz. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
source/system-autofan/scripts/autofan (1)
180-187:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCritical: drive filtering breaks with multiple drives.
Lines 182 and 184 use command substitution
$(echo $INCLUDE | sed ...)to convert comma-separated drive lists into patterns forgrep. However, command substitution collapses newlines into spaces, so a multi-drive list like"sda,sdb"becomes the literal pattern"/dev/sda sdb"instead of matching individual drives.Example failure:
INCLUDE="sda,sdb"- After substitution:
grep -F "/dev/sda /dev/sdb"(single literal pattern)- Input:
"/dev/sda\n/dev/sdb"- Result: no match (expects literal
"/dev/sda /dev/sdb"on one line)Both include (line 182) and exclude (line 184) logic exhibit this bug. Single-drive filters work, but multi-drive filters silently fail, causing incorrect temperature monitoring.
🐛 Proposed fix using proper multi-pattern grep
# Get list of drives to monitor ALL_DRIVES=$(ls /dev/{[hs]d*[a-z],nvme[0-9]} 2>/dev/null | grep -v "$flash") if [[ -n $INCLUDE ]]; then - DRIVES=$(echo "$ALL_DRIVES" | grep -F "$(echo $INCLUDE | sed -e $'s/,/\\\n/g' | sed -e 's/[np][0-9]//g' | sed 's|^|/dev/|')") + # Build grep -e patterns for each included drive + include_pattern="" + for drive in $(echo $INCLUDE | tr ',' ' '); do + drive_clean=$(echo $drive | sed -e 's/[np][0-9]//g') + include_pattern="$include_pattern -e /dev/$drive_clean" + done + DRIVES=$(echo "$ALL_DRIVES" | grep -F $include_pattern) elif [[ -n $EXCLUDE ]]; then - DRIVES=$(echo "$ALL_DRIVES" | grep -v "/dev/$(echo $EXCLUDE | sed -e $'s/,/\\\n/g' | sed -e 's/[np][0-9]//g')") + # Build grep -v -e patterns for each excluded drive + exclude_pattern="" + for drive in $(echo $EXCLUDE | tr ',' ' '); do + drive_clean=$(echo $drive | sed -e 's/[np][0-9]//g') + exclude_pattern="$exclude_pattern -e /dev/$drive_clean" + done + DRIVES=$(echo "$ALL_DRIVES" | grep -v $exclude_pattern) else DRIVES=$ALL_DRIVES fiAlternatively, use extended regex with
grep -E:if [[ -n $INCLUDE ]]; then - DRIVES=$(echo "$ALL_DRIVES" | grep -F "$(echo $INCLUDE | sed -e $'s/,/\\\n/g' | sed -e 's/[np][0-9]//g' | sed 's|^|/dev/|')") + include_regex=$(echo $INCLUDE | sed -e 's/[np][0-9]//g' -e 's/,/|/g') + DRIVES=$(echo "$ALL_DRIVES" | grep -E "/dev/($include_regex)") elif [[ -n $EXCLUDE ]]; then - DRIVES=$(echo "$ALL_DRIVES" | grep -v "/dev/$(echo $EXCLUDE | sed -e $'s/,/\\\n/g' | sed -e 's/[np][0-9]//g')") + exclude_regex=$(echo $EXCLUDE | sed -e 's/[np][0-9]//g' -e 's/,/|/g') + DRIVES=$(echo "$ALL_DRIVES" | grep -v -E "/dev/($exclude_regex)") fi🤖 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 `@source/system-autofan/scripts/autofan` around lines 180 - 187, The drive filtering collapses comma-separated INCLUDE/EXCLUDE lists into a single-space string due to command substitution, so update the logic that sets DRIVES to treat each comma-separated entry as a separate grep pattern: for INCLUDE and EXCLUDE convert the variable into newline-separated entries (e.g. via tr ',' '\n' or printf '%s\n') and feed that to grep using -F -f /dev/stdin or construct an alternation for grep -E (replace commas with | and prefix /dev/), ensuring ALL_DRIVES is filtered correctly; modify the blocks that reference ALL_DRIVES, INCLUDE, EXCLUDE and set DRIVES accordingly.
♻️ Duplicate comments (3)
unRAIDv6/dynamix.system.autofan.plg (1)
6-6:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCritical: version still downgrades from upstream (duplicate of past review).
The version entity is set to
2025.06.10, which is lexicographically and chronologically lower than the upstream2025.11.14. Plugin management systems will reject this as a downgrade, preventing users on the upstream version from installing or updating to this enhanced fork.This issue was previously flagged suggesting a version scheme like
2025.11.14.1(incrementing from upstream) or2025.06.10-fork.1(fork suffix) to maintain monotonic ordering.📦 Recommended version fix
-<!ENTITY version "2025.06.10"> +<!ENTITY version "2025.11.14.1">Or with fork suffix:
-<!ENTITY version "2025.06.10"> +<!ENTITY version "2025.11.14-fork.1">Then update the download URL and CHANGES section to match.
🤖 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 `@unRAIDv6/dynamix.system.autofan.plg` at line 6, The VERSION entity in dynamix.system.autofan.plg is set to 2025.06.10 which is lower than upstream 2025.11.14 and will be treated as a downgrade; change the <!ENTITY version "2025.06.10"> to a monotonic, higher value (e.g., "2025.11.14.1" or "2025.06.10-fork.1") so plugin managers accept updates, and then update the plugin download URL and the CHANGES/metadata to match the new version string (ensure the new value is used wherever ENTITY version is referenced).source/system-autofan/scripts/autofan (1)
227-237:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPath validation for TEMP_SENSOR (duplicate of past review).
The function reads from
$TEMP_SENSORwithout validating that the path is under/sys/devices. While the configuration file is root-controlled, defense-in-depth principles suggest validating sensor paths to prevent reading arbitrary files.This issue was flagged in a previous review (lines 227-237) suggesting validation that the path starts with
/sys/devicesand is a regular file.🛡️ Proposed validation
function_get_sensor_temp() { # Read temperature from hwmon sensor file (values are in millidegrees Celsius) + # Validate that sensor path is under /sys/devices + case "$TEMP_SENSOR" in + /sys/devices/*) ;; + *) echo "$program_name: error: sensor path must be under /sys/devices" | logger -t$program_name; HIGHEST_TEMP=0; return ;; + esac local raw raw=$(cat "$TEMP_SENSOR" 2>/dev/null)🤖 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 `@source/system-autofan/scripts/autofan` around lines 227 - 237, In function_get_sensor_temp, validate TEMP_SENSOR before reading: ensure it is non-empty, begins with the prefix "/sys/devices" and is a regular readable file (e.g., test with a prefix check and [[ -f "$TEMP_SENSOR" ]] or similar) and only then read it; if the check fails set HIGHEST_TEMP=0 and log a warning referencing $TEMP_SENSOR and $program_name; replace the direct cat "$TEMP_SENSOR" call with the guarded read so arbitrary files cannot be read.source/system-autofan/FanSettings.page (1)
172-189:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winXSS risk in sensor dropdown construction (duplicate of past review).
The dropdown construction in
loadTempSensors(lines 179-181) still concatenatesitem.chip,item.label,item.temp, anditem.sensordirectly into HTML without escaping. While these values originate from kernel sysfs (generally safe), defense-in-depth principles recommend using safe DOM construction methods.This issue was flagged in a previous review (lines 172-189) and marked as addressed in commits 1a132d6 to 5b85225, but the current code still exhibits the same pattern. Please verify whether the fix was reverted or if the addressing note was premature.
🛡️ Safer construction using jQuery
$.each(data, function(i, item) { var label = item.chip + ' - ' + item.label + ' (' + item.temp + '°C)'; var selected = (item.sensor === current) ? ' selected' : ''; - sel.append('<option value="' + item.sensor + '"' + selected + '>' + label + '</option>'); + var opt = $('<option>').val(item.sensor).text(label); + if (selected) opt.prop('selected', true); + sel.append(opt); });🤖 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 `@source/system-autofan/FanSettings.page` around lines 172 - 189, The loadTempSensors function builds option HTML by concatenating item.chip, item.label, item.temp and item.sensor directly which creates an XSS risk; replace the string-concatenation with safe DOM construction (e.g., create option elements via jQuery or document.createElement, use .val()/.prop('selected', true) and .text() to set the visible label) when populating `#temp_sensor_select` and when setting input[name=temp_sensor], ensuring item.sensor is used only as the option value and all displayed text is set via text nodes rather than HTML concatenation.
🤖 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 `@source/system-autofan/FanSettings.page`:
- Around line 132-165: The autoPair() function builds HTML by concatenating
server values (p.chip, p.pwm_name, p.fan_name, p.rpm_delta) and injects
p.pwm/p.fan into an inline onclick, which risks HTML/attribute injection; update
autoPair() to build the table using safe DOM APIs/jQuery element creation (e.g.,
$('<tr>'), $('<td>') .text(...)) so values are inserted via .text() or text
nodes (not string concatenation), remove the inline onclick and instead attach a
click handler (or store pwm/fan in data-* attributes and bind a click listener
that calls applyPair(pwm, fan)), and ensure applyPair() is still used but
receives values from the bound handler rather than from HTML-string
interpolation.
In `@unRAIDv6/dynamix.system.autofan.plg`:
- Line 128: The <URL> element currently hardcodes the GitHub release tag
"2025.06.10" which must match the package version entity (&version;); update the
<URL> value to use the matching release tag for &version; (e.g., change
"2025.06.10" to "2025.11.14.1") so the download link and the &version; entity
are consistent; ensure the new tag exists in the GitHub releases and update any
other hardcoded occurrences of the old tag in the file (search for the <URL>
element and any references to the old tag).
---
Outside diff comments:
In `@source/system-autofan/scripts/autofan`:
- Around line 180-187: The drive filtering collapses comma-separated
INCLUDE/EXCLUDE lists into a single-space string due to command substitution, so
update the logic that sets DRIVES to treat each comma-separated entry as a
separate grep pattern: for INCLUDE and EXCLUDE convert the variable into
newline-separated entries (e.g. via tr ',' '\n' or printf '%s\n') and feed that
to grep using -F -f /dev/stdin or construct an alternation for grep -E (replace
commas with | and prefix /dev/), ensuring ALL_DRIVES is filtered correctly;
modify the blocks that reference ALL_DRIVES, INCLUDE, EXCLUDE and set DRIVES
accordingly.
---
Duplicate comments:
In `@source/system-autofan/FanSettings.page`:
- Around line 172-189: The loadTempSensors function builds option HTML by
concatenating item.chip, item.label, item.temp and item.sensor directly which
creates an XSS risk; replace the string-concatenation with safe DOM construction
(e.g., create option elements via jQuery or document.createElement, use
.val()/.prop('selected', true) and .text() to set the visible label) when
populating `#temp_sensor_select` and when setting input[name=temp_sensor],
ensuring item.sensor is used only as the option value and all displayed text is
set via text nodes rather than HTML concatenation.
In `@source/system-autofan/scripts/autofan`:
- Around line 227-237: In function_get_sensor_temp, validate TEMP_SENSOR before
reading: ensure it is non-empty, begins with the prefix "/sys/devices" and is a
regular readable file (e.g., test with a prefix check and [[ -f "$TEMP_SENSOR"
]] or similar) and only then read it; if the check fails set HIGHEST_TEMP=0 and
log a warning referencing $TEMP_SENSOR and $program_name; replace the direct cat
"$TEMP_SENSOR" call with the guarded read so arbitrary files cannot be read.
In `@unRAIDv6/dynamix.system.autofan.plg`:
- Line 6: The VERSION entity in dynamix.system.autofan.plg is set to 2025.06.10
which is lower than upstream 2025.11.14 and will be treated as a downgrade;
change the <!ENTITY version "2025.06.10"> to a monotonic, higher value (e.g.,
"2025.11.14.1" or "2025.06.10-fork.1") so plugin managers accept updates, and
then update the plugin download URL and the CHANGES/metadata to match the new
version string (ensure the new value is used wherever ENTITY version is
referenced).
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 07058621-3ca0-45e2-ae49-c8bb02884a1f
📒 Files selected for processing (7)
README.mdarchive/dynamix.system.autofan.txzsource/system-autofan/FanSettings.pagesource/system-autofan/README.mdsource/system-autofan/include/SystemFan.phpsource/system-autofan/scripts/autofanunRAIDv6/dynamix.system.autofan.plg
Comment on lines
+132
to
+165
| function autoPair() { | ||
| var pwm = $('select[name=controller]').val(); | ||
| $('#autopair_btn').prop('disabled',true).val('_(Scanning...)_'); | ||
| $('#autopair_results').html('<em>_(Please wait — cycling each PWM channel to detect fans...)_</em>'); | ||
| $.getJSON('/plugins/<?=$plugin?>/include/SystemFan.php', {op:'autopair', pwm:pwm}, function(data) { | ||
| $('#autopair_btn').prop('disabled',false).val('_(Auto Pair)_'); | ||
| if (!data || data.length === 0) { | ||
| $('#autopair_results').html('<span style="color:red">_(No fan/PWM pairs detected.)_</span>'); | ||
| return; | ||
| } | ||
| var html = '<table style="margin-top:6px;border-collapse:collapse">'; | ||
| html += '<tr><th style="padding:3px 8px;text-align:left">PWM</th><th style="padding:3px 8px;text-align:left">Fan</th><th style="padding:3px 8px;text-align:left">RPM change</th><th></th></tr>'; | ||
| $.each(data, function(i, p) { | ||
| html += '<tr>'; | ||
| html += '<td style="padding:3px 8px">' + p.chip + ' - ' + p.pwm_name + '</td>'; | ||
| html += '<td style="padding:3px 8px">' + p.fan_name + '</td>'; | ||
| html += '<td style="padding:3px 8px">+' + p.rpm_delta + ' rpm</td>'; | ||
| html += '<td style="padding:3px 8px"><input type="button" value="_(Apply)_" onclick="applyPair(\'' + p.pwm + '\',\'' + p.fan + '\')"></td>'; | ||
| html += '</tr>'; | ||
| }); | ||
| html += '</table>'; | ||
| $('#autopair_results').html(html); | ||
| }).fail(function() { | ||
| $('#autopair_btn').prop('disabled',false).val('_(Auto Pair)_'); | ||
| $('#autopair_results').html('<span style="color:red">_(Auto pair failed.)_</span>'); | ||
| }); | ||
| } | ||
| function applyPair(pwm, fan) { | ||
| $('select[name=controller]').val(pwm).trigger('change'); | ||
| $('select[name=fan]').val(fan).trigger('change'); | ||
| $('input[name=pwm]').prop('placeholder','_(Click DETECT)_'); | ||
| $('#pwm').prop('disabled',false); | ||
| $('#autopair_results').html(''); | ||
| } |
There was a problem hiding this comment.
HTML/attribute injection risk in autoPair results table.
Lines 146-149 concatenate server-provided values (p.chip, p.pwm_name, p.fan_name, p.rpm_delta) directly into HTML, and line 149 embeds p.pwm and p.fan paths into an onclick attribute using single-quote delimiters. While these values originate from kernel sysfs (generally safe), defense-in-depth principles recommend avoiding string concatenation for HTML construction.
If a path were to contain a single quote (e.g., via a compromised or unusual filesystem), the onclick handler would break out of its attribute boundary.
🛡️ Safer DOM construction using jQuery
var html = '<table style="margin-top:6px;border-collapse:collapse">';
html += '<tr><th style="padding:3px 8px;text-align:left">PWM</th><th style="padding:3px 8px;text-align:left">Fan</th><th style="padding:3px 8px;text-align:left">RPM change</th><th></th></tr>';
+ var tbody = $('<tbody>');
$.each(data, function(i, p) {
- html += '<tr>';
- html += '<td style="padding:3px 8px">' + p.chip + ' - ' + p.pwm_name + '</td>';
- html += '<td style="padding:3px 8px">' + p.fan_name + '</td>';
- html += '<td style="padding:3px 8px">+' + p.rpm_delta + ' rpm</td>';
- html += '<td style="padding:3px 8px"><input type="button" value="_(Apply)_" onclick="applyPair(\'' + p.pwm + '\',\'' + p.fan + '\')"></td>';
- html += '</tr>';
+ var row = $('<tr>');
+ row.append($('<td style="padding:3px 8px">').text(p.chip + ' - ' + p.pwm_name));
+ row.append($('<td style="padding:3px 8px">').text(p.fan_name));
+ row.append($('<td style="padding:3px 8px">').text('+' + p.rpm_delta + ' rpm'));
+ var btn = $('<input type="button" value="_(Apply)_">').data({pwm: p.pwm, fan: p.fan}).on('click', function() {
+ applyPair($(this).data('pwm'), $(this).data('fan'));
+ });
+ row.append($('<td style="padding:3px 8px">').append(btn));
+ tbody.append(row);
});
html += '</table>';
- $('`#autopair_results`').html(html);
+ $('`#autopair_results`').html(html.replace('</table>', '')).find('table').append(tbody).end().find('table').append('</table>');Alternatively, construct the entire table via jQuery without string templates.
🤖 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 `@source/system-autofan/FanSettings.page` around lines 132 - 165, The
autoPair() function builds HTML by concatenating server values (p.chip,
p.pwm_name, p.fan_name, p.rpm_delta) and injects p.pwm/p.fan into an inline
onclick, which risks HTML/attribute injection; update autoPair() to build the
table using safe DOM APIs/jQuery element creation (e.g., $('<tr>'), $('<td>')
.text(...)) so values are inserted via .text() or text nodes (not string
concatenation), remove the inline onclick and instead attach a click handler (or
store pwm/fan in data-* attributes and bind a click listener that calls
applyPair(pwm, fan)), and ensure applyPair() is still used but receives values
from the bound handler rather than from HTML-string interpolation.
| <!-- SOURCE PACKAGE --> | ||
| <FILE Name="&source;.txz" Run="upgradepkg --install-new --reinstall"> | ||
| <URL>https://raw.githubusercontent.com/unraid/dynamix/master/archive/&name;.txz</URL> | ||
| <URL>https://github.com/mrjacobarussell/dynamix/releases/download/2025.06.10/dynamix.system.autofan.txz</URL> |
There was a problem hiding this comment.
Download URL tag should match version entity.
Line 128 pins the download to GitHub tag 2025.06.10, which should match the &version; entity. If the version is updated to 2025.11.14.1 (as recommended to avoid downgrade issues), ensure the release tag and download URL are updated accordingly.
🤖 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 `@unRAIDv6/dynamix.system.autofan.plg` at line 128, The <URL> element currently
hardcodes the GitHub release tag "2025.06.10" which must match the package
version entity (&version;); update the <URL> value to use the matching release
tag for &version; (e.g., change "2025.06.10" to "2025.11.14.1") so the download
link and the &version; entity are consistent; ensure the new tag exists in the
GitHub releases and update any other hardcoded occurrences of the old tag in the
file (search for the <URL> element and any references to the old tag).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two improvements for fan control with USB hwmon controllers (e.g. Corsair Commander Pro):
1. Hardware sensor probe temperature source
-s <sensor_path>flag onautofanscript reads temp from any hwmontemp*_inputfile instead of scanning drives via smartctl2. Stable hwmon path resolution across reboots
hwmonNnumber each reboot depending on init orderrc.autofannow resolves the current hwmon directory by kernel device name at start/stop timehwmon_namecfg field (e.g.corsaircpro) — set it and paths are always correct regardless of reboot orderUI changes (FanSettings.page)
hwmon_namein cfgFiles changed
source/system-autofan/scripts/autofan— v1.7→v1.8, add-sflagsource/system-autofan/scripts/rc.autofan— addresolve_hwmon_in_string()source/system-autofan/include/SystemFan.php— addlist_temp()+op=tempsendpointsource/system-autofan/include/update.autofan.php— skiptemp_source/hwmon_namefrom optionssource/system-autofan/FanSettings.page— new UI fieldssource/system-autofan/default.cfg— new field defaultsTest plan
temp_source=sensorand valid sensor path, fan adjusts to probe temp not disk temphwmon_name=corsaircpro, reboot —rc.autofan startresolves correct hwmon dirtemp_source=hdd(default), existing behavior unchangedSummary by CodeRabbit
New Features
Bug Fixes
Documentation