Skip to content

Commit 76f23b1

Browse files
authored
Merge pull request #6 from evolution-foundation/fix/EVO-2178-review-ssrf-guard
fix(EVO-2178): validate incoming media URLs before fetching them
2 parents 2e70a6e + 8cc8b20 commit 76f23b1

9 files changed

Lines changed: 366 additions & 14 deletions

File tree

.env.example

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,15 @@
11
LISTEN_ADDR=:8090
22
REDIS_URL=redis://localhost:6379
3+
4+
# Required. Shared with the CRM, which sends it as X-Bot-Runtime-Secret on /events.
5+
# An empty value would authenticate any caller that omits the header.
36
BOT_RUNTIME_SECRET=
7+
48
AI_CALL_TIMEOUT_SECONDS=30
9+
10+
# Optional. Extra hosts allowed to serve incoming media, comma-separated, no scheme
11+
# or port (e.g. "minio.internal,cdn.example.com"). By default only the host of the
12+
# event's postback_url is allowed. Set this when blobs are served elsewhere
13+
# (ActiveStorage redirect mode, CDN); media on an unlisted host is skipped and
14+
# logged as pipeline.ai.attachment.blocked_url, and the text reply still goes out.
15+
MEDIA_HOST_ALLOWLIST=

.github/workflows/ci.yml

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,41 @@
1+
name: CI
2+
3+
# Nothing ran the Go suite on a PR before this: test/e2e sat non-compiling from
4+
# EVO-558 to EVO-2180 without a single red check. Redis is a service container
5+
# because the repository tests need a real one.
6+
7+
on:
8+
pull_request:
9+
push:
10+
branches: [develop, main]
11+
12+
jobs:
13+
test:
14+
runs-on: ubuntu-latest
15+
services:
16+
redis:
17+
image: redis:7-alpine
18+
ports: ['6379:6379']
19+
options: >-
20+
--health-cmd "redis-cli ping"
21+
--health-interval 5s
22+
--health-timeout 3s
23+
--health-retries 10
24+
env:
25+
REDIS_TEST_URL: redis://localhost:6379
26+
steps:
27+
- uses: actions/checkout@v4
28+
29+
- uses: actions/setup-go@v5
30+
with:
31+
go-version-file: go.mod
32+
cache: true
33+
34+
- name: Build
35+
run: go build ./...
36+
37+
- name: Vet
38+
run: go vet ./...
39+
40+
- name: Test
41+
run: go test ./...

