Harden notification release regressions

2026-04-24 18:44:25 +02:00
parent 4908064f6d
commit c052aeaa03
4 changed files with 286 additions and 20 deletions
@@ -78,26 +78,26 @@ class DeliverNotificationAction implements ShouldQueue
                NotificationChannel::Webhook => $this->sendWebhookRequest($notificationAction),
            };
        } catch (ConnectionException $exception) {
-            $this->handleConnectionFailure($deliveryLog, $notificationAction->channel, $attemptCount, $attemptedAt, $exception);
+            $this->handleConnectionFailure($deliveryLog, $notificationAction, $attemptCount, $attemptedAt, $exception);

            return;
        }

        if ($response->successful()) {
-            $this->markSucceeded($deliveryLog, $response, $attemptCount, $attemptedAt);
+            $this->markSucceeded($deliveryLog, $notificationAction, $response, $attemptCount, $attemptedAt);

            return;
        }

        if ($this->shouldRetry($response) && $this->hasAttemptsRemaining($attemptCount)) {
-            $this->markRetryable($deliveryLog, $notificationAction->channel, $response, $attemptCount, $attemptedAt);
+            $this->markRetryable($deliveryLog, $notificationAction, $response, $attemptCount, $attemptedAt);
            $this->release($this->backoffDelay($attemptCount));

            return;
        }

-        $this->markFailed($deliveryLog, $notificationAction->channel, $response, $attemptCount, $attemptedAt);
-        $this->fail(new \RuntimeException($this->responseFailureDescription($notificationAction->channel, $response)));
+        $this->markFailed($deliveryLog, $notificationAction, $response, $attemptCount, $attemptedAt);
+        $this->fail(new \RuntimeException($this->responseFailureDescription($notificationAction, $response)));
    }

    public function failed(?Throwable $exception): void
@@ -108,23 +108,29 @@ class DeliverNotificationAction implements ShouldQueue
            return;
        }

+        $notificationAction = NotificationAction::query()->find($this->notificationActionId);
+
        $deliveryLog->update([
            'attempt_count' => max($deliveryLog->attempt_count, $this->attempts()),
            'delivery_status' => NotificationDeliveryStatus::Failed,
            'delivered_at' => null,
-            'error_excerpt' => $deliveryLog->error_excerpt ?? $this->excerpt($exception?->getMessage()),
+            'error_excerpt' => $deliveryLog->error_excerpt ?? $this->sanitizedExcerpt(
+                $exception?->getMessage(),
+                $notificationAction,
+            ),
            'last_attempted_at' => $deliveryLog->last_attempted_at ?? CarbonImmutable::now(),
        ]);
    }

    private function handleConnectionFailure(
        NotificationDeliveryLog $deliveryLog,
-        NotificationChannel $channel,
+        NotificationAction $notificationAction,
        int $attemptCount,
        CarbonImmutable $attemptedAt,
        ConnectionException $exception,
    ): void {
-        $errorExcerpt = $this->excerpt($exception->getMessage()) ?? $this->requestFailureDescription($channel);
+        $errorExcerpt = $this->sanitizedExcerpt($exception->getMessage(), $notificationAction)
+            ?? $this->requestFailureDescription($notificationAction->channel);

        if ($this->hasAttemptsRemaining($attemptCount)) {
            $deliveryLog->update([
@@ -157,6 +163,7 @@ class DeliverNotificationAction implements ShouldQueue

    private function markSucceeded(
        NotificationDeliveryLog $deliveryLog,
+        NotificationAction $notificationAction,
        Response $response,
        int $attemptCount,
        CarbonImmutable $attemptedAt,
@@ -168,13 +175,13 @@ class DeliverNotificationAction implements ShouldQueue
            'error_excerpt' => null,
            'http_status' => $response->status(),
            'last_attempted_at' => $attemptedAt,
-            'response_excerpt' => $this->responseExcerpt($response),
+            'response_excerpt' => $this->responseExcerpt($response, $notificationAction),
        ]);
    }

    private function markRetryable(
        NotificationDeliveryLog $deliveryLog,
-        NotificationChannel $channel,
+        NotificationAction $notificationAction,
        Response $response,
        int $attemptCount,
        CarbonImmutable $attemptedAt,
@@ -183,16 +190,16 @@ class DeliverNotificationAction implements ShouldQueue
            'attempt_count' => $attemptCount,
            'delivery_status' => NotificationDeliveryStatus::Retryable,
            'delivered_at' => null,
-            'error_excerpt' => $this->responseFailureDescription($channel, $response),
+            'error_excerpt' => $this->responseFailureDescription($notificationAction, $response),
            'http_status' => $response->status(),
            'last_attempted_at' => $attemptedAt,
-            'response_excerpt' => $this->responseExcerpt($response),
+            'response_excerpt' => $this->responseExcerpt($response, $notificationAction),
        ]);
    }

    private function markFailed(
        NotificationDeliveryLog $deliveryLog,
-        NotificationChannel $channel,
+        NotificationAction $notificationAction,
        Response $response,
        int $attemptCount,
        CarbonImmutable $attemptedAt,
@@ -201,10 +208,10 @@ class DeliverNotificationAction implements ShouldQueue
            'attempt_count' => $attemptCount,
            'delivery_status' => NotificationDeliveryStatus::Failed,
            'delivered_at' => null,
-            'error_excerpt' => $this->responseFailureDescription($channel, $response),
+            'error_excerpt' => $this->responseFailureDescription($notificationAction, $response),
            'http_status' => $response->status(),
            'last_attempted_at' => $attemptedAt,
-            'response_excerpt' => $this->responseExcerpt($response),
+            'response_excerpt' => $this->responseExcerpt($response, $notificationAction),
        ]);
    }

