helper-based atomic-write cleanup; replace @unlink/@chmod#52
Open
mambax7 wants to merge 10 commits intoXOOPS:masterfrom
Open
helper-based atomic-write cleanup; replace @unlink/@chmod#52mambax7 wants to merge 10 commits intoXOOPS:masterfrom
mambax7 wants to merge 10 commits intoXOOPS:masterfrom
Conversation
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/<filename>' (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.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds scoped quiet chmod/unlink helpers, applies them to atomic-write and guard-file flows, hardens avatar deletion with path normalization and containment checks, requires the helpers in SystemMaintenance, and adds unit tests covering CleanAvatar() variants. ChangesFile Handling Safety and Error Recovery
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #52 +/- ##
============================================
- Coverage 18.08% 18.08% -0.01%
- Complexity 7840 7854 +14
============================================
Files 665 665
Lines 42982 43127 +145
============================================
+ Hits 7775 7799 +24
- Misses 35207 35328 +121 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Pull request overview
This PR reduces error-suppression usage in file cleanup paths by introducing a centralized “best-effort” delete helper and adopting it in atomic-write cleanup code, while also hardening orphaned-avatar deletion against path traversal.
Changes:
- Added
xoops_remove_file_quietly()incp_functions.phpand replaced multiple@unlinkcleanup sites with the helper. - Replaced
@chmodcalls with checkedchmod()that logs warnings on failure but continues. - Hardened
SystemMaintenance::CleanAvatar()orphan cleanup by normalizing and resolving avatar paths and enforcing containment underXOOPS_UPLOAD_PATH.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| htdocs/modules/system/class/maintenance.php | Requires cp_functions.php, adopts the new cleanup helper in atomic-write cleanup, and hardens orphaned avatar file removal with realpath() + containment checks. |
| htdocs/include/cp_functions.php | Introduces xoops_remove_file_quietly() and replaces multiple @unlink/@chmod usages in xoops_write_file_atomically(). |
Comment on lines
+194
to
+200
| $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) |
Comment on lines
+177
to
+181
| // Avatar files are stored as 'avatars/<filename>' (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 |
…out of loop Address PR XOOPS#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_<pid>_<uniqid> with finally-block cleanup so the tests do not collide with each other or with anything else in the upload tree.
Comment on lines
+343
to
+354
| /** | ||
| * 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); | ||
| } |
CI run XOOPS#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).
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@htdocs/modules/system/class/maintenance.php`:
- Line 200: The DELETE SQL concatenates $myrow['avatar_id'] directly into the
string; change the interpolation to cast the value to int before concatenation
to avoid injection/scan warnings—update the calls that use $this->db->exec(...)
with $this->db->prefix('avatar') and $myrow['avatar_id'] (the deletes at the
current line and the adjacent one around line 213) so the avatar_id is cast to
(int) (e.g., use (int)$myrow['avatar_id']) when building the WHERE clause.
In `@tests/unit/htdocs/modules/system/SystemMaintenanceTest.php`:
- Line 368: The mocks for XoopsDatabase::exec() use willReturn(1) which violates
exec()'s bool return type and causes PHPUnit IncompatibleReturnValueException;
locate every occurrence of $db->expects(...)->method('exec')->willReturn(1) in
the SystemMaintenanceTest cleanAvatar* tests and change willReturn(1) to
willReturn(true) (apply to all six instances where exec() is stubbed).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3dc5b9f2-faa6-4f9e-b34c-1076d70a5758
📒 Files selected for processing (2)
htdocs/modules/system/class/maintenance.phptests/unit/htdocs/modules/system/SystemMaintenanceTest.php
Comment on lines
+343
to
+354
| /** | ||
| * 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); | ||
| } |
Comment on lines
+382
to
+391
| // 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); |
Comment on lines
+413
to
+426
| // 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(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'); |
…st; (int) avatar_id Address PR XOOPS#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 '../../<basename>' 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 '../<basename>' 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.
Comment on lines
+203
to
+211
| 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 | ||
| ); | ||
| } |
Comment on lines
+290
to
+297
| 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 | ||
| ); | ||
| } |
Comment on lines
+203
to
+217
| $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); | ||
| continue; | ||
| } | ||
| $avatarPath = realpath(XOOPS_UPLOAD_PATH . '/' . $avatarFile); | ||
| if ( | ||
| is_string($avatarPath) | ||
| && is_string($uploadRootPrefix) | ||
| && str_starts_with($avatarPath, $uploadRootPrefix) | ||
| && is_file($avatarPath) | ||
| ) { | ||
| xoops_remove_file_quietly($avatarPath, 'orphaned avatar'); | ||
| } |
Address PR XOOPS#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/<filename>' (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/<unique>.doc and sets avatar_file to 'files/<unique>.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.
| $this->fail('Could not create test scratch dir: ' . $scratchAbs); | ||
| } | ||
| $absPath = $scratchAbs . '/' . $filename; | ||
| file_put_contents($absPath, 'fixture'); |
…atar Address PR XOOPS#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
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
htdocs/modules/system/class/maintenance.php (1)
156-171:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd
@throws \RuntimeExceptionto theCleanAvatar()docblock.The method throws a
\RuntimeException(line 169-173) but the PHPDoc is silent about it. As per coding guidelines, public methods must declare@throwswhen applicable.📝 Proposed docblock update
/** * CleanAvatar * * Clean up orphaned custom avatars left when a user is deleted. * * `@author` slider84 of Team FrXoops * + * `@throws` \RuntimeException If the avatar table query fails. * `@return` boolean */As per coding guidelines: "New public methods have PHPDoc with
@param,@return, and@throwstags".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@htdocs/modules/system/class/maintenance.php` around lines 156 - 171, The CleanAvatar() method can throw a \RuntimeException but its PHPDoc lacks a `@throws` tag; update the method's docblock for CleanAvatar to include "@throws \RuntimeException" (with a brief description) so the public method documents the possible exception thrown by the database error branch (the RuntimeException raised around the $this->db->error() call).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@htdocs/include/cp_functions.php`:
- Around line 138-187: Both xoops_chmod_quietly and xoops_remove_file_quietly
risk using an uninitialized $ok if a user error handler throws inside the try;
to fix, initialize $ok = false immediately before each try block (in
xoops_chmod_quietly and xoops_remove_file_quietly) so the subsequent if (!$ok)
checks are safe even when unlink()/chmod() never return due to an exception;
keep the scoped error_reporting() and existing trigger_error() behavior
unchanged.
In `@htdocs/modules/system/class/maintenance.php`:
- Around line 210-227: The two assignments to $result1 in the CleanAvatar method
are unused; remove the variable capture and call $this->db->exec(...) directly
(or alternatively check its boolean result and log/return false on failure).
Locate the two calls to $this->db->exec(...) that currently assign to $result1
in class Maintenance::CleanAvatar and either (A) replace " $result1 =
$this->db->exec(...);" with "$this->db->exec(...);" to make the intent explicit,
or (B) change to "if (!$this->db->exec(...)) { /* log via
processLogger/db->error and return false; */ }" so failures are propagated.
---
Outside diff comments:
In `@htdocs/modules/system/class/maintenance.php`:
- Around line 156-171: The CleanAvatar() method can throw a \RuntimeException
but its PHPDoc lacks a `@throws` tag; update the method's docblock for CleanAvatar
to include "@throws \RuntimeException" (with a brief description) so the public
method documents the possible exception thrown by the database error branch (the
RuntimeException raised around the $this->db->error() call).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 663a51de-03a6-4402-a4f1-f40999eb6701
📒 Files selected for processing (3)
htdocs/include/cp_functions.phphtdocs/modules/system/class/maintenance.phptests/unit/htdocs/modules/system/SystemMaintenanceTest.php
Address PR XOOPS#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.
Comment on lines
+22
to
+27
| // 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'; | ||
|
|
Comment on lines
+176
to
+192
| 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); | ||
| } finally { | ||
| error_reporting($previousLevel); | ||
| } | ||
| if (!$ok && file_exists($path)) { | ||
| trigger_error( |
Address PR XOOPS#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.
Comment on lines
+23
to
+25
| if (!defined('XOOPS_ROOT_PATH')) { | ||
| exit(); | ||
| } |
Comment on lines
+223
to
+227
| $avatarPath = realpath(XOOPS_UPLOAD_PATH . '/' . $avatarFile); | ||
| if ( | ||
| is_string($avatarPath) | ||
| && is_string($avatarRootPrefix) | ||
| && str_starts_with($avatarPath, $avatarRootPrefix) |
Comment on lines
+227
to
+231
| && str_starts_with($avatarPath, $avatarRootPrefix) | ||
| && is_file($avatarPath) | ||
| ) { | ||
| xoops_remove_file_quietly($avatarPath, 'orphaned avatar'); | ||
| } |
| @@ -154,6 +165,8 @@ public function CleanSession() | |||
| * @author slider84 of Team FrXoops | |||
| * | |||
| * @return boolean | |||
… guard Address PR XOOPS#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/<link> pointing to uploads/avatars/<real> 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.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
htdocs/modules/system/class/maintenance.php (1)
278-358: 🧹 Nitpick | 🔵 Trivial | 💤 Low value
writeFileWithWarning()atomic-write paths are consistent with the new helper conventions.Helper routing for temp-guard and backup-guard cleanup is uniform, the chmod demotion to non-fatal matches
xoops_write_file_atomically()semantics, and the$backupFile = nullinitialisation at Line 325 cleanly disambiguates the "no prior file" path from the "restoration needed" path at Lines 347–351.One minor observability nit (not blocking): when the initial replace fails but
@rename($backupFile, $filename)succeeds, the user sees onlyFailed to replace guard file, with no indication that the original was restored. Tracking it as something to consider if the message ever surfaces in support tickets.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@htdocs/modules/system/class/maintenance.php` around lines 278 - 358, The failure path in writeFileWithWarning() currently logs only "Failed to replace guard file" when rename(temp->target) fails even if the subsequent restore of the backup ($backupFile -> $filename) succeeds; update writeFileWithWarning() so that inside the if (!@rename($tempFile, $filename)) { ... if ($backupFile !== null) { if (!@rename($backupFile, $filename)) { ... } } } block you detect the return of `@rename`($backupFile, $filename) and emit an additional, clear log when the restore succeeds (e.g. a trigger_error/notice like "Restored original guard file: %s" using $label) so callers can observe that the original was restored; keep the existing error path when the restore fails.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@htdocs/modules/system/class/maintenance.php`:
- Around line 278-358: The failure path in writeFileWithWarning() currently logs
only "Failed to replace guard file" when rename(temp->target) fails even if the
subsequent restore of the backup ($backupFile -> $filename) succeeds; update
writeFileWithWarning() so that inside the if (!@rename($tempFile, $filename)) {
... if ($backupFile !== null) { if (!@rename($backupFile, $filename)) { ... } }
} block you detect the return of `@rename`($backupFile, $filename) and emit an
additional, clear log when the restore succeeds (e.g. a trigger_error/notice
like "Restored original guard file: %s" using $label) so callers can observe
that the original was restored; keep the existing error path when the restore
fails.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1d53766d-6c3d-48ac-8b53-1513a1b1f78a
📒 Files selected for processing (2)
htdocs/include/cp_functions.phphtdocs/modules/system/class/maintenance.php
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
htdocs/modules/system/class/maintenance.php:248
- CleanAvatar() always returns true and ignores the return values from the DELETE exec() calls (including the final avatar_user_link cleanup, currently assigned to $result2 but never checked). Since callers use the boolean return to decide whether the avatar purge succeeded (e.g. system/admin/maintenance/main.php), failures here will be reported as success. Consider tracking exec() results and returning false when a delete fails (or removing the boolean return entirely if you don't want callers to rely on it).
$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') . ')');
return true;
… writeFileWithWarning
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: <X>",
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: <X> (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.
|
Comment on lines
+138
to
+202
| 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 | ||
| ); | ||
| } | ||
| } |
Comment on lines
+233
to
+250
| $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'); | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.phpso 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, strips leading slashes, resolves via realpath() with upload-root containment, and confirms is_file() before removal. The DB row
cleanup runs unconditionally.
Intentionally retained:
- The 7 @rename(...) calls inside
if (!...)checks. These are the core atomic-move operations; the boolean return is already detected and reportedvia 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.
Summary by CodeRabbit
Bug Fixes
Tests