Fix #387: Prevent password from matching username#1608
Conversation
Add a validation check in XoopsUserUtility::validate() that rejects a password identical to the username (case-insensitive), covering both registration and profile edit flows. Add the corresponding language string _US_PWDEQUALSUNAME to htdocs/language/english/user.php. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (4)
WalkthroughAdds case-insensitive validation preventing a non-empty password from being identical to the username across core validation, user edit/save, profile password change, and admin user management; also adds two corresponding language strings for the error message. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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 |
There was a problem hiding this comment.
Pull request overview
This PR implements a security enhancement to prevent users from setting their password equal to their username during registration and profile editing. The change addresses issue #387 by adding a case-insensitive validation check in the core XoopsUserUtility::validate() method.
Changes:
- Added case-insensitive password-equals-username check in
XoopsUserUtility::validate() - Added English language string
_US_PWDEQUALSUNAMEfor the validation error message
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| htdocs/class/userutility.php | Added validation logic to reject passwords matching username (case-insensitive) |
| htdocs/language/english/user.php | Added error message constant for password-equals-username validation |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@htdocs/class/userutility.php`:
- Around line 223-224: The condition uses strtolower($pass) without guaranteeing
$pass is not null, causing deprecation/errors; update the conditional in the
user utility check (the branch that appends _US_PWDEQUALSUNAME to $stop) to
ensure $pass is not null (e.g., add $pass !== null) or replace the $pass check
with a null/empty-safe check like !empty($pass) before calling strtolower($pass)
so strtolower is never invoked with a null argument.
- userutility.php: add $pass !== null guard before strtolower($pass) to prevent PHP 8.1 deprecation / PHP 9.0 TypeError when $pass is null - edituser.php: add username=password check to the inline validation block that bypasses XoopsUserUtility::validate() (password change during profile edit) - modules/profile/changepass.php: add username=password check to the profile module's dedicated change-password page (also bypassed XoopsUserUtility::validate()) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@htdocs/class/userutility.php`:
- Around line 223-224: Add unit tests exercising the new password≠username rule
in userutility.php: write tests that call the user validation/registration
routine that uses $pass and $uname (the code path where strtolower($pass) ===
strtolower($uname) triggers) and assert that when password equals username
(case-insensitive) the validation fails and returns/contains the
_US_PWDEQUALSUNAME error, and also assert that when password and username differ
validation succeeds; place tests alongside existing registration/profile unit
tests to cover both registration and profile-update flows so this core behavior
is protected from regressions.
In `@htdocs/edituser.php`:
- Around line 64-65: The type cast "(string)" in the conditional using
strtolower((string)$xoopsUser->getVar(...)) violates PSR-12; add a space after
the cast so it reads "(string) $xoopsUser->getVar(...)" in the strtolower call
within the block that sets $errors[] = _US_PWDEQUALSUNAME; ensure the change is
applied to the conditional using strtolower and the getVar invocation.
In `@htdocs/modules/profile/changepass.php`:
- Around line 58-59: The condition in changepass.php uses a type cast without
the PSR-12 required space: replace occurrences like
(string)$GLOBALS['xoopsUser']->getVar('uname', 'n') in the password check (the
elseif that compares $password to
strtolower((string)$GLOBALS['xoopsUser']->getVar(...))) with a space after the
cast so it becomes (string) $GLOBALS['xoopsUser']->getVar(...); ensure the same
spacing rule is applied anywhere else using (string) casts in this file.
| } elseif ($pass !== '' && $pass !== null && $uname !== null && strtolower($pass) === strtolower($uname)) { | ||
| $stop .= _US_PWDEQUALSUNAME . '<br>'; |
There was a problem hiding this comment.
Add unit tests for the new password≠username rule.
This is new core validation behavior and should be covered by tests to prevent regressions in registration/profile flows.
Based on learnings: "New functionality has corresponding unit tests".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@htdocs/class/userutility.php` around lines 223 - 224, Add unit tests
exercising the new password≠username rule in userutility.php: write tests that
call the user validation/registration routine that uses $pass and $uname (the
code path where strtolower($pass) === strtolower($uname) triggers) and assert
that when password equals username (case-insensitive) the validation fails and
returns/contains the _US_PWDEQUALSUNAME error, and also assert that when
password and username differ validation succeeds; place tests alongside existing
registration/profile unit tests to cover both registration and profile-update
flows so this core behavior is protected from regressions.
…st spacing The admin panel (system/admin/users/main.php) was missing the password!=username validation that was already present in registration, profile edit, and password change forms. Also fixes PSR-12 spacing after (string) casts. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
| if ('' !== Request::getString('password') && | ||
| strtolower(Request::getString('password')) === strtolower(Request::getString('uname'))) { | ||
| xoops_error(_US_PWDEQUALSUNAME); | ||
| break; |
There was a problem hiding this comment.
_US_PWDEQUALSUNAME is a front-end user language constant, but this admin script doesn’t appear to load user.php language strings. In the admin UI this can result in the raw constant name being displayed instead of a translated error. Consider either loading the user language (xoops_loadLanguage('user')) before using this constant, or (preferably) adding an _AM_SYSTEM_USERS_* admin language constant and using that here.
| if ('' !== Request::getString('password') && | ||
| strtolower(Request::getString('password')) === strtolower(Request::getString('uname'))) { | ||
| xoops_error(_US_PWDEQUALSUNAME); | ||
| break; |
There was a problem hiding this comment.
Same issue as above: this uses _US_PWDEQUALSUNAME in an admin context without ensuring the user language file is loaded. Please switch to an admin language constant or explicitly load the user language file before this validation block.
| } | ||
| if ($password != $vpass) { | ||
| $errors[] = _US_PASSNOTSAME; | ||
| } elseif (!empty($password) && strtolower($password) === strtolower((string) $GLOBALS['xoopsUser']->getVar('uname', 'n'))) { |
There was a problem hiding this comment.
!empty($password) treats the string '0' as empty, which would skip the new username-equality check even though '0' is a valid non-empty password. Since you’re already in the non-mismatch branch, this guard isn’t necessary—prefer a strict empty-string check (or remove the guard) so the comparison always runs for any non-empty password.
| } elseif (!empty($password) && strtolower($password) === strtolower((string) $GLOBALS['xoopsUser']->getVar('uname', 'n'))) { | |
| } elseif ($password !== '' && strtolower($password) === strtolower((string) $GLOBALS['xoopsUser']->getVar('uname', 'n'))) { |
| $vpass = XoopsRequest::getString('vpass', ''); | ||
| if ($password != $vpass) { | ||
| $errors[] = _US_PASSNOTSAME; | ||
| } elseif (strtolower($password) === strtolower((string) $xoopsUser->getVar('uname', 'n'))) { | ||
| $errors[] = _US_PWDEQUALSUNAME; | ||
| } |
There was a problem hiding this comment.
The PR description says registration and profile edit are covered because they call XoopsUserUtility::validate(), but htdocs/edituser.php performs its own password validation instead of calling XoopsUserUtility::validate(). Either update the PR description to reflect the actual coverage, or refactor this flow to reuse XoopsUserUtility::validate() so all password rules stay consistent in one place.
| $stop .= _US_PASSNOTSAME . '<br>'; | ||
| } elseif (($pass != '') && (strlen($pass) < $xoopsConfigUser['minpass'])) { | ||
| $stop .= sprintf(_US_PWDTOOSHORT, $xoopsConfigUser['minpass']) . '<br>'; | ||
| } elseif ($pass !== '' && $pass !== null && $uname !== null && strtolower($pass) === strtolower($uname)) { | ||
| $stop .= _US_PWDEQUALSUNAME . '<br>'; |
There was a problem hiding this comment.
New validation behavior was added to XoopsUserUtility::validate() but there are currently no unit tests exercising validate() (tests only cover getIP()). Please add a unit test that verifies a password equal to the username (case-insensitive) is rejected, using the existing stub DB setup in the test bootstrap so the DB uniqueness checks don’t make the test brittle.
- Use _AM_SYSTEM_USERS_PWDEQUALSUNAME admin constant instead of _US_PWDEQUALSUNAME in admin users panel (user language file is not loaded in admin context) - Replace !empty($password) with $password !== '' in changepass.php to avoid treating the string '0' as empty Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1608 +/- ##
========================================
Coverage ? 0
========================================
Files ? 0
Lines ? 0
Branches ? 0
========================================
Hits ? 0
Misses ? 0
Partials ? 0 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@htdocs/modules/system/admin/users/main.php`:
- Around line 183-187: The pwdequalsuname check fires even when the confirmation
field is empty; change the condition that currently tests
Request::getString('password') to also require Request::getString('pass2') !==
'' (i.e., only run the xoops_error(_AM_SYSTEM_USERS_PWDEQUALSUNAME) check when
pass2 is non-empty) in both the update path and the add path so the check
matches the same gating used when the password is actually hashed/persisted.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
htdocs/edituser.phphtdocs/modules/profile/changepass.phphtdocs/modules/system/admin/users/main.phphtdocs/modules/system/language/english/admin/users.php
The check should only fire when a password change is actually being made (pass2 is non-empty), matching the existing password-mismatch check and the condition that triggers password hashing/persistence. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
| $stop .= _US_PASSNOTSAME . '<br>'; | ||
| } elseif (($pass != '') && (strlen($pass) < $xoopsConfigUser['minpass'])) { | ||
| $stop .= sprintf(_US_PWDTOOSHORT, $xoopsConfigUser['minpass']) . '<br>'; | ||
| } elseif ($pass !== '' && $pass !== null && $uname !== null && strtolower($pass) === strtolower($uname)) { | ||
| $stop .= _US_PWDEQUALSUNAME . '<br>'; | ||
| } |
There was a problem hiding this comment.
XoopsUserUtility::validate() has new password rule logic, but there are existing PHPUnit tests for this class (e.g., tests/unit/htdocs/class/XoopsUserUtilityTest.php) and they currently explicitly avoid testing validate(). Consider adding coverage for the new “password equals username” rejection—if direct testing is hard due to DB/global dependencies, extracting the check into a small pure helper (or injecting dependencies) would make it testable and prevent regressions.
| if ('' !== Request::getString('pass2') && | ||
| '' !== Request::getString('password') && | ||
| strtolower(Request::getString('password')) === strtolower(Request::getString('uname'))) { | ||
| xoops_error(_AM_SYSTEM_USERS_PWDEQUALSUNAME); | ||
| break; |
There was a problem hiding this comment.
This password==username validation runs whenever password is non-empty, but later the code only applies a password change when pass2 is non-empty. This can block saving unrelated user edits (or show an error) in cases where the password won’t actually be updated. Consider scoping this validation to the same condition used to set the password (e.g., only when pass2 is provided / when a password change is intended), or require confirmation when password is set.
|
|
||
| if ('' !== Request::getString('pass2') && | ||
| '' !== Request::getString('password') && | ||
| strtolower(Request::getString('password')) === strtolower(Request::getString('uname'))) { | ||
| xoops_error(_AM_SYSTEM_USERS_PWDEQUALSUNAME); |
There was a problem hiding this comment.
Same issue as above in the add-user flow: this check runs based on password alone, but the password is only persisted when pass2 is non-empty. Align the validation condition with the actual password-setting logic (or enforce that pass2 is required when password is provided) to avoid unexpected validation failures or confusing behavior.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@htdocs/modules/system/admin/users/main.php`:
- Around line 183-188: Replace ASCII-only strtolower() calls used when comparing
Request::getString('password') and Request::getString('uname') with
multibyte-aware mb_strtolower(..., 'UTF-8'); specifically change occurrences
like strtolower(Request::getString('password')) and
strtolower(Request::getString('uname')) to
mb_strtolower(Request::getString('password'), 'UTF-8') and
mb_strtolower(Request::getString('uname'), 'UTF-8') before calling
xoops_error(_AM_SYSTEM_USERS_PWDEQUALSUNAME); apply the same change to the
identical comparison in the update/add path referenced (the other occurrence
around lines 277–279).
Replace strtolower() with mb_strtolower(..., 'UTF-8') in all password!=username checks for proper Unicode case folding when loose username validation is enabled. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Summary
XoopsUserUtility::validate()that rejects a password identical to the usernameregister.php) and profile edit (edituser.php) since both callXoopsUserUtility::validate()_US_PWDEQUALSUNAMEtolanguage/english/user.phpTest plan
Fixes #387
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Localization