feat(webhooks): accept inbound media on POST /v1/webhooks/llm (sync mode)

[justintruong29]

POST /v1/webhooks/llm accepts text only today, so a webhook caller cannot send an image the way Telegram or the WebSocket client can. This adds an optional media[{url, filename}] array, sync mode only: the gateway downloads each URL to a temp file and hands local paths to agent.RunRequest.Media, where every other channel already converges.

Motivating case: a middleware sits between a platform callback (Zalo OA, Messenger) and the gateway, so the native channel is never in the path, and the platform's image URL has nowhere to go.

Why the agent side needs no change

RunRequest.Media []bus.MediaFile already exists and everything downstream is channel-agnostic — enrichInputMedia branches only on len(req.Media) > 0. The gap was entirely at the request boundary.

Design decisions

media[] by URL, not by path. Every existing request boundary (WS chat.go, MCP chat_runner.go) passes local paths because the caller pre-uploads via POST /v1/media/upload — which sits behind requireAuth needing a user/admin role, while a webhook caller holds only a wh_ bearer or an HMAC key. chat.go also passes item.Path straight into persistMedia unvalidated, which is safe only because that caller is an authenticated user. A caller-supplied path on a webhook would be an arbitrary local file read.

NewRedirectFollowingSafeClient, not NewSafeClient. The latter sets CheckRedirect to http.ErrUseLastResponse, so a presigned or CDN URL silently yields a 0-byte body with no error. The redirect-following client re-validates the resolved destination IP at every hop's dial, which also closes DNS rebinding. The test for this asserts on file size, not just err == nil — with the wrong client the failure is silent.

Sync only; mode=async + media[] returns 400. Not deferred work, a deliberate rejection: buildAuditPayload freezes requestPayload before the mode dispatch and handleAsync stores it verbatim, so server-computed local paths have no way to reach the worker. An accepted request would run with no media and no tag, then answer confidently about an image nobody sent, with status: "done" and nothing in last_error.

Media tags are load-bearing, not cosmetic. enrichImageIDs uses replaceFirstMediaTag — it rewrites an existing <media:image> tag and never inserts one. When an agent has a dedicated read_image provider (file-ref vision mode), images are not attached to the main LLM at all, so the tag is the model's only signal. No tag means a silent no-op: no error, no log, wrong answer — and only on that configuration, so it works on a dev box and fails at a customer.

Failure detail is a closed enum. SSRF errors embed the hostname and the resolved internal IP; url.Error embeds the caller's full URL plus "connection refused" vs "i/o timeout". Any of that reaching the response body or the prompt turns this endpoint into an internal-network and port-scanning oracle. Detail goes to slog only, and a reflection test pins that the failure struct has no free-text field.

Guards

