fix(EVO-2178): take the media host allowlist from config, not from the event#7
Merged
Merged
Conversation
…e event 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.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @gomessguii, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
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.
Fixes a regression I introduced in #6, and corrects the shape of the guard added there.
The regression
#6 anchored the media host allowlist on the host of the event's
postback_url. That host never matches the one the CRM signs attachment URLs with:BOT_RUNTIME_POSTBACK_BASE_URLevo-crm(internal DNS)ACTIVE_STORAGE_URL, falling back toBACKEND_URLSo on
developevery attachment was rejected aspipeline.ai.attachment.blocked_url, the text reply went out alone, and the agent stopped seeing images — the EVO-2178 bug, reintroduced. The tests in #6 did not catch it because everyhttptestserver binds127.0.0.1, so the postback host and the media host always matched there.Why the anchor was wrong in the first place
Whoever sends the event chooses every field in it, including the one being used to decide what that same event may reach. An allowlist read out of the event therefore constrains nobody it needs to constrain — it only stops a caller who can set
attachments[].urlbut not the other fields, which is not a real attacker.The allowlist now comes from
MEDIA_HOST_ALLOWLISTonly, so the decision sits with the operator where the caller cannot pick it.A2ARequest.PostbackURLis dropped again.The scheme check and the per-redirect re-check are unchanged — those are the parts of #6 that did hold up.
Consequence
The variable is now required wherever media is expected: unset means no attachment is fetched.
k8s/configmap.yamlandk8s/deployment.yamlcarry it here; the compose/swarm/env surfaces are wired in the umbrella PR, which should land together with this one.Verification
go build ./...·go vet ./...·go test ./...— green, e2e included, against a throwaway Redis.Part of EVO-2178.
🤖 Generated with Claude Code