diff --git a/app/Actions/Notifications/DeliverNotificationAction.php b/app/Actions/Notifications/DeliverNotificationAction.php index 58f642c..8cdaef5 100644 --- a/app/Actions/Notifications/DeliverNotificationAction.php +++ b/app/Actions/Notifications/DeliverNotificationAction.php @@ -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), ]); } @@ -300,15 +307,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()}."; @@ -367,4 +375,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); + } } diff --git a/tests/Feature/Api/TicketApiTest.php b/tests/Feature/Api/TicketApiTest.php index 224eadd..5015a28 100644 --- a/tests/Feature/Api/TicketApiTest.php +++ b/tests/Feature/Api/TicketApiTest.php @@ -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 @@ -193,6 +197,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(); diff --git a/tests/Feature/Mcp/KanbanGitMcpTest.php b/tests/Feature/Mcp/KanbanGitMcpTest.php index bcc56f7..1a44e9b 100644 --- a/tests/Feature/Mcp/KanbanGitMcpTest.php +++ b/tests/Feature/Mcp/KanbanGitMcpTest.php @@ -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_list_tickets_by_status_requires_a_status_selector(): void { $user = User::factory()->create(); diff --git a/tests/Feature/Notifications/DeliverNotificationActionTest.php b/tests/Feature/Notifications/DeliverNotificationActionTest.php index 87a0185..968af65 100644 --- a/tests/Feature/Notifications/DeliverNotificationActionTest.php +++ b/tests/Feature/Notifications/DeliverNotificationActionTest.php @@ -290,6 +290,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. */