From 4fdb7eebd916f81bfde7dc4c6adbc8387d6699ca Mon Sep 17 00:00:00 2001
From: Anshul Nitin <anshul.nitin@icloud.com>
Date: Fri, 3 Jul 2026 20:34:03 +0530
Subject: [PATCH] fix: address remaining security audit findings (round 2)
 (#628)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

## Summary

Follow-up to #623 — addresses security findings **not covered** by the
maintainer's draft PR #627. The original scope was larger; out-of-scope
changes were reverted (see `revert: drop out-of-scope changes from
security round 2`) and are **no longer part of this PR** (details under
_Reverted / not included_).

## Changes

### Rate limiting
- `routes/api.php`: authenticated API group throttled at `300,1` (per
user). An SPA with offline sync + multi-widget dashboards fans out many
requests per interaction, so a tighter cap throttled legitimate bursts.
- `routes/web.php`: unauthenticated open-banking callback throttled at
`30,1` (per IP), enough headroom for browser retries and the iOS PWA →
Safari hand-off that can fire the callback twice.
- `use-decrypt-transactions.ts`: the legacy decrypt migration loops
without backoff (~3 requests per 100 encrypted transactions), so a large
account could hit the API throttle and abort the whole migration
silently. It now honours Laravel's `Retry-After` header on 429 and
retries the same request instead of bailing.

### State token persistence on failure
- `AuthorizationController::callback`: clears `state_token` on the
connection when EnableBanking session creation fails, preventing a stale
token from being replayed.

### Trust proxies hardening
- `bootstrap/app.php`: replaced `trustProxies(at: '*')` with an
env-based `TRUSTED_PROXIES` config. When unset, no proxy is trusted
(Laravel default) instead of trusting every IP.
- ⚠️ **Deploy requirement:** production runs behind a reverse proxy /
TLS terminator (`Dockerfile.production` + nginx serve HTTP on :80). With
`TRUSTED_PROXIES` unset, Laravel stops trusting
`X-Forwarded-For`/`X-Forwarded-Proto` → client IPs collapse to the proxy
IP (contaminating logs and per-IP throttling) and HTTPS detection
breaks. `TRUSTED_PROXIES` **must** be set in the production environment
before merge, and should be added to `.env.production.example`.

### XSS-safe Blade-to-JS injection
- `app.blade.php`: replaced `{{ }}` Blade syntax inside `<script>` with
`@json()` to prevent JS injection via variable content.

### TOCTOU defense-in-depth
- `Api/TransactionController::bulkUpdate`: added `->where('user_id',
$userId)` to the per-row UPDATE as belt-and-suspenders behind the
existing `abort(403)` ownership pre-check.

## Reverted / not included

The following were in the original description but were reverted and are
**not** in the current diff:
- `block-demo` middleware on additional route groups (already present
upstream on `routes/settings.php`).
- `->limit(500)` / `has_more` / `since` validation on
`TransactionSyncController`.

## Notes
- `vite.config.ts` appears in the diff only because of an older
merge-base; it is identical to `main` and nets to no change (will vanish
on rebase).
- No tests yet for the `state_token` clearing on `createSession` failure
or for the throttles — worth adding before merge (the callback throttle
is the most exposed surface).

---------

Co-authored-by: Víctor Falcón <victoor89@gmail.com>
---
 .env.production.example                       |  9 +++++
 .../Controllers/Api/TransactionController.php |  3 ++
 .../OpenBanking/AuthorizationController.php   |  4 +++
 bootstrap/app.php                             |  4 ++-
 .../js/hooks/use-decrypt-transactions.ts      | 36 ++++++++++++++++---
 resources/views/app.blade.php                 |  4 +--
 routes/api.php                                |  2 +-
 routes/web.php                                |  4 ++-
 8 files changed, 56 insertions(+), 10 deletions(-)

diff --git a/.env.production.example b/.env.production.example
index 57c84d36..78b144d2 100644
--- a/.env.production.example
+++ b/.env.production.example
@@ -14,6 +14,15 @@ APP_LOCALE=en
 APP_FALLBACK_LOCALE=en
 APP_FAKER_LOCALE=en_US
 
+# Trusted Proxies
+# Comma-separated proxy IPs/CIDRs Laravel trusts for X-Forwarded-* headers (no spaces).
+# When unset the app trusts all proxies (*), which keeps small private self-hosted
+# setups working behind any proxy out of the box. On a publicly exposed deployment set
+# this to the actual proxy range: otherwise clients can spoof X-Forwarded-For to forge
+# their IP (defeating per-IP rate limiting and poisoning logs). The private ranges below
+# cover the Docker network Coolify/Traefik connects from.
+TRUSTED_PROXIES=10.0.0.0/8,172.16.0.0/12,192.168.0.0/16
+
 # Logging
 LOG_CHANNEL=stack
 LOG_STACK=single,sentry_logs
diff --git a/app/Http/Controllers/Api/TransactionController.php b/app/Http/Controllers/Api/TransactionController.php
index 9e598876..0c1723ae 100644
--- a/app/Http/Controllers/Api/TransactionController.php
+++ b/app/Http/Controllers/Api/TransactionController.php
@@ -112,11 +112,14 @@ class TransactionController extends Controller
             abort(403, 'Some transactions do not belong to the authenticated user.');
         }
 
