diff --git a/cli/src/__tests__/worktree.test.ts b/cli/src/__tests__/worktree.test.ts index 28e10376..afb83f73 100644 --- a/cli/src/__tests__/worktree.test.ts +++ b/cli/src/__tests__/worktree.test.ts @@ -190,8 +190,9 @@ describe("worktree helpers", () => { ).toEqual(["worktree", "add", "-b", "my-worktree", "/tmp/my-worktree", "origin/main"]); }); - it("rewrites loopback auth URLs to the new port only", () => { + it("rewrites auth URLs only when they already include a port", () => { expect(rewriteLocalUrlPort("http://127.0.0.1:3100", 3110)).toBe("http://127.0.0.1:3110/"); + expect(rewriteLocalUrlPort("http://my-host.ts.net:3100", 3110)).toBe("http://my-host.ts.net:3110/"); expect(rewriteLocalUrlPort("https://paperclip.example", 3110)).toBe("https://paperclip.example"); }); diff --git a/cli/src/commands/worktree-lib.ts b/cli/src/commands/worktree-lib.ts index 5d4a0721..2be4528e 100644 --- a/cli/src/commands/worktree-lib.ts +++ b/cli/src/commands/worktree-lib.ts @@ -75,11 +75,6 @@ function nonEmpty(value: string | null | undefined): string | null { return typeof value === "string" && value.trim().length > 0 ? value.trim() : null; } -function isLoopbackHost(hostname: string): boolean { - const value = hostname.trim().toLowerCase(); - return value === "127.0.0.1" || value === "localhost" || value === "::1"; -} - export function sanitizeWorktreeInstanceId(rawValue: string): string { const trimmed = rawValue.trim().toLowerCase(); const normalized = trimmed @@ -168,7 +163,8 @@ export function rewriteLocalUrlPort(rawUrl: string | undefined, port: number): s if (!rawUrl) return undefined; try { const parsed = new URL(rawUrl); - if (!isLoopbackHost(parsed.hostname)) return 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 { diff --git a/server/src/__tests__/server-startup-feedback-export.test.ts b/server/src/__tests__/server-startup-feedback-export.test.ts index 082c99a2..a08e0f02 100644 --- a/server/src/__tests__/server-startup-feedback-export.test.ts +++ b/server/src/__tests__/server-startup-feedback-export.test.ts @@ -4,6 +4,7 @@ const { createAppMock, createDbMock, detectPortMock, + loadConfigMock, feedbackExportServiceMock, feedbackServiceFactoryMock, fakeServer, @@ -11,59 +12,7 @@ const { const createAppMock = vi.fn(async () => ((_: unknown, __: unknown) => {}) as never); const createDbMock = vi.fn(() => ({}) as never); const detectPortMock = vi.fn(async (port: number) => port); - const feedbackExportServiceMock = { - flushPendingFeedbackTraces: vi.fn(async () => ({ attempted: 0, sent: 0, failed: 0 })), - }; - const feedbackServiceFactoryMock = vi.fn(() => feedbackExportServiceMock); - const fakeServer = { - once: vi.fn().mockReturnThis(), - off: vi.fn().mockReturnThis(), - listen: vi.fn((_port: number, _host: string, callback?: () => void) => { - callback?.(); - return fakeServer; - }), - close: vi.fn(), - }; - - return { - createAppMock, - createDbMock, - detectPortMock, - feedbackExportServiceMock, - feedbackServiceFactoryMock, - fakeServer, - }; -}); - -vi.mock("node:http", () => ({ - createServer: vi.fn(() => fakeServer), -})); - -vi.mock("detect-port", () => ({ - default: detectPortMock, -})); - -vi.mock("@paperclipai/db", () => ({ - createDb: createDbMock, - ensurePostgresDatabase: vi.fn(), - getPostgresDataDirectory: vi.fn(), - inspectMigrations: vi.fn(async () => ({ status: "upToDate" })), - applyPendingMigrations: vi.fn(), - reconcilePendingMigrationHistory: vi.fn(async () => ({ repairedMigrations: [] })), - formatDatabaseBackupResult: vi.fn(() => "ok"), - runDatabaseBackup: vi.fn(), - authUsers: {}, - companies: {}, - companyMemberships: {}, - instanceUserRoles: {}, -})); - -vi.mock("../app.js", () => ({ - createApp: createAppMock, -})); - -vi.mock("../config.js", () => ({ - loadConfig: vi.fn(() => ({ + const loadConfigMock = vi.fn(() => ({ deploymentMode: "authenticated", deploymentExposure: "private", bind: "loopback", @@ -99,7 +48,61 @@ vi.mock("../config.js", () => ({ heartbeatSchedulerEnabled: false, heartbeatSchedulerIntervalMs: 30000, companyDeletionEnabled: false, - })), + })); + const feedbackExportServiceMock = { + flushPendingFeedbackTraces: vi.fn(async () => ({ attempted: 0, sent: 0, failed: 0 })), + }; + const feedbackServiceFactoryMock = vi.fn(() => feedbackExportServiceMock); + const fakeServer = { + once: vi.fn().mockReturnThis(), + off: vi.fn().mockReturnThis(), + listen: vi.fn((_port: number, _host: string, callback?: () => void) => { + callback?.(); + return fakeServer; + }), + close: vi.fn(), + }; + + return { + createAppMock, + createDbMock, + detectPortMock, + loadConfigMock, + feedbackExportServiceMock, + feedbackServiceFactoryMock, + fakeServer, + }; +}); + +vi.mock("node:http", () => ({ + createServer: vi.fn(() => fakeServer), +})); + +vi.mock("detect-port", () => ({ + default: detectPortMock, +})); + +vi.mock("@paperclipai/db", () => ({ + createDb: createDbMock, + ensurePostgresDatabase: vi.fn(), + getPostgresDataDirectory: vi.fn(), + inspectMigrations: vi.fn(async () => ({ status: "upToDate" })), + applyPendingMigrations: vi.fn(), + reconcilePendingMigrationHistory: vi.fn(async () => ({ repairedMigrations: [] })), + formatDatabaseBackupResult: vi.fn(() => "ok"), + runDatabaseBackup: vi.fn(), + authUsers: {}, + companies: {}, + companyMemberships: {}, + instanceUserRoles: {}, +})); + +vi.mock("../app.js", () => ({ + createApp: createAppMock, +})); + +vi.mock("../config.js", () => ({ + loadConfig: loadConfigMock, })); vi.mock("../middleware/logger.js", () => ({ @@ -216,4 +219,36 @@ describe("startServer PAPERCLIP_API_URL handling", () => { expect(started.apiUrl).toBe("http://127.0.0.1:3210"); expect(process.env.PAPERCLIP_API_URL).toBe("http://127.0.0.1:3210"); }); + + it("rewrites explicit-port auth public URLs when detect-port selects a new port", async () => { + loadConfigMock.mockReturnValueOnce({ + ...loadConfigMock(), + port: 3100, + authBaseUrlMode: "explicit", + authPublicBaseUrl: "http://my-host.ts.net:3100", + }); + detectPortMock.mockResolvedValueOnce(3110); + + const started = await startServer(); + + 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"); + }); + + it("keeps no-port auth public URLs stable when detect-port selects a new port", async () => { + loadConfigMock.mockReturnValueOnce({ + ...loadConfigMock(), + port: 3100, + authBaseUrlMode: "explicit", + authPublicBaseUrl: "https://paperclip.example", + }); + detectPortMock.mockResolvedValueOnce(3110); + + const started = await startServer(); + + expect(started.listenPort).toBe(3110); + expect(started.apiUrl).toBe("https://paperclip.example"); + expect(process.env.PAPERCLIP_RUNTIME_API_URL).toBe("https://paperclip.example"); + }); }); diff --git a/server/src/__tests__/worktree-config.test.ts b/server/src/__tests__/worktree-config.test.ts index defc8a05..69a9c5ee 100644 --- a/server/src/__tests__/worktree-config.test.ts +++ b/server/src/__tests__/worktree-config.test.ts @@ -24,7 +24,7 @@ afterEach(() => { } }); -function buildLegacyConfig(sharedRoot: string) { +function buildLegacyConfig(sharedRoot: string, publicBaseUrl = "http://127.0.0.1:3100") { return { $meta: { version: 1, @@ -56,7 +56,7 @@ function buildLegacyConfig(sharedRoot: string) { }, auth: { baseUrlMode: "explicit" as const, - publicBaseUrl: "http://127.0.0.1:3100", + publicBaseUrl, disableSignUp: false, }, storage: { @@ -439,7 +439,7 @@ describe("worktree config repair", () => { expect(repairedConfig.database.embeddedPostgresPort).toBe(54331); }); - it("persists runtime-selected worktree ports back into config", async () => { + it("persists runtime-selected worktree ports back into explicit-port auth URLs", async () => { const tempRoot = await fs.mkdtemp(path.join(os.tmpdir(), "paperclip-worktree-ports-")); const worktreeRoot = path.join(tempRoot, "PAP-878-create-a-mine-tab-in-inbox"); const paperclipDir = path.join(worktreeRoot, ".paperclip"); @@ -452,7 +452,7 @@ describe("worktree config repair", () => { configPath, JSON.stringify( { - ...buildLegacyConfig(instanceRoot), + ...buildLegacyConfig(instanceRoot, "http://my-host.ts.net:3100"), database: { mode: "embedded-postgres", embeddedPostgresDataDir: path.join(instanceRoot, "db"), @@ -518,20 +518,122 @@ describe("worktree config repair", () => { expect(writtenConfig.server.port).toBe(3103); expect(writtenConfig.database.embeddedPostgresPort).toBe(54335); - expect(writtenConfig.auth.publicBaseUrl).toBe("http://127.0.0.1:3103/"); + expect(writtenConfig.auth.publicBaseUrl).toBe("http://my-host.ts.net:3103/"); }); - it("can update the in-memory config without rewriting env-driven ports", () => { - const { config, changed } = applyRuntimePortSelectionToConfig(buildLegacyConfig("/tmp/shared"), { - serverPort: 3104, - databasePort: 54340, - allowServerPortWrite: false, - allowDatabasePortWrite: true, + it("does not rewrite no-port public auth URLs when persisting runtime-selected ports", async () => { + const tempRoot = await fs.mkdtemp(path.join(os.tmpdir(), "paperclip-worktree-public-ports-")); + const worktreeRoot = path.join(tempRoot, "PAP-125-public-base-url"); + const paperclipDir = path.join(worktreeRoot, ".paperclip"); + const configPath = path.join(paperclipDir, "config.json"); + const isolatedHome = path.join(tempRoot, ".paperclip-worktrees"); + const instanceRoot = path.join(isolatedHome, "instances", "pap-125-public-base-url"); + + await fs.mkdir(paperclipDir, { recursive: true }); + await fs.writeFile( + configPath, + JSON.stringify( + { + ...buildLegacyConfig(instanceRoot, "https://paperclip.example"), + database: { + mode: "embedded-postgres", + embeddedPostgresDataDir: path.join(instanceRoot, "db"), + embeddedPostgresPort: 54331, + backup: { + enabled: true, + intervalMinutes: 60, + retentionDays: 30, + dir: path.join(instanceRoot, "data", "backups"), + }, + }, + logging: { + mode: "file", + logDir: path.join(instanceRoot, "logs"), + }, + server: { + deploymentMode: "local_trusted", + exposure: "private", + host: "127.0.0.1", + port: 3101, + allowedHostnames: [], + serveUi: true, + }, + storage: { + provider: "local_disk", + localDisk: { + baseDir: path.join(instanceRoot, "data", "storage"), + }, + s3: { + bucket: "paperclip", + region: "us-east-1", + prefix: "", + forcePathStyle: false, + }, + }, + secrets: { + provider: "local_encrypted", + strictMode: false, + localEncrypted: { + keyFilePath: path.join(instanceRoot, "secrets", "master.key"), + }, + }, + }, + null, + 2, + ) + "\n", + "utf8", + ); + + process.chdir(worktreeRoot); + process.env.PAPERCLIP_IN_WORKTREE = "true"; + process.env.PAPERCLIP_WORKTREE_NAME = "PAP-125-public-base-url"; + process.env.PAPERCLIP_HOME = isolatedHome; + process.env.PAPERCLIP_INSTANCE_ID = "pap-125-public-base-url"; + process.env.PAPERCLIP_CONFIG = configPath; + + maybePersistWorktreeRuntimePorts({ + serverPort: 3103, + databasePort: 54335, }); + const writtenConfig = JSON.parse(await fs.readFile(configPath, "utf8")); + + expect(writtenConfig.server.port).toBe(3103); + expect(writtenConfig.database.embeddedPostgresPort).toBe(54335); + expect(writtenConfig.auth.publicBaseUrl).toBe("https://paperclip.example"); + }); + + it("can update the in-memory config when auth URL already includes a port", () => { + const { config, changed } = applyRuntimePortSelectionToConfig( + buildLegacyConfig("/tmp/shared", "http://my-host.ts.net:3100"), + { + serverPort: 3104, + databasePort: 54340, + allowServerPortWrite: false, + allowDatabasePortWrite: true, + }, + ); + expect(changed).toBe(true); expect(config.server.port).toBe(3100); expect(config.database.embeddedPostgresPort).toBe(54340); - expect(config.auth.publicBaseUrl).toBe("http://127.0.0.1:3104/"); + expect(config.auth.publicBaseUrl).toBe("http://my-host.ts.net:3104/"); + }); + + it("does not rewrite the in-memory config when auth URL has no explicit port", () => { + const { config, changed } = applyRuntimePortSelectionToConfig( + buildLegacyConfig("/tmp/shared", "https://paperclip.example"), + { + serverPort: 3104, + databasePort: 54340, + allowServerPortWrite: false, + allowDatabasePortWrite: true, + }, + ); + + expect(changed).toBe(true); + expect(config.server.port).toBe(3100); + expect(config.database.embeddedPostgresPort).toBe(54340); + expect(config.auth.publicBaseUrl).toBe("https://paperclip.example"); }); }); diff --git a/server/src/index.ts b/server/src/index.ts index 67eec273..4dbf346f 100644 --- a/server/src/index.ts +++ b/server/src/index.ts @@ -191,7 +191,8 @@ export async function startServer(): Promise { if (!rawUrl) return undefined; try { const parsed = new URL(rawUrl); - if (!isLoopbackHost(parsed.hostname)) return 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 { diff --git a/server/src/worktree-config.ts b/server/src/worktree-config.ts index 2c6170bc..d4f3ccb1 100644 --- a/server/src/worktree-config.ts +++ b/server/src/worktree-config.ts @@ -27,16 +27,12 @@ function sanitizeWorktreeInstanceId(rawValue: string): string { return normalized || "worktree"; } -function isLoopbackHost(hostname: string): boolean { - const value = hostname.trim().toLowerCase(); - return value === "127.0.0.1" || value === "localhost" || value === "::1"; -} - function rewriteLocalUrlPort(rawUrl: string | undefined, port: number): string | undefined { if (!rawUrl) return undefined; try { const parsed = new URL(rawUrl); - if (!isLoopbackHost(parsed.hostname)) return 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 {