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 <noreply@anthropic.com>
This commit is contained in:
parent
23dcd158ec
commit
9f4667fbb4
10 changed files with 498 additions and 69 deletions
2
.gitignore
vendored
2
.gitignore
vendored
|
|
@ -186,7 +186,7 @@
|
|||
# **/.env.*
|
||||
.auth/
|
||||
# **/.auth/
|
||||
# **/zoho-config.php
|
||||
**/zoho-config.php
|
||||
# **/wp-config.php
|
||||
# **/wp-tests-config*.php
|
||||
memory-bank/mcpServers.md
|
||||
|
|
|
|||
260
MULTI-MODEL-CODE-REVIEW-REPORT.md
Normal file
260
MULTI-MODEL-CODE-REVIEW-REPORT.md
Normal file
|
|
@ -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*
|
||||
78
Status.md
78
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**
|
||||
|
||||
|
|
|
|||
|
|
@ -67,7 +67,10 @@ class HVAC_Certificate_Manager {
|
|||
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(
|
||||
|
|
|
|||
|
|
@ -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,39 +484,60 @@ 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
|
||||
* Verify secure token - O(1) complexity
|
||||
*
|
||||
* @param string $token Token to verify
|
||||
* 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;
|
||||
}
|
||||
|
||||
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);
|
||||
}
|
||||
}
|
||||
|
||||
// Initialize the security handler
|
||||
|
|
|
|||
|
|
@ -897,22 +897,16 @@ 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')) {
|
||||
|
|
@ -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
|
||||
}
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -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);
|
||||
|
|
|
|||
|
|
@ -25,9 +25,24 @@ 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) {
|
||||
|
|
|
|||
|
|
@ -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);
|
||||
}
|
||||
|
||||
/**
|
||||
|
|
|
|||
|
|
@ -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();
|
||||
|
|
|
|||
Loading…
Reference in a new issue