fix(runtime): only rewrite base-URL port for loopback hosts (#10258)
## 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://<host>: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://<host>: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) <noreply@anthropic.com>
This commit is contained in:
parent
0a1f9fda65
commit
166f381d3f
|
|
@ -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 () => {
|
||||
|
|
|
|||
|
|
@ -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<StartedServer> {
|
|||
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<StartedServer> {
|
|||
}
|
||||
}
|
||||
|
||||
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<StartedServer> {
|
|||
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";
|
||||
|
|
|
|||
|
|
@ -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);
|
||||
});
|
||||
});
|
||||
|
|
@ -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;
|
||||
}
|
||||
}
|
||||
|
|
@ -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<string, string> {
|
||||
const entries: Record<string, string> = {};
|
||||
|
||||
|
|
@ -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,
|
||||
|
|
|
|||
Loading…
Reference in New Issue