From 8175319c7df9b95380e9fc15990a35ae1fa56eac Mon Sep 17 00:00:00 2001 From: yassine Date: Sat, 30 May 2026 09:19:42 +0100 Subject: [PATCH] fix: defer OAuth token acquisition to first use; stop browser popups on transient refresh errors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The server acquired the OAuth token eagerly and synchronously at startup, blocking the MCP handshake on a network call. For self-hosted GitLab behind a VPN the network is often not ready at the instant the server spawns, so the refresh fetch hangs past the client's connection timeout, and then any refresh error fell through to the interactive browser flow — opening a browser on every cold start. - Construct the OAuth client synchronously at startup (no network). The token is now acquired lazily on the first tool call via ensureValidOAuthToken, by which point the network is ready. - Add a 10s timeout to the refresh request so it fails fast instead of hanging. - Only start the interactive browser flow when the refresh token is genuinely invalid (400/401 invalid_grant/invalid_token). Transient and configuration errors now propagate without opening a browser. Adds isAuthInvalidTokenResponse with unit tests. --- index.ts | 12 +++--- oauth.ts | 59 ++++++++++++++++++++++++----- package.json | 2 +- test/oauth-refresh-classify.test.ts | 45 ++++++++++++++++++++++ 4 files changed, 103 insertions(+), 15 deletions(-) create mode 100644 test/oauth-refresh-classify.test.ts diff --git a/index.ts b/index.ts index 536b3c1e5..f7bb9fdd8 100644 --- a/index.ts +++ b/index.ts @@ -177,7 +177,7 @@ import { CookieJar, parse as parseCookie } from "tough-cookie"; import { fileURLToPath, URL } from "node:url"; import { z } from "zod"; -import { initializeOAuthClient, GitLabOAuth } from "./oauth.js"; +import { createGitLabOAuthClient, GitLabOAuth } from "./oauth.js"; import { createGitLabOAuthProvider } from "./oauth-proxy.js"; import { mcpAuthRouter } from "@modelcontextprotocol/sdk/server/auth/router.js"; import { normalizeGitLabApiUrl } from "./utils/url.js"; @@ -12470,10 +12470,12 @@ async function runServer() { logger.info("Using OAuth authentication..."); try { const gitlabBaseUrl = GITLAB_API_URL.replace(/\/api\/v4$/, ""); - const oauthResult = await initializeOAuthClient(gitlabBaseUrl); - oauthClient = oauthResult.client; - OAUTH_ACCESS_TOKEN = oauthResult.accessToken; - logger.info("OAuth authentication successful"); + // Construct the client synchronously (no network). The token is + // acquired lazily on the first tool call via ensureValidOAuthToken, + // by which point the network is ready. This avoids blocking startup + // and avoids opening a browser on transient boot-time network errors. + oauthClient = createGitLabOAuthClient(gitlabBaseUrl); + logger.info("OAuth enabled; token acquired lazily on first use."); } catch (error) { logger.error("OAuth authentication failed:", error); process.exit(1); diff --git a/oauth.ts b/oauth.ts index b0bceceec..95e32c08a 100644 --- a/oauth.ts +++ b/oauth.ts @@ -29,6 +29,29 @@ const pendingAuthRequests = new Map< } >(); +const OAUTH_REFRESH_TIMEOUT_MS = 10000; + +/** + * Decide whether a failed token-refresh response means the refresh token is + * genuinely dead (interactive re-login would fix it) versus a transient or + * configuration problem (where opening a browser is wrong). + * + * Returns true ONLY for a 400/401 whose JSON body has error "invalid_grant" + * or "invalid_token". Everything else — config errors (invalid_client, + * invalid_request), 5xx, 429, and non-JSON bodies — returns false. + */ +export function isAuthInvalidTokenResponse(status: number, bodyText: string): boolean { + if (status !== 400 && status !== 401) { + return false; + } + try { + const body = JSON.parse(bodyText) as { error?: string }; + return body.error === "invalid_grant" || body.error === "invalid_token"; + } catch { + return false; + } +} + interface TokenData { access_token: string; refresh_token?: string; @@ -224,11 +247,15 @@ export class GitLabOAuth { "Content-Type": "application/x-www-form-urlencoded", }, body: params.toString(), + signal: AbortSignal.timeout(OAUTH_REFRESH_TIMEOUT_MS), }); if (!response.ok) { - const errorText = await response.text(); - throw new Error(`Token refresh failed: ${response.status} ${errorText}`); + const bodyText = await response.text(); + const authInvalid = isAuthInvalidTokenResponse(response.status, bodyText); + throw Object.assign(new Error(`Token refresh failed: ${response.status} ${bodyText}`), { + authInvalid, + }); } const data = (await response.json()) as { @@ -577,8 +604,13 @@ export class GitLabOAuth { tokenData = await this.refreshAccessToken(tokenData.refresh_token); this.saveToken(tokenData); } catch (error) { - logger.error("Token refresh failed. Starting new OAuth flow...", error); - tokenData = await this.startOAuthFlow(); + if ((error as { authInvalid?: boolean }).authInvalid === true) { + logger.error("Refresh token invalid, starting new OAuth flow...", error); + tokenData = await this.startOAuthFlow(); + } else { + logger.error("Token refresh failed (transient/network), not opening browser.", error); + throw error; + } } } else { logger.info("No refresh token available. Starting new OAuth flow..."); @@ -616,11 +648,11 @@ export class GitLabOAuth { } /** - * Create and initialize a GitLabOAuth client. - * Performs initial authentication (triggers browser flow if needed). - * Returns the client instance and the initial access token. + * Construct a GitLabOAuth client from environment configuration. + * Does NOT perform any network access — no token is acquired here. + * Throws if GITLAB_OAUTH_CLIENT_ID is missing. */ -export async function initializeOAuthClient(gitlabUrl: string = "https://gitlab.com"): Promise<{ client: GitLabOAuth; accessToken: string }> { +export function createGitLabOAuthClient(gitlabUrl: string = "https://gitlab.com"): GitLabOAuth { const clientId = process.env.GITLAB_OAUTH_CLIENT_ID; const clientSecret = process.env.GITLAB_OAUTH_CLIENT_SECRET; const redirectUri = process.env.GITLAB_OAUTH_REDIRECT_URI || "http://127.0.0.1:8888/callback"; @@ -632,7 +664,7 @@ export async function initializeOAuthClient(gitlabUrl: string = "https://gitlab. ); } - const oauth = new GitLabOAuth({ + return new GitLabOAuth({ clientId, clientSecret, redirectUri, @@ -640,6 +672,15 @@ export async function initializeOAuthClient(gitlabUrl: string = "https://gitlab. scopes: [process.env.GITLAB_READ_ONLY_MODE === "true" ? "read_api" : "api"], tokenStoragePath, }); +} + +/** + * Create and initialize a GitLabOAuth client. + * Performs initial authentication (triggers browser flow if needed). + * Returns the client instance and the initial access token. + */ +export async function initializeOAuthClient(gitlabUrl: string = "https://gitlab.com"): Promise<{ client: GitLabOAuth; accessToken: string }> { + const oauth = createGitLabOAuthClient(gitlabUrl); // Single call: triggers browser flow if needed, or reads cached token const accessToken = await oauth.getAccessToken(); diff --git a/package.json b/package.json index 409d8e278..3ec22d899 100644 --- a/package.json +++ b/package.json @@ -51,7 +51,7 @@ "changelog": "auto-changelog -p", "test": "npm run test:all", "test:all": "npm run build && npm run test:mock && npm run test:live", - "test:mock": "node --import tsx/esm --test test/remote-auth-simple-test.ts && node --import tsx/esm --test test/mcp-oauth-tests.ts && node --import tsx/esm --test test/streamable-http-static-token-auth.test.ts && tsx test/oauth-tests.ts && tsx test/test-list-merge-requests.ts && node --import tsx/esm --test test/test-merge-request-pipelines.ts && tsx test/test-list-project-members.ts && tsx test/test-download-attachment.ts && node --import tsx/esm --test test/test-upload-markdown.ts && node --import tsx/esm --test test/test-job-artifacts.ts && node --import tsx/esm --test test/test-deployment-tools.ts && node --import tsx/esm --test test/test-merge-request-approval-state-tools.ts && node --import tsx/esm --test test/test-search-code.ts && node --import tsx/esm --test test/test-tags.ts && node --import tsx/esm --test test/test-toolset-filtering.ts && node --import tsx/esm --test test/test-ci-lint.ts && node --import tsx/esm --test test/test-todos.ts && node --import tsx/esm --test test/test-auth-retry.ts && node --import tsx/esm --test test/test-issue-description-patch.ts && node --import tsx/esm --test test/test-geteffectiveprojectid.ts && node --import tsx/esm --test test/test-get-file-blame.ts && node --import tsx/esm --test test/stateless/codec.test.ts test/stateless/client-id.test.ts test/stateless/callback-proxy.test.ts test/stateless/session-id.test.ts test/stateless/session-id-integration.test.ts test/stateless/config-ttl.test.ts && node --import tsx/esm --test test/utils/tool-args.test.ts && node --import tsx/esm --test test/utils/merge-request-position.test.ts && node --import tsx/esm --test test/nullish-tool-arguments-schema.test.ts && node --import tsx/esm --test test/test-ci-variables.ts", + "test:mock": "node --import tsx/esm --test test/remote-auth-simple-test.ts && node --import tsx/esm --test test/mcp-oauth-tests.ts && node --import tsx/esm --test test/streamable-http-static-token-auth.test.ts && tsx test/oauth-tests.ts && tsx test/test-list-merge-requests.ts && node --import tsx/esm --test test/test-merge-request-pipelines.ts && tsx test/test-list-project-members.ts && tsx test/test-download-attachment.ts && node --import tsx/esm --test test/test-upload-markdown.ts && node --import tsx/esm --test test/test-job-artifacts.ts && node --import tsx/esm --test test/test-deployment-tools.ts && node --import tsx/esm --test test/test-merge-request-approval-state-tools.ts && node --import tsx/esm --test test/test-search-code.ts && node --import tsx/esm --test test/test-tags.ts && node --import tsx/esm --test test/test-toolset-filtering.ts && node --import tsx/esm --test test/test-ci-lint.ts && node --import tsx/esm --test test/test-todos.ts && node --import tsx/esm --test test/test-auth-retry.ts && node --import tsx/esm --test test/test-issue-description-patch.ts && node --import tsx/esm --test test/test-geteffectiveprojectid.ts && node --import tsx/esm --test test/test-get-file-blame.ts && node --import tsx/esm --test test/stateless/codec.test.ts test/stateless/client-id.test.ts test/stateless/callback-proxy.test.ts test/stateless/session-id.test.ts test/stateless/session-id-integration.test.ts test/stateless/config-ttl.test.ts && node --import tsx/esm --test test/utils/tool-args.test.ts && node --import tsx/esm --test test/utils/merge-request-position.test.ts && node --import tsx/esm --test test/nullish-tool-arguments-schema.test.ts && node --import tsx/esm --test test/test-ci-variables.ts && node --import tsx/esm --test test/oauth-refresh-classify.test.ts", "test:stateless": "npm run build && node --import tsx/esm --test test/stateless/codec.test.ts test/stateless/client-id.test.ts test/stateless/callback-proxy.test.ts test/stateless/session-id.test.ts test/stateless/session-id-integration.test.ts test/stateless/config-ttl.test.ts", "test:mcp-oauth": "npm run build && node --import tsx/esm --test test/mcp-oauth-tests.ts", "test:live": "node test/validate-api.js", diff --git a/test/oauth-refresh-classify.test.ts b/test/oauth-refresh-classify.test.ts new file mode 100644 index 000000000..75df516e0 --- /dev/null +++ b/test/oauth-refresh-classify.test.ts @@ -0,0 +1,45 @@ +/** + * Unit tests for isAuthInvalidTokenResponse. + * + * Pure-function tests — no env vars or external services needed. + * Importing ../oauth.js only constructs a logger; it does not touch the + * network or start any server. + */ + +import { describe, test } from "node:test"; +import assert from "node:assert"; +import { isAuthInvalidTokenResponse } from "../oauth.js"; + +describe("isAuthInvalidTokenResponse", () => { + test("400 invalid_grant is auth-invalid", () => { + assert.strictEqual(isAuthInvalidTokenResponse(400, '{"error":"invalid_grant"}'), true); + }); + + test("400 invalid_token is auth-invalid", () => { + assert.strictEqual(isAuthInvalidTokenResponse(400, '{"error":"invalid_token"}'), true); + }); + + test("401 invalid_grant is auth-invalid", () => { + assert.strictEqual(isAuthInvalidTokenResponse(401, '{"error":"invalid_grant"}'), true); + }); + + test("400 invalid_client is not auth-invalid (config error)", () => { + assert.strictEqual(isAuthInvalidTokenResponse(400, '{"error":"invalid_client"}'), false); + }); + + test("400 invalid_request is not auth-invalid (config error)", () => { + assert.strictEqual(isAuthInvalidTokenResponse(400, '{"error":"invalid_request"}'), false); + }); + + test("500 server_error is not auth-invalid (transient)", () => { + assert.strictEqual(isAuthInvalidTokenResponse(500, '{"error":"server_error"}'), false); + }); + + test("429 is not auth-invalid (rate limited)", () => { + assert.strictEqual(isAuthInvalidTokenResponse(429, '{"error":"invalid_grant"}'), false); + }); + + test("non-JSON body is not auth-invalid", () => { + assert.strictEqual(isAuthInvalidTokenResponse(400, "gateway timeout"), false); + }); +});