diff --git a/server/src/__tests__/server-startup-feedback-export.test.ts b/server/src/__tests__/server-startup-feedback-export.test.ts index 63dd0b716c..b01ad17d02 100644 --- a/server/src/__tests__/server-startup-feedback-export.test.ts +++ b/server/src/__tests__/server-startup-feedback-export.test.ts @@ -693,7 +693,7 @@ describe("startServer PAPERCLIP_API_URL handling", () => { ); }); - it("rewrites explicit-port auth public URLs when detect-port selects a new port", async () => { + it("preserves explicit-port external auth public URLs when detect-port selects a new port", async () => { loadConfigMock.mockReturnValueOnce(buildTestConfig({ port: 3100, authBaseUrlMode: "explicit", @@ -703,9 +703,12 @@ describe("startServer PAPERCLIP_API_URL handling", () => { const started = await startServer(); + // The server listens internally on 3110, but an explicit *external* base URL must keep + // its advertised port. Rewriting it to the internal listen port produced an unreachable + // URL that leaked to spawned agents as a dead PAPERCLIP_API_URL. (BRO-1558) expect(started.listenPort).toBe(3110); - expect(started.apiUrl).toBe("http://my-host.ts.net:3110"); - expect(process.env.PAPERCLIP_RUNTIME_API_URL).toBe("http://my-host.ts.net:3110"); + expect(started.apiUrl).toBe("http://my-host.ts.net:3100"); + expect(process.env.PAPERCLIP_RUNTIME_API_URL).toBe("http://my-host.ts.net:3100"); }); it("keeps no-port auth public URLs stable when detect-port selects a new port", async () => { diff --git a/server/src/index.ts b/server/src/index.ts index b46b9d2324..c338564356 100644 --- a/server/src/index.ts +++ b/server/src/index.ts @@ -78,6 +78,7 @@ import { } from "./services/adapter-registry-bootstrap.js"; import { createFeedbackTraceShareClientFromConfig } from "./services/feedback-share-client.js"; import { buildRuntimeApiCandidateUrls, choosePrimaryRuntimeApiUrl } from "./runtime-api.js"; +import { isLoopbackHost, rewriteLoopbackUrlPort } from "./url-utils.js"; import { createPluginWorkerManager } from "./services/plugin-worker-manager.js"; import { createStorageServiceFromConfig } from "./storage/index.js"; import { printStartupBanner } from "./startup-banner.js"; @@ -235,11 +236,6 @@ export async function startServer(): Promise { return "applied (pending migrations)"; } - function isLoopbackHost(host: string): boolean { - const normalized = host.trim().toLowerCase(); - return normalized === "127.0.0.1" || normalized === "localhost" || normalized === "::1"; - } - function isPostgresConnectionString(connectionString: string): boolean { try { const parsed = new URL(connectionString); @@ -265,19 +261,6 @@ export async function startServer(): Promise { } } - function rewriteLocalUrlPort(rawUrl: string | undefined, port: number): string | undefined { - if (!rawUrl) return undefined; - try { - const parsed = new URL(rawUrl); - // The URL API normalizes default ports like :80/:443 to "", so treat them as stable URLs. - if (!parsed.port) return rawUrl; - parsed.port = String(port); - return parsed.toString(); - } catch { - return rawUrl; - } - } - const LOCAL_BOARD_USER_ID = "local-board"; const LOCAL_BOARD_USER_EMAIL = "local@paperclip.local"; const LOCAL_BOARD_USER_NAME = "Board"; @@ -558,7 +541,7 @@ export async function startServer(): Promise { const requestedListenPort = config.port; const listenPort = await detectPort(requestedListenPort); if (config.authBaseUrlMode === "explicit" && config.authPublicBaseUrl) { - config.authPublicBaseUrl = rewriteLocalUrlPort(config.authPublicBaseUrl, listenPort); + config.authPublicBaseUrl = rewriteLoopbackUrlPort(config.authPublicBaseUrl, listenPort); } let authReady = config.deploymentMode === "local_trusted"; diff --git a/server/src/url-utils.test.ts b/server/src/url-utils.test.ts new file mode 100644 index 0000000000..7f061d5373 --- /dev/null +++ b/server/src/url-utils.test.ts @@ -0,0 +1,65 @@ +import { describe, expect, it } from "vitest"; +import { isLoopbackHost, rewriteLoopbackUrlPort, rewriteUrlPort } from "./url-utils.js"; + +describe("rewriteUrlPort", () => { + it("rewrites the port for any explicit-port URL, including external hosts", () => { + expect(rewriteUrlPort("http://localhost:5678", 3101)).toBe("http://localhost:3101/"); + expect(rewriteUrlPort("http://127.0.0.1:9999", 3101)).toBe("http://127.0.0.1:3101/"); + // A worktree advertises its own port even on a non-loopback host. + expect(rewriteUrlPort("http://my-host.ts.net:3100", 3103)).toBe("http://my-host.ts.net:3103/"); + }); + + it("leaves URLs without an explicit port stable", () => { + expect(rewriteUrlPort("https://paperclip.example", 3101)).toBe("https://paperclip.example"); + expect(rewriteUrlPort("http://localhost", 3101)).toBe("http://localhost"); + }); + + it("passes through empty and unparseable inputs", () => { + expect(rewriteUrlPort(undefined, 3101)).toBeUndefined(); + expect(rewriteUrlPort("", 3101)).toBeUndefined(); + expect(rewriteUrlPort("not a url", 3101)).toBe("not a url"); + }); +}); + +describe("rewriteLoopbackUrlPort", () => { + it("rewrites the port for loopback base URLs", () => { + expect(rewriteLoopbackUrlPort("http://localhost:5678", 3101)).toBe("http://localhost:3101/"); + expect(rewriteLoopbackUrlPort("http://127.0.0.1:9999", 3101)).toBe("http://127.0.0.1:3101/"); + expect(rewriteLoopbackUrlPort("http://[::1]:9999", 3101)).toBe("http://[::1]:3101/"); + }); + + it("leaves explicit external base URLs untouched (BRO-1558)", () => { + // A Tailscale Serve listener on :8443 must survive; rewriting its port to the internal + // listen port produced an unreachable URL that leaked to agents as a dead PAPERCLIP_API_URL. + const serve = "https://erics-mac-studio-1.tailc54c7.ts.net:8443"; + expect(rewriteLoopbackUrlPort(serve, 3101)).toBe(serve); + }); + + it("leaves URLs without an explicit port stable", () => { + expect(rewriteLoopbackUrlPort("https://paperclip.example.com", 3101)).toBe( + "https://paperclip.example.com", + ); + expect(rewriteLoopbackUrlPort("http://localhost", 3101)).toBe("http://localhost"); + }); + + it("passes through empty and unparseable inputs", () => { + expect(rewriteLoopbackUrlPort(undefined, 3101)).toBeUndefined(); + expect(rewriteLoopbackUrlPort("", 3101)).toBeUndefined(); + expect(rewriteLoopbackUrlPort("not a url", 3101)).toBe("not a url"); + }); +}); + +describe("isLoopbackHost", () => { + it("matches loopback hosts including bracketed IPv6", () => { + expect(isLoopbackHost("localhost")).toBe(true); + expect(isLoopbackHost("127.0.0.1")).toBe(true); + expect(isLoopbackHost("::1")).toBe(true); + expect(isLoopbackHost("[::1]")).toBe(true); + expect(isLoopbackHost("LOCALHOST")).toBe(true); + }); + + it("rejects external hosts", () => { + expect(isLoopbackHost("erics-mac-studio-1.tailc54c7.ts.net")).toBe(false); + expect(isLoopbackHost("paperclip.example.com")).toBe(false); + }); +}); diff --git a/server/src/url-utils.ts b/server/src/url-utils.ts new file mode 100644 index 0000000000..30992ec9cc --- /dev/null +++ b/server/src/url-utils.ts @@ -0,0 +1,57 @@ +/** + * Shared base-URL helpers used by both server startup (index.ts) and worktree + * config materialization (worktree-config.ts). Keeping a single source of truth + * for host classification and port rewriting prevents the two paths from drifting. + */ + +export function isLoopbackHost(host: string): boolean { + // Strip surrounding brackets so a URL hostname form ("[::1]") matches too. + const normalized = host.trim().toLowerCase().replace(/^\[|\]$/g, ""); + return normalized === "127.0.0.1" || normalized === "localhost" || normalized === "::1"; +} + +/** + * Rewrite the port of any explicit-port base URL to `port`, leaving portless and + * unparseable URLs untouched. + * + * Used by the worktree path: a worktree is a distinct instance on its own server + * port, so its advertised base URL must follow that port even for a non-loopback + * host (worktrees are reachable at the same host on their own port). + */ +export function rewriteUrlPort(rawUrl: string | undefined, port: number): string | undefined { + if (!rawUrl) return undefined; + try { + const parsed = new URL(rawUrl); + // The URL API normalizes default ports like :80/:443 to "", so treat them as stable URLs. + if (!parsed.port) return rawUrl; + parsed.port = String(port); + return parsed.toString(); + } catch { + return rawUrl; + } +} + +/** + * Like {@link rewriteUrlPort}, but only for loopback hosts. + * + * Used at server startup for the main instance's `authPublicBaseUrl`. An explicit + * *external* base URL (e.g. a Tailscale Serve listener on a non-default port like + * :8443) must survive untouched: rewriting its port to the internal listen port + * yields an unreachable URL (scheme/port mismatch) that then propagates to spawned + * agents as a dead PAPERCLIP_API_URL. (BRO-1558) + */ +export function rewriteLoopbackUrlPort( + rawUrl: string | undefined, + port: number, +): string | undefined { + if (!rawUrl) return undefined; + try { + const parsed = new URL(rawUrl); + if (!parsed.port) return rawUrl; + if (!isLoopbackHost(parsed.hostname)) return rawUrl; + parsed.port = String(port); + return parsed.toString(); + } catch { + return rawUrl; + } +} diff --git a/server/src/worktree-config.ts b/server/src/worktree-config.ts index b05686204e..db4e9fffe7 100644 --- a/server/src/worktree-config.ts +++ b/server/src/worktree-config.ts @@ -9,6 +9,7 @@ import { } from "@paperclipai/shared"; import { updateEnvFileContents, writeEnvFileAtomicallyIfChanged } from "@paperclipai/shared/env-file"; import { resolvePaperclipConfigPath, resolvePaperclipEnvPath } from "./paths.js"; +import { rewriteUrlPort } from "./url-utils.js"; function nonEmpty(value: string | null | undefined): string | null { return typeof value === "string" && value.trim().length > 0 ? value.trim() : null; @@ -33,19 +34,6 @@ function sanitizeWorktreeInstanceId(rawValue: string): string { return normalized || "worktree"; } -function rewriteLocalUrlPort(rawUrl: string | undefined, port: number): string | undefined { - if (!rawUrl) return undefined; - try { - const parsed = new URL(rawUrl); - // The URL API normalizes default ports like :80/:443 to "", so treat them as stable URLs. - if (!parsed.port) return rawUrl; - parsed.port = String(port); - return parsed.toString(); - } catch { - return rawUrl; - } -} - function parseEnvFile(contents: string): Record { const entries: Record = {}; @@ -444,7 +432,7 @@ function buildIsolatedWorktreeConfig( if (config.auth.baseUrlMode === "explicit" && config.auth.publicBaseUrl) { nextConfig.auth = { ...config.auth, - publicBaseUrl: rewriteLocalUrlPort(config.auth.publicBaseUrl, serverPort), + publicBaseUrl: rewriteUrlPort(config.auth.publicBaseUrl, serverPort), }; } @@ -520,7 +508,7 @@ export function applyRuntimePortSelectionToConfig( } if (nextConfig.auth.baseUrlMode === "explicit" && nextConfig.auth.publicBaseUrl) { - const rewritten = rewriteLocalUrlPort(nextConfig.auth.publicBaseUrl, input.serverPort); + const rewritten = rewriteUrlPort(nextConfig.auth.publicBaseUrl, input.serverPort); if (rewritten && rewritten !== nextConfig.auth.publicBaseUrl) { nextConfig = { ...nextConfig,