Skip to content

Fix Cloudflare Worker remote media conversion#727

Merged
Olmo Maldonado (ibolmo) merged 4 commits into
mainfrom
fix-dns-lookup
Jun 2, 2026
Merged

Fix Cloudflare Worker remote media conversion#727
Olmo Maldonado (ibolmo) merged 4 commits into
mainfrom
fix-dns-lookup

Conversation

@ibolmo
Copy link
Copy Markdown
Collaborator

@ibolmo Olmo Maldonado (ibolmo) commented Jun 2, 2026

Context

Remote media normalization runs inside the Cloudflare Worker proxy for multimodal provider requests. The previous hostname validation/fetch path depended on Node DNS lookup plus the Node HTTP transport; remote Worker testing showed public media URLs can fail through that transport even though the same URLs work through Worker-native fetch.

Description

  • Resolve media hostnames with Resolver.resolve4() / resolve6() instead of dns.promises.lookup().
  • Keep rejecting unsafe literal IPs and hostnames that resolve to private/reserved addresses before fetching.
  • Use Worker-native fetch for Cloudflare Workers while preserving the DNS-pinned Node http/https transport for Node runtimes.
  • Add focused provider util coverage for the Worker fetch path and private-resolution rejection.
  • Add pnpm run test:cloudflare-worker plus a gated CI job that runs a remote Wrangler Worker media conversion check when TEST_CLOUDFLARE_API_TOKEN and TEST_CLOUDFLARE_ACCOUNT_ID secrets are available.

@vercel
Copy link
Copy Markdown

vercel Bot commented Jun 2, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
ai-proxy Ready Ready Preview, Comment Jun 2, 2026 10:17pm

Request Review

@ibolmo Olmo Maldonado (ibolmo) changed the title Fix dns lookup Fix Cloudflare Worker remote media conversion Jun 2, 2026
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 5db75087ee

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "Codex (@codex) review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "Codex (@codex) address that feedback".


return { address: addresses[0].address, url };
return {
address: transport === "node" ? addresses[0] : hostname,
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Pin the checked address before Worker fetches

When running in Cloudflare Workers, this returns the original hostname after resolveHostnameAddresses() has validated a public address, and fetchValidatedMediaUrl() then calls native fetch(url.toString()), which performs a separate DNS resolution. For an attacker-controlled media hostname that returns a public IP during validation and then a private/link-local IP for the subsequent fetch, this reopens the SSRF case that the Node path avoids by connecting to addresses[0].

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Collaborator Author

@ibolmo Olmo Maldonado (ibolmo) Jun 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can't really fix this. this requires other node modules that are not in cloudflare. what this is complaining about is:

validate...()
<-- in between these two someone could change the url to poitn to something else
fetch();

because we're movign away from the ai proxy "soon" i think it's acceptable to avoid redirect pinning for cloudflare for now.

Comment thread packages/proxy/package.json
Copy link
Copy Markdown
Contributor

@CLowbrow Alex Z (CLowbrow) left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please fix codex p1

@ibolmo Olmo Maldonado (ibolmo) merged commit f3bd96d into main Jun 2, 2026
7 of 8 checks passed
@ibolmo Olmo Maldonado (ibolmo) deleted the fix-dns-lookup branch June 2, 2026 22:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants