From fc7f48b9a3f9162a8e19277f4b8f07cb494aae29 Mon Sep 17 00:00:00 2001 From: Guilherme Gomes Date: Tue, 21 Jul 2026 19:29:09 -0300 Subject: [PATCH] fix(EVO-2178): take the media host allowlist from config, not from the event MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes a regression I introduced in #6. The allowlist was anchored on the host of the event's postback_url, which never matches the host the CRM actually signs media URLs with: postback_url comes from BOT_RUNTIME_POSTBACK_BASE_URL (internal DNS, "evo-crm" in the shipped compose) while the URL is built from ACTIVE_STORAGE_URL, falling back to BACKEND_URL — which production requires to be a public host. So on develop every attachment was rejected as blocked_url and the agent stopped seeing images: the EVO-2178 bug, back. Anchoring on the event was also the wrong shape for the guard. Whoever sends the event chooses every field in it, including the one being used to decide what that same event may reach, so the check constrained nobody it needed to. Reading MEDIA_HOST_ALLOWLIST only puts the decision with the operator, where it cannot be chosen by the caller. A2ARequest.PostbackURL is dropped again. The scheme check and the per-redirect re-check are unchanged. This makes the variable required wherever media is expected: unset means no attachment is fetched. The deploy surfaces are wired up in the umbrella PR; k8s/configmap.yaml and k8s/deployment.yaml carry it here. --- .env.example | 13 ++++++++----- k8s/configmap.yaml | 3 +++ k8s/deployment.yaml | 5 +++++ pkg/ai/model/a2a.go | 3 --- pkg/ai/service/ai_adapter.go | 20 ++++++++------------ pkg/ai/service/ai_adapter_media_test.go | 19 ++++++++++++------- pkg/ai/service/ai_adapter_ssrf_test.go | 14 +++++++------- pkg/pipeline/service/pipeline_service.go | 1 - 8 files changed, 43 insertions(+), 35 deletions(-) diff --git a/.env.example b/.env.example index b16533e..a2b6115 100644 --- a/.env.example +++ b/.env.example @@ -7,9 +7,12 @@ BOT_RUNTIME_SECRET= AI_CALL_TIMEOUT_SECONDS=30 -# Optional. Extra hosts allowed to serve incoming media, comma-separated, no scheme -# or port (e.g. "minio.internal,cdn.example.com"). By default only the host of the -# event's postback_url is allowed. Set this when blobs are served elsewhere -# (ActiveStorage redirect mode, CDN); media on an unlisted host is skipped and -# logged as pipeline.ai.attachment.blocked_url, and the text reply still goes out. +# Required for incoming media. Hosts allowed to serve it, comma-separated, no +# scheme or port (e.g. "crm.example.com,minio.internal"). +# +# Set it to the host of the CRM's BACKEND_URL — that is what signs the attachment +# URLs — plus the storage host when ActiveStorage runs in redirect mode, or the CDN +# host. Leave it empty and no media reaches the agent: an attachment on an unlisted +# host is skipped and logged as pipeline.ai.attachment.blocked_url, and only the +# text reply goes out. MEDIA_HOST_ALLOWLIST= diff --git a/k8s/configmap.yaml b/k8s/configmap.yaml index 752820d..d1bee04 100644 --- a/k8s/configmap.yaml +++ b/k8s/configmap.yaml @@ -6,3 +6,6 @@ data: LISTEN_ADDR: ":8080" AI_PROCESSOR_URL: "http://ai-processor:8000" AI_CALL_TIMEOUT_SECONDS: "30" + # Hosts allowed to serve incoming media, comma-separated (no scheme/port). + # Must include the host of the CRM's BACKEND_URL, or no media reaches the agent. + MEDIA_HOST_ALLOWLIST: "" diff --git a/k8s/deployment.yaml b/k8s/deployment.yaml index df51e7f..4695513 100644 --- a/k8s/deployment.yaml +++ b/k8s/deployment.yaml @@ -44,6 +44,11 @@ spec: configMapKeyRef: name: evo-bot-runtime-config key: AI_CALL_TIMEOUT_SECONDS + - name: MEDIA_HOST_ALLOWLIST + valueFrom: + configMapKeyRef: + name: evo-bot-runtime-config + key: MEDIA_HOST_ALLOWLIST # Secrets - name: REDIS_URL valueFrom: diff --git a/pkg/ai/model/a2a.go b/pkg/ai/model/a2a.go index 64d0a62..5e6de87 100644 --- a/pkg/ai/model/a2a.go +++ b/pkg/ai/model/a2a.go @@ -10,9 +10,6 @@ type A2ARequest struct { Message string // aggregated buffer content (FR-15) Metadata map[string]any // CRM metadata passed through to processor (tools context) Attachments []Attachment // EVO-2180: incoming media to forward as A2A file parts - // PostbackURL anchors the media host allowlist (see allowedMediaHosts); it is - // not used for the A2A call itself. - PostbackURL string } // Attachment is an incoming media item (image/audio/…) the adapter downloads and diff --git a/pkg/ai/service/ai_adapter.go b/pkg/ai/service/ai_adapter.go index 21deacd..a144be5 100644 --- a/pkg/ai/service/ai_adapter.go +++ b/pkg/ai/service/ai_adapter.go @@ -50,9 +50,10 @@ const ( attachmentsTotalTimeFactor = 3 ) -// mediaHostAllowlistEnv names extra hosts authorized to serve incoming media, -// comma-separated, for deployments serving blobs off the CRM host (ActiveStorage -// redirect mode, CDN). The default anchor is the event's own postback host. +// mediaHostAllowlistEnv names the hosts authorized to serve incoming media, +// comma-separated. It is the *only* source: an allowlist read out of the event +// would be chosen by whoever sent the event, which is exactly who it must +// constrain. Unset means no media is fetched. const mediaHostAllowlistEnv = "MEDIA_HOST_ALLOWLIST" // maxBackoff caps the exponential backoff between retries so a large @@ -340,7 +341,7 @@ func (a *aiAdapter) buildFileParts(ctx context.Context, req *model.A2ARequest) [ defer cancelBudget() // Built once per turn: the client closes over the authorized hosts. - hosts := allowedMediaHosts(req.PostbackURL) + hosts := allowedMediaHosts() client := a.mediaClient(hosts) parts := make([]model.JSONRPCPart, 0, len(req.Attachments)) @@ -355,7 +356,7 @@ func (a *aiAdapter) buildFileParts(ctx context.Context, req *model.A2ARequest) [ "conversation_id", req.ConversationID, "file_type", att.FileType, "error", err, - "hint", "set "+mediaHostAllowlistEnv+" when blobs are served off the CRM host", + "hint", "add the host to "+mediaHostAllowlistEnv, ) continue } @@ -435,16 +436,11 @@ func (a *aiAdapter) attachmentTimeout() time.Duration { return d } -// allowedMediaHosts returns the hostnames authorized to serve this event's media. +// allowedMediaHosts returns the hostnames authorized to serve incoming media. // An empty set authorizes nothing: failing closed beats fetching a URL the caller // chose. -func allowedMediaHosts(postbackURL string) map[string]struct{} { +func allowedMediaHosts() map[string]struct{} { hosts := make(map[string]struct{}, 2) - if u, err := neturl.Parse(postbackURL); err == nil { - if h := strings.ToLower(u.Hostname()); h != "" { - hosts[h] = struct{}{} - } - } for _, h := range strings.Split(os.Getenv(mediaHostAllowlistEnv), ",") { if h = strings.ToLower(strings.TrimSpace(h)); h != "" { hosts[h] = struct{}{} diff --git a/pkg/ai/service/ai_adapter_media_test.go b/pkg/ai/service/ai_adapter_media_test.go index 6957953..329387e 100644 --- a/pkg/ai/service/ai_adapter_media_test.go +++ b/pkg/ai/service/ai_adapter_media_test.go @@ -40,6 +40,7 @@ func procServer(t *testing.T, capture *[]aiModel.JSONRPCPart) *httptest.Server { } func TestCall_ForwardsAttachmentAsFilePart(t *testing.T) { + t.Setenv("MEDIA_HOST_ALLOWLIST", "127.0.0.1") imgBytes := []byte("\x89PNG\r\n-fake-image-bytes") fileSrv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.Header().Set("Content-Type", "image/png") @@ -54,7 +55,6 @@ func TestCall_ForwardsAttachmentAsFilePart(t *testing.T) { adapter := aiService.NewAIAdapter(30, 0, 1) _, err := adapter.Call(context.Background(), &aiModel.A2ARequest{ OutgoingURL: proc.URL + "/api/v1/a2a/agent-1", - PostbackURL: proc.URL, ContactID: 1, ConversationID: 2, ApiKey: "k", @@ -92,6 +92,7 @@ func TestCall_ForwardsAttachmentAsFilePart(t *testing.T) { } func TestCall_AttachmentDownloadFailure_SendsTextOnly(t *testing.T) { + t.Setenv("MEDIA_HOST_ALLOWLIST", "127.0.0.1") var parts []aiModel.JSONRPCPart proc := procServer(t, &parts) defer proc.Close() @@ -99,7 +100,6 @@ func TestCall_AttachmentDownloadFailure_SendsTextOnly(t *testing.T) { adapter := aiService.NewAIAdapter(30, 0, 1) _, err := adapter.Call(context.Background(), &aiModel.A2ARequest{ OutgoingURL: proc.URL + "/api/v1/a2a/agent-1", - PostbackURL: proc.URL, ContactID: 1, ConversationID: 2, ApiKey: "k", @@ -143,6 +143,7 @@ func filePartsOf(parts []aiModel.JSONRPCPart) []aiModel.JSONRPCPart { // gateway's client_max_body_size. Over budget the extra media is dropped and the // call still goes out. func TestCall_TotalAttachmentBudget_DropsExcessAndStillSends(t *testing.T) { + t.Setenv("MEDIA_HOST_ALLOWLIST", "127.0.0.1") var hits int32 chunk := make([]byte, 6<<20) // 6 MiB each: 4 of them exceed the 20 MiB budget fileSrv := mediaServer("image/png", chunk, &hits) @@ -161,7 +162,7 @@ func TestCall_TotalAttachmentBudget_DropsExcessAndStillSends(t *testing.T) { adapter := aiService.NewAIAdapter(30, 0, 1) if _, err := adapter.Call(context.Background(), &aiModel.A2ARequest{ - OutgoingURL: proc.URL, PostbackURL: proc.URL, ContactID: 1, ConversationID: 2, Message: "album", Attachments: atts, + OutgoingURL: proc.URL, ContactID: 1, ConversationID: 2, Message: "album", Attachments: atts, }); err != nil { t.Fatalf("a media budget overflow must not error the call: %v", err) } @@ -178,6 +179,7 @@ func TestCall_TotalAttachmentBudget_DropsExcessAndStillSends(t *testing.T) { // An unreachable media host must not hold the turn hostage: the whole set shares a // time budget, so latency stays bounded no matter how many attachments arrive. func TestCall_AttachmentTimeBudget_IsBounded(t *testing.T) { + t.Setenv("MEDIA_HOST_ALLOWLIST", "127.0.0.1") hung := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { <-r.Context().Done() })) @@ -197,7 +199,7 @@ func TestCall_AttachmentTimeBudget_IsBounded(t *testing.T) { adapter := aiService.NewAIAdapter(1, 0, 1) start := time.Now() if _, err := adapter.Call(context.Background(), &aiModel.A2ARequest{ - OutgoingURL: proc.URL, PostbackURL: proc.URL, ContactID: 1, ConversationID: 2, Message: "hi", Attachments: atts, + OutgoingURL: proc.URL, ContactID: 1, ConversationID: 2, Message: "hi", Attachments: atts, }); err != nil { t.Fatalf("unreachable media must not error the call: %v", err) } @@ -213,6 +215,7 @@ func TestCall_AttachmentTimeBudget_IsBounded(t *testing.T) { // A Rails proxy URL that answers 200 with an error/login page must not be // forwarded as an image: the processor passes the mime straight into the model call. func TestCall_HTMLResponse_IsNotForwardedAsMedia(t *testing.T) { + t.Setenv("MEDIA_HOST_ALLOWLIST", "127.0.0.1") var hits int32 htmlSrv := mediaServer("text/html; charset=utf-8", []byte("login"), &hits) defer htmlSrv.Close() @@ -223,7 +226,7 @@ func TestCall_HTMLResponse_IsNotForwardedAsMedia(t *testing.T) { adapter := aiService.NewAIAdapter(30, 0, 1) if _, err := adapter.Call(context.Background(), &aiModel.A2ARequest{ - OutgoingURL: proc.URL, PostbackURL: proc.URL, ContactID: 1, ConversationID: 2, Message: "hi", + OutgoingURL: proc.URL, ContactID: 1, ConversationID: 2, Message: "hi", Attachments: []aiModel.Attachment{{URL: htmlSrv.URL + "/photo.jpg", ContentType: "image/jpeg", FileType: "image"}}, }); err != nil { t.Fatalf("Call: %v", err) @@ -238,6 +241,7 @@ func TestCall_HTMLResponse_IsNotForwardedAsMedia(t *testing.T) { // resolves to an opaque blob is dropped rather than sent as octet-stream, which the // model APIs reject. func TestCall_MimeTypeResolution(t *testing.T) { + t.Setenv("MEDIA_HOST_ALLOWLIST", "127.0.0.1") cases := []struct { name string respContentType string @@ -262,7 +266,7 @@ func TestCall_MimeTypeResolution(t *testing.T) { adapter := aiService.NewAIAdapter(30, 0, 1) if _, err := adapter.Call(context.Background(), &aiModel.A2ARequest{ - OutgoingURL: proc.URL, PostbackURL: proc.URL, ContactID: 1, ConversationID: 2, Message: "hi", + OutgoingURL: proc.URL, ContactID: 1, ConversationID: 2, Message: "hi", Attachments: []aiModel.Attachment{{URL: srv.URL + tc.urlPath, ContentType: tc.declared, FileType: "image"}}, }); err != nil { t.Fatalf("Call: %v", err) @@ -286,6 +290,7 @@ func TestCall_MimeTypeResolution(t *testing.T) { // A single file over the per-attachment cap is skipped, and the text still goes out. func TestCall_OversizeAttachment_SendsTextOnly(t *testing.T) { + t.Setenv("MEDIA_HOST_ALLOWLIST", "127.0.0.1") var hits int32 srv := mediaServer("image/png", make([]byte, (15<<20)+1), &hits) defer srv.Close() @@ -296,7 +301,7 @@ func TestCall_OversizeAttachment_SendsTextOnly(t *testing.T) { adapter := aiService.NewAIAdapter(30, 0, 1) if _, err := adapter.Call(context.Background(), &aiModel.A2ARequest{ - OutgoingURL: proc.URL, PostbackURL: proc.URL, ContactID: 1, ConversationID: 2, Message: "hi", + OutgoingURL: proc.URL, ContactID: 1, ConversationID: 2, Message: "hi", Attachments: []aiModel.Attachment{{URL: srv.URL + "/big.png", ContentType: "image/png", FileType: "image"}}, }); err != nil { t.Fatalf("an oversize attachment must not error the call: %v", err) diff --git a/pkg/ai/service/ai_adapter_ssrf_test.go b/pkg/ai/service/ai_adapter_ssrf_test.go index 6f8f79f..ab693f0 100644 --- a/pkg/ai/service/ai_adapter_ssrf_test.go +++ b/pkg/ai/service/ai_adapter_ssrf_test.go @@ -42,6 +42,7 @@ func fileParts(parts []aiModel.JSONRPCPart) []aiModel.JSONRPCPart { // The exfiltration shape: internal attachment URL, attacker-chosen outgoing_url. func TestCall_ForeignHostAttachment_IsNotFetched(t *testing.T) { + t.Setenv("MEDIA_HOST_ALLOWLIST", "evo-crm.internal") var reached bool internal := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { reached = true @@ -57,8 +58,6 @@ func TestCall_ForeignHostAttachment_IsNotFetched(t *testing.T) { adapter := aiService.NewAIAdapter(30, 0, 1) if _, err := adapter.Call(context.Background(), &aiModel.A2ARequest{ OutgoingURL: proc.URL, ContactID: 1, ConversationID: 2, Message: "hi", - // A CRM on a host that does not serve the attachment below. - PostbackURL: "http://evo-crm.internal:3000/webhooks/bot_runtime/postback/7", Attachments: []aiModel.Attachment{{URL: internal.URL + "/latest/meta-data/", ContentType: "image/png"}}, }); err != nil { t.Fatalf("a blocked attachment must not error the call: %v", err) @@ -91,7 +90,6 @@ func TestCall_AllowlistedHost_IsFetched(t *testing.T) { adapter := aiService.NewAIAdapter(30, 0, 1) if _, err := adapter.Call(context.Background(), &aiModel.A2ARequest{ OutgoingURL: proc.URL, ContactID: 1, ConversationID: 2, Message: "hi", - PostbackURL: "http://evo-crm.internal:3000/webhooks/bot_runtime/postback/7", Attachments: []aiModel.Attachment{{URL: blob.URL + "/photo.png", ContentType: "image/png"}}, }); err != nil { t.Fatalf("Call: %v", err) @@ -109,6 +107,8 @@ func TestCall_AllowlistedHost_IsFetched(t *testing.T) { // Checking the declared URL alone is not enough: a 302 could walk it internal. func TestCall_RedirectOffTheAuthorizedHost_IsRefused(t *testing.T) { + // Authorizes 127.0.0.1 (the redirector) but not "localhost" (the target). + t.Setenv("MEDIA_HOST_ALLOWLIST", "127.0.0.1") var reached bool internal := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { reached = true @@ -131,8 +131,6 @@ func TestCall_RedirectOffTheAuthorizedHost_IsRefused(t *testing.T) { adapter := aiService.NewAIAdapter(30, 0, 1) if _, err := adapter.Call(context.Background(), &aiModel.A2ARequest{ OutgoingURL: proc.URL, ContactID: 1, ConversationID: 2, Message: "hi", - // Authorizes 127.0.0.1 (the redirector) but not "localhost" (the target). - PostbackURL: redirector.URL, Attachments: []aiModel.Attachment{{URL: redirector.URL + "/photo.png", ContentType: "image/png"}}, }); err != nil { t.Fatalf("a refused redirect must not error the call: %v", err) @@ -148,6 +146,7 @@ func TestCall_RedirectOffTheAuthorizedHost_IsRefused(t *testing.T) { // Only http/https may be addressed. func TestCall_NonHTTPScheme_IsRejected(t *testing.T) { + t.Setenv("MEDIA_HOST_ALLOWLIST", "127.0.0.1") var parts []aiModel.JSONRPCPart proc := capturingProc(&parts) defer proc.Close() @@ -155,7 +154,6 @@ func TestCall_NonHTTPScheme_IsRejected(t *testing.T) { adapter := aiService.NewAIAdapter(30, 0, 1) if _, err := adapter.Call(context.Background(), &aiModel.A2ARequest{ OutgoingURL: proc.URL, ContactID: 1, ConversationID: 2, Message: "hi", - PostbackURL: proc.URL, Attachments: []aiModel.Attachment{{URL: "file:///etc/passwd", ContentType: "image/png"}}, }); err != nil { t.Fatalf("Call: %v", err) @@ -165,7 +163,9 @@ func TestCall_NonHTTPScheme_IsRejected(t *testing.T) { } } -// Nothing authorized means nothing fetched — failing closed is the point. +// Nothing authorized means nothing fetched — failing closed is the point. This is +// also what an unconfigured deployment gets, which is why MEDIA_HOST_ALLOWLIST has +// to be shipped alongside the service. func TestCall_NoAuthorizedHost_ForwardsNothing(t *testing.T) { var reached bool blob := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { diff --git a/pkg/pipeline/service/pipeline_service.go b/pkg/pipeline/service/pipeline_service.go index 38b1474..63c610a 100644 --- a/pkg/pipeline/service/pipeline_service.go +++ b/pkg/pipeline/service/pipeline_service.go @@ -398,7 +398,6 @@ func (s *pipelineService) runAIStage(ctx context.Context, contactID, conversatio Message: buffer, Metadata: metadata, Attachments: aiAttachments, - PostbackURL: postbackURL, // anchors the media host allowlist }) if err != nil { switch {