From 735187868cd9e2827ba4aa19c8b94544c43e5bb3 Mon Sep 17 00:00:00 2001 From: Andras Bacsai <5845193+andrasbacsai@users.noreply.github.com> Date: Mon, 21 Sep 2026 15:35:31 +0200 Subject: [PATCH] fix(webhooks): reject incomplete Stripe configuration --- .ai/lessons.md | 3 + app/Http/Controllers/Webhook/Stripe.php | 23 +-- tests/Feature/StripeWebhookSecurityTest.php | 148 ++++++++++++++++++++ 3 files changed, 166 insertions(+), 8 deletions(-) create mode 100644 tests/Feature/StripeWebhookSecurityTest.php diff --git a/.ai/lessons.md b/.ai/lessons.md index c2f648ca9e..de73b119be 100644 --- a/.ai/lessons.md +++ b/.ai/lessons.md @@ -46,3 +46,6 @@ - Use the database as the correctness source for dynamic cron occurrences shared by multiple scheduler and Horizon nodes; Redis locks are load controls, not a durable execution ledger. - Give each schedule occurrence a unique database identity and make queue consumers claim it atomically before external work. - Keep pending occurrences recoverable across publisher interruptions, and define an explicit bounded policy for late or offline schedules. + +## Fail closed at public webhook boundaries +- Reject missing or blank secrets before signature verification, and return generic errors without logging secrets, signatures, or payloads. diff --git a/app/Http/Controllers/Webhook/Stripe.php b/app/Http/Controllers/Webhook/Stripe.php index 41e70b2ce0..1b5dd31385 100644 --- a/app/Http/Controllers/Webhook/Stripe.php +++ b/app/Http/Controllers/Webhook/Stripe.php @@ -4,17 +4,24 @@ namespace App\Http\Controllers\Webhook; use App\Http\Controllers\Controller; use App\Jobs\StripeProcessJob; -use Exception; use Illuminate\Http\Request; use Stripe\Exception\SignatureVerificationException; use Stripe\Webhook; +use Throwable; class Stripe extends Controller { public function events(Request $request) { try { + $apiKey = config('subscription.stripe_api_key'); $webhookSecret = config('subscription.stripe_webhook_secret'); + if (! is_string($apiKey) || trim($apiKey) === '' || ! is_string($webhookSecret) || trim($webhookSecret) === '') { + auditLogWebhookFailure('stripe', 'stripe_not_configured'); + + return response('Invalid signature.', 400); + } + $signature = $request->header('Stripe-Signature'); $event = Webhook::constructEvent( $request->getContent(), @@ -24,14 +31,14 @@ class Stripe extends Controller StripeProcessJob::dispatch($event); return response('Webhook received. Cool cool cool cool cool.', 200); - } catch (SignatureVerificationException $e) { - auditLogWebhookFailure('stripe', 'invalid_signature', [ - 'error' => $e->getMessage(), - ]); + } catch (SignatureVerificationException) { + auditLogWebhookFailure('stripe', 'invalid_signature'); - return response($e->getMessage(), 400); - } catch (Exception $e) { - return response($e->getMessage(), 400); + return response('Invalid signature.', 400); + } catch (Throwable) { + auditLogWebhookFailure('stripe', 'invalid_payload'); + + return response('Invalid webhook.', 400); } } } diff --git a/tests/Feature/StripeWebhookSecurityTest.php b/tests/Feature/StripeWebhookSecurityTest.php new file mode 100644 index 0000000000..ccf818d032 --- /dev/null +++ b/tests/Feature/StripeWebhookSecurityTest.php @@ -0,0 +1,148 @@ +set('subscription.stripe_api_key', 'sk_test_configured'); +}); + +function stripeWebhookPayload(): string +{ + return json_encode([ + 'id' => 'evt_security_test', + 'type' => 'customer.subscription.updated', + 'data' => ['object' => [ + 'id' => 'sub_test', + 'customer' => 'cus_test', + 'metadata' => ['team_id' => 999], + 'status' => 'unpaid', + ]], + ], JSON_THROW_ON_ERROR); +} + +function stripeSignature(string $payload, string $secret, ?int $timestamp = null): string +{ + $timestamp ??= time(); + + return sprintf('t=%d,v1=%s', $timestamp, hash_hmac('sha256', $timestamp.'.'.$payload, $secret)); +} + +function postStripeWebhook(string $payload, ?string $signature = null): TestResponse +{ + $server = ['CONTENT_TYPE' => 'application/json']; + if ($signature !== null) { + $server['HTTP_STRIPE_SIGNATURE'] = $signature; + } + + return test()->call('POST', '/webhooks/payments/stripe/events', [], [], [], $server, $payload); +} + +test('missing null and blank webhook secrets fail closed without dispatching work', function (?string $secret) { + config()->set('subscription.stripe_webhook_secret', $secret); + $payload = stripeWebhookPayload(); + + postStripeWebhook($payload, stripeSignature($payload, trim((string) $secret))) + ->assertBadRequest() + ->assertContent('Invalid signature.'); + + Queue::assertNotPushed(StripeProcessJob::class); +})->with([ + 'null' => null, + 'empty' => '', + 'spaces' => ' ', +]); + +test('missing null and blank Stripe API keys disable the webhook', function (?string $apiKey) { + config()->set('subscription.stripe_api_key', $apiKey); + config()->set('subscription.stripe_webhook_secret', 'whsec_correct'); + $payload = stripeWebhookPayload(); + + postStripeWebhook($payload, stripeSignature($payload, 'whsec_correct')) + ->assertBadRequest() + ->assertContent('Invalid signature.'); + + Queue::assertNotPushed(StripeProcessJob::class); +})->with([ + 'null' => null, + 'empty' => '', + 'spaces' => ' ', +]); + +test('missing malformed invalid expired and wrong signatures fail closed', function (?string $signature) { + config()->set('subscription.stripe_webhook_secret', 'whsec_correct'); + $payload = stripeWebhookPayload(); + + if ($signature === 'expired') { + $signature = stripeSignature($payload, 'whsec_correct', time() - 301); + } elseif ($signature === 'wrong') { + $signature = stripeSignature($payload, 'whsec_wrong'); + } + + postStripeWebhook($payload, $signature) + ->assertBadRequest() + ->assertContent('Invalid signature.'); + + Queue::assertNotPushed(StripeProcessJob::class); +})->with([ + 'missing' => null, + 'malformed' => 'not-a-stripe-signature', + 'invalid' => 't=123,v1=invalid', + 'expired' => 'expired', + 'wrong secret' => 'wrong', +]); + +test('an event cannot change state when Stripe is not configured', function () { + config()->set('constants.coolify.self_hosted', true); + config()->set('subscription.stripe_webhook_secret', null); + $team = Team::factory()->create(); + $subscription = Subscription::create([ + 'team_id' => $team->id, + 'stripe_subscription_id' => 'sub_existing', + 'stripe_customer_id' => 'cus_existing', + 'stripe_invoice_paid' => true, + ]); + $server = Server::factory()->create(['team_id' => $team->id]); + $server->settings()->update(['is_usable' => true, 'is_reachable' => true]); + $payload = stripeWebhookPayload(); + + postStripeWebhook($payload, stripeSignature($payload, ''))->assertBadRequest(); + + expect($subscription->fresh()->stripe_subscription_id)->toBe('sub_existing') + ->and($subscription->fresh()->stripe_invoice_paid)->toBeTruthy() + ->and($server->fresh()->settings->is_usable)->toBeTruthy() + ->and($server->fresh()->settings->is_reachable)->toBeTruthy(); + Queue::assertNotPushed(StripeProcessJob::class); +}); + +test('a valid signature with a malformed payload is rejected before dispatch', function () { + config()->set('subscription.stripe_webhook_secret', 'whsec_correct'); + $payload = '{malformed'; + + postStripeWebhook($payload, stripeSignature($payload, 'whsec_correct')) + ->assertBadRequest() + ->assertContent('Invalid webhook.'); + + Queue::assertNotPushed(StripeProcessJob::class); +}); + +test('a valid signed event is accepted in cloud and self-hosted modes', function (bool $selfHosted) { + config()->set('constants.coolify.self_hosted', $selfHosted); + config()->set('subscription.stripe_webhook_secret', 'whsec_correct'); + $payload = stripeWebhookPayload(); + + postStripeWebhook($payload, stripeSignature($payload, 'whsec_correct'))->assertSuccessful(); + + Queue::assertPushed(StripeProcessJob::class, 1); +})->with([ + 'cloud' => false, + 'self-hosted with Stripe configured' => true, +]);