fix(web): address Copilot review round 1 on public addresses (#1862)
- Never echo a refused value into a warning: credentials, query and fragment are dropped, and an unparseable value is not shown at all. - Do not adopt MCP_SANDBOX_FULL_ADDRESS / MCP_APP_ORIGIN_FULL_ADDRESS after the EADDRINUSE fallback: the reverse proxy still routes them to the pinned port, which another process holds. - Refuse a public app origin equal to any trusted ancestor in the controller, where the real (possibly bind-derived, possibly fallback-port) sandbox origin is known; config time could not see it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HzuzRPJv7M8wGe3wyw5sSg Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
cliffhall
parent
12544d251c
commit
4dc5eaa6bb
6 changed files with 154 additions and 8 deletions
|
|
@ -328,6 +328,41 @@ export function createAppOriginController(
|
|||
// dedicated-origin rendering down. Retry once on an OS-assigned port,
|
||||
// loudly — whoever pinned the port to forward it needs to know.
|
||||
let retriedDynamic = false;
|
||||
|
||||
/**
|
||||
* The origin to publish under once bound: the operator's public origin
|
||||
* (#1862) when it is still safe to use, else the bind-derived one.
|
||||
*
|
||||
* Two reasons to refuse it here rather than only at config time:
|
||||
* - After the EADDRINUSE fallback the reverse proxy still routes the
|
||||
* public origin to the PINNED port, which someone else holds.
|
||||
* - It must differ from every trusted ancestor — the sandbox proxy and
|
||||
* the Inspector page, i.e. `embedderOrigins`. The frame is granted
|
||||
* `allow-same-origin` on this path, so sharing either origin hands
|
||||
* the app that realm. Config time cannot see a bind-derived sandbox
|
||||
* origin (it is known only once that listener binds, possibly on a
|
||||
* fallback port); the callers pass the real one here.
|
||||
*/
|
||||
const adoptedOrigin = (derived: string): string => {
|
||||
if (!publicOrigin) return derived;
|
||||
if (retriedDynamic) {
|
||||
console.warn(
|
||||
`App origin: not using MCP_APP_ORIGIN_FULL_ADDRESS, since it routes to port ${port}, ` +
|
||||
`which is taken; serving app documents from ${derived} instead.`,
|
||||
);
|
||||
return derived;
|
||||
}
|
||||
if (embedderOrigins?.includes(publicOrigin)) {
|
||||
console.warn(
|
||||
`App origin: not using MCP_APP_ORIGIN_FULL_ADDRESS=${publicOrigin}: it is the sandbox's or ` +
|
||||
`the Inspector UI's origin, and a dedicated app origin must differ from both; ` +
|
||||
`serving app documents from ${derived} instead.`,
|
||||
);
|
||||
return derived;
|
||||
}
|
||||
return publicOrigin;
|
||||
};
|
||||
|
||||
server.on("error", (err: NodeJS.ErrnoException) => {
|
||||
if (err.code === "EADDRINUSE") {
|
||||
if (!retriedDynamic && port !== 0) {
|
||||
|
|
@ -380,9 +415,7 @@ export function createAppOriginController(
|
|||
const urlHost = isAllInterfacesHost(canonicalHost)
|
||||
? "localhost"
|
||||
: canonicalHost;
|
||||
// An operator-supplied public origin (#1862) replaces the derived one
|
||||
// only now that the listener it routes to is actually up.
|
||||
origin = publicOrigin ?? `http://${urlHost}:${actualPort}`;
|
||||
origin = adoptedOrigin(`http://${urlHost}:${actualPort}`);
|
||||
boundPort = actualPort;
|
||||
settle({ port: actualPort, url: origin });
|
||||
});
|
||||
|
|
|
|||
|
|
@ -92,9 +92,24 @@ function parsePublicUrl(raw: string): Parsed {
|
|||
return { url };
|
||||
}
|
||||
|
||||
/**
|
||||
* The value as it may appear in a log line. Never the raw string: the values
|
||||
* refused for carrying credentials or a query are exactly the ones likely to
|
||||
* hold a secret, and echoing them would leak what the check refused. Userinfo,
|
||||
* query and fragment are dropped; an unparseable value is not echoed at all.
|
||||
*/
|
||||
export function describeForLog(raw: string): string {
|
||||
try {
|
||||
const u = new URL(raw);
|
||||
return `"${u.protocol}${u.host ? `//${u.host}` : ""}${u.pathname}"`;
|
||||
} catch {
|
||||
return "(unparseable value, not echoed)";
|
||||
}
|
||||
}
|
||||
|
||||
function warnIgnored(env: string, raw: string, reason: string): void {
|
||||
console.warn(
|
||||
`Ignoring ${env}="${raw}": ${reason}. Advertising the bind-derived address instead.`,
|
||||
`Ignoring ${env}=${describeForLog(raw)}: ${reason}. Advertising the bind-derived address instead.`,
|
||||
);
|
||||
}
|
||||
|
||||
|
|
@ -157,8 +172,11 @@ export function resolveSandboxPublicUrl(
|
|||
* `undefined` when it is unset, blank, or refused (warned).
|
||||
*
|
||||
* @param allowedOrigins The resolved Inspector UI origins.
|
||||
* @param sandboxUrl The advertised sandbox URL when it is known at config time
|
||||
* (i.e. overridden). A bind-derived one is on its own port and cannot match.
|
||||
* @param sandboxUrl The overridden sandbox URL, when there is one. This is the
|
||||
* early, config-time check; a bind-derived sandbox origin is only known once
|
||||
* that listener binds (possibly on a fallback port), so the authoritative
|
||||
* comparison against it is made by the app-origin controller at start, which
|
||||
* receives the real sandbox and Inspector origins as `embedderOrigins`.
|
||||
*/
|
||||
export function resolveAppOriginPublicOrigin(
|
||||
raw: string | undefined,
|
||||
|
|
|
|||
|
|
@ -338,9 +338,20 @@ export function createSandboxController(
|
|||
const urlHost = isAllInterfacesHost(canonicalHost)
|
||||
? "localhost"
|
||||
: canonicalHost;
|
||||
const derivedUrl = `http://${urlHost}:${actualPort}/sandbox`;
|
||||
// An operator-supplied public URL (#1862) replaces the derived one
|
||||
// only now that the listener it routes to is actually up.
|
||||
sandboxUrl = publicUrl ?? `http://${urlHost}:${actualPort}/sandbox`;
|
||||
// only now that the listener it routes to is actually up — and only
|
||||
// on the port it was configured for. After the EADDRINUSE fallback
|
||||
// the proxy still routes the public URL to the PINNED port, which
|
||||
// someone else holds, so advertising it would send the browser to
|
||||
// the wrong listener. The derived URL at least works locally.
|
||||
if (publicUrl && retriedDynamic) {
|
||||
console.warn(
|
||||
`Sandbox: not advertising MCP_SANDBOX_FULL_ADDRESS, since it routes to port ${port}, ` +
|
||||
`which is taken; advertising ${derivedUrl} instead.`,
|
||||
);
|
||||
}
|
||||
sandboxUrl = publicUrl && !retriedDynamic ? publicUrl : derivedUrl;
|
||||
boundPort = actualPort;
|
||||
settle({ port: actualPort, url: sandboxUrl });
|
||||
});
|
||||
|
|
|
|||
|
|
@ -393,6 +393,54 @@ describe("createAppOriginController", () => {
|
|||
}
|
||||
});
|
||||
|
||||
it("does not use a public origin after falling back off its pinned port (#1862)", async () => {
|
||||
const { port, release } = await claimPort();
|
||||
const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {});
|
||||
try {
|
||||
controller = createAppOriginController({
|
||||
port,
|
||||
host: "127.0.0.1",
|
||||
publicOrigin: "https://apps.example.com",
|
||||
});
|
||||
const result = await controller.start();
|
||||
expect(result.port).not.toBe(port);
|
||||
expect(result.url).toBe(`http://127.0.0.1:${result.port}`);
|
||||
expect(
|
||||
controller.publish({ html: "<p>x</p>" })!.url.startsWith(result.url),
|
||||
).toBe(true);
|
||||
expect(warnSpy).toHaveBeenCalledWith(
|
||||
expect.stringContaining("not using MCP_APP_ORIGIN_FULL_ADDRESS"),
|
||||
);
|
||||
} finally {
|
||||
warnSpy.mockRestore();
|
||||
await release();
|
||||
}
|
||||
});
|
||||
|
||||
it("refuses a public origin equal to a trusted ancestor, including a bind-derived sandbox (#1862)", async () => {
|
||||
// Config time cannot see the bind-derived sandbox origin; the callers pass
|
||||
// it in embedderOrigins, so the controller is where the collision is caught.
|
||||
const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {});
|
||||
try {
|
||||
controller = createAppOriginController({
|
||||
port: 0,
|
||||
host: "127.0.0.1",
|
||||
publicOrigin: "http://127.0.0.1:6275",
|
||||
embedderOrigins: appDocumentEmbedders("http://127.0.0.1:6275/sandbox", [
|
||||
"http://localhost:6274",
|
||||
]),
|
||||
});
|
||||
const result = await controller.start();
|
||||
expect(result.url).toBe(`http://127.0.0.1:${result.port}`);
|
||||
expect(controller.getOrigin()).not.toBe("http://127.0.0.1:6275");
|
||||
expect(warnSpy).toHaveBeenCalledWith(
|
||||
expect.stringContaining("must differ from both"),
|
||||
);
|
||||
} finally {
|
||||
warnSpy.mockRestore();
|
||||
}
|
||||
});
|
||||
|
||||
it("degrades to publish()===null when listen fails for a non-EADDRINUSE reason", async () => {
|
||||
// The backends await start() during boot; if it ever stopped resolving the
|
||||
// whole web server would hang. Instead it resolves empty and every publish
|
||||
|
|
|
|||
|
|
@ -72,6 +72,17 @@ describe("public addresses (#1862)", () => {
|
|||
expect(warned()).toContain(reason);
|
||||
});
|
||||
|
||||
it.each([
|
||||
["https://user:hunter2@sb.example.com/sandbox", "hunter2"],
|
||||
["https://sb.example.com/sandbox?token=hunter2", "hunter2"],
|
||||
["https://sb.example.com/sandbox#hunter2", "hunter2"],
|
||||
["not a url hunter2", "hunter2"],
|
||||
])("never echoes a secret from a refused value (%s)", (raw, secret) => {
|
||||
expect(resolveSandboxPublicUrl(raw, UI)).toBeUndefined();
|
||||
expect(warned()).toContain("MCP_SANDBOX_FULL_ADDRESS");
|
||||
expect(warned()).not.toContain(secret);
|
||||
});
|
||||
|
||||
it("refuses an origin shared with the Inspector UI — the same-origin collapse", () => {
|
||||
// The natural reverse-proxy layout: one hostname, /sandbox routed to 6275.
|
||||
// The spec requires host != sandbox origin, so this must not be advertised.
|
||||
|
|
|
|||
|
|
@ -495,6 +495,31 @@ describe("createSandboxController", () => {
|
|||
}
|
||||
});
|
||||
|
||||
it("does not advertise a public URL after falling back off its pinned port (#1862)", async () => {
|
||||
// The reverse proxy routes the public URL to the pinned port, which another
|
||||
// process holds — so the public URL would reach the wrong listener.
|
||||
const { port, release } = await claimPort();
|
||||
const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {});
|
||||
const controller = createSandboxController({
|
||||
port,
|
||||
host: "127.0.0.1",
|
||||
publicUrl: "https://sb.example.com/sandbox",
|
||||
});
|
||||
try {
|
||||
const result = await controller.start();
|
||||
expect(result.port).not.toBe(port);
|
||||
expect(result.url).toBe(`http://127.0.0.1:${result.port}/sandbox`);
|
||||
expect(controller.getUrl()).toBe(result.url);
|
||||
expect(warnSpy).toHaveBeenCalledWith(
|
||||
expect.stringContaining("not advertising MCP_SANDBOX_FULL_ADDRESS"),
|
||||
);
|
||||
} finally {
|
||||
warnSpy.mockRestore();
|
||||
await controller.close();
|
||||
await release();
|
||||
}
|
||||
});
|
||||
|
||||
it("resolves with empty values when even the dynamic retry fails", async () => {
|
||||
// The Vite plugin awaits start() in configureServer; if start() ever stops
|
||||
// resolving, the entire dev backend hangs. This pins the resolve-on-error
|
||||
|
|
|
|||
Loading…
Reference in a new issue