diff --git a/app/Http/Controllers/Webhook/Bitbucket.php b/app/Http/Controllers/Webhook/Bitbucket.php index e84d018ea7..e06a14e0bf 100644 --- a/app/Http/Controllers/Webhook/Bitbucket.php +++ b/app/Http/Controllers/Webhook/Bitbucket.php @@ -82,9 +82,6 @@ class Bitbucket extends Controller ]); } $failure_key = $this->manualWebhookFailureRateLimitKey($request, 'bitbucket', $full_name, $branch); - if ($this->hasTooManyManualWebhookFailures($failure_key)) { - return $this->tooManyManualWebhookFailuresResponse($failure_key); - } // A redelivery of the same signed payload is one guess. $failure_attempt = $this->manualWebhookSignedPayloadAttempt($request, $x_bitbucket_token); $applications = $this->manualWebhookApplications(Application::query()->where('git_branch', $branch), $full_name); diff --git a/app/Http/Controllers/Webhook/Concerns/MatchesManualWebhookApplications.php b/app/Http/Controllers/Webhook/Concerns/MatchesManualWebhookApplications.php index b62d5e873d..5f8f20ded4 100644 --- a/app/Http/Controllers/Webhook/Concerns/MatchesManualWebhookApplications.php +++ b/app/Http/Controllers/Webhook/Concerns/MatchesManualWebhookApplications.php @@ -66,7 +66,8 @@ trait MatchesManualWebhookApplications /** * Respond to a delivery that could not be authenticated (no matching - * application or no signature) and count it as a failed attempt. + * application or no valid signature). A locked scope gets a 429 without + * counting again; otherwise the delivery is counted as a failed attempt. * * Deliveries without a matching application are counted too: the failure * key is scoped to the repository and branch, so this cannot lock out other @@ -77,12 +78,19 @@ trait MatchesManualWebhookApplications */ protected function unauthenticatedManualWebhookResponse(string $failureKey, string $attempt): Response { + if ($this->hasTooManyManualWebhookFailures($failureKey)) { + return $this->tooManyManualWebhookFailuresResponse($failureKey); + } + $this->recordManualWebhookFailure($failureKey, $attempt); return response([$this->unauthenticatedManualWebhookFailurePayload()]); } /** + * Return the authorized payloads. When no matched application authorized + * the delivery, respond as an unauthenticated delivery. + * * @param string $attempt Attempt identity from manualWebhookTokenAttempt() or manualWebhookSignedPayloadAttempt(). */ protected function manualWebhookResponse(Collection $payloads, string $failureKey, string $attempt): Response diff --git a/app/Http/Controllers/Webhook/Concerns/ThrottlesManualWebhookFailures.php b/app/Http/Controllers/Webhook/Concerns/ThrottlesManualWebhookFailures.php index 3da4cac2f4..91caabe2d1 100644 --- a/app/Http/Controllers/Webhook/Concerns/ThrottlesManualWebhookFailures.php +++ b/app/Http/Controllers/Webhook/Concerns/ThrottlesManualWebhookFailures.php @@ -11,12 +11,16 @@ use Illuminate\Support\Facades\RateLimiter; /** * Throttles manual webhook deliveries that fail authentication. * - * Failures are counted per provider, client IP, repository and branch filter. - * The repository and branch come from the payload and select the applications - * whose secrets are checked, so a guesser can only exhaust the bucket of the - * applications it targets. Git hosts deliver from shared egress IPs; one - * misconfigured repository (wrong secret, deleted application, untracked - * branch) therefore cannot lock out deliveries for other repositories. + * Failures are counted per provider, client address, repository and branch + * filter. The repository and branch come from the payload and select the + * applications whose secrets are checked, so a guesser can only exhaust the + * bucket of the applications it targets. Git hosts deliver from shared egress + * IPs; one misconfigured repository (wrong secret, deleted application, + * untracked branch) therefore cannot lock out deliveries for other repositories. + * + * The lockout applies only to deliveries that fail authentication. A delivery + * with a valid secret for a matched application is always processed, so failed + * attempts from anyone sharing the scope cannot block real deliveries. * * In one failure window, only distinct failed attempts are counted. An attempt * is identified by what the secret check depends on: the GitLab token, or the @@ -43,7 +47,23 @@ trait ThrottlesManualWebhookFailures is_scalar($branch) ? (string) $branch : null, ]); - return "manual-webhook-failures:{$provider}:".auth_rate_limit_ip($request).':'.hash('sha256', (string) $scope); + return "manual-webhook-failures:{$provider}:".$this->manualWebhookFailureClientAddress($request).':'.hash('sha256', (string) $scope); + } + + /** + * The connection address of the client. X-Forwarded-For is ignored, since + * any client can set it and rotate it to get a new bucket. On cloud, a + * valid CF-Connecting-IP is used, as in auth_rate_limit_ip(). + */ + protected function manualWebhookFailureClientAddress(Request $request): string + { + $cloudflareIp = $request->header('CF-Connecting-IP'); + + if (isCloud() && is_string($cloudflareIp) && filter_var($cloudflareIp, FILTER_VALIDATE_IP) !== false) { + return $cloudflareIp; + } + + return (string) $request->server('REMOTE_ADDR'); } /** diff --git a/app/Http/Controllers/Webhook/Gitea.php b/app/Http/Controllers/Webhook/Gitea.php index 6ef1cfa644..b3c0b22a48 100644 --- a/app/Http/Controllers/Webhook/Gitea.php +++ b/app/Http/Controllers/Webhook/Gitea.php @@ -74,9 +74,6 @@ class Gitea extends Controller } $matched_branch = $x_gitea_event === 'pull_request' ? $base_branch : $branch; $failure_key = $this->manualWebhookFailureRateLimitKey($request, 'gitea', $full_name, $matched_branch); - if ($this->hasTooManyManualWebhookFailures($failure_key)) { - return $this->tooManyManualWebhookFailuresResponse($failure_key); - } // A redelivery of the same signed payload is one guess. $failure_attempt = $this->manualWebhookSignedPayloadAttempt($request, $x_hub_signature_256); $applications = Application::query(); diff --git a/app/Http/Controllers/Webhook/Github.php b/app/Http/Controllers/Webhook/Github.php index f1d5bb8cb9..8e9e277df7 100644 --- a/app/Http/Controllers/Webhook/Github.php +++ b/app/Http/Controllers/Webhook/Github.php @@ -93,9 +93,6 @@ class Github extends Controller default => $base_branch, }; $failure_key = $this->manualWebhookFailureRateLimitKey($request, 'github', $full_name, $matched_branch); - if ($this->hasTooManyManualWebhookFailures($failure_key)) { - return $this->tooManyManualWebhookFailuresResponse($failure_key); - } // A redelivery of the same signed payload is one guess. $failure_attempt = $this->manualWebhookSignedPayloadAttempt($request, $x_hub_signature_256); $applications = Application::query(); diff --git a/app/Http/Controllers/Webhook/Gitlab.php b/app/Http/Controllers/Webhook/Gitlab.php index 62199ba6ec..c135aef328 100644 --- a/app/Http/Controllers/Webhook/Gitlab.php +++ b/app/Http/Controllers/Webhook/Gitlab.php @@ -430,9 +430,6 @@ class Gitlab extends Controller } $matched_branch = $x_gitlab_event === 'merge_request' ? $base_branch : $branch; $failure_key = $this->manualWebhookFailureRateLimitKey($request, 'gitlab', $full_name, $matched_branch); - if ($this->hasTooManyManualWebhookFailures($failure_key)) { - return $this->tooManyManualWebhookFailuresResponse($failure_key); - } // GitLab sends a static token. A repeated wrong token is one guess. $failure_attempt = $this->manualWebhookTokenAttempt($x_gitlab_token); $applications = Application::query(); diff --git a/tests/Feature/Webhook/WebhookHmacTest.php b/tests/Feature/Webhook/WebhookHmacTest.php index be5109beaf..9224e50782 100644 --- a/tests/Feature/Webhook/WebhookHmacTest.php +++ b/tests/Feature/Webhook/WebhookHmacTest.php @@ -38,10 +38,10 @@ test('manual webhook routes are not rate limited per request', function (string * An invalid delivery uses a new random wrong secret unless $wrongSecret is set, * so each invalid delivery is a new guess (a new token or a new signature). */ -function sendManualWebhookPush(TestCase $test, string $provider, Application $application, bool $validSignature = true, string $ip = '203.0.113.10', string $repository = 'test-org/test-repo', string $branch = 'main', ?string $wrongSecret = null, string $commit = 'abc1234'): TestResponse +function sendManualWebhookPush(TestCase $test, string $provider, Application $application, bool $validSignature = true, string $ip = '203.0.113.10', string $repository = 'test-org/test-repo', string $branch = 'main', ?string $wrongSecret = null, string $commit = 'abc1234', array $extraServer = []): TestResponse { $secret = $validSignature ? $application->{"manual_webhook_secret_{$provider}"} : ($wrongSecret ?? 'wrong-secret-'.Str::random(24)); - $server = ['REMOTE_ADDR' => $ip, 'CONTENT_TYPE' => 'application/json']; + $server = $extraServer + ['REMOTE_ADDR' => $ip, 'CONTENT_TYPE' => 'application/json']; if ($provider === 'gitlab') { $payload = json_encode([ @@ -137,6 +137,61 @@ describe('Manual Webhook Failed Authentication Rate Limiting', function () { expect(RateLimiter::attempts(manualWebhookFailureKey($provider)))->toBe(0); })->with(['github', 'gitlab', 'bitbucket', 'gitea']); + test('a signed delivery is processed while the scope is locked by failed attempts', function (string $provider) { + Queue::fake(); + $application = makeWebhookApplicationServerFunctional(createApplicationWithWebhook()); + + lockOutManualWebhookRepository($this, $provider, $application); + + $response = sendManualWebhookPush($this, $provider, $application); + + $response->assertOk(); + expect($response->getContent())->toContain('Deployment queued'); + expect(ApplicationDeploymentQueue::query()->where('application_id', $application->id)->exists())->toBeTrue(); + })->with(['github', 'gitlab', 'bitbucket', 'gitea']); + + test('a locked scope keeps rejecting invalid signatures without counting them again', function (string $provider) { + $application = createApplicationWithWebhook(); + + lockOutManualWebhookRepository($this, $provider, $application); + $attempts = RateLimiter::attempts(manualWebhookFailureKey($provider)); + + sendManualWebhookPush($this, $provider, $application)->assertOk(); + sendManualWebhookPush($this, $provider, $application, validSignature: false) + ->assertStatus(429) + ->assertHeader('Retry-After') + ->assertJson(['status' => 'failed', 'message' => 'Too many failed webhook authentication attempts. Try again later.']); + + expect(RateLimiter::attempts(manualWebhookFailureKey($provider)))->toBe($attempts); + })->with(['github', 'gitlab', 'bitbucket', 'gitea']); + + test('failed attempts are counted per connection address, not per forwarded address', function (string $provider) { + $application = createApplicationWithWebhook(); + + for ($i = 0; $i < 30; $i++) { + $response = sendManualWebhookPush($this, $provider, $application, validSignature: false, extraServer: ['HTTP_X_FORWARDED_FOR' => "198.51.100.{$i}"]); + + $response->assertOk(); + expect($response->getContent())->toContain('Invalid signature'); + } + + sendManualWebhookPush($this, $provider, $application, validSignature: false, extraServer: ['HTTP_X_FORWARDED_FOR' => '198.51.100.200'])->assertStatus(429); + expect(RateLimiter::tooManyAttempts(manualWebhookFailureKey($provider), 30))->toBeTrue(); + })->with(['github', 'gitlab', 'bitbucket', 'gitea']); + + test('failed attempts use the Cloudflare client address on cloud', function () { + config()->set('constants.coolify.self_hosted', false); + $application = createApplicationWithWebhook(); + + for ($i = 0; $i < 30; $i++) { + sendManualWebhookPush($this, 'github', $application, validSignature: false, extraServer: ['HTTP_CF_CONNECTING_IP' => '198.51.100.7'])->assertOk(); + } + + sendManualWebhookPush($this, 'github', $application, validSignature: false, extraServer: ['HTTP_CF_CONNECTING_IP' => '198.51.100.7'])->assertStatus(429); + // Another Cloudflare client behind the same edge address has its own bucket. + sendManualWebhookPush($this, 'github', $application, validSignature: false, extraServer: ['HTTP_CF_CONNECTING_IP' => '198.51.100.8'])->assertOk(); + }); + test('repeated invalid signatures are throttled after 30 failures', function (string $provider) { $application = createApplicationWithWebhook(); @@ -145,16 +200,13 @@ describe('Manual Webhook Failed Authentication Rate Limiting', function () { expect(RateLimiter::tooManyAttempts(manualWebhookFailureKey($provider), 30))->toBeTrue(); })->with(['github', 'gitlab', 'bitbucket', 'gitea']); - test('a locked out repository rejects every delivery for that repository and branch before verification', function (string $provider) { + test('a locked out repository rejects failed deliveries for that repository and branch', function (string $provider) { Queue::fake(); $application = makeWebhookApplicationServerFunctional(createApplicationWithWebhook()); lockOutManualWebhookRepository($this, $provider, $application); sendManualWebhookPush($this, $provider, $application, validSignature: false)->assertStatus(429); - // A correct guess during the lockout must not succeed, otherwise the - // lockout does not limit how fast a secret can be guessed. - sendManualWebhookPush($this, $provider, $application)->assertStatus(429); sendManualWebhookPush($this, $provider, $application, validSignature: false, repository: 'TEST-ORG/Test-Repo.git')->assertStatus(429); expect(ApplicationDeploymentQueue::query()->where('application_id', $application->id)->exists())->toBeFalse(); @@ -1270,11 +1322,13 @@ describe('Manual Webhook Repeated Failed Deliveries', function () { } sendManualWebhookPush($this, 'gitlab', $application, validSignature: false, wrongSecret: 'guess-30')->assertStatus(429); - sendManualWebhookPush($this, 'gitlab', $application)->assertStatus(429); // A token that was already counted is also rejected during the lockout. sendManualWebhookPush($this, 'gitlab', $application, validSignature: false, wrongSecret: 'guess-0')->assertStatus(429); expect(ApplicationDeploymentQueue::query()->where('application_id', $application->id)->exists())->toBeFalse(); + + // The correct token is still processed during the lockout. + sendManualWebhookPush($this, 'gitlab', $application)->assertOk()->assertSee('Deployment queued'); }); test('repeated tokens do not extend the guess limit', function () { @@ -1353,8 +1407,10 @@ describe('Manual Webhook Repeated Failed Deliveries', function () { } sendManualWebhookPush($this, $provider, $application, validSignature: false, wrongSecret: 'guess-30')->assertStatus(429); - sendManualWebhookPush($this, $provider, $application)->assertStatus(429); expect(ApplicationDeploymentQueue::query()->where('application_id', $application->id)->exists())->toBeFalse(); + + // A correctly signed delivery is still processed during the lockout. + sendManualWebhookPush($this, $provider, $application)->assertOk()->assertSee('Deployment queued'); })->with(['github', 'bitbucket', 'gitea']); test('the same wrong signature for different payloads counts every payload', function (string $provider) {