From 35569923a527f0fbbc094ea701d88da77b9ad3fa Mon Sep 17 00:00:00 2001 From: dwindown Date: Fri, 17 Apr 2026 17:00:47 +0700 Subject: [PATCH] docs: add comprehensive audit report and architectural recommendation Checkpoint before implementation. Includes audit findings (FINDINGS.md), architectural recommendation (RECOMMENDATION.md), and existing code changes to Form, Order, Render, and form-action.js from recent development. Co-Authored-By: Claude Opus 4.7 --- FINDINGS.md | 196 +++++++++++++ RECOMMENDATION.md | 469 ++++++++++++++++++++++++++++++++ includes/Form.php | 45 ++- includes/Order.php | 124 +++------ includes/Render.php | 261 ++++++++++++++---- public/assets/js/form-action.js | 72 +++-- 6 files changed, 1014 insertions(+), 153 deletions(-) create mode 100644 FINDINGS.md create mode 100644 RECOMMENDATION.md diff --git a/FINDINGS.md b/FINDINGS.md new file mode 100644 index 000000000..dbd102366 --- /dev/null +++ b/FINDINGS.md @@ -0,0 +1,196 @@ +# πŸ” Formipay Plugin β€” Comprehensive Audit Report + +**Date:** April 17, 2026 +**Auditor:** GitHub Copilot +**Plugin Version:** 1.0.0 +**Files Analyzed:** ~60+ files, ~15,000+ lines of PHP, JS, CSS, HTML + +--- + +## Table of Contents + +- [1. Bugs & Defects](#1-bugs--defects) + - [1.1 Critical Bugs](#11-critical-bugs) + - [1.2 Moderate Bugs](#12-moderate-bugs) +- [2. Security Concerns](#2-security-concerns) +- [3. Architecture & Code Quality Issues](#3-architecture--code-quality-issues) +- [4. Missing Features & Modules](#4-missing-features--modules) +- [5. Performance Issues](#5-performance-issues) +- [6. Missing Admin Pages / Settings](#6-missing-admin-pages--settings) +- [7. Code Cleanup Needed](#7-code-cleanup-needed) +- [8. Opportunities & Nice-to-Haves](#8-opportunities--nice-to-haves) +- [9. Summary Priority Matrix](#9-summary-priority-matrix) +- [10. Architectural Recommendation](#10-architectural-recommendation) + +--- + +## 1. Bugs & Defects + +### 1.1 Critical Bugs + +| # | Location | Issue | Detail | +|---|----------|-------|--------| +| 1 | `includes/Customer.php` ~line 172 `update()` | **Undefined variable β€” fatal error** | Method builds `$insert_data` and `$where`, but the `$wpdb->update()` call references undefined `$table_name` and `$new_args`. Every customer update will throw a PHP fatal error. | +| 2 | `includes/Order.php` `delete()` | **Undefined `$id` variable** | Uses `$id` in the `$wpdb->delete()` where clause instead of the method parameter `$order_id`. Every order deletion call will fail. | +| 3 | `includes/Order.php` `formipay_bulk_delete_order()` | **Iterates wrong variable** | Loops `foreach($ids as $id)` but calls `$this->delete($order_id)` β€” `$order_id` comes from the outer scope (nonce check), not the loop variable. Bulk delete will repeatedly delete the same (or zero) orders. | +| 4 | `includes/Notification/Email.php` `send_email()` | **Wrong class reference β€” fatal error** | Calls `\Formipay_Notification::update_notification_data()` β€” this class does not exist. Should use `parent::update_notification_data()`. Email status tracking will crash. | +| 5 | `includes/Integration/Paypal.php` `auto_cancel_order_on_timeout()` | **Undefined `Order` class** | Calls `Order::update(...)` but unlike `BankTransfer.php`, `Paypal.php` does not import `use Formipay\Order as Order`. This will throw a class-not-found error on timeout. | +| 6 | `includes/Integration/Paypal.php` `process_payment()` | **Undefined `self::paypal_settings`** | The PayPal class never declares a `$paypal_settings` property. Accessing it leads to undefined property notices and broken payment flow. | + +### 1.2 Moderate Bugs + +| # | Location | Issue | +|---|----------|-------| +| 7 | `includes/Payment/BankTransfer.php` `check_unique_code()` | Uses `MAX(id)+1` for unique codes β€” predictable and subject to race conditions. Two concurrent orders can receive the same unique code. | +| 8 | `includes/Payment/BankTransfer.php` `add_unique_code_details()` | Calls `$this->check_unique_code()` **three times** per request (once for `'item'`, once for `'amount'`, once for `'subtotal'`). Each call queries the DB independently and may return different values. Displayed unique code may not match the stored one. | +| 9 | `admin/functions.php` `formipay_field_type_collection()` | Color field label says `'Number'` instead of `'Color'` β€” copy-paste error. | +| 10 | `includes/Render.php` field rendering | No `default` fallback rendered when field type doesn't match any case in the switch β€” unknown field types silently produce no output. | +| 11 | `includes/Order.php` `render_form_submit()` | `$field_value` is sometimes an array (from checkbox fields) but the code assumes string context. Nested `if(is_array($field_value))` only handles one level. | +| 12 | `includes/Thankyou.php` `check_parse_query()` vs `formipay_get_order()` | Old cookie-based URL (`base64_encode`) co-exists with new Token-based validation. `formipay_get_order()` generates the old-format URL for the `thankyou` link, but the new Token system expects a different format. Inconsistent access path. | + +--- + +## 2. Security Concerns + +| # | Severity | Location | Issue | +|---|----------|----------|-------| +| 1 | **High** | `includes/Order.php` `retrieve_form_data()` | Cookie `fp_access` uses `maybe_serialize()` β€” **PHP object injection** risk if an attacker can manipulate cookie values. Should use `json_encode()`/`json_decode()`. | +| 2 | **High** | `includes/Thankyou.php` `set_endpoint()` | Calls `flush_rewrite_rules()` on **every `init`** hook β€” extremely expensive and causes race conditions under concurrent load. Should only flush on activation/deactivation or settings save. | +| 3 | **High** | `includes/Payment/Payment.php` `set_endpoint()` | Same `flush_rewrite_rules()` issue β€” fires on every page load. | +| 4 | **Medium** | `includes/Order.php` `retrieve_form_data()` | Thank-you URL uses `base64_encode(form_id:::order_id:::session_id)` β€” base64 is not encryption. Sequential order IDs can be guessed. The `Token` class provides proper tokens but the old path remains active. | +| 5 | **Medium** | `includes/LicenseAPI.php` | All REST endpoints use `permission_callback => '__return_true'`. The `revoke` endpoint has a stub permission callback that always returns `true`. Anyone can revoke or manipulate licenses without authentication. | +| 6 | **Medium** | `includes/Integration/Paypal.php` `webhook_endpoint()` | No PayPal webhook signature verification. An attacker could forge webhook calls to mark orders as paid without actual payment. | +| 7 | **Medium** | `includes/Customer.php` `formipay_tabledata_customers()` | No nonce check (`check_ajax_referer`). Any authenticated user can dump all customer data via direct AJAX call. | +| 8 | **Low** | `includes/Render.php` | Inline `