diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index ac174979..f483354e 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -262,6 +262,14 @@ jobs: - name: Lint Frontend run: bun run lint + # Advisory for now: the codebase carries a backlog of pre-existing + # `tsc --noEmit` errors that predate this step, so it must not gate + # merges yet. It still surfaces type regressions in the CI log. Flip + # `continue-on-error` to false once the existing backlog is cleared. + - name: Type Check Frontend + run: bun run types + continue-on-error: true + - name: Frontend Tests run: bun run test diff --git a/app/Http/Middleware/HandleInertiaRequests.php b/app/Http/Middleware/HandleInertiaRequests.php index f7edc8fe..a7991136 100644 --- a/app/Http/Middleware/HandleInertiaRequests.php +++ b/app/Http/Middleware/HandleInertiaRequests.php @@ -5,6 +5,7 @@ namespace App\Http\Middleware; use App\Enums\BankingConnectionStatus; use App\Enums\BankingProvider; use App\Features\CalculateBalancesOnImport; +use App\Jobs\PurgeResidualEncryptionArtifactsJob; use App\Models\BankingConnection; use App\Services\CurrencyOptions; use Illuminate\Foundation\Inspiring; @@ -56,12 +57,12 @@ class HandleInertiaRequests extends Middleware ->where(fn ($q) => $q->whereNotNull('description_iv')->orWhereNotNull('notes_iv')) ->exists() ?? false; - // Clean up encryption data if no encrypted accounts or transactions remain - if (! $request->is('api/*') && $user?->encryption_salt !== null) { - if (! $hasEncryptedAccounts && ! $hasEncryptedTransactions) { - $user->encryptedMessage()->delete(); - $user->update(['encryption_salt' => null]); - } + // A shared-data provider must stay read-only, so hand the residual + // encryption cleanup off to a queued job instead of mutating the user + // inline during the render. The job re-checks the condition and is + // idempotent, so dispatching it on repeat requests is harmless. + if (! $request->is('api/*') && $user?->encryption_salt !== null && ! $hasEncryptedAccounts && ! $hasEncryptedTransactions) { + PurgeResidualEncryptionArtifactsJob::dispatch($user); } return [ diff --git a/app/Jobs/PurgeResidualEncryptionArtifactsJob.php b/app/Jobs/PurgeResidualEncryptionArtifactsJob.php new file mode 100644 index 00000000..120680c9 --- /dev/null +++ b/app/Jobs/PurgeResidualEncryptionArtifactsJob.php @@ -0,0 +1,72 @@ +user->id; + } + + public function handle(): void + { + $user = $this->user->fresh(); + + if ($user === null || $user->encryption_salt === null) { + return; + } + + if ($this->hasResidualEncryptedData($user)) { + return; + } + + $user->encryptedMessage()->delete(); + $user->update(['encryption_salt' => null]); + } + + private function hasResidualEncryptedData(User $user): bool + { + $hasEncryptedAccounts = $user->accounts() + ->whereNotNull('name_iv') + ->exists(); + + if ($hasEncryptedAccounts) { + return true; + } + + return $user->transactions() + ->where(function (Builder $query): void { + $query->whereNotNull('description_iv') + ->orWhereNotNull('notes_iv'); + }) + ->exists(); + } +} diff --git a/app/Models/User.php b/app/Models/User.php index 973e1c16..e128f2af 100644 --- a/app/Models/User.php +++ b/app/Models/User.php @@ -62,6 +62,11 @@ class User extends Authenticatable implements HasLocalePreference, MustVerifyEma 'two_factor_secret', 'two_factor_recovery_codes', 'remember_token', + 'stripe_id', + 'pm_type', + 'pm_last_four', + 'trial_ends_at', + 'encryption_salt', ]; /** diff --git a/resources/js/types/index.d.ts b/resources/js/types/index.d.ts index 525a0033..3ca7b705 100644 --- a/resources/js/types/index.d.ts +++ b/resources/js/types/index.d.ts @@ -105,7 +105,6 @@ export interface User { avatar?: string; email_verified_at: string | null; two_factor_enabled?: boolean; - currency_code?: string | null; created_at: string; updated_at: string; [key: string]: unknown; diff --git a/tests/Feature/AccountControllerTest.php b/tests/Feature/AccountControllerTest.php index 3220e2d1..2076e130 100644 --- a/tests/Feature/AccountControllerTest.php +++ b/tests/Feature/AccountControllerTest.php @@ -14,6 +14,11 @@ use App\Models\User; beforeEach(function () { config(['landing.hide_auth_buttons' => false]); + // The balance-evolution endpoint converts account currency via the external + // currency-rate provider; fake it so these tests stay hermetic under the + // stray-request guard instead of hitting the CDN. + fakeCurrencyApi(); + $this->user = User::factory()->onboarded()->create(); $this->actingAs($this->user); }); diff --git a/tests/Feature/InertiaSharedDataTest.php b/tests/Feature/InertiaSharedDataTest.php index a2d2c691..f86c4ec1 100644 --- a/tests/Feature/InertiaSharedDataTest.php +++ b/tests/Feature/InertiaSharedDataTest.php @@ -1,8 +1,10 @@ onboarded()->create([ + 'stripe_id' => 'cus_test123', + 'pm_type' => 'card', + 'pm_last_four' => '4242', + 'trial_ends_at' => now()->addDays(7), + 'encryption_salt' => str_repeat('a', 24), + ]); + + $response = actingAs($user)->withoutVite()->get(route('dashboard')); + + $response->assertInertia(fn (Assert $page) => $page + ->where('auth.user.email', $user->email) + ->missing('auth.user.stripe_id') + ->missing('auth.user.pm_type') + ->missing('auth.user.pm_last_four') + ->missing('auth.user.trial_ends_at') + ->missing('auth.user.encryption_salt') + ->missing('auth.user.password') + ->missing('auth.user.two_factor_secret') + ->missing('auth.user.two_factor_recovery_codes') + ->missing('auth.user.remember_token') + ); +}); + +test('a web GET does not mutate the user inline but queues the encryption cleanup', function () { + Queue::fake(); + + $user = User::factory()->onboarded()->create([ + 'encryption_salt' => str_repeat('a', 24), + ]); + + actingAs($user)->withoutVite()->get(route('dashboard'))->assertSuccessful(); + + // Rendering the page must stay read-only: the salt is untouched inline. + expect($user->fresh()->encryption_salt)->toBe(str_repeat('a', 24)); + + // The eventual cleanup is handed off to the queued job instead. + Queue::assertPushed( + PurgeResidualEncryptionArtifactsJob::class, + fn (PurgeResidualEncryptionArtifactsJob $job) => $job->user->is($user), + ); +}); + +test('a web GET does not queue encryption cleanup when the user has no salt', function () { + Queue::fake(); + + $user = User::factory()->onboarded()->create(['encryption_salt' => null]); + + actingAs($user)->withoutVite()->get(route('dashboard'))->assertSuccessful(); + + Queue::assertNotPushed(PurgeResidualEncryptionArtifactsJob::class); +}); + test('all pages receive app url in shared props', function () { $response = $this->withoutVite()->get(route('home')); diff --git a/tests/Feature/LoanTest.php b/tests/Feature/LoanTest.php index 6b140f8f..9f2f1521 100644 --- a/tests/Feature/LoanTest.php +++ b/tests/Feature/LoanTest.php @@ -15,6 +15,11 @@ use function Pest\Laravel\assertDatabaseHas; use function Pest\Laravel\assertDatabaseMissing; beforeEach(function () { + // The balance-evolution endpoint converts account currency via the external + // currency-rate provider; fake it so these tests stay hermetic under the + // stray-request guard instead of hitting the CDN. + fakeCurrencyApi(); + $this->user = User::factory()->onboarded()->create(); $this->bank = Bank::factory()->create(); $this->service = app(LoanAmortizationService::class); diff --git a/tests/Feature/PurgeResidualEncryptionArtifactsJobTest.php b/tests/Feature/PurgeResidualEncryptionArtifactsJobTest.php new file mode 100644 index 00000000..295533f2 --- /dev/null +++ b/tests/Feature/PurgeResidualEncryptionArtifactsJobTest.php @@ -0,0 +1,84 @@ +onboarded()->create([ + 'encryption_salt' => str_repeat('a', 24), + ]); + + EncryptedMessage::query()->create([ + 'user_id' => $user->id, + 'encrypted_content' => 'encrypted_test_content', + 'iv' => str_repeat('b', 16), + ]); + + return $user; +} + +test('it clears the salt and encrypted message when no encrypted data remains', function () { + $user = userWithEncryptionArtifacts(); + + PurgeResidualEncryptionArtifactsJob::dispatchSync($user); + + expect($user->fresh()->encryption_salt)->toBeNull(); + expect(EncryptedMessage::query()->where('user_id', $user->id)->exists())->toBeFalse(); +}); + +test('it keeps the salt when an encrypted transaction still exists', function () { + $user = userWithEncryptionArtifacts(); + Transaction::factory()->create([ + 'user_id' => $user->id, + 'description_iv' => str_repeat('c', 16), + ]); + + PurgeResidualEncryptionArtifactsJob::dispatchSync($user); + + expect($user->fresh()->encryption_salt)->toBe(str_repeat('a', 24)); + expect(EncryptedMessage::query()->where('user_id', $user->id)->exists())->toBeTrue(); +}); + +test('it keeps the salt when an account name is still encrypted', function () { + $user = userWithEncryptionArtifacts(); + // name_iv (not the stale `encrypted` flag) is the source of truth for + // whether an account name is still encrypted at rest. + Account::factory()->create([ + 'user_id' => $user->id, + 'name_iv' => str_repeat('d', 16), + 'encrypted' => true, + ]); + + PurgeResidualEncryptionArtifactsJob::dispatchSync($user); + + expect($user->fresh()->encryption_salt)->toBe(str_repeat('a', 24)); + expect(EncryptedMessage::query()->where('user_id', $user->id)->exists())->toBeTrue(); +}); + +test('it keeps the salt when an account name is encrypted despite a stale encrypted flag', function () { + $user = userWithEncryptionArtifacts(); + // Stale-false flag with a still-encrypted name: keying the purge off the + // flag would wrongly destroy the salt, so the job must refuse to purge. + Account::factory()->create([ + 'user_id' => $user->id, + 'name_iv' => str_repeat('d', 16), + 'encrypted' => false, + ]); + + PurgeResidualEncryptionArtifactsJob::dispatchSync($user); + + expect($user->fresh()->encryption_salt)->toBe(str_repeat('a', 24)); + expect(EncryptedMessage::query()->where('user_id', $user->id)->exists())->toBeTrue(); +}); + +test('it is a no-op when the salt is already null', function () { + $user = User::factory()->onboarded()->create(['encryption_salt' => null]); + + PurgeResidualEncryptionArtifactsJob::dispatchSync($user); + + expect($user->fresh()->encryption_salt)->toBeNull(); +}); diff --git a/tests/Feature/StrayHttpRequestGuardTest.php b/tests/Feature/StrayHttpRequestGuardTest.php new file mode 100644 index 00000000..8b40470a --- /dev/null +++ b/tests/Feature/StrayHttpRequestGuardTest.php @@ -0,0 +1,16 @@ +throws(StrayRequestException::class); + +test('a faked request still goes through when a matching fake is registered', function () { + Http::fake(['example.com/*' => Http::response(['ok' => true])]); + + $response = Http::get('https://example.com/allowed'); + + expect($response->json('ok'))->toBeTrue(); +}); diff --git a/tests/Pest.php b/tests/Pest.php index d3d6531f..ac647803 100644 --- a/tests/Pest.php +++ b/tests/Pest.php @@ -13,7 +13,9 @@ use App\Services\Banking\BalanceSyncService; use App\Services\Banking\Sync\BankingConnectionSyncerFactory; use App\Services\Banking\TransactionSyncService; use Illuminate\Foundation\Testing\RefreshDatabase; +use Illuminate\Http\Client\Request; use Illuminate\Support\Facades\DB; +use Illuminate\Support\Facades\Http; use Stripe\Collection as StripeCollection; use Stripe\Service\SubscriptionService; use Stripe\StripeClient; @@ -52,6 +54,20 @@ pest()->beforeEach(function () { $this->withoutVite(); })->in('Feature', 'Performance'); +/* +|-------------------------------------------------------------------------- +| Block stray HTTP requests in Feature tests +|-------------------------------------------------------------------------- +| +| Any Feature test whose code path hits the network without a matching +| Http::fake() should fail loudly instead of making a real request. Tests +| that legitimately talk to external services register their own fakes, +| which take precedence over this guard. +*/ +pest()->beforeEach(function () { + Http::preventStrayRequests(); +})->in('Feature'); + /* |-------------------------------------------------------------------------- | Expectations @@ -288,3 +304,37 @@ function runSync( $job->handle(app(BankingConnectionSyncerFactory::class)); } + +/** + * Fake the external currency-rate provider (the jsdelivr CDN and its pages.dev + * fallback) that CurrencyConversionService and ExchangeRateService fetch from, + * returning a deterministic 1:1 rate for every currency. Tests that trigger a + * currency conversion (e.g. the balance-evolution endpoint) can call this in + * their setup to stay hermetic under the stray-request guard. + * + * The stub only matches the provider's `/currencies/{code}.min.json` path and + * returns null otherwise, so it never shadows another test's own Http::fake() + * nor the stray-request guard for unrelated hosts. + */ +function fakeCurrencyApi(): void +{ + Http::fake(function (Request $request) { + if (! preg_match('#/currencies/([a-z0-9]+)\.min\.json#i', $request->url(), $matches)) { + return null; + } + + $currency = strtolower($matches[1]); + + return Http::response([ + 'date' => '2024-01-01', + $currency => [ + 'usd' => 1.0, + 'eur' => 1.0, + 'gbp' => 1.0, + 'jpy' => 1.0, + 'btc' => 1.0, + 'eth' => 1.0, + ], + ]); + }); +}