From 4fd613dc5fc24586cf76f415bb8a7064e4f4f10f Mon Sep 17 00:00:00 2001 From: Gagancreates Date: Tue, 19 May 2026 01:36:01 +0530 Subject: [PATCH] fix: keep createAuthServer fixed-port by default, opt-in fallback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address review feedback: - Flip createAuthServer default to fixed-port; fallback is now opt-in via { fallback: true }. Composio (composio-handler.ts) keeps exact-port semantics with no code change — only the Rowboat sign-in call site, which builds its redirect URI from the actual bound port, opts in. - Wrap post-bind setup (DCR, PKCE, auth URL) in try/catch and close the server on any failure so the port is released for retries. --- apps/x/apps/main/src/auth-server.ts | 25 +++++---- apps/x/apps/main/src/oauth-handler.ts | 81 ++++++++++++++++----------- 2 files changed, 63 insertions(+), 43 deletions(-) diff --git a/apps/x/apps/main/src/auth-server.ts b/apps/x/apps/main/src/auth-server.ts index e7cb5d62..5c46ca3f 100644 --- a/apps/x/apps/main/src/auth-server.ts +++ b/apps/x/apps/main/src/auth-server.ts @@ -106,31 +106,36 @@ function tryBindPort( /** * Create a local HTTP server to handle OAuth callback. * - * When `fixedPort` is true, only the given port is tried — used for providers - * with a pre-registered redirect URI (e.g. Google BYOK) where the port must - * match exactly what the user registered at the OAuth provider console. + * Defaults to fixed-port behaviour: only `port` is tried, and a clear error is + * thrown if it cannot be bound. This is the right behaviour for any provider + * whose redirect URI is pre-registered (Google BYOK, Composio, etc.) — those + * callers must keep using the exact port they've handed to the provider. * - * When `fixedPort` is false (default), tries `port` through `port + PORT_RANGE_SIZE - 1` - * and binds on the first available one, handling both EADDRINUSE and EACCES - * (the latter is common on Windows when Hyper-V/WSL2 reserve the port). + * Opt into `{ fallback: true }` only when the caller is prepared to use the + * port returned in `AuthServerResult` (i.e. the redirect URI is built from the + * actual bound port, not hard-coded). With fallback enabled, scans `port` + * through `port + PORT_RANGE_SIZE - 1` and binds the first available, handling + * both EADDRINUSE and EACCES (the latter is common on Windows when + * Hyper-V/WSL2 reserve the port). */ export async function createAuthServer( port: number = DEFAULT_PORT, onCallback: (callbackUrl: URL) => void | Promise, - fixedPort = false, + opts: { fallback?: boolean } = {}, ): Promise { - const limit = fixedPort ? port : port + PORT_RANGE_SIZE - 1; + const fallback = opts.fallback === true; + const limit = fallback ? port + PORT_RANGE_SIZE - 1 : port; for (let p = port; p <= limit; p++) { try { return await tryBindPort(p, onCallback); } catch (err) { const code = (err as NodeJS.ErrnoException).code; - if ((code === 'EADDRINUSE' || code === 'EACCES') && p < limit) { + if (fallback && (code === 'EADDRINUSE' || code === 'EACCES') && p < limit) { console.warn(`[OAuth] Port ${p} unavailable (${code}), trying ${p + 1}…`); continue; } - if (fixedPort) { + if (!fallback) { const reason = code === 'EACCES' || code === 'EADDRINUSE' ? `Port ${port} is unavailable (${code}). This port must be free for sign-in to work — close any app using it and try again.` : (err instanceof Error ? err.message : String(err)); diff --git a/apps/x/apps/main/src/oauth-handler.ts b/apps/x/apps/main/src/oauth-handler.ts index 343f6707..aebe5099 100644 --- a/apps/x/apps/main/src/oauth-handler.ts +++ b/apps/x/apps/main/src/oauth-handler.ts @@ -220,7 +220,8 @@ export async function resolveStartPort( const registeredPort = await clientRepo.getRegisteredPort(provider); try { - const probe = await createAuthServer(registeredPort, () => { /* probe */ }, true); + // Probe — fixed-port (no fallback) so we know whether the exact registered port is free + const probe = await createAuthServer(registeredPort, () => { /* probe */ }); probe.server.close(); console.log(`[OAuth] ${provider}: registered port ${registeredPort} still available`); return registeredPort; @@ -386,45 +387,59 @@ export async function connectProvider(provider: string, credentials?: { clientId } } }, - isStaticClient, // fixedPort=true for static providers (Google BYOK) + // Static providers (Google BYOK) keep fixed-port behaviour to match the + // pre-registered redirect URI at the provider's console. DCR providers + // can fall back since we register the actual bound port below. + { fallback: !isStaticClient }, ); - // --- OAuth config + PKCE (uses actual bound port for redirect URI) --- - const redirectUri = buildRedirectUri(boundPort); - const config = await getProviderConfiguration(provider, redirectUri, credentials); + // Server is bound. Any throw between here and `activeFlow = ...` would + // leak the port — `cancelActiveFlow` only closes it once activeFlow is set. + try { + const redirectUri = buildRedirectUri(boundPort); + const config = await getProviderConfiguration(provider, redirectUri, credentials); - const { verifier: codeVerifier, challenge: codeChallenge } = await oauthClient.generatePKCE(); - state = oauthClient.generateState(); + const { verifier: codeVerifier, challenge: codeChallenge } = await oauthClient.generatePKCE(); + state = oauthClient.generateState(); - const scopes = providerConfig.scopes || []; - activeFlows.set(state, { codeVerifier, provider, config }); + const scopes = providerConfig.scopes || []; + activeFlows.set(state, { codeVerifier, provider, config }); - const authUrl = oauthClient.buildAuthorizationUrl(config, { - redirect_uri: redirectUri, - scope: scopes.join(' '), - code_challenge: codeChallenge, - state, - }); + const authUrl = oauthClient.buildAuthorizationUrl(config, { + redirect_uri: redirectUri, + scope: scopes.join(' '), + code_challenge: codeChallenge, + state, + }); - // Set timeout to clean up abandoned flows (2 minutes) - const cleanupTimeout = setTimeout(() => { - if (activeFlow?.state === state) { - console.log(`[OAuth] Cleaning up abandoned OAuth flow for ${provider} (timeout)`); - cancelActiveFlow('timed_out'); + // Set timeout to clean up abandoned flows (2 minutes) + const cleanupTimeout = setTimeout(() => { + if (activeFlow?.state === state) { + console.log(`[OAuth] Cleaning up abandoned OAuth flow for ${provider} (timeout)`); + cancelActiveFlow('timed_out'); + } + }, 2 * 60 * 1000); + + activeFlow = { + provider, + state, + server, + cleanupTimeout, + }; + + // Open in system browser (shares cookies/sessions with user's regular browser) + shell.openExternal(authUrl.toString()); + + return { success: true }; + } catch (setupError) { + // Post-bind setup failed — close the server so the port is released and + // a retry isn't blocked by our own zombie listener. + server.close(); + if (state) { + activeFlows.delete(state); } - }, 2 * 60 * 1000); - - activeFlow = { - provider, - state, - server, - cleanupTimeout, - }; - - // Open in system browser (shares cookies/sessions with user's regular browser) - shell.openExternal(authUrl.toString()); - - return { success: true }; + throw setupError; + } } catch (error) { console.error('OAuth connection failed:', error); return {