diff --git a/app/Http/Controllers/Webhook/Bitbucket.php b/app/Http/Controllers/Webhook/Bitbucket.php index e06a14e0bf..588ae21633 100644 --- a/app/Http/Controllers/Webhook/Bitbucket.php +++ b/app/Http/Controllers/Webhook/Bitbucket.php @@ -82,6 +82,10 @@ 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/ThrottlesManualWebhookFailures.php b/app/Http/Controllers/Webhook/Concerns/ThrottlesManualWebhookFailures.php index 91caabe2d1..b3a8421581 100644 --- a/app/Http/Controllers/Webhook/Concerns/ThrottlesManualWebhookFailures.php +++ b/app/Http/Controllers/Webhook/Concerns/ThrottlesManualWebhookFailures.php @@ -18,9 +18,9 @@ use Illuminate\Support\Facades\RateLimiter; * 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. + * After the failure limit is reached, the scope rejects all deliveries before + * reading application secrets or verifying signatures. Correctly signed + * deliveries in that scope must wait until the failure window expires. * * 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 diff --git a/app/Http/Controllers/Webhook/Gitea.php b/app/Http/Controllers/Webhook/Gitea.php index b3c0b22a48..92eddb5759 100644 --- a/app/Http/Controllers/Webhook/Gitea.php +++ b/app/Http/Controllers/Webhook/Gitea.php @@ -74,6 +74,10 @@ 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 669db96624..564bb515cd 100644 --- a/app/Http/Controllers/Webhook/Github.php +++ b/app/Http/Controllers/Webhook/Github.php @@ -95,6 +95,10 @@ 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 c135aef328..760e7cc701 100644 --- a/app/Http/Controllers/Webhook/Gitlab.php +++ b/app/Http/Controllers/Webhook/Gitlab.php @@ -430,6 +430,10 @@ 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/app/Jobs/ApplicationDeploymentJob.php b/app/Jobs/ApplicationDeploymentJob.php index 96408c1aa2..1d38b14a35 100644 --- a/app/Jobs/ApplicationDeploymentJob.php +++ b/app/Jobs/ApplicationDeploymentJob.php @@ -676,7 +676,6 @@ class ApplicationDeploymentJob implements ShouldBeEncrypted, ShouldQueue [ "docker pull {$image}", 'hidden' => true, - 'ignore_errors' => true, ], [ "docker images -q {$image} 2>/dev/null", diff --git a/app/Jobs/ProvisionGithubRunnerJob.php b/app/Jobs/ProvisionGithubRunnerJob.php index 0ce1989aec..b417635748 100644 --- a/app/Jobs/ProvisionGithubRunnerJob.php +++ b/app/Jobs/ProvisionGithubRunnerJob.php @@ -76,11 +76,24 @@ class ProvisionGithubRunnerJob implements ShouldBeEncrypted, ShouldQueue $this->waitForDocker($container, $server); } + if ($this->stopIfFinished($execution)) { + return; + } + $runner = $api->generateJitConfig($container->name(), $config->registeredLabels(), $runnerGroupId); $execution->update(['runner_id' => $runner['runner_id']]); + if ($this->stopIfFinished($execution)) { + return; + } instant_remote_process($container->startCommands($runner['encoded_jit_config']), $server); - $execution->update(['ready_at' => now()]); + if ($this->stopIfFinished($execution)) { + return; + } + GithubRunnerExecution::query() + ->whereKey($execution->id) + ->whereIn('status', GithubRunnerStatus::occupying()) + ->update(['ready_at' => now()]); GithubRunnerExecution::query() ->whereKey($execution->id) ->where('status', GithubRunnerStatus::Provisioning) @@ -96,9 +109,20 @@ class ProvisionGithubRunnerJob implements ShouldBeEncrypted, ShouldQueue */ private function handleFailure(GithubRunnerExecution $execution, \Throwable $e): void { + if ($this->stopIfFinished($execution)) { + return; + } + $attempt = $execution->provision_attempts; if ($attempt >= self::MAX_ATTEMPTS) { - $execution->finish(GithubRunnerStatus::Failed, "Could not start a runner after {$attempt} attempts: {$e->getMessage()}"); + GithubRunnerExecution::query() + ->whereKey($execution->id) + ->where('status', GithubRunnerStatus::Provisioning) + ->update([ + 'status' => GithubRunnerStatus::Failed, + 'error_message' => "Could not start a runner after {$attempt} attempts: {$e->getMessage()}", + 'completed_at' => now(), + ]); CleanupGithubRunnerJob::dispatch($execution->id); return; @@ -106,18 +130,36 @@ class ProvisionGithubRunnerJob implements ShouldBeEncrypted, ShouldQueue // The retry keeps the execution uuid, so its Docker resources must be gone before it starts again. (new CleanupGithubRunnerJob($execution->id))->handle(); - $execution->update([ - 'status' => GithubRunnerStatus::Queued, - 'github_runner_config_id' => null, - 'server_id' => null, - 'runner_name' => null, - 'runner_id' => null, - 'ready_at' => null, - 'error_message' => "Attempt {$attempt} of ".self::MAX_ATTEMPTS." failed: {$e->getMessage()}", - ]); + $retried = GithubRunnerExecution::query() + ->whereKey($execution->id) + ->where('status', GithubRunnerStatus::Provisioning) + ->update([ + 'status' => GithubRunnerStatus::Queued, + 'github_runner_config_id' => null, + 'server_id' => null, + 'runner_name' => null, + 'runner_id' => null, + 'ready_at' => null, + 'error_message' => "Attempt {$attempt} of ".self::MAX_ATTEMPTS." failed: {$e->getMessage()}", + ]); + if (! $retried) { + return; + } self::dispatch($execution->id)->delay(now()->addSeconds(self::RETRY_DELAY_SECONDS * $attempt)); } + private function stopIfFinished(GithubRunnerExecution $execution): bool + { + $execution->refresh(); + if ($execution->isActive()) { + return false; + } + + CleanupGithubRunnerJob::dispatch($execution->id); + + return true; + } + /** * Picks the least busy matching server and moves the execution to "provisioning". * Row locks on the execution and on the App's configs stop parallel jobs from overbooking a server. diff --git a/app/Jobs/VolumeCloneJob.php b/app/Jobs/VolumeCloneJob.php index 86ddcabe86..677a01a35b 100644 --- a/app/Jobs/VolumeCloneJob.php +++ b/app/Jobs/VolumeCloneJob.php @@ -55,10 +55,13 @@ class VolumeCloneJob implements ShouldBeEncrypted, ShouldQueue } $sourceConfiguration = $this->sourceVolumeConfiguration(); - $createTargetVolume = $this->createTargetVolumeCommand($sourceConfiguration); + $isBindVolume = $this->isBindVolume($sourceConfiguration); + $targetConfiguration = $isBindVolume ? ['driver' => 'local', 'options' => []] : $sourceConfiguration; + $createTargetVolume = $this->createTargetVolumeCommand($targetConfiguration); - if ($this->isBindVolume($sourceConfiguration)) { + if ($isBindVolume) { instant_remote_process([$createTargetVolume], $this->sourceServer); + $this->rememberTargetVolumeWithoutDriverOptions($targetConfiguration); $this->logSkippedBindVolumeCopy($sourceConfiguration); return; @@ -76,10 +79,13 @@ class VolumeCloneJob implements ShouldBeEncrypted, ShouldQueue $srcVol = escapeshellarg($this->sourceVolume); $tgtVol = escapeshellarg($this->targetVolume); $sourceConfiguration = $this->sourceVolumeConfiguration(); - $createTargetVolume = $this->createTargetVolumeCommand($sourceConfiguration); + $isBindVolume = $this->isBindVolume($sourceConfiguration); + $targetConfiguration = $isBindVolume ? ['driver' => 'local', 'options' => []] : $sourceConfiguration; + $createTargetVolume = $this->createTargetVolumeCommand($targetConfiguration); - if ($this->isBindVolume($sourceConfiguration)) { + if ($isBindVolume) { instant_remote_process([$createTargetVolume], $this->targetServer); + $this->rememberTargetVolumeWithoutDriverOptions($targetConfiguration); $this->logSkippedBindVolumeCopy($sourceConfiguration); return; @@ -164,8 +170,8 @@ class VolumeCloneJob implements ShouldBeEncrypted, ShouldQueue } /** - * Creates the target volume like the source volume. The Compose file of the copy declares the same - * `driver` and `driver_opts`, and Docker Compose does not change the options of an existing volume. + * Creates the target volume with its selected driver and options. A bind clone uses a plain local + * volume and a name-only Compose declaration, so it never mounts the source host directory. * One `sh -c` line, so the non-root sudo parser only puts sudo in front of it and never changes * an option value. * @@ -187,8 +193,8 @@ class VolumeCloneJob implements ShouldBeEncrypted, ShouldQueue /** * A `local` volume with `type: none` and `o: bind` is a host folder. Copying the data would write - * into that folder (on the same server, the folder of the source volume itself), so the job only - * creates the volume. + * into that folder (on the same server, the folder of the source volume itself), so the clone gets + * an empty Docker-managed volume without copying the host folder. * * @param array{driver: string, options: array} $configuration */ @@ -205,7 +211,7 @@ class VolumeCloneJob implements ShouldBeEncrypted, ShouldQueue protected function logSkippedBindVolumeCopy(array $configuration): void { $device = $configuration['options']['device'] ?? 'unknown'; - \Log::info("Volume {$this->sourceVolume} is a bind mount of the host folder {$device}. Created {$this->targetVolume} with the same options and did not copy the data."); + \Log::info("Volume {$this->sourceVolume} is a bind mount of the host folder {$device}. Created independent empty volume {$this->targetVolume} and did not copy the data."); } /** diff --git a/tests/Feature/GithubRunnerFlowTest.php b/tests/Feature/GithubRunnerFlowTest.php index 59f3d39fe1..fe28fc2f55 100644 --- a/tests/Feature/GithubRunnerFlowTest.php +++ b/tests/Feature/GithubRunnerFlowTest.php @@ -418,6 +418,101 @@ describe('provisioning', function () { Queue::assertNotPushed(CleanupGithubRunnerJob::class); }); + it('preserves a terminal execution when provisioning fails', function (GithubRunnerStatus $status, int $attempts) { + Queue::fake(); + Process::fake(['*' => Process::result(output: '29.8.0')]); + runnerTestConfig(runnerTestServer($this->team), $this->githubApp); + $execution = runnerTestExecution($this->githubApp, ['provision_attempts' => $attempts]); + Http::fake([ + 'https://api.github.com/zen' => Http::response('ok', 200, ['Date' => now()->toRfc7231String()]), + 'https://api.github.com/app/installations/*' => Http::response(['token' => 'installation-token'], 201), + 'https://api.github.com/orgs/acme/actions/runners/42' => Http::response(null, 204), + 'https://api.github.com/orgs/acme/actions/runners/generate-jitconfig' => function () use ($execution, $status) { + $execution->finish($status, 'Stopped externally'); + + return Http::response(['message' => 'Resource not accessible by integration'], 403); + }, + ]); + + (new ProvisionGithubRunnerJob($execution->id))->handle(); + + expect($execution->fresh()->status)->toBe($status) + ->and($execution->fresh()->error_message)->toBe('Stopped externally'); + Queue::assertNotPushed(ProvisionGithubRunnerJob::class, fn ($job) => $job->executionId === $execution->id); + })->with([ + 'cancelled during first attempt' => [GithubRunnerStatus::Cancelled, 0], + 'cancelled during last attempt' => [GithubRunnerStatus::Cancelled, 2], + 'timed out' => [GithubRunnerStatus::TimedOut, 0], + 'completed' => [GithubRunnerStatus::Completed, 0], + ]); + + it('does not start a runner cancelled while GitHub registers it', function () { + Queue::fake(); + Process::fake(['*' => Process::result(output: '29.8.0')]); + runnerTestConfig(runnerTestServer($this->team), $this->githubApp); + $execution = runnerTestExecution($this->githubApp); + Http::fake([ + 'https://api.github.com/zen' => Http::response('ok', 200, ['Date' => now()->toRfc7231String()]), + 'https://api.github.com/app/installations/*' => Http::response(['token' => 'installation-token'], 201), + 'https://api.github.com/orgs/acme/actions/runners/42' => Http::response(null, 204), + 'https://api.github.com/orgs/acme/actions/runners/generate-jitconfig' => function () use ($execution) { + $execution->finish(GithubRunnerStatus::Cancelled); + + return Http::response(['runner' => ['id' => 42], 'encoded_jit_config' => 'SECRET-JIT-CONFIG'], 201); + }, + ]); + + (new ProvisionGithubRunnerJob($execution->id))->handle(); + + expect($execution->fresh()->status)->toBe(GithubRunnerStatus::Cancelled) + ->and($execution->fresh()->ready_at)->toBeNull(); + Process::assertDidntRun(fn (PendingProcess $process) => str_contains($process->command, '/home/runner/run.sh')); + Queue::assertPushed(CleanupGithubRunnerJob::class, fn ($job) => $job->executionId === $execution->id); + }); + + it('cleans up cancellation during remote provisioning without marking the runner ready', function (bool $duringStart) { + Queue::fake(); + runnerTestConfig(runnerTestServer($this->team), $this->githubApp); + $execution = runnerTestExecution($this->githubApp); + fakeRunnerGithubApi($this->githubApp); + Process::fake(['*' => function (PendingProcess $process) use ($execution, $duringStart) { + if (! $duringStart || str_contains($process->command, '/home/runner/run.sh')) { + $execution->finish(GithubRunnerStatus::Cancelled); + } + + return Process::result(output: '29.8.0'); + }]); + + (new ProvisionGithubRunnerJob($execution->id))->handle(); + + expect($execution->fresh()->status)->toBe(GithubRunnerStatus::Cancelled) + ->and($execution->fresh()->ready_at)->toBeNull(); + Queue::assertPushed(CleanupGithubRunnerJob::class, fn ($job) => $job->executionId === $execution->id); + if (! $duringStart) { + Http::assertNotSent(fn ($request) => str_ends_with($request->url(), '/generate-jitconfig')); + } + })->with(['during preparation' => false, 'during start' => true]); + + it('does not retry an execution cancelled while failed resources are removed', function () { + Queue::fake(); + runnerTestConfig(runnerTestServer($this->team), $this->githubApp); + $execution = runnerTestExecution($this->githubApp); + fakeRejectedJitConfig(); + Process::fake(['*' => function (PendingProcess $process) use ($execution) { + if (str_contains($process->command, 'docker rm -f -v')) { + $execution->finish(GithubRunnerStatus::Cancelled, 'Stopped externally'); + } + + return Process::result(output: '29.8.0'); + }]); + + (new ProvisionGithubRunnerJob($execution->id))->handle(); + + expect($execution->fresh()->status)->toBe(GithubRunnerStatus::Cancelled) + ->and($execution->fresh()->error_message)->toBe('Stopped externally'); + Queue::assertNotPushed(ProvisionGithubRunnerJob::class, fn ($job) => $job->executionId === $execution->id); + }); + it('marks the execution failed and cleans up after the last attempt', function () { Queue::fake(); Process::fake(['*' => Process::result(output: '29.8.0')]); diff --git a/tests/Feature/MultiServerApplicationTest.php b/tests/Feature/MultiServerApplicationTest.php index 260c5747d7..4e7214ffca 100644 --- a/tests/Feature/MultiServerApplicationTest.php +++ b/tests/Feature/MultiServerApplicationTest.php @@ -340,10 +340,16 @@ class MultiServerRecordingDeploymentJob extends ApplicationDeploymentJob /** Output that `docker images -q` returns; empty means the image is missing. */ public string $localImageOutput = 'sha256:0123456789ab'; + public bool $failImagePull = false; + public function execute_remote_command(...$commands): void { foreach ($commands as $command) { - $this->recordedCommands[] = $command['command'] ?? $command[0]; + $remoteCommand = $command['command'] ?? $command[0]; + $this->recordedCommands[] = $remoteCommand; + if ($this->failImagePull && str_starts_with($remoteCommand, 'docker pull ') && ! ($command['ignore_errors'] ?? false)) { + throw new RuntimeException('Registry image pull failed.'); + } if (isset($command['save'])) { (new ReflectionProperty(ApplicationDeploymentJob::class, 'saved_outputs'))->getValue($this)->put($command['save'], $this->localImageOutput); } @@ -390,6 +396,18 @@ describe('inline Dockerfile image on additional servers', function () { ]); }); + test('an additional server fails when pulling the image fails even if an old image is cached', function () { + [$job, $reflection] = makeInlineDockerfileJob($this->application->fresh(), true); + $job->failImagePull = true; + $job->localImageOutput = 'sha256:stale-cached-image'; + + expect(fn () => $reflection->getMethod('pull_image_for_additional_server')->invoke($job)) + ->toThrow(RuntimeException::class, 'Registry image pull failed.'); + expect($job->recordedCommands)->toBe([ + "docker pull 'registry.example.com/nginx-multi:latest'", + ]); + }); + test('a deployments only server explains that the main server must push a missing image', function () { $this->extraServer->settings->update(['server_role' => 'deployment']); [$job, $reflection] = makeInlineDockerfileJob($this->application->fresh(), true, $this->extraServer->fresh()); diff --git a/tests/Feature/VolumeCloneJobTest.php b/tests/Feature/VolumeCloneJobTest.php index e46eb24c92..be0ba9c1e9 100644 --- a/tests/Feature/VolumeCloneJobTest.php +++ b/tests/Feature/VolumeCloneJobTest.php @@ -9,10 +9,13 @@ use App\Models\Server; use App\Models\Team; use Illuminate\Foundation\Testing\RefreshDatabase; use Illuminate\Support\Facades\Process; +use Illuminate\Support\Facades\Storage; uses(RefreshDatabase::class); beforeEach(function () { + Storage::fake('ssh-keys'); + Storage::fake('ssh-mux'); Server::flushIdentityMap(); InstanceSettings::forceCreate(['id' => 0]); config(['app.maintenance.store' => 'array', 'constants.ssh.mux_enabled' => false]); @@ -114,7 +117,7 @@ test('a missing source volume still creates an empty target volume', function () expect(volumeCloneCommands())->toContain(escapeshellarg("docker volume create 'clone-target'")); }); -test('a bind-type source volume is created with its options but its host folder is not copied', function () { +test('a bind-type source volume gets independent empty storage without copying its host folder', function () { fakeVolumeCloneServer([ 'Name' => 'clone-source', 'Driver' => 'local', @@ -124,7 +127,9 @@ test('a bind-type source volume is created with its options but its host folder (new VolumeCloneJob('clone-source', 'clone-target', $this->server, $this->server, $this->volume))->handle(); $commands = volumeCloneCommands(); - expect($commands)->toContain(escapeshellarg("docker volume create --opt 'type=none' --opt 'o=bind' --opt 'device=/srv/app-data' 'clone-target'")) + expect($commands)->toContain(escapeshellarg("docker volume create 'clone-target'")) + ->and($this->volume->fresh()->ignores_compose_driver_options)->toBeTrue() + ->and(composeRenamedVolumeDeclarationFor(['driver' => 'local', 'driver_opts' => ['type' => 'none', 'o' => 'bind', 'device' => '/srv/app-data']], 'clone-target', $this->volume->fresh()))->toBe(['name' => 'clone-target']) ->and($commands)->not->toContain('cp -a') ->and($commands)->not->toContain('chown -R'); }); @@ -148,7 +153,7 @@ test('a remote clone creates the target volume with the source driver options on ->and($commands)->toContain('tar xzf /clone/volume-data.tar.gz'); }); -test('a remote clone of a bind-type volume does not copy the host folder', function () { +test('a remote bind-type clone gets independent empty storage without copying the host folder', function () { $targetServer = Server::factory()->create([ 'team_id' => $this->team->id, 'user' => 'root', @@ -163,7 +168,8 @@ test('a remote clone of a bind-type volume does not copy the host folder', funct (new VolumeCloneJob('clone-source', 'clone-target', $this->server, $targetServer, $this->volume))->handle(); $commands = volumeCloneCommands(); - expect($commands)->toContain(escapeshellarg("docker volume create --opt 'type=none' --opt 'o=bind,rw' --opt 'device=/srv/app-data' 'clone-target'")) + expect($commands)->toContain(escapeshellarg("docker volume create 'clone-target'")) + ->and($this->volume->fresh()->ignores_compose_driver_options)->toBeTrue() ->and($commands)->not->toContain('tar czf') ->and($commands)->not->toContain('tar xzf'); }); diff --git a/tests/Feature/Webhook/WebhookHmacTest.php b/tests/Feature/Webhook/WebhookHmacTest.php index 9224e50782..4b924b5f35 100644 --- a/tests/Feature/Webhook/WebhookHmacTest.php +++ b/tests/Feature/Webhook/WebhookHmacTest.php @@ -124,7 +124,26 @@ function makeWebhookApplicationServerFunctional(Application $application): Appli } describe('Manual Webhook Failed Authentication Rate Limiting', function () { - test('valid signed deliveries are never throttled', function (string $provider) { + test('a locked failure scope rejects deliveries before reading application secrets', function (string $provider, bool $validSignature) { + $application = createApplicationWithWebhook(); + + lockOutManualWebhookRepository($this, $provider, $application); + DB::enableQueryLog(); + DB::flushQueryLog(); + + sendManualWebhookPush($this, $provider, $application, validSignature: $validSignature) + ->assertStatus(429) + ->assertHeader('Retry-After'); + + $applicationQueries = collect(DB::getQueryLog()) + ->filter(fn (array $query): bool => str_contains($query['query'], '"applications"')); + DB::disableQueryLog(); + + expect($applicationQueries)->toBeEmpty(); + expect(ApplicationDeploymentQueue::query()->where('application_id', $application->id)->exists())->toBeFalse(); + })->with(['github', 'gitlab', 'bitbucket', 'gitea'])->with([false, true]); + + test('valid signed deliveries do not consume the failure limit', function (string $provider) { $application = createApplicationWithWebhook(); for ($i = 0; $i < 80; $i++) { @@ -137,12 +156,19 @@ 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) { + test('a signed delivery is blocked during lockout and processed after the failure window expires', function (string $provider) { Queue::fake(); $application = makeWebhookApplicationServerFunctional(createApplicationWithWebhook()); lockOutManualWebhookRepository($this, $provider, $application); + sendManualWebhookPush($this, $provider, $application) + ->assertStatus(429) + ->assertHeader('Retry-After'); + expect(ApplicationDeploymentQueue::query()->where('application_id', $application->id)->exists())->toBeFalse(); + + $this->travel(61)->seconds(); + $response = sendManualWebhookPush($this, $provider, $application); $response->assertOk(); @@ -156,7 +182,7 @@ describe('Manual Webhook Failed Authentication Rate Limiting', function () { lockOutManualWebhookRepository($this, $provider, $application); $attempts = RateLimiter::attempts(manualWebhookFailureKey($provider)); - sendManualWebhookPush($this, $provider, $application)->assertOk(); + sendManualWebhookPush($this, $provider, $application)->assertStatus(429); sendManualWebhookPush($this, $provider, $application, validSignature: false) ->assertStatus(429) ->assertHeader('Retry-After') @@ -1327,8 +1353,8 @@ describe('Manual Webhook Repeated Failed Deliveries', function () { 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'); + sendManualWebhookPush($this, 'gitlab', $application)->assertStatus(429); + expect(ApplicationDeploymentQueue::query()->where('application_id', $application->id)->exists())->toBeFalse(); }); test('repeated tokens do not extend the guess limit', function () { @@ -1409,8 +1435,8 @@ describe('Manual Webhook Repeated Failed Deliveries', function () { sendManualWebhookPush($this, $provider, $application, validSignature: false, wrongSecret: 'guess-30')->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'); + sendManualWebhookPush($this, $provider, $application)->assertStatus(429); + expect(ApplicationDeploymentQueue::query()->where('application_id', $application->id)->exists())->toBeFalse(); })->with(['github', 'bitbucket', 'gitea']); test('the same wrong signature for different payloads counts every payload', function (string $provider) {