fix: address remaining security audit findings (round 2) (#628)
## 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>
This commit is contained in:
parent
021cb66643
commit
4fdb7eebd9
|
|
@ -14,6 +14,15 @@ APP_LOCALE=en
|
||||||
APP_FALLBACK_LOCALE=en
|
APP_FALLBACK_LOCALE=en
|
||||||
APP_FAKER_LOCALE=en_US
|
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
|
# Logging
|
||||||
LOG_CHANNEL=stack
|
LOG_CHANNEL=stack
|
||||||
LOG_STACK=single,sentry_logs
|
LOG_STACK=single,sentry_logs
|
||||||
|
|
|
||||||
|
|
@ -112,11 +112,14 @@ class TransactionController extends Controller
|
||||||
abort(403, 'Some transactions do not belong to the authenticated user.');
|
abort(403, 'Some transactions do not belong to the authenticated user.');
|
||||||
}
|
}
|
||||||
|
|
||||||
|
$userId = $request->user()->id;
|
||||||
|
|
||||||
foreach ($validated['transactions'] as $data) {
|
foreach ($validated['transactions'] as $data) {
|
||||||
$updateData = collect($data)->except('id')->toArray();
|
$updateData = collect($data)->except('id')->toArray();
|
||||||
|
|
||||||
Transaction::query()
|
Transaction::query()
|
||||||
->where('id', $data['id'])
|
->where('id', $data['id'])
|
||||||
|
->where('user_id', $userId)
|
||||||
->toBase()
|
->toBase()
|
||||||
->update($updateData);
|
->update($updateData);
|
||||||
}
|
}
|
||||||
|
|
|
||||||
|
|
@ -210,6 +210,10 @@ class AuthorizationController extends Controller
|
||||||
} catch (\Throwable $e) {
|
} catch (\Throwable $e) {
|
||||||
Log::error('EnableBanking session creation failed', ['error' => $e->getMessage()]);
|
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.');
|
return $this->finishRedirect($errorRedirectRoute, $errorRedirectParams, 'error', 'Failed to connect to your bank. Please try again.');
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -30,8 +30,10 @@ return Application::configure(basePath: dirname(__DIR__))
|
||||||
|
|
||||||
$middleware->encryptCookies(except: ['appearance', 'sidebar_state', 'chart-color-scheme']);
|
$middleware->encryptCookies(except: ['appearance', 'sidebar_state', 'chart-color-scheme']);
|
||||||
|
|
||||||
|
$trustedProxies = env('TRUSTED_PROXIES');
|
||||||
|
|
||||||
$middleware->trustProxies(
|
$middleware->trustProxies(
|
||||||
at: '*',
|
at: is_string($trustedProxies) ? array_map('trim', explode(',', $trustedProxies)) : '*',
|
||||||
headers: Request::HEADER_X_FORWARDED_FOR
|
headers: Request::HEADER_X_FORWARDED_FOR
|
||||||
| Request::HEADER_X_FORWARDED_HOST
|
| Request::HEADER_X_FORWARDED_HOST
|
||||||
| Request::HEADER_X_FORWARDED_PORT
|
| Request::HEADER_X_FORWARDED_PORT
|
||||||
|
|
|
||||||
|
|
@ -6,6 +6,28 @@ import { usePage } from '@inertiajs/react';
|
||||||
import axios from 'axios';
|
import axios from 'axios';
|
||||||
import { useEffect, useRef } from 'react';
|
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 {
|
interface EncryptedTransaction {
|
||||||
id: string;
|
id: string;
|
||||||
description: string;
|
description: string;
|
||||||
|
|
@ -55,8 +77,10 @@ export function useDecryptTransactions() {
|
||||||
// when a page yields nothing we can decrypt — otherwise rows
|
// when a page yields nothing we can decrypt — otherwise rows
|
||||||
// that always fail to decrypt would loop forever.
|
// that always fail to decrypt would loop forever.
|
||||||
while (true) {
|
while (true) {
|
||||||
const { data: page } = await axios.get<PaginatedResponse>(
|
const { data: page } = await withRetry(() =>
|
||||||
'/api/transactions?encrypted=true',
|
axios.get<PaginatedResponse>(
|
||||||
|
'/api/transactions?encrypted=true',
|
||||||
|
),
|
||||||
);
|
);
|
||||||
|
|
||||||
if (page.data.length === 0) {
|
if (page.data.length === 0) {
|
||||||
|
|
@ -102,9 +126,11 @@ export function useDecryptTransactions() {
|
||||||
// Send in chunks of 50
|
// Send in chunks of 50
|
||||||
for (let i = 0; i < batch.length; i += 50) {
|
for (let i = 0; i < batch.length; i += 50) {
|
||||||
const chunk = batch.slice(i, i + 50);
|
const chunk = batch.slice(i, i + 50);
|
||||||
await axios.patch('/api/transactions/bulk', {
|
await withRetry(() =>
|
||||||
transactions: chunk,
|
axios.patch('/api/transactions/bulk', {
|
||||||
});
|
transactions: chunk,
|
||||||
|
}),
|
||||||
|
);
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -10,7 +10,7 @@
|
||||||
{{-- Inline script to detect system dark mode preference and apply it immediately --}}
|
{{-- Inline script to detect system dark mode preference and apply it immediately --}}
|
||||||
<script>
|
<script>
|
||||||
(function() {
|
(function() {
|
||||||
const appearance = '{{ $appearance ?? "system" }}';
|
const appearance = @json($appearance ?? 'system');
|
||||||
|
|
||||||
if (appearance === 'system') {
|
if (appearance === 'system') {
|
||||||
const prefersDark = window.matchMedia('(prefers-color-scheme: dark)').matches;
|
const prefersDark = window.matchMedia('(prefers-color-scheme: dark)').matches;
|
||||||
|
|
@ -20,7 +20,7 @@
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
var chartScheme = '{{ $chartColorScheme ?? "colorful" }}';
|
var chartScheme = @json($chartColorScheme ?? 'colorful');
|
||||||
|
|
||||||
try {
|
try {
|
||||||
chartScheme = localStorage.getItem('chart-color-scheme') || chartScheme;
|
chartScheme = localStorage.getItem('chart-color-scheme') || chartScheme;
|
||||||
|
|
|
||||||
|
|
@ -13,7 +13,7 @@ use App\Http\Controllers\EncryptionController;
|
||||||
use App\Http\Controllers\Sync\TransactionSyncController;
|
use App\Http\Controllers\Sync\TransactionSyncController;
|
||||||
use Illuminate\Support\Facades\Route;
|
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)
|
// Encryption (legacy decrypt-migration support only)
|
||||||
Route::get('encryption/message', [EncryptionController::class, 'getMessage']);
|
Route::get('encryption/message', [EncryptionController::class, 'getMessage']);
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -159,7 +159,9 @@ Route::middleware(['auth', 'verified', 'onboarded', 'subscribed'])->group(functi
|
||||||
// The bank authorization callback is intentionally unauthenticated: iOS PWAs hand the
|
// 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
|
// redirect back to Safari where the app session does not exist. The connection is
|
||||||
// resolved from the signed state token EnableBanking echoes back instead.
|
// 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
|
// Open-banking routes are accessible without the onboarded/subscribed middleware
|
||||||
// so that users can connect their bank during the onboarding flow.
|
// so that users can connect their bank during the onboarding flow.
|
||||||
|
|
|
||||||
Loading…
Reference in New Issue