From 8ad121d5c3697c3927bcc8e6b311aa4098ff2896 Mon Sep 17 00:00:00 2001 From: jochen Date: Tue, 13 Oct 2026 16:26:48 +0200 Subject: [PATCH] lab: take the registry's address only from the forge's own host, built from the parsed URL (hq issue 624, review of #275) What the registry answers may come over plain HTTP, so it must not choose where packages come from, nor reach the run's npm configuration as a raw string. --- modules/lab/test/runs.test.ts | 40 +++++++++++++++++++-------- modules/lab/tools/runs.ts | 52 ++++++++++++++++++++++++++--------- 2 files changed, 68 insertions(+), 24 deletions(-) diff --git a/modules/lab/test/runs.test.ts b/modules/lab/test/runs.test.ts index 284a7164..87dc1d12 100644 --- a/modules/lab/test/runs.test.ts +++ b/modules/lab/test/runs.test.ts @@ -275,21 +275,39 @@ const packument = (tarball: string) => new Response(JSON.stringify({ test("a run names the package registry by the address the registry gives its tarballs", async () => { const r = aRegistry(() => packument("https://forge.example.org/api/packages/novox/npm/%40novox%2Fmesh-sdk/-/0.1.14/mesh-sdk-0.1.14.tgz")); - const got = await npmRegistry("http://novox.internal:20000/", r.get); + const got = await npmRegistry("http://forge.example.org:3000/", r.get); assert.deepEqual(got, { registry: "https://forge.example.org/api/packages/novox/npm/" }); - assert.deepEqual(r.asked, ["http://novox.internal:20000/api/packages/novox/npm/@novox%2fmesh-sdk"]); + assert.deepEqual(r.asked, ["http://forge.example.org:3000/api/packages/novox/npm/@novox%2fmesh-sdk"]); + // The forge's own scheme is taken too, with its port. + const plain = await npmRegistry("http://forge.example.org:3000", + aRegistry(() => packument("http://forge.example.org:3000/api/packages/novox/npm/%40novox%2Fmesh-sdk/-/0.1.14/mesh-sdk-0.1.14.tgz")).get); + assert.deepEqual(plain, { registry: "http://forge.example.org:3000/api/packages/novox/npm/" }); }); -test("a registry that does not say its address leaves the forge's, and the reason is said", async () => { - for (const answer of [ - () => new Response("not found", { status: 404 }), - () => packument("https://forge.example.org/elsewhere/mesh-sdk-0.1.14.tgz"), - () => new Response("{}", { status: 200 }), - () => { throw new Error("connect ECONNREFUSED"); }, - ]) { +// What the registry answers may come over plain HTTP, so it never decides where packages come from: only +// the forge's own host, at https or the forge's scheme, under the npm path, with nothing after it +// (review of mesh-catalog #275). +test("a registry that does not give an address the lab takes leaves the forge's, and the reason is said", async () => { + const sdk = "/api/packages/novox/npm/%40novox%2Fmesh-sdk/-/0.1.14/mesh-sdk-0.1.14.tgz"; + for (const [answer, why] of [ + [() => new Response("not found", { status: 404 }), /it answered 404/], + [() => new Response("{}", { status: 200 }), /no tarball/], + [() => { throw new Error("connect ECONNREFUSED"); }, /ECONNREFUSED/], + [() => packument(`https://forge.example.org/elsewhere/mesh-sdk-0.1.14.tgz`), /not under \/api\/packages\/novox\/npm\//], + [() => packument(`https://attacker.example.net${sdk}`), /attacker\.example\.net, which is not the forge's host novox\.internal/], + [() => packument(`https://attacker.example.net/x?u=https://novox.internal${sdk}`), /query or a fragment/], + [() => packument(`https://novox.internal${sdk}?x=1`), /query or a fragment/], + [() => packument(`https://novox.internal${sdk}#frag`), /query or a fragment/], + [() => packument(`https://attacker.example.net/#https://novox.internal${sdk}`), /query or a fragment/], + [() => packument(`ftp://novox.internal${sdk}`), /ftp: — neither https: nor the forge's http:/], + [() => packument(`file:///api/packages/novox/npm/x.tgz`), /file: — neither/], + [() => packument(`https://novox.internal/api/packages/novox/npm/\nregistry=https://attacker.example.net/\nallow-remote=all`), /whitespace or a control character/], + [() => packument(`https://user:pw@novox.internal${sdk}`), /user or password/], + ] as [() => Response, RegExp][]) { const got = await npmRegistry("http://novox.internal:20000", aRegistry(answer).get); - assert.equal(got.registry, "http://novox.internal:20000/api/packages/novox/npm/"); - assert.match(got.problem ?? "", /did not say its own address/); + assert.equal(got.registry, "http://novox.internal:20000/api/packages/novox/npm/", String(why)); + assert.match(got.problem ?? "", /did not give an address the lab takes/); + assert.match(got.problem ?? "", why); } }); diff --git a/modules/lab/tools/runs.ts b/modules/lab/tools/runs.ts index a5146648..5572b843 100644 --- a/modules/lab/tools/runs.ts +++ b/modules/lab/tools/runs.ts @@ -64,35 +64,61 @@ fi`; export const NPM_PATH = "/api/packages/novox/npm/"; /** - * The address a run names the @novox registry by: the one the registry names itself by (novox/hq issue 624). + * The address a run names the @novox registry by: the one the registry names itself by, when that is the + * forge's own host (novox/hq issue 624). * * **npm 12 installs a locked tarball only from the registry it is configured with.** Its `allow-remote` * defaults to `none`, and a lock file's `resolved` URL counts as the registry's only when its origin and * path are the configured scope registry's; any other is a "remote" tarball and refused (EALLOWREMOTE). * The forge writes every tarball URL — in what it answers, and so in every lock file made against it — at - * its own public address, whichever address it was asked at; the lab reaches it at the forge setting's, - * another one. So the lab asks the registry, at the forge, for the SDK's newest tarball and names the - * registry by the address in it, as the lock files do. Answered with the forge's own address and the - * reason when the registry does not say, so a run still starts and lab_check names it. + * its own public address, whichever address it was asked at. So the lab asks the registry, at the forge, + * for the SDK's newest tarball and names the registry by the origin of that URL. + * + * **What the registry answers is not trusted for where packages come from** (review of mesh-catalog #275): + * it may be fetched over plain HTTP. The tarball URL is parsed, and taken only when it is https (or the + * forge's own scheme), its path is under the npm path, it carries no query or fragment, and its host is + * the forge's own host. The registry is then built from + * the parsed origin and the npm path alone, never from the string answered, so nothing the registry says + * reaches the run's npm configuration but a scheme, a host and a port. Anything else falls back to the + * forge's own address and says why, so a run still starts and lab_check names it. + * + * So the lab's forge setting is the address the forge names itself by (its public https name), not a + * second name for it: a forge set as another name of the same server is refused here, named by lab_check, + * and its runs stop at the first locked @novox package. */ export async function npmRegistry(forge: string, get: typeof fetch = fetch): Promise<{ registry: string; problem?: string }> { const asked = `${forge.replace(/\/+$/, "")}${NPM_PATH}`; const url = `${asked}@novox%2fmesh-sdk`; const fallback = (why: string) => ({ registry: asked, - problem: `the package registry at ${url} did not say its own address (${why}), so a run names it ${asked}, and npm 12 refuses a locked @novox tarball at any other`, + problem: `the package registry at ${url} did not give an address the lab takes (${why}), so a run names it ${asked}, and npm 12 refuses a locked @novox tarball at any other`, }); + let own: URL; + try { + own = new URL(forge); + } catch { + return fallback(`the forge ${JSON.stringify(forge)} is not a URL`); + } try { const res = await get(url, { signal: AbortSignal.timeout(15_000) }); if (!res.ok) return fallback(`it answered ${res.status}`); - const doc = await res.json() as { "dist-tags"?: { latest?: string }; versions?: Record }; + const doc = await res.json() as { "dist-tags"?: { latest?: string }; versions?: Record }; const latest = doc["dist-tags"]?.latest ?? ""; - const tarball = doc.versions?.[latest]?.dist?.tarball ?? ""; - const at = tarball.indexOf(NPM_PATH); - if (at < 0) return fallback(tarball ? `its tarball ${tarball} is not under ${NPM_PATH}` : "no tarball for its latest version"); - const registry = tarball.slice(0, at + NPM_PATH.length); - new URL(registry); - return { registry }; + const tarball = doc.versions?.[latest]?.dist?.tarball; + if (typeof tarball !== "string" || !tarball) return fallback("no tarball for its latest version"); + if (/[\s\u0000-\u001f\u007f]/.test(tarball)) return fallback("its tarball URL holds whitespace or a control character"); + let t: URL; + try { + t = new URL(tarball); + } catch { + return fallback("its tarball is not a URL"); + } + if (t.protocol !== "https:" && t.protocol !== own.protocol) return fallback(`its tarball is ${t.protocol} — neither https: nor the forge's ${own.protocol}`); + if (t.username || t.password) return fallback("its tarball URL carries a user or password"); + if (t.search || t.hash) return fallback("its tarball URL carries a query or a fragment"); + if (!t.pathname.startsWith(NPM_PATH)) return fallback(`its tarball is not under ${NPM_PATH}`); + if (t.hostname !== own.hostname) return fallback(`its tarball is at ${t.hostname}, which is not the forge's host ${own.hostname}`); + return { registry: `${t.origin}${NPM_PATH}` }; } catch (e) { return fallback(e instanceof Error ? e.message : String(e)); }