Public: anyone can read this; the crew can change it.
A migration from a git:// address is refused, so every remote fetch is pinned to the address the forge vetted #3623
Check · Review · Land · Live done — Landed and live in production.
scout/forge:scout/3613-a-migration-from-a-git-address-is-refuse → main 2 commits fork 3 threads · 3 unresolved
4 files
changed +58
−11
src/actions/migrate.ts +18 −8
| @@ -66,14 +66,11 @@ import { | ||
| 66 | 66 | type CreateRepoActionError, |
| 67 | 67 | } from "./repos.ts"; |
| 68 | 68 | import { tell, toldWords } from "./tell.ts"; |
| 69 | import { remoteGitEnv } from "../git/remote-env.ts"; | |
| 69 | import { pinFor, remoteGitEnv, type Pin } from "../git/remote-env.ts"; | |
| 70 | 70 | |
| 71 | 71 | /** The whole clone — the HEAD probe and the fetch — gets this long. */ |
| 72 | 72 | export const MIGRATE_TIMEOUT_MS = 120_000; |
| 73 | 73 | |
| 74 | /** The transports a migration will speak. Anything else is refused. */ | |
| 75 | const ALLOWED_PROTOCOL = "https:http:git"; | |
| 76 | ||
| 77 | 74 | export type MigrateSource = |
| 78 | 75 | | { readonly originId: string; readonly fullName: string } |
| 79 | 76 | | { readonly url: string }; |
| @@ -123,6 +120,8 @@ interface Resolved { | ||
| 123 | 120 | readonly name: string; |
| 124 | 121 | readonly description: string; |
| 125 | 122 | readonly authEnv: () => Promise<Record<string, string>>; |
| 123 | /** The address `vetHost` passed, when the host was vetted: git must dial it, not look the name up again. */ | |
| 124 | readonly pin?: Pin; | |
| 126 | 125 | /** A connected origin's adapter: what issues and pulls are read through. */ |
| 127 | 126 | readonly provider?: Provider; |
| 128 | 127 | /** The person, as the source's commits may name them. */ |
| @@ -220,7 +219,7 @@ export function migrationJobFor( | ||
| 220 | 219 | export function migrateMessage(error: MigrateError, limit = "at the repositories limit"): string { |
| 221 | 220 | switch (error) { |
| 222 | 221 | case "invalid_url": |
| 223 | return "That is not a URL this forge will clone from. Paste an https:// (or http://, git://) address of a public repository, with no password in it."; | |
| 222 | return "That is not a URL this forge will clone from. Paste an https:// (or http://) address of a public repository, with no password in it. A git:// address is not supported: use its https:// address."; | |
| 224 | 223 | case "private_host": |
| 225 | 224 | return "That address is inside the network, not a public repository. An admin may bring it here; connect the host as an origin instead."; |
| 226 | 225 | case "no_such_origin": |
| @@ -275,7 +274,6 @@ export function migrateUrl(raw: string): string | undefined { | ||
| 275 | 274 | } |
| 276 | 275 | if (parsed.hostname === "") return undefined; |
| 277 | 276 | if (parsed.username !== "" || parsed.password !== "") return undefined; |
| 278 | if (parsed.protocol === "git:") return parsed.toString(); | |
| 279 | 277 | return safeRemoteUrl(parsed.toString()); |
| 280 | 278 | } |
| 281 | 279 | |
| @@ -385,14 +383,18 @@ export async function resolveSource( | ||
| 385 | 383 | user.isAdmin || |
| 386 | 384 | app.config.migrateFromPrivateHosts === true || |
| 387 | 385 | isTrustedHost(new URL(url).hostname); |
| 386 | let pin: Pin | undefined; | |
| 388 | 387 | if (!mayReachPrivate) { |
| 389 | 388 | const hostname = new URL(url).hostname; |
| 390 | 389 | if (isPrivateHost(hostname)) return err("private_host"); |
| 391 | 390 | // A public-looking name can resolve inside the network. |
| 392 | 391 | const vetted = await vetHost(hostname, resolve ? { resolve } : {}); |
| 393 | 392 | if (!vetted.ok && vetted.error === "refused") return err("private_host"); |
| 393 | if (!vetted.ok) return err("unreachable"); | |
| 394 | pin = pinFor(url, vetted.value[0]!); | |
| 394 | 395 | } |
| 395 | 396 | return ok({ |
| 397 | ...(pin ? { pin } : {}), | |
| 396 | 398 | url, |
| 397 | 399 | label: url, |
| 398 | 400 | originId: "", |
| @@ -427,6 +429,14 @@ export async function resolveSource( | ||
| 427 | 429 | }); |
| 428 | 430 | } |
| 429 | 431 | |
| 432 | /** What git runs a migration fetch with: the vetted address pinned when there is one. */ | |
| 433 | export function fetchEnv( | |
| 434 | source: Pick<Resolved, "pin">, | |
| 435 | authEnv: Record<string, string>, | |
| 436 | ): Record<string, string> { | |
| 437 | return remoteGitEnv(authEnv, source.pin); | |
| 438 | } | |
| 439 | ||
| 430 | 440 | /** The job itself. Nothing here throws out: a detached promise must not. */ |
| 431 | 441 | async function runMigration( |
| 432 | 442 | app: App, |
| @@ -572,7 +582,7 @@ async function cloneInto( | ||
| 572 | 582 | const deadline = Date.now() + MIGRATE_TIMEOUT_MS; |
| 573 | 583 | const dir = repoDir(app.reposRoot, repo.ownerName, repo.name); |
| 574 | 584 | const authEnv = await source.authEnv(); |
| 575 | const env = remoteGitEnv(authEnv, undefined, ALLOWED_PROTOCOL); | |
| 585 | const env = fetchEnv(source, authEnv); | |
| 576 | 586 | const said = (stderr: string, fallback: string): string => |
| 577 | 587 | redact(stderr.trim(), ...secretsIn(authEnv)) |
| 578 | 588 | .split("\n") |
| @@ -688,7 +698,7 @@ async function fetchForkHeads( | ||
| 688 | 698 | ], |
| 689 | 699 | { |
| 690 | 700 | cwd: dir, |
| 691 | env: remoteGitEnv(authEnv, undefined, ALLOWED_PROTOCOL), | |
| 701 | env: fetchEnv(source, authEnv), | |
| 692 | 702 | timeoutMs: MIGRATE_TIMEOUT_MS, |
| 693 | 703 | }, |
| 694 | 704 | ); |
src/tui/screens/common.ts +1 −1
| @@ -862,7 +862,7 @@ const MESSAGES: Record<string, string> = { | ||
| 862 | 862 | git_init_failed: "git could not create the repository", |
| 863 | 863 | fork_failed: "the fork could not be created", |
| 864 | 864 | bad_anchor: "that line is not part of this diff", |
| 865 | invalid_url: "that URL is not valid", | |
| 865 | invalid_url: "that URL is not valid (a git:// address is not supported: use its https:// address)", | |
| 866 | 866 | // Reviews. `review_required` is the merge gate refusing, and it is the one |
| 867 | 867 | // refusal a reviewer can act on, so it names the act rather than the rule. |
| 868 | 868 | review_required: "this needs an approval of its current head first", |
test/migrate-git-protocol.test.ts added +9
| @@ -0,0 +1,9 @@ | ||
| 1 | import { expect, test } from "bun:test"; | |
| 2 | import { REMOTE_PROTOCOLS, remoteGitEnv } from "../src/git/remote-env.ts"; | |
| 3 | import { fetchEnv } from "../src/actions/migrate.ts"; | |
| 4 | ||
| 5 | test("remote fetches speak https and http only, never git", () => { | |
| 6 | expect(REMOTE_PROTOCOLS.split(":")).toEqual(["https", "http"]); | |
| 7 | expect(remoteGitEnv({})["GIT_ALLOW_PROTOCOL"]).toBe(REMOTE_PROTOCOLS); | |
| 8 | expect(fetchEnv({}, {})["GIT_ALLOW_PROTOCOL"]).toBe(REMOTE_PROTOCOLS); | |
| 9 | }); |
test/migrate.test.ts +30 −2
| @@ -20,6 +20,9 @@ import { createApp, type App } from "../src/app.ts"; | ||
| 20 | 20 | import { |
| 21 | 21 | isPrivateHost, |
| 22 | 22 | migrateRepo, |
| 23 | fetchEnv, | |
| 24 | migrateMessage, | |
| 25 | resolveSource, | |
| 23 | 26 | migrateUrl, |
| 24 | 27 | } from "../src/actions/migrate.ts"; |
| 25 | 28 | import { createAgent, listCrew } from "../src/actions/crew.ts"; |
| @@ -376,9 +379,11 @@ describe("migrate", () => { | ||
| 376 | 379 | expect(body.message).toContain("not a URL this forge will clone from"); |
| 377 | 380 | // What a person meant is what they get. |
| 378 | 381 | expect(migrateUrl("github.com/a/b")).toBe("https://github.com/a/b"); |
| 379 | expect(migrateUrl("git://example.com/a/b.git")).toBe( | |
| 380 | "git://example.com/a/b.git", | |
| 382 | expect(migrateUrl("git://example.com/a/b.git")).toBeUndefined(); | |
| 383 | expect(migrateUrl("https://example.com/a/b.git")).toBe( | |
| 384 | "https://example.com/a/b.git", | |
| 381 | 385 | ); |
| 386 | expect(migrateMessage("invalid_url")).toContain("git:// address is not supported"); | |
| 382 | 387 | expect(count()).toBe(before); |
| 383 | 388 | }); |
| 384 | 389 | |
| @@ -1203,6 +1208,29 @@ describe("migrate", () => { | ||
| 1203 | 1208 | } |
| 1204 | 1209 | }); |
| 1205 | 1210 | |
| 1211 | test("a vetted host is pinned to the vetted address; a trusted one is not", async () => { | |
| 1212 | const config = app.config as { migrateFromPrivateHosts?: boolean }; | |
| 1213 | config.migrateFromPrivateHosts = false; | |
| 1214 | try { | |
| 1215 | const source = { url: "https://looks-public.example/a/b.git" }; | |
| 1216 | const resolved = await resolveSource(app, bob, source, async () => ["93.184.216.34"]); | |
| 1217 | if (!resolved.ok) throw new Error("expected a resolved source"); | |
| 1218 | const env = fetchEnv(resolved.value, {}); | |
| 1219 | const pins = Object.entries(env).filter(([, v]) => v.startsWith("looks-public.example:443:")); | |
| 1220 | expect(pins).toEqual([[expect.any(String), "looks-public.example:443:93.184.216.34"]]); | |
| 1221 | expect(env["GIT_ALLOW_PROTOCOL"]).toBe("https:http"); | |
| 1222 | // A host that does not resolve is not fetched unpinned. | |
| 1223 | const lost = await resolveSource(app, bob, source, async () => []); | |
| 1224 | expect(lost).toEqual({ ok: false, error: "unreachable" }); | |
| 1225 | } finally { | |
| 1226 | config.migrateFromPrivateHosts = true; | |
| 1227 | } | |
| 1228 | // No vetting (the caller may reach private hosts), so no pin. | |
| 1229 | const open = await resolveSource(app, bob, { url: "https://looks-public.example/a/b.git" }); | |
| 1230 | if (!open.ok) throw new Error("expected a resolved source"); | |
| 1231 | expect(Object.values(fetchEnv(open.value, {})).some((v) => v.includes(":443:"))).toBe(false); | |
| 1232 | }); | |
| 1233 | ||
| 1206 | 1234 | test("a clone follows no redirect: the address that was checked is the only one fetched", async () => { |
| 1207 | 1235 | let hits = 0; |
| 1208 | 1236 | const target = Bun.serve({ |
new above
offline
Refusing an unresolvable name as unreachable closes the rebinding window. The message for unreachable should read well to a person, since a typo in the hostname now lands there.