Skip to content

fix: defer OAuth token acquisition to first use; stop browser popups on transient refresh errors#495

Open
medyas wants to merge 1 commit into
zereight:mainfrom
YassineValue:fix/oauth-startup-no-browser
Open

fix: defer OAuth token acquisition to first use; stop browser popups on transient refresh errors#495
medyas wants to merge 1 commit into
zereight:mainfrom
YassineValue:fix/oauth-startup-no-browser

Conversation

@medyas

@medyas medyas commented May 30, 2026

Copy link
Copy Markdown
Contributor

Problem

When GITLAB_USE_OAUTH=true, the server acquires the OAuth token eagerly and synchronously at startup (runServerinitializeOAuthClientgetAccessToken), blocking the MCP handshake on a network call to GitLab.

For a self-hosted GitLab (often behind a VPN), the network frequently isn't ready at the instant the MCP client spawns the server. The refresh fetch then hangs past the client's connection timeout, and on any refresh error the code falls through to the interactive startOAuthFlow()opening a browser on every cold start. Reproduced reliably: the client reports connection timed out after 30000ms while the server is still blocked logging Token expired. Refreshing..., then a browser window opens.

Two distinct issues:

  1. Auth is eager + blocking at startup, even though a lazy path (ensureValidOAuthToken, per tool call) already exists.
  2. The refresh catch treats every failure — including transient network/timeout errors — as "must re-authenticate interactively", so a network blip opens a browser.

Fix

  • Defer token acquisition. Construct the OAuth client synchronously at startup (no network) via a new createGitLabOAuthClient. The token is acquired lazily on the first tool call, by which point the network is ready. initializeOAuthClient is kept and now delegates to the new constructor.
  • Fast-fail refresh. Add a 10s AbortSignal.timeout to the refresh request so it fails fast instead of hanging.
  • Only re-auth interactively when warranted. New pure helper isAuthInvalidTokenResponse(status, body) returns true only for a 400/401 with invalid_grant/invalid_token. The refresh catch opens the browser only in that case; transient and configuration errors propagate without a popup.

Tests

  • New test/oauth-refresh-classify.test.ts — 8 cases for the classifier (invalid_grant/invalid_token → true; invalid_client/invalid_request/5xx/429/non-JSON → false), wired into test:mock.
  • Existing test/test-auth-retry.ts still green (19/19). npm run build clean.

…on transient refresh errors

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.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8175319c7d

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread index.ts
// 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);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Set the cached OAuth token when deferring startup auth

When OAuth starts with an unexpired token already on disk, this deferred path never populates OAUTH_ACCESS_TOKEN: the first tool call enters ensureValidOAuthToken(), oauthClient.hasValidToken() returns true, and it returns before calling getAccessToken() or assigning the token. buildAuthHeaders() then sends no Authorization header, so standard GITLAB_USE_OAUTH=true servers with a valid cached token make unauthenticated GitLab requests until the token expires or is otherwise refreshed.

Useful? React with 👍 / 👎.

@zereight zereight left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix — deferring OAuth startup and only opening the browser for genuinely invalid refresh tokens is the right direction.

I agree with the open Codex comment about populating OAUTH_ACCESS_TOKEN when a valid cached token already exists. Please address that before merge.

A few additional items:

  1. Regression test — after the cached-token fix, please add a small test that covers the first tool call with a valid on-disk token (assert buildAuthHeaders() / the outgoing request includes Authorization). The new isAuthInvalidTokenResponse unit tests are helpful, but they do not exercise the lazy startup path end-to-end.

  2. Rebase onto current main — this branch will conflict in index.ts and package.json after recent merges.

Happy to take another look once the cached-token path is fixed and tests are in place.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants