diff --git a/internal/delivery/engine_integration_test.go b/internal/delivery/engine_integration_test.go index 784cf2a..a902a9a 100644 --- a/internal/delivery/engine_integration_test.go +++ b/internal/delivery/engine_integration_test.go @@ -1425,6 +1425,32 @@ func TestDeliverHTTP_InvalidConfig(t *testing.T) { ) } +// TestDeliverHTTP_InvalidConfigUnrecordedStaysPending: a delivery is +// failed for an invalid config only once the reason is recorded. +// Unrecorded, it stays pending, where the sweep finds it again. +func TestDeliverHTTP_InvalidConfigUnrecordedStaysPending(t *testing.T) { + t.Parallel() + + db := testWebhookDB(t) + e := testEngine(t, 1) + + event, del := iSeedEventAndDelivery( + t, db, `{"config":"invalid"}`, "", + ) + + task, d := iHTTPTaskAndDelivery( + event, del, "bad-config", `not-json`, 0, 1, + ) + + require.NoError(t, db.Exec("drop table delivery_results").Error) + + e.ExportDeliverHTTP(context.TODO(), db, d, task) + + iAssertStatus(t, db, del.ID, + database.DeliveryStatusPending, + ) +} + // --- Notify batching --- func TestNotify_MultipleTasks(t *testing.T) { diff --git a/internal/delivery/engine_test.go b/internal/delivery/engine_test.go index 48bb9d2..d91fbd0 100644 --- a/internal/delivery/engine_test.go +++ b/internal/delivery/engine_test.go @@ -5,6 +5,7 @@ import ( "context" "encoding/json" "fmt" + "io" "log/slog" "net/http" "net/http/httptest" @@ -1056,6 +1057,21 @@ func TestParseHTTPConfig_MissingURL(t *testing.T) { ) } +func TestParseHTTPConfig_Undecodable(t *testing.T) { + t.Parallel() + + e := testEngine(t, 1) + + _, err := e.ExportParseHTTPConfig( + `{"url":"https://example.com/hook","timeout":"soon"}`, + ) + + assert.Error(t, err, + "config that does not decode should return error, "+ + "even when the part that did names a URL", + ) +} + func TestScheduleRetry_SendsToRetryChannel( t *testing.T, ) { @@ -1241,6 +1257,33 @@ func TestDoHTTPRequest_ForwardsHeaders(t *testing.T) { ) } +// A response that ends before the length it announced is an error, not +// a short body. +func TestDoHTTPRequest_CutShortResponseIsAnError(t *testing.T) { + t.Parallel() + + ts := httptest.NewServer( + http.HandlerFunc( + func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Length", "100") + _, _ = w.Write([]byte("cut short")) + }, + ), + ) + defer ts.Close() + + e := testEngine(t, 1) + + _, body, _, err := e.ExportDoHTTPRequest( + context.TODO(), + &delivery.HTTPTargetConfig{URL: ts.URL}, + &database.Event{}, + ) + + require.ErrorIs(t, err, io.ErrUnexpectedEOF) + assert.Empty(t, body) +} + // The event's stored inbound headers carry the same Content-Type the // receiver saved as the event's ContentType, so a delivery could send // it twice. It must go out exactly once, with a Content-Type configured @@ -1317,6 +1360,34 @@ func TestApplyRequestHeaders_SendsOneContentType(t *testing.T) { } } +// Stored inbound headers that do not decode forward nothing, not the +// part of them that happened to decode. +func TestApplyRequestHeaders_UndecodableInboundForwardsNothing( + t *testing.T, +) { + t.Parallel() + + req, err := http.NewRequestWithContext( + context.Background(), + http.MethodPost, + "https://target.example.com/hook", + http.NoBody, + ) + require.NoError(t, err) + + names := delivery.ExportApplyRequestHeaders( + req, + &database.Event{ + Headers: `{"X-Custom":["value1"],"X-Broken":"not a list"}`, + }, + &delivery.HTTPTargetConfig{}, + "webhooker/dev", + ) + + assert.Empty(t, names) + assert.Empty(t, req.Header.Get("X-Custom")) +} + func TestProcessDelivery_RoutesToCorrectHandler( t *testing.T, ) { diff --git a/internal/delivery/recovery_durability_test.go b/internal/delivery/recovery_durability_test.go index a139eeb..2dc23c8 100644 --- a/internal/delivery/recovery_durability_test.go +++ b/internal/delivery/recovery_durability_test.go @@ -376,3 +376,97 @@ func TestFailedResultWriteLeavesDeliveryRecoverable( database.DeliveryStatusPending, ) } + +// TestFailedResultWriteWithRetriesLeavesDeliveryRecoverable is the same +// rule for a target with retries: whatever the receiver answered, the +// delivery stays pending and no retry is scheduled. The circuit breaker +// still learns the answer, because it describes the target's health, +// not the database's. +func TestFailedResultWriteWithRetriesLeavesDeliveryRecoverable( + t *testing.T, +) { + t.Parallel() + + // The "send succeeded" case starts with the breaker tripped open, + // so the delivery goes out as its probe and only a recorded + // success closes it again. + tests := []struct { + name string + answer int + tripped bool + wantBreaker delivery.CircuitState + }{ + {"send succeeded", http.StatusOK, true, delivery.CircuitClosed}, + {"send failed", http.StatusBadGateway, false, delivery.CircuitOpen}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + s := newISetup(t) + targetID := uuid.New().String() + + ts := httptest.NewServer(http.HandlerFunc( + func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(tc.answer) + }, + )) + defer ts.Close() + + event := iSeedEvent( + t, s.WebhookDB, s.WebhookID, `{"unwritable":true}`, + ) + + d := iSeedDelivery( + t, s.WebhookDB, event.ID, targetID, + database.DeliveryStatusPending, + ) + + require.NoError( + t, + s.WebhookDB.Exec("drop table delivery_results").Error, + ) + + // A single failure opens this breaker, and with no + // cooldown an open breaker lets the next delivery + // through as a probe. + cb := delivery.NewTestCircuitBreaker(1, 0) + if tc.tripped { + cb.RecordFailure() + } + + s.Engine.ExportSetCircuitBreaker(targetID, cb) + + full := &database.Delivery{ + EventID: event.ID, + TargetID: targetID, + Status: database.DeliveryStatusPending, + Event: event, + Target: database.Target{ + Name: "unwritable", + Type: database.TargetTypeHTTP, + Config: iHTTPConfig(ts.URL), + MaxRetries: 3, + }, + } + full.ID = d.ID + + sched := &recordingScheduler{} + + s.Engine.ExportDeliverHTTPWithScheduler( + context.Background(), s.WebhookDB, full, + &delivery.Task{ + DeliveryID: d.ID, + TargetID: targetID, + AttemptNum: 1, + }, + sched, + ) + + iAssertStatus(t, s.WebhookDB, d.ID, database.DeliveryStatusPending) + assert.Empty(t, sched.delays, "no retry may be scheduled") + assert.Equal(t, tc.wantBreaker, cb.State()) + }) + } +} diff --git a/internal/delivery/url_mask_test.go b/internal/delivery/url_mask_test.go index f699b36..da5327c 100644 --- a/internal/delivery/url_mask_test.go +++ b/internal/delivery/url_mask_test.go @@ -179,6 +179,27 @@ func TestDoHTTPRequest_TransportErrorMasksURL(t *testing.T) { ) } +// TestDoHTTPRequest_UnparsableURLIsMasked is the same for an HTTP +// target URL that no request can be built from. +func TestDoHTTPRequest_UnparsableURLIsMasked(t *testing.T) { + t.Parallel() + + e := testEngine(t, 1) + + statusCode, _, _, reqErr := e.ExportDoHTTPRequest( + context.TODO(), + &delivery.HTTPTargetConfig{ + URL: "https://hooks.example.com" + maskSecretPath + "\n", + }, + &database.Event{}, + ) + require.Error(t, reqErr) + assert.Zero(t, statusCode) + + assertNoCredential(t, reqErr.Error()) + assert.Contains(t, reqErr.Error(), "invalid control character") +} + // TestValidateTargetURL_UnparsableURLIsMasked proves the SSRF // validator's error does not carry the submitted URL, which // the handler both logs and shows.