From b2496c80672f15481f7af48952b0d6d68ec208de Mon Sep 17 00:00:00 2001 From: Devin Foley Date: Sun, 26 Apr 2026 15:40:39 -0700 Subject: [PATCH] fix(auth): trust allowed hostname port variants on detected listen port (#4554) ## Thinking Path > - Paperclip is the control plane for autonomous AI companies, so authenticated board access has to be predictable across local and worktree deployments. > - This change sits in the authenticated-mode server startup and Better Auth origin-trust wiring. > - The original auth branch fixed one real gap by adding port-qualified trusted origins for allowed hostnames on non-default ports. > - Review of that branch found a second-order bug: trusted origins were still derived from the configured port before startup detected the actual listen port. > - In isolated worktrees, that meant a common `3100 -> 3101` port shift could still leave Better Auth trusting the stale origin. > - This pull request keeps the original allowed-hostname port-variant fix, then moves trust derivation onto the resolved listen port and adds regression coverage around startup wiring. > - The benefit is that authenticated sessions keep working on allowed private hostnames even when Paperclip has to auto-shift to a different local port. ## What Changed - Added `:port` trusted-origin variants for authenticated-mode `allowedHostnames` when Paperclip runs on non-default ports. - Changed authenticated startup so `listenPort` is detected before Better Auth initialization, and explicit auth base URLs are rewritten before auth startup. - Updated `deriveAuthTrustedOrigins()` to accept the resolved listen port so Better Auth trusts the actual browser origin instead of the stale configured port. - Added focused regression coverage in `server/src/__tests__/better-auth.test.ts` and `server/src/__tests__/server-startup-feedback-export.test.ts`. ## Verification - `pnpm exec vitest run server/src/__tests__/better-auth.test.ts server/src/__tests__/server-startup-feedback-export.test.ts` - Reviewer re-check: reviewed commits `380f5b9f` and `092bb34c` after the follow-up fix landed and found no remaining issues. ## Risks - Low risk: this only affects authenticated-mode origin derivation and startup ordering around detected listen ports. - Main behavioral shift: startup no longer mutates `config.port` to the selected port; it now carries `requestedListenPort` separately and uses `listenPort` where runtime behavior needs the resolved value. - If another path was implicitly relying on `config.port` being overwritten during startup, that path would need follow-up, though the current startup/test coverage did not reveal one. > I checked `ROADMAP.md` and did not find an overlapping planned core work item for this auth trusted-origin port handling fix. ## Model Used - OpenAI Codex via Paperclip `codex_local` agents for implementation and review. Exact backend model ID/context window were not surfaced in this run context; work was performed through the Codex local adapter with tool use, code execution, and review passes. ## 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 run tests locally and they pass - [x] I have added or updated tests where applicable - [x] If this change affects the UI, I have included before/after screenshots - [x] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [x] I will address all Greptile and reviewer comments before requesting merge --- server/src/__tests__/better-auth.test.ts | 35 +++++ .../server-startup-feedback-export.test.ts | 127 +++++++++++++----- server/src/auth/better-auth.ts | 14 +- server/src/index.ts | 23 ++-- 4 files changed, 148 insertions(+), 51 deletions(-) diff --git a/server/src/__tests__/better-auth.test.ts b/server/src/__tests__/better-auth.test.ts index 79bfa3e7..dd672d67 100644 --- a/server/src/__tests__/better-auth.test.ts +++ b/server/src/__tests__/better-auth.test.ts @@ -4,6 +4,7 @@ import { getCookies } from "better-auth/cookies"; import { buildBetterAuthAdvancedOptions, deriveAuthCookiePrefix, + deriveAuthTrustedOrigins, } from "../auth/better-auth.js"; const ORIGINAL_INSTANCE_ID = process.env.PAPERCLIP_INSTANCE_ID; @@ -40,4 +41,38 @@ describe("Better Auth cookie scoping", () => { useSecureCookies: false, }); }); + + it("adds hostname port variants for authenticated mode on non-default ports", () => { + const trustedOrigins = deriveAuthTrustedOrigins({ + deploymentMode: "authenticated", + authBaseUrlMode: "auto", + authPublicBaseUrl: undefined, + allowedHostnames: ["Board.Example.Test"], + port: 3101, + } as Parameters[0]); + + expect(trustedOrigins).toEqual(expect.arrayContaining([ + "https://board.example.test", + "http://board.example.test", + "https://board.example.test:3101", + "http://board.example.test:3101", + ])); + }); + + it("prefers an explicit resolved listen port over the configured port", () => { + const trustedOrigins = deriveAuthTrustedOrigins({ + deploymentMode: "authenticated", + authBaseUrlMode: "auto", + authPublicBaseUrl: undefined, + allowedHostnames: ["board.example.test"], + port: 3100, + } as Parameters[0], { listenPort: 3101 }); + + expect(trustedOrigins).toEqual(expect.arrayContaining([ + "https://board.example.test:3101", + "http://board.example.test:3101", + ])); + expect(trustedOrigins).not.toContain("https://board.example.test:3100"); + expect(trustedOrigins).not.toContain("http://board.example.test:3100"); + }); }); diff --git a/server/src/__tests__/server-startup-feedback-export.test.ts b/server/src/__tests__/server-startup-feedback-export.test.ts index a08e0f02..35fb0767 100644 --- a/server/src/__tests__/server-startup-feedback-export.test.ts +++ b/server/src/__tests__/server-startup-feedback-export.test.ts @@ -2,17 +2,50 @@ import { beforeEach, describe, expect, it, vi } from "vitest"; const { createAppMock, + createBetterAuthInstanceMock, createDbMock, detectPortMock, - loadConfigMock, + deriveAuthTrustedOriginsMock, feedbackExportServiceMock, feedbackServiceFactoryMock, fakeServer, + loadConfigMock, } = vi.hoisted(() => { const createAppMock = vi.fn(async () => ((_: unknown, __: unknown) => {}) as never); + const createBetterAuthInstanceMock = vi.fn(() => ({})); const createDbMock = vi.fn(() => ({}) as never); const detectPortMock = vi.fn(async (port: number) => port); - const loadConfigMock = vi.fn(() => ({ + const deriveAuthTrustedOriginsMock = vi.fn(() => []); + 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(), + }; + const loadConfigMock = vi.fn(); + + return { + createAppMock, + createBetterAuthInstanceMock, + createDbMock, + detectPortMock, + deriveAuthTrustedOriginsMock, + feedbackExportServiceMock, + feedbackServiceFactoryMock, + fakeServer, + loadConfigMock, + }; +}); + +function buildTestConfig(overrides: Record = {}) { + return { deploymentMode: "authenticated", deploymentExposure: "private", bind: "loopback", @@ -48,31 +81,9 @@ const { heartbeatSchedulerEnabled: false, heartbeatSchedulerIntervalMs: 30000, companyDeletionEnabled: false, - })); - const feedbackExportServiceMock = { - flushPendingFeedbackTraces: vi.fn(async () => ({ attempted: 0, sent: 0, failed: 0 })), + ...overrides, }; - 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), @@ -169,8 +180,8 @@ vi.mock("../board-claim.js", () => ({ vi.mock("../auth/better-auth.js", () => ({ createBetterAuthHandler: vi.fn(() => undefined), - createBetterAuthInstance: vi.fn(() => ({})), - deriveAuthTrustedOrigins: vi.fn(() => []), + createBetterAuthInstance: createBetterAuthInstanceMock, + deriveAuthTrustedOrigins: deriveAuthTrustedOriginsMock, resolveBetterAuthSession: vi.fn(async () => null), resolveBetterAuthSessionFromHeaders: vi.fn(async () => null), })); @@ -180,6 +191,9 @@ import { startServer } from "../index.ts"; describe("startServer feedback export wiring", () => { beforeEach(() => { vi.clearAllMocks(); + loadConfigMock.mockReturnValue(buildTestConfig()); + createBetterAuthInstanceMock.mockReturnValue({}); + deriveAuthTrustedOriginsMock.mockReturnValue([]); process.env.BETTER_AUTH_SECRET = "test-secret"; }); @@ -197,9 +211,56 @@ describe("startServer feedback export wiring", () => { }); }); +describe("startServer authenticated auth origin setup", () => { + beforeEach(() => { + vi.clearAllMocks(); + loadConfigMock.mockReturnValue(buildTestConfig()); + createBetterAuthInstanceMock.mockReturnValue({}); + deriveAuthTrustedOriginsMock.mockReturnValue([]); + process.env.BETTER_AUTH_SECRET = "test-secret"; + }); + + it("derives trusted origins from the detected listen port before auth initializes", async () => { + loadConfigMock.mockReturnValue(buildTestConfig({ + port: 3210, + allowedHostnames: ["board.example.test"], + authBaseUrlMode: "explicit", + authPublicBaseUrl: "http://127.0.0.1:3210", + })); + detectPortMock.mockResolvedValueOnce(3211); + deriveAuthTrustedOriginsMock.mockImplementation( + (_config: { port: number; authPublicBaseUrl?: string }, opts?: { listenPort?: number }) => [ + `http://board.example.test:${opts?.listenPort ?? 0}`, + ], + ); + + await startServer(); + + expect(deriveAuthTrustedOriginsMock).toHaveBeenCalledWith( + expect.objectContaining({ + port: 3210, + authPublicBaseUrl: "http://127.0.0.1:3211/", + }), + { listenPort: 3211 }, + ); + expect(createBetterAuthInstanceMock).toHaveBeenCalledWith( + expect.anything(), + expect.objectContaining({ + port: 3210, + authPublicBaseUrl: "http://127.0.0.1:3211/", + }), + ["http://board.example.test:3211"], + ); + expect(createAppMock.mock.calls[0]?.[1]).toMatchObject({ + serverPort: 3211, + }); + }); +}); + describe("startServer PAPERCLIP_API_URL handling", () => { beforeEach(() => { vi.clearAllMocks(); + loadConfigMock.mockReturnValue(buildTestConfig()); process.env.BETTER_AUTH_SECRET = "test-secret"; delete process.env.PAPERCLIP_API_URL; }); @@ -221,12 +282,11 @@ describe("startServer PAPERCLIP_API_URL handling", () => { }); it("rewrites explicit-port auth public URLs when detect-port selects a new port", async () => { - loadConfigMock.mockReturnValueOnce({ - ...loadConfigMock(), + loadConfigMock.mockReturnValueOnce(buildTestConfig({ port: 3100, authBaseUrlMode: "explicit", authPublicBaseUrl: "http://my-host.ts.net:3100", - }); + })); detectPortMock.mockResolvedValueOnce(3110); const started = await startServer(); @@ -237,12 +297,11 @@ describe("startServer PAPERCLIP_API_URL handling", () => { }); it("keeps no-port auth public URLs stable when detect-port selects a new port", async () => { - loadConfigMock.mockReturnValueOnce({ - ...loadConfigMock(), + loadConfigMock.mockReturnValueOnce(buildTestConfig({ port: 3100, authBaseUrlMode: "explicit", authPublicBaseUrl: "https://paperclip.example", - }); + })); detectPortMock.mockResolvedValueOnce(3110); const started = await startServer(); diff --git a/server/src/auth/better-auth.ts b/server/src/auth/better-auth.ts index 402dad52..3c84b9f5 100644 --- a/server/src/auth/better-auth.ts +++ b/server/src/auth/better-auth.ts @@ -61,7 +61,7 @@ function headersFromExpressRequest(req: Request): Headers { return headersFromNodeHeaders(req.headers); } -export function deriveAuthTrustedOrigins(config: Config): string[] { +export function deriveAuthTrustedOrigins(config: Config, opts?: { listenPort?: number }): string[] { const baseUrl = config.authBaseUrlMode === "explicit" ? config.authPublicBaseUrl : undefined; const trustedOrigins = new Set(); @@ -73,18 +73,24 @@ export function deriveAuthTrustedOrigins(config: Config): string[] { } } if (config.deploymentMode === "authenticated") { + const port = opts?.listenPort ?? config.port; + const needsPortVariants = port !== 80 && port !== 443; for (const hostname of config.allowedHostnames) { const trimmed = hostname.trim().toLowerCase(); if (!trimmed) continue; trustedOrigins.add(`https://${trimmed}`); trustedOrigins.add(`http://${trimmed}`); + if (needsPortVariants) { + trustedOrigins.add(`https://${trimmed}:${port}`); + trustedOrigins.add(`http://${trimmed}:${port}`); + } } } return Array.from(trustedOrigins); } -export function createBetterAuthInstance(db: Db, config: Config, trustedOrigins?: string[]): BetterAuthInstance { +export function createBetterAuthInstance(db: Db, config: Config, trustedOrigins: string[]): BetterAuthInstance { const baseUrl = config.authBaseUrlMode === "explicit" ? config.authPublicBaseUrl : undefined; const secret = process.env.BETTER_AUTH_SECRET ?? process.env.PAPERCLIP_AGENT_JWT_SECRET; if (!secret) { @@ -93,15 +99,13 @@ export function createBetterAuthInstance(db: Db, config: Config, trustedOrigins? "For local development, set BETTER_AUTH_SECRET=paperclip-dev-secret in your .env file.", ); } - const effectiveTrustedOrigins = trustedOrigins ?? deriveAuthTrustedOrigins(config); - const publicUrl = process.env.PAPERCLIP_PUBLIC_URL ?? baseUrl; const isHttpOnly = publicUrl ? publicUrl.startsWith("http://") : false; const authConfig = { baseURL: baseUrl, secret, - trustedOrigins: effectiveTrustedOrigins, + trustedOrigins, database: drizzleAdapter(db, { provider: "pg", schema: { diff --git a/server/src/index.ts b/server/src/index.ts index 4dbf346f..35e2ca5f 100644 --- a/server/src/index.ts +++ b/server/src/index.ts @@ -468,6 +468,12 @@ 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); + } let authReady = config.deploymentMode === "local_trusted"; let betterAuthHandler: RequestHandler | undefined; @@ -488,7 +494,7 @@ export async function startServer(): Promise { resolveBetterAuthSession, resolveBetterAuthSessionFromHeaders, } = await import("./auth/better-auth.js"); - const derivedTrustedOrigins = deriveAuthTrustedOrigins(config); + const derivedTrustedOrigins = deriveAuthTrustedOrigins(config, { listenPort }); const envTrustedOrigins = (process.env.BETTER_AUTH_TRUSTED_ORIGINS ?? "") .split(",") .map((value) => value.trim()) @@ -513,17 +519,10 @@ export async function startServer(): Promise { await initializeBoardClaimChallenge(db as any, { deploymentMode: config.deploymentMode }); authReady = true; } - - const listenPort = await detectPort(config.port); - if (listenPort !== config.port) { - config.port = listenPort; - } + if (resolvedEmbeddedPostgresPort !== null && resolvedEmbeddedPostgresPort !== config.embeddedPostgresPort) { config.embeddedPostgresPort = resolvedEmbeddedPostgresPort; } - if (config.authBaseUrlMode === "explicit" && config.authPublicBaseUrl) { - config.authPublicBaseUrl = rewriteLocalUrlPort(config.authPublicBaseUrl, listenPort); - } maybePersistWorktreeRuntimePorts({ serverPort: listenPort, databasePort: resolvedEmbeddedPostgresPort, @@ -627,8 +626,8 @@ export async function startServer(): Promise { server.keepAliveTimeout = 185000; server.headersTimeout = 186000; - if (listenPort !== config.port) { - logger.warn(`Requested port is busy; using next free port (requestedPort=${config.port}, selectedPort=${listenPort})`); + if (listenPort !== requestedListenPort) { + logger.warn(`Requested port is busy; using next free port (requestedPort=${requestedListenPort}, selectedPort=${listenPort})`); } const runtimeListenHost = config.host; @@ -821,7 +820,7 @@ export async function startServer(): Promise { deploymentMode: config.deploymentMode, deploymentExposure: config.deploymentExposure, authReady, - requestedPort: config.port, + requestedPort: requestedListenPort, listenPort, uiMode, db: startupDbInfo,