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