helper-based atomic-write cleanup; replace @unlink/@chmod

htdocs/include/cp_functions.php

@@ -120,6 +120,87 @@
     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 @@
     $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,34 +248,47 @@
             $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;
         }
 
         $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;
         }
 
         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 @@
                 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;

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 @@
      *
      * @author slider84 of Team FrXoops
      *
-     * @return boolean
+     * @return bool
+     *
+     * @throws \RuntimeException If the avatar table query fails.
      */
     public function CleanAvatar()
     {
@@ -166,17 +178,87 @@
             );
         }
 
+        // 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/<filename>'
+            // (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 @@
         $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,37 +324,63 @@
                     $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;
                 }
             }
 
             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');
             }
         }
     }