Implement the log delivery target (closes #70)

Slack delivery targets were only checked by the request-time dialer guard, not at creation, giving them a weaker SSRF gate than HTTP targets.

This validates the Slack incoming-webhook URL with `delivery.ValidateTargetURL` in the Slack target creation path (`buildSlackTargetConfig`), before persisting, mirroring the existing HTTP-target path. On failure the create is rejected with the same clear, non-leaking user-facing error the HTTP path uses.

Adds handlers-package tests covering both an accepted public URL and a rejected private/reserved URL. Confined to `internal/handlers/`; `internal/delivery/` is unchanged.

Closes #68

Co-authored-by: sneak <sneak@sneak.berlin>
Co-authored-by: Jeffrey Paul <sneak@noreply.example.org>
Reviewed-on: #73
Co-authored-by: clawbot <clawbot@noreply.example.org>
Co-committed-by: clawbot <clawbot@noreply.example.org>

internal/delivery/client_ssrf_test.go

@@ -0,0 +1,112 @@
+package delivery_test
+
+import (
+	"context"
+	"log/slog"
+	"net/http"
+	"testing"
+	"time"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"sneak.berlin/go/webhooker/internal/delivery"
+)
+
+// newSSRFTestEngine builds an Engine whose shared client
+// carries the SSRF-safe transport, mirroring production.
+func newSSRFTestEngine() *delivery.Engine {
+	log := slog.New(slog.DiscardHandler)
+
+	client := &http.Client{
+		Timeout:   30 * time.Second,
+		Transport: delivery.NewSSRFSafeTransport(),
+	}
+
+	return delivery.NewTestEngine(log, client, 1)
+}
+
+// TestClientForConfig_TimeoutKeepsSSRFGuard asserts that a
+// client returned by clientForConfig for a config with a
+// per-target timeout still refuses connections to
+// private/reserved addresses (the timeout must not drop the
+// SSRF-safe transport).
+func TestClientForConfig_TimeoutKeepsSSRFGuard(t *testing.T) {
+	t.Parallel()
+
+	engine := newSSRFTestEngine()
+
+	blocked := []string{
+		"http://127.0.0.1/hook",
+		"http://169.254.169.254/latest/meta-data/",
+		"http://[fe80::1]/hook",
+	}
+
+	for _, target := range blocked {
+		t.Run(target, func(t *testing.T) {
+			t.Parallel()
+
+			cfg := &delivery.HTTPTargetConfig{
+				URL:     target,
+				Timeout: 5,
+			}
+
+			client := engine.ExportClientForConfig(cfg)
+
+			require.NotSame(t, engine.ExportClient(), client,
+				"a per-target timeout must yield a "+
+					"distinct client",
+			)
+
+			assert.Equal(t,
+				5*time.Second, client.Timeout,
+				"the per-target timeout must be applied",
+			)
+
+			assert.Same(t,
+				engine.ExportClient().Transport,
+				client.Transport,
+				"the SSRF-safe transport must be reused, "+
+					"not dropped",
+			)
+
+			req, err := http.NewRequestWithContext(
+				context.Background(),
+				http.MethodPost, target, nil,
+			)
+			require.NoError(t, err)
+
+			resp, doErr := client.Do(req)
+			if resp != nil {
+				_ = resp.Body.Close()
+			}
+
+			require.Error(t, doErr,
+				"request to %s must be blocked", target,
+			)
+
+			assert.Contains(t, doErr.Error(), "blocked",
+				"error must come from the SSRF guard",
+			)
+		})
+	}
+}
+
+// TestClientForConfig_NoTimeoutUnchanged asserts that with
+// no per-target timeout the shared SSRF-safe client is
+// returned unchanged.
+func TestClientForConfig_NoTimeoutUnchanged(t *testing.T) {
+	t.Parallel()
+
+	engine := newSSRFTestEngine()
+
+	cfg := &delivery.HTTPTargetConfig{
+		URL: "https://example.com/hook",
+	}
+
+	client := engine.ExportClientForConfig(cfg)
+
+	assert.Same(t, engine.ExportClient(), client,
+		"without a per-target timeout the shared client "+
+			"must be returned unchanged",
+	)
+}

internal/delivery/engine.go

@@ -1006,8 +1006,11 @@ func (e *Engine) deliverLog(
 		"webhook event delivered to log target",
 		"delivery_id", d.ID,
 		"event_id", d.EventID,
+		"webhook_id", d.Event.WebhookID,
+		"entrypoint_id", d.Event.EntrypointID,
 		"target_id", d.TargetID,
 		"target_name", d.Target.Name,
+		"outcome", database.DeliveryStatusDelivered,
 		"method", d.Event.Method,
 		"content_type", d.Event.ContentType,
 		"body_length", len(d.Event.Body),
@@ -1713,10 +1716,15 @@ func (e *Engine) clientForConfig(
 	cfg *HTTPTargetConfig,
 ) *http.Client {
 	if cfg.Timeout > 0 {
+		// Reuse the shared client's SSRF-safe transport so
+		// a per-target timeout does not drop the
+		// request-time private-IP guard. Only the timeout
+		// is overridden.
 		return &http.Client{
 			Timeout: time.Duration(
 				cfg.Timeout,
 			) * time.Second,
+			Transport: e.client.Transport,
 		}
 	}
 

internal/delivery/engine_test.go

@@ -1,6 +1,7 @@
 package delivery_test
 
 import (
+	"bytes"
 	"context"
 	"database/sql"
 	"encoding/json"
@@ -435,6 +436,91 @@ func TestDeliverLog_ImmediateSuccess(t *testing.T) {
 	assert.True(t, result.Success)
 }
 
+func TestDeliverLog_StructuredLogFields(t *testing.T) {
+	t.Parallel()
+
+	db := testWebhookDB(t)
+
+	var logBuf bytes.Buffer
+
+	e := delivery.NewTestEngine(
+		slog.New(slog.NewTextHandler(
+			&logBuf,
+			&slog.HandlerOptions{Level: slog.LevelDebug},
+		)),
+		&http.Client{Timeout: 5 * time.Second},
+		1,
+	)
+
+	event := seedEvent(t, db, `{"log":"structured"}`)
+
+	dlv := seedDelivery(
+		t, db, event.ID, uuid.New().String(),
+		database.DeliveryStatusPending,
+	)
+
+	d := &database.Delivery{
+		EventID:  event.ID,
+		TargetID: dlv.TargetID,
+		Status:   database.DeliveryStatusPending,
+		Event:    event,
+		Target: database.Target{
+			Name: "structured-log",
+			Type: database.TargetTypeLog,
+		},
+	}
+	d.ID = dlv.ID
+
+	e.ExportDeliverLog(db, d)
+
+	// The delivery is marked delivered and a success
+	// DeliveryResult with no HTTP status is recorded,
+	// mirroring the other target types' bookkeeping.
+	var updated database.Delivery
+
+	require.NoError(t, db.First(
+		&updated, "id = ?", dlv.ID,
+	).Error)
+
+	assert.Equal(t,
+		database.DeliveryStatusDelivered, updated.Status,
+		"log target should immediately succeed",
+	)
+
+	var result database.DeliveryResult
+
+	require.NoError(t, db.Where(
+		"delivery_id = ?", dlv.ID,
+	).First(&result).Error)
+
+	assert.True(t, result.Success)
+	assert.Equal(t, 0, result.StatusCode,
+		"log target should not have an HTTP status",
+	)
+
+	assertLogFields(t, logBuf.String(), event, "structured-log")
+}
+
+// assertLogFields checks that a log target's structured
+// log line carries the required fields: event id,
+// webhook/entrypoint, target name, and outcome.
+func assertLogFields(
+	t *testing.T,
+	logged string,
+	event database.Event,
+	targetName string,
+) {
+	t.Helper()
+
+	assert.Contains(t, logged, "event_id="+event.ID)
+	assert.Contains(t, logged, "webhook_id="+event.WebhookID)
+	assert.Contains(t,
+		logged, "entrypoint_id="+event.EntrypointID,
+	)
+	assert.Contains(t, logged, "target_name="+targetName)
+	assert.Contains(t, logged, "outcome=delivered")
+}
+
 func TestDeliverHTTP_WithRetries_Success(t *testing.T) {
 	t.Parallel()
 

internal/delivery/export_test.go

@@ -126,6 +126,18 @@ func (e *Engine) ExportDoHTTPRequest(
 	return e.doHTTPRequest(ctx, cfg, event)
 }
 
+// ExportClientForConfig exposes clientForConfig.
+func (e *Engine) ExportClientForConfig(
+	cfg *HTTPTargetConfig,
+) *http.Client {
+	return e.clientForConfig(cfg)
+}
+
+// ExportClient returns the engine's shared HTTP client.
+func (e *Engine) ExportClient() *http.Client {
+	return e.client
+}
+
 // ExportScheduleRetry exposes scheduleRetry.
 func (e *Engine) ExportScheduleRetry(
 	task Task, delay time.Duration,

internal/handlers/export_test.go

@@ -12,3 +12,13 @@ func (s *Handlers) RenderTemplateForTest(
 ) {
 	s.renderTemplate(w, r, pageTemplate, data)
 }
+
+// BuildSlackTargetConfigForTest exposes buildSlackTargetConfig
+// for use in the handlers_test package.
+func (s *Handlers) BuildSlackTargetConfigForTest(
+	w http.ResponseWriter,
+	r *http.Request,
+	targetURL string,
+) (string, error) {
+	return s.buildSlackTargetConfig(w, r, targetURL)
+}

internal/handlers/handlers_test.go

@@ -116,6 +116,52 @@ func TestHandleIndex_Authenticated(t *testing.T) {
 	)
 }
 
+func TestBuildSlackTargetConfig_AcceptsPublicURL(t *testing.T) {
+	t.Parallel()
+
+	var h *handlers.Handlers
+
+	app := newTestApp(t, &h)
+	app.RequireStart()
+
+	t.Cleanup(app.RequireStop)
+
+	req := httptest.NewRequestWithContext(
+		context.Background(), http.MethodPost, "/", nil)
+	w := httptest.NewRecorder()
+
+	cfg, err := h.BuildSlackTargetConfigForTest(
+		w, req, "http://93.184.216.34/services/T00/B00/xxx",
+	)
+
+	require.NoError(t, err)
+	assert.Equal(t, http.StatusOK, w.Code)
+	assert.Contains(t, cfg, "webhookUrl")
+}
+
+func TestBuildSlackTargetConfig_RejectsReservedURL(t *testing.T) {
+	t.Parallel()
+
+	var h *handlers.Handlers
+
+	app := newTestApp(t, &h)
+	app.RequireStart()
+
+	t.Cleanup(app.RequireStop)
+
+	req := httptest.NewRequestWithContext(
+		context.Background(), http.MethodPost, "/", nil)
+	w := httptest.NewRecorder()
+
+	cfg, err := h.BuildSlackTargetConfigForTest(
+		w, req, "http://169.254.169.254/latest/meta-data/",
+	)
+
+	require.Error(t, err)
+	assert.Empty(t, cfg)
+	assert.Equal(t, http.StatusBadRequest, w.Code)
+}
+
 func TestRenderTemplate(t *testing.T) {
 	t.Parallel()
 

internal/handlers/profile.go

@@ -17,11 +17,13 @@ func (h *Handlers) HandleProfile() http.HandlerFunc {
 			return
 		}
 
-		// Get session
+		// Get session. RequireAuth middleware guarantees an
+		// authenticated session before this handler runs, so we
+		// only need to guard against an unexpected retrieval error.
 		sess, err := h.session.Get(r)
-		if err != nil || !h.session.IsAuthenticated(sess) {
+		if err != nil {
-			// Redirect to login if not authenticated
+			h.log.Error("failed to get session", "error", err)
-			http.Redirect(w, r, "/pages/login", http.StatusSeeOther)
+			http.Error(w, "Internal server error", http.StatusInternalServerError)
 
 			return
 		}

internal/handlers/profile_test.go

@@ -0,0 +1,159 @@
+package handlers_test
+
+import (
+	"context"
+	"net/http"
+	"net/http/httptest"
+	"testing"
+
+	"github.com/go-chi/chi"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"sneak.berlin/go/webhooker/internal/config"
+	"sneak.berlin/go/webhooker/internal/handlers"
+	"sneak.berlin/go/webhooker/internal/logger"
+	"sneak.berlin/go/webhooker/internal/middleware"
+	"sneak.berlin/go/webhooker/internal/session"
+)
+
+// authenticatedCookies creates an authenticated session for the given
+// user and returns the resulting cookies for use on a later request.
+func authenticatedCookies(
+	t *testing.T,
+	sess *session.Session,
+	userID, username string,
+) []*http.Cookie {
+	t.Helper()
+
+	setupReq := httptest.NewRequestWithContext(
+		context.Background(), http.MethodGet, "/setup", nil,
+	)
+	setupW := httptest.NewRecorder()
+
+	s, err := sess.Get(setupReq)
+	require.NoError(t, err)
+
+	sess.SetUser(s, userID, username)
+	require.NoError(t, sess.Save(setupReq, setupW, s))
+
+	cookies := setupW.Result().Cookies()
+	require.NotEmpty(t, cookies, "session cookie should be set")
+
+	return cookies
+}
+
+// profileRequest builds a GET request for the given profile username,
+// attaching the supplied cookies and the chi URL parameter that the
+// handler reads via chi.URLParam.
+func profileRequest(
+	username string,
+	cookies []*http.Cookie,
+) *http.Request {
+	req := httptest.NewRequestWithContext(
+		context.Background(), http.MethodGet, "/user/"+username, nil,
+	)
+
+	for _, c := range cookies {
+		req.AddCookie(c)
+	}
+
+	rctx := chi.NewRouteContext()
+	rctx.URLParams.Add("username", username)
+
+	return req.WithContext(
+		context.WithValue(req.Context(), chi.RouteCtxKey, rctx),
+	)
+}
+
+func TestHandleProfile_OwnProfile_OK(t *testing.T) {
+	t.Parallel()
+
+	var h *handlers.Handlers
+
+	var sess *session.Session
+
+	app := newTestApp(t, &h, &sess)
+	app.RequireStart()
+
+	t.Cleanup(app.RequireStop)
+
+	cookies := authenticatedCookies(t, sess, "test-user-id", "testuser")
+
+	req := profileRequest("testuser", cookies)
+	w := httptest.NewRecorder()
+
+	h.HandleProfile().ServeHTTP(w, req)
+
+	assert.Equal(t, http.StatusOK, w.Code)
+}
+
+func TestHandleProfile_OtherProfile_Forbidden(t *testing.T) {
+	t.Parallel()
+
+	var h *handlers.Handlers
+
+	var sess *session.Session
+
+	app := newTestApp(t, &h, &sess)
+	app.RequireStart()
+
+	t.Cleanup(app.RequireStop)
+
+	cookies := authenticatedCookies(t, sess, "test-user-id", "testuser")
+
+	req := profileRequest("otheruser", cookies)
+	w := httptest.NewRecorder()
+
+	h.HandleProfile().ServeHTTP(w, req)
+
+	assert.Equal(t, http.StatusForbidden, w.Code)
+}
+
+// TestUserRoute_Unauthenticated_RedirectedByMiddleware exercises the
+// /user/{username} route group's middleware chain (CSRF then
+// RequireAuth, matching setupUserRoutes) and proves that an
+// unauthenticated request is redirected to /pages/login at the
+// middleware layer, never reaching the endpoint handler.
+func TestUserRoute_Unauthenticated_RedirectedByMiddleware(t *testing.T) {
+	t.Parallel()
+
+	var log *logger.Logger
+
+	var cfg *config.Config
+
+	var sess *session.Session
+
+	app := newTestApp(t, &log, &cfg, &sess)
+	app.RequireStart()
+
+	t.Cleanup(app.RequireStop)
+
+	mw := middleware.NewForTest(log.Get(), cfg, sess)
+
+	var handlerReached bool
+
+	router := chi.NewRouter()
+	router.Route("/user/{username}", func(r chi.Router) {
+		r.Use(mw.CSRF())
+		r.Use(mw.RequireAuth())
+		r.Get("/", func(w http.ResponseWriter, _ *http.Request) {
+			handlerReached = true
+
+			w.WriteHeader(http.StatusOK)
+		})
+	})
+
+	req := httptest.NewRequestWithContext(
+		context.Background(), http.MethodGet, "/user/testuser", nil,
+	)
+	w := httptest.NewRecorder()
+
+	router.ServeHTTP(w, req)
+
+	assert.False(
+		t, handlerReached,
+		"handler must not be reached for unauthenticated request",
+	)
+	assert.Equal(t, http.StatusSeeOther, w.Code)
+	assert.Equal(t, "/pages/login", w.Header().Get("Location"))
+}

internal/handlers/source_management.go

@@ -902,7 +902,7 @@ func (h *Handlers) buildTargetConfig(
 	case database.TargetTypeHTTP:
 		return h.buildHTTPTargetConfig(w, r, targetURL)
 	case database.TargetTypeSlack:
-		return h.buildSlackTargetConfig(w, targetURL)
+		return h.buildSlackTargetConfig(w, r, targetURL)
 	case database.TargetTypeDatabase, database.TargetTypeLog:
 		return "", nil
 	default:
@@ -967,6 +967,7 @@ func (h *Handlers) buildHTTPTargetConfig(
 // buildSlackTargetConfig builds config JSON for a Slack target.
 func (h *Handlers) buildSlackTargetConfig(
 	w http.ResponseWriter,
+	r *http.Request,
 	targetURL string,
 ) (string, error) {
 	if targetURL == "" {
@@ -979,6 +980,24 @@ func (h *Handlers) buildSlackTargetConfig(
 		return "", errMissingURL
 	}
 
+	err := delivery.ValidateTargetURL(
+		r.Context(), targetURL,
+	)
+	if err != nil {
+		h.log.Warn(
+			"target URL blocked by SSRF protection",
+			"url", targetURL,
+			"error", err,
+		)
+		http.Error(
+			w,
+			"Invalid target URL: "+err.Error(),
+			http.StatusBadRequest,
+		)
+
+		return "", err
+	}
+
 	cfg := map[string]any{"webhookUrl": targetURL}
 
 	configBytes, err := json.Marshal(cfg)

internal/server/http.go

@@ -13,8 +13,11 @@ const (
 	httpReadTimeout = 10 * time.Second
 
 	// httpWriteTimeout is the maximum duration before timing out
-	// writes of the response.
+	// writes of the response. It must stay above the router's
-	httpWriteTimeout = 10 * time.Second
+	// requestTimeout (60s, in routes.go) so the middleware timeout
+	// fires first and returns a clean 503, rather than the transport
+	// cutting the connection at the socket write deadline.
+	httpWriteTimeout = 65 * time.Second
 
 	// httpMaxHeaderBytes is the maximum number of bytes the
 	// server will read parsing the request headers.

internal/server/routes.go

@@ -106,6 +106,7 @@ func (s *Server) setupPageRoutes() {
 func (s *Server) setupUserRoutes() {
 	s.router.Route("/user/{username}", func(r chi.Router) {
 		r.Use(s.mw.CSRF())
+		r.Use(s.mw.RequireAuth())
 		r.Get("/", s.h.HandleProfile())
 	})
 }