@@ -297,15 +304,16 @@ class DeliverNotificationAction implements ShouldQueue
        return $backoff[$index];
    }

-    private function responseExcerpt(Response $response): ?string
+    private function responseExcerpt(Response $response, NotificationAction $notificationAction): ?string
    {
-        return $this->excerpt($response->body()) ?? $this->excerpt($response->reason());
+        return $this->sanitizedExcerpt($response->body(), $notificationAction)
+            ?? $this->sanitizedExcerpt($response->reason(), $notificationAction);
    }

-    private function responseFailureDescription(NotificationChannel $channel, Response $response): string
+    private function responseFailureDescription(NotificationAction $notificationAction, Response $response): string
    {
-        $reason = $this->excerpt($response->reason());
-        $subject = $this->deliverySubject($channel);
+        $reason = $this->sanitizedExcerpt($response->reason(), $notificationAction);
+        $subject = $this->deliverySubject($notificationAction->channel);

        if ($reason === null) {
            return "{$subject} returned HTTP {$response->status()}.";
@@ -364,4 +372,24 @@ class DeliverNotificationAction implements ShouldQueue

        return $excerpt === '' ? null : $excerpt;
    }
+
+    private function sanitizedExcerpt(?string $value, ?NotificationAction $notificationAction): ?string
+    {
+        if (
+            $value !== null
+            && $notificationAction !== null
+            && filled($notificationAction->delivery_url)
+        ) {
+            $value = str_replace(
+                [
+                    $notificationAction->delivery_url,
+                    str_replace('/', '\/', $notificationAction->delivery_url),
+                ],
+                '[redacted]',
+                $value,
+            );
+        }
+
+        return $this->excerpt($value);
+    }
 }
@@ -3,8 +3,11 @@
 namespace Tests\Feature\Api;

 use App\Actions\Git\GitProviderKey;
+use App\Actions\Notifications\DeliverNotificationAction;
 use App\Actions\ProjectTicketMetadata\InitializeProjectTicketMetadata;
+use App\Enums\NotificationDeliveryStatus;
 use App\Models\BoardColumn;
+use App\Models\NotificationAction;
 use App\Models\Project;
 use App\Models\RepositoryConnection;
 use App\Models\Ticket;
@@ -14,6 +17,7 @@ use App\Models\User;
 use App\Models\UserGitConnection;
 use Illuminate\Foundation\Testing\RefreshDatabase;
 use Illuminate\Support\Facades\Http;
+use Illuminate\Support\Facades\Queue;
 use Tests\TestCase;

 class TicketApiTest extends TestCase
@@ -122,6 +126,80 @@ class TicketApiTest extends TestCase
        ]);
    }

