diff --git a/htdocs/include/cp_functions.php b/htdocs/include/cp_functions.php index 53574653b..3c910e1fe 100644 --- a/htdocs/include/cp_functions.php +++ b/htdocs/include/cp_functions.php @@ -120,6 +120,87 @@ 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') +{ + // 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); + } 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 + * 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') +{ + // 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 + // rationale (error_reporting(0) does not disable user-defined + // error handlers). + $ok = false; + $previousLevel = error_reporting(0); + try { + $ok = unlink($path); + } finally { + error_reporting($previousLevel); + } + 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 + ); + } +} + /** * Write a file through a temporary sibling and replace the target on success. * @@ -141,13 +222,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 +248,22 @@ function xoops_write_file_atomically($filename, $content) $targetPerms = $currentPerms & 0777; } } - @chmod($tempFile, $targetPerms); - + // 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 + // 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 +271,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 +288,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 +301,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..d34e5b420 100644 --- a/htdocs/modules/system/class/maintenance.php +++ b/htdocs/modules/system/class/maintenance.php @@ -15,9 +15,19 @@ * @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. 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, +// 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 @@ -153,7 +163,9 @@ public function CleanSession() * * @author slider84 of Team FrXoops * - * @return boolean + * @return bool + * + * @throws \RuntimeException If the avatar table query fails. */ public function CleanAvatar() { @@ -166,17 +178,87 @@ public function CleanAvatar() ); } + // 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; + + // 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))) { - //delete file - @unlink(XOOPS_UPLOAD_PATH . '/' . $myrow['avatar_file']); + // 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 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 + // 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) { + if (!$this->db->exec('DELETE FROM ' . $this->db->prefix('avatar') . ' WHERE avatar_id=' . $avatarId)) { + $deleteOk = false; + } + continue; + } + $avatarCandidate = XOOPS_UPLOAD_PATH . '/' . $avatarFile; + $avatarParent = realpath(dirname($avatarCandidate)); + if ( + is_string($avatarParent) + && is_string($avatarRootPrefix) + && str_starts_with(rtrim($avatarParent, DIRECTORY_SEPARATOR) . DIRECTORY_SEPARATOR, $avatarRootPrefix) + && (is_file($avatarCandidate) || is_link($avatarCandidate)) + ) { + xoops_remove_file_quietly($avatarCandidate, 'orphaned avatar'); + } //clean avatar table - $result1 = $this->db->exec('DELETE FROM ' . $this->db->prefix('avatar') . ' WHERE avatar_id=' . $myrow['avatar_id']); + 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; } /** @@ -221,10 +303,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 +324,31 @@ private function writeFileWithWarning($filename, $content) $targetPerms = $currentPerms & 0777; } } - @chmod($tempFile, $targetPerms); - + // 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 + // 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,16 +356,31 @@ private function writeFileWithWarning($filename, $content) } if (!@rename($tempFile, $filename)) { - @unlink($tempFile); + 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); - } elseif ($backupFile !== null && file_exists($backupFile) && !@unlink($backupFile)) { - trigger_error(sprintf('Failed to remove backup 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'); } } } diff --git a/tests/unit/htdocs/modules/system/SystemMaintenanceTest.php b/tests/unit/htdocs/modules/system/SystemMaintenanceTest.php index 71771c364..a501cdcdc 100644 --- a/tests/unit/htdocs/modules/system/SystemMaintenanceTest.php +++ b/tests/unit/htdocs/modules/system/SystemMaintenanceTest.php @@ -261,4 +261,314 @@ 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; + // 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]; + } + + /** + * 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. + * 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 + { + $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(true); + + $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 + { + // 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'; + $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(); + $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(true); + + $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 { + @unlink($outside); + } + } + + #[Test] + public function cleanAvatarSkipsAbsolutePathButStillDeletesDbRow(): void + { + // An absolute path stored in avatar_file should not allow the + // 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'); + $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, [ + ['avatar_id' => 100, 'avatar_file' => $outside], + ]); + $db->expects($this->exactly(2))->method('exec')->willReturn(true); + + $maintenance = $this->createMaintenance($db); + try { + $maintenance->CleanAvatar(); + $this->assertFileExists($outside, 'absolute-path fixture must not be removed by avatar cleanup'); + } finally { + @unlink($outside); + } + } + + #[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(true); + + $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(true); + + $maintenance = $this->createMaintenance($db); + $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; + $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(); + $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 + { + // 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(true); + + $maintenance = $this->createMaintenance($db); + try { + $maintenance->CleanAvatar(); + $this->assertFileDoesNotExist($abs, 'backslash-normalised avatar should be removed'); + } finally { + $this->removeScratchDir($scratchRel); + } + } }