From 32a8bd9a88e9311e9965067696d9143717f997fa Mon Sep 17 00:00:00 2001 From: nexxo Date: Thu, 30 Jul 2026 11:58:01 +0200 Subject: [PATCH] Refuse the sale instead of reaching Stripe without a key MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Task 5: Billing::purchase() now checks SecretVault for a stripe.secret before touching anything else — no customer, no plan lookup, no Order row — and the customer reads a plain sentence instead of a 500 page. HttpStripeClient's own secret() throws the same StripeNotConfigured exception rather than sending a request with an empty bearer token and letting Stripe's 401 stand in for the real cause. isConfigured() was fixed alongside it: it used to call secret() too, which would have turned every "is Stripe set up?" check (the checkout controller, the catalogue sync commands) into an uncaught exception the moment a key went missing — the opposite of what this task is for. It now reads the vault directly. The new guard is unconditional, including for the cart-only purchase types (storage, addon, plan changes) that never call Stripe themselves — a cart order nobody can ever pay for is pointless to create. That broke every existing Billing/Cart/Downgrade test that calls purchase() without a stored key; each affected fixture now deposits one via the new withStripeSecret() Pest helper. A few of those tests (CustomDomainAccessTest, DowngradeTest) were passing already but for the wrong reason — the new guard, not the check they were named for — since both only assert an order was NOT created; they get the same fixture fix so they still prove what they claim. Co-Authored-By: Claude Opus 5 --- app/Exceptions/StripeNotConfigured.php | 20 ++++++ app/Livewire/Billing.php | 10 +++ app/Services/Stripe/HttpStripeClient.php | 36 +++++++---- lang/de/billing.php | 4 ++ lang/en/billing.php | 4 ++ resources/views/livewire/billing.blade.php | 9 ++- .../Feature/Billing/AddonEntitlementTest.php | 4 ++ .../Billing/CustomDomainAccessTest.php | 6 ++ .../Billing/PaidOrderFulfilmentTest.php | 7 +++ .../Feature/Billing/PendingPlanChangeTest.php | 5 ++ .../Feature/Billing/StorageAllowanceTest.php | 5 ++ tests/Feature/BillingTest.php | 4 ++ tests/Feature/CartTest.php | 21 ++++--- .../Feature/CheckoutWithoutStripeKeyTest.php | 62 +++++++++++++++++++ tests/Feature/DowngradeTest.php | 7 ++- tests/Pest.php | 17 +++++ 16 files changed, 200 insertions(+), 21 deletions(-) create mode 100644 app/Exceptions/StripeNotConfigured.php create mode 100644 tests/Feature/CheckoutWithoutStripeKeyTest.php diff --git a/app/Exceptions/StripeNotConfigured.php b/app/Exceptions/StripeNotConfigured.php new file mode 100644 index 0000000..d252a47 --- /dev/null +++ b/app/Exceptions/StripeNotConfigured.php @@ -0,0 +1,20 @@ +get('stripe.secret'))) { + $this->addError('purchase', __('billing.stripe_not_configured')); + + return; + } + $customer = $this->requireCustomer(); if ($customer === null) { return; diff --git a/app/Services/Stripe/HttpStripeClient.php b/app/Services/Stripe/HttpStripeClient.php index a88a1a0..1a4e660 100644 --- a/app/Services/Stripe/HttpStripeClient.php +++ b/app/Services/Stripe/HttpStripeClient.php @@ -2,7 +2,9 @@ namespace App\Services\Stripe; +use App\Exceptions\StripeNotConfigured; use App\Services\Secrets\SecretVault; +use App\Support\OperatingMode; use Illuminate\Http\Client\PendingRequest; use Illuminate\Support\Facades\Http; use RuntimeException; @@ -16,7 +18,12 @@ class HttpStripeClient implements StripeClient { public function isConfigured(): bool { - return filled($this->secret()); + // Reads the vault directly rather than through secret(): that method + // now THROWS when the key is missing (see below), and every caller + // here — the checkout controller, the catalogue sync commands — asks + // this question to decide whether to proceed gracefully. Routing it + // through secret() would turn "is it configured?" into an exception. + return filled(app(SecretVault::class)->get('stripe.secret')); } public function createCheckoutSession( @@ -336,13 +343,9 @@ class HttpStripeClient implements StripeClient private function request(?string $idempotencyKey = null): PendingRequest { - $secret = (string) $this->secret(); - - if (blank($secret)) { - throw new RuntimeException('Stripe is not configured (STRIPE_SECRET is empty).'); - } - - $request = Http::withToken($secret)->acceptJson()->timeout(20); + // secret() itself throws when the key is missing, so there is nothing + // blank left to check here — see its docblock. + $request = Http::withToken($this->secret())->acceptJson()->timeout(20); // Stripe replays the original response for a repeated key instead of // creating a second object. That covers the gap this cannot close on @@ -355,16 +358,25 @@ class HttpStripeClient implements StripeClient } /** - * The key in force: the one stored in the console if there is one, else the - * environment. + * The key in force: the one stored in the console for the active operating + * mode. Throws rather than returning blank — see StripeNotConfigured. * * Resolved HERE, at the point of use, rather than overlaid onto config at * boot — an overlay would add a query to every request including the public * site, and a queue worker would keep whatever was true when it started. */ - private function secret(): ?string + private function secret(): string { - return app(SecretVault::class)->get('stripe.secret'); + $secret = app(SecretVault::class)->get('stripe.secret'); + + // Ohne Token abzusenden hieße, Stripe mit einem leeren Bearer zu fragen + // und einen 401 zu bekommen, den niemand einem fehlenden Schlüssel + // zuordnet. Hier abzubrechen sagt, was fehlt. + if (blank($secret)) { + throw StripeNotConfigured::forMode(OperatingMode::current()->value); + } + + return $secret; } private function url(string $path): string diff --git a/lang/de/billing.php b/lang/de/billing.php index ee0b526..890bef2 100644 --- a/lang/de/billing.php +++ b/lang/de/billing.php @@ -126,6 +126,10 @@ return [ 'pending_change_note' => 'Wechsel auf :plan zum :date vorgemerkt. Bis dahin ändert sich nichts an Ihrem Paket.', 'purchased' => 'Kauf vorgemerkt — wir schalten ihn nach der Zahlung frei.', 'mock_note' => 'Zahlung & Bereitstellung folgen nach Anbindung des Zahlungsanbieters.', + // Bewusst ohne Grund: weder "Stripe-Schlüssel fehlt" noch der aktive + // Betriebsmodus gehören dem Kunden mitgeteilt — das ist eine + // Betreiberangelegenheit und steht auf der Bereitschaftsseite. + 'stripe_not_configured' => 'Die Bezahlung ist derzeit nicht eingerichtet. Bitte versuchen Sie es später erneut.', 'plan' => [ 'start' => 'Start', diff --git a/lang/en/billing.php b/lang/en/billing.php index d16f43b..9693921 100644 --- a/lang/en/billing.php +++ b/lang/en/billing.php @@ -126,6 +126,10 @@ return [ 'pending_change_note' => 'A move to :plan is booked for :date. Nothing about your package changes until then.', 'purchased' => 'Purchase noted — we activate it after payment.', 'mock_note' => 'Payment & fulfillment follow once the payment provider is connected.', + // Deliberately without a reason: neither "the Stripe key is missing" nor + // which operating mode is active is the customer's to know — that is an + // operator matter and belongs on the readiness page instead. + 'stripe_not_configured' => 'Payment is not set up at the moment. Please try again later.', 'plan' => [ 'start' => 'Start', diff --git a/resources/views/livewire/billing.blade.php b/resources/views/livewire/billing.blade.php index 6ba8ae9..15be227 100644 --- a/resources/views/livewire/billing.blade.php +++ b/resources/views/livewire/billing.blade.php @@ -1,4 +1,4 @@ - +
@php $loc = app()->getLocale(); $eur = fn ($c) => Number::currency($c / 100, in: 'EUR', locale: $loc); @endphp @@ -7,6 +7,13 @@

{{ __('billing.subtitle') }}

+ {{-- purchase() refuses before touching anything when the active mode has + no Stripe key — a sentence here, not a 500 page, and not a cart entry + nobody can ever pay for. --}} + @error('purchase') + {{ $message }} + @enderror + {{-- Current plan --}}
diff --git a/tests/Feature/Billing/AddonEntitlementTest.php b/tests/Feature/Billing/AddonEntitlementTest.php index a970f59..2b82461 100644 --- a/tests/Feature/Billing/AddonEntitlementTest.php +++ b/tests/Feature/Billing/AddonEntitlementTest.php @@ -51,6 +51,10 @@ function entitlementShopper(string $plan = 'team'): array $order->subscription->update(['instance_id' => $instance->id]); + // The tests that buy through the shop call Billing::purchase(), which + // since Task 5 refuses outright without a Stripe key for the active mode. + withStripeSecret(); + return [$customer, $user, $order->subscription->fresh()]; } diff --git a/tests/Feature/Billing/CustomDomainAccessTest.php b/tests/Feature/Billing/CustomDomainAccessTest.php index c63bacc..af14c35 100644 --- a/tests/Feature/Billing/CustomDomainAccessTest.php +++ b/tests/Feature/Billing/CustomDomainAccessTest.php @@ -46,6 +46,12 @@ function domainCustomer(string $plan, array $instanceAttributes = []): array 'instance_id' => $instance->id, ]); + // One test below calls Billing::purchase(), which since Task 5 refuses + // outright without a Stripe key for the active mode. Without this, that + // test would see the same "no order" result for the wrong reason — the + // Stripe guard, not the plan-restriction it names — and prove nothing. + withStripeSecret(); + return [$customer, $user, $instance, $subscription]; } diff --git a/tests/Feature/Billing/PaidOrderFulfilmentTest.php b/tests/Feature/Billing/PaidOrderFulfilmentTest.php index bdf4d48..b60a1ea 100644 --- a/tests/Feature/Billing/PaidOrderFulfilmentTest.php +++ b/tests/Feature/Billing/PaidOrderFulfilmentTest.php @@ -33,6 +33,13 @@ use Livewire\Livewire; beforeEach(function () { $this->stripe = new FakeStripeClient; app()->instance(StripeClient::class, $this->stripe); + + // Every test here calls Billing::purchase() to place the order it then + // marks paid by hand. Since Task 5, that call refuses outright without a + // Stripe key for the active mode — binding the fake StripeClient above + // does not help, because the guard reads the vault directly rather than + // going through the StripeClient interface. + withStripeSecret(); }); /** A customer on a live contract, with a machine. */ diff --git a/tests/Feature/Billing/PendingPlanChangeTest.php b/tests/Feature/Billing/PendingPlanChangeTest.php index be5c640..49369ad 100644 --- a/tests/Feature/Billing/PendingPlanChangeTest.php +++ b/tests/Feature/Billing/PendingPlanChangeTest.php @@ -45,6 +45,11 @@ function pendingChangeCustomer(string $plan = 'business'): array // due date arrives against this id, so there has to be one. $subscription->update(['instance_id' => $instance->id, 'stripe_subscription_id' => 'sub_'.$customer->id]); + // These tests call Billing::purchase() to book the downgrade/upgrade, + // which since Task 5 refuses outright without a Stripe key for the + // active mode. + withStripeSecret(); + return [$customer, $user, $subscription->fresh(), $instance]; } diff --git a/tests/Feature/Billing/StorageAllowanceTest.php b/tests/Feature/Billing/StorageAllowanceTest.php index 8cbaa3e..82e3ec7 100644 --- a/tests/Feature/Billing/StorageAllowanceTest.php +++ b/tests/Feature/Billing/StorageAllowanceTest.php @@ -65,6 +65,11 @@ function storageFixture(string $plan = 'team', array $instanceAttributes = []): $subscription->update(['instance_id' => $instance->id]); + // Some of these tests book storage packs through Billing::purchase(), + // which since Task 5 refuses outright without a Stripe key for the + // active mode. + withStripeSecret(); + return [ 'host' => $host, 'order' => $order, diff --git a/tests/Feature/BillingTest.php b/tests/Feature/BillingTest.php index c1eb0d0..0b7340a 100644 --- a/tests/Feature/BillingTest.php +++ b/tests/Feature/BillingTest.php @@ -14,6 +14,10 @@ function billingSetup(string $plan = 'start'): array $order = Order::factory()->create(['customer_id' => $customer->id, 'plan' => $plan]); Instance::factory()->create(['customer_id' => $customer->id, 'order_id' => $order->id, 'plan' => $plan, 'status' => 'active']); + // Several of these tests call Billing::purchase(), which since Task 5 + // refuses outright without a Stripe key for the active mode. + withStripeSecret(); + return compact('user', 'customer'); } diff --git a/tests/Feature/CartTest.php b/tests/Feature/CartTest.php index ae83d52..32f9479 100644 --- a/tests/Feature/CartTest.php +++ b/tests/Feature/CartTest.php @@ -3,14 +3,21 @@ use App\Livewire\Billing; use App\Livewire\ConfirmRemoveOrder; use App\Models\Customer; +use App\Models\Instance; use App\Models\Order; use App\Models\User; +use Illuminate\Support\Carbon; +use Illuminate\Support\Number; function cartCustomer(): array { $customer = Customer::factory()->create(); $user = User::factory()->create(['email' => $customer->email]); + // Some of these tests call Billing::purchase(), which since Task 5 + // refuses outright without a Stripe key for the active mode. + withStripeSecret(); + return [$customer, $user]; } @@ -73,7 +80,7 @@ it('refuses to remove an order that is already paid', function () { it('keeps at most one plan change in the cart', function () { [$customer, $user] = cartCustomer(); - App\Models\Instance::factory()->for($customer)->create(['plan' => 'start', 'status' => 'active']); + Instance::factory()->for($customer)->create(['plan' => 'start', 'status' => 'active']); $component = Livewire\Livewire::actingAs($user)->test(Billing::class); $component->call('purchase', 'upgrade', 'business'); @@ -88,7 +95,7 @@ it('keeps at most one plan change in the cart', function () { it('still lets add-ons stack', function () { [$customer, $user] = cartCustomer(); - App\Models\Instance::factory()->for($customer)->create(['plan' => 'team', 'status' => 'active']); + Instance::factory()->for($customer)->create(['plan' => 'team', 'status' => 'active']); $component = Livewire\Livewire::actingAs($user)->test(Billing::class); $component->call('purchase', 'storage'); @@ -127,7 +134,7 @@ it('separates what recurs from a one-off top-up', function () { // Formatted through the same helper the view uses, or the non-breaking // space in the currency makes this a test about whitespace. $expected = __('billing.cart.recurring_note', [ - 'amount' => Illuminate\Support\Number::currency(12.00, in: 'EUR', locale: 'de'), + 'amount' => Number::currency(12.00, in: 'EUR', locale: 'de'), ]); Livewire\Livewire::actingAs($user)->test(Billing::class)->assertSee($expected); @@ -143,7 +150,7 @@ it('keeps the recurring figure consistent with the total to the cent', function Order::factory()->for($customer)->create(['type' => 'storage', 'amount_cents' => 3, 'status' => 'pending']); Order::factory()->for($customer)->create(['type' => 'storage', 'amount_cents' => 3, 'status' => 'pending']); - $expected = Illuminate\Support\Number::currency(0.08, in: 'EUR', locale: 'de'); + $expected = Number::currency(0.08, in: 'EUR', locale: 'de'); Livewire\Livewire::actingAs($user)->test(Billing::class) ->assertSee($expected) // total gross @@ -188,7 +195,7 @@ it('accepts a verified value stored in display form', function () { config()->set('provisioning.tax.rate_percent', 20); config()->set('provisioning.tax.seller_country', 'AT'); - [$customer, ] = cartCustomer(); + [$customer] = cartCustomer(); Order::factory()->for($customer)->create(['type' => 'storage', 'amount_cents' => 1000, 'status' => 'pending']); // A verifier may hand back the number the way it prints it; spaces are @@ -200,7 +207,7 @@ it('accepts a verified value stored in display form', function () { ]); expect($customer->fresh()->hasVerifiedVatId())->toBeTrue() - ->and($customer->fresh()->vat_id_verified_at)->toBeInstanceOf(Illuminate\Support\Carbon::class) + ->and($customer->fresh()->vat_id_verified_at)->toBeInstanceOf(Carbon::class) ->and(Order::query()->first()->fresh()->grossCents())->toBe(1000); }); @@ -208,7 +215,7 @@ it('will not let a typed-in VAT ID zero the tax', function () { config()->set('provisioning.tax.rate_percent', 20); config()->set('provisioning.tax.seller_country', 'AT'); - [$customer, ] = cartCustomer(); + [$customer] = cartCustomer(); Order::factory()->for($customer)->create(['type' => 'storage', 'amount_cents' => 1000, 'status' => 'pending']); // Plausible-looking but unverified: still domestic VAT. diff --git a/tests/Feature/CheckoutWithoutStripeKeyTest.php b/tests/Feature/CheckoutWithoutStripeKeyTest.php new file mode 100644 index 0000000..ea49efe --- /dev/null +++ b/tests/Feature/CheckoutWithoutStripeKeyTest.php @@ -0,0 +1,62 @@ + app(HttpStripeClient::class)->createPrice('prod_x', 100, 'EUR', 'month')) + ->toThrow(StripeNotConfigured::class); +}); + +it('does not create an order when the key is missing', function () { + OperatingMode::set(OperatingMode::Test); + $customer = Customer::factory()->create(); + // A portal user is matched to its customer by email, not by a foreign + // key — see the existing Billing feature tests (e.g. StorageAllowanceTest). + $user = User::factory()->create(['email' => $customer->email]); + + // 'team' is a real, sellable plan (seeded by the plan-catalogue migration) + // that ranks above the customer's implicit 'start' plan — a genuine + // upgrade the purchase() guards would otherwise accept. A made-up key + // would be refused by the existing plan-validity check regardless of + // whether the Stripe guard works at all, and would prove nothing. + // + // assertHasErrors(), not assertHasNoErrors(): the guard reports through + // addError('purchase', …) — the same page-blocking pattern order.blade.php + // already uses for "cannot proceed" — and Livewire's test harness folds + // any non-empty error bag into the call's result. Asserting no errors + // here would contradict the very behaviour this test exists to prove. + Livewire::actingAs($user) + ->test(Billing::class) + ->call('purchase', 'upgrade', 'team') + ->assertHasErrors('purchase'); + + // Der Punkt: nach der Zahlung wäre es zu spät. Es darf gar nichts entstehen. + expect(Order::count())->toBe(0); +}); + +it('says why instead of failing silently', function () { + OperatingMode::set(OperatingMode::Test); + $customer = Customer::factory()->create(); + $user = User::factory()->create(['email' => $customer->email]); + + Livewire::actingAs($user) + ->test(Billing::class) + ->call('purchase', 'upgrade', 'team') + ->assertSee(__('billing.stripe_not_configured')); +}); diff --git a/tests/Feature/DowngradeTest.php b/tests/Feature/DowngradeTest.php index d8700c4..33627ac 100644 --- a/tests/Feature/DowngradeTest.php +++ b/tests/Feature/DowngradeTest.php @@ -19,7 +19,6 @@ use Livewire\Livewire; * disabled button that does not name the obstacle is the thing people ring * about. */ - function downgradeCustomer(int $seats = 1, ?int $usedBytes = null): array { $user = User::factory()->create(); @@ -44,6 +43,12 @@ function downgradeCustomer(int $seats = 1, ?int $usedBytes = null): array ]); } + // One test below calls Billing::purchase(), which since Task 5 refuses + // outright without a Stripe key for the active mode. Without this, that + // test would see the same "no order" result for the wrong reason — the + // Stripe guard, not the seat block it names — and prove nothing. + withStripeSecret(); + return [$user, $customer, $instance]; } diff --git a/tests/Pest.php b/tests/Pest.php index 7db9704..26b48b7 100644 --- a/tests/Pest.php +++ b/tests/Pest.php @@ -11,6 +11,7 @@ use App\Services\Monitoring\FakeMonitoringClient; use App\Services\Monitoring\MonitoringClient; use App\Services\Proxmox\FakeProxmoxClient; use App\Services\Proxmox\ProxmoxClient; +use App\Services\Secrets\SecretVault; use App\Services\Ssh\FakeRemoteShell; use App\Services\Ssh\RemoteShell; use App\Services\Traefik\FakeTraefikWriter; @@ -117,6 +118,22 @@ function admin(): Operator return Operator::factory()->role('Owner')->create(['password' => 'password']); } +/** + * Deposits a Stripe key for whichever operating mode is current, so + * Billing::purchase() — refusing outright without one since Task 5 — actually + * reaches the code a test is about instead of stopping before anything is + * touched. + * + * Deliberately NOT a global beforeEach(): CheckoutWithoutStripeKeyTest and + * StripeStrictModeTest are precisely about the absence of this key, and a + * suite-wide fixture would quietly defeat them. Call this only from a test's + * own fixture/beforeEach where a purchase is expected to go through. + */ +function withStripeSecret(): void +{ + app(SecretVault::class)->put('stripe.secret', 'sk_test_fixture', Operator::factory()->create()); +} + /** * Undo the mailbox-seeding migration's baseline, for tests written before it. *