From 4eaa5c6675eea5ced7a7b570c0635f6af27baef5 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 26 Dec 2025 18:43:07 +0000 Subject: [PATCH 1/4] fix: replace Bun.serve with Node.js http module in auth-server The auth command was failing with "Bun is not defined" because auth-server.ts used Bun.serve() which is a Bun-specific API that doesn't exist when the code is compiled to run on Node.js. Replaced with Node.js native http.createServer() which works in both Bun and Node.js environments. --- cli/lib/auth-server.ts | 140 ++++++++++++++++++++++------------------- 1 file changed, 75 insertions(+), 65 deletions(-) diff --git a/cli/lib/auth-server.ts b/cli/lib/auth-server.ts index 9ea2e52..e97fd25 100644 --- a/cli/lib/auth-server.ts +++ b/cli/lib/auth-server.ts @@ -1,3 +1,5 @@ +import * as http from "http"; + /** * OAuth callback result */ @@ -31,7 +33,7 @@ function escapeHtml(str: string | null): string { } /** - * Create and start callback server using Bun.serve + * Create and start callback server using Node.js http module * * @param port - Port to listen on (0 for random available port) * @param expectedState - Expected state parameter for CSRF protection @@ -53,53 +55,60 @@ export function createCallbackServer( let actualPort = port; let resolveCallback: (value: CallbackResult) => void; let rejectCallback: (reason: Error) => void; - let serverInstance: ReturnType | null = null; + let serverInstance: http.Server | null = null; const promise = new Promise((resolve, reject) => { resolveCallback = resolve; rejectCallback = reject; }); + const stopServer = () => { + if (serverInstance) { + serverInstance.close(); + serverInstance = null; + } + }; + // Timeout handler const timeout = setTimeout(() => { if (!resolved) { resolved = true; - if (serverInstance) { - serverInstance.stop(); - } + stopServer(); rejectCallback(new Error("Authentication timeout. Please try again.")); } }, timeoutMs); - serverInstance = Bun.serve({ - port: port, - hostname: "127.0.0.1", - fetch(req) { - if (resolved) { - return new Response("Already handled", { status: 200 }); - } + serverInstance = http.createServer((req, res) => { + if (resolved) { + res.writeHead(200, { "Content-Type": "text/plain" }); + res.end("Already handled"); + return; + } - const url = new URL(req.url); + const url = new URL(req.url || "/", `http://127.0.0.1:${actualPort}`); - // Only handle /callback path - if (!url.pathname.startsWith("/callback")) { - return new Response("Not Found", { status: 404 }); - } + // Only handle /callback path + if (!url.pathname.startsWith("/callback")) { + res.writeHead(404, { "Content-Type": "text/plain" }); + res.end("Not Found"); + return; + } - const code = url.searchParams.get("code"); - const state = url.searchParams.get("state"); - const error = url.searchParams.get("error"); - const errorDescription = url.searchParams.get("error_description"); + const code = url.searchParams.get("code"); + const state = url.searchParams.get("state"); + const error = url.searchParams.get("error"); + const errorDescription = url.searchParams.get("error_description"); - // Handle OAuth error - if (error) { - resolved = true; - clearTimeout(timeout); + // Handle OAuth error + if (error) { + resolved = true; + clearTimeout(timeout); - setTimeout(() => serverInstance?.stop(), 100); - rejectCallback(new Error(`OAuth error: ${error}${errorDescription ? ` - ${errorDescription}` : ""}`)); + setTimeout(() => stopServer(), 100); + rejectCallback(new Error(`OAuth error: ${error}${errorDescription ? ` - ${errorDescription}` : ""}`)); - return new Response(` + res.writeHead(400, { "Content-Type": "text/html" }); + res.end(` @@ -120,12 +129,14 @@ export function createCallbackServer( - `, { status: 400, headers: { "Content-Type": "text/html" } }); - } + `); + return; + } - // Validate required parameters - if (!code || !state) { - return new Response(` + // Validate required parameters + if (!code || !state) { + res.writeHead(400, { "Content-Type": "text/html" }); + res.end(` @@ -144,18 +155,20 @@ export function createCallbackServer( - `, { status: 400, headers: { "Content-Type": "text/html" } }); - } + `); + return; + } - // Validate state (CSRF protection) - if (expectedState && state !== expectedState) { - resolved = true; - clearTimeout(timeout); + // Validate state (CSRF protection) + if (expectedState && state !== expectedState) { + resolved = true; + clearTimeout(timeout); - setTimeout(() => serverInstance?.stop(), 100); - rejectCallback(new Error("State mismatch (possible CSRF attack)")); + setTimeout(() => stopServer(), 100); + rejectCallback(new Error("State mismatch (possible CSRF attack)")); - return new Response(` + res.writeHead(400, { "Content-Type": "text/html" }); + res.end(` @@ -174,19 +187,21 @@ export function createCallbackServer( - `, { status: 400, headers: { "Content-Type": "text/html" } }); - } + `); + return; + } - // Success! - resolved = true; - clearTimeout(timeout); + // Success! + resolved = true; + clearTimeout(timeout); - // Resolve first, then stop server asynchronously after response is sent. - // The 100ms delay ensures the HTTP response is fully written before closing. - resolveCallback({ code, state }); - setTimeout(() => serverInstance?.stop(), 100); + // Resolve first, then stop server asynchronously after response is sent. + // The 100ms delay ensures the HTTP response is fully written before closing. + resolveCallback({ code, state }); + setTimeout(() => stopServer(), 100); - return new Response(` + res.writeHead(200, { "Content-Type": "text/html" }); + res.end(` @@ -205,24 +220,19 @@ export function createCallbackServer( - `, { status: 200, headers: { "Content-Type": "text/html" } }); - }, + `); }); - actualPort = serverInstance.port; + serverInstance.listen(port, "127.0.0.1", () => { + const address = serverInstance?.address(); + if (address && typeof address === "object") { + actualPort = address.port; + } + }); return { - server: { stop: () => serverInstance?.stop() }, + server: { stop: stopServer }, promise, getPort: () => actualPort, }; } - -/** - * Get the actual port the server is listening on - * @param server - Bun server instance - * @returns Port number - */ -export function getServerPort(server: ReturnType): number { - return server.port; -} From 5e5f1d4d59b8549910eddfecb99d4abfc7f4eaf2 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 26 Dec 2025 20:21:28 +0000 Subject: [PATCH 2/4] fix: address race condition and missing error handler in auth-server - Add `ready` promise to CallbackServer interface that resolves when server is actually listening, fixing race condition where getPort() could return 0 before server started - Add error handler for server 'error' event to properly handle EADDRINUSE and other binding errors instead of silent failures - Update postgres-ai.ts to await ready promise instead of fragile 100ms setTimeout workaround --- cli/bin/postgres-ai.ts | 5 ++--- cli/lib/auth-server.ts | 29 ++++++++++++++++++++++++++++- 2 files changed, 30 insertions(+), 4 deletions(-) diff --git a/cli/bin/postgres-ai.ts b/cli/bin/postgres-ai.ts index bd278c9..c31e153 100644 --- a/cli/bin/postgres-ai.ts +++ b/cli/bin/postgres-ai.ts @@ -1622,9 +1622,8 @@ auth const requestedPort = opts.port || 0; // 0 = OS assigns available port const callbackServer = authServer.createCallbackServer(requestedPort, params.state, 120000); // 2 minute timeout - // Wait a bit for server to start and get port - await new Promise(resolve => setTimeout(resolve, 100)); - const actualPort = callbackServer.getPort(); + // Wait for server to start and get the actual port + const actualPort = await callbackServer.ready; const redirectUri = `http://localhost:${actualPort}/callback`; console.log(`Callback server listening on port ${actualPort}`); diff --git a/cli/lib/auth-server.ts b/cli/lib/auth-server.ts index e97fd25..dfc620c 100644 --- a/cli/lib/auth-server.ts +++ b/cli/lib/auth-server.ts @@ -14,6 +14,7 @@ export interface CallbackResult { export interface CallbackServer { server: { stop: () => void }; promise: Promise; + ready: Promise; // Resolves with actual port when server is listening getPort: () => number; } @@ -38,9 +39,12 @@ function escapeHtml(str: string | null): string { * @param port - Port to listen on (0 for random available port) * @param expectedState - Expected state parameter for CSRF protection * @param timeoutMs - Timeout in milliseconds - * @returns Server object with promise and getPort function + * @returns Server object with promise, ready promise, and getPort function * * @remarks + * The `ready` promise resolves with the actual port once the server is listening. + * Callers should await `ready` before using `getPort()` when using port 0. + * * The server stops asynchronously ~100ms after the callback resolves/rejects. * This delay ensures the HTTP response is fully sent before closing the connection. * Callers should not attempt to reuse the same port immediately after the promise @@ -55,6 +59,8 @@ export function createCallbackServer( let actualPort = port; let resolveCallback: (value: CallbackResult) => void; let rejectCallback: (reason: Error) => void; + let resolveReady: (port: number) => void; + let rejectReady: (reason: Error) => void; let serverInstance: http.Server | null = null; const promise = new Promise((resolve, reject) => { @@ -62,6 +68,11 @@ export function createCallbackServer( rejectCallback = reject; }); + const ready = new Promise((resolve, reject) => { + resolveReady = resolve; + rejectReady = reject; + }); + const stopServer = () => { if (serverInstance) { serverInstance.close(); @@ -223,16 +234,32 @@ export function createCallbackServer( `); }); + // Handle server errors (e.g., EADDRINUSE) + serverInstance.on("error", (err: NodeJS.ErrnoException) => { + clearTimeout(timeout); + if (err.code === "EADDRINUSE") { + rejectReady(new Error(`Port ${port} is already in use`)); + } else { + rejectReady(new Error(`Server error: ${err.message}`)); + } + if (!resolved) { + resolved = true; + rejectCallback(err); + } + }); + serverInstance.listen(port, "127.0.0.1", () => { const address = serverInstance?.address(); if (address && typeof address === "object") { actualPort = address.port; } + resolveReady(actualPort); }); return { server: { stop: stopServer }, promise, + ready, getPort: () => actualPort, }; } From 15eccaf7a969fdd2fb54ba3a4cf579235f3813be Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 26 Dec 2025 20:24:54 +0000 Subject: [PATCH 3/4] fix: address review feedback - stopServer idempotency and host mismatch - Make stopServer() fully idempotent by clearing timeout on manual stop - Use 127.0.0.1 consistently in redirect URI to match server bind address (fixes potential IPv6 issues on dual-stack hosts where localhost may resolve to ::1 while server only binds to 127.0.0.1) --- cli/bin/postgres-ai.ts | 3 ++- cli/lib/auth-server.ts | 30 +++++++++++++++++++++++++----- 2 files changed, 27 insertions(+), 6 deletions(-) diff --git a/cli/bin/postgres-ai.ts b/cli/bin/postgres-ai.ts index c31e153..552c6dd 100644 --- a/cli/bin/postgres-ai.ts +++ b/cli/bin/postgres-ai.ts @@ -1624,7 +1624,8 @@ auth // Wait for server to start and get the actual port const actualPort = await callbackServer.ready; - const redirectUri = `http://localhost:${actualPort}/callback`; + // Use 127.0.0.1 to match the server bind address (avoids IPv6 issues on some hosts) + const redirectUri = `http://127.0.0.1:${actualPort}/callback`; console.log(`Callback server listening on port ${actualPort}`); diff --git a/cli/lib/auth-server.ts b/cli/lib/auth-server.ts index dfc620c..652ad96 100644 --- a/cli/lib/auth-server.ts +++ b/cli/lib/auth-server.ts @@ -73,7 +73,14 @@ export function createCallbackServer( rejectReady = reject; }); + let timeoutId: ReturnType | null = null; + const stopServer = () => { + // Clear timeout to prevent it firing after manual stop + if (timeoutId) { + clearTimeout(timeoutId); + timeoutId = null; + } if (serverInstance) { serverInstance.close(); serverInstance = null; @@ -81,9 +88,10 @@ export function createCallbackServer( }; // Timeout handler - const timeout = setTimeout(() => { + timeoutId = setTimeout(() => { if (!resolved) { resolved = true; + timeoutId = null; // Already fired, clear reference stopServer(); rejectCallback(new Error("Authentication timeout. Please try again.")); } @@ -113,7 +121,10 @@ export function createCallbackServer( // Handle OAuth error if (error) { resolved = true; - clearTimeout(timeout); + if (timeoutId) { + clearTimeout(timeoutId); + timeoutId = null; + } setTimeout(() => stopServer(), 100); rejectCallback(new Error(`OAuth error: ${error}${errorDescription ? ` - ${errorDescription}` : ""}`)); @@ -173,7 +184,10 @@ export function createCallbackServer( // Validate state (CSRF protection) if (expectedState && state !== expectedState) { resolved = true; - clearTimeout(timeout); + if (timeoutId) { + clearTimeout(timeoutId); + timeoutId = null; + } setTimeout(() => stopServer(), 100); rejectCallback(new Error("State mismatch (possible CSRF attack)")); @@ -204,7 +218,10 @@ export function createCallbackServer( // Success! resolved = true; - clearTimeout(timeout); + if (timeoutId) { + clearTimeout(timeoutId); + timeoutId = null; + } // Resolve first, then stop server asynchronously after response is sent. // The 100ms delay ensures the HTTP response is fully written before closing. @@ -236,7 +253,10 @@ export function createCallbackServer( // Handle server errors (e.g., EADDRINUSE) serverInstance.on("error", (err: NodeJS.ErrnoException) => { - clearTimeout(timeout); + if (timeoutId) { + clearTimeout(timeoutId); + timeoutId = null; + } if (err.code === "EADDRINUSE") { rejectReady(new Error(`Port ${port} is already in use`)); } else { From 656da752250667ad7170d2234eca509ae1b6a076 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 26 Dec 2025 20:36:09 +0000 Subject: [PATCH 4/4] test: increase timeout for --verify integration test The test was timing out at 5s in CI. Increased to 15s to account for slower CI environments spinning up temporary PostgreSQL instances. --- cli/test/init.integration.test.ts | 72 ++++++++++++++++--------------- 1 file changed, 38 insertions(+), 34 deletions(-) diff --git a/cli/test/init.integration.test.ts b/cli/test/init.integration.test.ts index 38ceb49..146e3eb 100644 --- a/cli/test/init.integration.test.ts +++ b/cli/test/init.integration.test.ts @@ -326,41 +326,45 @@ describe.skipIf(skipTests)("integration: prepare-db", () => { } }); - test("--verify returns 0 when ok and non-zero when missing", async () => { - pg = await createTempPostgres(); - - try { - // Prepare: run init - { - const r = runCliInit([pg.adminUri, "--password", "monpw", "--skip-optional-permissions"]); - expect(r.status).toBe(0); - } - - // Verify should pass - { - const r = runCliInit([pg.adminUri, "--verify", "--skip-optional-permissions"]); - expect(r.status).toBe(0); - expect(r.stdout).toMatch(/prepare-db verify: OK/i); - } - - // Break a required privilege and ensure verify fails - { - const c = new Client({ connectionString: pg.adminUri }); - await c.connect(); - await c.query("revoke select on pg_catalog.pg_index from public"); - await c.query("revoke select on pg_catalog.pg_index from postgres_ai_mon"); - await c.end(); - } - { - const r = runCliInit([pg.adminUri, "--verify", "--skip-optional-permissions"]); - expect(r.status).not.toBe(0); - expect(r.stderr).toMatch(/prepare-db verify failed/i); - expect(r.stderr).toMatch(/pg_catalog\.pg_index/i); + test( + "--verify returns 0 when ok and non-zero when missing", + async () => { + pg = await createTempPostgres(); + + try { + // Prepare: run init + { + const r = runCliInit([pg.adminUri, "--password", "monpw", "--skip-optional-permissions"]); + expect(r.status).toBe(0); + } + + // Verify should pass + { + const r = runCliInit([pg.adminUri, "--verify", "--skip-optional-permissions"]); + expect(r.status).toBe(0); + expect(r.stdout).toMatch(/prepare-db verify: OK/i); + } + + // Break a required privilege and ensure verify fails + { + const c = new Client({ connectionString: pg.adminUri }); + await c.connect(); + await c.query("revoke select on pg_catalog.pg_index from public"); + await c.query("revoke select on pg_catalog.pg_index from postgres_ai_mon"); + await c.end(); + } + { + const r = runCliInit([pg.adminUri, "--verify", "--skip-optional-permissions"]); + expect(r.status).not.toBe(0); + expect(r.stderr).toMatch(/prepare-db verify failed/i); + expect(r.stderr).toMatch(/pg_catalog\.pg_index/i); + } + } finally { + await pg.cleanup(); } - } finally { - await pg.cleanup(); - } - }); + }, + { timeout: 15000 } + ); test("--reset-password updates the monitoring role login password", async () => { pg = await createTempPostgres();