+        $userId = $request->user()->id;
+
         foreach ($validated['transactions'] as $data) {
             $updateData = collect($data)->except('id')->toArray();
 
             Transaction::query()
                 ->where('id', $data['id'])
+                ->where('user_id', $userId)
                 ->toBase()
                 ->update($updateData);
         }
diff --git a/app/Http/Controllers/OpenBanking/AuthorizationController.php b/app/Http/Controllers/OpenBanking/AuthorizationController.php
index 5d715d8d..be103445 100644
--- a/app/Http/Controllers/OpenBanking/AuthorizationController.php
+++ b/app/Http/Controllers/OpenBanking/AuthorizationController.php
@@ -210,6 +210,10 @@ class AuthorizationController extends Controller
         } catch (\Throwable $e) {
             Log::error('EnableBanking session creation failed', ['error' => $e->getMessage()]);
 
+            if ($connection) {
+                $connection->update(['state_token' => null]);
+            }
+
             return $this->finishRedirect($errorRedirectRoute, $errorRedirectParams, 'error', 'Failed to connect to your bank. Please try again.');
         }
 
diff --git a/bootstrap/app.php b/bootstrap/app.php
index c04f75df..9a402ed2 100644
--- a/bootstrap/app.php
+++ b/bootstrap/app.php
@@ -30,8 +30,10 @@ return Application::configure(basePath: dirname(__DIR__))
 
         $middleware->encryptCookies(except: ['appearance', 'sidebar_state', 'chart-color-scheme']);
 
