Repository navigation
fix(engine): downloads refuse hosts that resolve to private addresses - #6571
Conversation
![heygengenesis[bot]](https://avatars.githubusercontent.com/in/2913576?s=60&v=4){kind=small}
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 137 of thoseThe gate passes. Quarantined, measured but not gated (0) |
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.
There was a problem hiding this comment.
✅ Fixed in d5d455e — npx shim green on all 3 OSes.
e5856f0 to
7cdf401
Compare
There was a problem hiding this comment.
Verdict: REQUEST CHANGES — 2 resolved, 0 open, 2 new · e5856f0 → d5d455e
- Required CI red (run 40744720190), caused by this PR: 16 producer tests still stub the global
fetch, so the undici transport never calls the stub and the downloads fail with ENOTFOUND. These are 15 inproducer/src/services/htmlCompiler.test.ts(localizeRemoteMediaSources/ImageSources/FontFaces, stubs at :1952, :1980, :2008 …) anddistributed/plan.test.ts:712("maps an open-ended remote video HTTP 404…", integration lane). Main is green. Move them to_setDownloadFetchForTests, likeserver.test.ts. - Body is missing a
fails without the fix:line naming the test, e.g.fails without the fix: urlDownloader.publicAddress.test.ts > ….
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.fails without the fix: urlDownloader.publicAddress.test.ts > the private IPv4, mixed link-local and loopback IPv6 cases (each expects the HF_NON_PUBLIC_ADDRESS refusal).
Checked on a Linux box