From 273fd83643d06651098f2fcd6d728fbd39a898f1 Mon Sep 17 00:00:00 2001 From: Guilherme Gomes Date: Tue, 21 Jul 2026 17:02:58 -0300 Subject: [PATCH 1/2] fix(EVO-2178): validate incoming media URLs before fetching them Review follow-up to EVO-2180. Attachment URLs arrive inside the /events payload and the adapter fetched them verbatim, so the endpoint doubled as a read primitive aimed by its caller: the bytes of any URL reachable from this service were base64-encoded into the A2A call, whose destination (outgoing_url) comes from the same payload. Reproduced end to end against a local metadata-style endpoint, both directly and through a 302. - checkMediaURL pins the scheme to http/https and requires the host to be one the CRM is known to serve blobs from: the postback URL's host (already mandatory in MessageEvent.Validate, so no new config for the default topology) plus whatever MEDIA_HOST_ALLOWLIST names, for deployments serving blobs off an S3/MinIO/CDN host. Unauthorized media is skipped and logged; the text reply is unaffected, like every other media failure here. - The download client re-runs that check on every redirect hop, so an authorized host cannot walk the fetch onto an internal address. - BOT_RUNTIME_SECRET becomes required. It was read with os.Getenv, and SecretMiddleware compares the header against it, so an empty value authenticated every caller that simply omitted the header. - The media buffer key gets a TTL. ClearState remains the normal cleanup; the TTL only stops a turn that dies before reaching it from leaving media URLs in Redis forever. - Attachment download failures now log the HTTP status: the common production case is a 404 from a signed link that expired while the queue was backed up, and it read identically to an unreachable host. - Adds .github/workflows/ci.yml. Nothing ran the Go suite on a PR, which is how test/e2e stayed non-compiling from EVO-558 until EVO-2180. --- .env.example | 17 ++ .github/workflows/ci.yml | 42 ++++ internal/config/config.go | 10 +- pkg/ai/model/a2a.go | 4 + pkg/ai/service/ai_adapter.go | 127 ++++++++++- pkg/ai/service/ai_adapter_media_test.go | 12 +- pkg/ai/service/ai_adapter_ssrf_test.go | 203 ++++++++++++++++++ .../repository/redis_pipeline_repository.go | 16 +- pkg/pipeline/service/pipeline_service.go | 3 + 9 files changed, 420 insertions(+), 14 deletions(-) create mode 100644 .github/workflows/ci.yml create mode 100644 pkg/ai/service/ai_adapter_ssrf_test.go diff --git a/.env.example b/.env.example index ea13f0d..1df8729 100644 --- a/.env.example +++ b/.env.example @@ -1,4 +1,21 @@ LISTEN_ADDR=:8090 REDIS_URL=redis://localhost:6379 + +# Required. Shared with the CRM, which sends it as X-Bot-Runtime-Secret on /events. +# An empty value would authenticate every caller that omits the header, so the +# service refuses to start without it. Use a value unique to the deployment. BOT_RUNTIME_SECRET= + AI_CALL_TIMEOUT_SECONDS=30 + +# Optional. Extra hostnames authorized to serve incoming media, comma-separated, +# without scheme or port (e.g. "minio.internal,cdn.example.com"). +# +# Incoming attachments are downloaded by this service from a URL that arrives in the +# /events payload, so the host is validated before the fetch: by default only the +# host of that event's postback_url (the CRM) is allowed. Set this when blobs are +# served from somewhere else — ActiveStorage in redirect mode (ATTACHMENT_DELIVERY= +# redirect) hands out presigned S3/MinIO links, and a CDN in front of the CRM is the +# other common case. Media on an unlisted host is skipped and logged as +# pipeline.ai.attachment.blocked_url; the text reply still goes out. +MEDIA_HOST_ALLOWLIST= diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml new file mode 100644 index 0000000..90a754a --- /dev/null +++ b/.github/workflows/ci.yml @@ -0,0 +1,42 @@ +name: CI + +# Until now the only workflow here was docker-publish.yml, so nothing ran the Go +# suite on a PR: `test/e2e` sat non-compiling from EVO-558 to EVO-2180 without a +# single red check. The repository tests need Redis, which is why this runs it as a +# service container rather than skipping the packages that touch it. + +on: + pull_request: + push: + branches: [develop, main] + +jobs: + test: + runs-on: ubuntu-latest + services: + redis: + image: redis:7-alpine + ports: ['6379:6379'] + options: >- + --health-cmd "redis-cli ping" + --health-interval 5s + --health-timeout 3s + --health-retries 10 + env: + REDIS_TEST_URL: redis://localhost:6379 + steps: + - uses: actions/checkout@v4 + + - uses: actions/setup-go@v5 + with: + go-version-file: go.mod + cache: true + + - name: Build + run: go build ./... + + - name: Vet + run: go vet ./... + + - name: Test + run: go test ./... diff --git a/internal/config/config.go b/internal/config/config.go index 76592f4..f81f024 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -27,7 +27,15 @@ func Load() (*Config, error) { if err != nil { return nil, err } - botRuntimeSecret := os.Getenv("BOT_RUNTIME_SECRET") + // Required, not optional: SecretMiddleware compares the header against this + // value, so an empty secret authenticates every caller that simply omits the + // header. /events accepts an outgoing_url and (since EVO-2180) attachment URLs + // this service fetches itself, which makes an unauthenticated endpoint an + // outbound-request gadget rather than just a spam vector. + botRuntimeSecret, err := mustGetEnv("BOT_RUNTIME_SECRET") + if err != nil { + return nil, err + } aiCallTimeout, err := getEnvIntOrDefault("AI_CALL_TIMEOUT_SECONDS", 30) if err != nil { return nil, err diff --git a/pkg/ai/model/a2a.go b/pkg/ai/model/a2a.go index 5e6de87..7233b51 100644 --- a/pkg/ai/model/a2a.go +++ b/pkg/ai/model/a2a.go @@ -10,6 +10,10 @@ 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 is the CRM base this event came from. It is not used for the A2A + // call itself — it anchors the host allowlist that decides which attachment URLs + // may be downloaded (see allowedMediaHosts). + 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 65b5b6d..dc6374a 100644 --- a/pkg/ai/service/ai_adapter.go +++ b/pkg/ai/service/ai_adapter.go @@ -13,6 +13,7 @@ import ( "mime" "net/http" neturl "net/url" + "os" "path" "strings" "time" @@ -49,6 +50,23 @@ const ( attachmentsTotalTimeFactor = 3 ) +// mediaHostAllowlistEnv names the extra hosts authorized to serve incoming media. +// +// Attachment URLs arrive inside the /events payload and downloading one is a request +// this service makes from inside the network, so an unvalidated URL turns /events +// into an SSRF gadget: the fetched bytes are handed straight to the outgoing_url +// that came in the same payload. The media is served by the CRM that sent the event, +// so the postback host — already mandatory in MessageEvent.Validate — is the natural +// anchor and needs no new configuration. +// +// This variable is the escape hatch for deployments that serve blobs from somewhere +// else: ActiveStorage in redirect mode (ATTACHMENT_DELIVERY=redirect) hands out +// presigned S3/MinIO links, and a CDN in front of the CRM is the other common case. +// Comma-separated hostnames, no scheme and no port, e.g. +// "minio.internal,cdn.example.com". An attachment on any other host is skipped with +// a log line and the text reply still goes out. +const mediaHostAllowlistEnv = "MEDIA_HOST_ALLOWLIST" + // maxBackoff caps the exponential backoff between retries so a large // AI_CALL_RETRY_BASE_MS or retry count cannot balloon the wait. const maxBackoff = 5 * time.Second @@ -333,12 +351,28 @@ func (a *aiAdapter) buildFileParts(ctx context.Context, req *model.A2ARequest) [ budgetCtx, cancelBudget := context.WithTimeout(ctx, attachmentsTotalTimeFactor*perDownload) defer cancelBudget() + // Built once per turn, not per attachment: it carries the authorized hosts into + // the redirect check, so a 302 off the CRM cannot walk the download onto an + // internal address. + hosts := allowedMediaHosts(req.PostbackURL) + client := a.mediaClient(hosts) + parts := make([]model.JSONRPCPart, 0, len(req.Attachments)) remaining := maxAttachmentsTotalBytes for i, att := range req.Attachments { if att.URL == "" { continue } + if err := checkMediaURL(att.URL, hosts); err != nil { + slog.Warn("pipeline.ai.attachment.blocked_url", + "contact_id", req.ContactID, + "conversation_id", req.ConversationID, + "file_type", att.FileType, + "error", err, + "hint", "set "+mediaHostAllowlistEnv+" when blobs are served off the CRM host", + ) + continue + } if budgetCtx.Err() != nil { slog.Warn("pipeline.ai.attachment.budget_exhausted", "contact_id", req.ContactID, @@ -360,12 +394,18 @@ func (a *aiAdapter) buildFileParts(ctx context.Context, req *model.A2ARequest) [ ) break } - data, respContentType, err := a.downloadAttachment(budgetCtx, att.URL, perDownload, limit) + data, respContentType, err := downloadAttachment(budgetCtx, client, att.URL, perDownload, limit) if err != nil { + // The status is logged separately because the most common production + // failure is a 404 from an expired signed link (the CRM mints it with a + // 15-minute TTL at delegation time, and a backed-up queue can outlive + // that). Without it "download_failed" reads the same as an unreachable + // host, and the customer just sees the agent ignore the image. slog.Warn("pipeline.ai.attachment.download_failed", "contact_id", req.ContactID, "conversation_id", req.ConversationID, "file_type", att.FileType, + "status", statusOf(err), "error", err, ) continue @@ -412,10 +452,83 @@ func (a *aiAdapter) attachmentTimeout() time.Duration { return d } -// downloadAttachment GETs the URL with the adapter's client and the given timeout, -// reading at most limit bytes. It returns the body and the response Content-Type so -// the caller can decide what the bytes actually are. -func (a *aiAdapter) downloadAttachment(ctx context.Context, url string, timeout time.Duration, limit int) ([]byte, string, error) { +// allowedMediaHosts returns the hostnames authorized to serve this event's media: +// the host of the event's own postback URL, plus whatever mediaHostAllowlistEnv +// names. An empty set means nothing is authorized and every attachment is skipped — +// failing closed is deliberate, because the alternative is fetching an +// attacker-chosen URL from inside the network. +func allowedMediaHosts(postbackURL string) 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{}{} + } + } + return hosts +} + +// checkMediaURL reports why a media URL must not be fetched, or nil when it may be. +// Scheme is pinned to http/https so the URL cannot address another protocol handler, +// and the host must be one the CRM is known to serve blobs from. +func checkMediaURL(rawURL string, hosts map[string]struct{}) error { + u, err := neturl.Parse(rawURL) + if err != nil { + return fmt.Errorf("unparseable media url: %w", err) + } + if scheme := strings.ToLower(u.Scheme); scheme != "http" && scheme != "https" { + return fmt.Errorf("scheme %q is not allowed for media", u.Scheme) + } + host := strings.ToLower(u.Hostname()) + if host == "" { + return errors.New("media url has no host") + } + if _, ok := hosts[host]; !ok { + return fmt.Errorf("host %q is not authorized to serve media for this event", host) + } + return nil +} + +// mediaClient is the client used for attachment downloads. It shares the adapter's +// transport (connection pool) but re-runs checkMediaURL on every redirect hop: an +// allowlisted host that answers 302 must not be able to walk the download onto a +// link-local or internal address. +func (a *aiAdapter) mediaClient(hosts map[string]struct{}) *http.Client { + return &http.Client{ + Transport: a.client.Transport, + CheckRedirect: func(req *http.Request, via []*http.Request) error { + if len(via) >= 10 { + return errors.New("stopped after 10 redirects") + } + return checkMediaURL(req.URL.String(), hosts) + }, + } +} + +// httpStatusError carries the status of a non-200 media response so the caller can +// log it without re-parsing the message. +type httpStatusError struct{ status int } + +func (e *httpStatusError) Error() string { return fmt.Sprintf("unexpected status %d", e.status) } + +// statusOf extracts the HTTP status from a download error, or 0 when the request +// never got a response (DNS failure, refused connection, timeout, blocked redirect). +func statusOf(err error) int { + var se *httpStatusError + if errors.As(err, &se) { + return se.status + } + return 0 +} + +// downloadAttachment GETs the URL with the given client and timeout, reading at most +// limit bytes. It returns the body and the response Content-Type so the caller can +// decide what the bytes actually are. +func downloadAttachment(ctx context.Context, client *http.Client, url string, timeout time.Duration, limit int) ([]byte, string, error) { dlCtx, cancel := context.WithTimeout(ctx, timeout) defer cancel() @@ -423,13 +536,13 @@ func (a *aiAdapter) downloadAttachment(ctx context.Context, url string, timeout if err != nil { return nil, "", fmt.Errorf("new_request: %w", err) } - resp, err := a.client.Do(httpReq) + resp, err := client.Do(httpReq) if err != nil { return nil, "", fmt.Errorf("do: %w", err) } defer resp.Body.Close() if resp.StatusCode != http.StatusOK { - return nil, "", fmt.Errorf("unexpected status %d", resp.StatusCode) + return nil, "", &httpStatusError{status: resp.StatusCode} } // +1 so an exactly-at-cap read is distinguishable from an oversize one. data, err := io.ReadAll(io.LimitReader(resp.Body, int64(limit)+1)) diff --git a/pkg/ai/service/ai_adapter_media_test.go b/pkg/ai/service/ai_adapter_media_test.go index def62d5..6957953 100644 --- a/pkg/ai/service/ai_adapter_media_test.go +++ b/pkg/ai/service/ai_adapter_media_test.go @@ -54,6 +54,7 @@ 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", @@ -98,6 +99,7 @@ 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", @@ -159,7 +161,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, ContactID: 1, ConversationID: 2, Message: "album", Attachments: atts, + OutgoingURL: proc.URL, PostbackURL: 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) } @@ -195,7 +197,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, ContactID: 1, ConversationID: 2, Message: "hi", Attachments: atts, + OutgoingURL: proc.URL, PostbackURL: proc.URL, ContactID: 1, ConversationID: 2, Message: "hi", Attachments: atts, }); err != nil { t.Fatalf("unreachable media must not error the call: %v", err) } @@ -221,7 +223,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, ContactID: 1, ConversationID: 2, Message: "hi", + OutgoingURL: proc.URL, PostbackURL: 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) @@ -260,7 +262,7 @@ func TestCall_MimeTypeResolution(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", + OutgoingURL: proc.URL, PostbackURL: 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) @@ -294,7 +296,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, ContactID: 1, ConversationID: 2, Message: "hi", + OutgoingURL: proc.URL, PostbackURL: 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 new file mode 100644 index 0000000..221a767 --- /dev/null +++ b/pkg/ai/service/ai_adapter_ssrf_test.go @@ -0,0 +1,203 @@ +package service_test + +// The attachment URL and the outgoing_url arrive in the same /events payload, so an +// unvalidated download is not "a fetch that might fail" — it is a read primitive +// aimed by the caller whose response is delivered back to the caller. These tests +// pin the guard that keeps the fetch on hosts the CRM is known to serve blobs from. + +import ( + "context" + "encoding/base64" + "encoding/json" + "net/http" + "net/http/httptest" + "strings" + "testing" + + aiModel "github.com/EvolutionAPI/evo-bot-runtime/pkg/ai/model" + aiService "github.com/EvolutionAPI/evo-bot-runtime/pkg/ai/service" +) + +// capturingProc records the parts the adapter posts and answers with a minimal +// successful A2A response. +func capturingProc(parts *[]aiModel.JSONRPCPart) *httptest.Server { + return httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + var req aiModel.JSONRPCRequest + _ = json.NewDecoder(r.Body).Decode(&req) + *parts = req.Params.Message.Parts + _ = json.NewEncoder(w).Encode(aiModel.A2AResponse{ + Result: &aiModel.A2AResult{ + Artifacts: []aiModel.A2AArtifact{{Parts: []aiModel.A2APart{{Type: "text", Text: "ok"}}}}, + }, + }) + })) +} + +func fileParts(parts []aiModel.JSONRPCPart) []aiModel.JSONRPCPart { + out := make([]aiModel.JSONRPCPart, 0, len(parts)) + for _, p := range parts { + if p.Type == "file" { + out = append(out, p) + } + } + return out +} + +// The exfiltration shape: an attachment URL pointing at an internal service, and an +// outgoing_url pointing at the attacker. Nothing internal may reach the file parts. +func TestCall_ForeignHostAttachment_IsNotFetched(t *testing.T) { + var reached bool + internal := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + reached = true + w.Header().Set("Content-Type", "text/plain") + _, _ = w.Write([]byte("internal-credentials")) + })) + defer internal.Close() + + var parts []aiModel.JSONRPCPart + proc := capturingProc(&parts) + defer proc.Close() + + 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) + } + + if reached { + t.Error("the unauthorized host was fetched — the URL guard did not run") + } + if got := fileParts(parts); len(got) != 0 { + t.Fatalf("file parts = %d, want the attachment dropped", len(got)) + } + if len(parts) != 1 || parts[0].Type != "text" { + t.Fatalf("parts = %+v, want the text reply to survive the block", parts) + } +} + +// A host allowlisted through MEDIA_HOST_ALLOWLIST must still be reachable: blob +// storage does not always live on the CRM host (ActiveStorage redirect mode, CDN). +func TestCall_AllowlistedHost_IsFetched(t *testing.T) { + blob := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "image/png") + _, _ = w.Write([]byte("png-bytes")) + })) + defer blob.Close() + + var parts []aiModel.JSONRPCPart + proc := capturingProc(&parts) + defer proc.Close() + + t.Setenv("MEDIA_HOST_ALLOWLIST", "127.0.0.1") + 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) + } + + got := fileParts(parts) + if len(got) != 1 { + t.Fatalf("file parts = %d, want the allowlisted host forwarded", len(got)) + } + decoded, _ := base64.StdEncoding.DecodeString(got[0].File.Bytes) + if string(decoded) != "png-bytes" { + t.Errorf("decoded = %q, want the blob bytes", decoded) + } +} + +// A host check on the declared URL alone is not enough: an authorized host that +// answers 302 could otherwise walk the download onto an internal address. +func TestCall_RedirectOffTheAuthorizedHost_IsRefused(t *testing.T) { + var reached bool + internal := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + reached = true + _, _ = w.Write([]byte("internal-after-redirect")) + })) + defer internal.Close() + + // httptest always binds 127.0.0.1, so the redirect target is addressed by a + // hostname the allowlist does not carry ("localhost") — same machine, different + // host string. Without the redirect check the fetch would succeed. + target := strings.Replace(internal.URL, "127.0.0.1", "localhost", 1) + redirector := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + http.Redirect(w, r, target+"/secret", http.StatusFound) + })) + defer redirector.Close() + + var parts []aiModel.JSONRPCPart + proc := capturingProc(&parts) + defer proc.Close() + + 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) + } + + if reached { + t.Error("the redirect target was fetched — the redirect is not re-checked") + } + if got := fileParts(parts); len(got) != 0 { + t.Fatalf("file parts = %d, want the redirected download dropped", len(got)) + } +} + +// Only http/https may be addressed: the URL must not be able to reach another +// protocol handler. +func TestCall_NonHTTPScheme_IsRejected(t *testing.T) { + var parts []aiModel.JSONRPCPart + proc := capturingProc(&parts) + defer proc.Close() + + 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) + } + if got := fileParts(parts); len(got) != 0 { + t.Fatalf("file parts = %d, want the file:// URL dropped", len(got)) + } +} + +// With no postback URL and no allowlist nothing is authorized, so nothing is +// fetched. Failing closed is the point: the alternative is fetching whatever the +// payload asks for. +func TestCall_NoAuthorizedHost_ForwardsNothing(t *testing.T) { + var reached bool + blob := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + reached = true + })) + defer blob.Close() + + var parts []aiModel.JSONRPCPart + proc := capturingProc(&parts) + defer proc.Close() + + adapter := aiService.NewAIAdapter(30, 0, 1) + if _, err := adapter.Call(context.Background(), &aiModel.A2ARequest{ + OutgoingURL: proc.URL, ContactID: 1, ConversationID: 2, Message: "hi", + Attachments: []aiModel.Attachment{{URL: blob.URL + "/photo.png", ContentType: "image/png"}}, + }); err != nil { + t.Fatalf("Call: %v", err) + } + if reached { + t.Error("an attachment was fetched with no authorized host configured") + } + if len(parts) != 1 || parts[0].Type != "text" { + t.Fatalf("parts = %+v, want text only", parts) + } +} diff --git a/pkg/pipeline/repository/redis_pipeline_repository.go b/pkg/pipeline/repository/redis_pipeline_repository.go index 95838f0..fd50e28 100644 --- a/pkg/pipeline/repository/redis_pipeline_repository.go +++ b/pkg/pipeline/repository/redis_pipeline_repository.go @@ -82,7 +82,15 @@ func (r *redisPipelineRepository) AppendAttachments(ctx context.Context, contact } values = append(values, b) } - return r.rdb.RPush(ctx, attachBufferKey(contactID, conversationID), values...).Err() + key := attachBufferKey(contactID, conversationID) + if err := r.rdb.RPush(ctx, key, values...).Err(); err != nil { + return err + } + // ClearState is the normal cleanup, but any turn that dies without reaching it + // (panic, killed pod, a Redis blip on the Del) would otherwise leave this key — + // and the media URLs in it — in Redis forever. The TTL is a floor, not the + // debounce window: it only has to outlive the longest possible turn. + return r.rdb.Expire(ctx, key, attachBufferTTL).Err() } // GetAttachments returns the media aggregated during the debounce window. EVO-2180. @@ -175,6 +183,12 @@ func bufferKey(contactID, conversationID int64) string { return fmt.Sprintf("bot_runtime:buffer:%d:%d", contactID, conversationID) } +// attachBufferTTL bounds how long an orphaned media buffer can survive. It is far +// longer than any debounce window on purpose — it is a leak backstop, not a +// deadline — and shorter than the 15-minute TTL the CRM signs the media URLs with, +// so an entry can never outlive the links it holds. +const attachBufferTTL = 10 * time.Minute + func attachBufferKey(contactID, conversationID int64) string { return fmt.Sprintf("bot_runtime:attach:%d:%d", contactID, conversationID) } diff --git a/pkg/pipeline/service/pipeline_service.go b/pkg/pipeline/service/pipeline_service.go index 63c610a..8e47f6c 100644 --- a/pkg/pipeline/service/pipeline_service.go +++ b/pkg/pipeline/service/pipeline_service.go @@ -398,6 +398,9 @@ func (s *pipelineService) runAIStage(ctx context.Context, contactID, conversatio Message: buffer, Metadata: metadata, Attachments: aiAttachments, + // Anchors the media host allowlist: attachments are served by the same CRM + // that hands us the postback URL. + PostbackURL: postbackURL, }) if err != nil { switch { From 8cc8b203550addbeb0f9354b54fefc57005057c2 Mon Sep 17 00:00:00 2001 From: Guilherme Gomes Date: Tue, 21 Jul 2026 19:21:44 -0300 Subject: [PATCH 2/2] style(EVO-2180): trim the comments to what is not obvious from the code --- .env.example | 18 +++---- .github/workflows/ci.yml | 7 ++- internal/config/config.go | 7 +-- pkg/ai/model/a2a.go | 5 +- pkg/ai/service/ai_adapter.go | 54 +++++-------------- pkg/ai/service/ai_adapter_ssrf_test.go | 28 ++++------ .../repository/redis_pipeline_repository.go | 11 ++-- pkg/pipeline/service/pipeline_service.go | 4 +- 8 files changed, 40 insertions(+), 94 deletions(-) diff --git a/.env.example b/.env.example index 1df8729..b16533e 100644 --- a/.env.example +++ b/.env.example @@ -2,20 +2,14 @@ LISTEN_ADDR=:8090 REDIS_URL=redis://localhost:6379 # Required. Shared with the CRM, which sends it as X-Bot-Runtime-Secret on /events. -# An empty value would authenticate every caller that omits the header, so the -# service refuses to start without it. Use a value unique to the deployment. +# An empty value would authenticate any caller that omits the header. BOT_RUNTIME_SECRET= AI_CALL_TIMEOUT_SECONDS=30 -# Optional. Extra hostnames authorized to serve incoming media, comma-separated, -# without scheme or port (e.g. "minio.internal,cdn.example.com"). -# -# Incoming attachments are downloaded by this service from a URL that arrives in the -# /events payload, so the host is validated before the fetch: by default only the -# host of that event's postback_url (the CRM) is allowed. Set this when blobs are -# served from somewhere else — ActiveStorage in redirect mode (ATTACHMENT_DELIVERY= -# redirect) hands out presigned S3/MinIO links, and a CDN in front of the CRM is the -# other common case. Media on an unlisted host is skipped and logged as -# pipeline.ai.attachment.blocked_url; the text reply still goes out. +# 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. MEDIA_HOST_ALLOWLIST= diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 90a754a..8c74f6b 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -1,9 +1,8 @@ name: CI -# Until now the only workflow here was docker-publish.yml, so nothing ran the Go -# suite on a PR: `test/e2e` sat non-compiling from EVO-558 to EVO-2180 without a -# single red check. The repository tests need Redis, which is why this runs it as a -# service container rather than skipping the packages that touch it. +# Nothing ran the Go suite on a PR before this: test/e2e sat non-compiling from +# EVO-558 to EVO-2180 without a single red check. Redis is a service container +# because the repository tests need a real one. on: pull_request: diff --git a/internal/config/config.go b/internal/config/config.go index f81f024..9781220 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -27,11 +27,8 @@ func Load() (*Config, error) { if err != nil { return nil, err } - // Required, not optional: SecretMiddleware compares the header against this - // value, so an empty secret authenticates every caller that simply omits the - // header. /events accepts an outgoing_url and (since EVO-2180) attachment URLs - // this service fetches itself, which makes an unauthenticated endpoint an - // outbound-request gadget rather than just a spam vector. + // Required: SecretMiddleware compares the header against this, so an empty value + // authenticates every caller that omits it. botRuntimeSecret, err := mustGetEnv("BOT_RUNTIME_SECRET") if err != nil { return nil, err diff --git a/pkg/ai/model/a2a.go b/pkg/ai/model/a2a.go index 7233b51..64d0a62 100644 --- a/pkg/ai/model/a2a.go +++ b/pkg/ai/model/a2a.go @@ -10,9 +10,8 @@ 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 is the CRM base this event came from. It is not used for the A2A - // call itself — it anchors the host allowlist that decides which attachment URLs - // may be downloaded (see allowedMediaHosts). + // PostbackURL anchors the media host allowlist (see allowedMediaHosts); it is + // not used for the A2A call itself. PostbackURL string } diff --git a/pkg/ai/service/ai_adapter.go b/pkg/ai/service/ai_adapter.go index dc6374a..21deacd 100644 --- a/pkg/ai/service/ai_adapter.go +++ b/pkg/ai/service/ai_adapter.go @@ -50,21 +50,9 @@ const ( attachmentsTotalTimeFactor = 3 ) -// mediaHostAllowlistEnv names the extra hosts authorized to serve incoming media. -// -// Attachment URLs arrive inside the /events payload and downloading one is a request -// this service makes from inside the network, so an unvalidated URL turns /events -// into an SSRF gadget: the fetched bytes are handed straight to the outgoing_url -// that came in the same payload. The media is served by the CRM that sent the event, -// so the postback host — already mandatory in MessageEvent.Validate — is the natural -// anchor and needs no new configuration. -// -// This variable is the escape hatch for deployments that serve blobs from somewhere -// else: ActiveStorage in redirect mode (ATTACHMENT_DELIVERY=redirect) hands out -// presigned S3/MinIO links, and a CDN in front of the CRM is the other common case. -// Comma-separated hostnames, no scheme and no port, e.g. -// "minio.internal,cdn.example.com". An attachment on any other host is skipped with -// a log line and the text reply still goes out. +// 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. const mediaHostAllowlistEnv = "MEDIA_HOST_ALLOWLIST" // maxBackoff caps the exponential backoff between retries so a large @@ -351,9 +339,7 @@ func (a *aiAdapter) buildFileParts(ctx context.Context, req *model.A2ARequest) [ budgetCtx, cancelBudget := context.WithTimeout(ctx, attachmentsTotalTimeFactor*perDownload) defer cancelBudget() - // Built once per turn, not per attachment: it carries the authorized hosts into - // the redirect check, so a 302 off the CRM cannot walk the download onto an - // internal address. + // Built once per turn: the client closes over the authorized hosts. hosts := allowedMediaHosts(req.PostbackURL) client := a.mediaClient(hosts) @@ -396,11 +382,8 @@ func (a *aiAdapter) buildFileParts(ctx context.Context, req *model.A2ARequest) [ } data, respContentType, err := downloadAttachment(budgetCtx, client, att.URL, perDownload, limit) if err != nil { - // The status is logged separately because the most common production - // failure is a 404 from an expired signed link (the CRM mints it with a - // 15-minute TTL at delegation time, and a backed-up queue can outlive - // that). Without it "download_failed" reads the same as an unreachable - // host, and the customer just sees the agent ignore the image. + // Status separates the common 404-from-an-expired-signed-link case from + // an unreachable host; they logged identically before. slog.Warn("pipeline.ai.attachment.download_failed", "contact_id", req.ContactID, "conversation_id", req.ConversationID, @@ -452,11 +435,9 @@ func (a *aiAdapter) attachmentTimeout() time.Duration { return d } -// allowedMediaHosts returns the hostnames authorized to serve this event's media: -// the host of the event's own postback URL, plus whatever mediaHostAllowlistEnv -// names. An empty set means nothing is authorized and every attachment is skipped — -// failing closed is deliberate, because the alternative is fetching an -// attacker-chosen URL from inside the network. +// allowedMediaHosts returns the hostnames authorized to serve this event's media. +// An empty set authorizes nothing: failing closed beats fetching a URL the caller +// chose. func allowedMediaHosts(postbackURL string) map[string]struct{} { hosts := make(map[string]struct{}, 2) if u, err := neturl.Parse(postbackURL); err == nil { @@ -473,8 +454,6 @@ func allowedMediaHosts(postbackURL string) map[string]struct{} { } // checkMediaURL reports why a media URL must not be fetched, or nil when it may be. -// Scheme is pinned to http/https so the URL cannot address another protocol handler, -// and the host must be one the CRM is known to serve blobs from. func checkMediaURL(rawURL string, hosts map[string]struct{}) error { u, err := neturl.Parse(rawURL) if err != nil { @@ -493,10 +472,8 @@ func checkMediaURL(rawURL string, hosts map[string]struct{}) error { return nil } -// mediaClient is the client used for attachment downloads. It shares the adapter's -// transport (connection pool) but re-runs checkMediaURL on every redirect hop: an -// allowlisted host that answers 302 must not be able to walk the download onto a -// link-local or internal address. +// mediaClient shares the adapter's transport but re-runs checkMediaURL on every +// redirect hop, so an authorized host cannot 302 the download onto an internal one. func (a *aiAdapter) mediaClient(hosts map[string]struct{}) *http.Client { return &http.Client{ Transport: a.client.Transport, @@ -509,14 +486,12 @@ func (a *aiAdapter) mediaClient(hosts map[string]struct{}) *http.Client { } } -// httpStatusError carries the status of a non-200 media response so the caller can -// log it without re-parsing the message. +// httpStatusError carries the status of a non-200 media response. type httpStatusError struct{ status int } func (e *httpStatusError) Error() string { return fmt.Sprintf("unexpected status %d", e.status) } -// statusOf extracts the HTTP status from a download error, or 0 when the request -// never got a response (DNS failure, refused connection, timeout, blocked redirect). +// statusOf returns the HTTP status of a download error, or 0 if there was no response. func statusOf(err error) int { var se *httpStatusError if errors.As(err, &se) { @@ -526,8 +501,7 @@ func statusOf(err error) int { } // downloadAttachment GETs the URL with the given client and timeout, reading at most -// limit bytes. It returns the body and the response Content-Type so the caller can -// decide what the bytes actually are. +// limit bytes. It returns the body and the response Content-Type. func downloadAttachment(ctx context.Context, client *http.Client, url string, timeout time.Duration, limit int) ([]byte, string, error) { dlCtx, cancel := context.WithTimeout(ctx, timeout) defer cancel() diff --git a/pkg/ai/service/ai_adapter_ssrf_test.go b/pkg/ai/service/ai_adapter_ssrf_test.go index 221a767..6f8f79f 100644 --- a/pkg/ai/service/ai_adapter_ssrf_test.go +++ b/pkg/ai/service/ai_adapter_ssrf_test.go @@ -1,9 +1,7 @@ package service_test // The attachment URL and the outgoing_url arrive in the same /events payload, so an -// unvalidated download is not "a fetch that might fail" — it is a read primitive -// aimed by the caller whose response is delivered back to the caller. These tests -// pin the guard that keeps the fetch on hosts the CRM is known to serve blobs from. +// unvalidated download is a read primitive aimed by its caller. These pin the guard. import ( "context" @@ -18,8 +16,7 @@ import ( aiService "github.com/EvolutionAPI/evo-bot-runtime/pkg/ai/service" ) -// capturingProc records the parts the adapter posts and answers with a minimal -// successful A2A response. +// capturingProc records the parts the adapter posts. func capturingProc(parts *[]aiModel.JSONRPCPart) *httptest.Server { return httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { var req aiModel.JSONRPCRequest @@ -43,8 +40,7 @@ func fileParts(parts []aiModel.JSONRPCPart) []aiModel.JSONRPCPart { return out } -// The exfiltration shape: an attachment URL pointing at an internal service, and an -// outgoing_url pointing at the attacker. Nothing internal may reach the file parts. +// The exfiltration shape: internal attachment URL, attacker-chosen outgoing_url. func TestCall_ForeignHostAttachment_IsNotFetched(t *testing.T) { var reached bool internal := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { @@ -79,8 +75,7 @@ func TestCall_ForeignHostAttachment_IsNotFetched(t *testing.T) { } } -// A host allowlisted through MEDIA_HOST_ALLOWLIST must still be reachable: blob -// storage does not always live on the CRM host (ActiveStorage redirect mode, CDN). +// Blob storage does not always live on the CRM host, so the allowlist must work. func TestCall_AllowlistedHost_IsFetched(t *testing.T) { blob := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.Header().Set("Content-Type", "image/png") @@ -112,8 +107,7 @@ func TestCall_AllowlistedHost_IsFetched(t *testing.T) { } } -// A host check on the declared URL alone is not enough: an authorized host that -// answers 302 could otherwise walk the download onto an internal address. +// Checking the declared URL alone is not enough: a 302 could walk it internal. func TestCall_RedirectOffTheAuthorizedHost_IsRefused(t *testing.T) { var reached bool internal := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { @@ -122,9 +116,8 @@ func TestCall_RedirectOffTheAuthorizedHost_IsRefused(t *testing.T) { })) defer internal.Close() - // httptest always binds 127.0.0.1, so the redirect target is addressed by a - // hostname the allowlist does not carry ("localhost") — same machine, different - // host string. Without the redirect check the fetch would succeed. + // httptest always binds 127.0.0.1, so the target uses a host string the + // allowlist does not carry. Without the redirect check this would succeed. target := strings.Replace(internal.URL, "127.0.0.1", "localhost", 1) redirector := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { http.Redirect(w, r, target+"/secret", http.StatusFound) @@ -153,8 +146,7 @@ func TestCall_RedirectOffTheAuthorizedHost_IsRefused(t *testing.T) { } } -// Only http/https may be addressed: the URL must not be able to reach another -// protocol handler. +// Only http/https may be addressed. func TestCall_NonHTTPScheme_IsRejected(t *testing.T) { var parts []aiModel.JSONRPCPart proc := capturingProc(&parts) @@ -173,9 +165,7 @@ func TestCall_NonHTTPScheme_IsRejected(t *testing.T) { } } -// With no postback URL and no allowlist nothing is authorized, so nothing is -// fetched. Failing closed is the point: the alternative is fetching whatever the -// payload asks for. +// Nothing authorized means nothing fetched — failing closed is the point. 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/repository/redis_pipeline_repository.go b/pkg/pipeline/repository/redis_pipeline_repository.go index fd50e28..48ca9d5 100644 --- a/pkg/pipeline/repository/redis_pipeline_repository.go +++ b/pkg/pipeline/repository/redis_pipeline_repository.go @@ -86,10 +86,7 @@ func (r *redisPipelineRepository) AppendAttachments(ctx context.Context, contact if err := r.rdb.RPush(ctx, key, values...).Err(); err != nil { return err } - // ClearState is the normal cleanup, but any turn that dies without reaching it - // (panic, killed pod, a Redis blip on the Del) would otherwise leave this key — - // and the media URLs in it — in Redis forever. The TTL is a floor, not the - // debounce window: it only has to outlive the longest possible turn. + // Leak backstop for turns that die before ClearState; not the debounce window. return r.rdb.Expire(ctx, key, attachBufferTTL).Err() } @@ -183,10 +180,8 @@ func bufferKey(contactID, conversationID int64) string { return fmt.Sprintf("bot_runtime:buffer:%d:%d", contactID, conversationID) } -// attachBufferTTL bounds how long an orphaned media buffer can survive. It is far -// longer than any debounce window on purpose — it is a leak backstop, not a -// deadline — and shorter than the 15-minute TTL the CRM signs the media URLs with, -// so an entry can never outlive the links it holds. +// attachBufferTTL outlives any turn but stays under the 15-minute TTL the CRM signs +// the media URLs with, so an orphaned entry never outlives its links. const attachBufferTTL = 10 * time.Minute func attachBufferKey(contactID, conversationID int64) string { diff --git a/pkg/pipeline/service/pipeline_service.go b/pkg/pipeline/service/pipeline_service.go index 8e47f6c..38b1474 100644 --- a/pkg/pipeline/service/pipeline_service.go +++ b/pkg/pipeline/service/pipeline_service.go @@ -398,9 +398,7 @@ func (s *pipelineService) runAIStage(ctx context.Context, contactID, conversatio Message: buffer, Metadata: metadata, Attachments: aiAttachments, - // Anchors the media host allowlist: attachments are served by the same CRM - // that hands us the postback URL. - PostbackURL: postbackURL, + PostbackURL: postbackURL, // anchors the media host allowlist }) if err != nil { switch {