Repository navigation
fix(engine): downloads refuse hosts that resolve to private addresses - #6571
miguel-heygen wants to merge 2 commits into
Conversation
Downloads checked the host name in the URL but connected to whatever the name resolved to, so a public name pointing at a private address was fetched. Every download now connects through a resolver that refuses any answer in a private, loopback or link-local range, on the address it actually connects to. The transport is undici's own fetch with an Agent, loaded on the first download; the tests replace it through one seam instead of stubbing the global fetch.
There was a problem hiding this comment.
Verdict: REQUEST CHANGES · 1 blocker · 0 important · 0 nits · head 3ac69653
The address check itself is sound: it runs on the connect-time lookup, handles all: true for happy-eyeballs, and the new test exercises real undici. The blocker is that this transport drops env-proxy support. Inline.
— Genesis
Edit accuracy: accurate 271 (base branch 271), smooth 170 of thoseThe gate passes. Quarantined, measured but not gated (0) |
The address check resolves the host on this machine, which a network that only reaches the internet through HTTPS_PROXY cannot do. When Node's fetch follows the proxy variables, downloads now use it and the URL and redirect checks stay the guard. The test for "does fetch follow the proxy" moves to core, where the CLI's DNS probe and its publish hint use the same one.
There was a problem hiding this comment.
Verdict: REQUEST CHANGES — 1 resolved, 0 open, 2 new · 3ac6965 → e5856f0
The proxy fix is right. Required CI is red on this head (run 40566977216) for two reasons this PR causes:
- Producer: unit tests.
packages/producer/src/server.test.ts:229and:238(prepareRenderBody — previewUrl) still stub the globalfetch.fetchPublicHttpsTextnow goes through undici, so the stub is never called: 0 calls, and noprepared. This is the same move the engine tests got. Switch them to_setDownloadFetchForTests, which may need exporting from the engine index. - CLI: npx shim (all 3 OSes). See inline.
— Genesis
| @@ -1,13 +1,8 @@ | |||
| import { promises as dns } from "node:dns"; | |||
| import { fetchGoesThroughEnvProxy } from "@hyperframes/core/env-proxy"; | |||
There was a problem hiding this comment.
blocker (CI): the npx-shim job doesn't build core, so under Node the node condition resolves @hyperframes/core/env-proxy to dist/envProxy.js, which doesn't exist. skills.test.ts fails 40 tests with ERR_MODULE_NOT_FOUND (via updateCheck.ts → hostAnswers.ts). Either build core in that job or keep this import out of the shim's module graph.
What changes
Downloads refuse hosts that resolve to private addresses. The downloader already refused private, loopback and link-local host names written in the URL and re-checked every redirect; it still connected to whatever a public name resolved to. Now every download resolves the host itself and refuses to connect when any answer is a private, loopback, link-local or otherwise reserved address (the same ranges the URL check uses, one list). The check runs on the address the connection uses, so there is no second lookup to race.
fetchwith anAgentwhoseconnect.lookupdoes the check (undici7.25.0, exact, the version already in the lockfile; a dependency of the engine and the CLI, which keeps it external like the browser packages). It is imported on the first download, so a process that downloads nothing pays nothing._setDownloadFetchForTests) instead of stubbing the globalfetch; the existing download tests moved to it unchanged otherwise.HTTPS_PROXYwithNODE_USE_ENV_PROXY=1or--use-env-proxy) the proxy resolves the name, so downloads use Node's ownfetchthere and the URL and redirect checks stay the guard.fetchGoesThroughEnvProxymoved to@hyperframes/core/env-proxy, one owner for the CLI's DNS probe, the publish hint and the downloader.http_rejectedfailure ("Download URL is not permitted"), so it is not retried.Tests
urlDownloader.publicAddress.test.tsresolves a host to a private IPv4, a link-local address mixed in with a public one, and a loopback IPv6 address: all three are rejected without a retry, fordownloadToTempandfetchPublicHttpsText. They fail with the address check removed.Checked on a Linux box