fix(profile): guard the logout cookie name in the profile module's own logout too - #198
Conversation
…n logout too XOOPS#195 guarded the remember-me cookie clearing on logout in user.php, but the profile module's user.php carries its own logout with the same two unguarded calls and is reached directly, so a site with remember-me disabled still failed on logout through that page. The same guard is applied there, and the regression test now covers both logout pages. The 2.7.4 Beta 1 release note for this fix named the users admin as a guarded path; it clears no cookie. The note now names the two logout pages and the session restore.
Reviewer's GuideThe PR fixes logout when remember-me is disabled by guarding the profile module’s duplicate cookie-clearing calls, expands the source-based regression test to cover both logout implementations, and corrects the release-note wording for the affected paths. Sequence diagram for guarded logout cookie clearingsequenceDiagram
participant User
participant LogoutPage
participant Session
participant Cookie
User->>LogoutPage: logout
LogoutPage->>Session: regenerate_id(true)
LogoutPage->>Session: clear session
alt usercookie is configured
LogoutPage->>Cookie: xoops_setcookie(usercookie, null, expiry, /, domain, 0)
LogoutPage->>Cookie: xoops_setcookie(usercookie, null, expiry, /)
end
LogoutPage-->>User: logout completes
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. WalkthroughThe profile logout handler now clears the remember-me cookie only when a cookie name is configured. The regression test covers both core and profile logout entry points. The changelog describes the corrected behavior. ChangesRemember-me cookie logout guard
Estimated code review effort: 2 (Simple) | ~10 minutes Severity of issue fixed: Low Merge Risk: ⚪ Minimal · up to Logout now avoids attempting to clear an unnamed remember-me cookie in the profile flow while retaining configured-cookie behavior. The change is covered for both logout pages and is ready to merge. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 2 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Some tools did not complete. Review the errors below. 🔧 PHPStan (2.2.9)PHP Fatal error: Uncaught Error: Class "Nette\DI\ServiceCreationException" not found in /.cache/phpstan/phpstan/cache/nette.configurator/Container_cbb1c40a91.php:8719 ... [truncated 2695 characters] ... Stan\Command\AnalyseCommand->execute() 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 |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="tests/unit/htdocs/LogoutCookieNameGuardTest.php" line_range="58" />
<code_context>
#[Test]
- public function logoutClearsTheRememberMeCookieOnlyWhenANameIsConfigured(): void
+ #[DataProvider('logoutPages')]
+ public function logoutClearsTheRememberMeCookieOnlyWhenANameIsConfigured(string $file): void
{
- $this->loadSourceFile('htdocs/user.php');
</code_context>
<issue_to_address>
**issue (testing):** The regression test only scans the logout source text and never executes either logout path, so it can pass even when the runtime guard does not actually prevent `xoops_setcookie()` from being called with an empty configured name.
**Triggers:** When a future edit preserves the expected source strings but changes the runtime condition, control flow, or included code.
**Suggested fix:** Execute each logout source file with an empty and a configured `usercookie` value, asserting that the cookie helper is respectively not called and called twice.
</issue_to_address>Sourcery assessment
Needs a human reviewer. 1 finding to address first, and if the guard were wrong for configured cookie names, logout could leave a remember-me cookie in the browser and allow the account to be restored after logout. Reverting would fix future logouts, but cookies already left behind could remain usable until they expire or are otherwise cleared.
Blocking findings: tests/unit/htdocs/LogoutCookieNameGuardTest.php:58
| #[Test] | ||
| public function logoutClearsTheRememberMeCookieOnlyWhenANameIsConfigured(): void | ||
| #[DataProvider('logoutPages')] | ||
| public function logoutClearsTheRememberMeCookieOnlyWhenANameIsConfigured(string $file): void |
There was a problem hiding this comment.
issue (testing): The regression test only scans the logout source text and never executes either logout path, so it can pass even when the runtime guard does not actually prevent xoops_setcookie() from being called with an empty configured name.
Triggers: When a future edit preserves the expected source strings but changes the runtime condition, control flow, or included code.
Suggested fix: Execute each logout source file with an empty and a configured usercookie value, asserting that the cookie helper is respectively not called and called twice.
 |
There was a problem hiding this comment.
🟢 Approval recommended
The change is narrowly scoped, matches the established guard pattern, and is backed by an updated regression test covering both logout entry points.
Pull request overview
This pull request closes the remaining logout failure when “remember-me” is disabled by applying the same cookie-name guard to the profile module’s direct logout entry point, and extends the existing regression test to cover both logout pages. It also corrects the 2.7.4 Beta 1 release note to reflect the actual guarded paths.
Changes:
- Guard
xoops_setcookie()calls inmodules/profile/user.phpso logout doesn’t attempt to clear a cookie with an empty name. - Extend
LogoutCookieNameGuardTestto run against bothhtdocs/user.phpandhtdocs/modules/profile/user.php. - Update the 2.7.4 Beta 1 changelog entry to name the two logout pages plus session restore (and remove the incorrect users-admin mention).
File summaries
| File | Description |
|---|---|
| tests/unit/htdocs/LogoutCookieNameGuardTest.php | Adds a data provider so the existing guard assertion runs against both logout entry points. |
| htdocs/modules/profile/user.php | Wraps remember-me cookie clearing in an !empty(usercookie) guard to prevent PHP 8+ empty-cookie-name errors. |
| docs/changelog.270.txt | Corrects the release note wording to match the guarded logout/restore paths. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #198 +/- ##
============================================
- Coverage 20.63% 20.63% -0.01%
Complexity 7995 7995
============================================
Files 675 675
Lines 43514 43515 +1
============================================
Hits 8981 8981
- Misses 34533 34534 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Summary
#195 guarded the remember-me cookie clearing on logout in
user.php, but the profile module'suser.phpcarries its own logout with the same two unguarded calls and is reached directly, so a site with remember-me disabled still failed on logout through that page. The same guard is applied there, andLogoutCookieNameGuardTestnow covers both logout pages.The 2.7.4 Beta 1 release note for this fix named the users admin as a guarded path; it clears no cookie. The note now names the two logout pages and the session restore. Both points were raised on #197 after it merged.
Tests
The profile case failed before the guard and passes after; removing the guard fails it again.
tests/at baseline on PHP 8.4.Summary by Sourcery
Prevent logout failures when remember-me cookies are disabled across both logout entry points.
Bug Fixes:
Documentation:
Tests:
Summary by CodeRabbit