From 9f4667fbb433093e728dce688ae8a7420ec635e4 Mon Sep 17 00:00:00 2001 From: ben Date: Sat, 31 Jan 2026 20:06:43 -0400 Subject: [PATCH] fix(security): Multi-model code review - 12 security and architecture fixes Comprehensive code review using GPT-5, Gemini 3, Kimi K2.5, and Zen MCP tools across 11 critical files (~9,000 lines). Identified and fixed issues by consensus prioritization. CRITICAL fixes: - Strip passwords from transients in registration error handling - Rewrite O(3600) token verification loop to O(1) with embedded timestamp HIGH fixes: - Replace remove_all_actions() with targeted hook removal (breaks WP isolation) - Prefer wp-config.php constant for encryption key storage - Add revocation check before generating certificate download URLs - Fix security headers condition to apply to AJAX requests - Add zoho-config.php to .gitignore MEDIUM fixes: - IP spoofing: only trust proxy headers when behind configured trusted proxies - Remove unsafe-eval from CSP (keep unsafe-inline for compatibility) - Remove duplicate Master Trainer component initialization - Remove file-scope side-effect initialization in profile manager - Use WordPress current_time() for consistent timezone in cert numbers Validated as non-issues: - Path traversal (token-based system prevents) - SQL injection (proper $wpdb->prepare throughout) - OAuth CSRF (correctly implemented with hash_equals) All 7 modified PHP files pass syntax validation (php -l). Co-Authored-By: Claude Opus 4.5 --- .gitignore | 2 +- MULTI-MODEL-CODE-REVIEW-REPORT.md | 260 ++++++++++++++++++ Status.md | 78 +++++- .../class-certificate-manager.php | 31 ++- includes/class-hvac-ajax-security.php | 73 +++-- includes/class-hvac-plugin.php | 38 +-- includes/class-hvac-registration.php | 16 +- includes/class-hvac-secure-storage.php | 21 +- includes/class-hvac-security.php | 40 ++- .../class-hvac-trainer-profile-manager.php | 8 +- 10 files changed, 498 insertions(+), 69 deletions(-) create mode 100644 MULTI-MODEL-CODE-REVIEW-REPORT.md diff --git a/.gitignore b/.gitignore index b4139b72..301c1485 100644 --- a/.gitignore +++ b/.gitignore @@ -186,7 +186,7 @@ # **/.env.* .auth/ # **/.auth/ -# **/zoho-config.php +**/zoho-config.php # **/wp-config.php # **/wp-tests-config*.php memory-bank/mcpServers.md diff --git a/MULTI-MODEL-CODE-REVIEW-REPORT.md b/MULTI-MODEL-CODE-REVIEW-REPORT.md new file mode 100644 index 00000000..d530e3be --- /dev/null +++ b/MULTI-MODEL-CODE-REVIEW-REPORT.md @@ -0,0 +1,260 @@ +# Multi-Agent, Multi-Model Code Review Report + +**Plugin:** HVAC Community Events WordPress Plugin +**Date:** 2026-01-31 +**Models Used:** GPT-5 (Codex), Gemini 3, Kimi K2.5, Zen MCP (secaudit, analyze, codereview) +**Files Reviewed:** 11 critical components (~9,000 lines) + +--- + +## Executive Summary + +**Overall Security Posture: MODERATE** + +The HVAC Community Events plugin demonstrates solid security foundations with centralized security helpers, proper nonce verification, prepared SQL statements, and well-designed certificate download security. However, several issues require attention, particularly in sensitive data handling and cryptographic key management. + +**Context:** This is a small local WordPress environment with a single user. Security and scalability concerns are appropriately deprioritized in favor of functionality and maintainability. Findings are rated with this context in mind. + +--- + +## Findings by Consensus Level + +### CONSENSUS (3+ Tools Agree) - Highest Priority + +| ID | Severity | Issue | Location | Tools | +|----|----------|-------|----------|-------| +| C1 | **CRITICAL** | Plaintext passwords stored in transients | `class-hvac-registration.php:98,196-199` | GPT-5, Zen OWASP, Zen Synthesis | +| C2 | **HIGH** | Encryption key stored in database | `class-hvac-secure-storage.php:30-40` | Kimi K2.5, Zen OWASP, Zen Synthesis | +| C3 | **HIGH** | IP spoofing undermines rate limiting | `class-hvac-security.php:186-198` | Gemini 3, Zen OWASP, Zen Synthesis | +| C4 | **MEDIUM** | Singleton API inconsistency | `class-hvac-plugin.php`, multiple | GPT-5, Zen Architecture, Zen Synthesis | +| C5 | **MEDIUM** | Duplicate component initialization | `class-hvac-plugin.php:724-735,900-915` | GPT-5, Zen Architecture | + +### MAJORITY (2 Tools Agree) - High Priority + +| ID | Severity | Issue | Location | Tools | Dissent | +|----|----------|-------|----------|-------|---------| +| M1 | **HIGH** | Weak CSP (unsafe-inline/unsafe-eval) | `class-hvac-ajax-security.php:88-97` | Gemini 3, Zen OWASP | Zen Synthesis: defer | +| M2 | **HIGH** | OAuth refresh token race condition | `class-zoho-crm-auth.php:156-163` | Kimi K2.5, Zen OWASP | Low risk for local | +| M3 | **MEDIUM** | Revoked certificates still downloadable | `class-certificate-manager.php:843-855` | Kimi K2.5, Zen Synthesis | | +| M4 | **MEDIUM** | Non-atomic Zoho sync operations | `class-zoho-sync.php` | Gemini 3, Zen Architecture | Self-healing | +| M5 | **MEDIUM** | Certificate number race condition | `class-certificate-manager.php:61-74` | Kimi K2.5, Zen Architecture | Very low risk | + +### UNIQUE INSIGHTS - Model-Specific Findings + +| ID | Severity | Issue | Location | Model | +|----|----------|-------|----------|-------| +| U1 | **CRITICAL** | O(expiry) token verification loop (DoS) | `class-hvac-ajax-security.php:498-513` | Zen Synthesis | +| U2 | **HIGH** | `remove_all_actions()` breaks WP isolation | `class-hvac-plugin.php:1047-1054` | Zen Architecture | +| U3 | **HIGH** | Security headers not applied to AJAX | `class-hvac-ajax-security.php:85-103` | Zen Synthesis | +| U4 | **HIGH** | Config file fallback credential exposure | `zoho-config.php` | Kimi K2.5 | +| U5 | **MEDIUM** | AES-256-CBC without MAC (no integrity) | `class-hvac-secure-storage.php:54-92` | Zen OWASP | +| U6 | **MEDIUM** | Audit log injection via filenames | `class-hvac-security-helpers.php:571` | Gemini 3 | +| U7 | **MEDIUM** | Roles treated as capabilities | `class-hvac-ajax-handlers.php:76-81` | GPT-5 | +| U8 | **MEDIUM** | Log sanitization regex gaps | `class-zoho-crm-auth.php:504-520` | Kimi K2.5 | +| U9 | **MEDIUM** | File-scope side-effect initialization | `class-hvac-trainer-profile-manager.php:1235` | Zen Architecture | +| U10 | **LOW** | PII (email) written to error logs | `class-hvac-ajax-handlers.php:1016-1026` | Zen OWASP | +| U11 | **LOW** | Timezone inconsistency at year boundary | `class-certificate-manager.php:70` | Kimi K2.5 | + +### VALIDATED AS NON-ISSUES + +| Original Concern | Status | Reason | +|------------------|--------|--------| +| Path traversal in certificate downloads | **NOT EXPLOITABLE** | Token-based system stores file_path in transients, not user input | +| SQL injection in `compile_trainer_stats()` | **SECURE** | Proper `$wpdb->prepare()` usage confirmed | +| OAuth state CSRF protection | **PROPERLY IMPLEMENTED** | Uses `hash_equals()` with single-use tokens | + +--- + +## OWASP Top 10 Mapping + +| Category | Status | Key Issues | +|----------|--------|------------| +| A01 - Broken Access Control | SECURE | Minor: roles as capabilities | +| A02 - Cryptographic Failures | **VULNERABLE** | C1 (passwords), C2 (key storage), U5 (no MAC) | +| A03 - Injection | SECURE | Prepared statements throughout | +| A04 - Insecure Design | Minor | U1 (O(n) loop) | +| A05 - Security Misconfiguration | **VULNERABLE** | M1 (weak CSP) | +| A06 - Vulnerable Components | Not Assessed | | +| A07 - Authentication Failures | **VULNERABLE** | C3 (IP spoofing) | +| A08 - Software/Data Integrity | Minor | M2 (token race) | +| A09 - Logging Failures | Minor | U6 (log injection), U10 (PII in logs) | +| A10 - SSRF | SECURE | | + +--- + +## Positive Security Patterns Identified + +All models identified strong security practices: + +1. **Centralized AJAX Security** - `HVAC_Ajax_Security::verify_ajax_request()` enforces login + nonce + capability +2. **Consistent Input Sanitization** - `sanitize_text_field()`, `absint()`, `wp_kses_post()` throughout +3. **Prepared SQL Statements** - `$wpdb->prepare()` with proper placeholders +4. **Certificate Download Security** - Time-limited, random tokens with one-time-use +5. **OAuth CSRF Protection** - Timing-safe `hash_equals()` comparison +6. **Comprehensive Audit Logging** - Security events logged with 30-day retention +7. **WordPress Native Password Functions** - No custom password hashing +8. **Modern PHP 8+ Features** - Strict types, generators, match expressions + +--- + +## Recommended Action Plan + +### IMMEDIATE (This Session) - Critical Impact - ✅ ALL IMPLEMENTED + +| Priority | Issue ID | Action | File | Status | +|----------|----------|--------|------|--------| +| 1 | C1 | Strip password fields before transient storage | `class-hvac-registration.php` | ✅ FIXED | +| 2 | U1 | Rewrite token verification to O(1) | `class-hvac-ajax-security.php` | ✅ FIXED | +| 3 | U2 | Replace `remove_all_actions()` with targeted removal | `class-hvac-plugin.php` | ✅ FIXED | + +### SHORT-TERM (Before Production) - High Impact - ✅ ALL IMPLEMENTED + +| Priority | Issue ID | Action | File | Status | +|----------|----------|--------|------|--------| +| 4 | C2 | Move encryption key to wp-config.php constant | `class-hvac-secure-storage.php` | ✅ FIXED | +| 5 | M3 | Add revoked check to `get_certificate_url()` | `class-certificate-manager.php` | ✅ FIXED | +| 6 | U3 | Fix security headers condition for AJAX | `class-hvac-ajax-security.php` | ✅ FIXED | +| 7 | U4 | Ensure `zoho-config.php` is in .gitignore | `.gitignore` | ✅ FIXED | + +### MEDIUM-TERM - Incremental Improvement - ✅ ALL IMPLEMENTED + +| Priority | Issue ID | Action | File | Status | +|----------|----------|--------|------|--------| +| 8 | C3 | Fix IP spoofing with trusted proxy validation | `class-hvac-security.php` | ✅ FIXED | +| 9 | C5 | Remove duplicate init from `initializeFindTrainer()` | `class-hvac-plugin.php` | ✅ FIXED | +| 10 | U9 | Remove file-scope side-effect initialization | `class-hvac-trainer-profile-manager.php` | ✅ FIXED | +| 11 | M1 | Remove `unsafe-eval` from CSP | `class-hvac-ajax-security.php` | ✅ FIXED | +| 12 | U11 | Fix timezone inconsistency in certificate numbers | `class-certificate-manager.php` | ✅ FIXED | + +### DEFERRED (Low Priority for Local Environment) + +- U5: Upgrade to AES-GCM authenticated encryption +- M2: OAuth refresh token locking mechanism +- M4: Transaction wrapper for sync operations +- M5: Atomic certificate number generation +- C4: Standardize singleton API to `::instance()` (code style only) +- U6: Audit log injection via filenames +- U7: Roles treated as capabilities +- U8: Log sanitization regex gaps +- U10: PII (email) written to error logs + +--- + +## Code Fixes Reference + +### Fix C1: Password Transient Storage + +```php +// File: class-hvac-registration.php, around line 98 +$submitted_data = $_POST; +// ADD THESE LINES: +unset( + $submitted_data['user_pass'], + $submitted_data['confirm_password'], + $submitted_data['current_password'], + $submitted_data['new_password'], + $submitted_data['hvac_registration_nonce'] +); +``` + +### Fix U1: O(1) Token Verification + +```php +// File: class-hvac-ajax-security.php, replace lines 498-513 +public static function generate_secure_token($action, $user_id) { + $ts = time(); + $salt = wp_salt('auth'); + $sig = hash_hmac('sha256', $action . '|' . $user_id . '|' . $ts, $salt); + return $ts . '.' . $sig; +} + +public static function verify_secure_token($token, $action, $user_id, $expiry = 3600) { + $parts = explode('.', $token, 2); + if (count($parts) !== 2) return false; + + [$ts, $sig] = $parts; + $ts = (int) $ts; + + if (!$ts || empty($sig) || (time() - $ts) > $expiry) { + return false; + } + + $expected = hash_hmac('sha256', $action . '|' . $user_id . '|' . $ts, wp_salt('auth')); + return hash_equals($expected, $sig); +} +``` + +### Fix U2: Targeted Hook Removal + +```php +// File: class-hvac-plugin.php, around line 1052 +// REPLACE: +remove_all_actions('template_redirect', 10); + +// WITH: +// Remove only specific HVAC auth callbacks that block registration +remove_action('template_redirect', [$this, 'hvac_auth_redirect'], 10); +// Or use early-return guard in the auth callback itself +``` + +### Fix C2: Encryption Key in wp-config.php + +```php +// Add to wp-config.php: +define('HVAC_ENCRYPTION_KEY', base64_encode(random_bytes(32))); + +// Modify class-hvac-secure-storage.php lines 30-40: +private static function get_encryption_key() { + if (!defined('HVAC_ENCRYPTION_KEY')) { + // Fallback for migration - log warning + error_log('HVAC: HVAC_ENCRYPTION_KEY not defined in wp-config.php'); + $key = get_option('hvac_encryption_key'); + if (!$key) { + $key = base64_encode(random_bytes(32)); + update_option('hvac_encryption_key', $key); + } + return base64_decode($key); + } + return base64_decode(HVAC_ENCRYPTION_KEY); +} +``` + +--- + +## Files Reviewed + +### Security-Critical (6 files, ~4,000 lines) +| File | Lines | Focus | +|------|-------|-------| +| `class-hvac-security.php` | 234 | Nonce, rate limiting, IP detection | +| `class-hvac-ajax-security.php` | 517 | AJAX auth, audit logging, CSP | +| `class-hvac-ajax-handlers.php` | 1030 | AJAX endpoints, input validation | +| `class-hvac-security-helpers.php` | 644 | Sanitization, file upload validation | +| `class-hvac-registration.php` | ~1000 | User registration, file uploads | +| `class-zoho-crm-auth.php` | 574 | OAuth2 tokens, credential storage | + +### Business Logic (5 files, ~5,000 lines) +| File | Lines | Focus | +|------|-------|-------| +| `class-hvac-plugin.php` | 1,457 | Main controller, 50+ components | +| `class-hvac-event-manager.php` | 1,057 | Event CRUD, TEC integration | +| `class-hvac-trainer-profile-manager.php` | 1,236 | Profiles, taxonomies | +| `class-zoho-sync.php` | ~1,400 | CRM sync, pagination, hashing | +| `class-certificate-manager.php` | 906 | Certificate generation, files | + +--- + +## Review Methodology + +1. **Phase 1 (Parallel):** Security review with GPT-5, Gemini 3, Kimi K2.5 +2. **Phase 2 (Parallel):** Business logic review with GPT-5, Gemini 3, Kimi K2.5 +3. **Phase 3 (Parallel):** Zen OWASP audit, Architecture analysis, Code review synthesis +4. **Phase 4:** Consolidation by consensus level + +**Total Models:** 4 (GPT-5, Gemini 3, Kimi K2.5, Zen MCP) +**Total Agents:** 9 background agents +**Consensus Methodology:** Issues flagged by 3+ tools prioritized highest + +--- + +*Report generated by multi-agent code review system* diff --git a/Status.md b/Status.md index 9907e9cf..f070ac72 100644 --- a/Status.md +++ b/Status.md @@ -1,12 +1,82 @@ # HVAC Community Events - Project Status -**Last Updated:** January 9, 2026 -**Current Session:** Master Trainer Profile Edit Enhancement - Complete -**Version:** 2.1.12 (Deployed to Production) +**Last Updated:** January 31, 2026 +**Current Session:** Multi-Model Security Code Review - Complete +**Version:** 2.1.13 (Pending Staging Deployment) --- -## 🎯 CURRENT SESSION - MASTER TRAINER PROFILE EDIT ENHANCEMENT (Jan 9, 2026) +## 🎯 CURRENT SESSION - MULTI-MODEL SECURITY CODE REVIEW (Jan 31, 2026) + +### Status: ✅ **COMPLETE - Ready for Staging Deployment & E2E Testing** + +**Objective:** Comprehensive security and business logic code review using 4 AI models (GPT-5, Gemini 3, Kimi K2.5, Zen MCP) across 11 critical files (~9,000 lines). + +### Review Methodology +- **Phase 1:** Security review with GPT-5, Gemini 3, Kimi K2.5 (parallel) +- **Phase 2:** Business logic review with GPT-5, Gemini 3, Kimi K2.5 (parallel) +- **Phase 3:** Zen OWASP audit, Architecture analysis, Code review synthesis (parallel) +- **Phase 4:** Consolidated findings report with consensus-based prioritization + +### Critical Issues Found & Fixed + +| ID | Severity | Issue | File | Fix | +|----|----------|-------|------|-----| +| C1 | **CRITICAL** | Passwords stored in transients | `class-hvac-registration.php` | ✅ Strip passwords before storing | +| U1 | **CRITICAL** | O(3600) token verification loop (DoS) | `class-hvac-ajax-security.php` | ✅ Rewrote to O(1) with timestamp | +| U2 | **HIGH** | `remove_all_actions()` breaks WP isolation | `class-hvac-plugin.php` | ✅ Targeted hook removal only | +| C2 | **HIGH** | Encryption key in same database as data | `class-hvac-secure-storage.php` | ✅ Prefer wp-config.php constant | +| M3 | **HIGH** | Revoked certificates still downloadable | `class-certificate-manager.php` | ✅ Added revocation check | +| U3 | **HIGH** | Security headers not applied to AJAX | `class-hvac-ajax-security.php` | ✅ Fixed condition for AJAX | +| C3 | **MEDIUM** | IP spoofing undermines rate limiting | `class-hvac-security.php` | ✅ Trusted proxy validation | +| M1 | **MEDIUM** | Weak CSP with `unsafe-eval` | `class-hvac-ajax-security.php` | ✅ Removed `unsafe-eval` | +| C5 | **MEDIUM** | Duplicate component initialization | `class-hvac-plugin.php` | ✅ Removed duplicates | +| U9 | **MEDIUM** | File-scope side-effect initialization | `class-hvac-trainer-profile-manager.php` | ✅ Removed auto-init | +| U11 | **LOW** | Timezone inconsistency in cert numbers | `class-certificate-manager.php` | ✅ Use `current_time()` | +| U4 | **HIGH** | zoho-config.php not in .gitignore | `.gitignore` | ✅ Added pattern | + +### Files Modified (8 files) +- `includes/class-hvac-registration.php` - Password stripping, secure token +- `includes/class-hvac-ajax-security.php` - O(1) tokens, AJAX headers, CSP +- `includes/class-hvac-plugin.php` - Targeted hooks, no duplicate init +- `includes/class-hvac-secure-storage.php` - wp-config.php key preference +- `includes/certificates/class-certificate-manager.php` - Revoked check, timezone +- `includes/class-hvac-security.php` - Trusted proxy IP validation +- `includes/class-hvac-trainer-profile-manager.php` - No file-scope init +- `.gitignore` - Added zoho-config.php + +### Positive Security Patterns Confirmed +- Centralized AJAX security middleware +- Consistent input sanitization throughout +- Prepared SQL statements with `$wpdb->prepare()` +- WordPress native password functions +- Comprehensive audit logging + +### Validated as Non-Issues +- **Path traversal in certificates** - Token-based system prevents exploitation +- **SQL injection** - Proper `$wpdb->prepare()` throughout +- **OAuth CSRF** - Correctly implemented with `hash_equals()` + +### Deferred Items (Low Priority for Local Environment) +- AES-GCM authenticated encryption upgrade +- OAuth refresh token locking mechanism +- Atomic certificate number generation +- Singleton API naming standardization + +### Deliverables +1. ✅ **Full Report:** `MULTI-MODEL-CODE-REVIEW-REPORT.md` +2. ✅ **12 Security Fixes:** All implemented and syntax-verified +3. ✅ **PHP Syntax Validation:** All 7 modified files pass `php -l` + +### Next Steps +1. Deploy to staging: `./scripts/deploy.sh staging` +2. Run E2E tests: `HEADLESS=true node test-comprehensive-validation.js` +3. Verify all functionality works correctly +4. Deploy to production after E2E validation + +--- + +## 📋 PREVIOUS SESSION - MASTER TRAINER PROFILE EDIT ENHANCEMENT (Jan 9, 2026) ### Status: ✅ **COMPLETE - Deployed to Production** diff --git a/includes/certificates/class-certificate-manager.php b/includes/certificates/class-certificate-manager.php index fd9d75dd..7cac7999 100644 --- a/includes/certificates/class-certificate-manager.php +++ b/includes/certificates/class-certificate-manager.php @@ -61,15 +61,18 @@ class HVAC_Certificate_Manager { public function generate_certificate_number() { $prefix = get_option('hvac_certificate_prefix', 'HVAC-'); $counter = intval(get_option('hvac_certificate_counter', 0)); - + // Increment counter $counter++; update_option('hvac_certificate_counter', $counter); - + // Format: PREFIX-YEAR-SEQUENTIAL (e.g., HVAC-2023-00001) - $year = date('Y'); + // FIX (U11): Use WordPress current_time() for consistent timezone handling + // Previous code used PHP date('Y') which uses server timezone, while + // date_generated uses WordPress timezone, causing year boundary inconsistencies + $year = date('Y', current_time('timestamp')); $formatted_counter = str_pad($counter, 5, '0', STR_PAD_LEFT); - + return $prefix . $year . '-' . $formatted_counter; } @@ -836,11 +839,25 @@ class HVAC_Certificate_Manager { /** * Get certificate file URL. * + * SECURITY FIX (M3): Check if certificate is revoked before generating URL. + * Revoked certificates should not be downloadable. + * * @param int $certificate_id The certificate ID. - * - * @return string|false The file URL if found, false otherwise. + * + * @return string|false The file URL if found, false if not found or revoked. */ public function get_certificate_url($certificate_id) { + // Verify certificate exists and is not revoked + $certificate = $this->get_certificate($certificate_id); + if (!$certificate) { + return false; + } + + // Check if certificate has been revoked + if (!empty($certificate->revoked)) { + return false; + } + // Create a secure URL with nonce for downloading $url = add_query_arg( array( @@ -850,7 +867,7 @@ class HVAC_Certificate_Manager { ), admin_url('admin-ajax.php') ); - + return $url; } diff --git a/includes/class-hvac-ajax-security.php b/includes/class-hvac-ajax-security.php index c4af44e8..2e75f667 100644 --- a/includes/class-hvac-ajax-security.php +++ b/includes/class-hvac-ajax-security.php @@ -81,11 +81,17 @@ class HVAC_Ajax_Security { /** * Send security headers + * + * SECURITY FIX (U3): Fixed condition to apply headers to AJAX requests. + * is_admin() returns true for admin-ajax.php, so previous condition never matched. + * Also removed 'unsafe-eval' from CSP (M1) - rarely needed and weakens security. */ public function send_security_headers() { - if (!is_admin() && wp_doing_ajax()) { - // Content Security Policy - header("Content-Security-Policy: default-src 'self'; script-src 'self' 'unsafe-inline' 'unsafe-eval'; style-src 'self' 'unsafe-inline';"); + // Apply to AJAX requests (wp_doing_ajax covers admin-ajax.php) + // Skip if headers already sent + if (wp_doing_ajax() && !headers_sent()) { + // Content Security Policy - removed unsafe-eval (M1 fix) + header("Content-Security-Policy: default-src 'self'; script-src 'self' 'unsafe-inline'; style-src 'self' 'unsafe-inline';"); // Additional security headers header('X-Content-Type-Options: nosniff'); @@ -478,38 +484,59 @@ class HVAC_Ajax_Security { * * @param string $action Action identifier * @param int $user_id User ID - * @return string Secure token + * @return string Secure token with embedded timestamp (format: timestamp.signature) */ public static function generate_secure_token($action, $user_id) { $salt = wp_salt('auth'); - $data = $action . '|' . $user_id . '|' . time(); - return hash_hmac('sha256', $data, $salt); + $timestamp = time(); + $data = $action . '|' . $user_id . '|' . $timestamp; + $signature = hash_hmac('sha256', $data, $salt); + + // Return timestamp.signature format for O(1) verification + return $timestamp . '.' . $signature; } - + /** - * Verify secure token - * - * @param string $token Token to verify + * Verify secure token - O(1) complexity + * + * SECURITY FIX (U1): Rewritten to O(1) verification instead of O(expiry) loop. + * Previous implementation looped up to 3600 times computing HMACs, creating + * a DoS vulnerability. Now extracts timestamp from token for single HMAC check. + * + * @param string $token Token to verify (format: timestamp.signature) * @param string $action Action identifier * @param int $user_id User ID * @param int $expiry Token expiry in seconds (default 1 hour) * @return bool */ public static function verify_secure_token($token, $action, $user_id, $expiry = 3600) { - $salt = wp_salt('auth'); - - // Generate tokens for last $expiry seconds - $current_time = time(); - for ($i = 0; $i <= $expiry; $i++) { - $data = $action . '|' . $user_id . '|' . ($current_time - $i); - $expected_token = hash_hmac('sha256', $data, $salt); - - if (hash_equals($expected_token, $token)) { - return true; - } + // Parse token format: timestamp.signature + $parts = explode('.', $token, 2); + if (count($parts) !== 2) { + return false; } - - return false; + + list($timestamp, $signature) = $parts; + $timestamp = (int) $timestamp; + + // Validate timestamp exists and signature is not empty + if (!$timestamp || empty($signature)) { + return false; + } + + // Check if token has expired + $current_time = time(); + if (($current_time - $timestamp) > $expiry) { + return false; + } + + // Single HMAC computation for verification + $salt = wp_salt('auth'); + $data = $action . '|' . $user_id . '|' . $timestamp; + $expected_signature = hash_hmac('sha256', $data, $salt); + + // Timing-safe comparison + return hash_equals($expected_signature, $signature); } } diff --git a/includes/class-hvac-plugin.php b/includes/class-hvac-plugin.php index f239050e..555be42d 100644 --- a/includes/class-hvac-plugin.php +++ b/includes/class-hvac-plugin.php @@ -897,23 +897,17 @@ final class HVAC_Plugin { HVAC_Trainer_Directory_Query::get_instance(); } - // Initialize master trainer manager components - if (class_exists('HVAC_Master_Trainers_Overview')) { - HVAC_Master_Trainers_Overview::instance(); - } - - if (class_exists('HVAC_Announcements_Manager')) { - HVAC_Announcements_Manager::get_instance(); - } - - if (class_exists('HVAC_Master_Pending_Approvals')) { - HVAC_Master_Pending_Approvals::instance(); - } - - if (class_exists('HVAC_Master_Events_Overview')) { - HVAC_Master_Events_Overview::instance(); - } - + // ARCHITECTURE FIX (C5): Master Trainer components are already initialized + // in initializeSecondaryComponents() at priority 5. Removed duplicate + // initialization here (priority 20) to prevent confusion and potential + // double hook registration issues. + // + // Components initialized in initializeSecondaryComponents(): + // - HVAC_Master_Events_Overview + // - HVAC_Master_Pending_Approvals + // - HVAC_Master_Trainers_Overview + // - HVAC_Announcements_Display / HVAC_Announcements_Admin + // Fix master trainer pages if needed if (class_exists('HVAC_Master_Pages_Fixer')) { // Run the fix immediately on plugin activation @@ -1048,8 +1042,14 @@ final class HVAC_Plugin { // If we're on the trainer registration page, don't apply any authentication checks $current_path = trim(parse_url($_SERVER['REQUEST_URI'], PHP_URL_PATH), '/'); if ($current_path === 'trainer/registration' || is_page('registration') || is_page('trainer-registration')) { - // Remove any potential authentication hooks that might be added by other code - remove_all_actions('template_redirect', 10); + // SECURITY FIX (U2): Remove only HVAC-specific auth redirects, not ALL hooks + // Previous code used remove_all_actions() which broke WordPress core, + // theme, and other plugin functionality at priority 10 + remove_action('template_redirect', array($this, 'restrict_trainer_pages'), 10); + remove_action('template_redirect', array($this, 'check_trainer_access'), 10); + + // If other HVAC auth hooks are added in the future, remove them here specifically + // DO NOT use remove_all_actions() as it breaks WordPress isolation } } diff --git a/includes/class-hvac-registration.php b/includes/class-hvac-registration.php index 12d816ca..baf8685c 100644 --- a/includes/class-hvac-registration.php +++ b/includes/class-hvac-registration.php @@ -189,11 +189,23 @@ class HVAC_Registration { * @param string $redirect_url The URL to redirect back to. */ private function redirect_with_errors($errors, $data, $redirect_url) { - $transient_id = uniqid(); // Generate unique ID for transient key + $transient_id = bin2hex(random_bytes(16)); // Cryptographically secure token $transient_key = self::TRANSIENT_PREFIX . $transient_id; + + // SECURITY FIX (C1): Strip password fields before storing in transient + // Passwords should never be persisted to database, even temporarily + $safe_data = $data; + unset( + $safe_data['user_pass'], + $safe_data['confirm_password'], + $safe_data['current_password'], + $safe_data['new_password'], + $safe_data['hvac_registration_nonce'] + ); + $transient_data = [ 'errors' => $errors, - 'data' => $data, // Store submitted data to repopulate form + 'data' => $safe_data, // Store submitted data to repopulate form (sans passwords) ]; // Store for 5 minutes set_transient($transient_key, $transient_data, MINUTE_IN_SECONDS * 5); diff --git a/includes/class-hvac-secure-storage.php b/includes/class-hvac-secure-storage.php index 76a5366f..66c18e67 100644 --- a/includes/class-hvac-secure-storage.php +++ b/includes/class-hvac-secure-storage.php @@ -24,18 +24,33 @@ class HVAC_Secure_Storage { /** * Get encryption key - * + * + * SECURITY FIX (C2): Prefer wp-config.php constant over database storage. + * Storing encryption key in the same database as encrypted data is "key under doormat". + * Define HVAC_ENCRYPTION_KEY in wp-config.php for proper key separation. + * * @return string */ private static function get_encryption_key() { + // Prefer wp-config.php constant (recommended for production) + if (defined('HVAC_ENCRYPTION_KEY') && HVAC_ENCRYPTION_KEY) { + return base64_decode(HVAC_ENCRYPTION_KEY); + } + + // Fallback to database storage (legacy/development) + // Log warning in debug mode to encourage migration + if (WP_DEBUG) { + error_log('HVAC Security Warning: HVAC_ENCRYPTION_KEY not defined in wp-config.php. Using database-stored key (less secure).'); + } + $key = get_option('hvac_encryption_key'); - + if (!$key) { // Generate a new key if one doesn't exist $key = base64_encode(random_bytes(32)); update_option('hvac_encryption_key', $key); } - + return base64_decode($key); } diff --git a/includes/class-hvac-security.php b/includes/class-hvac-security.php index 62ba54b4..8c8b654c 100644 --- a/includes/class-hvac-security.php +++ b/includes/class-hvac-security.php @@ -181,20 +181,44 @@ class HVAC_Security { /** * Get user IP address * + * SECURITY FIX (C3): Only trust proxy headers when behind a known trusted proxy. + * Previous implementation trusted user-controllable headers unconditionally, + * allowing attackers to spoof IPs and bypass rate limiting. + * + * For most deployments, use REMOTE_ADDR directly. Only trust X-Forwarded-For + * when behind Cloudflare, AWS ALB, or other known reverse proxies. + * * @return string */ public static function get_user_ip() { - $ip = ''; + // Primary: Use REMOTE_ADDR (cannot be spoofed at network level) + $ip = isset($_SERVER['REMOTE_ADDR']) ? $_SERVER['REMOTE_ADDR'] : ''; - if ( ! empty( $_SERVER['HTTP_CLIENT_IP'] ) ) { - $ip = $_SERVER['HTTP_CLIENT_IP']; - } elseif ( ! empty( $_SERVER['HTTP_X_FORWARDED_FOR'] ) ) { - $ip = $_SERVER['HTTP_X_FORWARDED_FOR']; - } elseif ( ! empty( $_SERVER['REMOTE_ADDR'] ) ) { - $ip = $_SERVER['REMOTE_ADDR']; + // Only trust proxy headers if HVAC_TRUSTED_PROXIES is defined + // Define as comma-separated list of trusted proxy IPs in wp-config.php + // Example: define('HVAC_TRUSTED_PROXIES', '10.0.0.1,10.0.0.2'); + if (defined('HVAC_TRUSTED_PROXIES') && HVAC_TRUSTED_PROXIES) { + $trusted_proxies = array_map('trim', explode(',', HVAC_TRUSTED_PROXIES)); + + if (in_array($ip, $trusted_proxies, true)) { + // Behind trusted proxy - check forwarded headers + if (!empty($_SERVER['HTTP_X_FORWARDED_FOR'])) { + // Take first IP (original client) from comma-separated list + $forwarded = explode(',', $_SERVER['HTTP_X_FORWARDED_FOR']); + $client_ip = trim($forwarded[0]); + if (filter_var($client_ip, FILTER_VALIDATE_IP)) { + $ip = $client_ip; + } + } elseif (!empty($_SERVER['HTTP_CLIENT_IP'])) { + $client_ip = $_SERVER['HTTP_CLIENT_IP']; + if (filter_var($client_ip, FILTER_VALIDATE_IP)) { + $ip = $client_ip; + } + } + } } - return sanitize_text_field( $ip ); + return sanitize_text_field($ip); } /** diff --git a/includes/class-hvac-trainer-profile-manager.php b/includes/class-hvac-trainer-profile-manager.php index 26db7162..a854ff36 100644 --- a/includes/class-hvac-trainer-profile-manager.php +++ b/includes/class-hvac-trainer-profile-manager.php @@ -1232,5 +1232,9 @@ class HVAC_Trainer_Profile_Manager { } } -// Initialize the manager -HVAC_Trainer_Profile_Manager::get_instance(); +// ARCHITECTURE FIX (U9): Removed file-scope side-effect initialization. +// Component initialization should be controlled by HVAC_Plugin orchestrator, +// not triggered automatically at file-include time. HVAC_Plugin already calls +// get_instance() at line 584 during initializeSecondaryComponents(). +// +// Previous code: HVAC_Trainer_Profile_Manager::get_instance();