From 5d20f54f7125bc80ccf181b9e4883d6cea2fc92c Mon Sep 17 00:00:00 2001 From: Andras Bacsai <5845193+andrasbacsai@users.noreply.github.com> Date: Tue, 6 Oct 2026 12:56:50 +0200 Subject: [PATCH] fix(webhooks): use a neutral reply for manual webhooks without a match --- .../MatchesManualWebhookApplications.php | 2 +- .../AdvisorySecurityRegressionTest.php | 6 +- tests/Feature/Security/AuditLogTest.php | 8 +- tests/Feature/Webhook/WebhookHmacTest.php | 76 +++++++++++-------- 4 files changed, 53 insertions(+), 39 deletions(-) diff --git a/app/Http/Controllers/Webhook/Concerns/MatchesManualWebhookApplications.php b/app/Http/Controllers/Webhook/Concerns/MatchesManualWebhookApplications.php index 5f8f20ded4..7f2feb1e04 100644 --- a/app/Http/Controllers/Webhook/Concerns/MatchesManualWebhookApplications.php +++ b/app/Http/Controllers/Webhook/Concerns/MatchesManualWebhookApplications.php @@ -60,7 +60,7 @@ trait MatchesManualWebhookApplications { return [ 'status' => 'failed', - 'message' => 'Invalid signature.', + 'message' => 'No matching application or invalid signature.', ]; } diff --git a/tests/Feature/AdvisorySecurityRegressionTest.php b/tests/Feature/AdvisorySecurityRegressionTest.php index 5e7e49281a..452308d22a 100644 --- a/tests/Feature/AdvisorySecurityRegressionTest.php +++ b/tests/Feature/AdvisorySecurityRegressionTest.php @@ -85,7 +85,7 @@ it('throttles only failed authentication on manual webhook routes', function (st $helper->reply([['status' => 'success', 'message' => 'queued']], $failureKey); expect(RateLimiter::attempts($failureKey))->toBe(0); - $helper->reply([['status' => 'failed', 'message' => 'Invalid signature.']], $failureKey); + $helper->reply([['status' => 'failed', 'message' => 'No matching application or invalid signature.']], $failureKey); expect(RateLimiter::attempts($failureKey))->toBe(1); })->with(['github', 'gitlab', 'bitbucket', 'gitea']); @@ -99,11 +99,11 @@ it('does not reveal how many applications share a manual webhook repository', fu return $this->manualWebhookResponse(collect($payloads), 'manual-webhook-failures:test', $this->manualWebhookTokenAttempt('wrong-token'))->getContent(); } }; - $failure = ['status' => 'failed', 'message' => 'Invalid signature.']; + $failure = ['status' => 'failed', 'message' => 'No matching application or invalid signature.']; expect($helper->reply([$failure, $failure]))->toBe($helper->reply([$failure])); expect($helper->reply([$failure, ['status' => 'success', 'message' => 'queued']])) - ->not->toContain('Invalid signature.'); + ->not->toContain('No matching application or invalid signature.'); }); it('never exposes a shown-once database variable even with sensitive read access', function () { diff --git a/tests/Feature/Security/AuditLogTest.php b/tests/Feature/Security/AuditLogTest.php index ebf6f13d8d..dc2408ab25 100644 --- a/tests/Feature/Security/AuditLogTest.php +++ b/tests/Feature/Security/AuditLogTest.php @@ -247,7 +247,7 @@ describe('webhook signature failure logging', function () { ], $payload); $response->assertOk(); - expect($response->getContent())->toContain('Invalid signature'); + expect($response->getContent())->toContain('No matching application or invalid signature.'); }); test('GitLab manual webhook with bad token logs to audit channel', function () { @@ -275,7 +275,7 @@ describe('webhook signature failure logging', function () { ]); $response->assertOk(); - expect($response->getContent())->toContain('Invalid signature'); + expect($response->getContent())->toContain('No matching application or invalid signature.'); }); test('Bitbucket manual webhook with malformed signature logs to audit channel', function () { @@ -304,7 +304,7 @@ describe('webhook signature failure logging', function () { ], $payload); $response->assertOk(); - expect($response->getContent())->toContain('Invalid signature'); + expect($response->getContent())->toContain('No matching application or invalid signature.'); }); test('Gitea manual webhook with bad signature logs to audit channel', function () { @@ -335,7 +335,7 @@ describe('webhook signature failure logging', function () { ], $payload); $response->assertOk(); - expect($response->getContent())->toContain('Invalid signature'); + expect($response->getContent())->toContain('No matching application or invalid signature.'); }); }); diff --git a/tests/Feature/Webhook/WebhookHmacTest.php b/tests/Feature/Webhook/WebhookHmacTest.php index f62697554f..1ff2e2c742 100644 --- a/tests/Feature/Webhook/WebhookHmacTest.php +++ b/tests/Feature/Webhook/WebhookHmacTest.php @@ -104,7 +104,7 @@ function lockOutManualWebhookRepository(TestCase $test, string $provider, Applic $response = sendManualWebhookPush($test, $provider, $application, validSignature: false, repository: $repository, branch: $branch); $response->assertOk(); - expect($response->getContent())->toContain('Invalid signature'); + expect($response->getContent())->toContain('No matching application or invalid signature.'); } sendManualWebhookPush($test, $provider, $application, validSignature: false, repository: $repository, branch: $branch) @@ -144,7 +144,7 @@ describe('Manual Webhook Failed Authentication Rate Limiting', function () { $response = sendManualWebhookPush($this, $provider, $application); expect($response->getStatusCode())->toBe(200); - expect($response->getContent())->not->toContain('Invalid signature'); + expect($response->getContent())->not->toContain('No matching application or invalid signature.'); } expect(RateLimiter::attempts(manualWebhookFailureKey($provider)))->toBe(0); @@ -190,7 +190,7 @@ describe('Manual Webhook Failed Authentication Rate Limiting', function () { $response = sendManualWebhookPush($this, $provider, $application, validSignature: false, extraServer: ['HTTP_X_FORWARDED_FOR' => "198.51.100.{$i}"]); $response->assertOk(); - expect($response->getContent())->toContain('Invalid signature'); + expect($response->getContent())->toContain('No matching application or invalid signature.'); } sendManualWebhookPush($this, $provider, $application, validSignature: false, extraServer: ['HTTP_X_FORWARDED_FOR' => '198.51.100.200'])->assertStatus(429); @@ -277,7 +277,7 @@ describe('Manual Webhook Failed Authentication Rate Limiting', function () { expect($response->getContent())->toContain('Deployment queued'); })->with(['github', 'gitlab', 'bitbucket', 'gitea']); - test('unknown repositories respond like invalid signatures and are still throttled', function () { + test('unknown repositories get the neutral invalid signature response and are still throttled', function () { $application = createApplicationWithWebhook(); // Each delivery has a different payload. Identical redeliveries count once. @@ -285,7 +285,7 @@ describe('Manual Webhook Failed Authentication Rate Limiting', function () { $response = sendManualWebhookPush($this, 'github', $application, repository: 'unknown-org/unknown-repo', commit: sprintf('abc%04d', $i)); $response->assertOk(); - expect($response->getContent())->toContain('Invalid signature'); + expect($response->getContent())->toContain('No matching application or invalid signature.'); } sendManualWebhookPush($this, 'github', $application, repository: 'unknown-org/unknown-repo', commit: 'abc0030')->assertStatus(429); @@ -299,7 +299,7 @@ describe('Manual Webhook Failed Authentication Rate Limiting', function () { $response = sendManualWebhookPush($this, 'github', $application); $response->assertOk(); - expect($response->getContent())->not->toContain('Invalid signature'); + expect($response->getContent())->not->toContain('No matching application or invalid signature.'); } expect(RateLimiter::attempts(manualWebhookFailureKey('github')))->toBe(0); @@ -316,7 +316,7 @@ describe('Manual Webhook Failed Authentication Rate Limiting', function () { ]); $response->assertOk(); - expect($response->getContent())->toContain('Invalid signature'); + expect($response->getContent())->toContain('No matching application or invalid signature.'); } expect(RateLimiter::attempts(manualWebhookFailureKey('gitlab', '127.0.0.1')))->toBe(0); @@ -336,7 +336,7 @@ describe('Manual Webhook Failed Authentication Rate Limiting', function () { $response = sendManualWebhookPush($this, 'github', $application); $response->assertOk(); - expect($response->getContent())->not->toContain('Invalid signature'); + expect($response->getContent())->not->toContain('No matching application or invalid signature.'); }); test('failures on one provider do not block another provider', function () { @@ -348,7 +348,7 @@ describe('Manual Webhook Failed Authentication Rate Limiting', function () { $response = sendManualWebhookPush($this, $provider, $application); $response->assertOk(); - expect($response->getContent())->not->toContain('Invalid signature'); + expect($response->getContent())->not->toContain('No matching application or invalid signature.'); } }); @@ -363,7 +363,7 @@ describe('Manual Webhook Failed Authentication Rate Limiting', function () { $response = sendManualWebhookPush($this, 'github', $application, ip: '203.0.113.10'); $response->assertOk(); - expect($response->getContent())->not->toContain('Invalid signature'); + expect($response->getContent())->not->toContain('No matching application or invalid signature.'); }); }); @@ -408,7 +408,7 @@ describe('GitHub Manual Webhook HMAC', function () { ], $payload); $response->assertOk(); - expect($response->getContent())->toContain('Invalid signature'); + expect($response->getContent())->toContain('No matching application or invalid signature.'); }); test('rejects push with forged hash', function () { @@ -428,7 +428,7 @@ describe('GitHub Manual Webhook HMAC', function () { ], $payload); $response->assertOk(); - expect($response->getContent())->toContain('Invalid signature'); + expect($response->getContent())->toContain('No matching application or invalid signature.'); }); test('accepts push with valid hash', function () { @@ -452,7 +452,7 @@ describe('GitHub Manual Webhook HMAC', function () { $response->assertOk(); $content = $response->getContent(); - expect($content)->not->toContain('Invalid signature'); + expect($content)->not->toContain('No matching application or invalid signature.'); expect($content)->not->toContain('Webhook secret not configured'); }); }); @@ -507,7 +507,7 @@ describe('GitLab Manual Webhook HMAC', function () { ]); $response->assertOk(); - expect($response->getContent())->toContain('Invalid signature'); + expect($response->getContent())->toContain('No matching application or invalid signature.'); }); test('rejects push with wrong token', function () { @@ -524,7 +524,7 @@ describe('GitLab Manual Webhook HMAC', function () { ]); $response->assertOk(); - expect($response->getContent())->toContain('Invalid signature'); + expect($response->getContent())->toContain('No matching application or invalid signature.'); }); test('accepts push with valid token', function () { @@ -543,7 +543,7 @@ describe('GitLab Manual Webhook HMAC', function () { $response->assertOk(); $content = $response->getContent(); - expect($content)->not->toContain('Invalid signature'); + expect($content)->not->toContain('No matching application or invalid signature.'); expect($content)->not->toContain('Webhook secret not configured'); }); }); @@ -567,7 +567,7 @@ describe('Bitbucket Manual Webhook HMAC', function () { ], $payload); $response->assertOk(); - expect($response->getContent())->toContain('Invalid signature'); + expect($response->getContent())->toContain('No matching application or invalid signature.'); }); test('rejects push with non-sha256 algorithm', function () { @@ -586,7 +586,7 @@ describe('Bitbucket Manual Webhook HMAC', function () { ], $payload); $response->assertOk(); - expect($response->getContent())->toContain('Invalid signature'); + expect($response->getContent())->toContain('No matching application or invalid signature.'); }); test('rejects push with forged hash', function () { @@ -604,7 +604,7 @@ describe('Bitbucket Manual Webhook HMAC', function () { ], $payload); $response->assertOk(); - expect($response->getContent())->toContain('Invalid signature'); + expect($response->getContent())->toContain('No matching application or invalid signature.'); }); test('accepts push with valid sha256 hash', function () { @@ -626,7 +626,7 @@ describe('Bitbucket Manual Webhook HMAC', function () { $response->assertOk(); $content = $response->getContent(); - expect($content)->not->toContain('Invalid signature'); + expect($content)->not->toContain('No matching application or invalid signature.'); expect($content)->not->toContain('Webhook secret not configured'); }); }); @@ -652,7 +652,7 @@ describe('Gitea Manual Webhook HMAC', function () { ], $payload); $response->assertOk(); - expect($response->getContent())->toContain('Invalid signature'); + expect($response->getContent())->toContain('No matching application or invalid signature.'); }); test('rejects push with forged hash', function () { @@ -672,7 +672,7 @@ describe('Gitea Manual Webhook HMAC', function () { ], $payload); $response->assertOk(); - expect($response->getContent())->toContain('Invalid signature'); + expect($response->getContent())->toContain('No matching application or invalid signature.'); }); test('accepts push with valid hash', function () { @@ -696,7 +696,7 @@ describe('Gitea Manual Webhook HMAC', function () { $response->assertOk(); $content = $response->getContent(); - expect($content)->not->toContain('Invalid signature'); + expect($content)->not->toContain('No matching application or invalid signature.'); expect($content)->not->toContain('Webhook secret not configured'); }); }); @@ -710,7 +710,7 @@ describe('Manual Webhook HMAC in the local environment', function () { $response = sendManualWebhookPush($this, $provider, $application, validSignature: false); $response->assertOk(); - expect($response->getContent())->toContain('Invalid signature') + expect($response->getContent())->toContain('No matching application or invalid signature.') ->not->toContain('Deployment queued'); expect(ApplicationDeploymentQueue::query()->where('application_id', $application->id)->exists())->toBeFalse(); })->with(['github', 'gitea', 'bitbucket']); @@ -724,12 +724,26 @@ describe('Manual Webhook HMAC in the local environment', function () { $response->assertOk(); expect($response->getContent())->toContain('Deployment queued') - ->not->toContain('Invalid signature'); + ->not->toContain('No matching application or invalid signature.'); expect(ApplicationDeploymentQueue::query()->where('application_id', $application->id)->exists())->toBeTrue(); })->with(['github', 'gitea', 'bitbucket']); }); describe('Manual Webhook Repository Matching', function () { + test('a signed delivery without a matching application gets the same neutral response as an invalid signature', function (string $provider) { + $application = createApplicationWithWebhook(); + $neutral = json_encode([['status' => 'failed', 'message' => 'No matching application or invalid signature.']]); + + $unknownRepository = sendManualWebhookPush($this, $provider, $application, repository: 'unknown-org/unknown-repo'); + $invalidSignature = sendManualWebhookPush($this, $provider, $application, validSignature: false); + + $unknownRepository->assertOk(); + $invalidSignature->assertOk(); + expect($unknownRepository->getContent())->toBe($neutral) + ->and($invalidSignature->getContent())->toBe($neutral) + ->and(RateLimiter::attempts(manualWebhookFailureKey($provider, repository: 'unknown-org/unknown-repo')))->toBe(1); + })->with(['github', 'gitlab', 'bitbucket', 'gitea']); + test('github rejects empty repository without leaking applications', function () { $app = createApplicationWithWebhook(overrides: ['name' => 'secret-github-app']); @@ -771,7 +785,7 @@ describe('Manual Webhook Repository Matching', function () { $response->assertOk(); $content = $response->getContent(); - expect($content)->toContain('Invalid signature.') + expect($content)->toContain('No matching application or invalid signature.') ->not->toContain('secret-github-app') ->not->toContain($app->uuid); }); @@ -794,7 +808,7 @@ describe('Manual Webhook Repository Matching', function () { $response->assertOk(); $content = $response->getContent(); - expect($content)->toContain('Invalid signature') + expect($content)->toContain('No matching application or invalid signature.') ->not->toContain('secret-github-app') ->not->toContain($app->uuid) ->not->toContain('application_uuid') @@ -865,7 +879,7 @@ describe('Manual Webhook Repository Matching', function () { $response->assertOk(); $content = $response->getContent(); - expect($content)->toContain('Invalid signature.') + expect($content)->toContain('No matching application or invalid signature.') ->not->toContain("secret-{$provider}-app") ->not->toContain($app->uuid); })->with([ @@ -1316,7 +1330,7 @@ describe('Manual Webhook Repeated Failed Deliveries', function () { $response = sendManualWebhookPush($this, 'gitlab', $application, validSignature: false, wrongSecret: 'old-hook-token'); $response->assertOk(); - expect($response->getContent())->toContain('Invalid signature'); + expect($response->getContent())->toContain('No matching application or invalid signature.'); } expect(RateLimiter::attempts(manualWebhookFailureKey('gitlab')))->toBe(1); @@ -1336,7 +1350,7 @@ describe('Manual Webhook Repeated Failed Deliveries', function () { $response = sendManualWebhookPush($this, 'gitlab', $application, validSignature: false, wrongSecret: "guess-{$i}"); $response->assertOk(); - expect($response->getContent())->toContain('Invalid signature'); + expect($response->getContent())->toContain('No matching application or invalid signature.'); } sendManualWebhookPush($this, 'gitlab', $application, validSignature: false, wrongSecret: 'guess-30')->assertStatus(429); @@ -1404,7 +1418,7 @@ describe('Manual Webhook Repeated Failed Deliveries', function () { $response = sendManualWebhookPush($this, $provider, $application, validSignature: false, wrongSecret: 'old-hook-secret'); $response->assertOk(); - expect($response->getContent())->toContain('Invalid signature'); + expect($response->getContent())->toContain('No matching application or invalid signature.'); } expect(RateLimiter::attempts(manualWebhookFailureKey($provider)))->toBe(1);