Security Review - ICoTA Members Plugin

Code review conducted on 2026-02-06

Overview

This document outlines security and code quality concerns found during a review of the ICoTA Members plugin.

Critical Issues

None Found

No critical security vulnerabilities were identified.

Medium Priority Issues

1. Debug Function Exposed via Twig

Location: src/twigextensions/PurchaseExtension.php:141

Issue: The debugUserPurchases() function is exposed to Twig templates and reveals internal system information:

• Database query results
• Plugin internals
• Customer IDs
• Object structures

Concern:

• Could leak sensitive information if accidentally used in production templates
• Exposes internal system structure

Recommendation:

• Remove from production or wrap in environment check:

public function debugUserPurchases(User $user): array
{
    if (!Craft::$app->getConfig()->getGeneral()->devMode) {
        return ['error' => 'Debug functions only available in dev mode'];
    }
    // existing code...
}

---

Low Priority Issues

2. Duplicate Code in CpController

Location: src/controllers/CpController.php

Issue: The actionIndex() and actionExportCsv() methods contain nearly identical enrichment logic (lines 34-100 and lines 259-325).

Concern:

• Code duplication makes maintenance difficult
• Changes need to be applied in two places
• Potential for bugs if one is updated but not the other

Recommendation: Extract enrichment logic into a private method:

private function enrichMemberships(array $memberships): array
{
    // shared enrichment logic
}

---

3. Unvalidated User Input in Filters

Location: src/controllers/CpController.php:26-28

Issue:

$statusFilter = $request->getParam('status');
$chapterFilter = $request->getParam('chapter');
$countryFilter = $request->getParam('country');

Concern:

• No validation of filter values before use
• Could potentially be used for injection if not properly escaped in queries

Current Mitigation:

• Values are only used in array comparisons and in-array checks
• Not directly used in SQL queries
• Craft's ORM handles escaping

Recommendation: Add validation for peace of mind:

$statusFilter = $request->getParam('status');
if ($statusFilter && !in_array($statusFilter, ['active', 'expired', 'canceled'])) {
    $statusFilter = null;
}

---

4. JSON Decoding Without Error Handling

Location: Multiple files (CpController.php:60, 285; MemberPurchases.php:134, 154)

Issue:

$purchaseData = json_decode($purchase->purchaseData, true);
if ($purchaseData && isset($purchaseData['payment_details']['order_reference'])) {

Concern:

• If purchaseData contains malformed JSON, json_decode() returns null
• No error logging for malformed data
• Silent failures could mask data corruption

Recommendation: Add error handling:

$purchaseData = json_decode($purchase->purchaseData, true);
if (json_last_error() !== JSON_ERROR_NONE) {
    Craft::error("Failed to decode purchase data: " . json_last_error_msg(), __METHOD__);
    $purchaseData = null;
}

---

5. Verbose Debug Logging

Location: Multiple locations (Plugin.php, MembershipService.php, PurchaseService.php)

Issue: Extensive use of Craft::info() logging throughout the codebase.

Concern:

• Logs may grow large in production
• Some logs contain sensitive data (customer IDs, email addresses)

Current State:

• Logging is appropriate for debugging
• Most logs are at INFO level, not ERROR

Recommendation:

• Review logs before production deployment
• Consider logging at DEBUG level instead of INFO for verbose details
• Ensure log rotation is configured

---

Good Practices Found

✅ Permission Checks

• MembershipController properly checks editUsers permission (line 28)
• Requires POST requests for state-changing operations

✅ Input Validation

• Date validation in MembershipController (lines 74-90)
• Checks for required fields (line 56)
• Validates date logic (expiry after start)

✅ CSRF Protection

• Uses Craft's requirePostRequest() which includes CSRF validation
• All state-changing operations are POST-only

✅ Error Handling

• Extensive try-catch blocks
• Proper error logging
• Returns JSON responses with success/failure status

✅ SQL Injection Prevention

• Uses Craft's ORM/Active Record exclusively
• No raw SQL queries
• Parameterized queries via Craft's Query Builder

✅ Type Safety

• Good use of PHP type hints
• Returns nullable types appropriately
• Validates element types (e.g., instanceof User)

✅ Stripe Webhook Security

• Webhooks are processed through Craft Stripe plugin
• Plugin handles webhook signature verification
• All webhook events are logged to database

---

Recommendations Summary

High Priority

1. Disable or gate debug function with dev mode check

Medium Priority

2. Extract duplicate enrichment code in CpController
3. Add input validation for filter parameters

Low Priority

4. Add JSON decode error handling
5. Review and reduce verbose logging

---

Security Best Practices Already Implemented

• ✅ Uses Craft's permission system
• ✅ CSRF protection via POST requirements
• ✅ SQL injection prevention via ORM
• ✅ No raw file operations
• ✅ No eval() or similar dangerous functions
• ✅ Proper error handling and logging
• ✅ Type-safe parameter handling
• ✅ XSS prevention via Craft's templating

---

Conclusion

The plugin is generally well-written with good security practices. The main concerns are:

1. Code quality: Extract duplicate logic and improve maintainability
2. Defense in depth: Add extra validation layers even though current code is safe
3. Production hardening: Disable debug functions in production

Recent Fixes:

• ✅ CSV export now requires admin access
• ✅ Removed hardcoded debug function linkUserPurchases()

No critical vulnerabilities were found that would prevent production use, but the remaining recommendations above should be addressed for production readiness.