Skip to content

fix(mcp): open the browser and keep the authorization URL visible for /mcp auth - #920

Open
ismetanin wants to merge 2 commits into
code-yeongyu:mainfrom
ismetanin:fix/mcp-oauth-browser-launch
Open

fix(mcp): open the browser and keep the authorization URL visible for /mcp auth#920
ismetanin wants to merge 2 commits into
code-yeongyu:mainfrom
ismetanin:fix/mcp-oauth-browser-launch

Conversation

@ismetanin

Copy link
Copy Markdown
Contributor

Problem

/mcp auth <server> can never complete an interactive OAuth login. It prints

Opening browser to authorize sentry...

and then hangs forever with no further output.

Two bugs compound:

  1. No browser is ever launched. auth/commands-auth-dispatch.ts wired the openBrowser dependency to ctx.ui.notify(...), so the announcement was simply untrue. The repo already ships a real launcher at src/utils/open-browser.ts (used by the provider login dialog).
  2. The fallback URL is erased. The URL notification is emitted from redirectToAuthorization inside beginAuthorization, and the Opening browser… line was emitted immediately after it. Consecutive status notifications coalesce in the TUI — showStatus rewrites the trailing status text in place when the last two chat children are the previous status spacer/text — so the second line overwrote the URL milliseconds after it appeared.

With no browser and no reachable authorization URL, runInteractive parks on channel.waitForCode() until the session ends.

Fix

  • auth/commands-auth-dispatch.tsopenBrowser calls utils/open-browser.ts for real.
  • auth/commands-auth.ts — each branch of runInteractive emits exactly one notification, carrying the authorization URL, so a failed launch still leaves a copy-pasteable link. Behavior for the callbackUrl-override (paste) branch is unchanged apart from the merged message.
  • builtin/mcp/changes.md — fork ledger entry.

No config, transport, or token-storage behavior changes.

Tests

test/mcp/oauth-callback.test.ts gains an assertion that the single announcement contains the authorization URL, for both the loopback and the callback-override branch. Both assertions fail on main and pass with the fix (verified by reverting the two source files with the test in place).

Verification

Gate Result
npm run check exit 0
test/mcp/oauth-callback.test.ts, oauth-headless.test.ts, auth-modes.test.ts 33 passed
senpi-qa Channel 4 (cli-smoke.mjs --self-test) 8/8 passed, real auth unchanged
senpi-qa flow driver (evidence 20260817-mcp-oauth-browser-launch) 5/5 passed

The QA driver runs runAuth from source against the repo's fake IdP fixture, with a recording stub named open first on PATH so no real browser opens:

{
 "interactive flow completes (token stored, no hang)": true,
 "single announcement before the authorized message": true,
 "announcement carries the authorization URL": true,
 "browser launcher exec'd with the URL": true,
 "real auth.json untouched": true
}

--- notification as the user sees it ---
Opening browser to authorize fix. If it does not open, visit:
<authorization-url>

Note: test/mcp/oauth-race.test.ts fails identically with and without this change in my environment (@earendil-works/pi-tui workspace dist not built); unrelated to this PR.

`/mcp auth <server>` announced "Opening browser to authorize <server>..." but
nothing ever launched a browser: the auth command bridge wired `openBrowser` to
a notification instead of `utils/open-browser.ts`. The URL that notification
carried was then destroyed, because the announcement followed it immediately and
consecutive status lines coalesce in the TUI (`showStatus` rewrites the trailing
status text in place). With no browser and no reachable URL, the interactive
flow blocked on its loopback callback until the session ended.

The bridge now calls the real launcher, and each branch of `runInteractive`
emits a single notification that carries the authorization URL, so a failed
launch still leaves the user a copy-pasteable link.
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.

1 participant