Add option to restrict media domains

[russellb]

https://github.com/vllm-project/vllm/security/advisories/GHSA-3f6c-7fw2-ppm4j

[mergify]

This pull request has merge conflicts that must be resolved before it can be
merged. Please rebase the PR, @russellb.

https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork

[gemini-code-assist]

Code Review

This pull request introduces a security feature to restrict the domains from which multimodal media can be fetched, aiming to mitigate Server-Side Request Forgery (SSRF) vulnerabilities. The implementation adds an --allowed-media-domains argument and checks it before fetching media from HTTP(S) URLs. While this is a good step, I've identified a critical security flaw where HTTP redirects can bypass this check. I've also included a suggestion to improve the performance of the domain checking logic. The documentation and test changes look good, but the tests should also cover the redirect scenario.

[gemini-code-assist]

The current implementation checks the domain of the initial URL, but it does not prevent SSRF vulnerabilities that arise from HTTP redirects. If an allowed URL redirects to a URL on a disallowed domain (including internal network addresses), connection.get_bytes might still fetch it if it follows redirects by default, which is mentioned as the root cause in the security advisory GHSA-3f6c-7fw2-ppm4j.

To properly mitigate this, you should either disable redirects or verify the domain of the final URL after all redirects have been followed. Disabling redirects might be the safest option if they are not a required feature. If they are required, the HTTPConnection class should be modified to not follow redirects automatically, and instead, redirects should be handled manually within MediaConnector to ensure every URL in the redirect chain is validated against the allowed domains.

[russellb]

@huachenheli @DarkLight1337 this seems like a good point. I wouldn't block merging the current change over this, but it seems worth a follow-up change.

[huachenheli]

Yeah. This PR should be fine for immediate needs. We already have media_io_kwargs that we can use to control MediaConnector behavior, so we just need to pass that to the HttpConnection to disallow redirects.

[gemini-code-assist]

For performance, it's better to convert allowed_media_domains to a set during initialization. Checking for an item's existence is O(1) on average for a set, while it's O(n) for a list. This can be significant if the list of allowed domains is large.

Suggested change

	if allowed_media_domains is None:
	allowed_media_domains = []
	self.allowed_media_domains = allowed_media_domains
	if allowed_media_domains is None:
	self.allowed_media_domains = set()
	else:
	self.allowed_media_domains = set(allowed_media_domains)