fix(EVO-2178): validate incoming media URLs before fetching them#6
Merged
Conversation
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.
Reviewer's GuideThis PR hardens media handling in the AI adapter to prevent SSRF/exfiltration, enforces a non-empty bot runtime secret, bounds the lifetime of Redis media buffers, improves logging of media download failures, and adds a CI workflow that runs the Go build/vet/test suite on pushes and PRs. Sequence diagram for validated media download with SSRF protectionsequenceDiagram
participant PipelineService
participant AIAdapter
participant MediaClient
participant MediaHost
PipelineService->>AIAdapter: runAIStage(...)
AIAdapter->>AIAdapter: allowedMediaHosts(PostbackURL)
AIAdapter->>AIAdapter: mediaClient(hosts)
loop for each Attachment
AIAdapter->>AIAdapter: checkMediaURL(Attachment.URL, hosts)
alt URL not allowed
AIAdapter->>AIAdapter: slog.Warn("pipeline.ai.attachment.blocked_url")
else URL allowed
AIAdapter->>MediaClient: downloadAttachment(ctx, client, URL, timeout, limit)
activate MediaClient
MediaClient->>MediaHost: GET URL
alt Redirect response
MediaHost-->>MediaClient: 3xx Location
MediaClient->>MediaClient: CheckRedirect(...)
MediaClient->>AIAdapter: checkMediaURL(newURL, hosts)
alt checkMediaURL fails
MediaClient-->>AIAdapter: error
else checkMediaURL ok
MediaClient->>MediaHost: GET newURL
end
else 200 OK
MediaHost-->>MediaClient: 200 OK body
MediaClient-->>AIAdapter: data, contentType
else Non-200
MediaHost-->>MediaClient: status != 200
MediaClient-->>AIAdapter: &httpStatusError
end
deactivate MediaClient
alt error from downloadAttachment
AIAdapter->>AIAdapter: statusOf(err)
AIAdapter->>AIAdapter: slog.Warn("pipeline.ai.attachment.download_failed")
else success
AIAdapter->>AIAdapter: append file part
end
end
end
Flow diagram for media host allowlist and Redis attachment TTLflowchart LR
subgraph MediaAllowlist
A[PostbackURL] -->|hostname| B[allowedMediaHosts]
C[MEDIA_HOST_ALLOWLIST] -->|comma-separated hosts| B
B --> D{hosts map empty?}
D -->|yes| E[All attachments skipped<br/>fail-closed]
D -->|no| F["checkMediaURL(rawURL, hosts)"]
F -->|invalid/unauthorized| G[blocked_url log]
F -->|ok| H[downloadAttachment]
end
subgraph RedisBufferTTL
I[AppendAttachments] --> J[RPush attachBufferKey]
J --> K[Expire attachBufferKey<br/>attachBufferTTL]
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="pkg/ai/service/ai_adapter.go" line_range="500-510" />
<code_context>
+// 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)
+ },
+ }
+}
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** mediaClient drops http.Client-level settings from a.client; consider reusing or cloning the existing client.
This creates a new http.Client that only reuses a.client.Transport and adds CheckRedirect, dropping any other configuration on a.client (e.g., CookieJar, Timeout, etc.) for media downloads.
Consider either:
- Basing mediaClient on a.client and overriding only CheckRedirect, or
- Clearly documenting that mediaClient is intentionally configured differently.
This avoids surprises if a.client is later updated (e.g., global Timeout or Jar) with the expectation that media downloads share those settings.
```suggestion
func (a *aiAdapter) mediaClient(hosts map[string]struct{}) *http.Client {
// Clone the base client so media downloads inherit all configuration
// (timeouts, cookie jar, etc.) but enforce our own redirect policy.
clientCopy := *a.client
clientCopy.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)
}
return &clientCopy
}
```
</issue_to_address>
### Comment 2
<location path="pkg/pipeline/repository/redis_pipeline_repository.go" line_range="89-93" />
<code_context>
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()
}
</code_context>
<issue_to_address>
**suggestion (bug_risk):** TTL failure now makes AppendAttachments fail; consider whether this should be best-effort instead of hard-fail.
With this change, any transient Expire failure (e.g., network blip, Redis failover) will cause AppendAttachments to return an error even though RPush has already succeeded and the data is durably written.
Since attachBufferTTL is a safety net rather than a correctness requirement, consider treating Expire as best-effort: log on failure but still return nil when RPush succeeds, so the main write path doesn’t gain new failure modes solely due to TTL issues.
```suggestion
// 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.
//
// Expire is best-effort: a failure here should not turn a successful append
// into an error, since the TTL is a safety net rather than a correctness
// requirement.
_ = r.rdb.Expire(ctx, key, attachBufferTTL).Err()
return nil
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+500
to
+510
| 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) | ||
| }, | ||
| } | ||
| } |
There was a problem hiding this comment.
suggestion (bug_risk): mediaClient drops http.Client-level settings from a.client; consider reusing or cloning the existing client.
This creates a new http.Client that only reuses a.client.Transport and adds CheckRedirect, dropping any other configuration on a.client (e.g., CookieJar, Timeout, etc.) for media downloads.
Consider either:
- Basing mediaClient on a.client and overriding only CheckRedirect, or
- Clearly documenting that mediaClient is intentionally configured differently.
This avoids surprises if a.client is later updated (e.g., global Timeout or Jar) with the expectation that media downloads share those settings.
Suggested change
| 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) | |
| }, | |
| } | |
| } | |
| func (a *aiAdapter) mediaClient(hosts map[string]struct{}) *http.Client { | |
| // Clone the base client so media downloads inherit all configuration | |
| // (timeouts, cookie jar, etc.) but enforce our own redirect policy. | |
| clientCopy := *a.client | |
| clientCopy.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) | |
| } | |
| return &clientCopy | |
| } |
Comment on lines
+89
to
+93
| // 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() |
There was a problem hiding this comment.
suggestion (bug_risk): TTL failure now makes AppendAttachments fail; consider whether this should be best-effort instead of hard-fail.
With this change, any transient Expire failure (e.g., network blip, Redis failover) will cause AppendAttachments to return an error even though RPush has already succeeded and the data is durably written.
Since attachBufferTTL is a safety net rather than a correctness requirement, consider treating Expire as best-effort: log on failure but still return nil when RPush succeeds, so the main write path doesn’t gain new failure modes solely due to TTL issues.
Suggested change
| // 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() | |
| // 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. | |
| // | |
| // Expire is best-effort: a failure here should not turn a successful append | |
| // into an error, since the TTL is a safety net rather than a correctness | |
| // requirement. | |
| _ = r.rdb.Expire(ctx, key, attachBufferTTL).Err() | |
| return nil |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Review follow-up to EVO-2180 (PR #5, already merged). Findings from the EVO-2178 review of the merged media path.
🔴 SSRF with exfiltration (critical)
Attachment URLs arrive inside the
/eventspayload and the adapter fetched them verbatim — no scheme check, no host check, and the client followed redirects. So/eventsdoubled 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, directly and through a302.checkMediaURLpins the scheme tohttp/httpsand requires the host to be one the CRM is known to serve blobs from: the host of the event's ownpostback_url(already mandatory inMessageEvent.Validate, so no new config for the default topology) plus whateverMEDIA_HOST_ALLOWLISTnames, 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.Also in this PR
BOT_RUNTIME_SECRETis now required. It was read withos.Getenv, andSecretMiddlewarecompares the header against it, so an empty value authenticated every caller that simply omitted the header.ClearStateremains the normal cleanup; the TTL only stops a turn that dies before reaching it from leaving media URLs in Redis forever.404from a signed link that expired while the queue was backed up, and it read identically to an unreachable host..github/workflows/ci.yml. Nothing ran the Go suite on a PR, which is howtest/e2estayed non-compiling from EVO-558 until EVO-2180.Verification
go build ./...·go vet ./...·go test ./...— green, e2e included, against a throwaway Redis. The new SSRF tests were mutation-checked: with the guard disabled, 3 of the 5 fail (the URL check, the redirect re-check, the fail-closed default), so they are not vacuous.Part of EVO-2178 · follow-up to EVO-2180.
🤖 Generated with Claude Code
Summary by Sourcery
Harden media handling in the AI adapter to prevent SSRF and ensure only trusted hosts and schemes are used when downloading attachments, while preserving text responses when media is blocked or fails.
Bug Fixes:
Enhancements:
CI:
Tests: