fix(webhooks): never lock out manual webhook deliveries with a valid signature

The failed-authentication lockout for manual webhooks was checked before
the signature, and the bucket used the client IP from X-Forwarded-For.
An unauthenticated client could send failed attempts with the Git host's
address for a public repository and branch, and correctly signed
deliveries then got 429, so deployments were lost.

The lockout now applies only to deliveries that fail authentication; a
delivery with a valid secret for a matched application is always
processed. Failures are counted per connection address (REMOTE_ADDR, or
CF-Connecting-IP on cloud), so rotating X-Forwarded-For no longer gives
a new bucket.

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);

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

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');
     }
 
     /**

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();

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();

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();

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) {