XOOPS /
XoopsCore25
| Conditions | 46 |
| Paths | > 20000 |
| Total Lines | 244 |
| Code Lines | 139 |
| Lines | 0 |
| Ratio | 0 % |
| Changes | 5 | ||
| Bugs | 0 | Features | 0 |
Small methods make your code easier to understand, in particular if combined with a good name. Besides, if your method is small, finding a good name is usually much easier.
For example, if you find yourself adding comments to a method's body, this is usually a good sign to extract the commented part to a new method, and use the comment as a starting point when coming up with a good name for this new method.
Commonly applied refactorings include:
If many parameters/temporary variables are present:
| 1 | <?php |
||
| 502 | public function getUsersByGroupLink( |
||
| 503 | $groups, |
||
| 504 | $criteria = null, |
||
| 505 | $asobject = false, |
||
| 506 | $id_as_key = false |
||
| 507 | ) { |
||
| 508 | // Type coercion for backwards compatibility |
||
| 509 | $groups = is_array($groups) ? $groups : [$groups]; |
||
| 510 | $asobject = (bool)$asobject; |
||
| 511 | $id_as_key = (bool)$id_as_key; |
||
| 512 | |||
| 513 | // Debug configuration using only current XOOPS debug system |
||
| 514 | // Check XOOPS debug mode - we only want PHP debugging (1=inline, 2=popup) |
||
| 515 | $xoopsDebugMode = isset($GLOBALS['xoopsConfig']['debug_mode']) ? (int)$GLOBALS['xoopsConfig']['debug_mode'] : 0; |
||
| 516 | $xoopsPhpDebugEnabled = ($xoopsDebugMode === 1 || $xoopsDebugMode === 2); |
||
| 517 | |||
| 518 | // Check if debug is allowed for current user based on debugLevel |
||
| 519 | $xoopsDebugAllowed = $xoopsPhpDebugEnabled; |
||
| 520 | if ($xoopsPhpDebugEnabled && isset($GLOBALS['xoopsConfig']['debugLevel'])) { |
||
| 521 | $debugLevel = (int)$GLOBALS['xoopsConfig']['debugLevel']; |
||
| 522 | $xoopsUser = $GLOBALS['xoopsUser'] ?? null; |
||
| 523 | $xoopsUserIsAdmin = isset($GLOBALS['xoopsUserIsAdmin']) ? $GLOBALS['xoopsUserIsAdmin'] : false; |
||
| 524 | |||
| 525 | // Apply XOOPS debug level restrictions |
||
| 526 | switch ($debugLevel) { |
||
| 527 | case 2: // Admins only |
||
| 528 | $xoopsDebugAllowed = $xoopsUserIsAdmin; |
||
| 529 | break; |
||
| 530 | case 1: // Members only |
||
| 531 | $xoopsDebugAllowed = ($xoopsUser !== null); |
||
| 532 | break; |
||
| 533 | case 0: // All users |
||
| 534 | default: |
||
| 535 | $xoopsDebugAllowed = true; |
||
| 536 | break; |
||
| 537 | } |
||
| 538 | } |
||
| 539 | |||
| 540 | // Production safety check - use secure environment detection |
||
| 541 | // Note: SERVER_NAME can be spoofed via Host header, so it's not secure for production detection |
||
| 542 | // For security, set XOOPS_ENV=production in your server environment or use a config constant |
||
| 543 | $isProd = false; |
||
| 544 | |||
| 545 | if (defined('XOOPS_PRODUCTION') && XOOPS_PRODUCTION) { |
||
| 546 | // Most secure: use a defined constant set in configuration |
||
| 547 | $isProd = true; |
||
| 548 | } elseif (getenv('XOOPS_ENV') === 'production') { |
||
| 549 | // Secure: use environment variable (not spoofable by clients) |
||
| 550 | $isProd = true; |
||
| 551 | } else { |
||
| 552 | // Fallback: assume production unless explicitly in known development environments |
||
| 553 | // This is more secure than the old approach - defaults to restrictive mode |
||
| 554 | $isProd = true; |
||
| 555 | // Only allow debug in explicitly known safe development indicators |
||
| 556 | if ((defined('XOOPS_DEBUG') && XOOPS_DEBUG) |
||
| 557 | || (php_sapi_name() === 'cli') |
||
| 558 | || (isset($_SERVER['SERVER_ADDR']) && $_SERVER['SERVER_ADDR'] === '127.0.0.1')) { |
||
| 559 | $isProd = false; |
||
| 560 | } |
||
| 561 | } |
||
| 562 | |||
| 563 | // Enable SQL logging only if XOOPS PHP debug is allowed and not in production |
||
| 564 | $isDebug = $xoopsDebugAllowed && !$isProd; |
||
| 565 | |||
| 566 | /** |
||
| 567 | * Redact sensitive SQL literals in debug logs while preserving query structure |
||
| 568 | * @param string $sql The SQL query to redact |
||
| 569 | * @return string Redacted SQL query |
||
| 570 | */ |
||
| 571 | $redactSql = static function (string $sql): string { |
||
| 572 | // Replace quoted strings with placeholders |
||
| 573 | $sql = preg_replace("/'[^']*'/", "'?'", $sql); |
||
| 574 | $sql = preg_replace('/"[^"]*"/', '"?"', $sql); |
||
| 575 | // Replace hex literals |
||
| 576 | $sql = preg_replace("/x'[0-9A-Fa-f]+'/", "x'?'", $sql); |
||
| 577 | // Replace large numbers (potential IDs) but keep small ones |
||
| 578 | $sql = preg_replace('/\b\d{6,}\b/', '[ID]', $sql); |
||
| 579 | return $sql; |
||
| 580 | }; |
||
| 581 | |||
| 582 | $ret = []; |
||
| 583 | $criteriaCompo = new CriteriaCompo(); |
||
| 584 | $select = $asobject ? 'u.*' : 'u.uid'; |
||
| 585 | $sql = "SELECT {$select} FROM " . $this->userHandler->db->prefix('users') . ' u'; |
||
| 586 | $whereParts = []; |
||
| 587 | $limit = 0; |
||
| 588 | $start = 0; |
||
| 589 | |||
| 590 | // Sanitize and validate groups once - clean and efficient |
||
| 591 | $validGroups = array_values( |
||
| 592 | array_unique( |
||
| 593 | array_filter( |
||
| 594 | array_map('intval', $groups), |
||
| 595 | static fn($id) => $id > 0 |
||
| 596 | ) |
||
| 597 | ) |
||
| 598 | ); |
||
| 599 | |||
| 600 | // Build group filtering with EXISTS subquery (no re-validation needed) |
||
| 601 | if (!empty($validGroups)) { |
||
| 602 | $group_in = '(' . implode(', ', $validGroups) . ')'; |
||
| 603 | $whereParts[] = 'EXISTS (SELECT 1 FROM ' . $this->membershipHandler->db->prefix('groups_users_link') |
||
| 604 | . " m WHERE m.uid = u.uid AND m.groupid IN {$group_in})"; |
||
| 605 | } |
||
| 606 | |||
| 607 | // Handle criteria - compatible with CriteriaElement and subclasses |
||
| 608 | if ($criteria instanceof \CriteriaElement) { |
||
| 609 | $criteriaCompo->add($criteria, 'AND'); |
||
| 610 | $sqlCriteria = trim($criteriaCompo->render()); |
||
| 611 | |||
| 612 | // Remove WHERE keyword if present |
||
| 613 | $sqlCriteria = preg_replace('/^\s*WHERE\s+/i', '', $sqlCriteria ?? ''); |
||
| 614 | |||
| 615 | if ('' !== $sqlCriteria) { |
||
| 616 | $whereParts[] = $sqlCriteria; |
||
| 617 | } |
||
| 618 | |||
| 619 | $limit = (int)$criteria->getLimit(); |
||
| 620 | $start = (int)$criteria->getStart(); |
||
| 621 | } |
||
| 622 | |||
| 623 | // Build WHERE clause |
||
| 624 | if (!empty($whereParts)) { |
||
| 625 | $sql .= ' WHERE ' . implode(' AND ', $whereParts); |
||
| 626 | } |
||
| 627 | |||
| 628 | // Handle ORDER BY with enhanced security whitelist |
||
| 629 | if ($criteria instanceof \CriteriaElement) { |
||
| 630 | $sort = trim($criteria->getSort()); |
||
| 631 | $order = trim($criteria->getOrder()); |
||
| 632 | if ('' !== $sort) { |
||
| 633 | // Use the whitelist method for safe sorting columns |
||
| 634 | $allowedSorts = $this->allowedSortMap(); |
||
| 635 | |||
| 636 | if (isset($allowedSorts[$sort])) { |
||
| 637 | $orderDirection = ('DESC' === strtoupper($order)) ? ' DESC' : ' ASC'; |
||
| 638 | $sql .= ' ORDER BY ' . $allowedSorts[$sort] . $orderDirection; |
||
| 639 | } |
||
| 640 | } |
||
| 641 | } |
||
| 642 | |||
| 643 | // Execute query with comprehensive error handling |
||
| 644 | $result = $this->userHandler->db->query($sql, $limit, $start); |
||
| 645 | |||
| 646 | if (!$this->userHandler->db->isResultSet($result)) { |
||
| 647 | // Enhanced error logging with security considerations |
||
| 648 | $logger = class_exists('XoopsLogger') ? \XoopsLogger::getInstance() : null; |
||
| 649 | $error = $this->userHandler->db->error(); |
||
| 650 | |||
| 651 | $msg = "Database query failed in " . __METHOD__ . ": {$error}"; |
||
| 652 | |||
| 653 | if ($isDebug) { |
||
| 654 | // Comprehensive log sanitizers to prevent injection and spoofing attacks |
||
| 655 | $sanitizeLogValue = static function ($value): string { |
||
| 656 | $s = (string)$value; |
||
| 657 | // Strip ASCII control chars (including CR/LF) and DEL |
||
| 658 | $s = preg_replace('/[\x00-\x1F\x7F]/', '', $s); |
||
| 659 | // Strip Unicode bidi/isolation controls that can spoof log layout |
||
| 660 | // U+202A..U+202E (LRE..RLO) and U+2066..U+2069 (LRI..PDI) |
||
| 661 | $s = preg_replace('/[\x{202A}-\x{202E}\x{2066}-\x{2069}]/u', '', $s); |
||
| 662 | // Collapse excessive whitespace |
||
| 663 | $s = preg_replace('/\s+/', ' ', $s); |
||
| 664 | // Length cap with mbstring fallback |
||
| 665 | if (function_exists('mb_substr')) { |
||
| 666 | $s = mb_substr($s, 0, 256, 'UTF-8'); |
||
| 667 | } else { |
||
| 668 | $s = substr($s, 0, 256); |
||
| 669 | } |
||
| 670 | return $s; |
||
| 671 | }; |
||
| 672 | |||
| 673 | $sanitizeMethod = static function ($method) use ($sanitizeLogValue): string { |
||
| 674 | $m = strtoupper($sanitizeLogValue($method)); |
||
| 675 | $allow = ['GET', 'POST', 'PUT', 'DELETE', 'PATCH', 'HEAD', 'OPTIONS']; |
||
| 676 | return in_array($m, $allow, true) ? $m : 'OTHER'; |
||
| 677 | }; |
||
| 678 | |||
| 679 | $sanitizeUri = static function ($uri) use ($sanitizeLogValue): string { |
||
| 680 | $u = (string)$uri; |
||
| 681 | $parts = parse_url($u); |
||
| 682 | $path = $sanitizeLogValue($parts['path'] ?? '/'); |
||
| 683 | // Redact sensitive query params |
||
| 684 | $qs = ''; |
||
| 685 | if (!empty($parts['query'])) { |
||
| 686 | parse_str($parts['query'], $q); |
||
| 687 | $redact = ['token', 'access_token', 'id_token', 'password', 'pass', 'pwd', 'secret', 'key', 'api_key', 'apikey', 'auth', 'authorization', 'session', 'sid', 'code']; |
||
| 688 | foreach ($q as $k => &$v) { |
||
| 689 | $kLower = strtolower((string)$k); |
||
| 690 | if (in_array($kLower, $redact, true)) { |
||
| 691 | $v = 'REDACTED'; |
||
| 692 | } else { |
||
| 693 | $v = is_array($v) ? $sanitizeLogValue(json_encode($v)) : $sanitizeLogValue($v); |
||
| 694 | } |
||
| 695 | } |
||
| 696 | unset($v); |
||
| 697 | $qs = $sanitizeLogValue(http_build_query($q)); |
||
| 698 | } |
||
| 699 | return $qs !== '' ? $path . '?' . $qs : $path; |
||
| 700 | }; |
||
| 701 | |||
| 702 | // Add correlation context for easier debugging |
||
| 703 | $context = [ |
||
| 704 | 'user_id' => isset($GLOBALS['xoopsUser']) && $GLOBALS['xoopsUser'] |
||
| 705 | ? (int)$GLOBALS['xoopsUser']->getVar('uid') |
||
| 706 | : 'anonymous', |
||
| 707 | 'uri' => isset($_SERVER['REQUEST_URI']) |
||
| 708 | ? $sanitizeUri($_SERVER['REQUEST_URI']) |
||
| 709 | : 'cli', |
||
| 710 | 'method' => isset($_SERVER['REQUEST_METHOD']) |
||
| 711 | ? $sanitizeMethod($_SERVER['REQUEST_METHOD']) |
||
| 712 | : 'CLI', |
||
| 713 | 'groups_count' => (int)count($validGroups), |
||
| 714 | ]; |
||
| 715 | $msg .= ' Context: ' . json_encode($context, JSON_UNESCAPED_SLASHES); |
||
| 716 | $msg .= ' SQL: ' . $redactSql($sql); |
||
| 717 | } |
||
| 718 | |||
| 719 | if ($logger) { |
||
| 720 | $logger->handleError(E_USER_WARNING, $msg, __FILE__, __LINE__); |
||
| 721 | } else { |
||
| 722 | // Enhanced fallback logging with file/line info |
||
| 723 | error_log($msg . " in " . __FILE__ . " on line " . __LINE__); |
||
| 724 | } |
||
| 725 | |||
| 726 | return $ret; |
||
| 727 | } |
||
| 728 | |||
| 729 | // Process results with enhanced type safety |
||
| 730 | while (false !== ($myrow = $this->userHandler->db->fetchArray($result))) { |
||
| 731 | if ($asobject) { |
||
| 732 | $user = new XoopsUser(); |
||
| 733 | $user->assignVars($myrow); |
||
| 734 | if ($id_as_key) { |
||
| 735 | $ret[(int)$myrow['uid']] = $user; |
||
| 736 | } else { |
||
| 737 | $ret[] = $user; |
||
| 738 | } |
||
| 739 | } else { |
||
| 740 | // Ensure consistent integer return for UIDs |
||
| 741 | $ret[] = (int)$myrow['uid']; |
||
| 742 | } |
||
| 743 | } |
||
| 744 | |||
| 745 | return $ret; |
||
| 746 | } |
||
| 784 |
Loading history...
