From 0fa3d049903cb21b16a43749cd4f758b1b226981 Mon Sep 17 00:00:00 2001 From: David Gageot Date: Thu, 7 May 2026 17:06:26 +0200 Subject: [PATCH 1/2] feat: add http_post builtin hook --- pkg/hooks/builtins/builtins.go | 2 + pkg/hooks/builtins/builtins_test.go | 1 + pkg/hooks/builtins/http_post.go | 49 +++++++++ pkg/hooks/builtins/http_post_test.go | 159 +++++++++++++++++++++++++++ 4 files changed, 211 insertions(+) create mode 100644 pkg/hooks/builtins/http_post.go create mode 100644 pkg/hooks/builtins/http_post_test.go diff --git a/pkg/hooks/builtins/builtins.go b/pkg/hooks/builtins/builtins.go index 8df4204fe..cdfa98705 100644 --- a/pkg/hooks/builtins/builtins.go +++ b/pkg/hooks/builtins/builtins.go @@ -23,6 +23,7 @@ // tool output. Same builtin, dispatches on // event so a single name covers all three // legs of the feature. +// - http_post (any event) — POST args[1] to args[0] // // Reference any of them from a hook YAML entry as // `{type: builtin, command: ""}`. The runtime additionally @@ -118,6 +119,7 @@ func Register(r *hooks.Registry) (*State, error) { r.RegisterBuiltin(MaxIterations, state.maxIterations.hook), r.RegisterBuiltin(Snapshot, state.snapshot.hook), r.RegisterBuiltin(RedactSecrets, redactSecrets), + r.RegisterBuiltin(HTTPPost, httpPost), ); err != nil { return nil, err } diff --git a/pkg/hooks/builtins/builtins_test.go b/pkg/hooks/builtins/builtins_test.go index 622a5eddc..8fa1dc43c 100644 --- a/pkg/hooks/builtins/builtins_test.go +++ b/pkg/hooks/builtins/builtins_test.go @@ -36,6 +36,7 @@ func TestRegisterInstallsAllBuiltins(t *testing.T) { builtins.MaxIterations, builtins.Snapshot, builtins.RedactSecrets, + builtins.HTTPPost, } { fn, ok := r.LookupBuiltin(name) assert.True(t, ok, "builtin %q must be registered", name) diff --git a/pkg/hooks/builtins/http_post.go b/pkg/hooks/builtins/http_post.go new file mode 100644 index 000000000..f6957dc45 --- /dev/null +++ b/pkg/hooks/builtins/http_post.go @@ -0,0 +1,49 @@ +package builtins + +import ( + "context" + "fmt" + "io" + "log/slog" + "net/http" + "strings" + + "github.com/docker/docker-agent/pkg/hooks" +) + +// HTTPPost is the registered name of the http_post builtin. +const HTTPPost = "http_post" + +// httpPost POSTs args[1] to args[0] with Content-Type: application/json. +// Empty URL is a no-op; network errors and non-2xx responses are +// logged and swallowed so the dispatch verdict stays nil. The hook +// executor already wraps ctx with [Hook.GetTimeout]. +func httpPost(ctx context.Context, _ *hooks.Input, args []string) (*hooks.Output, error) { + if len(args) == 0 || args[0] == "" { + return nil, nil + } + url := args[0] + var body string + if len(args) >= 2 { + body = args[1] + } + + req, err := http.NewRequestWithContext(ctx, http.MethodPost, url, strings.NewReader(body)) + if err != nil { + return nil, fmt.Errorf("http_post: build request: %w", err) + } + req.Header.Set("Content-Type", "application/json") + + resp, err := http.DefaultClient.Do(req) + if err != nil { + slog.WarnContext(ctx, "http_post: request failed", "url", url, "error", err) + return nil, nil + } + defer resp.Body.Close() + // Drain so the connection can be reused by keep-alive. + _, _ = io.Copy(io.Discard, resp.Body) + if resp.StatusCode >= 400 { + slog.WarnContext(ctx, "http_post: non-success response", "url", url, "status", resp.StatusCode) + } + return nil, nil +} diff --git a/pkg/hooks/builtins/http_post_test.go b/pkg/hooks/builtins/http_post_test.go new file mode 100644 index 000000000..a22bcc764 --- /dev/null +++ b/pkg/hooks/builtins/http_post_test.go @@ -0,0 +1,159 @@ +package builtins_test + +import ( + "context" + "io" + "net/http" + "net/http/httptest" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/docker/docker-agent/pkg/hooks" + "github.com/docker/docker-agent/pkg/hooks/builtins" +) + +// TestHTTPPostSendsBodyToURL pins the happy path: POST with body +// and Content-Type: application/json, and a nil Output. +func TestHTTPPostSendsBodyToURL(t *testing.T) { + t.Parallel() + + const payload = `{"event":"turn_start"}` + + var ( + gotMethod string + gotContentType string + gotBody []byte + ) + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + gotMethod = r.Method + gotContentType = r.Header.Get("Content-Type") + gotBody, _ = io.ReadAll(r.Body) + w.WriteHeader(http.StatusNoContent) + })) + t.Cleanup(srv.Close) + + fn := lookup(t, builtins.HTTPPost) + + out, err := fn(t.Context(), &hooks.Input{SessionID: "s"}, []string{srv.URL, payload}) + require.NoError(t, err) + assert.Nil(t, out) + + assert.Equal(t, http.MethodPost, gotMethod) + assert.Equal(t, "application/json", gotContentType) + assert.JSONEq(t, payload, string(gotBody)) +} + +// TestHTTPPostEmptyBodyIsAllowed: omitting the second arg sends an +// empty body — useful for ping-style webhooks. +func TestHTTPPostEmptyBodyIsAllowed(t *testing.T) { + t.Parallel() + + var gotBody []byte + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + gotBody, _ = io.ReadAll(r.Body) + w.WriteHeader(http.StatusOK) + })) + t.Cleanup(srv.Close) + + fn := lookup(t, builtins.HTTPPost) + + out, err := fn(t.Context(), &hooks.Input{SessionID: "s"}, []string{srv.URL}) + require.NoError(t, err) + assert.Nil(t, out) + assert.Empty(t, gotBody) +} + +// TestHTTPPostNoOpWithoutURL: a missing or empty URL is a no-op so +// a misconfigured YAML doesn't break the run loop. +func TestHTTPPostNoOpWithoutURL(t *testing.T) { + t.Parallel() + + fn := lookup(t, builtins.HTTPPost) + + cases := [][]string{ + nil, + {}, + {""}, + {"", "body"}, + } + for _, args := range cases { + out, err := fn(t.Context(), &hooks.Input{SessionID: "s"}, args) + require.NoErrorf(t, err, "args=%v: must not error", args) + assert.Nilf(t, out, "args=%v: must be a no-op", args) + } +} + +// TestHTTPPostSwallowsErrors: neither a non-2xx response nor an +// unreachable receiver propagates as a hook error. +func TestHTTPPostSwallowsErrors(t *testing.T) { + t.Parallel() + + serverError := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusInternalServerError) + })) + t.Cleanup(serverError.Close) + + // Bind, capture URL, then close: the port is now guaranteed-unreachable. + unreachable := httptest.NewServer(http.HandlerFunc(func(http.ResponseWriter, *http.Request) {})) + unreachableURL := unreachable.URL + unreachable.Close() + + cases := map[string]string{ + "non-2xx response": serverError.URL, + "unreachable receiver": unreachableURL, + } + + fn := lookup(t, builtins.HTTPPost) + for name, url := range cases { + t.Run(name, func(t *testing.T) { + t.Parallel() + out, err := fn(t.Context(), &hooks.Input{SessionID: "s"}, []string{url, "{}"}) + require.NoError(t, err) + assert.Nil(t, out) + }) + } +} + +// TestHTTPPostMalformedURLReturnsError: an unparseable URL is the +// one error path that surfaces, so `on_error: warn` flags the +// misconfig. +func TestHTTPPostMalformedURLReturnsError(t *testing.T) { + t.Parallel() + + fn := lookup(t, builtins.HTTPPost) + + out, err := fn(t.Context(), &hooks.Input{SessionID: "s"}, []string{"http://\x7f\x00.example", "{}"}) + require.Error(t, err) + assert.Nil(t, out) + assert.Contains(t, err.Error(), "http_post:") +} + +// TestHTTPPostHonoursContextCancellation: the request returns +// promptly after ctx deadline instead of waiting for the handler. +func TestHTTPPostHonoursContextCancellation(t *testing.T) { + t.Parallel() + + // Bounded sleep so the client must abandon before the response, + // while keeping httptest.Server.Close() cleanup quick. + srv := httptest.NewServer(http.HandlerFunc(func(_ http.ResponseWriter, _ *http.Request) { + time.Sleep(300 * time.Millisecond) + })) + t.Cleanup(srv.Close) + + fn := lookup(t, builtins.HTTPPost) + + ctx, cancel := context.WithTimeout(t.Context(), 30*time.Millisecond) + defer cancel() + + start := time.Now() + out, err := fn(ctx, &hooks.Input{SessionID: "s"}, []string{srv.URL, "{}"}) + elapsed := time.Since(start) + + // Network errors (incl. cancellation) are swallowed by design. + require.NoError(t, err) + assert.Nil(t, out) + assert.Less(t, elapsed, 250*time.Millisecond) +} From 29c6a0ff459b65049bfe9a32a6a1da0b7eccf622 Mon Sep 17 00:00:00 2001 From: David Gageot Date: Thu, 7 May 2026 17:26:38 +0200 Subject: [PATCH 2/2] harden http_post: ssrf-safe client, scheme check, redacted logs, bounded drain --- pkg/hooks/builtins/export_test.go | 18 +++++++++++++ pkg/hooks/builtins/http_post.go | 40 +++++++++++++++++++++------- pkg/hooks/builtins/http_post_test.go | 27 +++++++++++++------ pkg/hooks/builtins/main_test.go | 18 +++++++++++++ 4 files changed, 85 insertions(+), 18 deletions(-) create mode 100644 pkg/hooks/builtins/export_test.go create mode 100644 pkg/hooks/builtins/main_test.go diff --git a/pkg/hooks/builtins/export_test.go b/pkg/hooks/builtins/export_test.go new file mode 100644 index 000000000..e28725895 --- /dev/null +++ b/pkg/hooks/builtins/export_test.go @@ -0,0 +1,18 @@ +package builtins + +import ( + "time" + + "github.com/docker/docker-agent/pkg/httpclient" +) + +// SetHTTPPostClientUnsafeForTest swaps the httpPost client for one that +// bypasses SSRF dial-time protection so tests can talk to +// httptest.NewServer (which binds to 127.0.0.1). Returns a restore +// function. Test-only — this file is *_test.go so it never compiles +// into release binaries. +func SetHTTPPostClientUnsafeForTest() func() { + prev := httpPostClient + httpPostClient = httpclient.NewSafeClient(30*time.Second, true) + return func() { httpPostClient = prev } +} diff --git a/pkg/hooks/builtins/http_post.go b/pkg/hooks/builtins/http_post.go index f6957dc45..bfa8a905e 100644 --- a/pkg/hooks/builtins/http_post.go +++ b/pkg/hooks/builtins/http_post.go @@ -2,48 +2,68 @@ package builtins import ( "context" + "errors" "fmt" "io" "log/slog" "net/http" + "net/url" "strings" + "time" "github.com/docker/docker-agent/pkg/hooks" + "github.com/docker/docker-agent/pkg/httpclient" ) // HTTPPost is the registered name of the http_post builtin. const HTTPPost = "http_post" +// httpPostClient is the HTTP client used by httpPost. It refuses +// connections to non-public IPs at dial time (defeating DNS rebinding +// to loopback / RFC1918 / link-local incl. cloud metadata at +// 169.254.169.254) and bounds redirects at 10 hops. Tests swap it for +// an unsafe variant via export_test.go since httptest.NewServer binds +// to 127.0.0.1. +var httpPostClient = httpclient.NewSafeClient(30*time.Second, false) + // httpPost POSTs args[1] to args[0] with Content-Type: application/json. -// Empty URL is a no-op; network errors and non-2xx responses are -// logged and swallowed so the dispatch verdict stays nil. The hook -// executor already wraps ctx with [Hook.GetTimeout]. +// An empty URL is a no-op (lenient args contract). A non-http(s) or +// otherwise unparseable URL surfaces as an error so on_error: warn +// flags the misconfig. Network errors and non-2xx responses are +// logged (with credentials redacted) and swallowed so a bad webhook +// never breaks the run loop. The hook executor already wraps ctx with +// [Hook.GetTimeout]; the client's Timeout is a backstop. func httpPost(ctx context.Context, _ *hooks.Input, args []string) (*hooks.Output, error) { if len(args) == 0 || args[0] == "" { return nil, nil } - url := args[0] + target, err := url.Parse(args[0]) + if err != nil || target.Host == "" || (target.Scheme != "http" && target.Scheme != "https") { + return nil, errors.New("http_post: only http(s) URLs are supported") + } var body string if len(args) >= 2 { body = args[1] } + redacted := target.Redacted() - req, err := http.NewRequestWithContext(ctx, http.MethodPost, url, strings.NewReader(body)) + req, err := http.NewRequestWithContext(ctx, http.MethodPost, target.String(), strings.NewReader(body)) if err != nil { return nil, fmt.Errorf("http_post: build request: %w", err) } req.Header.Set("Content-Type", "application/json") - resp, err := http.DefaultClient.Do(req) + resp, err := httpPostClient.Do(req) if err != nil { - slog.WarnContext(ctx, "http_post: request failed", "url", url, "error", err) + slog.WarnContext(ctx, "http_post: request failed", "url", redacted, "error", err) return nil, nil } defer resp.Body.Close() - // Drain so the connection can be reused by keep-alive. - _, _ = io.Copy(io.Discard, resp.Body) + // Cap the drain so a malicious receiver can't pin the goroutine on + // an unbounded read; 64 KiB is plenty for a webhook ack. + _, _ = io.Copy(io.Discard, io.LimitReader(resp.Body, 64<<10)) if resp.StatusCode >= 400 { - slog.WarnContext(ctx, "http_post: non-success response", "url", url, "status", resp.StatusCode) + slog.WarnContext(ctx, "http_post: non-success response", "url", redacted, "status", resp.StatusCode) } return nil, nil } diff --git a/pkg/hooks/builtins/http_post_test.go b/pkg/hooks/builtins/http_post_test.go index a22bcc764..5b404e759 100644 --- a/pkg/hooks/builtins/http_post_test.go +++ b/pkg/hooks/builtins/http_post_test.go @@ -117,18 +117,29 @@ func TestHTTPPostSwallowsErrors(t *testing.T) { } } -// TestHTTPPostMalformedURLReturnsError: an unparseable URL is the -// one error path that surfaces, so `on_error: warn` flags the -// misconfig. -func TestHTTPPostMalformedURLReturnsError(t *testing.T) { +// TestHTTPPostRejectsNonHTTPSchemes: file://, ftp://, javascript: and +// scheme-less or host-less inputs all surface as a config error +// rather than being silently dispatched to a transport. +func TestHTTPPostRejectsNonHTTPSchemes(t *testing.T) { t.Parallel() fn := lookup(t, builtins.HTTPPost) - out, err := fn(t.Context(), &hooks.Input{SessionID: "s"}, []string{"http://\x7f\x00.example", "{}"}) - require.Error(t, err) - assert.Nil(t, out) - assert.Contains(t, err.Error(), "http_post:") + cases := []string{ + "file:///etc/passwd", + "ftp://example.com/", + "javascript:alert(1)", + "not-a-url", + "http://", + "http://\x7f\x00.example", + } + for _, raw := range cases { + out, err := fn(t.Context(), &hooks.Input{SessionID: "s"}, []string{raw, "{}"}) + require.Errorf(t, err, "input %q must be rejected", raw) + assert.Nil(t, out) + assert.Contains(t, err.Error(), "http_post:") + assert.Contains(t, err.Error(), "http(s)") + } } // TestHTTPPostHonoursContextCancellation: the request returns diff --git a/pkg/hooks/builtins/main_test.go b/pkg/hooks/builtins/main_test.go new file mode 100644 index 000000000..900350724 --- /dev/null +++ b/pkg/hooks/builtins/main_test.go @@ -0,0 +1,18 @@ +package builtins_test + +import ( + "os" + "testing" + + "github.com/docker/docker-agent/pkg/hooks/builtins" +) + +// TestMain flips the http_post client to its unsafe variant for the +// duration of this test binary, since httptest.NewServer binds to +// 127.0.0.1 and is otherwise rejected by the production SSRF dialer. +// Production callers always go through the safe client wired in +// http_post.go. +func TestMain(m *testing.M) { + builtins.SetHTTPPostClientUnsafeForTest() + os.Exit(m.Run()) +}