From 80117c3edeaf5c5a5166f3815fc555a15b5ce686 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?V=C3=ADctor=20Falc=C3=B3n?= Date: Sat, 17 Jan 2026 09:58:52 +0100 Subject: [PATCH] fix: Check IDOR vulnerabilities (#60) --- .../BulkUpdateTransactionsRequest.php | 38 +- .../Requests/StoreAccountBalanceRequest.php | 8 +- app/Http/Requests/StoreTransactionRequest.php | 27 +- .../Requests/UpdateTransactionRequest.php | 8 +- phpunit.xml | 4 + .../AccountBalanceSyncControllerTest.php | 12 +- tests/Feature/IdorVulnerabilityTest.php | 752 ++++++++++++++++++ tests/Feature/Sync/TransactionSyncTest.php | 13 +- 8 files changed, 844 insertions(+), 18 deletions(-) create mode 100644 tests/Feature/IdorVulnerabilityTest.php diff --git a/app/Http/Requests/BulkUpdateTransactionsRequest.php b/app/Http/Requests/BulkUpdateTransactionsRequest.php index affcf9b5..d5de9a1c 100644 --- a/app/Http/Requests/BulkUpdateTransactionsRequest.php +++ b/app/Http/Requests/BulkUpdateTransactionsRequest.php @@ -3,6 +3,7 @@ namespace App\Http\Requests; use Illuminate\Foundation\Http\FormRequest; +use Illuminate\Validation\Rule; class BulkUpdateTransactionsRequest extends FormRequest { @@ -22,17 +23,41 @@ class BulkUpdateTransactionsRequest extends FormRequest 'filters.amount_min' => ['nullable', 'numeric'], 'filters.amount_max' => ['nullable', 'numeric'], 'filters.category_ids' => ['nullable', 'array'], - 'filters.category_ids.*' => ['string', 'uuid', 'exists:categories,id'], + 'filters.category_ids.*' => [ + 'string', + 'uuid', + Rule::exists('categories', 'id')->where(function ($query) { + $query->where('user_id', $this->user()->id); + }), + ], 'filters.account_ids' => ['nullable', 'array'], 'filters.account_ids.*' => ['string', 'uuid'], 'filters.label_ids' => ['nullable', 'array'], - 'filters.label_ids.*' => ['string', 'uuid', 'exists:labels,id'], + 'filters.label_ids.*' => [ + 'string', + 'uuid', + Rule::exists('labels', 'id')->where(function ($query) { + $query->where('user_id', $this->user()->id); + }), + ], 'filters.search_text' => ['nullable', 'string'], - 'category_id' => ['nullable', 'exists:categories,id'], + 'category_id' => [ + 'nullable', + Rule::exists('categories', 'id')->where(function ($query) { + $query->where('user_id', $this->user()->id); + }), + ], 'notes' => ['nullable', 'string'], 'notes_iv' => ['nullable', 'string', 'size:16'], 'label_ids' => ['nullable', 'array'], - 'label_ids.*' => ['required', 'string', 'uuid', 'exists:labels,id'], + 'label_ids.*' => [ + 'required', + 'string', + 'uuid', + Rule::exists('labels', 'id')->where(function ($query) { + $query->where('user_id', $this->user()->id); + }), + ], ]; } @@ -40,7 +65,10 @@ class BulkUpdateTransactionsRequest extends FormRequest { return [ 'transaction_ids.*.uuid' => 'Invalid transaction ID format.', - 'category_id.exists' => 'The selected category does not exist.', + 'category_id.exists' => 'The selected category does not exist or does not belong to you.', + 'filters.category_ids.*.exists' => 'One or more filter categories do not exist or do not belong to you.', + 'filters.label_ids.*.exists' => 'One or more filter labels do not exist or do not belong to you.', + 'label_ids.*.exists' => 'One or more selected labels do not exist or do not belong to you.', 'notes_iv.size' => 'The notes IV must be exactly 16 characters.', ]; } diff --git a/app/Http/Requests/StoreAccountBalanceRequest.php b/app/Http/Requests/StoreAccountBalanceRequest.php index 736a3160..a4ffe046 100644 --- a/app/Http/Requests/StoreAccountBalanceRequest.php +++ b/app/Http/Requests/StoreAccountBalanceRequest.php @@ -3,6 +3,7 @@ namespace App\Http\Requests; use Illuminate\Foundation\Http\FormRequest; +use Illuminate\Validation\Rule; class StoreAccountBalanceRequest extends FormRequest { @@ -15,7 +16,12 @@ class StoreAccountBalanceRequest extends FormRequest { return [ 'id' => ['sometimes', 'uuid'], - 'account_id' => ['required', 'exists:accounts,id'], + 'account_id' => [ + 'required', + Rule::exists('accounts', 'id')->where(function ($query) { + $query->where('user_id', $this->user()->id); + }), + ], 'balance_date' => ['required', 'date'], 'balance' => ['required', 'integer'], ]; diff --git a/app/Http/Requests/StoreTransactionRequest.php b/app/Http/Requests/StoreTransactionRequest.php index 764aae12..d5d2cd65 100644 --- a/app/Http/Requests/StoreTransactionRequest.php +++ b/app/Http/Requests/StoreTransactionRequest.php @@ -17,8 +17,18 @@ class StoreTransactionRequest extends FormRequest { return [ 'id' => ['sometimes', 'uuid'], - 'account_id' => ['required', 'exists:accounts,id'], - 'category_id' => ['nullable', 'exists:categories,id'], + 'account_id' => [ + 'required', + Rule::exists('accounts', 'id')->where(function ($query) { + $query->where('user_id', $this->user()->id); + }), + ], + 'category_id' => [ + 'nullable', + Rule::exists('categories', 'id')->where(function ($query) { + $query->where('user_id', $this->user()->id); + }), + ], 'description' => ['required', 'string'], 'description_iv' => ['required', 'string', 'size:16'], 'transaction_date' => ['required', 'date'], @@ -28,7 +38,13 @@ class StoreTransactionRequest extends FormRequest 'notes_iv' => ['nullable', 'string', 'size:16'], 'source' => ['required', Rule::enum(TransactionSource::class)], 'label_ids' => ['nullable', 'array'], - 'label_ids.*' => ['string', 'uuid', 'exists:labels,id'], + 'label_ids.*' => [ + 'string', + 'uuid', + Rule::exists('labels', 'id')->where(function ($query) { + $query->where('user_id', $this->user()->id); + }), + ], ]; } @@ -36,8 +52,9 @@ class StoreTransactionRequest extends FormRequest { return [ 'account_id.required' => 'The account is required.', - 'account_id.exists' => 'The selected account does not exist.', - 'category_id.exists' => 'The selected category does not exist.', + 'account_id.exists' => 'The selected account does not exist or does not belong to you.', + 'category_id.exists' => 'The selected category does not exist or does not belong to you.', + 'label_ids.*.exists' => 'One or more selected labels do not exist or do not belong to you.', 'description.required' => 'The description is required.', 'description_iv.required' => 'The description IV is required.', 'description_iv.size' => 'The description IV must be exactly 16 characters.', diff --git a/app/Http/Requests/UpdateTransactionRequest.php b/app/Http/Requests/UpdateTransactionRequest.php index bed5f73f..00880d43 100644 --- a/app/Http/Requests/UpdateTransactionRequest.php +++ b/app/Http/Requests/UpdateTransactionRequest.php @@ -3,6 +3,7 @@ namespace App\Http\Requests; use Illuminate\Foundation\Http\FormRequest; +use Illuminate\Validation\Rule; class UpdateTransactionRequest extends FormRequest { @@ -14,7 +15,12 @@ class UpdateTransactionRequest extends FormRequest public function rules(): array { return [ - 'category_id' => ['nullable', 'exists:categories,id'], + 'category_id' => [ + 'nullable', + Rule::exists('categories', 'id')->where(function ($query) { + $query->where('user_id', $this->user()->id); + }), + ], 'description' => ['sometimes', 'string'], 'description_iv' => ['sometimes', 'string', 'size:16'], 'notes' => ['nullable', 'string'], diff --git a/phpunit.xml b/phpunit.xml index 59c1e978..db43410e 100644 --- a/phpunit.xml +++ b/phpunit.xml @@ -26,8 +26,12 @@ + + + + diff --git a/tests/Feature/AccountBalanceSyncControllerTest.php b/tests/Feature/AccountBalanceSyncControllerTest.php index 50e2a50a..f36bce3d 100644 --- a/tests/Feature/AccountBalanceSyncControllerTest.php +++ b/tests/Feature/AccountBalanceSyncControllerTest.php @@ -150,12 +150,13 @@ it('can update an account balance', function () { it('cannot update another user account balance', function () { $user = User::factory()->create(); + $userAccount = Account::factory()->for($user)->create(); $otherUser = User::factory()->create(); - $account = Account::factory()->for($otherUser)->create(); - $balance = AccountBalance::factory()->for($account)->create(); + $otherAccount = Account::factory()->for($otherUser)->create(); + $balance = AccountBalance::factory()->for($otherAccount)->create(); $updateData = [ - 'account_id' => $account->id, + 'account_id' => $userAccount->id, 'balance_date' => $balance->balance_date->toDateString(), 'balance' => 999999, ]; @@ -163,6 +164,11 @@ it('cannot update another user account balance', function () { $response = $this->actingAs($user)->patchJson("/api/sync/account-balances/{$balance->id}", $updateData); $response->assertForbidden(); + + $this->assertDatabaseHas('account_balances', [ + 'id' => $balance->id, + 'account_id' => $otherAccount->id, + ]); }); it('updates existing balance when creating with duplicate account_id and balance_date', function () { diff --git a/tests/Feature/IdorVulnerabilityTest.php b/tests/Feature/IdorVulnerabilityTest.php new file mode 100644 index 00000000..d0770947 --- /dev/null +++ b/tests/Feature/IdorVulnerabilityTest.php @@ -0,0 +1,752 @@ +create(); + $victim = User::factory()->create(); + $victimAccount = Account::factory()->for($victim)->create(); + + actingAs($attacker); + + // Attacker tries to update victim's account balance + $response = $this->putJson("/api/accounts/{$victimAccount->id}/balance/current", [ + 'balance' => 9999999, + ]); + + $response->assertForbidden(); + + $this->assertDatabaseMissing('account_balances', [ + 'account_id' => $victimAccount->id, + 'balance' => 9999999, + ]); + }); + + it('cannot update current balance for another user account', function () { + $attacker = User::factory()->create(); + $victim = User::factory()->create(); + $victimAccount = Account::factory()->for($victim)->create(); + + actingAs($attacker); + + $response = $this->putJson("/api/accounts/{$victimAccount->id}/balance/current", [ + 'balance' => 5000000, + ]); + + $response->assertForbidden(); + + $this->assertDatabaseMissing('account_balances', [ + 'account_id' => $victimAccount->id, + 'balance' => 5000000, + ]); + }); + + it('cannot update existing balance for another user account', function () { + $attacker = User::factory()->create(); + $victim = User::factory()->create(); + $victimAccount = Account::factory()->for($victim)->create(); + $victimBalance = AccountBalance::factory()->for($victimAccount)->create([ + 'balance' => 1000000, + ]); + + actingAs($attacker); + + $response = $this->putJson("/api/accounts/{$victimAccount->id}/balance/current", [ + 'balance' => 9999999, + ]); + + $response->assertForbidden(); + + $this->assertDatabaseHas('account_balances', [ + 'id' => $victimBalance->id, + 'balance' => 1000000, // Balance should remain unchanged + ]); + }); + + it('cannot delete balance from another user account', function () { + $attacker = User::factory()->create(); + $victim = User::factory()->create(); + $victimAccount = Account::factory()->for($victim)->create(); + $victimBalance = AccountBalance::factory()->for($victimAccount)->create(); + + actingAs($attacker); + + $response = $this->deleteJson("/api/accounts/{$victimAccount->id}/balances/{$victimBalance->id}"); + + $response->assertForbidden(); + + $this->assertDatabaseHas('account_balances', [ + 'id' => $victimBalance->id, + ]); + }); + + it('cannot list balances from another user account', function () { + $attacker = User::factory()->create(); + $victim = User::factory()->create(); + $victimAccount = Account::factory()->for($victim)->create(); + AccountBalance::factory()->for($victimAccount)->count(3)->create(); + + actingAs($attacker); + + $response = $this->getJson("/api/accounts/{$victimAccount->id}/balances"); + + $response->assertForbidden(); + }); + + it('cannot delete balance using mismatched account and balance IDs', function () { + $attacker = User::factory()->create(); + $attackerAccount = Account::factory()->for($attacker)->create(); + $victim = User::factory()->create(); + $victimAccount = Account::factory()->for($victim)->create(); + $victimBalance = AccountBalance::factory()->for($victimAccount)->create(); + + actingAs($attacker); + + // Attempt to delete victim's balance using attacker's account ID + $response = $this->deleteJson("/api/accounts/{$attackerAccount->id}/balances/{$victimBalance->id}"); + + $response->assertNotFound(); + + $this->assertDatabaseHas('account_balances', [ + 'id' => $victimBalance->id, + ]); + }); +}); + +describe('Transaction IDOR Protection', function () { + it('cannot create transaction for another user account', function () { + $attacker = User::factory()->onboarded()->create(); + $attackerAccount = Account::factory()->for($attacker)->create(); + $victim = User::factory()->onboarded()->create(); + $victimAccount = Account::factory()->for($victim)->create(); + + actingAs($attacker); + + $response = $this->post('/transactions', [ + 'account_id' => $victimAccount->id, + 'description' => 'Malicious transaction', + 'description_iv' => str_repeat('a', 16), + 'transaction_date' => now()->format('Y-m-d'), + 'amount' => -50000, + 'currency_code' => 'USD', + 'source' => 'manual', + ]); + + // The transaction might be created but should belong to attacker, not victim + // OR it should fail if account validation includes ownership check + if ($response->status() === 201) { + $transaction = Transaction::latest()->first(); + expect($transaction->user_id)->toBe($attacker->id) + ->and($transaction->account_id)->not->toBe($victimAccount->id); + } else { + // If there's validation, it should fail with 422 or redirect with errors + expect($response->status())->toBeIn([302, 422]); + } + }); + + it('cannot update transaction from another user', function () { + $attacker = User::factory()->onboarded()->create(); + $attackerCategory = Category::factory()->for($attacker)->create(); + + $victim = User::factory()->onboarded()->create(); + $victimAccount = Account::factory()->for($victim)->create(); + $victimCategory = Category::factory()->for($victim)->create(); + $victimTransaction = Transaction::factory() + ->for($victim) + ->for($victimAccount) + ->for($victimCategory) + ->create(); + + actingAs($attacker); + + $response = $this->patch("/transactions/{$victimTransaction->id}", [ + 'category_id' => $attackerCategory->id, + 'notes' => 'Hacked notes', + 'notes_iv' => str_repeat('b', 16), + ]); + + $response->assertForbidden(); + + $this->assertDatabaseHas('transactions', [ + 'id' => $victimTransaction->id, + 'category_id' => $victimCategory->id, + 'user_id' => $victim->id, + ]); + + $this->assertDatabaseMissing('transactions', [ + 'id' => $victimTransaction->id, + 'notes' => 'Hacked notes', + ]); + }); + + it('cannot delete transaction from another user', function () { + $attacker = User::factory()->onboarded()->create(); + $victim = User::factory()->onboarded()->create(); + $victimAccount = Account::factory()->for($victim)->create(); + $victimTransaction = Transaction::factory() + ->for($victim) + ->for($victimAccount) + ->create(); + + actingAs($attacker); + + $response = $this->delete("/transactions/{$victimTransaction->id}"); + + $response->assertForbidden(); + + $this->assertDatabaseHas('transactions', [ + 'id' => $victimTransaction->id, + 'deleted_at' => null, + ]); + }); + + it('cannot bulk update transactions from another user by ID', function () { + $attacker = User::factory()->onboarded()->create(); + $attackerAccount = Account::factory()->for($attacker)->create(); + $attackerCategory = Category::factory()->for($attacker)->create(); + + $victim = User::factory()->onboarded()->create(); + $victimAccount = Account::factory()->for($victim)->create(); + $victimCategory = Category::factory()->for($victim)->create(); + $victimTransactions = Transaction::factory() + ->for($victim) + ->for($victimAccount) + ->for($victimCategory) + ->count(3) + ->create(); + + actingAs($attacker); + + $response = $this->patch('/transactions/bulk', [ + 'transaction_ids' => $victimTransactions->pluck('id')->toArray(), + 'category_id' => $attackerCategory->id, + ]); + + // Should return 403 because transaction IDs don't belong to attacker + $response->assertForbidden(); + + // Verify victim's transactions remain unchanged + foreach ($victimTransactions as $transaction) { + $this->assertDatabaseHas('transactions', [ + 'id' => $transaction->id, + 'category_id' => $victimCategory->id, + 'user_id' => $victim->id, + ]); + } + }); + + it('cannot bulk update transactions from another user using filters', function () { + $attacker = User::factory()->onboarded()->create(); + $attackerCategory = Category::factory()->for($attacker)->create(); + + $victim = User::factory()->onboarded()->create(); + $victimAccount = Account::factory()->for($victim)->create(); + $victimCategory = Category::factory()->for($victim)->create(); + $victimTransactions = Transaction::factory() + ->for($victim) + ->for($victimAccount) + ->for($victimCategory) + ->count(5) + ->create([ + 'transaction_date' => now()->subDays(5), + 'amount' => 10000, + ]); + + actingAs($attacker); + + // Attempt to bulk update victim's transactions using date/amount filters + $response = $this->patch('/transactions/bulk', [ + 'filters' => [ + 'date_from' => now()->subDays(10)->format('Y-m-d'), + 'date_to' => now()->format('Y-m-d'), + 'amount_min' => 50, + 'amount_max' => 150, + ], + 'category_id' => $attackerCategory->id, + ]); + + $response->assertOk(); + + // Bulk update should only affect attacker's own transactions (0 in this case) + expect($response->json('count'))->toBe(0); + + // Verify victim's transactions remain unchanged + foreach ($victimTransactions as $transaction) { + $this->assertDatabaseHas('transactions', [ + 'id' => $transaction->id, + 'category_id' => $victimCategory->id, + 'user_id' => $victim->id, + ]); + } + }); + + it('bulk update only affects authenticated user transactions', function () { + $attacker = User::factory()->onboarded()->create(); + $attackerAccount = Account::factory()->for($attacker)->create(); + $attackerCategory = Category::factory()->for($attacker)->create(); + $attackerTransactions = Transaction::factory() + ->for($attacker) + ->for($attackerAccount) + ->count(2) + ->create(); + + $victim = User::factory()->onboarded()->create(); + $victimAccount = Account::factory()->for($victim)->create(); + $victimCategory = Category::factory()->for($victim)->create(); + $victimTransactions = Transaction::factory() + ->for($victim) + ->for($victimAccount) + ->for($victimCategory) + ->count(3) + ->create(); + + actingAs($attacker); + + // Update all transactions without filters + $response = $this->patch('/transactions/bulk', [ + 'category_id' => $attackerCategory->id, + ]); + + $response->assertOk(); + expect($response->json('count'))->toBe(2); // Only attacker's transactions + + // Verify only attacker's transactions were updated + foreach ($attackerTransactions as $transaction) { + $this->assertDatabaseHas('transactions', [ + 'id' => $transaction->id, + 'category_id' => $attackerCategory->id, + ]); + } + + // Verify victim's transactions remain unchanged + foreach ($victimTransactions as $transaction) { + $this->assertDatabaseHas('transactions', [ + 'id' => $transaction->id, + 'category_id' => $victimCategory->id, + 'user_id' => $victim->id, + ]); + } + }); +}); + +describe('Cross-Resource IDOR Protection', function () { + it('cannot use another user category when creating transaction', function () { + $attacker = User::factory()->onboarded()->create(); + $attackerAccount = Account::factory()->for($attacker)->create(); + + $victim = User::factory()->onboarded()->create(); + $victimCategory = Category::factory()->for($victim)->create(); + + actingAs($attacker); + + $response = $this->post('/transactions', [ + 'account_id' => $attackerAccount->id, + 'category_id' => $victimCategory->id, // Attempt to use victim's category + 'description' => 'Test transaction', + 'description_iv' => str_repeat('a', 16), + 'transaction_date' => now()->format('Y-m-d'), + 'amount' => -10000, + 'currency_code' => 'USD', + 'source' => 'manual', + ]); + + // Should either fail validation or ignore the category + if ($response->status() === 201) { + $transaction = Transaction::latest()->first(); + expect($transaction->category_id)->not->toBe($victimCategory->id); + } else { + expect($response->status())->toBeIn([302, 422]); + } + }); + + it('cannot use another user category when updating transaction', function () { + $attacker = User::factory()->onboarded()->create(); + $attackerAccount = Account::factory()->for($attacker)->create(); + $attackerCategory = Category::factory()->for($attacker)->create(); + $attackerTransaction = Transaction::factory() + ->for($attacker) + ->for($attackerAccount) + ->for($attackerCategory) + ->create(); + + $victim = User::factory()->onboarded()->create(); + $victimCategory = Category::factory()->for($victim)->create(); + + actingAs($attacker); + + $response = $this->patch("/transactions/{$attackerTransaction->id}", [ + 'category_id' => $victimCategory->id, // Attempt to use victim's category + ]); + + // Should fail validation with 422 or redirect with errors + expect($response->status())->toBeIn([302, 422]); + + // Verify transaction category remains unchanged + $attackerTransaction->refresh(); + expect($attackerTransaction->category_id)->toBe($attackerCategory->id); + }); + + it('cannot use another user account when creating transaction', function () { + $attacker = User::factory()->onboarded()->create(); + $victim = User::factory()->onboarded()->create(); + $victimAccount = Account::factory()->for($victim)->create(); + + actingAs($attacker); + + $response = $this->post('/transactions', [ + 'account_id' => $victimAccount->id, // Attempt to use victim's account + 'description' => 'Malicious transaction', + 'description_iv' => str_repeat('a', 16), + 'transaction_date' => now()->format('Y-m-d'), + 'amount' => -50000, + 'currency_code' => 'USD', + 'source' => 'manual', + ]); + + // Should either fail validation or the transaction should not be linked to victim's account + if ($response->status() === 201) { + $transaction = Transaction::latest()->first(); + expect($transaction->account_id)->not->toBe($victimAccount->id); + } else { + expect($response->status())->toBeIn([302, 422]); + } + }); +}); + +describe('Sync Endpoints IDOR Protection', function () { + describe('AccountBalanceSyncController', function () { + it('cannot list account balances from another user account', function () { + $attacker = User::factory()->create(); + $victim = User::factory()->create(); + $victimAccount = Account::factory()->for($victim)->create(); + AccountBalance::factory()->for($victimAccount)->count(3)->create(); + + actingAs($attacker); + + $response = $this->getJson('/api/sync/account-balances'); + + $response->assertSuccessful(); + $balances = $response->json('data'); + expect($balances)->toBeArray(); + expect(count($balances))->toBe(0); + }); + + it('cannot create account balance for another user account', function () { + $attacker = User::factory()->create(); + $victim = User::factory()->create(); + $victimAccount = Account::factory()->for($victim)->create(); + + actingAs($attacker); + + $response = $this->postJson('/api/sync/account-balances', [ + 'account_id' => $victimAccount->id, + 'balance_date' => now()->format('Y-m-d'), + 'balance' => 9999999, + ]); + + $response->assertUnprocessable(); + expect($response->json('errors.account_id'))->toBeArray(); + + $this->assertDatabaseMissing('account_balances', [ + 'account_id' => $victimAccount->id, + 'balance' => 9999999, + ]); + }); + + it('cannot update account balance from another user account', function () { + $attacker = User::factory()->create(); + $attackerAccount = Account::factory()->for($attacker)->create(); + $victim = User::factory()->create(); + $victimAccount = Account::factory()->for($victim)->create(); + $victimBalance = AccountBalance::factory()->for($victimAccount)->create([ + 'balance' => 1000000, + ]); + + actingAs($attacker); + + $response = $this->patchJson("/api/sync/account-balances/{$victimBalance->id}", [ + 'account_id' => $attackerAccount->id, + 'balance_date' => $victimBalance->balance_date->format('Y-m-d'), + 'balance' => 9999999, + ]); + + $response->assertForbidden(); + + $this->assertDatabaseHas('account_balances', [ + 'id' => $victimBalance->id, + 'balance' => 1000000, + 'account_id' => $victimAccount->id, + ]); + }); + }); + + describe('AccountSyncController', function () { + it('cannot list accounts from another user', function () { + $attacker = User::factory()->create(); + $victim = User::factory()->create(); + Account::factory()->for($victim)->count(3)->create(); + + actingAs($attacker); + + $response = $this->getJson('/api/sync/accounts'); + + $response->assertSuccessful(); + $accounts = $response->json('data'); + expect($accounts)->toBeArray(); + expect(count($accounts))->toBe(0); + }); + }); + + describe('TransactionSyncController', function () { + it('cannot list transactions from another user', function () { + $attacker = User::factory()->onboarded()->create(); + $victim = User::factory()->onboarded()->create(); + $victimAccount = Account::factory()->for($victim)->create(); + Transaction::factory()->for($victim)->for($victimAccount)->count(3)->create(); + + actingAs($attacker); + + $response = $this->getJson('/api/sync/transactions'); + + $response->assertSuccessful(); + $transactions = $response->json('data'); + expect($transactions)->toBeArray(); + expect(count($transactions))->toBe(0); + }); + + it('cannot create transaction for another user account via sync endpoint', function () { + $attacker = User::factory()->onboarded()->create(); + $victim = User::factory()->onboarded()->create(); + $victimAccount = Account::factory()->for($victim)->create(); + + actingAs($attacker); + + $response = $this->postJson('/api/sync/transactions', [ + 'account_id' => $victimAccount->id, + 'description' => 'Malicious transaction', + 'description_iv' => str_repeat('a', 16), + 'transaction_date' => now()->format('Y-m-d'), + 'amount' => -50000, + 'currency_code' => 'USD', + 'source' => 'manual', + ]); + + $response->assertUnprocessable(); + expect($response->json('errors.account_id'))->toBeArray(); + }); + + it('cannot create transaction using another user transaction ID via sync endpoint', function () { + $attacker = User::factory()->onboarded()->create(); + $attackerAccount = Account::factory()->for($attacker)->create(); + $attackerCategory = Category::factory()->for($attacker)->create(); + + $victim = User::factory()->onboarded()->create(); + $victimAccount = Account::factory()->for($victim)->create(); + $victimCategory = Category::factory()->for($victim)->create(); + $victimTransaction = Transaction::factory() + ->for($victim) + ->for($victimAccount) + ->for($victimCategory) + ->create(); + + actingAs($attacker); + + $response = $this->postJson('/api/sync/transactions', [ + 'id' => $victimTransaction->id, + 'account_id' => $attackerAccount->id, + 'description' => 'Attempted ID reuse', + 'description_iv' => str_repeat('a', 16), + 'transaction_date' => now()->format('Y-m-d'), + 'amount' => -50000, + 'currency_code' => 'USD', + 'source' => 'manually_created', + ]); + + $response->assertStatus(500); + + $this->assertDatabaseHas('transactions', [ + 'id' => $victimTransaction->id, + 'user_id' => $victim->id, + ]); + + $this->assertDatabaseMissing('transactions', [ + 'id' => $victimTransaction->id, + 'user_id' => $attacker->id, + ]); + }); + + it('idempotent create returns existing transaction when same user provides same ID', function () { + $user = User::factory()->onboarded()->create(); + $account = Account::factory()->for($user)->create(); + $category = Category::factory()->for($user)->create(); + $existingTransaction = Transaction::factory() + ->for($user) + ->for($account) + ->for($category) + ->create([ + 'description' => 'Original transaction', + 'description_iv' => str_repeat('a', 16), + ]); + + actingAs($user); + + $response = $this->postJson('/api/sync/transactions', [ + 'id' => $existingTransaction->id, + 'account_id' => $account->id, + 'description' => 'Attempted duplicate', + 'description_iv' => str_repeat('b', 16), + 'transaction_date' => now()->format('Y-m-d'), + 'amount' => -50000, + 'currency_code' => 'USD', + 'source' => 'manually_created', + ]); + + $response->assertSuccessful(); + expect($response->json('data.id'))->toBe($existingTransaction->id); + expect($response->json('data.description'))->toBe('Original transaction'); + + $this->assertDatabaseHas('transactions', [ + 'id' => $existingTransaction->id, + 'description' => 'Original transaction', + ]); + }); + + it('cannot update transaction from another user via sync endpoint', function () { + $attacker = User::factory()->onboarded()->create(); + $attackerAccount = Account::factory()->for($attacker)->create(); + $victim = User::factory()->onboarded()->create(); + $victimAccount = Account::factory()->for($victim)->create(); + $victimCategory = Category::factory()->for($victim)->create(); + $victimTransaction = Transaction::factory() + ->for($victim) + ->for($victimAccount) + ->for($victimCategory) + ->create(); + + actingAs($attacker); + + $response = $this->patchJson("/api/sync/transactions/{$victimTransaction->id}", [ + 'account_id' => $attackerAccount->id, + 'description' => 'Hacked transaction', + 'description_iv' => str_repeat('b', 16), + 'transaction_date' => $victimTransaction->transaction_date->format('Y-m-d'), + 'amount' => $victimTransaction->amount, + 'currency_code' => $victimTransaction->currency_code, + 'source' => $victimTransaction->source->value, + ]); + + $response->assertForbidden(); + + $this->assertDatabaseHas('transactions', [ + 'id' => $victimTransaction->id, + 'user_id' => $victim->id, + 'account_id' => $victimAccount->id, + ]); + }); + + it('cannot delete transaction from another user via sync endpoint', function () { + $attacker = User::factory()->onboarded()->create(); + $victim = User::factory()->onboarded()->create(); + $victimAccount = Account::factory()->for($victim)->create(); + $victimTransaction = Transaction::factory() + ->for($victim) + ->for($victimAccount) + ->create(); + + actingAs($attacker); + + $response = $this->deleteJson("/api/sync/transactions/{$victimTransaction->id}"); + + $response->assertForbidden(); + + $this->assertDatabaseHas('transactions', [ + 'id' => $victimTransaction->id, + 'deleted_at' => null, + ]); + }); + }); + + describe('CategorySyncController', function () { + it('cannot list categories from another user', function () { + $attacker = User::factory()->create(); + $victim = User::factory()->create(); + Category::factory()->for($victim)->count(3)->create(); + + actingAs($attacker); + + $response = $this->getJson('/api/sync/categories'); + + $response->assertSuccessful(); + $categories = $response->json('data'); + expect($categories)->toBeArray(); + expect(count($categories))->toBe(0); + }); + }); + + describe('LabelSyncController', function () { + it('cannot list labels from another user', function () { + $attacker = User::factory()->create(); + $victim = User::factory()->create(); + Label::factory()->for($victim)->count(3)->create(); + + actingAs($attacker); + + $response = $this->getJson('/api/sync/labels'); + + $response->assertSuccessful(); + $labels = $response->json('data'); + expect($labels)->toBeArray(); + expect(count($labels))->toBe(0); + }); + }); + + describe('BankSyncController', function () { + it('only returns global banks or user banks', function () { + $attacker = User::factory()->create(); + $victim = User::factory()->create(); + $victimBank = Bank::factory()->for($victim)->create(); + $globalBank = Bank::factory()->create(['user_id' => null]); + + actingAs($attacker); + + $response = $this->getJson('/api/sync/banks'); + + $response->assertSuccessful(); + $banks = $response->json('data'); + expect($banks)->toBeArray(); + $bankIds = collect($banks)->pluck('id')->toArray(); + expect($bankIds)->toContain($globalBank->id); + expect($bankIds)->not->toContain($victimBank->id); + }); + }); + + describe('AutomationRuleSyncController', function () { + it('cannot list automation rules from another user', function () { + $attacker = User::factory()->create(); + $victim = User::factory()->create(); + AutomationRule::factory()->for($victim)->count(3)->create(); + + actingAs($attacker); + + $response = $this->getJson('/api/sync/automation-rules'); + + $response->assertSuccessful(); + $rules = $response->json(); + expect($rules)->toBeArray(); + expect(count($rules))->toBe(0); + }); + }); +}); diff --git a/tests/Feature/Sync/TransactionSyncTest.php b/tests/Feature/Sync/TransactionSyncTest.php index b5d57655..5b759059 100644 --- a/tests/Feature/Sync/TransactionSyncTest.php +++ b/tests/Feature/Sync/TransactionSyncTest.php @@ -192,12 +192,13 @@ it('can update a transaction', function () { it('cannot update another user transaction', function () { $user = User::factory()->create(); + $userAccount = Account::factory()->for($user)->create(); $otherUser = User::factory()->create(); - $account = Account::factory()->for($otherUser)->create(); - $transaction = Transaction::factory()->for($otherUser)->for($account)->create(); + $otherAccount = Account::factory()->for($otherUser)->create(); + $transaction = Transaction::factory()->for($otherUser)->for($otherAccount)->create(); $updateData = [ - 'account_id' => $account->id, + 'account_id' => $userAccount->id, 'category_id' => null, 'description' => 'hacked', 'description_iv' => $transaction->description_iv, @@ -212,6 +213,12 @@ it('cannot update another user transaction', function () { $response = $this->actingAs($user)->patchJson("/api/sync/transactions/{$transaction->id}", $updateData); $response->assertForbidden(); + + $this->assertDatabaseHas('transactions', [ + 'id' => $transaction->id, + 'user_id' => $otherUser->id, + 'account_id' => $otherAccount->id, + ]); }); it('can delete a transaction', function () {