From 1e649627083fb6a9493a938c28dd86be65b69434 Mon Sep 17 00:00:00 2001 From: Michael Beck Date: Thu, 7 May 2026 15:52:34 -0400 Subject: [PATCH 01/10] helper-based atomic-write cleanup; replace @unlink/@chmod MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Changes: cp_functions.php: + xoops_remove_file_quietly($path, $context): new helper for best-effort file removal. Skips non-existent paths, suppresses the unlink warning via a SCOPED error_reporting() toggle (no @ operator) wrapped in try/finally, and re-checks file_exists() after a failed unlink — only logging when the file is still present so TOCTOU races resolve silently. Uses xoops_file_label() for non-sensitive path labels in warnings. - 9 @unlink cleanup sites replaced with the helper. - @chmod($tempFile, $perms) replaced with checked chmod() that logs on failure but continues (content is already written). modules/system/class/maintenance.php: + Explicit `require_once .../include/cp_functions.php` so SystemMaintenance is self-sufficient regardless of which caller (admin, install, modulesadmin, preferences) loads it. - 6 atomic-write @unlink cleanup sites replaced with the helper. - @chmod replaced with checked + warn (mirrors cp_functions.php). - cleanOrphanedAvatars(): @unlink replaced with the helper plus a path-traversal-safe resolution. Avatar rows store 'avatars/' (kernel/avatar.php and 14 admin/profile writers), so basename() would have stripped the directory and silently bypassed all orphan cleanup. The new form: - normalises backslashes to '/' (Windows-historic data) - strips leading slashes (defends against absolute paths) - resolves via realpath() so '../' segments collapse - confirms containment under realpath(XOOPS_UPLOAD_PATH) with a trailing-separator boundary check - confirms is_file() before removal. The DB row cleanup runs unconditionally (file gone or not). Intentionally retained: - The 7 @rename(...) calls inside `if (!...)` checks. These are the core atomic-move operations; the boolean return is already detected and reported via trigger_error(). Removing the @ would let PHP's native warning fire alongside our diagnostic, double- reporting one event into display_errors output. Comment block in each file documents the rationale. --- htdocs/include/cp_functions.php | 69 ++++++++++++++++++--- htdocs/modules/system/class/maintenance.php | 67 ++++++++++++++++---- 2 files changed, 115 insertions(+), 21 deletions(-) diff --git a/htdocs/include/cp_functions.php b/htdocs/include/cp_functions.php index 53574653b..2209b9de8 100644 --- a/htdocs/include/cp_functions.php +++ b/htdocs/include/cp_functions.php @@ -120,6 +120,39 @@ function xoops_file_label($filename) return basename($filename); } +/** + * Best-effort file removal used by atomic-write cleanup paths and similar + * fire-and-forget cleanup. Skips non-existent paths so already-deleted + * files don't trigger warnings, suppresses the unlink() warning via a + * scoped error_reporting() toggle (no `@` operator), and re-checks + * existence after a failed unlink — only logging when the file is still + * present, so TOCTOU races resolve silently. + * + * @param string $path Absolute path to the file to remove. + * @param string $context Short label used in the warning message + * (e.g. 'temporary', 'backup'). + * + * @return void + */ +function xoops_remove_file_quietly($path, $context = 'temporary') +{ + if (!file_exists($path)) { + return; + } + $previousLevel = error_reporting(0); + try { + $ok = unlink($path); + } finally { + error_reporting($previousLevel); + } + if (!$ok && file_exists($path)) { + trigger_error( + sprintf('Failed to remove %s file: %s', $context, xoops_file_label($path)), + E_USER_WARNING + ); + } +} + /** * Write a file through a temporary sibling and replace the target on success. * @@ -141,13 +174,13 @@ function xoops_write_file_atomically($filename, $content) $expectedBytes = strlen($content); $bytesWritten = file_put_contents($tempFile, $content, LOCK_EX); if ($bytesWritten === false) { - @unlink($tempFile); + xoops_remove_file_quietly($tempFile, 'temporary'); trigger_error(sprintf('Failed to write file: %s', $label), E_USER_WARNING); return false; } if ($bytesWritten !== $expectedBytes) { - @unlink($tempFile); + xoops_remove_file_quietly($tempFile, 'temporary'); trigger_error( sprintf( 'Short write for %s: wrote %d of %d bytes', @@ -167,11 +200,25 @@ function xoops_write_file_atomically($filename, $content) $targetPerms = $currentPerms & 0777; } } - @chmod($tempFile, $targetPerms); + if (!chmod($tempFile, $targetPerms)) { + // Non-fatal: file is written, only the perms didn't take. Continue + // with the rename rather than aborting — most callers care about + // content integrity over exact perms. + trigger_error( + sprintf('Failed to set permissions on temp file for %s', $label), + E_USER_WARNING + ); + } + // The four @rename(...) calls below are inside `if (!...)` checks — + // failure is detected by the boolean return and reported via + // trigger_error(). The `@` is retained to suppress PHP's native + // warning, which would otherwise double-report alongside our own + // diagnostic line. Removing it would not improve detection but would + // pollute display_errors output. if (!@rename($tempFile, $filename)) { if (!file_exists($filename)) { - @unlink($tempFile); + xoops_remove_file_quietly($tempFile, 'temporary'); trigger_error(sprintf(XOOPS_WRITE_FILE_WRITE_ERROR, $label), E_USER_WARNING); return false; @@ -179,14 +226,16 @@ function xoops_write_file_atomically($filename, $content) $backupFile = tempnam($directory, 'xwb'); if ($backupFile === false) { - @unlink($tempFile); + xoops_remove_file_quietly($tempFile, 'temporary'); trigger_error(sprintf(XOOPS_WRITE_FILE_WRITE_ERROR, $label), E_USER_WARNING); return false; } - @unlink($backupFile); + // tempnam() created a 0-byte placeholder we don't need; remove it + // so the rename below can take its slot. + xoops_remove_file_quietly($backupFile, 'backup'); if (!@rename($filename, $backupFile)) { - @unlink($tempFile); + xoops_remove_file_quietly($tempFile, 'temporary'); trigger_error(sprintf(XOOPS_WRITE_FILE_WRITE_ERROR, $label), E_USER_WARNING); return false; @@ -194,7 +243,7 @@ function xoops_write_file_atomically($filename, $content) if (!@rename($tempFile, $filename)) { if (!@rename($backupFile, $filename)) { - @unlink($tempFile); + xoops_remove_file_quietly($tempFile, 'temporary'); trigger_error( sprintf( 'Failed to replace file and restore original: %s. Original content was left in backup file %s; manual restoration may be required.', @@ -207,13 +256,13 @@ function xoops_write_file_atomically($filename, $content) return false; } - @unlink($tempFile); + xoops_remove_file_quietly($tempFile, 'temporary'); trigger_error(sprintf(XOOPS_WRITE_FILE_WRITE_ERROR, $label), E_USER_WARNING); return false; } - @unlink($backupFile); + xoops_remove_file_quietly($backupFile, 'backup'); } return true; diff --git a/htdocs/modules/system/class/maintenance.php b/htdocs/modules/system/class/maintenance.php index 567f4f5d4..1c2035d23 100644 --- a/htdocs/modules/system/class/maintenance.php +++ b/htdocs/modules/system/class/maintenance.php @@ -19,6 +19,12 @@ // throw new \RuntimeException('XOOPS root path not defined'); //} +// xoops_remove_file_quietly() lives in cp_functions.php; admin and install +// callers normally load it via cp_header.php / page_moduleinstaller.php, +// but require it explicitly here so SystemMaintenance is self-sufficient +// regardless of which context instantiates it. +require_once XOOPS_ROOT_PATH . '/include/cp_functions.php'; + /** * System Maintenance * @@ -168,8 +174,33 @@ public function CleanAvatar() /** @var array $myrow */ while (false !== ($myrow = $this->db->fetchArray($result))) { - //delete file - @unlink(XOOPS_UPLOAD_PATH . '/' . $myrow['avatar_file']); + // Avatar files are stored as 'avatars/' (see + // kernel/avatar.php and the various admin/edituser writers), + // so basename() would silently bypass cleanup. Instead: + // - normalise backslashes to '/' (Windows-historic data) + // - strip leading slashes (defends against absolute paths + // accidentally or maliciously stored in the column) + // - resolve via realpath() so any '../' segments collapse + // - confirm the resolved path is contained inside the + // resolved upload-root (prefix check with a trailing + // separator so 'uploadsX/...' doesn't satisfy 'uploads') + // - confirm it's a regular file before removal. + // Only then invoke the cleanup helper. + $avatarFile = ltrim(str_replace('\\', '/', (string) ($myrow['avatar_file'] ?? '')), '/'); + if ('' === $avatarFile) { + $result1 = $this->db->exec('DELETE FROM ' . $this->db->prefix('avatar') . ' WHERE avatar_id=' . $myrow['avatar_id']); + continue; + } + $avatarPath = realpath(XOOPS_UPLOAD_PATH . '/' . $avatarFile); + $uploadRoot = realpath(XOOPS_UPLOAD_PATH); + if ( + is_string($avatarPath) + && is_string($uploadRoot) + && str_starts_with($avatarPath, rtrim($uploadRoot, DIRECTORY_SEPARATOR) . DIRECTORY_SEPARATOR) + && is_file($avatarPath) + ) { + xoops_remove_file_quietly($avatarPath, 'orphaned avatar'); + } //clean avatar table $result1 = $this->db->exec('DELETE FROM ' . $this->db->prefix('avatar') . ' WHERE avatar_id=' . $myrow['avatar_id']); } @@ -221,10 +252,10 @@ private function writeFileWithWarning($filename, $content) $result = file_put_contents($tempFile, $content, LOCK_EX); if ($result === false) { - @unlink($tempFile); + xoops_remove_file_quietly($tempFile, 'temp guard'); trigger_error(sprintf('Failed to write guard file: %s', $label), E_USER_WARNING); } elseif ($result !== $expected) { - @unlink($tempFile); + xoops_remove_file_quietly($tempFile, 'temp guard'); trigger_error( sprintf( 'Short write for guard file %s: wrote %d of %d bytes', @@ -242,20 +273,34 @@ private function writeFileWithWarning($filename, $content) $targetPerms = $currentPerms & 0777; } } - @chmod($tempFile, $targetPerms); + if (!chmod($tempFile, $targetPerms)) { + // Non-fatal: content is written, only the perms didn't + // take. Continue with the rename rather than aborting. + trigger_error( + sprintf('Failed to set permissions on temp guard file for %s', $label), + E_USER_WARNING + ); + } + // The @rename(...) calls below are inside `if (!...)` checks — + // failure is detected by the boolean return and reported via + // trigger_error(). The `@` is retained to suppress PHP's + // native warning, which would otherwise double-report + // alongside our own diagnostic. $backupFile = null; if (file_exists($filename)) { $backupFile = tempnam(dirname($filename), 'mtb'); if ($backupFile === false) { - @unlink($tempFile); + xoops_remove_file_quietly($tempFile, 'temp guard'); trigger_error(sprintf('Failed to create backup file for %s', $label), E_USER_WARNING); return; } - @unlink($backupFile); + // tempnam() created a 0-byte placeholder; remove it so + // the rename below can take its slot. + xoops_remove_file_quietly($backupFile, 'backup guard'); if (!@rename($filename, $backupFile)) { - @unlink($tempFile); + xoops_remove_file_quietly($tempFile, 'temp guard'); trigger_error(sprintf('Failed to back up guard file: %s', $label), E_USER_WARNING); return; @@ -263,7 +308,7 @@ private function writeFileWithWarning($filename, $content) } if (!@rename($tempFile, $filename)) { - @unlink($tempFile); + xoops_remove_file_quietly($tempFile, 'temp guard'); if ($backupFile !== null) { if (!@rename($backupFile, $filename)) { trigger_error(sprintf('Failed to restore original guard file: %s', $label), E_USER_WARNING); @@ -271,8 +316,8 @@ private function writeFileWithWarning($filename, $content) } trigger_error(sprintf('Failed to replace guard file: %s', $label), E_USER_WARNING); - } elseif ($backupFile !== null && file_exists($backupFile) && !@unlink($backupFile)) { - trigger_error(sprintf('Failed to remove backup guard file: %s', $label), E_USER_WARNING); + } elseif ($backupFile !== null) { + xoops_remove_file_quietly($backupFile, 'backup guard'); } } } From b5fc7bfa998a9514315b3a7cb5eb2e2fd239b7ab Mon Sep 17 00:00:00 2001 From: Michael Beck Date: Thu, 7 May 2026 19:23:44 -0400 Subject: [PATCH 02/10] test(system): cover CleanAvatar() orphan path safety; hoist realpath out of loop MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address PR #52 follow-up review (Copilot non-blocking comments): modules/system/class/maintenance.php (CleanAvatar): realpath(XOOPS_UPLOAD_PATH) and the trailing-separator prefix are constant for the whole run, so computing them per row in the while-loop is wasted filesystem work on installations with large orphaned-avatar tables. Hoisted outside the loop into \$uploadRoot and \$uploadRootPrefix; the in-loop check now uses str_starts_with against the cached prefix. tests/unit/htdocs/modules/system/SystemMaintenanceTest.php: Added six #[Test] cases covering the path-traversal-safe orphan cleanup logic introduced when @unlink() was replaced by xoops_remove_file_quietly(): - cleanAvatarRemovesValidAvatarFileUnderUploadRoot — happy path - cleanAvatarSkipsTraversalPathButStillDeletesDbRow — ../ blocked - cleanAvatarSkipsAbsolutePathButStillDeletesDbRow — /etc/hosts - cleanAvatarHandlesMissingFileAndStillDeletesDbRow - cleanAvatarHandlesEmptyAvatarFileAndStillDeletesDbRow - cleanAvatarNormalisesBackslashesInAvatarFile Each test asserts exec() is called exactly twice (avatar DELETE + avatar_user_link cleanup) regardless of file removal outcome, so the 'DB rows deleted regardless of file outcome' invariant is codified rather than relying on inspection. Filesystem fixtures live in a unique scratch subdirectory under XOOPS_UPLOAD_PATH/avatars/_test__ with finally-block cleanup so the tests do not collide with each other or with anything else in the upload tree. --- htdocs/modules/system/class/maintenance.php | 14 +- .../modules/system/SystemMaintenanceTest.php | 219 ++++++++++++++++++ 2 files changed, 230 insertions(+), 3 deletions(-) diff --git a/htdocs/modules/system/class/maintenance.php b/htdocs/modules/system/class/maintenance.php index 1c2035d23..1f1055f73 100644 --- a/htdocs/modules/system/class/maintenance.php +++ b/htdocs/modules/system/class/maintenance.php @@ -172,6 +172,15 @@ public function CleanAvatar() ); } + // Resolve the upload-root once for the whole sweep. Both realpath() + // and the trailing-separator prefix are constant for the run, so + // computing them per row would do unnecessary filesystem work on + // installations with large numbers of orphaned avatars. + $uploadRoot = realpath(XOOPS_UPLOAD_PATH); + $uploadRootPrefix = is_string($uploadRoot) + ? rtrim($uploadRoot, DIRECTORY_SEPARATOR) . DIRECTORY_SEPARATOR + : null; + /** @var array $myrow */ while (false !== ($myrow = $this->db->fetchArray($result))) { // Avatar files are stored as 'avatars/' (see @@ -192,11 +201,10 @@ public function CleanAvatar() continue; } $avatarPath = realpath(XOOPS_UPLOAD_PATH . '/' . $avatarFile); - $uploadRoot = realpath(XOOPS_UPLOAD_PATH); if ( is_string($avatarPath) - && is_string($uploadRoot) - && str_starts_with($avatarPath, rtrim($uploadRoot, DIRECTORY_SEPARATOR) . DIRECTORY_SEPARATOR) + && is_string($uploadRootPrefix) + && str_starts_with($avatarPath, $uploadRootPrefix) && is_file($avatarPath) ) { xoops_remove_file_quietly($avatarPath, 'orphaned avatar'); diff --git a/tests/unit/htdocs/modules/system/SystemMaintenanceTest.php b/tests/unit/htdocs/modules/system/SystemMaintenanceTest.php index 71771c364..ac8ebda3a 100644 --- a/tests/unit/htdocs/modules/system/SystemMaintenanceTest.php +++ b/tests/unit/htdocs/modules/system/SystemMaintenanceTest.php @@ -261,4 +261,223 @@ public function isValidTableCachesResults(): void // Invalid table still false from cache $this->assertFalse($method->invoke($maintenance, 'nonexistent')); } + + // --------------------------------------------------------------- + // CleanAvatar() — orphan avatar file/row cleanup. + // + // The method scans the avatar table for custom-avatar rows whose + // owning user has been deleted, removes the on-disk file (when it + // is safely contained under XOOPS_UPLOAD_PATH), and deletes both + // the avatar row and any leftover avatar_user_link rows. + // + // These tests cover the path-traversal-safe resolution introduced + // when @unlink() was replaced by xoops_remove_file_quietly(): + // - happy path: avatars/ under XOOPS_UPLOAD_PATH is removed + // - traversal / absolute / outside-root inputs are silently skipped + // - the avatar DB row is deleted regardless of file-removal outcome + // - empty avatar_file deletes the DB row and continues + // + // Filesystem fixtures live in a unique subdirectory under + // XOOPS_UPLOAD_PATH/avatars/ so the tests do not collide with each + // other or with anything else in the upload tree. + // --------------------------------------------------------------- + + /** + * Returns the per-test scratch subdirectory under XOOPS_UPLOAD_PATH/avatars/. + * The path is RELATIVE to the upload root, formatted with forward slashes + * so it can be embedded in avatar_file values verbatim. + */ + private function avatarScratchRel(): string + { + return 'avatars/_test_' . getmypid() . '_' . uniqid(); + } + + /** + * Build the absolute filesystem path that corresponds to a relative + * avatar_file value (e.g. 'avatars/foo.png'). + */ + private function uploadAbs(string $rel): string + { + return XOOPS_UPLOAD_PATH . '/' . $rel; + } + + /** + * Create the scratch directory and place a fixture avatar file inside. + * Returns [$relPath, $absPath] where $relPath is what would be stored + * in the avatar_file column and $absPath is the on-disk location. + */ + private function placeFixtureAvatar(string $scratchRel, string $filename): array + { + $scratchAbs = $this->uploadAbs($scratchRel); + if (!is_dir($scratchAbs) && !mkdir($scratchAbs, 0755, true) && !is_dir($scratchAbs)) { + $this->fail('Could not create test scratch dir: ' . $scratchAbs); + } + $absPath = $scratchAbs . '/' . $filename; + file_put_contents($absPath, 'fixture'); + $relPath = $scratchRel . '/' . $filename; + + return [$relPath, $absPath]; + } + + /** + * Recursively remove the per-test scratch directory. + */ + private function removeScratchDir(string $scratchRel): void + { + $scratchAbs = $this->uploadAbs($scratchRel); + if (!is_dir($scratchAbs)) { + return; + } + foreach (scandir($scratchAbs) ?: [] as $entry) { + if ('.' === $entry || '..' === $entry) { + continue; + } + $path = $scratchAbs . '/' . $entry; + if (is_file($path)) { + @unlink($path); + } + } + @rmdir($scratchAbs); + } + + /** + * Stub the database mock so $db->query() returns a sentinel result and + * $db->fetchArray() yields the supplied avatar rows once, then false. + * Returns the mock for further expectations. + */ + private function stubAvatarSweep($db, array $rows): void + { + $db->method('query')->willReturn('mock_result'); + $db->method('isResultSet')->willReturn(true); + $rows[] = false; // end-of-result sentinel + $db->method('fetchArray')->willReturnOnConsecutiveCalls(...$rows); + } + + #[Test] + public function cleanAvatarRemovesValidAvatarFileUnderUploadRoot(): void + { + $scratchRel = $this->avatarScratchRel(); + [$rel, $abs] = $this->placeFixtureAvatar($scratchRel, 'foo.png'); + + $db = $this->createMockDatabase(); + $this->stubAvatarSweep($db, [ + ['avatar_id' => 42, 'avatar_file' => $rel], + ]); + // exec() called twice: once for DELETE FROM avatar, once for the + // avatar_user_link cleanup at the end of CleanAvatar(). + $db->expects($this->exactly(2))->method('exec')->willReturn(1); + + $maintenance = $this->createMaintenance($db); + try { + $maintenance->CleanAvatar(); + $this->assertFileDoesNotExist($abs, 'fixture avatar should have been removed'); + } finally { + $this->removeScratchDir($scratchRel); + } + } + + #[Test] + public function cleanAvatarSkipsTraversalPathButStillDeletesDbRow(): void + { + // Place a real fixture OUTSIDE the upload root so a successful + // traversal would visibly remove it; the test asserts it survives. + $outside = sys_get_temp_dir() . '/xoops_avatar_traversal_target_' . uniqid() . '.png'; + file_put_contents($outside, 'must-not-be-removed'); + + // Compute the relative '../' path from XOOPS_UPLOAD_PATH that + // would resolve to $outside. Even if the relative form is not + // exact, realpath() will either resolve outside the upload root + // (rejected by the prefix check) or fail (also rejected). + $traversalRel = '../../' . basename($outside); + + $db = $this->createMockDatabase(); + $this->stubAvatarSweep($db, [ + ['avatar_id' => 99, 'avatar_file' => $traversalRel], + ]); + // DB row + avatar_user_link cleanup still execute. + $db->expects($this->exactly(2))->method('exec')->willReturn(1); + + $maintenance = $this->createMaintenance($db); + try { + $maintenance->CleanAvatar(); + $this->assertFileExists($outside, 'traversal target outside upload root must not be removed'); + } finally { + @unlink($outside); + } + } + + #[Test] + public function cleanAvatarSkipsAbsolutePathButStillDeletesDbRow(): void + { + // An absolute path stored in avatar_file should not allow the + // cleanup to escape XOOPS_UPLOAD_PATH. The ltrim('/') step in + // CleanAvatar() turns '/etc/hosts' into 'etc/hosts' which is + // then resolved relative to the upload root — and almost + // certainly does not exist. realpath() returns false → skipped. + $db = $this->createMockDatabase(); + $this->stubAvatarSweep($db, [ + ['avatar_id' => 100, 'avatar_file' => '/etc/hosts'], + ]); + $db->expects($this->exactly(2))->method('exec')->willReturn(1); + + $maintenance = $this->createMaintenance($db); + $this->assertFileExists('/etc/hosts', 'pre-test sanity: /etc/hosts should exist'); + $maintenance->CleanAvatar(); + $this->assertFileExists('/etc/hosts', '/etc/hosts must not be removed by avatar cleanup'); + } + + #[Test] + public function cleanAvatarHandlesMissingFileAndStillDeletesDbRow(): void + { + $db = $this->createMockDatabase(); + $this->stubAvatarSweep($db, [ + // File reference that does not exist on disk — realpath() will + // return false. The DB row cleanup must still run. + ['avatar_id' => 200, 'avatar_file' => 'avatars/nonexistent_' . uniqid() . '.png'], + ]); + $db->expects($this->exactly(2))->method('exec')->willReturn(1); + + $maintenance = $this->createMaintenance($db); + $maintenance->CleanAvatar(); // assertion is the exec() call count + } + + #[Test] + public function cleanAvatarHandlesEmptyAvatarFileAndStillDeletesDbRow(): void + { + $db = $this->createMockDatabase(); + $this->stubAvatarSweep($db, [ + ['avatar_id' => 300, 'avatar_file' => ''], + ]); + $db->expects($this->exactly(2))->method('exec')->willReturn(1); + + $maintenance = $this->createMaintenance($db); + $maintenance->CleanAvatar(); + } + + #[Test] + public function cleanAvatarNormalisesBackslashesInAvatarFile(): void + { + // Windows-historic data may store 'avatars\foo.png'. The cleanup + // should normalise it and remove the file under XOOPS_UPLOAD_PATH. + $scratchRel = $this->avatarScratchRel(); + [$rel, $abs] = $this->placeFixtureAvatar($scratchRel, 'win.png'); + // Replace forward slashes with backslashes only in the segment + // separator, NOT inside the scratch directory name (it has '_' + // not '\\'). This is what a Windows-saved row looked like. + $winRel = str_replace('/', '\\', $rel); + + $db = $this->createMockDatabase(); + $this->stubAvatarSweep($db, [ + ['avatar_id' => 400, 'avatar_file' => $winRel], + ]); + $db->expects($this->exactly(2))->method('exec')->willReturn(1); + + $maintenance = $this->createMaintenance($db); + try { + $maintenance->CleanAvatar(); + $this->assertFileDoesNotExist($abs, 'backslash-normalised avatar should be removed'); + } finally { + $this->removeScratchDir($scratchRel); + } + } } From fc1dfd89d95b35933f650438a6a196f21f4ee73a Mon Sep 17 00:00:00 2001 From: Michael Beck Date: Thu, 7 May 2026 19:28:16 -0400 Subject: [PATCH 03/10] =?UTF-8?q?test(system):=20fix=20exec()=20mock=20ret?= =?UTF-8?q?urn=20type=20=E2=80=94=20bool,=20not=20int?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CI run #52 failed with 6 IncompatibleReturnValueException errors: Method exec may not return value of type int, its declared return type is \"bool\" XoopsMySQLDatabase::exec(string \$sql): bool returns true on success and false on failure (or false from PHP's mysqli_query for many DDL statements). The CleanAvatar() tests stubbed exec() with willReturn(1), which works under loose return-type checking but trips PHPUnit 11's strict return-type validation. Fix: all 6 exec() stubs use willReturn(true). --- .../htdocs/modules/system/SystemMaintenanceTest.php | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/tests/unit/htdocs/modules/system/SystemMaintenanceTest.php b/tests/unit/htdocs/modules/system/SystemMaintenanceTest.php index ac8ebda3a..64e9fffaf 100644 --- a/tests/unit/htdocs/modules/system/SystemMaintenanceTest.php +++ b/tests/unit/htdocs/modules/system/SystemMaintenanceTest.php @@ -365,7 +365,7 @@ public function cleanAvatarRemovesValidAvatarFileUnderUploadRoot(): void ]); // exec() called twice: once for DELETE FROM avatar, once for the // avatar_user_link cleanup at the end of CleanAvatar(). - $db->expects($this->exactly(2))->method('exec')->willReturn(1); + $db->expects($this->exactly(2))->method('exec')->willReturn(true); $maintenance = $this->createMaintenance($db); try { @@ -395,7 +395,7 @@ public function cleanAvatarSkipsTraversalPathButStillDeletesDbRow(): void ['avatar_id' => 99, 'avatar_file' => $traversalRel], ]); // DB row + avatar_user_link cleanup still execute. - $db->expects($this->exactly(2))->method('exec')->willReturn(1); + $db->expects($this->exactly(2))->method('exec')->willReturn(true); $maintenance = $this->createMaintenance($db); try { @@ -418,7 +418,7 @@ public function cleanAvatarSkipsAbsolutePathButStillDeletesDbRow(): void $this->stubAvatarSweep($db, [ ['avatar_id' => 100, 'avatar_file' => '/etc/hosts'], ]); - $db->expects($this->exactly(2))->method('exec')->willReturn(1); + $db->expects($this->exactly(2))->method('exec')->willReturn(true); $maintenance = $this->createMaintenance($db); $this->assertFileExists('/etc/hosts', 'pre-test sanity: /etc/hosts should exist'); @@ -435,7 +435,7 @@ public function cleanAvatarHandlesMissingFileAndStillDeletesDbRow(): void // return false. The DB row cleanup must still run. ['avatar_id' => 200, 'avatar_file' => 'avatars/nonexistent_' . uniqid() . '.png'], ]); - $db->expects($this->exactly(2))->method('exec')->willReturn(1); + $db->expects($this->exactly(2))->method('exec')->willReturn(true); $maintenance = $this->createMaintenance($db); $maintenance->CleanAvatar(); // assertion is the exec() call count @@ -448,7 +448,7 @@ public function cleanAvatarHandlesEmptyAvatarFileAndStillDeletesDbRow(): void $this->stubAvatarSweep($db, [ ['avatar_id' => 300, 'avatar_file' => ''], ]); - $db->expects($this->exactly(2))->method('exec')->willReturn(1); + $db->expects($this->exactly(2))->method('exec')->willReturn(true); $maintenance = $this->createMaintenance($db); $maintenance->CleanAvatar(); @@ -470,7 +470,7 @@ public function cleanAvatarNormalisesBackslashesInAvatarFile(): void $this->stubAvatarSweep($db, [ ['avatar_id' => 400, 'avatar_file' => $winRel], ]); - $db->expects($this->exactly(2))->method('exec')->willReturn(1); + $db->expects($this->exactly(2))->method('exec')->willReturn(true); $maintenance = $this->createMaintenance($db); try { From 5fb1d97cf93d3a1fcc8bad4c6393d9b794b9f944 Mon Sep 17 00:00:00 2001 From: Michael Beck Date: Thu, 7 May 2026 19:40:44 -0400 Subject: [PATCH 04/10] test(system): real-resolve traversal fixture; OS-portable abs-path test; (int) avatar_id MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address PR #52 follow-up review (Copilot, CodeRabbit Trivial): modules/system/class/maintenance.php (CleanAvatar): Cast $myrow['avatar_id'] to int once at the top of the loop body and use $avatarId in both DELETE statements (avatar row + the empty-avatar_file fast path). Defence-in-depth on the SQL concat even though the value is DB-origin; also silences SonarCloud's string-concatenation warning on these statements per project SQL hygiene. tests/unit/htdocs/modules/system/SystemMaintenanceTest.php: cleanAvatarSkipsTraversalPathButStillDeletesDbRow: The previous form created the fixture in sys_get_temp_dir() but used '../../' as the avatar_file. That string resolves from htdocs/uploads/ to a path under/near the project root — NOT the temp dir. realpath() returned false, the cleanup was skipped, and the test passed for the wrong reason: it never exercised the containment-prefix branch. New form: place the fixture at dirname(realpath(XOOPS_UPLOAD_PATH)) so '../' actually resolves to it. realpath() now succeeds, and the prefix check is the rejection mechanism. Added an explicit assertSame() sanity assertion comparing the resolved path to realpath($outside) so the test self-documents that it really exercises the containment branch. cleanAvatarSkipsAbsolutePathButStillDeletesDbRow: Replaced the hard-coded /etc/hosts assertion with a temp fixture via tempnam(). Windows CI runners don't have /etc/hosts at the same path, and even on POSIX systems the test relied on a pre-existing system file. tempnam() works on every supported OS. stubAvatarSweep(): Removed the misleading "Returns the mock for further expectations" docblock line. The method is `: void`; callers attach further expectations directly to the $db they passed in. --- htdocs/modules/system/class/maintenance.php | 10 ++- .../modules/system/SystemMaintenanceTest.php | 61 +++++++++++++------ 2 files changed, 51 insertions(+), 20 deletions(-) diff --git a/htdocs/modules/system/class/maintenance.php b/htdocs/modules/system/class/maintenance.php index 1f1055f73..c8ca93806 100644 --- a/htdocs/modules/system/class/maintenance.php +++ b/htdocs/modules/system/class/maintenance.php @@ -195,9 +195,15 @@ public function CleanAvatar() // separator so 'uploadsX/...' doesn't satisfy 'uploads') // - confirm it's a regular file before removal. // Only then invoke the cleanup helper. + // (int) cast on avatar_id is defence-in-depth: the value is + // DB-origin so SQL injection is implausible, but the project + // convention is to never concatenate non-cast values into + // SQL strings. The cast also silences SonarCloud's + // concatenation warning on these DELETE statements. + $avatarId = (int) ($myrow['avatar_id'] ?? 0); $avatarFile = ltrim(str_replace('\\', '/', (string) ($myrow['avatar_file'] ?? '')), '/'); if ('' === $avatarFile) { - $result1 = $this->db->exec('DELETE FROM ' . $this->db->prefix('avatar') . ' WHERE avatar_id=' . $myrow['avatar_id']); + $result1 = $this->db->exec('DELETE FROM ' . $this->db->prefix('avatar') . ' WHERE avatar_id=' . $avatarId); continue; } $avatarPath = realpath(XOOPS_UPLOAD_PATH . '/' . $avatarFile); @@ -210,7 +216,7 @@ public function CleanAvatar() xoops_remove_file_quietly($avatarPath, 'orphaned avatar'); } //clean avatar table - $result1 = $this->db->exec('DELETE FROM ' . $this->db->prefix('avatar') . ' WHERE avatar_id=' . $myrow['avatar_id']); + $result1 = $this->db->exec('DELETE FROM ' . $this->db->prefix('avatar') . ' WHERE avatar_id=' . $avatarId); } //clean any deleted users from avatar_user_link table $result2 = $this->db->exec('DELETE FROM ' . $this->db->prefix('avatar_user_link') . ' WHERE user_id NOT IN (SELECT uid FROM ' . $this->db->prefix('users') . ')'); diff --git a/tests/unit/htdocs/modules/system/SystemMaintenanceTest.php b/tests/unit/htdocs/modules/system/SystemMaintenanceTest.php index 64e9fffaf..fb6000e38 100644 --- a/tests/unit/htdocs/modules/system/SystemMaintenanceTest.php +++ b/tests/unit/htdocs/modules/system/SystemMaintenanceTest.php @@ -343,7 +343,8 @@ private function removeScratchDir(string $scratchRel): void /** * Stub the database mock so $db->query() returns a sentinel result and * $db->fetchArray() yields the supplied avatar rows once, then false. - * Returns the mock for further expectations. + * The caller still holds the mock and can attach further expectations + * directly (this helper does not return it). */ private function stubAvatarSweep($db, array $rows): void { @@ -379,16 +380,23 @@ public function cleanAvatarRemovesValidAvatarFileUnderUploadRoot(): void #[Test] public function cleanAvatarSkipsTraversalPathButStillDeletesDbRow(): void { - // Place a real fixture OUTSIDE the upload root so a successful - // traversal would visibly remove it; the test asserts it survives. - $outside = sys_get_temp_dir() . '/xoops_avatar_traversal_target_' . uniqid() . '.png'; + // To meaningfully exercise the upload-root containment check the + // fixture has to live at the location the traversal string + // ACTUALLY resolves to. dirname(realpath(XOOPS_UPLOAD_PATH)) is + // the parent of the upload root, so a fixture placed there is + // reachable via '../' relative to XOOPS_UPLOAD_PATH. + // Result: + // - realpath() succeeds (the file exists at the parent dir) + // - the prefix check rejects it (not under uploads/) + // - the fixture survives — exercising the containment branch + // Without this setup, realpath() would just return false on a + // non-existent target and the test would pass for the wrong + // reason. + $uploadRoot = realpath(XOOPS_UPLOAD_PATH); + $this->assertIsString($uploadRoot, 'XOOPS_UPLOAD_PATH must resolve via realpath()'); + $outside = dirname($uploadRoot) . DIRECTORY_SEPARATOR . 'xoops_avatar_traversal_target_' . uniqid() . '.png'; file_put_contents($outside, 'must-not-be-removed'); - - // Compute the relative '../' path from XOOPS_UPLOAD_PATH that - // would resolve to $outside. Even if the relative form is not - // exact, realpath() will either resolve outside the upload root - // (rejected by the prefix check) or fail (also rejected). - $traversalRel = '../../' . basename($outside); + $traversalRel = '../' . basename($outside); $db = $this->createMockDatabase(); $this->stubAvatarSweep($db, [ @@ -399,6 +407,13 @@ public function cleanAvatarSkipsTraversalPathButStillDeletesDbRow(): void $maintenance = $this->createMaintenance($db); try { + // Sanity: realpath of the traversal path resolves to the + // fixture (i.e. realpath() succeeds) — proving this test + // really exercises the prefix-check branch and not the + // realpath()-returns-false short-circuit. + $resolved = realpath(XOOPS_UPLOAD_PATH . '/' . $traversalRel); + $this->assertSame(realpath($outside), $resolved, 'traversal must actually resolve to the fixture'); + $maintenance->CleanAvatar(); $this->assertFileExists($outside, 'traversal target outside upload root must not be removed'); } finally { @@ -410,20 +425,30 @@ public function cleanAvatarSkipsTraversalPathButStillDeletesDbRow(): void public function cleanAvatarSkipsAbsolutePathButStillDeletesDbRow(): void { // An absolute path stored in avatar_file should not allow the - // cleanup to escape XOOPS_UPLOAD_PATH. The ltrim('/') step in - // CleanAvatar() turns '/etc/hosts' into 'etc/hosts' which is - // then resolved relative to the upload root — and almost - // certainly does not exist. realpath() returns false → skipped. + // cleanup to escape XOOPS_UPLOAD_PATH. Use a temp fixture rather + // than hard-coding /etc/hosts so the test runs on every OS + // (Windows CI doesn't have /etc/hosts at the same location). + // The ltrim('/') step turns '/abs/path/foo.png' into + // 'abs/path/foo.png' which is then resolved relative to the + // upload root — typically to a non-existent path, so realpath() + // returns false and the cleanup is skipped. + $outside = tempnam(sys_get_temp_dir(), 'xoops_avatar_absolute_'); + $this->assertNotFalse($outside, 'tempnam should succeed'); + file_put_contents($outside, 'must-not-be-removed'); + $db = $this->createMockDatabase(); $this->stubAvatarSweep($db, [ - ['avatar_id' => 100, 'avatar_file' => '/etc/hosts'], + ['avatar_id' => 100, 'avatar_file' => $outside], ]); $db->expects($this->exactly(2))->method('exec')->willReturn(true); $maintenance = $this->createMaintenance($db); - $this->assertFileExists('/etc/hosts', 'pre-test sanity: /etc/hosts should exist'); - $maintenance->CleanAvatar(); - $this->assertFileExists('/etc/hosts', '/etc/hosts must not be removed by avatar cleanup'); + try { + $maintenance->CleanAvatar(); + $this->assertFileExists($outside, 'absolute-path fixture must not be removed by avatar cleanup'); + } finally { + @unlink($outside); + } } #[Test] From bc04df777f0a4dbe266d7cbfba1c09ec5d02c446 Mon Sep 17 00:00:00 2001 From: Michael Beck Date: Thu, 7 May 2026 20:11:45 -0400 Subject: [PATCH 05/10] fix/test(system): chmod helper; narrow CleanAvatar to avatars/ subdir MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address PR #52 follow-up review (Copilot, 3 actionable): cp_functions.php: + xoops_chmod_quietly($path, $perms, $context): new helper mirroring xoops_remove_file_quietly(). Suppresses chmod()'s native PHP warning via the same scoped error_reporting() toggle + try/finally and emits a single project-standard trigger_error on the boolean false return. Without this, a single chmod failure produced TWO log lines (PHP warning + trigger_error). - The chmod() + trigger_error block in xoops_write_file_atomically() replaced with the helper. modules/system/class/maintenance.php: - chmod() + trigger_error block in writeFileWithWarning() replaced with the same helper (already require_once'ing cp_functions.php). - CleanAvatar(): containment check narrowed from realpath(XOOPS_UPLOAD_PATH) to realpath(XOOPS_UPLOAD_PATH . '/avatars'). Custom avatars are stored as 'avatars/' (kernel/avatar.php and 14 admin/edituser writers), so the broad upload-root check would have allowed deletion of any file under uploads/ if avatar_file pointed elsewhere — legacy data, custom- module write, or accidental insertion. The narrow prefix is defence-in-depth: skip rather than silently delete unrelated uploads. Hoisted out of the loop so the realpath() resolution is computed once per sweep. tests/unit/htdocs/modules/system/SystemMaintenanceTest.php: + cleanAvatarSkipsNonAvatarsSubdirUnderUploadRoot: places a fixture at uploads/files/.doc and sets avatar_file to 'files/.doc'. Asserts the resolved path is under uploads/ (proving the test exercises the narrow-prefix branch, not the realpath-fail branch), runs CleanAvatar(), and verifies the fixture survives — codifying the narrowing as a regression test. No @ operators remain in either file's unlink/chmod paths; the seven @rename(...) calls inside `if (!...)` checks are still retained with the documented rationale. --- htdocs/include/cp_functions.php | 48 +++++++++++++---- htdocs/modules/system/class/maintenance.php | 48 +++++++++-------- .../modules/system/SystemMaintenanceTest.php | 51 +++++++++++++++++++ 3 files changed, 116 insertions(+), 31 deletions(-) diff --git a/htdocs/include/cp_functions.php b/htdocs/include/cp_functions.php index 2209b9de8..368459ce5 100644 --- a/htdocs/include/cp_functions.php +++ b/htdocs/include/cp_functions.php @@ -120,6 +120,39 @@ function xoops_file_label($filename) return basename($filename); } +/** + * Set file permissions, suppressing the native PHP warning on failure + * via the same scoped error_reporting() toggle used by + * xoops_remove_file_quietly(). Without this, a chmod() failure produces + * TWO log lines: the native PHP warning AND the project's own + * trigger_error(). The helper consolidates them into a single + * project-standard warning based on the boolean return value. + * + * @param string $path Absolute path to the file. + * @param int $perms Permission bits (octal). + * @param string $context Short label used in the warning message + * (e.g. 'temp', 'temp guard'). + * + * @return bool True on success, false on failure (warning already emitted). + */ +function xoops_chmod_quietly($path, $perms, $context = 'temp') +{ + $previousLevel = error_reporting(0); + try { + $ok = chmod($path, $perms); + } finally { + error_reporting($previousLevel); + } + if (!$ok) { + trigger_error( + sprintf('Failed to set permissions on %s file: %s', $context, xoops_file_label($path)), + E_USER_WARNING + ); + } + + return $ok; +} + /** * Best-effort file removal used by atomic-write cleanup paths and similar * fire-and-forget cleanup. Skips non-existent paths so already-deleted @@ -200,15 +233,12 @@ function xoops_write_file_atomically($filename, $content) $targetPerms = $currentPerms & 0777; } } - if (!chmod($tempFile, $targetPerms)) { - // Non-fatal: file is written, only the perms didn't take. Continue - // with the rename rather than aborting — most callers care about - // content integrity over exact perms. - trigger_error( - sprintf('Failed to set permissions on temp file for %s', $label), - E_USER_WARNING - ); - } + // Non-fatal: file is written, only the perms may not take. Continue + // with the rename rather than aborting — most callers care about + // content integrity over exact perms. The helper suppresses the + // native PHP warning so a single failure produces a single + // project-standard log line. + xoops_chmod_quietly($tempFile, $targetPerms, 'temp'); // The four @rename(...) calls below are inside `if (!...)` checks — // failure is detected by the boolean return and reported via diff --git a/htdocs/modules/system/class/maintenance.php b/htdocs/modules/system/class/maintenance.php index c8ca93806..625c3eea9 100644 --- a/htdocs/modules/system/class/maintenance.php +++ b/htdocs/modules/system/class/maintenance.php @@ -172,27 +172,34 @@ public function CleanAvatar() ); } - // Resolve the upload-root once for the whole sweep. Both realpath() - // and the trailing-separator prefix are constant for the run, so - // computing them per row would do unnecessary filesystem work on - // installations with large numbers of orphaned avatars. - $uploadRoot = realpath(XOOPS_UPLOAD_PATH); - $uploadRootPrefix = is_string($uploadRoot) - ? rtrim($uploadRoot, DIRECTORY_SEPARATOR) . DIRECTORY_SEPARATOR + // Resolve the avatars subdirectory once for the whole sweep. + // Custom avatars live under XOOPS_UPLOAD_PATH/avatars/ (see + // kernel/avatar.php and the various admin/edituser writers, all + // of which prepend 'avatars/' to the stored filename). Narrowing + // the containment check to this subtree is defence-in-depth: an + // avatar_file value that points elsewhere under uploads/ — + // legacy data, custom-module write, or accidental insertion — + // is now skipped instead of silently deleting an unrelated + // upload. realpath() is constant for the run, so per-row + // resolution would just be wasted filesystem work on + // installations with large orphaned-avatar tables. + $avatarRoot = realpath(XOOPS_UPLOAD_PATH . '/avatars'); + $avatarRootPrefix = is_string($avatarRoot) + ? rtrim($avatarRoot, DIRECTORY_SEPARATOR) . DIRECTORY_SEPARATOR : null; /** @var array $myrow */ while (false !== ($myrow = $this->db->fetchArray($result))) { - // Avatar files are stored as 'avatars/' (see - // kernel/avatar.php and the various admin/edituser writers), - // so basename() would silently bypass cleanup. Instead: + // Avatar files are stored as 'avatars/' + // (kernel/avatar.php and 14 admin/edituser writers), so + // basename() would silently bypass cleanup. Instead: // - normalise backslashes to '/' (Windows-historic data) // - strip leading slashes (defends against absolute paths // accidentally or maliciously stored in the column) // - resolve via realpath() so any '../' segments collapse // - confirm the resolved path is contained inside the - // resolved upload-root (prefix check with a trailing - // separator so 'uploadsX/...' doesn't satisfy 'uploads') + // resolved avatars/ subdir (trailing-separator prefix + // so 'avatarsX/...' doesn't satisfy 'avatars') // - confirm it's a regular file before removal. // Only then invoke the cleanup helper. // (int) cast on avatar_id is defence-in-depth: the value is @@ -209,8 +216,8 @@ public function CleanAvatar() $avatarPath = realpath(XOOPS_UPLOAD_PATH . '/' . $avatarFile); if ( is_string($avatarPath) - && is_string($uploadRootPrefix) - && str_starts_with($avatarPath, $uploadRootPrefix) + && is_string($avatarRootPrefix) + && str_starts_with($avatarPath, $avatarRootPrefix) && is_file($avatarPath) ) { xoops_remove_file_quietly($avatarPath, 'orphaned avatar'); @@ -287,14 +294,11 @@ private function writeFileWithWarning($filename, $content) $targetPerms = $currentPerms & 0777; } } - if (!chmod($tempFile, $targetPerms)) { - // Non-fatal: content is written, only the perms didn't - // take. Continue with the rename rather than aborting. - trigger_error( - sprintf('Failed to set permissions on temp guard file for %s', $label), - E_USER_WARNING - ); - } + // Non-fatal: content is written, only the perms may not + // take. Continue with the rename rather than aborting. The + // helper suppresses the native PHP warning so a single + // failure produces a single project-standard log line. + xoops_chmod_quietly($tempFile, $targetPerms, 'temp guard'); // The @rename(...) calls below are inside `if (!...)` checks — // failure is detected by the boolean return and reported via diff --git a/tests/unit/htdocs/modules/system/SystemMaintenanceTest.php b/tests/unit/htdocs/modules/system/SystemMaintenanceTest.php index fb6000e38..9be7e1e84 100644 --- a/tests/unit/htdocs/modules/system/SystemMaintenanceTest.php +++ b/tests/unit/htdocs/modules/system/SystemMaintenanceTest.php @@ -479,6 +479,57 @@ public function cleanAvatarHandlesEmptyAvatarFileAndStillDeletesDbRow(): void $maintenance->CleanAvatar(); } + #[Test] + public function cleanAvatarSkipsNonAvatarsSubdirUnderUploadRoot(): void + { + // Defence-in-depth: even an avatar_file value that points to a + // legitimate path UNDER XOOPS_UPLOAD_PATH but OUTSIDE the + // avatars/ subtree must not be removed by the avatar sweep. + // Place a fixture at uploads/files/.doc, set avatar_file + // to that path, verify the file survives. + $filesDir = XOOPS_UPLOAD_PATH . '/files'; + $created = false; + if (!is_dir($filesDir)) { + if (!mkdir($filesDir, 0755, true) && !is_dir($filesDir)) { + $this->fail('Could not create test files dir: ' . $filesDir); + } + $created = true; + } + $fixtureName = '_test_nonavatar_' . getmypid() . '_' . uniqid() . '.doc'; + $fixturePath = $filesDir . '/' . $fixtureName; + file_put_contents($fixturePath, 'must-not-be-removed'); + $rel = 'files/' . $fixtureName; + + $db = $this->createMockDatabase(); + $this->stubAvatarSweep($db, [ + ['avatar_id' => 500, 'avatar_file' => $rel], + ]); + $db->expects($this->exactly(2))->method('exec')->willReturn(true); + + $maintenance = $this->createMaintenance($db); + try { + // Sanity: the resolved path IS under uploads/ but NOT under + // uploads/avatars/, so the broad upload-root check would + // have allowed deletion. Asserting the resolution succeeds + // proves the test really exercises the narrow prefix branch. + $resolved = realpath($fixturePath); + $this->assertIsString($resolved, 'fixture should resolve'); + $this->assertStringStartsWith( + rtrim((string) realpath(XOOPS_UPLOAD_PATH), DIRECTORY_SEPARATOR) . DIRECTORY_SEPARATOR, + $resolved, + 'fixture must be inside uploads/' + ); + + $maintenance->CleanAvatar(); + $this->assertFileExists($fixturePath, 'non-avatar file under uploads/ must not be removed'); + } finally { + @unlink($fixturePath); + if ($created) { + @rmdir($filesDir); + } + } + } + #[Test] public function cleanAvatarNormalisesBackslashesInAvatarFile(): void { From 1f4622c6bbbd16d524f8b513cd8339f18f018ac5 Mon Sep 17 00:00:00 2001 From: Michael Beck Date: Thu, 7 May 2026 20:35:37 -0400 Subject: [PATCH 06/10] test(system): assert fixture writes succeed before exercising CleanAvatar MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address PR #52 follow-up review (Copilot, line 316): placeFixtureAvatar() and the three inline file_put_contents() sites in cleanAvatar* tests previously ignored the return value. If the write failed (FS permissions, full disk, etc.) the happy-path tests could pass for the wrong reason — CleanAvatar() would find no file to delete and assertFileDoesNotExist() would succeed even though the cleanup never ran on a real fixture. All four fixture-creation sites now: - capture file_put_contents() return into $bytesWritten - assertNotFalse with a path-suffixed message for triage - assertSame(strlen(content), $bytesWritten) so partial writes are caught too, not just false-return failures Sites covered: - placeFixtureAvatar() (the one Copilot flagged) - cleanAvatarSkipsTraversalPathButStillDeletesDbRow - cleanAvatarSkipsAbsolutePathButStillDeletesDbRow - cleanAvatarSkipsNonAvatarsSubdirUnderUploadRoot --- .../modules/system/SystemMaintenanceTest.php | 23 +++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-) diff --git a/tests/unit/htdocs/modules/system/SystemMaintenanceTest.php b/tests/unit/htdocs/modules/system/SystemMaintenanceTest.php index 9be7e1e84..a501cdcdc 100644 --- a/tests/unit/htdocs/modules/system/SystemMaintenanceTest.php +++ b/tests/unit/htdocs/modules/system/SystemMaintenanceTest.php @@ -313,7 +313,16 @@ private function placeFixtureAvatar(string $scratchRel, string $filename): array $this->fail('Could not create test scratch dir: ' . $scratchAbs); } $absPath = $scratchAbs . '/' . $filename; - file_put_contents($absPath, 'fixture'); + // Assert the fixture write actually succeeded with the expected + // byte count — otherwise a happy-path test could later "pass" + // because CleanAvatar() found no file to delete + // (assertFileDoesNotExist would succeed even though the cleanup + // never ran on a real fixture). Checking the byte count also + // catches partial-write failures, not just "false return". + $bytesWritten = file_put_contents($absPath, 'fixture'); + $this->assertNotFalse($bytesWritten, 'Could not write fixture avatar: ' . $absPath); + $this->assertSame(strlen('fixture'), $bytesWritten); + $this->assertFileExists($absPath); $relPath = $scratchRel . '/' . $filename; return [$relPath, $absPath]; @@ -395,7 +404,9 @@ public function cleanAvatarSkipsTraversalPathButStillDeletesDbRow(): void $uploadRoot = realpath(XOOPS_UPLOAD_PATH); $this->assertIsString($uploadRoot, 'XOOPS_UPLOAD_PATH must resolve via realpath()'); $outside = dirname($uploadRoot) . DIRECTORY_SEPARATOR . 'xoops_avatar_traversal_target_' . uniqid() . '.png'; - file_put_contents($outside, 'must-not-be-removed'); + $bytesWritten = file_put_contents($outside, 'must-not-be-removed'); + $this->assertNotFalse($bytesWritten, 'Could not write traversal fixture: ' . $outside); + $this->assertSame(strlen('must-not-be-removed'), $bytesWritten); $traversalRel = '../' . basename($outside); $db = $this->createMockDatabase(); @@ -434,7 +445,9 @@ public function cleanAvatarSkipsAbsolutePathButStillDeletesDbRow(): void // returns false and the cleanup is skipped. $outside = tempnam(sys_get_temp_dir(), 'xoops_avatar_absolute_'); $this->assertNotFalse($outside, 'tempnam should succeed'); - file_put_contents($outside, 'must-not-be-removed'); + $bytesWritten = file_put_contents($outside, 'must-not-be-removed'); + $this->assertNotFalse($bytesWritten, 'Could not write absolute-path fixture: ' . $outside); + $this->assertSame(strlen('must-not-be-removed'), $bytesWritten); $db = $this->createMockDatabase(); $this->stubAvatarSweep($db, [ @@ -497,7 +510,9 @@ public function cleanAvatarSkipsNonAvatarsSubdirUnderUploadRoot(): void } $fixtureName = '_test_nonavatar_' . getmypid() . '_' . uniqid() . '.doc'; $fixturePath = $filesDir . '/' . $fixtureName; - file_put_contents($fixturePath, 'must-not-be-removed'); + $bytesWritten = file_put_contents($fixturePath, 'must-not-be-removed'); + $this->assertNotFalse($bytesWritten, 'Could not write non-avatar fixture: ' . $fixturePath); + $this->assertSame(strlen('must-not-be-removed'), $bytesWritten); $rel = 'files/' . $fixtureName; $db = $this->createMockDatabase(); From 87e6cc58761adb7d901387d446b55478e4ecb465 Mon Sep 17 00:00:00 2001 From: Michael Beck Date: Fri, 8 May 2026 16:28:17 -0400 Subject: [PATCH 07/10] fix(system): defensive $ok init; drop unused $result1; @throws docblock MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address PR #52 follow-up review (CodeRabbit, 3 findings): cp_functions.php (xoops_chmod_quietly, xoops_remove_file_quietly): Initialise $ok = false before each try block. error_reporting(0) does NOT disable user-defined error handlers; only the native warning. A handler that throws (e.g. a strict ErrorException conversion that doesn't check error_reporting() & $errno) would propagate out of the try block before chmod()/unlink() returns, leaving $ok unset and triggering an "Undefined variable" warning on the subsequent if (!$ok) check. The defensive default makes the helpers safe under any error-handler shape. modules/system/class/maintenance.php (CleanAvatar): Drop the unused `$result1 = ` capture on both $this->db->exec() calls. PHPMD flagged this; the value was never read so the assignment was misleading (no error propagation either way). Removing makes the discard explicit. Also added `@throws \RuntimeException` to the CleanAvatar() docblock per project convention — the method does throw RuntimeException when the avatar query fails, but the PHPDoc was silent about it. --- htdocs/include/cp_functions.php | 10 ++++++++++ htdocs/modules/system/class/maintenance.php | 6 ++++-- 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/htdocs/include/cp_functions.php b/htdocs/include/cp_functions.php index 368459ce5..ebc628c32 100644 --- a/htdocs/include/cp_functions.php +++ b/htdocs/include/cp_functions.php @@ -137,6 +137,12 @@ function xoops_file_label($filename) */ function xoops_chmod_quietly($path, $perms, $context = 'temp') { + // Initialise $ok before the try block: error_reporting(0) does NOT + // disable user-defined error handlers, only the native warning. A + // naively written handler that always throws (without checking + // error_reporting() & $errno) would propagate out of the try block + // before chmod() returns, leaving $ok unset. Defensive default. + $ok = false; $previousLevel = error_reporting(0); try { $ok = chmod($path, $perms); @@ -172,6 +178,10 @@ function xoops_remove_file_quietly($path, $context = 'temporary') if (!file_exists($path)) { return; } + // Initialise $ok defensively — see xoops_chmod_quietly() for the + // rationale (error_reporting(0) does not disable user-defined + // error handlers). + $ok = false; $previousLevel = error_reporting(0); try { $ok = unlink($path); diff --git a/htdocs/modules/system/class/maintenance.php b/htdocs/modules/system/class/maintenance.php index 625c3eea9..6a2026d35 100644 --- a/htdocs/modules/system/class/maintenance.php +++ b/htdocs/modules/system/class/maintenance.php @@ -160,6 +160,8 @@ public function CleanSession() * @author slider84 of Team FrXoops * * @return boolean + * + * @throws \RuntimeException If the avatar table query fails. */ public function CleanAvatar() { @@ -210,7 +212,7 @@ public function CleanAvatar() $avatarId = (int) ($myrow['avatar_id'] ?? 0); $avatarFile = ltrim(str_replace('\\', '/', (string) ($myrow['avatar_file'] ?? '')), '/'); if ('' === $avatarFile) { - $result1 = $this->db->exec('DELETE FROM ' . $this->db->prefix('avatar') . ' WHERE avatar_id=' . $avatarId); + $this->db->exec('DELETE FROM ' . $this->db->prefix('avatar') . ' WHERE avatar_id=' . $avatarId); continue; } $avatarPath = realpath(XOOPS_UPLOAD_PATH . '/' . $avatarFile); @@ -223,7 +225,7 @@ public function CleanAvatar() xoops_remove_file_quietly($avatarPath, 'orphaned avatar'); } //clean avatar table - $result1 = $this->db->exec('DELETE FROM ' . $this->db->prefix('avatar') . ' WHERE avatar_id=' . $avatarId); + $this->db->exec('DELETE FROM ' . $this->db->prefix('avatar') . ' WHERE avatar_id=' . $avatarId); } //clean any deleted users from avatar_user_link table $result2 = $this->db->exec('DELETE FROM ' . $this->db->prefix('avatar_user_link') . ' WHERE user_id NOT IN (SELECT uid FROM ' . $this->db->prefix('users') . ')'); From 4eaf734ca9b9f98580e970635fb5b342708a6a94 Mon Sep 17 00:00:00 2001 From: Michael Beck Date: Fri, 8 May 2026 16:36:50 -0400 Subject: [PATCH 08/10] fix(system): direct-access guard; cleanup dangling symlinks too MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address PR #52 follow-up review (Copilot, 2 findings — CodeRabbit APPROVED): modules/system/class/maintenance.php: Add a `defined('XOOPS_ROOT_PATH') || exit()` guard before the require_once. The previous code commented out the guard but immediately uses XOOPS_ROOT_PATH in require_once, so direct access (or any include before bootstrap) would fail with an undefined-constant fatal that leaks server path detail in the error message. The guard makes direct access a clean exit. include/cp_functions.php (xoops_remove_file_quietly): file_exists() returns false for broken symlinks, so a dangling symlink would skip both the early-return guard AND the post-unlink existence check — the cleanup helper would never attempt removal and never warn that it was left behind. Add `|| is_link($path)` to both checks: unlink() can remove broken symlinks just fine, and the targets they point to are not what this helper is responsible for. --- htdocs/include/cp_functions.php | 9 +++++++-- htdocs/modules/system/class/maintenance.php | 11 ++++++++--- 2 files changed, 15 insertions(+), 5 deletions(-) diff --git a/htdocs/include/cp_functions.php b/htdocs/include/cp_functions.php index ebc628c32..3c910e1fe 100644 --- a/htdocs/include/cp_functions.php +++ b/htdocs/include/cp_functions.php @@ -175,7 +175,12 @@ function xoops_chmod_quietly($path, $perms, $context = 'temp') */ function xoops_remove_file_quietly($path, $context = 'temporary') { - if (!file_exists($path)) { + // file_exists() returns false for broken symlinks, so a dangling + // symlink would be skipped here and also bypass the post-unlink + // existence check below — leaving the orphaned link in place. Treat + // links as existing too: unlink() can remove broken symlinks just + // fine, and the targets they point to are not what we care about. + if (!file_exists($path) && !is_link($path)) { return; } // Initialise $ok defensively — see xoops_chmod_quietly() for the @@ -188,7 +193,7 @@ function xoops_remove_file_quietly($path, $context = 'temporary') } finally { error_reporting($previousLevel); } - if (!$ok && file_exists($path)) { + if (!$ok && (file_exists($path) || is_link($path))) { trigger_error( sprintf('Failed to remove %s file: %s', $context, xoops_file_label($path)), E_USER_WARNING diff --git a/htdocs/modules/system/class/maintenance.php b/htdocs/modules/system/class/maintenance.php index 6a2026d35..d061cb84e 100644 --- a/htdocs/modules/system/class/maintenance.php +++ b/htdocs/modules/system/class/maintenance.php @@ -15,9 +15,14 @@ * @package system */ -//if (!defined('XOOPS_ROOT_PATH')) { -// throw new \RuntimeException('XOOPS root path not defined'); -//} +// Direct-access guard: this file is module/admin-only and must never +// execute outside a bootstrapped XOOPS context. Without this, the +// require_once below would fail with an "undefined constant" fatal +// when the file is hit via a direct URL, leaking server path details +// in the error message. +if (!defined('XOOPS_ROOT_PATH')) { + exit(); +} // xoops_remove_file_quietly() lives in cp_functions.php; admin and install // callers normally load it via cp_header.php / page_moduleinstaller.php, From efc6796058268e14357a7c72e2d91b57a836c02e Mon Sep 17 00:00:00 2001 From: Michael Beck Date: Sun, 10 May 2026 14:28:34 -0400 Subject: [PATCH 09/10] fix(system): symlink-safe CleanAvatar; project-standard direct-access guard MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address PR #52 follow-up review (Copilot, 4 findings): modules/system/class/maintenance.php: Direct-access guard (line 24): Switched from `if (!defined(...)) { exit(); }` to the project- standard one-liner: defined('XOOPS_ROOT_PATH') || exit('Restricted access'); Consistent with the other guards across the codebase, and the 'Restricted access' message is informative for log triage. CleanAvatar (symlink safety, lines 232-241): Previous code called realpath() on the full avatar path, then used the resolved path for both containment validation AND for the unlink. That's unsafe: realpath() follows symlinks, so an avatar entry that's a symlink within uploads/avatars/ would resolve to its target and the unlink would delete the wrong file (potentially another avatar) while leaving the symlink in place. New form keeps two paths: - $avatarCandidate = original path (used for is_file/is_link and the unlink itself — operates on the symlink, not the target) - $avatarParent = realpath(dirname($candidate)) (used ONLY for containment validation — resolves the parent directory's location without resolving the symlink itself) is_file() also gained an `|| is_link()` so symlinks within the avatars/ subtree are now eligible for cleanup. Smoke-tested: a symlink at uploads/avatars/ pointing to uploads/avatars/ is now correctly cleaned up (the symlink is removed, the target survives). PHPDoc (line 167): @return boolean → @return bool for consistency with the rest of the file. --- htdocs/modules/system/class/maintenance.php | 42 +++++++++++++-------- 1 file changed, 26 insertions(+), 16 deletions(-) diff --git a/htdocs/modules/system/class/maintenance.php b/htdocs/modules/system/class/maintenance.php index d061cb84e..db25d1dc9 100644 --- a/htdocs/modules/system/class/maintenance.php +++ b/htdocs/modules/system/class/maintenance.php @@ -19,10 +19,9 @@ // execute outside a bootstrapped XOOPS context. Without this, the // require_once below would fail with an "undefined constant" fatal // when the file is hit via a direct URL, leaking server path details -// in the error message. -if (!defined('XOOPS_ROOT_PATH')) { - exit(); -} +// in the error message. Uses the project-standard one-liner shape +// for consistency with the rest of the codebase. +defined('XOOPS_ROOT_PATH') || exit('Restricted access'); // xoops_remove_file_quietly() lives in cp_functions.php; admin and install // callers normally load it via cp_header.php / page_moduleinstaller.php, @@ -164,7 +163,7 @@ public function CleanSession() * * @author slider84 of Team FrXoops * - * @return boolean + * @return bool * * @throws \RuntimeException If the avatar table query fails. */ @@ -203,12 +202,22 @@ public function CleanAvatar() // - normalise backslashes to '/' (Windows-historic data) // - strip leading slashes (defends against absolute paths // accidentally or maliciously stored in the column) - // - resolve via realpath() so any '../' segments collapse - // - confirm the resolved path is contained inside the - // resolved avatars/ subdir (trailing-separator prefix - // so 'avatarsX/...' doesn't satisfy 'avatars') - // - confirm it's a regular file before removal. - // Only then invoke the cleanup helper. + // - resolve the parent directory via realpath() so any + // '../' segments collapse, and confirm the parent is + // inside the resolved avatars/ subdir (trailing- + // separator prefix so 'avatarsX/...' doesn't satisfy + // 'avatars') + // - confirm the candidate is a regular file OR a symlink + // before removal. + // Then invoke the cleanup helper on the ORIGINAL candidate + // path (not the realpath result). This matters when the + // avatar entry is a symlink: realpath() resolves to the + // symlink target, so unlinking the resolved path would + // delete the target file (potentially another avatar) + // instead of the symlink itself. Resolving the PARENT only + // keeps the containment check honest without following the + // symlink into the wrong file. + // // (int) cast on avatar_id is defence-in-depth: the value is // DB-origin so SQL injection is implausible, but the project // convention is to never concatenate non-cast values into @@ -220,14 +229,15 @@ public function CleanAvatar() $this->db->exec('DELETE FROM ' . $this->db->prefix('avatar') . ' WHERE avatar_id=' . $avatarId); continue; } - $avatarPath = realpath(XOOPS_UPLOAD_PATH . '/' . $avatarFile); + $avatarCandidate = XOOPS_UPLOAD_PATH . '/' . $avatarFile; + $avatarParent = realpath(dirname($avatarCandidate)); if ( - is_string($avatarPath) + is_string($avatarParent) && is_string($avatarRootPrefix) - && str_starts_with($avatarPath, $avatarRootPrefix) - && is_file($avatarPath) + && str_starts_with(rtrim($avatarParent, DIRECTORY_SEPARATOR) . DIRECTORY_SEPARATOR, $avatarRootPrefix) + && (is_file($avatarCandidate) || is_link($avatarCandidate)) ) { - xoops_remove_file_quietly($avatarPath, 'orphaned avatar'); + xoops_remove_file_quietly($avatarCandidate, 'orphaned avatar'); } //clean avatar table $this->db->exec('DELETE FROM ' . $this->db->prefix('avatar') . ' WHERE avatar_id=' . $avatarId); From 4ff281d039e5d76f6bb4466c926097123ab3f4d7 Mon Sep 17 00:00:00 2001 From: Michael Beck Date: Sun, 10 May 2026 14:55:54 -0400 Subject: [PATCH 10/10] fix(system): propagate CleanAvatar DELETE failures; restore-status in writeFileWithWarning MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit modules/system/class/maintenance.php: CleanAvatar() — return contract: The method's @return bool was always returning true regardless of whether any DELETE actually succeeded. If a row delete or the final avatar_user_link cleanup failed, the orphaned row was left behind and the caller had no way to know. Track a $deleteOk flag across all three exec() sites (empty-avatar_file branch, regular avatar-row branch, final avatar_user_link cleanup) and return it. Existing CleanAvatar tests stub exec() with willReturn(true) so $deleteOk stays true and the return value is unchanged in the happy path. writeFileWithWarning() — restore observability: When the rename($tempFile, $filename) replace step fails but the rename($backupFile, $filename) restore step succeeds, the log previously said only "Failed to replace guard file: ", leaving operators unsure whether manual recovery was needed. Track $restoredBackup and fold the status into the existing composite failure warning: "Failed to replace guard file: (original restored)" when the restore worked, or the unsuffixed form (plus the pre-existing "Failed to restore original guard file" line) when both steps failed and manual intervention is needed. No additional success warnings introduced. --- htdocs/modules/system/class/maintenance.php | 38 ++++++++++++++++++--- 1 file changed, 33 insertions(+), 5 deletions(-) diff --git a/htdocs/modules/system/class/maintenance.php b/htdocs/modules/system/class/maintenance.php index db25d1dc9..d34e5b420 100644 --- a/htdocs/modules/system/class/maintenance.php +++ b/htdocs/modules/system/class/maintenance.php @@ -194,6 +194,13 @@ public function CleanAvatar() ? rtrim($avatarRoot, DIRECTORY_SEPARATOR) . DIRECTORY_SEPARATOR : null; + // Track whether every DELETE in the sweep succeeded. The method + // contract is `@return bool`, but the previous implementation + // unconditionally returned true even if a DELETE failed and an + // orphaned row was left behind. Callers can now distinguish a + // clean sweep from a partial one. + $deleteOk = true; + /** @var array $myrow */ while (false !== ($myrow = $this->db->fetchArray($result))) { // Avatar files are stored as 'avatars/' @@ -226,7 +233,9 @@ public function CleanAvatar() $avatarId = (int) ($myrow['avatar_id'] ?? 0); $avatarFile = ltrim(str_replace('\\', '/', (string) ($myrow['avatar_file'] ?? '')), '/'); if ('' === $avatarFile) { - $this->db->exec('DELETE FROM ' . $this->db->prefix('avatar') . ' WHERE avatar_id=' . $avatarId); + if (!$this->db->exec('DELETE FROM ' . $this->db->prefix('avatar') . ' WHERE avatar_id=' . $avatarId)) { + $deleteOk = false; + } continue; } $avatarCandidate = XOOPS_UPLOAD_PATH . '/' . $avatarFile; @@ -240,12 +249,16 @@ public function CleanAvatar() xoops_remove_file_quietly($avatarCandidate, 'orphaned avatar'); } //clean avatar table - $this->db->exec('DELETE FROM ' . $this->db->prefix('avatar') . ' WHERE avatar_id=' . $avatarId); + if (!$this->db->exec('DELETE FROM ' . $this->db->prefix('avatar') . ' WHERE avatar_id=' . $avatarId)) { + $deleteOk = false; + } } //clean any deleted users from avatar_user_link table - $result2 = $this->db->exec('DELETE FROM ' . $this->db->prefix('avatar_user_link') . ' WHERE user_id NOT IN (SELECT uid FROM ' . $this->db->prefix('users') . ')'); + if (!$this->db->exec('DELETE FROM ' . $this->db->prefix('avatar_user_link') . ' WHERE user_id NOT IN (SELECT uid FROM ' . $this->db->prefix('users') . ')')) { + $deleteOk = false; + } - return true; + return $deleteOk; } /** @@ -344,13 +357,28 @@ private function writeFileWithWarning($filename, $content) if (!@rename($tempFile, $filename)) { xoops_remove_file_quietly($tempFile, 'temp guard'); + // Track whether the backup-restore step succeeded so the + // composite failure warning can communicate that the + // original guard file is intact (vs. the worse case + // where both replace and restore failed and manual + // intervention may be required). + $restoredBackup = false; if ($backupFile !== null) { if (!@rename($backupFile, $filename)) { trigger_error(sprintf('Failed to restore original guard file: %s', $label), E_USER_WARNING); + } else { + $restoredBackup = true; } } - trigger_error(sprintf('Failed to replace guard file: %s', $label), E_USER_WARNING); + trigger_error( + sprintf( + 'Failed to replace guard file: %s%s', + $label, + $restoredBackup ? ' (original restored)' : '' + ), + E_USER_WARNING + ); } elseif ($backupFile !== null) { xoops_remove_file_quietly($backupFile, 'backup guard'); }