From 0d24f4da491dc63fbefd540fea5cba1607eeb6e0 Mon Sep 17 00:00:00 2001 From: David McKay Date: Thu, 20 Aug 2026 10:44:46 -0700 Subject: [PATCH] Check the address a supervisor hands back before calling it `resolveBaseUrl` returns a URL and the next line puts it into `fetch` with this deployment's computer token on it. That was fine while the only supervisor was ours, answering on loopback. It stops being fine with a hosted provider, where the address arrives from a third party's API and we call whatever it says. Deliberately not the navigation check. That one refuses private hosts, which is correct for a Bot browsing and exactly wrong here: our own supervisor answers `http://127.0.0.1:` for a container on this machine, so reusing it would refuse the ordinary case. What survives is what holds however the address was produced. The scheme has to be one a computer speaks, and the cloud metadata addresses are refused whatever anything says, because that is how a container's credentials leave it and no supervisor has a reason to name one. Groundwork for A10, where the supervisor becomes somebody else's API. --- server/src/computer/client.ts | 24 ++++++++++--- server/src/computer/target.ts | 44 +++++++++++++++++++++++ server/tests/computer-target.test.ts | 53 +++++++++++++++++++++++++++- 3 files changed, 115 insertions(+), 6 deletions(-) diff --git a/server/src/computer/client.ts b/server/src/computer/client.ts index f019ef81..26bd83e8 100644 --- a/server/src/computer/client.ts +++ b/server/src/computer/client.ts @@ -22,7 +22,7 @@ import type { WriteFileInput, WriteFileResult, } from "./schema"; -import { checkNavigationTarget } from "./target"; +import { checkComputerAddress, checkNavigationTarget } from "./target"; /** * How the server talks to a Bot's computer. @@ -164,10 +164,24 @@ export function createComputerClient(options: ComputerClientOptions) { // Outside the try below on purpose: that catch reports "the computer is not running", which is // true of a computer that will not answer and misleading about a supervisor that could not be // reached or refused. Those are different operator-facing problems. - const target = - botId && options.resolveBaseUrl - ? (await options.resolveBaseUrl(botId)).replace(/\/$/, "") - : base; + let target: string; + if (botId && options.resolveBaseUrl) { + const located = (await options.resolveBaseUrl(botId)).replace( + /\/$/, + "", + ); + // Checked because this address is not necessarily ours. A hosted provider answers from its + // own API, and whatever comes back is about to be called with this deployment's computer + // token. Our own supervisor answers with a private address, which is fine and why this is + // not the navigation check. + const verdict = checkComputerAddress(located); + if (!verdict.allowed) { + throw new ComputerUnavailableError(verdict.reason); + } + target = located; + } else { + target = base; + } // Already stopped before this left: do not dispatch at all. Relying on fetch to reject an // aborted signal makes "did the click happen" depend on how quickly the runtime notices, and diff --git a/server/src/computer/target.ts b/server/src/computer/target.ts index 99d7719f..937879e3 100644 --- a/server/src/computer/target.ts +++ b/server/src/computer/target.ts @@ -56,6 +56,50 @@ function isPrivateIpv4(hostname: string): boolean { return false; } +/** + * Decide whether an address a supervisor handed back may be called at all. + * + * Deliberately not {@link checkNavigationTarget}. That one judges where a Bot may browse, and its + * private-host rule is exactly wrong here: our own supervisor answers with `http://127.0.0.1:` + * for a container on this machine, so applying it would refuse the normal case. + * + * What survives is what holds however the address was produced. The scheme must be one we speak, and + * the cloud metadata addresses are refused whatever anything says, because that is how a container's + * credentials leave it and no supervisor has a reason to name one. + * + * This matters because the address stops being ours. With a hosted provider (A10) it arrives from a + * third party's API and goes straight into `fetch` carrying this deployment's computer token, so it + * is worth one check that it is an address rather than a surprise. + */ +export function checkComputerAddress(raw: string): TargetVerdict { + let url: URL; + try { + url = new URL(raw); + } catch { + return { + allowed: false, + reason: `The computer's address is not a URL: ${raw}`, + }; + } + + if (!ALLOWED_PROTOCOLS.has(url.protocol)) { + return { + allowed: false, + reason: `A computer must be reached over http or https, not ${url.protocol.replace(":", "")}.`, + }; + } + + if (NEVER_ALLOWED_HOSTNAMES.has(url.hostname.toLowerCase())) { + return { + allowed: false, + reason: + "That address holds this deployment's own cloud credentials, so it is never called as a computer.", + }; + } + + return { allowed: true, url: url.toString() }; +} + /** * Decide whether a Bot may navigate here. * diff --git a/server/tests/computer-target.test.ts b/server/tests/computer-target.test.ts index 36456b31..c3acadf6 100644 --- a/server/tests/computer-target.test.ts +++ b/server/tests/computer-target.test.ts @@ -1,5 +1,8 @@ import { describe, expect, test } from "bun:test"; -import { checkNavigationTarget } from "../src/computer/target"; +import { + checkComputerAddress, + checkNavigationTarget, +} from "../src/computer/target"; describe("navigation targets", () => { test("allows an ordinary public address", () => { @@ -77,3 +80,51 @@ describe("navigation targets", () => { expect(checkNavigationTarget("http://172.31.255.255/").allowed).toBe(false); }); }); + +/** + * The address a supervisor hands back, before anything is called on it. + * + * Not the navigation check. Our own supervisor answers with a loopback address for a container on + * this machine, so refusing private hosts here would refuse the ordinary case. What is worth + * checking is that the thing is an address at all, and that it is not the one place a token must + * never be sent, because with a hosted provider it arrives from somebody else's API. + */ +describe("checkComputerAddress", () => { + test("allows the private address our own supervisor returns", () => { + expect(checkComputerAddress("http://127.0.0.1:49213")).toEqual({ + allowed: true, + url: "http://127.0.0.1:49213/", + }); + }); + + test("allows a hosted provider's public address", () => { + const verdict = checkComputerAddress("https://sandbox-abc123.daytona.app"); + expect(verdict.allowed).toBe(true); + }); + + test.each(["169.254.169.254", "metadata.google.internal", "metadata.goog"])( + "refuses the cloud metadata address %s however it arrived", + (host) => { + const verdict = checkComputerAddress(`http://${host}/latest/meta-data/`); + expect(verdict.allowed).toBe(false); + if (!verdict.allowed) { + expect(verdict.reason).toContain("cloud credentials"); + } + }, + ); + + test.each(["file:///etc/passwd", "ftp://example.com", "gopher://x"])( + "refuses %s, which is not a scheme a computer speaks", + (raw) => { + expect(checkComputerAddress(raw).allowed).toBe(false); + }, + ); + + test("refuses something that is not a URL", () => { + const verdict = checkComputerAddress("not-an-address"); + expect(verdict.allowed).toBe(false); + if (!verdict.allowed) { + expect(verdict.reason).toContain("not a URL"); + } + }); +});