+    public function test_status_moves_still_queue_notification_deliveries_over_the_api(): void
+    {
+        Queue::fake();
+
+        [$user, $project] = $this->projectContext();
+
+        $backlogStatus = $project->ticketStatuses()->firstOrFail();
+        $boardColumn = BoardColumn::factory()->for($project)->create([
+            'name' => 'Doing',
+            'position' => 1,
+        ]);
+        $doingStatus = TicketStatus::factory()
+            ->for($project)
+            ->for($boardColumn)
+            ->create([
+                'name' => 'Doing',
+                'position' => 1,
+            ]);
+        $backlogStatus->allowedTargetStatuses()->sync([$doingStatus->id]);
+
+        $ticket = $this->ticket($project, $backlogStatus);
+        $notificationAction = NotificationAction::factory()
+            ->webhook()
+            ->for($project)
+            ->create();
+
+        $this->withApplicationKey($user)
+            ->patchJson(route('api.projects.tickets.status.update', [$project, $ticket]), [
+                'ticket_status_id' => $doingStatus->id,
+            ])
+            ->assertOk()
+            ->assertJsonPath('data.ticket_status_id', $doingStatus->id)
+            ->assertJsonPath('meta.message', 'CORE-1 moved to Doing.');
+
+        Queue::assertPushed(DeliverNotificationAction::class, function (DeliverNotificationAction $job) use ($notificationAction): bool {
+            return $job->notificationActionId === $notificationAction->id
+                && $job->payload->ticketKey === 'CORE-1'
+                && $job->payload->toStatusLabel === 'Doing'
+                && $job->notificationDeliveryLogId !== null;
+        });
+
+        $this->assertDatabaseHas('notification_delivery_logs', [
+            'notification_action_id' => $notificationAction->id,
+            'delivery_status' => NotificationDeliveryStatus::Pending->value,
+            'attempt_count' => 0,
+        ]);
+    }
+
+    public function test_no_op_status_moves_keep_the_existing_api_response_and_do_not_queue_notifications(): void
+    {
+        Queue::fake();
+
+        [$user, $project] = $this->projectContext();
+
+        $backlogStatus = $project->ticketStatuses()->firstOrFail();
+        $ticket = $this->ticket($project, $backlogStatus);
+
+        NotificationAction::factory()
+            ->webhook()
+            ->for($project)
+            ->create();
+
+        $this->withApplicationKey($user)
+            ->patchJson(route('api.projects.tickets.status.update', [$project, $ticket]), [
+                'ticket_status_id' => $backlogStatus->id,
+            ])
+            ->assertOk()
+            ->assertJsonPath('data.ticket_status_id', $backlogStatus->id)
+            ->assertJsonPath('meta.message', 'CORE-1 already belongs to Backlog.');
+
+        Queue::assertNothingPushed();
+        $this->assertDatabaseCount('ticket_worklog_entries', 0);
+    }
+
    public function test_ticket_links_and_branch_endpoints_are_available(): void
    {
        Http::preventStrayRequests();
@@ -2,7 +2,9 @@

 namespace Tests\Feature\Mcp;

+use App\Actions\Notifications\DeliverNotificationAction;
 use App\Actions\ProjectTicketMetadata\InitializeProjectTicketMetadata;
+use App\Enums\NotificationDeliveryStatus;
 use App\Mcp\Servers\KanbanGitServer;
 use App\Mcp\Tools\GetTicket;
 use App\Mcp\Tools\ListMyAssignedTickets;
@@ -11,11 +13,13 @@ use App\Mcp\Tools\MoveTicketStatus;
 use App\Mcp\Tools\UpdateTicketAssignee;
 use App\Mcp\Tools\UpdateTicketDescription;
 use App\Models\BoardColumn;
+use App\Models\NotificationAction;
 use App\Models\Project;
 use App\Models\Ticket;
 use App\Models\TicketStatus;
 use App\Models\User;
 use Illuminate\Foundation\Testing\RefreshDatabase;
+use Illuminate\Support\Facades\Queue;
 use Illuminate\Testing\Fluent\AssertableJson;
 use Tests\TestCase;

@@ -319,6 +323,90 @@ class KanbanGitMcpTest extends TestCase
                ->etc());
    }