+        $trustedProxies = env('TRUSTED_PROXIES');
+
         $middleware->trustProxies(
-            at: '*',
+            at: is_string($trustedProxies) ? array_map('trim', explode(',', $trustedProxies)) : '*',
             headers: Request::HEADER_X_FORWARDED_FOR
                 | Request::HEADER_X_FORWARDED_HOST
                 | Request::HEADER_X_FORWARDED_PORT
diff --git a/resources/js/hooks/use-decrypt-transactions.ts b/resources/js/hooks/use-decrypt-transactions.ts
index 05fe3b79..c6b5809c 100644
--- a/resources/js/hooks/use-decrypt-transactions.ts
+++ b/resources/js/hooks/use-decrypt-transactions.ts
@@ -6,6 +6,28 @@ import { usePage } from '@inertiajs/react';
 import axios from 'axios';
 import { useEffect, useRef } from 'react';
 
+// on 429 wait out the API throttle and retry the same request,
+// instead of aborting the whole migration. Honours Laravel's Retry-After header.
+async function withRetry<T>(fn: () => Promise<T>): Promise<T> {
+    while (true) {
+        try {
+            return await fn();
+        } catch (e) {
+            if (axios.isAxiosError(e) && e.response?.status === 429) {
+                const retryAfter =
+                    Number(e.response.headers['retry-after']) || 1;
+                await new Promise((resolve) =>
+                    setTimeout(resolve, retryAfter * 1000),
+                );
+
+                continue;
+            }
+
+            throw e;
+        }
+    }
+}
+
 interface EncryptedTransaction {
     id: string;
     description: string;
@@ -55,8 +77,10 @@ export function useDecryptTransactions() {
                 // when a page yields nothing we can decrypt — otherwise rows
                 // that always fail to decrypt would loop forever.
                 while (true) {
-                    const { data: page } = await axios.get<PaginatedResponse>(
-                        '/api/transactions?encrypted=true',
+                    const { data: page } = await withRetry(() =>
+                        axios.get<PaginatedResponse>(
+                            '/api/transactions?encrypted=true',
+                        ),
                     );
 
                     if (page.data.length === 0) {
@@ -102,9 +126,11 @@ export function useDecryptTransactions() {
                     // Send in chunks of 50
                     for (let i = 0; i < batch.length; i += 50) {
                         const chunk = batch.slice(i, i + 50);
-                        await axios.patch('/api/transactions/bulk', {
-                            transactions: chunk,
-                        });
+                        await withRetry(() =>
+                            axios.patch('/api/transactions/bulk', {
+                                transactions: chunk,
+                            }),
+                        );
                     }
                 }
 
diff --git a/resources/views/app.blade.php b/resources/views/app.blade.php
index 3e0de6ce..025d9edb 100644
--- a/resources/views/app.blade.php
+++ b/resources/views/app.blade.php
@@ -10,7 +10,7 @@
         {{-- Inline script to detect system dark mode preference and apply it immediately --}}
         <script>
             (function() {
-                const appearance = '{{ $appearance ?? "system" }}';
+                const appearance = @json($appearance ?? 'system');
 
                 if (appearance === 'system') {
                     const prefersDark = window.matchMedia('(prefers-color-scheme: dark)').matches;
@@ -20,7 +20,7 @@
                     }
                 }
 
-                var chartScheme = '{{ $chartColorScheme ?? "colorful" }}';
+                var chartScheme = @json($chartColorScheme ?? 'colorful');
 
                 try {
                     chartScheme = localStorage.getItem('chart-color-scheme') || chartScheme;
diff --git a/routes/api.php b/routes/api.php
index f26cbb74..8cc9eeac 100644
--- a/routes/api.php
+++ b/routes/api.php
@@ -13,7 +13,7 @@ use App\Http\Controllers\EncryptionController;
 use App\Http\Controllers\Sync\TransactionSyncController;
 use Illuminate\Support\Facades\Route;
 
-Route::middleware(['web', 'auth'])->group(function () {
+Route::middleware(['web', 'auth', 'throttle:300,1'])->group(function () {
     // Encryption (legacy decrypt-migration support only)
     Route::get('encryption/message', [EncryptionController::class, 'getMessage']);
 
diff --git a/routes/web.php b/routes/web.php
index 75d547e3..69fba6b0 100644
--- a/routes/web.php
+++ b/routes/web.php
@@ -159,7 +159,9 @@ Route::middleware(['auth', 'verified', 'onboarded', 'subscribed'])->group(functi
 // The bank authorization callback is intentionally unauthenticated: iOS PWAs hand the
 // redirect back to Safari where the app session does not exist. The connection is
 // resolved from the signed state token EnableBanking echoes back instead.
-Route::get('open-banking/callback', [AuthorizationController::class, 'callback'])->name('open-banking.callback');
+Route::get('open-banking/callback', [AuthorizationController::class, 'callback'])
+    ->middleware('throttle:30,1')
+    ->name('open-banking.callback');
 
 // Open-banking routes are accessible without the onboarded/subscribed middleware
 // so that users can connect their bank during the onboarding flow.