internal/config/config.go

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,12 @@ func Load() (*Config, error) {
2727
if err != nil {
2828
return nil, err
2929
}
30-
botRuntimeSecret := os.Getenv("BOT_RUNTIME_SECRET")
30+
// Required: SecretMiddleware compares the header against this, so an empty value
31+
// authenticates every caller that omits it.
32+
botRuntimeSecret, err := mustGetEnv("BOT_RUNTIME_SECRET")
33+
if err != nil {
34+
return nil, err
35+
}
3136
aiCallTimeout, err := getEnvIntOrDefault("AI_CALL_TIMEOUT_SECONDS", 30)
3237
if err != nil {
3338
return nil, err

pkg/ai/model/a2a.go

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,9 @@ type A2ARequest struct {
1010
Message string // aggregated buffer content (FR-15)
1111
Metadata map[string]any // CRM metadata passed through to processor (tools context)
1212
Attachments []Attachment // EVO-2180: incoming media to forward as A2A file parts
13+
// PostbackURL anchors the media host allowlist (see allowedMediaHosts); it is
14+
// not used for the A2A call itself.
15+
PostbackURL string
1316
}
1417

1518
// Attachment is an incoming media item (image/audio/…) the adapter downloads and

pkg/ai/service/ai_adapter.go

Lines changed: 94 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ import (
1313
"mime"
1414
"net/http"
1515
neturl "net/url"
16+
"os"
1617
"path"
1718
"strings"
1819
"time"
@@ -49,6 +50,11 @@ const (
4950
attachmentsTotalTimeFactor = 3
5051
)
5152

53+
// mediaHostAllowlistEnv names extra hosts authorized to serve incoming media,
54+
// comma-separated, for deployments serving blobs off the CRM host (ActiveStorage
55+
// redirect mode, CDN). The default anchor is the event's own postback host.
56+
const mediaHostAllowlistEnv = "MEDIA_HOST_ALLOWLIST"
57+
5258
// maxBackoff caps the exponential backoff between retries so a large
5359
// AI_CALL_RETRY_BASE_MS or retry count cannot balloon the wait.
5460
const maxBackoff = 5 * time.Second
@@ -333,12 +339,26 @@ func (a *aiAdapter) buildFileParts(ctx context.Context, req *model.A2ARequest) [
333339
budgetCtx, cancelBudget := context.WithTimeout(ctx, attachmentsTotalTimeFactor*perDownload)
334340
defer cancelBudget()
335341

342+
// Built once per turn: the client closes over the authorized hosts.
343+
hosts := allowedMediaHosts(req.PostbackURL)
344+
client := a.mediaClient(hosts)
345+
336346
parts := make([]model.JSONRPCPart, 0, len(req.Attachments))
337347
remaining := maxAttachmentsTotalBytes
338348
for i, att := range req.Attachments {
339349
if att.URL == "" {
340350
continue
341351
}
352+
if err := checkMediaURL(att.URL, hosts); err != nil {
353+
slog.Warn("pipeline.ai.attachment.blocked_url",
354+
"contact_id", req.ContactID,
355+
"conversation_id", req.ConversationID,
356+
"file_type", att.FileType,
357+
"error", err,
358+
"hint", "set "+mediaHostAllowlistEnv+" when blobs are served off the CRM host",
359+
)
360+
continue
361+
}
342362
if budgetCtx.Err() != nil {
343363
slog.Warn("pipeline.ai.attachment.budget_exhausted",
344364
"contact_id", req.ContactID,
@@ -360,12 +380,15 @@ func (a *aiAdapter) buildFileParts(ctx context.Context, req *model.A2ARequest) [
360380
)
361381
break
362382
}
363-
data, respContentType, err := a.downloadAttachment(budgetCtx, att.URL, perDownload, limit)
383+
data, respContentType, err := downloadAttachment(budgetCtx, client, att.URL, perDownload, limit)
364384
if err != nil {
385+
// Status separates the common 404-from-an-expired-signed-link case from
386+
// an unreachable host; they logged identically before.
365387
slog.Warn("pipeline.ai.attachment.download_failed",
366388
"contact_id", req.ContactID,
367389
"conversation_id", req.ConversationID,
368390
"file_type", att.FileType,
391+
"status", statusOf(err),
369392
"error", err,
370393
)
371394
continue
@@ -412,24 +435,88 @@ func (a *aiAdapter) attachmentTimeout() time.Duration {
412435
return d
413436
}
414437

415-
// downloadAttachment GETs the URL with the adapter's client and the given timeout,
416-
// reading at most limit bytes. It returns the body and the response Content-Type so
417-
// the caller can decide what the bytes actually are.
418-
func (a *aiAdapter) downloadAttachment(ctx context.Context, url string, timeout time.Duration, limit int) ([]byte, string, error) {
438+
// allowedMediaHosts returns the hostnames authorized to serve this event's media.
439+
// An empty set authorizes nothing: failing closed beats fetching a URL the caller
440+
// chose.
441+
func allowedMediaHosts(postbackURL string) map[string]struct{} {
442+
hosts := make(map[string]struct{}, 2)
443+
if u, err := neturl.Parse(postbackURL); err == nil {
444+
if h := strings.ToLower(u.Hostname()); h != "" {
445+
hosts[h] = struct{}{}
446+
}
447+
}
448+
for _, h := range strings.Split(os.Getenv(mediaHostAllowlistEnv), ",") {
449+
if h = strings.ToLower(strings.TrimSpace(h)); h != "" {
450+
hosts[h] = struct{}{}
451+
}
452+
}
453+
return hosts
454+
}
455+
456+
// checkMediaURL reports why a media URL must not be fetched, or nil when it may be.
457+
func checkMediaURL(rawURL string, hosts map[string]struct{}) error {
458+
u, err := neturl.Parse(rawURL)
459+
if err != nil {
460+
return fmt.Errorf("unparseable media url: %w", err)
461+
}
462+
if scheme := strings.ToLower(u.Scheme); scheme != "http" && scheme != "https" {
463+
return fmt.Errorf("scheme %q is not allowed for media", u.Scheme)
464+
}
465+
host := strings.ToLower(u.Hostname())
466+
if host == "" {
467+
return errors.New("media url has no host")
468+
}
469+
if _, ok := hosts[host]; !ok {
470+
return fmt.Errorf("host %q is not authorized to serve media for this event", host)
471+
}
472+
return nil
473+
}
474+
475+
// mediaClient shares the adapter's transport but re-runs checkMediaURL on every
476+
// redirect hop, so an authorized host cannot 302 the download onto an internal one.
477+
func (a *aiAdapter) mediaClient(hosts map[string]struct{}) *http.Client {
478+
return &http.Client{
479+
Transport: a.client.Transport,
480+
CheckRedirect: func(req *http.Request, via []*http.Request) error {
481+
if len(via) >= 10 {
482+
return errors.New("stopped after 10 redirects")
483+
}
484+
return checkMediaURL(req.URL.String(), hosts)
485+
},
486+
}
487+
}
488+
489+
// httpStatusError carries the status of a non-200 media response.
490+
type httpStatusError struct{ status int }
491+
492+
func (e *httpStatusError) Error() string { return fmt.Sprintf("unexpected status %d", e.status) }
493+
494+
// statusOf returns the HTTP status of a download error, or 0 if there was no response.
495+
func statusOf(err error) int {
496+
var se *httpStatusError
497+
if errors.As(err, &se) {
498+
return se.status
499+
}
500+
return 0
501+
}
502+
503+
// downloadAttachment GETs the URL with the given client and timeout, reading at most
504+
// limit bytes. It returns the body and the response Content-Type.
505+
func downloadAttachment(ctx context.Context, client *http.Client, url string, timeout time.Duration, limit int) ([]byte, string, error) {
419506
dlCtx, cancel := context.WithTimeout(ctx, timeout)
420507
defer cancel()
421508

422509
httpReq, err := http.NewRequestWithContext(dlCtx, http.MethodGet, url, nil)
423510
if err != nil {
424511
return nil, "", fmt.Errorf("new_request: %w", err)
425512
}
426-
resp, err := a.client.Do(httpReq)
513+
resp, err := client.Do(httpReq)
427514
if err != nil {
428515
return nil, "", fmt.Errorf("do: %w", err)
429516
}
430517
defer resp.Body.Close()
431518
if resp.StatusCode != http.StatusOK {
432-
return nil, "", fmt.Errorf("unexpected status %d", resp.StatusCode)
519+
return nil, "", &httpStatusError{status: resp.StatusCode}
433520
}
434521
// +1 so an exactly-at-cap read is distinguishable from an oversize one.
435522
data, err := io.ReadAll(io.LimitReader(resp.Body, int64(limit)+1))

pkg/ai/service/ai_adapter_media_test.go

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,7 @@ func TestCall_ForwardsAttachmentAsFilePart(t *testing.T) {
5454
adapter := aiService.NewAIAdapter(30, 0, 1)
5555
_, err := adapter.Call(context.Background(), &aiModel.A2ARequest{
5656
OutgoingURL: proc.URL + "/api/v1/a2a/agent-1",
57+
PostbackURL: proc.URL,
5758
ContactID: 1,
5859
ConversationID: 2,
5960
ApiKey: "k",
@@ -98,6 +99,7 @@ func TestCall_AttachmentDownloadFailure_SendsTextOnly(t *testing.T) {
9899
adapter := aiService.NewAIAdapter(30, 0, 1)
99100
_, err := adapter.Call(context.Background(), &aiModel.A2ARequest{
100101
OutgoingURL: proc.URL + "/api/v1/a2a/agent-1",
102+
PostbackURL: proc.URL,
101103
ContactID: 1,
102104
ConversationID: 2,
103105
ApiKey: "k",
@@ -159,7 +161,7 @@ func TestCall_TotalAttachmentBudget_DropsExcessAndStillSends(t *testing.T) {
159161

160162
adapter := aiService.NewAIAdapter(30, 0, 1)
161163
if _, err := adapter.Call(context.Background(), &aiModel.A2ARequest{
162-
OutgoingURL: proc.URL, ContactID: 1, ConversationID: 2, Message: "album", Attachments: atts,
164+
OutgoingURL: proc.URL, PostbackURL: proc.URL, ContactID: 1, ConversationID: 2, Message: "album", Attachments: atts,
163165
}); err != nil {
164166
t.Fatalf("a media budget overflow must not error the call: %v", err)
165167
}
@@ -195,7 +197,7 @@ func TestCall_AttachmentTimeBudget_IsBounded(t *testing.T) {
195197
adapter := aiService.NewAIAdapter(1, 0, 1)
196198
start := time.Now()
197199
if _, err := adapter.Call(context.Background(), &aiModel.A2ARequest{
198-
OutgoingURL: proc.URL, ContactID: 1, ConversationID: 2, Message: "hi", Attachments: atts,
200+
OutgoingURL: proc.URL, PostbackURL: proc.URL, ContactID: 1, ConversationID: 2, Message: "hi", Attachments: atts,
199201
}); err != nil {
200202
t.Fatalf("unreachable media must not error the call: %v", err)
201203
}
@@ -221,7 +223,7 @@ func TestCall_HTMLResponse_IsNotForwardedAsMedia(t *testing.T) {
221223

222224
adapter := aiService.NewAIAdapter(30, 0, 1)
223225
if _, err := adapter.Call(context.Background(), &aiModel.A2ARequest{
224-
OutgoingURL: proc.URL, ContactID: 1, ConversationID: 2, Message: "hi",
226+
OutgoingURL: proc.URL, PostbackURL: proc.URL, ContactID: 1, ConversationID: 2, Message: "hi",
225227
Attachments: []aiModel.Attachment{{URL: htmlSrv.URL + "/photo.jpg", ContentType: "image/jpeg", FileType: "image"}},
226228
}); err != nil {
227229
t.Fatalf("Call: %v", err)
@@ -260,7 +262,7 @@ func TestCall_MimeTypeResolution(t *testing.T) {
260262

261263
adapter := aiService.NewAIAdapter(30, 0, 1)
262264
if _, err := adapter.Call(context.Background(), &aiModel.A2ARequest{
263-
OutgoingURL: proc.URL, ContactID: 1, ConversationID: 2, Message: "hi",
265+
OutgoingURL: proc.URL, PostbackURL: proc.URL, ContactID: 1, ConversationID: 2, Message: "hi",
264266
Attachments: []aiModel.Attachment{{URL: srv.URL + tc.urlPath, ContentType: tc.declared, FileType: "image"}},
265267
}); err != nil {
266268
t.Fatalf("Call: %v", err)
@@ -294,7 +296,7 @@ func TestCall_OversizeAttachment_SendsTextOnly(t *testing.T) {
294296

295297
adapter := aiService.NewAIAdapter(30, 0, 1)
296298
if _, err := adapter.Call(context.Background(), &aiModel.A2ARequest{
297-
OutgoingURL: proc.URL, ContactID: 1, ConversationID: 2, Message: "hi",
299+
OutgoingURL: proc.URL, PostbackURL: proc.URL, ContactID: 1, ConversationID: 2, Message: "hi",
298300
Attachments: []aiModel.Attachment{{URL: srv.URL + "/big.png", ContentType: "image/png", FileType: "image"}},
299301
}); err != nil {
300302
t.Fatalf("an oversize attachment must not error the call: %v", err)

0 commit comments

Comments
 (0)