Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 60 additions & 0 deletions .codex/coderabbit-fixes-wip.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
# CodeRabbit Fixes WIP

## Context

- Repo: unraid/webgui
- Branch: feat/backend-task-queue
- PR: 2665
- PR URL: https://github.com/unraid/webgui/pull/2665
- Generated at: 2026-06-18

## Inputs Pulled

- [x] Unresolved robot review threads pulled (7)
- [x] Top-level robot review notes and PR conversation comments pulled
- [x] Top-level actionable review-body/PR comments extracted into queue (5 nitpicks)
- [x] User asked whether to include human review comments
- [x] Human review comments included in queue: no (user chose robot-only)
- [x] Existing non-CodeRabbit thread replies checked before adding duplicate feedback (none from bots/humans; only a github-actions test-plugin notice)

## Fix Queue

| Item ID | Type | File | Line | Summary | Status | Link | Evidence |
| --- | --- | --- | --- | --- | --- | --- | --- |
| CR-001 | thread | BodyInlineJS.php | 214 | XSS in swal title via task.title (html:true) | DONE | https://github.com/unraid/webgui/pull/2665#discussion_r3436502042 | escapeTaskHtml(task.title) in swal title; php -l OK |
| CR-002 | thread | BodyInlineJS.php | 279-291 | t.id unescaped in onclick handlers | DONE | https://github.com/unraid/webgui/pull/2665#discussion_r3436502054 | safeId=escapeTaskHtml(t.id) used in all 5 handlers; php -l OK |
| CR-003 | thread | HeadInlineJS.php | 183-192 | createTask silent failure on POST error | DONE | https://github.com/unraid/webgui/pull/2665#discussion_r3436502071 | .fail() hides spinner + error swal; php -l OK |
| CR-004 | thread | TaskQueue.php | 123 | Command injection via unescaped $args in bash -c | DONE | https://github.com/unraid/webgui/pull/2665#discussion_r3436502088 | escapeshellarg() over whole bash -c payload (preserves multi-arg word-split); reply posted explaining deviation from literal suggestion; php -l OK |
| CR-005 | thread | nchan/tasks | 27-30 | Empty/non-numeric PID breaks /proc liveness check | DONE | https://github.com/unraid/webgui/pull/2665#discussion_r3436502115 | ctype_digit() guard before file_exists; php -l OK |
| CR-006 | thread | default-base.css | 1919-1924 | Add min-width:0 for reliable ellipsis | DONE | https://github.com/unraid/webgui/pull/2665#discussion_r3436502123 | min-width:0 added to .op-tray .op-title |
| CR-007 | thread | nchan/tasks | 62-67 | Daemon sleeps forever on queued-only restart | DONE | https://github.com/unraid/webgui/pull/2665#discussion_r3437357364 | queued-without-running recovery pass before advance/active check; php -l OK |
| RVW-001 | review-body | TaskQueue.php | 147-177 | Race: concurrent create/launch can break one-running-per-type | DONE | https://github.com/unraid/webgui/pull/2665#pullrequestreview-4525730471 | per-type flock around dedupe->create->launch; php -l OK |
| RVW-002 | review-body | TaskCommand.php | 35-51 | abort/dismiss return no JSON body | BLOCKED | https://github.com/unraid/webgui/pull/2665#pullrequestreview-4525730471 | Declined: response body is unused; canonical success signal is the `tasks` nchan broadcast. Adding unused API surface conflicts with no-speculative-contract policy. Reply: https://github.com/unraid/webgui/pull/2665#issuecomment-4745856704 |
| RVW-003 | review-body | TaskCommand.php | 38-44 | Validate PID numeric before kill | DONE | https://github.com/unraid/webgui/pull/2665#pullrequestreview-4525730471 | ctype_digit()+(int)>1 guard before kill; php -l OK |
| RVW-004 | review-body | nchan/tasks | 32-37 | _ERROR_ marker substring false positives | DONE | https://github.com/unraid/webgui/pull/2665#pullrequestreview-4525730471 | match _ERROR_ as discrete \x1e record (canonical sentinel, same as routeMessage); reads tail in PHP, removing exec last-line fragility; php -l OK |
| RVW-005 | review-body | BodyInlineJS.php | 126-177 | Unescaped HTML in nchan message rendering | BLOCKED | https://github.com/unraid/webgui/pull/2665#pullrequestreview-4525730471 | Declined: renderMessage is a deliberate HTML/span protocol (addToID injects `<span id=...>`); blanket escaping breaks the live-log structure. Data originates from trusted server-side processes on a server-controlled channel, not untrusted client input. Reply posted. |