+    public function test_mcp_status_moves_still_queue_notification_deliveries_when_actions_are_enabled(): void
+    {
+        Queue::fake();
+
+        $user = User::factory()->create();
+        $project = $this->project('CORE', 'Core project');
+        $backlogStatus = $project->ticketStatuses()->firstOrFail();
+        $doingStatus = $this->ticketStatus($project, 'Doing');
+        $ticket = $this->ticket($project, $backlogStatus, [
+            'ticket_key' => 'CORE-1',
+            'title' => 'Move ticket',
+        ]);
+
+        $backlogStatus->allowedTargetStatuses()->sync([$doingStatus->id]);
+        $this->syncProjectPermissions($project, $user, ['write']);
+
+        $notificationAction = NotificationAction::factory()
+            ->webhook()
+            ->for($project)
+            ->create();
+
+        KanbanGitServer::actingAs($user)
+            ->tool(MoveTicketStatus::class, [
+                'ticket_id' => $ticket->id,
+                'ticket_status_id' => $doingStatus->id,
+            ])
+            ->assertOk()
+            ->assertStructuredContent(fn (AssertableJson $json) => $json
+                ->where('data.ticket.ticket_status_id', $doingStatus->id)
+                ->where('meta.destination_label', 'Doing')
+                ->where('meta.message', 'CORE-1 moved to Doing.')
+                ->where('meta.moved', true)
+                ->etc());
+
+        Queue::assertPushed(DeliverNotificationAction::class, function (DeliverNotificationAction $job) use ($notificationAction): bool {
+            return $job->notificationActionId === $notificationAction->id
+                && $job->payload->ticketKey === 'CORE-1'
+                && $job->payload->toStatusLabel === 'Doing'
+                && $job->notificationDeliveryLogId !== null;
+        });
+
+        $this->assertDatabaseHas('notification_delivery_logs', [
+            'notification_action_id' => $notificationAction->id,
+            'delivery_status' => NotificationDeliveryStatus::Pending->value,
+            'attempt_count' => 0,
+        ]);
+    }
+
+    public function test_mcp_no_op_status_moves_do_not_queue_notification_deliveries(): void
+    {
+        Queue::fake();
+
+        $user = User::factory()->create();
+        $project = $this->project('CORE', 'Core project');
+        $backlogStatus = $project->ticketStatuses()->firstOrFail();
+        $ticket = $this->ticket($project, $backlogStatus, [
+            'ticket_key' => 'CORE-1',
+            'title' => 'Move ticket',
+        ]);
+
+        $this->syncProjectPermissions($project, $user, ['write']);
+
+        NotificationAction::factory()
+            ->webhook()
+            ->for($project)
+            ->create();
+
+        KanbanGitServer::actingAs($user)
+            ->tool(MoveTicketStatus::class, [
+                'ticket_id' => $ticket->id,
+                'ticket_status_id' => $backlogStatus->id,
+            ])
+            ->assertOk()
+            ->assertStructuredContent(fn (AssertableJson $json) => $json
+                ->where('data.ticket.ticket_status_id', $backlogStatus->id)
+                ->where('meta.destination_label', 'Backlog')
+                ->where('meta.message', 'CORE-1 already belongs to Backlog.')
+                ->where('meta.moved', false)
+                ->etc());
+
+        Queue::assertNothingPushed();
+        $this->assertDatabaseCount('ticket_worklog_entries', 0);
+    }
+
    public function test_ambiguous_status_name_returns_mcp_error(): void
    {
        $user = User::factory()->create();
@@ -287,6 +287,78 @@ class DeliverNotificationActionTest extends TestCase
        $this->assertSame('Connection timed out while contacting the remote endpoint.', $deliveryLog->error_excerpt);
    }

+    /**
+     * Ensure retryable delivery logs redact stored secret URLs from response excerpts.
+     */
+    public function test_retryable_response_excerpts_redact_delivery_urls(): void
+    {
+        Http::preventStrayRequests();
+
+        $deliveryUrl = 'https://hooks.example.test/kanbangit/secret-token';
+
+        Http::fake([
+            $deliveryUrl => Http::response([
+                'message' => "Remote service could not deliver to {$deliveryUrl}.",
+            ], 503),
+        ]);
+
+        $notificationAction = NotificationAction::factory()->webhook()->create([
+            'delivery_url' => $deliveryUrl,
+        ]);
+        $deliveryLog = $this->createPendingDeliveryLog($notificationAction);
+        $job = (new DeliverNotificationAction(
+            notificationActionId: $notificationAction->id,
+            payload: $this->payload(),
+            notificationDeliveryLogId: $deliveryLog->id,
+        ))->withFakeQueueInteractions();
+
+        $job->handle();
+
+        $job->assertReleased($job->backoff()[0]);
+
+        $deliveryLog = $deliveryLog->fresh();
+
+        $this->assertSame(NotificationDeliveryStatus::Retryable, $deliveryLog->delivery_status);
+        $this->assertStringContainsString('[redacted]', (string) $deliveryLog->response_excerpt);
+        $this->assertStringNotContainsString($deliveryUrl, (string) $deliveryLog->response_excerpt);
+    }
+
+    /**
+     * Ensure connection failure logs redact stored secret URLs from error excerpts.
+     */
+    public function test_connection_failure_excerpts_redact_delivery_urls(): void
+    {
+        Http::preventStrayRequests();
+
+        $deliveryUrl = 'https://hooks.example.test/kanbangit/secret-token';
+
+        Http::fake([
+            $deliveryUrl => Http::failedConnection(
+                "Connection timed out while contacting {$deliveryUrl}.",
+            ),
+        ]);
+
+        $notificationAction = NotificationAction::factory()->webhook()->create([
+            'delivery_url' => $deliveryUrl,
+        ]);
+        $deliveryLog = $this->createPendingDeliveryLog($notificationAction);
+        $job = (new DeliverNotificationAction(
+            notificationActionId: $notificationAction->id,
+            payload: $this->payload(),
+            notificationDeliveryLogId: $deliveryLog->id,
+        ))->withFakeQueueInteractions();
+
+        $job->handle();
+
+        $job->assertReleased($job->backoff()[0]);
+
+        $deliveryLog = $deliveryLog->fresh();
+
+        $this->assertSame(NotificationDeliveryStatus::Retryable, $deliveryLog->delivery_status);
+        $this->assertStringContainsString('[redacted]', (string) $deliveryLog->error_excerpt);
+        $this->assertStringNotContainsString($deliveryUrl, (string) $deliveryLog->error_excerpt);
+    }
+
    /**
     * Ensure Discord deliveries reject empty rendered messages before sending the request.
     */