• SSRF validation up front, plus per-redirect-hop dial re-validation.
• 10-item count cap — rejected, not truncated. Silent truncation reads as "we handled everything" when we did not.
• Content sniff cross-check against the declared Content-Type. That header is attacker-controlled and persistMedia trusts it verbatim to decide routing.
• 25 MB per file, matching the outbound /message cap so both directions of the API agree on one number.
• A 50 MP image gate read from the header via image.DecodeConfig before any full decode. SanitizeImage calls imaging.Open before checking dimensions, so ten 25 MB items each declaring 30000x30000 would decode to ~3.6 GB of RGBA. The gate also rejects an image/* file that does not decode at all, because SanitizeImage fails open — it logs and proceeds with the original bytes under the caller's declared MIME, which is fine for authenticated channels and not fine here.
• A 512 MB process-wide byte budget. The per-webhook rate limiter bounds none of this: allow() returns true whenever rpm <= 0 and RateLimitPerMin has no non-zero default, leaving a 600 rpm x 10 items x 25 MB worst case.

Semantics

All-or-nothing. Any failed item fails the whole request and the agent is never invoked. Partial success was dropped deliberately — the chat-channel precedent it would copy annotates the message for a human to read, and there is no human on a machine-to-machine API.

Deterministic status. Every item is attempted even after one fails, and the status comes from a fixed severity order (ssrf > mime_denied > too_large > download_failed > budget_exhausted) rather than from array position.

Cleanup is owned by the run. lane.Submit runs its closure in a detached goroutine and returns immediately; the handler then selects on the request-derived context while the run uses context.WithoutCancel. A defer in the handler would delete the files on client disconnect while ag.Run was still executing — persistMedia fails its copy, logs a warning, drops the ref, and the agent burns minutes answering about a tag with no image behind it. Cleanup therefore lives inside the closure, with the two paths that never reach it (a Submit error, the idempotency early return) cleaning up themselves. Each has a test.

One deadline covers download and run, so a sync call cannot outlive gateway.webhook_sync_timeout_sec.

URL query strings are stripped wherever the value is retained or forwarded — the audit row (request_payload is readable via the admin calls endpoint for 30 days), the <media:image url="..."> tag (which reaches the LLM provider and persisted session history), and every log line. A presigned URL's signature is a bearer credential.

Supporting changes

• security.RedactURL and security.ErrBlockedDial exported. The sentinel lets callers classify a blocked dial with errors.Is rather than matching error text — text matching is brittle in the unsafe direction, since a reworded message would silently downgrade a real SSRF block to a generic failure.
• The media MIME allowlist moves to internal/webhooks as one source of truth; the private copy in internal/http is deleted and probeMediaURL points at the exported map.
• One new i18n key across all five catalogs. catalog_ko.go also gains the three existing MsgWebhookMedia* keys it was missing.
• docs/webhooks.md: the media[] contract, a middleware integration guide, and the 413 row the /v1/webhooks/message error table has always been missing (the code has returned it since webhooks_message.go).

Testing

42 new tests — 25 in internal/webhooks, 17 in internal/http — all green under -race. go build ./..., go build -tags sqliteonly ./..., and go vet ./... clean on this base.

Tests that pin the non-obvious behaviour rather than just executing code: the redirect test asserts file size; the core wiring test asserts both RunRequest.Media and <media:image in the message, since asserting only the former passes while file-ref vision mode is silently broken; the disconnect test asserts from inside the agent stub that the file still exists after the handler has returned; and one test proves security.ErrBlockedDial survives the net.OpError → url.Error wrapping, which is what makes errors.Is viable there.

Surface parity: ui/web N/A — the admin test endpoint (POST /v1/webhooks/{id}/test) and its UI dialog stay text-only in this change, stated in docs/webhooks.md. No schema change, so no migration on either PG or SQLite. The only public-contract additions are the two exported internal/security symbols and the optional media request field.

Out of scope

Outbound media in the sync response (webhookLLMSyncResp drops RunResult.Media), media[] on the admin test endpoint, and async support. Async is documented as rejected rather than deferred, with the reasoning kept so the same design is not rebuilt.

Known limitation

A caller that omits user_id lands in <workspace>/<agent_key>/.uploads/, shared with every other such caller. Documented, not changed — it is pre-existing behaviour of the upload path, not introduced here.

[clark-cant]

🔍 Maintainer Review (github-maintain cron)

Verdict: Approve — well-engineered feature with strong security posture.

Mandatory Gates

• Duplicate/prior implementation: Clear — no duplicate PR or issue found for webhook inbound media.
• Project standards: Follows existing webhook handler patterns, SSRF policy, i18n catalog conventions, and audit payload structure.
• Strategic necessity: Clear value — closes a real gap where webhook callers (Zalo OA, Messenger middleware) cannot send images the way Telegram/WebSocket clients already can.

Assessment

Strengths:

• SSRF defense-in-depth: per-redirect-hop dial re-validation, DNS rebinding protection, closed-enum failure responses (no oracle)
• All-or-nothing semantics with deterministic severity-ordered status — correct for M2M API
• 50MP image header gate before full decode prevents memory bomb (10×25MB×30000×30000 → 3.6GB RGBA)
• Process-wide 512MB byte budget with proper reserve/commit/release lifecycle
• Async+media correctly rejected (400) rather than silently broken
• URL query string stripping in audit/media-tag/logs protects presigned URL credentials
• Media tag injection is load-bearing for file-ref vision mode — good catch
• Cleanup ownership correctly placed inside lane closure (not handler defer)
• 42 new tests with race-clean, including non-obvious behavioral assertions (file size on redirect, both RunRequest.Media AND media tag, disconnect-doesnt-delete)

Risk level: Medium (scope is 15 files / +2252 lines, but well-scoped to the feature; no breaking changes to existing contracts)

No Critical or Important findings. The PR body demonstrates exceptional attention to security edge cases and failure modes. Documentation in docs/webhooks.md is thorough and integrator-friendly.

Recommendation: Safe to merge. CI is green, mergeState is CLEAN, no conflicting reviews.

Posted by github-maintain automation — 2026-08-27T12:40Z

[clark-cant]

Review\n\nImportant — idempotent retries fetch media before the idempotency reservation.\n\nhandle calls FetchInboundMedia before handleSync, but reserveIdempotentCall runs only inside handleSync. Consequently, a duplicate request with the same idempotency key re-downloads every media[] URL before replaying the already-completed response. If the original request used a short-lived signed URL, a legitimate retry can now fail during the fetch (or consume the global download budget) instead of returning the stored result. It also repeats a caller-controlled outbound request despite idempotency.\n\nPlease move the idempotency reservation/replay decision ahead of media fetching, or otherwise ensure a matching stored completed call is returned before any URL fetch. Preserve cleanup ownership for only the newly reserved run and add regression coverage proving an idempotent replay neither calls the media fetcher nor requires the original URL to remain reachable.\n\nThe SSRF, MIME, size, redirect, cleanup, and deadline work is thoughtful; CI is green, but this retry-contract regression needs resolution before merge.