## Execution Log

1. CR-001 swal title — escaped task.title with escapeTaskHtml. DONE.
2. CR-002 onclick ids — introduced safeId=escapeTaskHtml(t.id), used everywhere. DONE.
3. CR-003 createTask — added .fail() with spinner hide + error swal. DONE.
4. CR-004 command injection — wrapped the whole `sleep .3 && $name $args$suffix` payload in escapeshellarg() (superior to literal escapeshellarg($args), which would collapse multiple space-separated args). DONE + thread reply.
5. CR-005 task_pid_alive — ctype_digit() numeric guard. DONE.
6. CR-007 queued-only recovery — added recovery pass + $changed=true so launch publishes. DONE.
7. RVW-004 _ERROR_ — discrete \x1e record match read in PHP. DONE.
8. CR-006 CSS — min-width:0. DONE.
9. RVW-001 race — per-type flock around critical section, released on every return path. DONE.
10. RVW-003 PID kill — ctype_digit()+(int)>1. DONE.
11. RVW-002 — BLOCKED (unused response surface). Reply posted.
12. RVW-005 — BLOCKED (deliberate HTML protocol, trusted source). Reply posted.

Validation: `php -l` clean on TaskQueue.php, TaskCommand.php, nchan/tasks, BodyInlineJS.php, HeadInlineJS.php.

## Final Checks

