Skip to content

fix(mcp): wire requireAuthentication into auth path and gate restart race (#1093)#1453

Merged
datlechin merged 4 commits into
mainfrom
fix/1093-mcp-auth-deadlock
May 28, 2026
Merged

fix(mcp): wire requireAuthentication into auth path and gate restart race (#1093)#1453
datlechin merged 4 commits into
mainfrom
fix/1093-mcp-auth-deadlock

Conversation

@datlechin

Copy link
Copy Markdown
Member

Summary

Fixes #1093. The reported symptom (first authenticated request hangs after turning on Require Authentication; quitting and relaunching does not recover) is the surface of three converging bugs:

  • requireAuthentication was a dead toggle. It was persisted, displayed, and triggered a server restart, but no code on the server side ever read it. MCPHttpRequestRouter always called authenticate(), and MCPBearerTokenAuthenticator always demanded a bearer token. The toggle did nothing semantically, it only fired a restart.
  • Restarts raced. AppSettingsManager.mcp.didSet enqueued unstructured Task { restart } calls that could overlap, and MCPHttpServerTransport.start() returned before the NWListener actually reached .ready. The user's curl could land in the gap.
  • MCPTokenCreateSheet had no @FocusState. Mouse-click into the Token Name field on first sheet open was silently dropped until Tab established first responder.

The fix is structural so the wrong state becomes unreachable:

  • New MCPCompositeAuthenticator wraps the bearer authenticator. When requireAuthentication = false and the request is loopback, it returns an anonymous principal (matches Jupyter --no-token). Remote connections always require a bearer (existing allowRemoteConnections → requireAuthentication = true coercion in AppSettingsManager is unchanged).
  • MCPServerManager now exposes scheduleStart / scheduleStop / scheduleRestart that chain through a serial lifecycleTask. AppSettingsManager.mcp.didSet uses the scheduler instead of fire-and-forget Tasks.
  • MCPHttpServerTransport.start() awaits NWListener.ready (or .failed, .cancelled) via a single-resume CheckedContinuation with a 5s timeout. Callers no longer return until the listener is actually accepting.
  • AppSettingsManager.setRequireAuthentication(_:) auto-provisions a default token on first enable, persists to disk before the restart task fires (mirrors the existing ConnectionStorage "persist first, then notify" invariant), and surfaces the new token to the UI via the existing reveal sheet.
  • MCPTokenCreateSheet uses .defaultFocus($focused, .name) (macOS 14+, WWDC23 session 10162).

Test plan

  • Build in Xcode (Debug). New files are picked up via PBXFileSystemSynchronizedRootGroup; no pbxproj edits.
  • Reproduce the original report: enable MCP, flip Require Authentication ON, take the auto-revealed token, curl /mcp initialize. First request should return 200 OK with Mcp-Session-Id, no hang.
  • Curl with Require Authentication OFF (loopback): should authenticate as anonymous, accepted.
  • Curl with Require Authentication OFF from a remote IP (or simulate): should still demand bearer (regression check, since allowRemoteConnections coerces the flag anyway).
  • Flip Require Authentication OFF then ON repeatedly: lifecycle tasks should serialize cleanly, no overlapping restarts.
  • Open Generate Token sheet: Token Name field accepts mouse click on first open, Generate button enables as soon as you type.
  • swiftlint lint --strict on the changed files (already clean locally).
  • Run MCPCompositeAuthenticatorTests (six matrix cases for loopback/remote × auth on/off × token-present).

CHANGELOG and docs/features/mcp.mdx updated.

@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: d5563281c4

ℹ️ About Codex in GitHub

Your team has set up Codex to 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 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

return nil
}

let tokenStore = MCPTokenStore()

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 Persist bootstrap token through the scheduled restart

When Require authentication is enabled while the MCP server is already running and there are no user tokens, this creates the default token in a separate MCPTokenStore. The subsequent scheduled restart first stops the currently running server, whose older in-memory token store only contains the bridge token; cleanupBridgeToken() then saves that stale store back to mcp-tokens.json, overwriting the just-created default token. In this first-enable path the UI reveals a token that is no longer on disk, and the restarted server loads no user token, so authenticated requests with the revealed token fail.

Useful? React with 👍 / 👎.

@datlechin datlechin merged commit 7562c38 into main May 28, 2026
2 checks passed
@datlechin datlechin deleted the fix/1093-mcp-auth-deadlock branch May 28, 2026 11:32
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.

MCP server: auth-required + freshly-generated token deadlocks first authenticated request (v0.38.0)

1 participant