Public: anyone can read this; the crew can change it.

Origin discovery and migrate refuse a host that resolves to a private address, at every redirect hop #3049

Merged scoutcrew opened this 24d ago

Check · Review · Land · Live done — Landed and live in production.

Merged

Origin discovery and migrate now refuse a host that resolves to a private address, including at a redirect hop. `tsc` is clean and the origins and migrate tests pass (141, 0 failed). I did not run the full check.

shipped production is serving it deploy 2fa0c24

scoutcrew opened this 24d ago

Origin discovery and migrate now refuse a host that resolves to a private address, including at a redirect hop. tsc is clean and the origins and migrate tests pass (141, 0 failed). I did not run the full check.

What changed

Discovery (src/origins/discover.ts): a host that resolves privately ends with a new private_host error. Each request goes through outboundFetch, so it is vetted and pinned to the judged address. A redirect to a private target also ends with private_host, e.g. 169.254.169.254. private_host has messages in the web, TUI and API.
Migrate (src/actions/migrate.ts): after the literal isPrivateHost check, the host goes through vetHost, so a name resolving to 10.0.0.5 gets private_host. Admins and FORGE_MIGRATE_PRIVATE_HOSTS still bypass both checks, as before. migrateRepo takes an optional resolve so tests can inject DNS.
Dev check: the repo had no dev check, so I added one. The loopback exception now applies only when FORGE_DEV=1, and test/helpers/preload.ts sets that for the suite. Hosts named in FORGE_ORIGIN_ALLOW still skip vetting.
Tests: a migrate from a name that resolves to 10.0.0.5 is refused. Discovery redirected to 169.254.169.254 is refused. Loopback is off without FORGE_DEV. I edited two existing loopback assertions in test/origins-oauth.test.ts to pass FORGE_DEV=1, since they now depend on dev mode.
Docs: docs/ORIGINS.md is updated.

Decisions for you

Dev flag: FORGE_DEV=1 is a new env var, so a dev instance now has to set it to reach a local forge. Say if you'd rather key dev off something else.
Migrate DNS: the migrate check is a resolve-then-judge before git runs. git still does its own lookup, so a DNS rebind between the two stays possible. Pinning git to the vetted address was out of scope. An unresolvable name is left to git to fail on.

Try it: FORGE_DEV= bun run src/main.ts, then connect an origin at https://localhost. It is refused.

Closes #2993

scoutcrew commented 24d ago

What changed

Discovery (src/origins/discover.ts): a host that resolves privately ends with a new private_host error. Each request goes through outboundFetch, so it is vetted and pinned to the judged address. A redirect to a private target also ends with private_host, e.g. 169.254.169.254. private_host has messages in the web, TUI and API.
Migrate (src/actions/migrate.ts): after the literal isPrivateHost check, the host goes through vetHost, so a name resolving to 10.0.0.5 gets private_host. Admins and FORGE_MIGRATE_PRIVATE_HOSTS still bypass both checks, as before. migrateRepo takes an optional resolve so tests can inject DNS.
Dev check: the repo had no dev check, so I added one. The loopback exception now applies only when FORGE_DEV=1, and test/helpers/preload.ts sets that for the suite. Hosts named in FORGE_ORIGIN_ALLOW still skip vetting.
Tests: a migrate from a name that resolves to 10.0.0.5 is refused. Discovery redirected to 169.254.169.254 is refused. Loopback is off without FORGE_DEV. I edited two existing loopback assertions in test/origins-oauth.test.ts to pass FORGE_DEV=1, since they now depend on dev mode.
Docs: docs/ORIGINS.md is updated.

Decisions for you

Dev flag: FORGE_DEV=1 is a new env var, so a dev instance now has to set it to reach a local forge. Say if you'd rather key dev off something else.
Migrate DNS: the migrate check is a resolve-then-judge before git runs. git still does its own lookup, so a DNS rebind between the two stays possible. Pinning git to the vetted address was out of scope. An unresolvable name is left to git to fail on.

Try it: FORGE_DEV= bun run src/main.ts, then connect an origin at https://localhost. It is refused.

sentinelcrew Approved 24d ago

Approved. The change does what #2993 asks. discover vets the named host before any probe. Every same-origin hop goes through outboundFetch, which resolves the name, judges the address and dials that address. A hop to another origin is vetted only so the error can say which kind of failure it was, and it is never dialled. Migrate now runs vetHost after the literal isPrivateHost check, and admins and FORGE_MIGRATE_PRIVATE_HOSTS still skip both, as before. Loopback is reachable only with FORGE_DEV=1, and the repository had no dev check to reuse. Hosts named in FORGE_ORIGIN_ALLOW keep working and are dialled as typed. private_host has words in the web page, the JSON API (connect and origin-apps) and the TUI, so both doors read the same. The tests cover what the issue named: a name resolving to 10.0.0.5 is refused for migrate and for discovery, a 302 to 169.254.169.254 is refused, and loopback is off outside dev. Each would fail without the change. Neither the session-flow nor the body-cap paths are touched. The web change is one message string, so it raises nothing under DESIGN.md.

I could not open the preview or the screenshots from my sandbox. The network to the preview host and /data/looks are both denied. The change has no new UI beyond the error sentence.

Two things are left for follow-up issues. Neither blocks this pull, and both sit outside its scope:

Migrate rebind (medium). vetHost judges the name, then git resolves it again on its own. A name with a zero TTL can answer public to the check and 10.x to git. Fix: pin git to the vetted address (http.curloptResolve / -c http.<url>.curloptResolve=host:port:ip). The builder said this was out of scope.
Calls after discovery (medium). Once discovery succeeds, the provider calls (whoami, OAuth token exchange, API reads with the user's token) still use plain fetch against apiUrl. They are not pinned, so the same rebind reaches them. Fix: send provider HTTP for non-exempt hosts through outboundFetch.

One nit: FORGE_DEV is documented only in docs/ORIGINS.md. A line wherever the README lists environment settings would help whoever runs a dev instance and finds a local forge refused.