Snapshot of main at 9f39a6e1e (2026-11-02)

SECURITY_FIXES.md

@@ -0,0 +1,165 @@
+# Payment Security Fixes
+
+## Overview
+Security enhancements have been added to the payment flow in `app/Http/Livewire/Pay.php` to prevent tampering with front-end properties via browser developer tools or by modifying Livewire component state.
+
+## Fixes Applied
+
+### 1. Balance Limit Check Now Blocks Execution
+**Location:** `Pay.php`, `checkBalanceLimits()` method
+
+**What was wrong:**
+- The method set `isPaymentLimitErrorModalOpen = true` but returned `true`
+- `doTransfer()` didn't check the return value, so execution continued
+- A user could tamper with the check by modifying `isPaymentLimitErrorModalOpen` in the browser console
+
+**What was fixed:**
+- `checkBalanceLimits()` now returns a boolean: `true` if limits OK, `false` if exceeded
+- `doTransfer()` now checks the return value: `if (!$limitsOk) { return; }`
+- If limits are exceeded, the transaction is NOT saved and a security warning is logged
+
+**Test Scenario:**
+```javascript
+// Before fix: User could bypass by setting in browser console:
+Livewire.find('pay-component').isPaymentLimitErrorModalOpen = false;
+// And payment would still be saved!
+
+// After fix: Even if user tampersthis property, the backend check prevents saving
+```
+
+### 2. Amount Validation
+**Location:** `Pay.php`, `doTransfer()` method
+
+**What was added:**
+```php
+// Security: Validate amount is a valid positive number
+if (!is_numeric($amount) || $amount <= 0) {
+    $warningMessage = 'Security: Invalid payment amount (must be positive number)';
+    return $this->logAndReport(...);
+}
+```
+
+**Prevents:**
+- Negative amounts (e.g., `amount = -50`)
+- Zero amount (e.g., `amount = 0`)
+- Non-numeric values (e.g., `amount = 'abc'`)
+
+### 3. FromAccountId Validation
+**Location:** `Pay.php`, `doTransfer()` method (added first)
+
+**What was added:**
+```php
+// Security: Verify fromAccountId is an integer (prevent tampering)
+if (!is_int($fromAccountId) || $fromAccountId <= 0) {
+    $warningMessage = 'Security: Invalid From account ID';
+    return $this->logAndReport(...);
+}
+```
+
+**Prevents:**
+- Non-integer account IDs
+- Negative or zero account IDs
+- String values passed as account ID
+
+**Note:** This validation happens BEFORE the ownership check, catching obvious tampering early.
+
+### 4. ToAccountId Validation
+**Location:** `Pay.php`, `doTransfer()` method
+
+**What was added:**
+```php
+// Security: Verify toAccountId is an integer (prevent tampering)
+if (!is_int($toAccountId) || $toAccountId <= 0) {
+    $warningMessage = 'Security: Invalid To account ID';
+    return $this->logAndReport(...);
+}
+```
+
+**Prevents:**
+- Same issues as FromAccountId validation
+
+### 5. Existing Ownership Check (Still in Place)
+**Location:** `Pay.php`, `doTransfer()` method
+
+This was already in place and remains:
+```php
+$accountsInfo = collect($transactionController->getAccountsInfo());
+if (!$accountsInfo->contains('id', $fromAccountId)) {
+    $warningMessage = 'Unauthorized payment attempt: illegal access of From account';
+    return $this->logAndReport(...);
+}
+```
+
+**Why this is important:**
+- Even though the browser interface only shows accounts the user owns, a user could tamper with Livewire to send a `fromAccountId` for an account they don't own
+- This backend check verifies the FromAccountId belongs to the active session profile
+- The check happens AFTER type validation but BEFORE the transaction is saved
+
+## Security Log Messages
+
+All security violations are logged with a `[TRANSFER]` prefix. To check for security violations:
+
+```bash
+grep "\[TRANSFER\].*Security" storage/logs/laravel-*.log
+```
+
+Example log entries:
+```
+[2026-06-06 14:30:45] local.WARNING: [TRANSFER] Security: Invalid payment amount (must be positive number)
+[2026-06-06 14:31:12] local.WARNING: [TRANSFER] Security: Invalid From account ID
+[2026-06-06 14:32:33] local.WARNING: [TRANSFER] Security: Invalid To account ID
+```
+
+## Validation Order
+
+The checks happen in this order in `doTransfer()`:
+
+1. **Permission check** - Does user have right to make payments?
+2. **Amount validation** - Is amount numeric and positive?
+3. **FromAccountId type check** - Is it a positive integer?
+4. **ToAccountId type check** - Is it a positive integer?
+5. **Account existence check** - Do both accounts exist in DB?
+6. **Account ownership check** - Does FromAccountId belong to active profile?
+7. **Accountable check** - Are both account holders not removed?
+8. **Same account check** - Are From and To different?
+9. **Transaction type check** - Is type allowed?
+10. **Balance limits check** - Does payment exceed limits?
+
+If ANY check fails, execution stops and the transaction is NOT saved.
+
+## What's NOT Fixed (By Design)
+
+These are intentionally NOT checked in the backend because they're enforced by the database schema:
+
+- Transaction immutability - Transactions cannot be modified/deleted (DB user permissions)
+- Account balance accuracy - Balance is calculated from immutable transactions (MySQL window functions)
+- Transaction type validity - Enforced by foreign key constraint
+
+## Testing
+
+**Manual Testing Instructions:**
+
+1. Make a normal payment to verify it works
+2. Open browser console and try:
+   ```javascript
+   // Try to set negative amount
+   Livewire.find('pay-component-id').set('amount', -50);
+   // Click Pay button - should be blocked with security log entry
+   ```
+
+3. Try to inject invalid account IDs:
+   ```javascript
+   Livewire.find('pay-component-id').set('toAccountId', -1);
+   // Should be blocked with security log entry
+   ```
+
+4. Check logs for security violations:
+   ```bash
+   tail -f storage/logs/laravel-$(date +%Y-%m-%d).log | grep "\[TRANSFER\]"
+   ```
+
+## Related Code
+
+- `PaymentSecurityTest.php` - Automated tests for security validations (if needed)
+- `app/Http/Controllers/TransactionController.php` - API endpoint that also validates payments
+- `app/Observers/TransactionObserver.php` - Clears caches on successful transaction creation