From 166f381d3fa51ac96c60cc3b7315b3149bdc478f Mon Sep 17 00:00:00 2001 From: Eric Brookfield Date: Thu, 13 Aug 2026 12:47:13 -0400 Subject: [PATCH] fix(runtime): only rewrite base-URL port for loopback hosts (#10258) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - The server derives each spawned agent's `PAPERCLIP_API_URL` from `authPublicBaseUrl` via `choosePrimaryRuntimeApiUrl` → `buildPaperclipEnv` > - At startup, `rewriteLocalUrlPort` rewrote the port of the configured `auth.publicBaseUrl` to the internal listen port > - The rewrite was applied to *any* explicit-port URL, not just loopback ones — so an external base URL on a non-default port (e.g. a Tailscale Serve listener on `:8443`) got clobbered to the internal HTTP port `:3101` > - `https://host:3101` (HTTPS scheme against the plaintext HTTP port) is unreachable, and that dead value propagated to every spawned agent's `PAPERCLIP_API_URL` > - This pull request preserves explicit external base URLs at startup while keeping the worktree path's intended per-worktree port rewrite > - The benefit is that agents following the documented `curl "$PAPERCLIP_API_URL/..."` pattern no longer hit a dead endpoint ## Linked Issues or Issue Description No public GitHub issue; describing inline (bug report). **Summary:** at server startup, `rewriteLocalUrlPort` corrupts an explicit external `auth.publicBaseUrl`, leaking a dead `PAPERCLIP_API_URL` to spawned agents. **Steps to reproduce:** 1. Configure `auth.publicBaseUrl = https://:8443` (an external listener on a non-default port, e.g. Tailscale Serve). 2. Start the server (internal listen port `3101`). 3. Inspect a spawned agent run's env: `PAPERCLIP_API_URL=https://:3101`. **Expected:** the agent-facing URL points at a reachable origin. **Actual:** `curl "$PAPERCLIP_API_URL/..."` → `http_code=000` (HTTPS against the plaintext HTTP port; TLS handshake fails). The fleet stays healthy only because the runtime falls through its candidate list, but any agent following the documented curl pattern silently hits a dead endpoint first. Related open PRs in the same area (dedup — none merged; this is a smaller, targeted fix with regression tests): - Refs #9916 (PAPERCLIP_RUNTIME_API_URL precedence + authPublicBaseUrl port preservation) - Refs #7342 (preserve explicit authPublicBaseUrl during startup, GH#7341) - Refs #9228 (prefer reachable runtime API URLs for local adapters) ## What Changed - New `server/src/url-utils.ts` with two intent-revealing helpers (single source of truth): - `rewriteUrlPort` — rewrite any explicit-port URL to a new port. - `rewriteLoopbackUrlPort` — rewrite **only** loopback hosts; explicit external URLs survive untouched. - `isLoopbackHost` — bracket-tolerant so a URL hostname form `[::1]` matches. - `server/src/index.ts` (startup, the bug): `authPublicBaseUrl` now uses `rewriteLoopbackUrlPort`, so an external Serve URL keeps its port. Nested helper copies removed in favor of the shared module. - `server/src/worktree-config.ts` (worktree path): uses `rewriteUrlPort` — **behavior unchanged**; a worktree still advertises its own server port even on a non-loopback host (this is intended and asserted by the existing worktree suite). - `server/src/url-utils.test.ts`: regression coverage for both helpers. - Updated one stale assertion in `server-startup-feedback-export.test.ts` that had encoded the old (buggy) external-host rewrite at startup. ## Verification - `vitest run src/url-utils.test.ts src/__tests__/worktree-config.test.ts src/__tests__/server-startup-feedback-export.test.ts` → **33 passed**; the only local failure is a pre-existing, environment-coupled test (`derives trusted origins…`) that leaks the dev machine's real Tailscale identity into an origins list and passes in CI (it is unrelated to this change — its `authPublicBaseUrl` is loopback and rewrites identically before/after). - `npm run typecheck` (`tsc --noEmit`) → **clean, exit 0**. - PR CI: Build, Typecheck + Release Registry, serialized server suites, and `review` gate green. ## Risks Low risk. The only behavioral change is at startup: an explicit *external* base URL on a non-default port is no longer rewritten to the internal listen port (the bug). Loopback/worktree behavior is unchanged. No schema/migration changes. ## Model Used Claude Opus 4.8, 1M context (`claude-opus-4-8[1m]`), extended thinking, with tool use / code execution (Claude Code). ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes #` / `Refs #` OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip` URLs) - [ ] My branch name describes the change (e.g. `docs/...`, `fix/...`) and contains no internal Paperclip ticket id or instance-derived details - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [ ] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [ ] All Paperclip CI gates are green - [ ] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge --------- Co-authored-by: Claude Opus 4.8 (1M context) --- .../server-startup-feedback-export.test.ts | 9 ++- server/src/index.ts | 21 +----- server/src/url-utils.test.ts | 65 +++++++++++++++++++ server/src/url-utils.ts | 57 ++++++++++++++++ server/src/worktree-config.ts | 18 +---- 5 files changed, 133 insertions(+), 37 deletions(-) create mode 100644 server/src/url-utils.test.ts create mode 100644 server/src/url-utils.ts 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,