- [x] Queue reviewed: no `TODO` left
- [x] Remaining `BLOCKED` items documented with reason (RVW-002, RVW-005)
- [x] Every `BLOCKED`/not-valid CodeRabbit suggestion has a PR reply with the reason it was not applied
- [x] Re-pulled CodeRabbit threads and reviews
- [x] No unhandled top-level review-body comment remains
4 changes: 2 additions & 2 deletions emhttp/plugins/dynamix.docker.manager/javascript/docker.js
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ function updateContainer(container) {
swal({
title:_('Are you sure?'),text:_('Update container')+': '+container, type:'warning',html:true,showCancelButton:true,closeOnConfirm:false,confirmButtonText:_('Yes, update it!'),cancelButtonText:_('Cancel')
},function(){
openDocker('update_container '+encodeURIComponent(container),_('Updating the container'),'','loadlist');
openDocker('update_container '+encodeURIComponent(container),_('Update container')+': '+container,'','loadlist');
});
}
function rmContainer(container, image, id) {
Expand Down Expand Up @@ -182,7 +182,7 @@ function updateAll() {
$('input[type=button]').prop('disabled',true);
var ct = [];
for (var i=0,d; d=docker[i]; i++) if (d.update==1) ct.push(encodeURIComponent(d.name));
openDocker('update_container '+ct.join('*'),_('Updating all Containers'),'','loadlist');
openDocker('update_container '+ct.join('*'),_('Updating all Containers')+' ('+ct.length+')','','loadlist');
}
function rebuildAll() {
$('input[type=button]').prop('disabled',true);
Expand Down
18 changes: 18 additions & 0 deletions emhttp/plugins/dynamix.plugin.manager/Plugins.page
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ Title="Installed Plugins"
Tag="icon-plugins"
Tabs="true"
Code="e944"
Markdown="false"
---
<?PHP
/* Copyright 2005-2023, Lime Technology
Expand Down Expand Up @@ -143,6 +144,23 @@ function loadlist(id,check) {
$('#checkall').find('input').prop('disabled',false);
});
}
// Row buttons are rendered server-side (make_link) from the task queue: a plugin
// with a running/queued task shows "Upgrading" (disabled). Refresh the rows when
// the set of active plugin tasks changes, so that state appears/clears live —
// loadlist(null,1) re-renders just the status cells (no network re-check, no
// flickery full reload). The task queue is the single source of truth.
var _plgTaskSig = '';
function onPluginTasksChanged() {
var sig = '';
if (typeof taskList !== 'undefined') {
for (var i=0;i<taskList.length;i++) {
var t = taskList[i];
if (t && t.type=='plugins' && (t.status=='running'||t.status=='queued')) sig += t.id+';';
}
}
if (sig !== _plgTaskSig) { _plgTaskSig = sig; loadlist(null,1); }
}
window.onTaskListChanged = onPluginTasksChanged;
$(function() {
initlist();
$('.tabs-container').append("<span id='checkall' class='status vhshift'><input type='button' value=\"_(Check For Updates)_\" onclick='openPlugin(\"checkall\",\"_(Plugin Update Check)_\",\":return\")' disabled></span>");
Expand Down
17 changes: 16 additions & 1 deletion emhttp/plugins/dynamix.plugin.manager/include/PluginHelpers.php
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,18 @@
<?
$docroot ??= ($_SERVER['DOCUMENT_ROOT'] ?: '/usr/local/emhttp');
require_once "$docroot/webGui/include/Wrappers.php";
require_once "$docroot/plugins/dynamix/include/TaskQueue.php";

// true when the backend task queue has a running/queued task targeting this .plg
// (the task queue is the source of truth for an in-flight install/update)
function plugin_task_busy($arg) {
if (!$arg) return false;
foreach (task_list() as $t) {
if ($t['type']==='plugins' && in_array($t['status'],['running','queued'],true)
&& in_array($arg, preg_split('/\s+/', trim($t['cmd'])), true)) return true;
}
return false;
}

// Invoke the plugin command with indicated method
function plugin($method, $arg = '', $dontCache = false) {
Expand Down Expand Up @@ -71,7 +83,10 @@ function make_link($method, $arg, $extra='') {
$cmd = "plugin $method $arg".($extra?" $extra":"");
$func = "loadlist";
}
if (is_file("/tmp/plugins/pluginPending/$arg") && !$check) {
if (in_array($method,['update','install']) && plugin_task_busy($arg)) {
$label = $method=='install' ? _('Installing') : _('Upgrading');
return "<span class='orange-text'><i class='fa fa-hourglass-o fa-fw'></i>&nbsp;$label</span>";
} elseif (is_file("/tmp/plugins/pluginPending/$arg") && !$check) {
return "<span class='orange-text'><i class='fa fa-hourglass-o fa-fw'></i>&nbsp;"._('pending')."</span>";
} else {
return "$check<input type='button' id='$id' data='$arg' class='$method' value=\""._(ucfirst($method))."\" onclick='openInstall(\"$cmd\",\""._(ucwords($method)." Plugin")."\",\"$plg\",\"$func\");'$disabled>";
Expand Down
5 changes: 4 additions & 1 deletion emhttp/plugins/dynamix/Apps.page
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,10 @@ Code="e942"
?>
<script>
function installPlugin(file) {
openPlugin("plugin install "+file,"_(Install Plugin)_","","refresh");
// show which plugin in the progress heading; sanitize to a safe display slug
// (the heading is rendered as HTML), derived from the .plg basename
var name = (file.split('/').pop()||'').replace(/\.[a-z0-9]+$/i,'').replace(/[^\w.\- ]+/g,' ').trim();
openPlugin("plugin install "+file,"_(Install Plugin)_"+(name?': '+name:''),"","refresh");
}
</script>

Expand Down
5 changes: 4 additions & 1 deletion emhttp/plugins/dynamix/Language.page
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,10 @@ function getZIPfile(event,form) {
}
function installXML(name) {
var file = name.trim();
if (file) openPlugin('language install '+file, "_(Install Language)_");
// show which language pack in the progress heading; sanitize to a safe display
// slug (the heading is rendered as HTML), derived from the file basename
var disp = (file.split('/').pop()||'').replace(/\.[a-z0-9]+$/i,'').replace(/[^\w.\- ]+/g,' ').trim();
if (file) openPlugin('language install '+file, "_(Install Language)_"+(disp?': '+disp:''));
}
$(function() {
$('input.view').switchButton({labels_placement:'left', off_label:"_(User)_", on_label:"_(Developer)_"});
Expand Down
5 changes: 4 additions & 1 deletion emhttp/plugins/dynamix/Tailscale.page
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,10 @@ Title="Tailscale"
?>
<script>
function installPlugin(file) {
openPlugin("plugin install "+file,"_(Install Plugin)_","","refresh");
// show which plugin in the progress heading; sanitize to a safe display slug
// (the heading is rendered as HTML), derived from the .plg basename
var name = (file.split('/').pop()||'').replace(/\.[a-z0-9]+$/i,'').replace(/[^\w.\- ]+/g,' ').trim();
openPlugin("plugin install "+file,"_(Install Plugin)_"+(name?': '+name:''),"","refresh");
}
</script>

Expand Down
4 changes: 4 additions & 0 deletions emhttp/plugins/dynamix/include/DefaultPageLayout.php
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,10 @@
if ($wlan0) {
$nchan[] = 'webGui/nchan/wlan0';
}
// keep the task scheduler alive while background operations exist (it exits when idle)
if (glob('/var/local/emhttp/tasks/*.json')) {
$nchan[] = 'plugins/dynamix/nchan/tasks';
}
// build nchan scripts from found pages
$allPages = array_merge($taskPages, $buttonPages, $pages);
foreach ($allPages as $page) {
Expand Down
Loading
Loading