User Search Issue #1559 #1560
User Search Issue #1559 #1560
Conversation
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
We need to merge this code! |
This change ensures backward compatibility while signaling to developers that they should migrate to safer, modern methods for SQL escaping.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Fix Deprecated addSlashes
|
@GregMage I still need to do some more testing, and then I'll merge it. |
Updated addSlashes method to log deprecation notice.
Hide empty userinfo value
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull Request Overview
This PR addresses security issues by replacing deprecated MyTextSanitizer::addSlashes() calls with the proper database escaping method $xoopsDB->escape() throughout the codebase. The changes also include conditional display improvements in user profile rendering and significant enhancements to the member handler class for better security and performance.
- Replaces all instances of
$myts->addSlashes()with$xoopsDB->escape()for proper SQL injection prevention - Adds conditional checks to display user profile fields only when they contain data
- Enhances the XoopsMemberHandler class with security hardening, better error handling, and PHP 7.4-8.4 compatibility
Reviewed Changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| htdocs/userinfo.php | Adds conditional rendering for user profile fields |
| htdocs/search.php | Replaces addSlashes with proper database escaping |
| htdocs/modules/system/admin/users/main.php | Updates email validation to use database escaping |
| htdocs/modules/profile/search.php | Replaces addSlashes and removes unused MyTextSanitizer instance |
| htdocs/modules/profile/lostpass.php | Updates password recovery to use database escaping |
| htdocs/modules/profile/edituser.php | Updates avatar handling with proper escaping |
| htdocs/modules/profile/activate.php | Updates user activation with database escaping |
| htdocs/lostpass.php | Updates main password recovery with database escaping |
| htdocs/kernel/member.php | Major enhancement with security hardening and performance improvements |
| htdocs/edituser.php | Updates user editing with proper database escaping |
| htdocs/class/xoopstopic.php | Updates topic storage with database escaping |
| htdocs/class/xoopsstory.php | Updates story storage with database escaping |
| htdocs/class/module.textsanitizer.php | Updates deprecated addSlashes method to use database escaping |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| defined('XOOPS_ROOT_PATH') || exit('Restricted access'); | ||
|
|
||
| if (!defined('XOOPS_ROOT_PATH')) { | ||
| throw new RuntimeException('Restricted access'); |
There was a problem hiding this comment.
The change from exit('Restricted access') to throwing a RuntimeException is a breaking change that could affect error handling in existing code. Consider maintaining backward compatibility by keeping the original exit behavior or providing a configuration option for the new behavior.
| throw new RuntimeException('Restricted access'); | |
| // Backward compatibility: use exit() unless XOOPS_STRICT_ACCESS_CHECK is defined and true | |
| if (defined('XOOPS_STRICT_ACCESS_CHECK') && XOOPS_STRICT_ACCESS_CHECK) { | |
| throw new RuntimeException('Restricted access'); | |
| } else { | |
| exit('Restricted access'); | |
| } |
| // Use Criteria for safe querying of username; password is handled separately for verification | ||
| $criteria = new Criteria('uname', (string)$uname); | ||
| $criteria->setLimit(2); // Fetch at most 2 to detect duplicates | ||
|
|
||
| $users = $this->userHandler->getObjects($criteria, false) ?: []; | ||
| if (count($users) !== 1) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
The username is cast to string but not properly escaped for the Criteria object. While Criteria should handle escaping internally, the comment suggests password handling is separate, but username should also be properly sanitized before use in database queries.
| $expectedHash = md5($pwd); | ||
| if (!$this->hashEquals($expectedHash, $hash)) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Using MD5 for password hashing is cryptographically insecure. While this appears to be handling legacy passwords for migration purposes, consider adding a time-based deprecation warning or forced upgrade path to encourage users to update their passwords.
| $raw = random_bytes($bytes); | ||
| } catch (\Throwable $e) { | ||
| $msg = '[CRITICAL] No CSPRNG available to generate a secure token in ' . __METHOD__; | ||
| if (class_exists('XoopsLogger')) { | ||
| \XoopsLogger::getInstance()->handleError(E_USER_ERROR, $msg, __FILE__, __LINE__); | ||
| } else { | ||
| error_log($msg); | ||
| } | ||
| throw new \RuntimeException('No CSPRNG available to generate a secure token.', 0, $e); | ||
| } |
There was a problem hiding this comment.
The method throws a RuntimeException when CSPRNG is unavailable, but this could break existing code that expects the method to always return a string. Consider providing a fallback mechanism or documenting this breaking change clearly.
| global $xoopsDB; | ||
| $GLOBALS['xoopsLogger']->addDeprecated( | ||
| __METHOD__ . ' is deprecated. Use $xoopsDB->escape() or $xoopsDB->quoteString() instead.' | ||
| ); | ||
| return $xoopsDB->escape($text); |
There was a problem hiding this comment.
Using global $xoopsDB assumes the global variable is always available and initialized. If $xoopsDB is null or not set, this will cause a fatal error. Add a null check or use XoopsDatabaseFactory::getDatabaseConnection() for safer database access.
| global $xoopsDB; | |
| $GLOBALS['xoopsLogger']->addDeprecated( | |
| __METHOD__ . ' is deprecated. Use $xoopsDB->escape() or $xoopsDB->quoteString() instead.' | |
| ); | |
| return $xoopsDB->escape($text); | |
| $db = \XoopsDatabaseFactory::getDatabaseConnection(); | |
| $GLOBALS['xoopsLogger']->addDeprecated( | |
| __METHOD__ . ' is deprecated. Use $db->escape() or $db->quoteString() instead.' | |
| ); | |
| return $db->escape($text); |
#1559