From b1f43c9520a6ec60fcfa7c213e7362d7009759a2 Mon Sep 17 00:00:00 2001 From: clawbot Date: Fri, 7 Aug 2026 14:03:38 +0200 Subject: [PATCH] Keep the SSRF-safe transport in clientForConfig (closes #69) (#74) `clientForConfig()` in `internal/delivery/engine.go` built a fresh `http.Client` without a Transport when a per-target timeout was configured, dropping the request-time private-IP guard for that path. It now reuses the shared client's SSRF-safe transport (`e.client.Transport`, the same `NewSSRFSafeTransport` instance), overriding only the `Timeout`. Behaviour is unchanged when no per-target timeout is set (the shared client is returned as before), so no engine code path makes an outbound target request with a client lacking the SSRF-safe transport. Adds a delivery-package test proving a client from `clientForConfig()` with a per-target timeout still refuses private/reserved/link-local destinations, that the timeout is applied, that the SSRF-safe transport is reused (not duplicated), and that the no-timeout path returns the shared client unchanged. Confined to `internal/delivery/` only; handlers and server code untouched. Closes #69 Co-authored-by: sneak Co-authored-by: Jeffrey Paul Reviewed-on: https://git.eeqj.de/sneak/webhooker/pulls/74 Co-authored-by: clawbot Co-committed-by: clawbot --- internal/delivery/client_ssrf_test.go | 112 ++++++++++++++++++++++++++ internal/delivery/engine.go | 5 ++ internal/delivery/export_test.go | 12 +++ 3 files changed, 129 insertions(+) create mode 100644 internal/delivery/client_ssrf_test.go diff --git a/internal/delivery/client_ssrf_test.go b/internal/delivery/client_ssrf_test.go new file mode 100644 index 0000000..d31164a --- /dev/null +++ b/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", + ) +} diff --git a/internal/delivery/engine.go b/internal/delivery/engine.go index 2197bcf..674f33b 100644 --- a/internal/delivery/engine.go +++ b/internal/delivery/engine.go @@ -1713,10 +1713,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, } } diff --git a/internal/delivery/export_test.go b/internal/delivery/export_test.go index 283e1ba..f9b987d 100644 --- a/internal/delivery/export_test.go +++ b/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,