Skip to content

feat(hooks): add http_post builtin#2705

Merged
dgageot merged 2 commits intodocker:mainfrom
dgageot:board/18211a338ad4b59e
May 7, 2026
Merged

feat(hooks): add http_post builtin#2705
dgageot merged 2 commits intodocker:mainfrom
dgageot:board/18211a338ad4b59e

Conversation

@dgageot
Copy link
Copy Markdown
Member

@dgageot dgageot commented May 7, 2026

Adds a tiny generic http_post builtin hook under pkg/hooks/builtins/.

Behaviour

  • Args layout: [url, body?] — first arg is the target URL, optional second arg is the request body sent verbatim with Content-Type: application/json.
  • Empty URL is a no-op (lenient args contract, same posture as max_iterations).
  • Non-http(s) / scheme-less / host-less / unparseable URLs surface as a config error so on_error: warn flags the misconfig.
  • Network errors and non-2xx responses are logged at warn level and swallowed so a bad webhook never breaks the agent run loop.
  • Inherits the per-hook timeout via the executor's ctx wrap; client-side Timeout: 30s is a backstop.

Hooks that need fail-closed webhook semantics (billing guards, pre-tool-use gates) belong in a type: command entry that can shape the failure mode itself; this builtin trades fidelity for never being the reason an agent run dies.

Security hardening

  • SSRF: uses httpclient.NewSafeClient — refuses connections to non-public IPs at dial time (defeating DNS rebinding to loopback / RFC1918 / link-local incl. 169.254.169.254 cloud metadata) and bounds redirects at 10 hops.
  • Scheme allowlist: http/https only, parsed upfront with net/url. file://, ftp://, javascript:, etc. are rejected before any I/O.
  • Credential redaction: log output uses target.Redacted(), so webhook URLs with user:pass@host or secret-bearing query strings don't leak to logs.
  • Bounded drain: response body is drained through io.LimitReader(_, 64 KiB) so a malicious receiver returning a huge response can't pin the goroutine on an unbounded read.

Tests

Six tests covering the happy path, empty-body pings, no-op-on-empty-URL, swallowed server / network errors, scheme + host rejection, and prompt return on ctx deadline. TestMain swaps in the unsafe httpclient variant for the test binary so httptest.NewServer (which binds to 127.0.0.1) keeps working — production callers always go through the safe client.

@dgageot dgageot requested a review from a team as a code owner May 7, 2026 15:38
Copy link
Copy Markdown
Contributor

@aheritier aheritier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Tight, security-conscious primitive.

Strong points:

  • Correct use of httpclient.NewSafeClient — SSRF dial-time protection (loopback / RFC1918 / link-local incl. 169.254.169.254) and bounded 10-hop redirect chain.
  • Scheme allowlist + non-empty Host check rejects file://, javascript:, scheme-less, and host-less inputs.
  • Body drain capped at 64 KiB via io.LimitReader — DoS-safe.
  • target.Redacted() before logging — no userinfo leaked into logs.
  • Errors and non-2xx swallowed by design (fire-and-forget webhook), with slog.WarnContext so operators still see them.
  • Test-only export_test.go + TestMain to swap the SSRF dialer for httptest is the right pattern — the unsafe path doesn't compile into release binaries.
  • Test matrix is complete: happy path, empty body, no-op, swallowed network/HTTP errors, scheme rejection, ctx cancellation.

Non-blocking observations (no action needed):

  • Content-Type: application/json is hardcoded. Fine for the documented use case, but worth keeping in mind if a future caller needs application/x-www-form-urlencoded for legacy webhooks.
  • The "only http(s) URLs are supported" error message covers both parse failures and disallowed schemes. Slightly less precise than splitting them, but the test contract pins the substring so leave it.

@aheritier aheritier added kind/feat PR adds a new feature (maps to feat: commit prefix) area/agent For work that has to do with the general agent loop/agentic features of the app area/security Authentication, authorization, secrets, vulnerabilities effort:small Isolated change, clear solution, single area go Pull requests that update go code labels May 7, 2026
@docker-agent
Copy link
Copy Markdown

PR Review Failed — The review agent encountered an error and could not complete the review. View logs.

@dgageot dgageot merged commit 9d110ae into docker:main May 7, 2026
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/agent For work that has to do with the general agent loop/agentic features of the app area/security Authentication, authorization, secrets, vulnerabilities effort:small Isolated change, clear solution, single area go Pull requests that update go code kind/feat PR adds a new feature (maps to feat: commit prefix)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants