mirror of
https://github.com/alkimake/paperclip.git
synced 2026-06-14 01:50:39 +09:00
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
This commit is contained in:
Devin Foley
GitHub
parent
08af830430
commit
b2496c8067
4 changed files with 148 additions and 51 deletions
|
|
@ -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<typeof deriveAuthTrustedOrigins>[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<typeof deriveAuthTrustedOrigins>[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");
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -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<string, unknown> = {}) {
|
||||
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();
|
||||
|
|
|
|||
|
|
@ -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<string>();
|
||||
|
||||
|
|
@ -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: {
|
||||
|
|
|
|||
|
|
@ -468,6 +468,12 @@ 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);
|
||||
}
|
||||
|
||||
let authReady = config.deploymentMode === "local_trusted";
|
||||
let betterAuthHandler: RequestHandler | undefined;
|
||||
|
|
@ -488,7 +494,7 @@ export async function startServer(): Promise<StartedServer> {
|
|||
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<StartedServer> {
|
|||
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<StartedServer> {
|
|||
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<StartedServer> {
|
|||
deploymentMode: config.deploymentMode,
|
||||
deploymentExposure: config.deploymentExposure,
|
||||
authReady,
|
||||
requestedPort: config.port,
|
||||
requestedPort: requestedListenPort,
|
||||
listenPort,
|
||||
uiMode,
|
||||
db: startupDbInfo,
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue