Skip to content

Commit ff6f2ea

Browse files
mguttmgutt
committed
Additional code review improvements
Control.php improvements: - Use ctype_digit() instead of is_numeric() for undo row validation Prevents accepting floats (1.9), scientific notation (1e3), or negatives (-1) - Update popular destinations only when jobs actually start Move updatePopularDestinations() to job execution (not queue creation) Prevents tracking popularity for cancelled/failed jobs - Add null coalescing operators (??) for $_POST['source'] and $_POST['target'] Avoids PHP notices when parameters are missing - Optimize invalid JSON scanning to avoid O(n²) complexity Replace array_shift() loop with index-based scan and single array_slice() Log once with count instead of spamming per entry - Prevent path traversal in 'stop' mode Use basename() to strip directory components and prevent ../escapes Only delete if filename is not empty Browse.page improvements: - Add rationale comments for FOLDER_EXPAND_DELAY and NAVIGATION_BUFFER - Use requestAnimationFrame() instead of setTimeout() in resetFileTree() Semantically correct for DOM operations, waits for browser render cycle
1 parent e599a9c commit ff6f2ea

2 files changed

Lines changed: 35 additions & 30 deletions

File tree

‎emhttp/plugins/dynamix/Browse.page‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -387,8 +387,8 @@ filemonitor.on('message', function(state) {
387387
setTimeout(function(){filemonitor.start();},3000);
388388

389389
// File tree navigation constants
390-
var FOLDER_EXPAND_DELAY = 300; // Delay per folder expansion in openFolderRecursive (ms)
391-
var NAVIGATION_BUFFER = 500; // Additional buffer time for navigation completion (ms)
390+
var FOLDER_EXPAND_DELAY = 300; // Delay per folder expansion in openFolderRecursive (ms) - allows DOM updates to complete
391+
var NAVIGATION_BUFFER = 500; // Additional buffer time for navigation completion (ms) - ensures all async operations finish
392392

393393
function setupTargetNavigation() {
394394
var $target = dfm.window.find('#dfm_target');
@@ -591,9 +591,9 @@ function resetFileTree($target) {
591591

592592
// Show the tree again by simulating a click
593593
// fileTreeAttach checks if html() is empty and will reload
594-
setTimeout(function() {
594+
requestAnimationFrame(function() {
595595
$target.click();
596-
}, 100);
596+
});
597597
}
598598

599599
function navigateFileTree(path) {

‎emhttp/plugins/dynamix/include/Control.php‎

Lines changed: 31 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -94,7 +94,7 @@ function quoted($name) {return is_array($name) ? implode(' ',array_map('escape',
9494
die();
9595
case 'calc':
9696
extract(parse_plugin_cfg('dynamix',true));
97-
$source = explode("\n",rawurldecode($_POST['source']));
97+
$source = explode("\n",rawurldecode($_POST['source'] ?? ''));
9898
[$null,$root,$main,$rest] = my_explode('/',$source[0],4);
9999
if ($root=='mnt' && in_array($main,['user','user0'])) {
100100
$disks = parse_ini_file('state/disks.ini',true);
@@ -121,8 +121,8 @@ function quoted($name) {return is_array($name) ? implode(' ',array_map('escape',
121121
$calc = '<div style="text-align:left;margin-left:56px">'.implode('<br>',$calc).'</div>';
122122
die($calc);
123123
case 'home':
124-
$source = explode("\n",rawurldecode($_POST['source']));
125-
$target = rawurldecode($_POST['target']);
124+
$source = explode("\n",rawurldecode($_POST['source'] ?? ''));
125+
$target = rawurldecode($_POST['target'] ?? '');
126126
$disks = parse_ini_file('state/disks.ini',true);
127127
$tag = implode('|',array_merge(['disk'],pools_filter($disks)));
128128
$loc1 = implode(',',array_unique(array_filter(explode(',',preg_replace("/($tag)/",',$1',exec("getfattr --no-dereference --absolute-names --only-values -n system.LOCATIONS ".quoted($source)." 2>/dev/null"))))));
@@ -153,8 +153,9 @@ function quoted($name) {return is_array($name) ? implode(' ',array_map('escape',
153153
if ($file = validname(rawurldecode($_POST['file']))) file_put_contents($file,rawurldecode($_POST['data']));
154154
die();
155155
case 'stop':
156-
$file = rawurldecode($_POST['file']);
157-
delete_file("/var/tmp/$file.tmp");
156+
// Prevent path traversal: only use basename (no directory components)
157+
$file = basename(rawurldecode($_POST['file'] ?? ''));
158+
if ($file !== '') delete_file("/var/tmp/$file.tmp");
158159
die();
159160
case 'start':
160161
$active = '/var/tmp/file.manager.active';
@@ -164,16 +165,17 @@ function quoted($name) {return is_array($name) ? implode(' ',array_map('escape',
164165
// read first JSON line from jobs file and write to active
165166
$lines = file($jobs, FILE_IGNORE_NEW_LINES);
166167
if (!empty($lines)) {
167-
// Skip invalid JSON entries (loop until we find valid JSON or run out of entries)
168-
while (!empty($lines)) {
169-
$data = json_decode($lines[0], true);
170-
if ($data) {
171-
// Valid JSON found, use it
172-
break;
173-
}
174-
// Invalid JSON, remove this entry and try next
175-
exec('logger -t webGUI "Warning: Skipped invalid JSON in file manager job queue"');
176-
array_shift($lines);
168+
// Skip invalid JSON entries (scan once, slice once)
169+
$skipped = 0;
170+
$data = null;
171+
for ($i = 0, $n = count($lines); $i < $n; $i++) {
172+
$data = json_decode($lines[$i], true);
173+
if ($data) break;
174+
$skipped++;
175+
}
176+
if ($skipped > 0) {
177+
exec('logger -t webGUI "Warning: Skipped '.$skipped.' invalid JSON entr'.($skipped===1?'y':'ies').' in file manager job queue"');
178+
$lines = array_slice($lines, $skipped);
177179
}
178180

179181
if (empty($lines)) {
@@ -182,6 +184,11 @@ function quoted($name) {return is_array($name) ? implode(' ',array_map('escape',
182184
die('0');
183185
}
184186

187+
// Update popular destinations when dequeuing a job
188+
if (in_array((int)($data['action'] ?? 0), [3, 4, 8, 9]) && !empty($data['target'] ?? '')) {
189+
updatePopularDestinations($data['target']);
190+
}
191+
185192
file_put_contents($active, $lines[0]);
186193
// remove first line from jobs file
187194
array_shift($lines);
@@ -199,14 +206,13 @@ function quoted($name) {return is_array($name) ? implode(' ',array_map('escape',
199206
$jobs = '/var/tmp/file.manager.jobs';
200207
$undo = '0';
201208
if (file_exists($jobs)) {
202-
$rows = explode(',',$_POST['row']);
209+
$rows = explode(',', $_POST['row'] ?? '');
203210
$lines = file($jobs, FILE_IGNORE_NEW_LINES);
204211
foreach ($rows as $row) {
205-
// Validate and convert to integer to prevent non-numeric input
206-
if (!is_numeric($row)) {
207-
continue; // Skip invalid entries
208-
}
212+
$row = trim($row);
213+
if ($row === '' || !ctype_digit($row)) continue;
209214
$row = (int)$row;
215+
if ($row < 1) continue;
210216
$line_number = $row - 1; // Convert 1-based job number to 0-based array index
211217
if (isset($lines[$line_number])) {
212218
unset($lines[$line_number]);
@@ -245,12 +251,11 @@ function quoted($name) {return is_array($name) ? implode(' ',array_map('escape',
245251
} else {
246252
// start operation
247253
file_put_contents($active, json_encode($data));
248-
}
249-
250-
// Update popular destinations for copy/move operations
251-
// Action types: 3=copy folder, 4=move folder, 8=copy file, 9=move file
252-
if (in_array((int)$data['action'], [3, 4, 8, 9]) && !empty($data['target'])) {
253-
updatePopularDestinations($data['target']);
254+
// Update popular destinations only when an operation actually starts
255+
// Action types: 3=copy folder, 4=move folder, 8=copy file, 9=move file
256+
if (in_array((int)$data['action'], [3, 4, 8, 9]) && !empty($data['target'])) {
257+
updatePopularDestinations($data['target']);
258+
}
254259
}
255260

256261
die();

0 commit comments

Comments
 (0)