From 4f57b467fd4a506cc91ef469f5264cc9c7f84b4e Mon Sep 17 00:00:00 2001 From: Sam Morrow Date: Mon, 13 Jul 2026 23:14:47 +0200 Subject: [PATCH 1/3] build(deps): bump go-sdk to 1.7.0-pre.2 and migrate OAuth to multi-round-trip elicitation The go-sdk 1.7.0-pre.2 bump defaults to MCP protocol 2026-07-28, which per SEP-2322 forbids the server from initiating JSON-RPC requests (including `elicitation/create`) while serving a request. The OAuth login flow presents the authorization prompt via `ServerSession.Elicit`, so on 2026-07-28 sessions it now errors ("cannot be sent while serving a request ... return an InputRequests map instead"), which broke TestSessionPrompterPromptActions and would break real 2026-07-28 clients (stdio included, since server/discover is transport-agnostic). Migrate the OAuth middleware to multi-round-trip requests (MRTR) while keeping pre-2026-07-28 clients unchanged: - Legacy clients (< 2026-07-28) keep presenting the prompt via server-initiated elicitation exactly as before. - Modern clients (>= 2026-07-28) receive the authorization prompt as an `InputRequests` elicitation returned from the tool call; the client fulfills it and retries, and the middleware then awaits the token and proceeds. This keeps the authorization URL out of the model context. oauth.Manager gains AwaitToken (resume half of MRTR) and Cancel (tear down on decline). Tests cover the accept/decline/no-capability MRTR paths and assert that server-initiated elicitation is reported undeliverable on 2026-07-28. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 18e70efa-1b2d-4290-ba51-b82998db4ff8 --- go.mod | 2 +- go.sum | 4 +- internal/ghmcp/oauth.go | 155 ++++++++++++++++++++++--- internal/ghmcp/oauth_test.go | 197 +++++++++++++++++++++++++------- internal/oauth/manager.go | 80 ++++++++++--- third-party-licenses.darwin.md | 4 +- third-party-licenses.linux.md | 4 +- third-party-licenses.windows.md | 4 +- 8 files changed, 374 insertions(+), 76 deletions(-) diff --git a/go.mod b/go.mod index 358a271a7a..b11455139d 100644 --- a/go.mod +++ b/go.mod @@ -10,7 +10,7 @@ require ( github.com/josephburnett/jd/v2 v2.5.0 github.com/lithammer/fuzzysearch v1.1.8 github.com/microcosm-cc/bluemonday v1.0.27 - github.com/modelcontextprotocol/go-sdk v1.7.0-pre.1 + github.com/modelcontextprotocol/go-sdk v1.7.0-pre.2 github.com/muesli/cache2go v0.0.0-20221011235721-518229cd8021 github.com/shurcooL/githubv4 v0.0.0-20240727222349-48295856cce7 github.com/shurcooL/graphql v0.0.0-20230722043721-ed46e5a46466 diff --git a/go.sum b/go.sum index f3f23b549a..64e5b47839 100644 --- a/go.sum +++ b/go.sum @@ -39,8 +39,8 @@ github.com/lithammer/fuzzysearch v1.1.8 h1:/HIuJnjHuXS8bKaiTMeeDlW2/AyIWk2brx1V8 github.com/lithammer/fuzzysearch v1.1.8/go.mod h1:IdqeyBClc3FFqSzYq/MXESsS4S0FsZ5ajtkr5xPLts4= github.com/microcosm-cc/bluemonday v1.0.27 h1:MpEUotklkwCSLeH+Qdx1VJgNqLlpY2KXwXFM08ygZfk= github.com/microcosm-cc/bluemonday v1.0.27/go.mod h1:jFi9vgW+H7c3V0lb6nR74Ib/DIB5OBs92Dimizgw2cA= -github.com/modelcontextprotocol/go-sdk v1.7.0-pre.1 h1:GlMIJyMHFX76bBSQuBCLXZ7pB9cGh4VBS6O5wGd0tgI= -github.com/modelcontextprotocol/go-sdk v1.7.0-pre.1/go.mod h1:dL7u98E/zjJTGzEq+j30jQ8K2k1mb6LeAH4inEcSGts= +github.com/modelcontextprotocol/go-sdk v1.7.0-pre.2 h1:3JwUps1pdSpXYndBMGO9SMca6CSkP9AKnOaKAkSSGHc= +github.com/modelcontextprotocol/go-sdk v1.7.0-pre.2/go.mod h1:dL7u98E/zjJTGzEq+j30jQ8K2k1mb6LeAH4inEcSGts= github.com/muesli/cache2go v0.0.0-20221011235721-518229cd8021 h1:31Y+Yu373ymebRdJN1cWLLooHH8xAr0MhKTEJGV/87g= github.com/muesli/cache2go v0.0.0-20221011235721-518229cd8021/go.mod h1:WERUkUryfUWlrHnFSO/BEUZ+7Ns8aZy7iVOGewxKzcc= github.com/pelletier/go-toml/v2 v2.2.4 h1:mye9XuhQ6gvn5h28+VilKrrPoQVanw5PMw/TB0t5Ec4= diff --git a/internal/ghmcp/oauth.go b/internal/ghmcp/oauth.go index abc6d3d11c..130ada1655 100644 --- a/internal/ghmcp/oauth.go +++ b/internal/ghmcp/oauth.go @@ -91,17 +91,47 @@ func (p *sessionPrompter) PromptForm(ctx context.Context, prompt oauth.Prompt) e type oauthAuthenticator interface { HasToken() bool Authenticate(ctx context.Context, prompter oauth.Prompter) (*oauth.Outcome, error) + AwaitToken(ctx context.Context) (*oauth.Outcome, error) + Cancel() +} + +// oauthElicitID is the stable key for the authorization elicitation in the +// multi-round-trip flow. The client echoes it back in InputResponses when it +// retries the tool call, so the middleware can recognize the user's response. +const oauthElicitID = "github_authorization" + +// protocolVersionNoServerElicitation is the first MCP protocol version that +// forbids server-initiated JSON-RPC requests (SEP-2322): from this version on +// the server may not send elicitation/create while serving a request and must +// instead return an InputRequests map from the tool call (multi round-trip +// requests). It mirrors the go-sdk's internal constant of the same value, which +// the SDK does not export. +const protocolVersionNoServerElicitation = "2026-07-28" + +// serverMayInitiateElicitation reports whether the server is permitted to send +// elicitation requests to the client itself, which the spec allows only before +// protocol version 2026-07-28. A nil or un-negotiated session (only reached in +// unit tests; a real tools/call is always initialized) is treated as legacy. +func serverMayInitiateElicitation(ss *mcp.ServerSession) bool { + if ss == nil { + return true + } + params := ss.InitializeParams() + return params == nil || params.ProtocolVersion < protocolVersionNoServerElicitation } // createOAuthMiddleware returns receiving middleware that authorizes the session // lazily, on the first tool call. Authorization is deferred until here (rather // than at startup) because the prompts depend on an initialized session whose -// elicitation capabilities are known. +// elicitation capabilities and protocol version are known. // // When a token is already available the call proceeds untouched. Otherwise the -// flow runs: secure channels (browser, URL elicitation) block until the token -// arrives and then the call proceeds; the last-resort channel returns the -// instruction to the user as a tool result and asks them to retry. +// authorization flow runs, presenting its prompt over whichever channel the +// negotiated protocol allows: on protocol versions before 2026-07-28 the server +// elicits directly; from 2026-07-28 on, where server-initiated requests are +// forbidden (SEP-2322), it uses multi-round-trip elicitation returned from the +// tool call. Either way the last-resort channel returns the instruction as a +// tool result and asks the user to retry. func createOAuthMiddleware(mgr oauthAuthenticator, logger *slog.Logger) func(next mcp.MethodHandler) mcp.MethodHandler { return func(next mcp.MethodHandler) mcp.MethodHandler { return func(ctx context.Context, method string, request mcp.Request) (mcp.Result, error) { @@ -114,18 +144,115 @@ func createOAuthMiddleware(mgr oauthAuthenticator, logger *slog.Logger) func(nex return next(ctx, method, request) } - outcome, err := mgr.Authenticate(ctx, &sessionPrompter{session: callReq.Session}) - if err != nil { - return nil, fmt.Errorf("github authorization failed: %w", err) - } - if outcome != nil && outcome.UserAction != nil { - logger.Info("surfacing github authorization instructions to user") - return &mcp.CallToolResult{ - Content: []mcp.Content{&mcp.TextContent{Text: outcome.UserAction.Message}}, - }, nil + if serverMayInitiateElicitation(callReq.Session) { + return authorizeViaServerElicitation(ctx, mgr, next, method, request, callReq, logger) } - return next(ctx, method, request) + return authorizeViaMultiRoundTrip(ctx, mgr, next, method, request, callReq, logger) + } + } +} + +// authorizeViaServerElicitation drives authorization on legacy protocol versions +// (before 2026-07-28), where the server may present the prompt itself via +// server-initiated elicitation. It blocks until the token arrives, then proceeds. +func authorizeViaServerElicitation(ctx context.Context, mgr oauthAuthenticator, next mcp.MethodHandler, method string, request mcp.Request, callReq *mcp.CallToolRequest, logger *slog.Logger) (mcp.Result, error) { + outcome, err := mgr.Authenticate(ctx, &sessionPrompter{session: callReq.Session}) + if err != nil { + return nil, fmt.Errorf("github authorization failed: %w", err) + } + if outcome != nil && outcome.UserAction != nil { + logger.Info("surfacing github authorization instructions to user") + return &mcp.CallToolResult{ + Content: []mcp.Content{&mcp.TextContent{Text: outcome.UserAction.Message}}, + }, nil + } + return next(ctx, method, request) +} + +// authorizeViaMultiRoundTrip drives authorization on protocol version 2026-07-28 +// and later, where server-initiated requests are forbidden (SEP-2322). The first +// tool call starts the flow and returns the authorization prompt as an +// elicitation input request; the client presents it and retries the call with +// the user's response, at which point we wait for the token and proceed. +func authorizeViaMultiRoundTrip(ctx context.Context, mgr oauthAuthenticator, next mcp.MethodHandler, method string, request mcp.Request, callReq *mcp.CallToolRequest, logger *slog.Logger) (mcp.Result, error) { + // Retry: the client fulfilled the authorization elicitation and re-sent the + // call with the user's response. + if resp, ok := callReq.Params.InputResponses[oauthElicitID]; ok { + res, _ := resp.(*mcp.ElicitResult) + if res == nil || res.Action != "accept" { + // The user declined or dismissed the prompt; tear the flow down so it + // does not linger, and let them retry when they are ready. + mgr.Cancel() + return &mcp.CallToolResult{ + Content: []mcp.Content{&mcp.TextContent{Text: "GitHub authorization was declined. Retry when you're ready to authorize."}}, + }, nil } + outcome, err := mgr.AwaitToken(ctx) + if err != nil { + return nil, fmt.Errorf("github authorization failed: %w", err) + } + if outcome != nil && outcome.UserAction != nil { + // The user acknowledged the prompt but has not finished authorizing; + // surface the instructions so they can complete it and retry. + logger.Info("surfacing github authorization instructions to user") + return &mcp.CallToolResult{ + Content: []mcp.Content{&mcp.TextContent{Text: outcome.UserAction.Message}}, + }, nil + } + return next(ctx, method, request) + } + + // First attempt: start the flow. A nil prompter keeps the manager from + // initiating any elicitation itself (forbidden on this protocol); it opens a + // server-side browser when possible, otherwise returns the authorization + // instructions for us to present via multi-round-trip elicitation. + outcome, err := mgr.Authenticate(ctx, nil) + if err != nil { + return nil, fmt.Errorf("github authorization failed: %w", err) + } + if outcome == nil || outcome.UserAction == nil { + // Already authorized (e.g. the server opened a browser and the flow + // completed); proceed. + return next(ctx, method, request) + } + + elicit := authorizationElicitParams(outcome.UserAction, &sessionPrompter{session: callReq.Session}) + if elicit == nil { + // The client cannot present an elicitation (no capability, or no URL to + // show); fall back to returning the instructions as a tool result. + logger.Info("surfacing github authorization instructions to user") + return &mcp.CallToolResult{ + Content: []mcp.Content{&mcp.TextContent{Text: outcome.UserAction.Message}}, + }, nil + } + logger.Info("requesting github authorization via elicitation") + return &mcp.CallToolResult{ + InputRequests: mcp.InputRequestMap{oauthElicitID: elicit}, + RequestState: "github-authorization-pending", + }, nil +} + +// authorizationElicitParams builds the elicitation that presents the +// authorization instructions to the user. It mirrors sessionPrompter's channel +// selection: URL-mode when the client supports it, otherwise form-mode. It +// returns nil when the client advertised no elicitation capability or there is +// no authorization URL to show, so the caller falls back to a tool-result +// message. +func authorizationElicitParams(ua *oauth.UserAction, p *sessionPrompter) *mcp.ElicitParams { + if ua.URL == "" { + return nil + } + message := "Authorize the GitHub MCP Server to continue." + if ua.UserCode != "" { + message = fmt.Sprintf("Enter code %s to authorize the GitHub MCP Server.", ua.UserCode) + } + switch { + case p.CanPromptURL(): + return &mcp.ElicitParams{Mode: "url", Message: message, URL: ua.URL, ElicitationID: rand.Text()} + case p.CanPromptForm(): + return &mcp.ElicitParams{Mode: "form", Message: message} + default: + return nil } } diff --git a/internal/ghmcp/oauth_test.go b/internal/ghmcp/oauth_test.go index 732d080e40..5dbba67ade 100644 --- a/internal/ghmcp/oauth_test.go +++ b/internal/ghmcp/oauth_test.go @@ -140,57 +140,49 @@ func TestSessionPrompterCapabilities(t *testing.T) { } } -func TestSessionPrompterPromptActions(t *testing.T) { +// TestSessionPrompterModernProtocolUnavailable verifies that on protocol version +// 2026-07-28 and later — the default negotiated by current clients — the server +// may not initiate elicitation (SEP-2322), so PromptURL and PromptForm report +// the prompt as undeliverable. This is what routes authorization to the +// multi-round-trip path instead (see authorizeViaMultiRoundTrip). +func TestSessionPrompterModernProtocolUnavailable(t *testing.T) { t.Parallel() - tests := []struct { - name string - action string - wantDecline bool - }{ - {name: "accept", action: "accept", wantDecline: false}, - {name: "decline", action: "decline", wantDecline: true}, - {name: "cancel", action: "cancel", wantDecline: true}, - } - caps := &mcp.ClientCapabilities{Elicitation: &mcp.ElicitationCapabilities{ URL: &mcp.URLElicitationCapabilities{}, Form: &mcp.FormElicitationCapabilities{}, }} - for _, tc := range tests { - // URL and form modes share the accept/decline mapping; cover both. - for _, mode := range []string{"url", "form"} { - t.Run(tc.name+"/"+mode, func(t *testing.T) { - t.Parallel() - - handler := func(_ context.Context, _ *mcp.ElicitRequest) (*mcp.ElicitResult, error) { - return &mcp.ElicitResult{Action: tc.action}, nil - } + // The handler should never be reached: the SDK blocks the server-initiated + // request before it leaves the server. + handler := func(_ context.Context, _ *mcp.ElicitRequest) (*mcp.ElicitResult, error) { + return &mcp.ElicitResult{Action: "accept"}, nil + } - got := runProbe(t, caps, handler, func(ctx context.Context, p *sessionPrompter) string { - var err error - if mode == "url" { - err = p.PromptURL(ctx, oauth.Prompt{Message: "msg", URL: "https://example.com/auth"}) - } else { - err = p.PromptForm(ctx, oauth.Prompt{Message: "msg"}) - } - if err == nil { - return "ok" - } - if err == oauth.ErrPromptDeclined { - return "declined" - } - return "error: " + err.Error() - }) + for _, mode := range []string{"url", "form"} { + t.Run(mode, func(t *testing.T) { + t.Parallel() - if tc.wantDecline { - assert.Equal(t, "declined", got) + got := runProbe(t, caps, handler, func(ctx context.Context, p *sessionPrompter) string { + var err error + if mode == "url" { + err = p.PromptURL(ctx, oauth.Prompt{Message: "msg", URL: "https://example.com/auth"}) } else { - assert.Equal(t, "ok", got) + err = p.PromptForm(ctx, oauth.Prompt{Message: "msg"}) + } + switch { + case err == nil: + return "ok" + case errors.Is(err, oauth.ErrPromptUnavailable): + return "unavailable" + default: + return "error: " + err.Error() } }) - } + + assert.Equal(t, "unavailable", got, + "server-initiated elicitation must be reported undeliverable on protocol 2026-07-28+") + }) } } @@ -247,6 +239,15 @@ type fakeAuthenticator struct { err error authCalls int lastPrompter oauth.Prompter + + // awaitOutcome/awaitErr are returned by AwaitToken; tokenAfterAwait flips + // HasToken to true once AwaitToken is called, simulating a flow that + // acquires the token while the user acts on the elicitation. + awaitOutcome *oauth.Outcome + awaitErr error + tokenAfterAwait bool + awaitCalls int + cancelCalls int } func (f *fakeAuthenticator) HasToken() bool { return f.hasToken } @@ -257,6 +258,16 @@ func (f *fakeAuthenticator) Authenticate(_ context.Context, prompter oauth.Promp return f.outcome, f.err } +func (f *fakeAuthenticator) AwaitToken(context.Context) (*oauth.Outcome, error) { + f.awaitCalls++ + if f.tokenAfterAwait { + f.hasToken = true + } + return f.awaitOutcome, f.awaitErr +} + +func (f *fakeAuthenticator) Cancel() { f.cancelCalls++ } + func TestCreateOAuthMiddleware(t *testing.T) { t.Parallel() @@ -332,6 +343,114 @@ func TestCreateOAuthMiddleware(t *testing.T) { }) } +// runOAuthMiddlewareCall stands up an in-memory client/server pair with the +// OAuth middleware installed ahead of a probe tool, then calls the tool from a +// default (protocol 2026-07-28) client — driving the multi-round-trip +// authorization path. It returns the final tool-result text and whether the +// probe tool ultimately ran. +func runOAuthMiddlewareCall( + t *testing.T, + fake *fakeAuthenticator, + clientCaps *mcp.ClientCapabilities, + elicitationHandler func(context.Context, *mcp.ElicitRequest) (*mcp.ElicitResult, error), +) (string, bool) { + t.Helper() + + server := mcp.NewServer(&mcp.Implementation{Name: "test-server", Version: "v0.0.1"}, nil) + var toolRan bool + mcp.AddTool(server, &mcp.Tool{Name: probeToolName}, func(_ context.Context, _ *mcp.CallToolRequest, _ struct{}) (*mcp.CallToolResult, any, error) { + toolRan = true + return &mcp.CallToolResult{Content: []mcp.Content{&mcp.TextContent{Text: "tool-ran"}}}, nil, nil + }) + server.AddReceivingMiddleware(createOAuthMiddleware(fake, discardLogger())) + + st, ct := mcp.NewInMemoryTransports() + + ss, err := server.Connect(context.Background(), st, nil) + require.NoError(t, err) + t.Cleanup(func() { _ = ss.Close() }) + + client := mcp.NewClient(&mcp.Implementation{Name: "test-client", Version: "v0.0.1"}, &mcp.ClientOptions{ + Capabilities: clientCaps, + ElicitationHandler: elicitationHandler, + }) + cs, err := client.Connect(context.Background(), ct, nil) + require.NoError(t, err) + t.Cleanup(func() { _ = cs.Close() }) + + res, err := cs.CallTool(context.Background(), &mcp.CallToolParams{Name: probeToolName}) + require.NoError(t, err) + require.Len(t, res.Content, 1) + text, ok := res.Content[0].(*mcp.TextContent) + require.True(t, ok, "tool result should be text content") + return text.Text, toolRan +} + +// TestOAuthMiddlewareMultiRoundTrip exercises the protocol-2026-07-28 path, where +// server-initiated elicitation is forbidden and authorization must be presented +// as a multi-round-trip input request that the client fulfills and retries. +func TestOAuthMiddlewareMultiRoundTrip(t *testing.T) { + t.Parallel() + + urlCaps := &mcp.ClientCapabilities{Elicitation: &mcp.ElicitationCapabilities{URL: &mcp.URLElicitationCapabilities{}}} + + t.Run("accepted elicitation authorizes and proceeds", func(t *testing.T) { + t.Parallel() + fake := &fakeAuthenticator{ + outcome: &oauth.Outcome{UserAction: &oauth.UserAction{URL: "https://example.com/auth", Message: "manual"}}, + tokenAfterAwait: true, + } + var elicited int + accept := func(_ context.Context, _ *mcp.ElicitRequest) (*mcp.ElicitResult, error) { + elicited++ + return &mcp.ElicitResult{Action: "accept"}, nil + } + + text, toolRan := runOAuthMiddlewareCall(t, fake, urlCaps, accept) + + assert.Equal(t, "tool-ran", text, "the tool should run once authorization completes") + assert.True(t, toolRan) + assert.Equal(t, 1, elicited, "the client should be asked to authorize exactly once") + assert.Equal(t, 1, fake.awaitCalls, "the middleware should await the token on retry") + assert.Zero(t, fake.cancelCalls) + assert.Nil(t, fake.lastPrompter, "the manager must not be given a prompter on this protocol") + }) + + t.Run("declined elicitation cancels and does not run the tool", func(t *testing.T) { + t.Parallel() + fake := &fakeAuthenticator{ + outcome: &oauth.Outcome{UserAction: &oauth.UserAction{URL: "https://example.com/auth", Message: "manual"}}, + } + decline := func(_ context.Context, _ *mcp.ElicitRequest) (*mcp.ElicitResult, error) { + return &mcp.ElicitResult{Action: "decline"}, nil + } + + text, toolRan := runOAuthMiddlewareCall(t, fake, urlCaps, decline) + + assert.False(t, toolRan, "the tool must not run when authorization is declined") + assert.Contains(t, text, "declined") + assert.Equal(t, 1, fake.cancelCalls, "a decline should cancel the in-flight flow") + assert.Zero(t, fake.awaitCalls) + }) + + t.Run("no elicitation capability falls back to a tool-result message", func(t *testing.T) { + t.Parallel() + const message = "Open https://example.com/auth to authorize, then retry." + fake := &fakeAuthenticator{ + outcome: &oauth.Outcome{UserAction: &oauth.UserAction{URL: "https://example.com/auth", Message: message}}, + } + + // No elicitation capability advertised, and no handler needed since the + // middleware should not issue an input request. + text, toolRan := runOAuthMiddlewareCall(t, fake, &mcp.ClientCapabilities{}, nil) + + assert.False(t, toolRan, "the tool must not run before authorization completes") + assert.Equal(t, message, text, "the manual instructions should be surfaced as a tool result") + assert.Zero(t, fake.awaitCalls) + assert.Zero(t, fake.cancelCalls) + }) +} + // TestRunStdioServerRejectsTokenAndOAuth verifies the mutually-exclusive guard: // supplying both a static token and an OAuth manager is rejected before the // server starts, rather than silently preferring one for auth and the other for diff --git a/internal/oauth/manager.go b/internal/oauth/manager.go index a78e919df7..5ca31f831c 100644 --- a/internal/oauth/manager.go +++ b/internal/oauth/manager.go @@ -68,6 +68,7 @@ type Manager struct { status flowStatus pending *UserAction done chan struct{} + cancelFlow context.CancelFunc // cancels the in-flight flow, if any lastErr error refreshErrLogged bool // true once a refresh failure has been logged, reset on re-auth } @@ -181,6 +182,9 @@ func (m *Manager) Authenticate(ctx context.Context, prompter Prompter) (*Outcome m.mu.Unlock() bgCtx, cancel := context.WithTimeout(context.Background(), DefaultAuthTimeout) + m.mu.Lock() + m.cancelFlow = cancel + m.mu.Unlock() go m.runFlow(bgCtx, cancel, plan) if plan.userAction != nil { @@ -189,6 +193,46 @@ func (m *Manager) Authenticate(ctx context.Context, prompter Prompter) (*Outcome return m.joinWait(ctx, done) } +// AwaitToken blocks until the in-flight authorization flow yields a token, the +// flow ends without one, or ctx is done. It is the resume half of the +// multi-round-trip flow: a transport that presented the authorization prompt +// itself (via elicitation returned from a tool call) calls this once the user +// has acted, to wait for the background token acquisition to finish. +// +// It returns (nil, nil) once a token is available (proceed), (&Outcome{UserAction}, +// nil) when the user must still act out of band, or (nil, err) on failure. +func (m *Manager) AwaitToken(ctx context.Context) (*Outcome, error) { + if m.AccessToken() != "" { + return nil, nil + } + m.mu.Lock() + done := m.done + m.mu.Unlock() + if done == nil { + // No flow is in flight; report whatever terminal state it left behind. + return m.outcomeAfterFlow() + } + select { + case <-done: + return m.outcomeAfterFlow() + case <-ctx.Done(): + return nil, ctx.Err() + } +} + +// Cancel aborts the in-flight authorization flow, if any. It is used when the +// user declines the authorization prompt so the background flow (callback +// listener or device poll) is torn down promptly rather than lingering until it +// times out. It is a no-op when no flow is running. +func (m *Manager) Cancel() { + m.mu.Lock() + cancel := m.cancelFlow + m.mu.Unlock() + if cancel != nil { + cancel() + } +} + // runFlow executes a prepared flow in the background and records the result. The // optional display prompt runs concurrently: a decline (or other failure) aborts // the flow, while an undeliverable prompt degrades to the manual fallback without @@ -254,6 +298,7 @@ func (m *Manager) complete(tok *oauth2.Token, err error) { m.status = statusIdle m.pending = nil + m.cancelFlow = nil if err != nil { m.lastErr = err m.logger.Debug("oauth flow failed", "error", err) @@ -280,25 +325,32 @@ func (m *Manager) complete(tok *oauth2.Token, err error) { func (m *Manager) joinWait(ctx context.Context, done chan struct{}) (*Outcome, error) { select { case <-done: - if m.AccessToken() != "" { - return nil, nil - } - m.mu.Lock() - pending := m.pending - err := m.lastErr - m.mu.Unlock() - if pending != nil { - return &Outcome{UserAction: pending}, nil - } - if err != nil { - return nil, err - } - return nil, errors.New("authorization did not complete") + return m.outcomeAfterFlow() case <-ctx.Done(): return nil, ctx.Err() } } +// outcomeAfterFlow reports the result once the flow's done channel has closed +// (or when there is no flow in flight): a token to proceed (nil, nil), a pending +// user action to surface, or the flow's error. +func (m *Manager) outcomeAfterFlow() (*Outcome, error) { + if m.AccessToken() != "" { + return nil, nil + } + m.mu.Lock() + pending := m.pending + err := m.lastErr + m.mu.Unlock() + if pending != nil { + return &Outcome{UserAction: pending}, nil + } + if err != nil { + return nil, err + } + return nil, errors.New("authorization did not complete") +} + func (m *Manager) oauth2Config(redirectURL string) *oauth2.Config { return &oauth2.Config{ ClientID: m.config.ClientID, diff --git a/third-party-licenses.darwin.md b/third-party-licenses.darwin.md index 88235f3f40..fca63f2256 100644 --- a/third-party-licenses.darwin.md +++ b/third-party-licenses.darwin.md @@ -24,8 +24,8 @@ The following packages are included for the amd64, arm64 architectures. - [github.com/josephburnett/jd/v2](https://pkg.go.dev/github.com/josephburnett/jd/v2) ([MIT](https://github.com/josephburnett/jd/blob/v2.5.0/v2/LICENSE)) - [github.com/lithammer/fuzzysearch/fuzzy](https://pkg.go.dev/github.com/lithammer/fuzzysearch/fuzzy) ([MIT](https://github.com/lithammer/fuzzysearch/blob/v1.1.8/LICENSE)) - [github.com/microcosm-cc/bluemonday](https://pkg.go.dev/github.com/microcosm-cc/bluemonday) ([BSD-3-Clause](https://github.com/microcosm-cc/bluemonday/blob/v1.0.27/LICENSE.md)) - - [github.com/modelcontextprotocol/go-sdk](https://pkg.go.dev/github.com/modelcontextprotocol/go-sdk) ([Apache-2.0](https://github.com/modelcontextprotocol/go-sdk/blob/v1.7.0-pre.1/LICENSE)) - - [github.com/modelcontextprotocol/go-sdk](https://pkg.go.dev/github.com/modelcontextprotocol/go-sdk) ([MIT](https://github.com/modelcontextprotocol/go-sdk/blob/v1.7.0-pre.1/LICENSE)) + - [github.com/modelcontextprotocol/go-sdk](https://pkg.go.dev/github.com/modelcontextprotocol/go-sdk) ([Apache-2.0](https://github.com/modelcontextprotocol/go-sdk/blob/v1.7.0-pre.2/LICENSE)) + - [github.com/modelcontextprotocol/go-sdk](https://pkg.go.dev/github.com/modelcontextprotocol/go-sdk) ([MIT](https://github.com/modelcontextprotocol/go-sdk/blob/v1.7.0-pre.2/LICENSE)) - [github.com/muesli/cache2go](https://pkg.go.dev/github.com/muesli/cache2go) ([BSD-3-Clause](https://github.com/muesli/cache2go/blob/518229cd8021/LICENSE.txt)) - [github.com/pelletier/go-toml/v2](https://pkg.go.dev/github.com/pelletier/go-toml/v2) ([MIT](https://github.com/pelletier/go-toml/blob/v2.2.4/LICENSE)) - [github.com/sagikazarmark/locafero](https://pkg.go.dev/github.com/sagikazarmark/locafero) ([MIT](https://github.com/sagikazarmark/locafero/blob/v0.11.0/LICENSE)) diff --git a/third-party-licenses.linux.md b/third-party-licenses.linux.md index e3762f5c04..dc3798c769 100644 --- a/third-party-licenses.linux.md +++ b/third-party-licenses.linux.md @@ -24,8 +24,8 @@ The following packages are included for the 386, amd64, arm64 architectures. - [github.com/josephburnett/jd/v2](https://pkg.go.dev/github.com/josephburnett/jd/v2) ([MIT](https://github.com/josephburnett/jd/blob/v2.5.0/v2/LICENSE)) - [github.com/lithammer/fuzzysearch/fuzzy](https://pkg.go.dev/github.com/lithammer/fuzzysearch/fuzzy) ([MIT](https://github.com/lithammer/fuzzysearch/blob/v1.1.8/LICENSE)) - [github.com/microcosm-cc/bluemonday](https://pkg.go.dev/github.com/microcosm-cc/bluemonday) ([BSD-3-Clause](https://github.com/microcosm-cc/bluemonday/blob/v1.0.27/LICENSE.md)) - - [github.com/modelcontextprotocol/go-sdk](https://pkg.go.dev/github.com/modelcontextprotocol/go-sdk) ([Apache-2.0](https://github.com/modelcontextprotocol/go-sdk/blob/v1.7.0-pre.1/LICENSE)) - - [github.com/modelcontextprotocol/go-sdk](https://pkg.go.dev/github.com/modelcontextprotocol/go-sdk) ([MIT](https://github.com/modelcontextprotocol/go-sdk/blob/v1.7.0-pre.1/LICENSE)) + - [github.com/modelcontextprotocol/go-sdk](https://pkg.go.dev/github.com/modelcontextprotocol/go-sdk) ([Apache-2.0](https://github.com/modelcontextprotocol/go-sdk/blob/v1.7.0-pre.2/LICENSE)) + - [github.com/modelcontextprotocol/go-sdk](https://pkg.go.dev/github.com/modelcontextprotocol/go-sdk) ([MIT](https://github.com/modelcontextprotocol/go-sdk/blob/v1.7.0-pre.2/LICENSE)) - [github.com/muesli/cache2go](https://pkg.go.dev/github.com/muesli/cache2go) ([BSD-3-Clause](https://github.com/muesli/cache2go/blob/518229cd8021/LICENSE.txt)) - [github.com/pelletier/go-toml/v2](https://pkg.go.dev/github.com/pelletier/go-toml/v2) ([MIT](https://github.com/pelletier/go-toml/blob/v2.2.4/LICENSE)) - [github.com/sagikazarmark/locafero](https://pkg.go.dev/github.com/sagikazarmark/locafero) ([MIT](https://github.com/sagikazarmark/locafero/blob/v0.11.0/LICENSE)) diff --git a/third-party-licenses.windows.md b/third-party-licenses.windows.md index eb0743558a..db7db1ec1b 100644 --- a/third-party-licenses.windows.md +++ b/third-party-licenses.windows.md @@ -25,8 +25,8 @@ The following packages are included for the 386, amd64, arm64 architectures. - [github.com/josephburnett/jd/v2](https://pkg.go.dev/github.com/josephburnett/jd/v2) ([MIT](https://github.com/josephburnett/jd/blob/v2.5.0/v2/LICENSE)) - [github.com/lithammer/fuzzysearch/fuzzy](https://pkg.go.dev/github.com/lithammer/fuzzysearch/fuzzy) ([MIT](https://github.com/lithammer/fuzzysearch/blob/v1.1.8/LICENSE)) - [github.com/microcosm-cc/bluemonday](https://pkg.go.dev/github.com/microcosm-cc/bluemonday) ([BSD-3-Clause](https://github.com/microcosm-cc/bluemonday/blob/v1.0.27/LICENSE.md)) - - [github.com/modelcontextprotocol/go-sdk](https://pkg.go.dev/github.com/modelcontextprotocol/go-sdk) ([Apache-2.0](https://github.com/modelcontextprotocol/go-sdk/blob/v1.7.0-pre.1/LICENSE)) - - [github.com/modelcontextprotocol/go-sdk](https://pkg.go.dev/github.com/modelcontextprotocol/go-sdk) ([MIT](https://github.com/modelcontextprotocol/go-sdk/blob/v1.7.0-pre.1/LICENSE)) + - [github.com/modelcontextprotocol/go-sdk](https://pkg.go.dev/github.com/modelcontextprotocol/go-sdk) ([Apache-2.0](https://github.com/modelcontextprotocol/go-sdk/blob/v1.7.0-pre.2/LICENSE)) + - [github.com/modelcontextprotocol/go-sdk](https://pkg.go.dev/github.com/modelcontextprotocol/go-sdk) ([MIT](https://github.com/modelcontextprotocol/go-sdk/blob/v1.7.0-pre.2/LICENSE)) - [github.com/muesli/cache2go](https://pkg.go.dev/github.com/muesli/cache2go) ([BSD-3-Clause](https://github.com/muesli/cache2go/blob/518229cd8021/LICENSE.txt)) - [github.com/pelletier/go-toml/v2](https://pkg.go.dev/github.com/pelletier/go-toml/v2) ([MIT](https://github.com/pelletier/go-toml/blob/v2.2.4/LICENSE)) - [github.com/sagikazarmark/locafero](https://pkg.go.dev/github.com/sagikazarmark/locafero) ([MIT](https://github.com/sagikazarmark/locafero/blob/v0.11.0/LICENSE)) From 1f463b66cd5fd380281a975105e64e2ec55ad72b Mon Sep 17 00:00:00 2001 From: Sam Morrow Date: Tue, 14 Jul 2026 00:18:45 +0200 Subject: [PATCH 2/3] fix(oauth): harden multi-round-trip authorization Move OAuth interception into tool-handler middleware so go-sdk finalizes multi-round-trip results with resultType input_required. Correlate responses to a per-flow ID, retire cancellations synchronously, and ignore late completions from stale flows. Also preserve actionable URLs for form-only clients and add wire-level, concurrency, and real manager lifecycle coverage. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 18e70efa-1b2d-4290-ba51-b82998db4ff8 --- internal/ghmcp/oauth.go | 154 +++++++++++++----------- internal/ghmcp/oauth_test.go | 173 ++++++++++++++++++++------- internal/ghmcp/server.go | 41 +++---- internal/oauth/manager.go | 188 ++++++++++++++++++++---------- internal/oauth/manager_test.go | 117 +++++++++++++++++++ pkg/github/server.go | 7 +- pkg/inventory/registry.go | 8 +- pkg/inventory/server_tool.go | 9 +- pkg/inventory/server_tool_test.go | 45 +++++++ 9 files changed, 544 insertions(+), 198 deletions(-) diff --git a/internal/ghmcp/oauth.go b/internal/ghmcp/oauth.go index 130ada1655..35e48f5bbc 100644 --- a/internal/ghmcp/oauth.go +++ b/internal/ghmcp/oauth.go @@ -3,10 +3,13 @@ package ghmcp import ( "context" "crypto/rand" + "errors" "fmt" "log/slog" + "strings" "github.com/github/github-mcp-server/internal/oauth" + "github.com/github/github-mcp-server/pkg/inventory" "github.com/modelcontextprotocol/go-sdk/mcp" ) @@ -91,14 +94,14 @@ func (p *sessionPrompter) PromptForm(ctx context.Context, prompt oauth.Prompt) e type oauthAuthenticator interface { HasToken() bool Authenticate(ctx context.Context, prompter oauth.Prompter) (*oauth.Outcome, error) - AwaitToken(ctx context.Context) (*oauth.Outcome, error) - Cancel() + AwaitToken(ctx context.Context, flowID string) (*oauth.Outcome, error) + Cancel(flowID string) bool } -// oauthElicitID is the stable key for the authorization elicitation in the -// multi-round-trip flow. The client echoes it back in InputResponses when it -// retries the tool call, so the middleware can recognize the user's response. -const oauthElicitID = "github_authorization" +// oauthElicitIDPrefix identifies authorization responses in the multi-round-trip +// InputResponses map. The suffix is the manager's per-flow ID, which prevents a +// delayed response from an older prompt from affecting a newer flow. +const oauthElicitIDPrefix = "github_authorization:" // protocolVersionNoServerElicitation is the first MCP protocol version that // forbids server-initiated JSON-RPC requests (SEP-2322): from this version on @@ -120,10 +123,11 @@ func serverMayInitiateElicitation(ss *mcp.ServerSession) bool { return params == nil || params.ProtocolVersion < protocolVersionNoServerElicitation } -// createOAuthMiddleware returns receiving middleware that authorizes the session -// lazily, on the first tool call. Authorization is deferred until here (rather -// than at startup) because the prompts depend on an initialized session whose -// elicitation capabilities and protocol version are known. +// createOAuthToolMiddleware returns tool-handler middleware that authorizes the +// session lazily, on the first tool call. It runs inside the SDK's +// Server.callTool handler so results returned here still receive SDK +// finalization, including resultType: "input_required" for multi-round-trip +// responses. // // When a token is already available the call proceeds untouched. Otherwise the // authorization flow runs, presenting its prompt over whichever channel the @@ -132,22 +136,22 @@ func serverMayInitiateElicitation(ss *mcp.ServerSession) bool { // forbidden (SEP-2322), it uses multi-round-trip elicitation returned from the // tool call. Either way the last-resort channel returns the instruction as a // tool result and asks the user to retry. -func createOAuthMiddleware(mgr oauthAuthenticator, logger *slog.Logger) func(next mcp.MethodHandler) mcp.MethodHandler { - return func(next mcp.MethodHandler) mcp.MethodHandler { - return func(ctx context.Context, method string, request mcp.Request) (mcp.Result, error) { - if method != "tools/call" || mgr.HasToken() { - return next(ctx, method, request) +func createOAuthToolMiddleware(mgr oauthAuthenticator, logger *slog.Logger) inventory.ToolHandlerMiddleware { + return func(next mcp.ToolHandler) mcp.ToolHandler { + return func(ctx context.Context, req *mcp.CallToolRequest) (*mcp.CallToolResult, error) { + if !serverMayInitiateElicitation(req.Session) { + if flowID, response, ok := authorizationElicitResponse(req.Params.InputResponses); ok { + return resumeMultiRoundTripAuthorization(ctx, mgr, next, req, flowID, response, logger) + } } - callReq, ok := request.(*mcp.CallToolRequest) - if !ok { - return next(ctx, method, request) + if mgr.HasToken() { + return next(ctx, req) } - - if serverMayInitiateElicitation(callReq.Session) { - return authorizeViaServerElicitation(ctx, mgr, next, method, request, callReq, logger) + if serverMayInitiateElicitation(req.Session) { + return authorizeViaServerElicitation(ctx, mgr, next, req, logger) } - return authorizeViaMultiRoundTrip(ctx, mgr, next, method, request, callReq, logger) + return startMultiRoundTripAuthorization(ctx, mgr, next, req, logger) } } } @@ -155,8 +159,8 @@ func createOAuthMiddleware(mgr oauthAuthenticator, logger *slog.Logger) func(nex // authorizeViaServerElicitation drives authorization on legacy protocol versions // (before 2026-07-28), where the server may present the prompt itself via // server-initiated elicitation. It blocks until the token arrives, then proceeds. -func authorizeViaServerElicitation(ctx context.Context, mgr oauthAuthenticator, next mcp.MethodHandler, method string, request mcp.Request, callReq *mcp.CallToolRequest, logger *slog.Logger) (mcp.Result, error) { - outcome, err := mgr.Authenticate(ctx, &sessionPrompter{session: callReq.Session}) +func authorizeViaServerElicitation(ctx context.Context, mgr oauthAuthenticator, next mcp.ToolHandler, req *mcp.CallToolRequest, logger *slog.Logger) (*mcp.CallToolResult, error) { + outcome, err := mgr.Authenticate(ctx, &sessionPrompter{session: req.Session}) if err != nil { return nil, fmt.Errorf("github authorization failed: %w", err) } @@ -166,46 +170,28 @@ func authorizeViaServerElicitation(ctx context.Context, mgr oauthAuthenticator, Content: []mcp.Content{&mcp.TextContent{Text: outcome.UserAction.Message}}, }, nil } - return next(ctx, method, request) + return next(ctx, req) } -// authorizeViaMultiRoundTrip drives authorization on protocol version 2026-07-28 -// and later, where server-initiated requests are forbidden (SEP-2322). The first -// tool call starts the flow and returns the authorization prompt as an -// elicitation input request; the client presents it and retries the call with -// the user's response, at which point we wait for the token and proceed. -func authorizeViaMultiRoundTrip(ctx context.Context, mgr oauthAuthenticator, next mcp.MethodHandler, method string, request mcp.Request, callReq *mcp.CallToolRequest, logger *slog.Logger) (mcp.Result, error) { - // Retry: the client fulfilled the authorization elicitation and re-sent the - // call with the user's response. - if resp, ok := callReq.Params.InputResponses[oauthElicitID]; ok { - res, _ := resp.(*mcp.ElicitResult) - if res == nil || res.Action != "accept" { - // The user declined or dismissed the prompt; tear the flow down so it - // does not linger, and let them retry when they are ready. - mgr.Cancel() - return &mcp.CallToolResult{ - Content: []mcp.Content{&mcp.TextContent{Text: "GitHub authorization was declined. Retry when you're ready to authorize."}}, - }, nil - } - outcome, err := mgr.AwaitToken(ctx) - if err != nil { - return nil, fmt.Errorf("github authorization failed: %w", err) +// authorizationElicitResponse finds the authorization response and extracts the +// flow ID encoded in its input-request key. +func authorizationElicitResponse(responses mcp.InputResponseMap) (string, *mcp.ElicitResult, bool) { + for id, response := range responses { + flowID, ok := strings.CutPrefix(id, oauthElicitIDPrefix) + if !ok || flowID == "" { + continue } - if outcome != nil && outcome.UserAction != nil { - // The user acknowledged the prompt but has not finished authorizing; - // surface the instructions so they can complete it and retry. - logger.Info("surfacing github authorization instructions to user") - return &mcp.CallToolResult{ - Content: []mcp.Content{&mcp.TextContent{Text: outcome.UserAction.Message}}, - }, nil - } - return next(ctx, method, request) + result, _ := response.(*mcp.ElicitResult) + return flowID, result, true } + return "", nil, false +} - // First attempt: start the flow. A nil prompter keeps the manager from - // initiating any elicitation itself (forbidden on this protocol); it opens a - // server-side browser when possible, otherwise returns the authorization - // instructions for us to present via multi-round-trip elicitation. +// startMultiRoundTripAuthorization starts authorization on protocol version +// 2026-07-28 or later. Server-initiated requests are forbidden there (SEP-2322), +// so the prompt is returned as an elicitation input request for the client to +// fulfill and retry. +func startMultiRoundTripAuthorization(ctx context.Context, mgr oauthAuthenticator, next mcp.ToolHandler, req *mcp.CallToolRequest, logger *slog.Logger) (*mcp.CallToolResult, error) { outcome, err := mgr.Authenticate(ctx, nil) if err != nil { return nil, fmt.Errorf("github authorization failed: %w", err) @@ -213,13 +199,14 @@ func authorizeViaMultiRoundTrip(ctx context.Context, mgr oauthAuthenticator, nex if outcome == nil || outcome.UserAction == nil { // Already authorized (e.g. the server opened a browser and the flow // completed); proceed. - return next(ctx, method, request) + return next(ctx, req) } - elicit := authorizationElicitParams(outcome.UserAction, &sessionPrompter{session: callReq.Session}) - if elicit == nil { + elicit := authorizationElicitParams(outcome.UserAction, &sessionPrompter{session: req.Session}) + if elicit == nil || outcome.FlowID == "" { // The client cannot present an elicitation (no capability, or no URL to - // show); fall back to returning the instructions as a tool result. + // show), or the flow cannot be correlated; fall back to returning the + // instructions as a tool result. logger.Info("surfacing github authorization instructions to user") return &mcp.CallToolResult{ Content: []mcp.Content{&mcp.TextContent{Text: outcome.UserAction.Message}}, @@ -227,11 +214,46 @@ func authorizeViaMultiRoundTrip(ctx context.Context, mgr oauthAuthenticator, nex } logger.Info("requesting github authorization via elicitation") return &mcp.CallToolResult{ - InputRequests: mcp.InputRequestMap{oauthElicitID: elicit}, - RequestState: "github-authorization-pending", + InputRequests: mcp.InputRequestMap{oauthElicitIDPrefix + outcome.FlowID: elicit}, }, nil } +// resumeMultiRoundTripAuthorization handles the client's retry after it +// fulfilled the authorization elicitation. +func resumeMultiRoundTripAuthorization(ctx context.Context, mgr oauthAuthenticator, next mcp.ToolHandler, req *mcp.CallToolRequest, flowID string, response *mcp.ElicitResult, logger *slog.Logger) (*mcp.CallToolResult, error) { + if response == nil || response.Action != "accept" { + if !mgr.Cancel(flowID) { + return expiredAuthorizationResult(), nil + } + return &mcp.CallToolResult{ + Content: []mcp.Content{&mcp.TextContent{Text: "GitHub authorization was declined. Retry when you're ready to authorize."}}, + }, nil + } + + outcome, err := mgr.AwaitToken(ctx, flowID) + if errors.Is(err, oauth.ErrStaleAuthorizationFlow) { + return expiredAuthorizationResult(), nil + } + if err != nil { + return nil, fmt.Errorf("github authorization failed: %w", err) + } + if outcome != nil && outcome.UserAction != nil { + // The user acknowledged the prompt but has not finished authorizing; + // surface the instructions so they can complete it and retry. + logger.Info("surfacing github authorization instructions to user") + return &mcp.CallToolResult{ + Content: []mcp.Content{&mcp.TextContent{Text: outcome.UserAction.Message}}, + }, nil + } + return next(ctx, req) +} + +func expiredAuthorizationResult() *mcp.CallToolResult { + return &mcp.CallToolResult{ + Content: []mcp.Content{&mcp.TextContent{Text: "This GitHub authorization prompt has expired. Retry the request to authorize again."}}, + } +} + // authorizationElicitParams builds the elicitation that presents the // authorization instructions to the user. It mirrors sessionPrompter's channel // selection: URL-mode when the client supports it, otherwise form-mode. It @@ -250,7 +272,7 @@ func authorizationElicitParams(ua *oauth.UserAction, p *sessionPrompter) *mcp.El case p.CanPromptURL(): return &mcp.ElicitParams{Mode: "url", Message: message, URL: ua.URL, ElicitationID: rand.Text()} case p.CanPromptForm(): - return &mcp.ElicitParams{Mode: "form", Message: message} + return &mcp.ElicitParams{Mode: "form", Message: ua.Message} default: return nil } diff --git a/internal/ghmcp/oauth_test.go b/internal/ghmcp/oauth_test.go index 5dbba67ade..ca9b6177ac 100644 --- a/internal/ghmcp/oauth_test.go +++ b/internal/ghmcp/oauth_test.go @@ -13,6 +13,7 @@ import ( "github.com/github/github-mcp-server/pkg/github" "github.com/github/github-mcp-server/pkg/http/headers" "github.com/github/github-mcp-server/pkg/utils" + "github.com/google/jsonschema-go/jsonschema" "github.com/modelcontextprotocol/go-sdk/mcp" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -243,11 +244,14 @@ type fakeAuthenticator struct { // awaitOutcome/awaitErr are returned by AwaitToken; tokenAfterAwait flips // HasToken to true once AwaitToken is called, simulating a flow that // acquires the token while the user acts on the elicitation. - awaitOutcome *oauth.Outcome - awaitErr error - tokenAfterAwait bool - awaitCalls int - cancelCalls int + awaitOutcome *oauth.Outcome + awaitErr error + tokenAfterAwait bool + awaitCalls int + cancelCalls int + cancelResult bool + lastAwaitFlowID string + lastCancelFlowID string } func (f *fakeAuthenticator) HasToken() bool { return f.hasToken } @@ -258,44 +262,38 @@ func (f *fakeAuthenticator) Authenticate(_ context.Context, prompter oauth.Promp return f.outcome, f.err } -func (f *fakeAuthenticator) AwaitToken(context.Context) (*oauth.Outcome, error) { +func (f *fakeAuthenticator) AwaitToken(_ context.Context, flowID string) (*oauth.Outcome, error) { f.awaitCalls++ + f.lastAwaitFlowID = flowID if f.tokenAfterAwait { f.hasToken = true } return f.awaitOutcome, f.awaitErr } -func (f *fakeAuthenticator) Cancel() { f.cancelCalls++ } +func (f *fakeAuthenticator) Cancel(flowID string) bool { + f.cancelCalls++ + f.lastCancelFlowID = flowID + return f.cancelResult +} -func TestCreateOAuthMiddleware(t *testing.T) { +func TestCreateOAuthToolMiddleware(t *testing.T) { t.Parallel() const nextText = "handler-ran" - newNext := func(called *bool) mcp.MethodHandler { - return func(_ context.Context, _ string, _ mcp.Request) (mcp.Result, error) { + newNext := func(called *bool) mcp.ToolHandler { + return func(_ context.Context, _ *mcp.CallToolRequest) (*mcp.CallToolResult, error) { *called = true return &mcp.CallToolResult{Content: []mcp.Content{&mcp.TextContent{Text: nextText}}}, nil } } - t.Run("non tool call passes through without authenticating", func(t *testing.T) { - t.Parallel() - fake := &fakeAuthenticator{hasToken: false} - var called bool - mw := createOAuthMiddleware(fake, discardLogger()) - _, err := mw(newNext(&called))(context.Background(), "initialize", &mcp.InitializeRequest{}) - require.NoError(t, err) - assert.True(t, called, "next should run") - assert.Zero(t, fake.authCalls, "authentication must not run for non tool calls") - }) - t.Run("existing token short circuits authentication", func(t *testing.T) { t.Parallel() fake := &fakeAuthenticator{hasToken: true} var called bool - mw := createOAuthMiddleware(fake, discardLogger()) - _, err := mw(newNext(&called))(context.Background(), "tools/call", &mcp.CallToolRequest{}) + mw := createOAuthToolMiddleware(fake, discardLogger()) + _, err := mw(newNext(&called))(context.Background(), &mcp.CallToolRequest{}) require.NoError(t, err) assert.True(t, called, "next should run") assert.Zero(t, fake.authCalls, "authentication must be skipped when a token already exists") @@ -305,15 +303,13 @@ func TestCreateOAuthMiddleware(t *testing.T) { t.Parallel() fake := &fakeAuthenticator{hasToken: false, outcome: nil, err: nil} var called bool - mw := createOAuthMiddleware(fake, discardLogger()) - res, err := mw(newNext(&called))(context.Background(), "tools/call", &mcp.CallToolRequest{}) + mw := createOAuthToolMiddleware(fake, discardLogger()) + res, err := mw(newNext(&called))(context.Background(), &mcp.CallToolRequest{}) require.NoError(t, err) assert.Equal(t, 1, fake.authCalls) assert.True(t, called, "next should run once authorized") - callRes, ok := res.(*mcp.CallToolResult) - require.True(t, ok) - require.Len(t, callRes.Content, 1) - assert.Equal(t, nextText, callRes.Content[0].(*mcp.TextContent).Text) + require.Len(t, res.Content, 1) + assert.Equal(t, nextText, res.Content[0].(*mcp.TextContent).Text) }) t.Run("pending user action is surfaced as a tool result", func(t *testing.T) { @@ -321,22 +317,20 @@ func TestCreateOAuthMiddleware(t *testing.T) { const message = "Open https://example.com/auth to authorize, then retry." fake := &fakeAuthenticator{hasToken: false, outcome: &oauth.Outcome{UserAction: &oauth.UserAction{Message: message}}} var called bool - mw := createOAuthMiddleware(fake, discardLogger()) - res, err := mw(newNext(&called))(context.Background(), "tools/call", &mcp.CallToolRequest{}) + mw := createOAuthToolMiddleware(fake, discardLogger()) + res, err := mw(newNext(&called))(context.Background(), &mcp.CallToolRequest{}) require.NoError(t, err) assert.False(t, called, "next must not run while the user still needs to authorize") - callRes, ok := res.(*mcp.CallToolResult) - require.True(t, ok) - require.Len(t, callRes.Content, 1) - assert.Equal(t, message, callRes.Content[0].(*mcp.TextContent).Text) + require.Len(t, res.Content, 1) + assert.Equal(t, message, res.Content[0].(*mcp.TextContent).Text) }) t.Run("authentication error is returned", func(t *testing.T) { t.Parallel() fake := &fakeAuthenticator{hasToken: false, err: assert.AnError} var called bool - mw := createOAuthMiddleware(fake, discardLogger()) - _, err := mw(newNext(&called))(context.Background(), "tools/call", &mcp.CallToolRequest{}) + mw := createOAuthToolMiddleware(fake, discardLogger()) + _, err := mw(newNext(&called))(context.Background(), &mcp.CallToolRequest{}) require.Error(t, err) assert.ErrorIs(t, err, assert.AnError) assert.False(t, called, "next must not run when authentication fails") @@ -358,11 +352,14 @@ func runOAuthMiddlewareCall( server := mcp.NewServer(&mcp.Implementation{Name: "test-server", Version: "v0.0.1"}, nil) var toolRan bool - mcp.AddTool(server, &mcp.Tool{Name: probeToolName}, func(_ context.Context, _ *mcp.CallToolRequest, _ struct{}) (*mcp.CallToolResult, any, error) { + handler := createOAuthToolMiddleware(fake, discardLogger())(func(_ context.Context, _ *mcp.CallToolRequest) (*mcp.CallToolResult, error) { toolRan = true - return &mcp.CallToolResult{Content: []mcp.Content{&mcp.TextContent{Text: "tool-ran"}}}, nil, nil + return &mcp.CallToolResult{Content: []mcp.Content{&mcp.TextContent{Text: "tool-ran"}}}, nil }) - server.AddReceivingMiddleware(createOAuthMiddleware(fake, discardLogger())) + server.AddTool(&mcp.Tool{ + Name: probeToolName, + InputSchema: &jsonschema.Schema{Type: "object"}, + }, handler) st, ct := mcp.NewInMemoryTransports() @@ -397,7 +394,7 @@ func TestOAuthMiddlewareMultiRoundTrip(t *testing.T) { t.Run("accepted elicitation authorizes and proceeds", func(t *testing.T) { t.Parallel() fake := &fakeAuthenticator{ - outcome: &oauth.Outcome{UserAction: &oauth.UserAction{URL: "https://example.com/auth", Message: "manual"}}, + outcome: &oauth.Outcome{UserAction: &oauth.UserAction{URL: "https://example.com/auth", Message: "manual"}, FlowID: "flow-1"}, tokenAfterAwait: true, } var elicited int @@ -412,6 +409,7 @@ func TestOAuthMiddlewareMultiRoundTrip(t *testing.T) { assert.True(t, toolRan) assert.Equal(t, 1, elicited, "the client should be asked to authorize exactly once") assert.Equal(t, 1, fake.awaitCalls, "the middleware should await the token on retry") + assert.Equal(t, "flow-1", fake.lastAwaitFlowID) assert.Zero(t, fake.cancelCalls) assert.Nil(t, fake.lastPrompter, "the manager must not be given a prompter on this protocol") }) @@ -419,7 +417,8 @@ func TestOAuthMiddlewareMultiRoundTrip(t *testing.T) { t.Run("declined elicitation cancels and does not run the tool", func(t *testing.T) { t.Parallel() fake := &fakeAuthenticator{ - outcome: &oauth.Outcome{UserAction: &oauth.UserAction{URL: "https://example.com/auth", Message: "manual"}}, + outcome: &oauth.Outcome{UserAction: &oauth.UserAction{URL: "https://example.com/auth", Message: "manual"}, FlowID: "flow-1"}, + cancelResult: true, } decline := func(_ context.Context, _ *mcp.ElicitRequest) (*mcp.ElicitResult, error) { return &mcp.ElicitResult{Action: "decline"}, nil @@ -430,14 +429,62 @@ func TestOAuthMiddlewareMultiRoundTrip(t *testing.T) { assert.False(t, toolRan, "the tool must not run when authorization is declined") assert.Contains(t, text, "declined") assert.Equal(t, 1, fake.cancelCalls, "a decline should cancel the in-flight flow") + assert.Equal(t, "flow-1", fake.lastCancelFlowID) assert.Zero(t, fake.awaitCalls) }) + t.Run("stale decline does not cancel the current flow", func(t *testing.T) { + t.Parallel() + fake := &fakeAuthenticator{ + outcome: &oauth.Outcome{UserAction: &oauth.UserAction{URL: "https://example.com/auth", Message: "manual"}, FlowID: "old-flow"}, + } + decline := func(_ context.Context, _ *mcp.ElicitRequest) (*mcp.ElicitResult, error) { + return &mcp.ElicitResult{Action: "decline"}, nil + } + + text, toolRan := runOAuthMiddlewareCall(t, fake, urlCaps, decline) + + assert.False(t, toolRan) + assert.Contains(t, text, "expired") + assert.Equal(t, "old-flow", fake.lastCancelFlowID) + assert.Zero(t, fake.awaitCalls) + }) + + t.Run("form-only client receives actionable instructions", func(t *testing.T) { + t.Parallel() + const ( + authURL = "https://example.com/auth" + message = "Open https://example.com/auth and enter code ABCD-1234." + ) + fake := &fakeAuthenticator{ + outcome: &oauth.Outcome{ + UserAction: &oauth.UserAction{URL: authURL, UserCode: "ABCD-1234", Message: message}, + FlowID: "flow-1", + }, + tokenAfterAwait: true, + } + var elicited *mcp.ElicitParams + accept := func(_ context.Context, req *mcp.ElicitRequest) (*mcp.ElicitResult, error) { + elicited = req.Params + return &mcp.ElicitResult{Action: "accept"}, nil + } + formCaps := &mcp.ClientCapabilities{Elicitation: &mcp.ElicitationCapabilities{Form: &mcp.FormElicitationCapabilities{}}} + + text, toolRan := runOAuthMiddlewareCall(t, fake, formCaps, accept) + + assert.True(t, toolRan) + assert.Equal(t, "tool-ran", text) + require.NotNil(t, elicited) + assert.Equal(t, "form", elicited.Mode) + assert.Contains(t, elicited.Message, authURL) + assert.Contains(t, elicited.Message, "ABCD-1234") + }) + t.Run("no elicitation capability falls back to a tool-result message", func(t *testing.T) { t.Parallel() const message = "Open https://example.com/auth to authorize, then retry." fake := &fakeAuthenticator{ - outcome: &oauth.Outcome{UserAction: &oauth.UserAction{URL: "https://example.com/auth", Message: message}}, + outcome: &oauth.Outcome{UserAction: &oauth.UserAction{URL: "https://example.com/auth", Message: message}, FlowID: "flow-1"}, } // No elicitation capability advertised, and no handler needed since the @@ -451,6 +498,46 @@ func TestOAuthMiddlewareMultiRoundTrip(t *testing.T) { }) } +func TestOAuthMultiRoundTripResultType(t *testing.T) { + t.Parallel() + + fake := &fakeAuthenticator{ + outcome: &oauth.Outcome{ + UserAction: &oauth.UserAction{URL: "https://example.com/auth", Message: "manual"}, + FlowID: "flow-1", + }, + } + server := mcp.NewServer(&mcp.Implementation{Name: "test-server", Version: "v0.0.1"}, nil) + var toolRan bool + handler := createOAuthToolMiddleware(fake, discardLogger())(func(_ context.Context, _ *mcp.CallToolRequest) (*mcp.CallToolResult, error) { + toolRan = true + return &mcp.CallToolResult{Content: []mcp.Content{&mcp.TextContent{Text: "tool-ran"}}}, nil + }) + server.AddTool(&mcp.Tool{ + Name: probeToolName, + InputSchema: &jsonschema.Schema{Type: "object"}, + }, handler) + + st, ct := mcp.NewInMemoryTransports() + ss, err := server.Connect(context.Background(), st, nil) + require.NoError(t, err) + t.Cleanup(func() { _ = ss.Close() }) + + client := mcp.NewClient(&mcp.Implementation{Name: "test-client", Version: "v0.0.1"}, &mcp.ClientOptions{ + Capabilities: &mcp.ClientCapabilities{Elicitation: &mcp.ElicitationCapabilities{URL: &mcp.URLElicitationCapabilities{}}}, + MultiRoundTrip: &mcp.MultiRoundTripOptions{Disabled: true}, + }) + cs, err := client.Connect(context.Background(), ct, nil) + require.NoError(t, err) + t.Cleanup(func() { _ = cs.Close() }) + + res, err := cs.CallTool(context.Background(), &mcp.CallToolParams{Name: probeToolName}) + require.NoError(t, err) + assert.True(t, res.NeedsInput(), "the wire response must declare resultType input_required") + assert.Contains(t, res.InputRequests, oauthElicitIDPrefix+"flow-1") + assert.False(t, toolRan) +} + // TestRunStdioServerRejectsTokenAndOAuth verifies the mutually-exclusive guard: // supplying both a static token and an OAuth manager is rejected before the // server starts, rather than silently preferring one for auth and the other for diff --git a/internal/ghmcp/server.go b/internal/ghmcp/server.go index 2267dd5d62..1e0611d648 100644 --- a/internal/ghmcp/server.go +++ b/internal/ghmcp/server.go @@ -314,38 +314,35 @@ func RunStdioServer(cfg StdioServerConfig) error { // For OAuth, the token is resolved lazily: empty until the user authorizes // on the first tool call, then refreshed for the rest of the session. var tokenProvider func() string + var toolHandlerMiddleware []inventory.ToolHandlerMiddleware if cfg.OAuthManager != nil { tokenProvider = cfg.OAuthManager.AccessToken + toolHandlerMiddleware = append(toolHandlerMiddleware, createOAuthToolMiddleware(cfg.OAuthManager, logger)) } ghServer, err := NewStdioMCPServer(ctx, github.MCPServerConfig{ - Version: cfg.Version, - Host: cfg.Host, - Token: cfg.Token, - EnabledToolsets: cfg.EnabledToolsets, - EnabledTools: cfg.EnabledTools, - EnabledFeatures: cfg.EnabledFeatures, - ReadOnly: cfg.ReadOnly, - Translator: t, - ContentWindowSize: cfg.ContentWindowSize, - LockdownMode: cfg.LockdownMode, - InsidersMode: cfg.InsidersMode, - ExcludeTools: cfg.ExcludeTools, - Logger: logger, - RepoAccessTTL: cfg.RepoAccessCacheTTL, - TokenScopes: tokenScopes, - TokenProvider: tokenProvider, + Version: cfg.Version, + Host: cfg.Host, + Token: cfg.Token, + EnabledToolsets: cfg.EnabledToolsets, + EnabledTools: cfg.EnabledTools, + EnabledFeatures: cfg.EnabledFeatures, + ReadOnly: cfg.ReadOnly, + Translator: t, + ContentWindowSize: cfg.ContentWindowSize, + LockdownMode: cfg.LockdownMode, + InsidersMode: cfg.InsidersMode, + ExcludeTools: cfg.ExcludeTools, + Logger: logger, + RepoAccessTTL: cfg.RepoAccessCacheTTL, + TokenScopes: tokenScopes, + TokenProvider: tokenProvider, + ToolHandlerMiddleware: toolHandlerMiddleware, }) if err != nil { return fmt.Errorf("failed to create MCP server: %w", err) } - // With OAuth, intercept tool calls to run the authorization flow on first - // use, before the handler tries to call GitHub with an empty token. - if cfg.OAuthManager != nil { - ghServer.AddReceivingMiddleware(createOAuthMiddleware(cfg.OAuthManager, logger)) - } - if cfg.ExportTranslations { // Once server is initialized, all translations are loaded dumpTranslations() diff --git a/internal/oauth/manager.go b/internal/oauth/manager.go index 5ca31f831c..8c16729f5b 100644 --- a/internal/oauth/manager.go +++ b/internal/oauth/manager.go @@ -2,6 +2,7 @@ package oauth import ( "context" + "crypto/rand" "errors" "log/slog" "net/http" @@ -20,6 +21,10 @@ const DefaultAuthTimeout = 5 * time.Minute // stalled GitHub token endpoint cannot block a tool call indefinitely. const tokenRefreshTimeout = 30 * time.Second +// ErrStaleAuthorizationFlow indicates that a prompt response belongs to an +// authorization flow that is no longer current. +var ErrStaleAuthorizationFlow = errors.New("authorization prompt has expired") + // flowStatus tracks the manager's single-flight authorization state. type flowStatus int @@ -37,6 +42,11 @@ type Outcome struct { // flow continues in the background; the user should retry once they have // completed it. UserAction *UserAction + + // FlowID correlates a user action with the authorization flow that produced + // it. Callers must pass it back to AwaitToken or Cancel so a delayed response + // cannot affect a newer flow. + FlowID string } // UserAction is an instruction for the user to complete authorization out of @@ -65,7 +75,9 @@ type Manager struct { mu sync.Mutex source oauth2.TokenSource // refreshing source, set once authorized + tokenGeneration uint64 // increments whenever source is replaced status flowStatus + flowID string pending *UserAction done chan struct{} cancelFlow context.CancelFunc // cancels the in-flight flow, if any @@ -94,11 +106,21 @@ func NewManager(cfg Config, logger *slog.Logger) *Manager { // re-authorization is required). It is cheap to call repeatedly: the underlying // token source caches and only refreshes when the token has expired. func (m *Manager) AccessToken() string { + token, _ := m.accessToken() + return token +} + +// accessToken returns the token together with the generation of the source it +// checked. Authenticate uses the generation to detect a source installed while +// token validation was in progress, without repeating a potentially blocking +// refresh request. +func (m *Manager) accessToken() (string, uint64) { m.mu.Lock() src := m.source + generation := m.tokenGeneration m.mu.Unlock() if src == nil { - return "" + return "", generation } // Refresh (if needed) happens here, off the lock, because ReuseTokenSource may // make a blocking network call and holding m.mu would serialize every tool call. @@ -110,17 +132,17 @@ func (m *Manager) AccessToken() string { // prompt. The oauth2 error carries the token endpoint's response, not the // access or refresh token. m.mu.Lock() - if !m.refreshErrLogged { + if m.tokenGeneration == generation && !m.refreshErrLogged { m.refreshErrLogged = true m.logger.Warn("OAuth token refresh failed; re-authorization required", "error", err) } m.mu.Unlock() - return "" + return "", generation } if !tok.Valid() { - return "" + return "", generation } - return tok.AccessToken + return tok.AccessToken, generation } // HasToken reports whether a valid token is currently available. @@ -138,59 +160,79 @@ func (m *Manager) HasToken() bool { // Only one flow runs at a time. Concurrent callers either join a running secure // flow, receive the pending user action, or are told to retry shortly. func (m *Manager) Authenticate(ctx context.Context, prompter Prompter) (*Outcome, error) { - if m.AccessToken() != "" { - return nil, nil - } + var flowID string + var done chan struct{} + for { + token, checkedTokenGeneration := m.accessToken() + if token != "" { + return nil, nil + } - m.mu.Lock() - switch m.status { - case statusAwaitingUser: - ua := m.pending - m.mu.Unlock() - return &Outcome{UserAction: ua}, nil - case statusStarting: - m.mu.Unlock() - return &Outcome{UserAction: &UserAction{ - Message: "GitHub authorization is already in progress. Please retry your request in a few seconds.", - }}, nil - case statusInProgress: - done := m.done + m.mu.Lock() + switch m.status { + case statusAwaitingUser: + ua := m.pending + flowID := m.flowID + m.mu.Unlock() + return &Outcome{UserAction: ua, FlowID: flowID}, nil + case statusStarting: + flowID := m.flowID + m.mu.Unlock() + return &Outcome{UserAction: &UserAction{ + Message: "GitHub authorization is already in progress. Please retry your request in a few seconds.", + }, FlowID: flowID}, nil + case statusInProgress: + done := m.done + flowID := m.flowID + m.mu.Unlock() + return m.joinWait(ctx, done, flowID) + } + + // A flow may have installed a token while the source above was being + // checked. Retry if the source changed before claiming the idle state. + if m.tokenGeneration != checkedTokenGeneration { + m.mu.Unlock() + continue + } + + // Idle: this call owns the new flow. + m.status = statusStarting + m.flowID = rand.Text() + flowID = m.flowID + m.lastErr = nil + m.done = make(chan struct{}) + done = m.done m.mu.Unlock() - return m.joinWait(ctx, done) + break } - // Idle: this call owns the new flow. - m.status = statusStarting - m.lastErr = nil - m.done = make(chan struct{}) - done := m.done - m.mu.Unlock() - plan, err := m.begin(prompter) if err != nil { - m.complete(nil, err) + m.complete(flowID, nil, err) return nil, err } + bgCtx, cancel := context.WithTimeout(context.Background(), DefaultAuthTimeout) m.mu.Lock() + if m.flowID != flowID { + m.mu.Unlock() + cancel() + return nil, ErrStaleAuthorizationFlow + } if plan.userAction != nil { m.status = statusAwaitingUser m.pending = plan.userAction } else { m.status = statusInProgress } - m.mu.Unlock() - - bgCtx, cancel := context.WithTimeout(context.Background(), DefaultAuthTimeout) - m.mu.Lock() m.cancelFlow = cancel m.mu.Unlock() - go m.runFlow(bgCtx, cancel, plan) + go m.runFlow(bgCtx, cancel, flowID, plan) if plan.userAction != nil { - return &Outcome{UserAction: plan.userAction}, nil + return &Outcome{UserAction: plan.userAction, FlowID: flowID}, nil } - return m.joinWait(ctx, done) + return m.joinWait(ctx, done, flowID) } // AwaitToken blocks until the in-flight authorization flow yields a token, the @@ -201,43 +243,59 @@ func (m *Manager) Authenticate(ctx context.Context, prompter Prompter) (*Outcome // // It returns (nil, nil) once a token is available (proceed), (&Outcome{UserAction}, // nil) when the user must still act out of band, or (nil, err) on failure. -func (m *Manager) AwaitToken(ctx context.Context) (*Outcome, error) { - if m.AccessToken() != "" { - return nil, nil - } +func (m *Manager) AwaitToken(ctx context.Context, flowID string) (*Outcome, error) { m.mu.Lock() + if flowID == "" || flowID != m.flowID { + m.mu.Unlock() + return nil, ErrStaleAuthorizationFlow + } done := m.done m.mu.Unlock() + if m.AccessToken() != "" { + return nil, nil + } if done == nil { // No flow is in flight; report whatever terminal state it left behind. - return m.outcomeAfterFlow() + return m.outcomeAfterFlow(flowID) } select { case <-done: - return m.outcomeAfterFlow() + return m.outcomeAfterFlow(flowID) case <-ctx.Done(): return nil, ctx.Err() } } -// Cancel aborts the in-flight authorization flow, if any. It is used when the -// user declines the authorization prompt so the background flow (callback -// listener or device poll) is torn down promptly rather than lingering until it -// times out. It is a no-op when no flow is running. -func (m *Manager) Cancel() { +// Cancel retires the matching authorization flow and aborts its background +// callback listener or device poll. It returns false if flowID is stale. +func (m *Manager) Cancel(flowID string) bool { m.mu.Lock() + if flowID == "" || flowID != m.flowID { + m.mu.Unlock() + return false + } cancel := m.cancelFlow + m.status = statusIdle + m.flowID = "" + m.pending = nil + m.cancelFlow = nil + m.lastErr = context.Canceled + if m.done != nil { + close(m.done) + m.done = nil + } m.mu.Unlock() if cancel != nil { cancel() } + return true } // runFlow executes a prepared flow in the background and records the result. The // optional display prompt runs concurrently: a decline (or other failure) aborts // the flow, while an undeliverable prompt degrades to the manual fallback without // tearing the flow down, so the user can still authorize out of band. -func (m *Manager) runFlow(ctx context.Context, cancel context.CancelFunc, plan *flowPlan) { +func (m *Manager) runFlow(ctx context.Context, cancel context.CancelFunc, flowID string, plan *flowPlan) { defer cancel() if plan.display != nil { @@ -256,7 +314,7 @@ func (m *Manager) runFlow(ctx context.Context, cancel context.CancelFunc, plan * // prompt. Surface the manual instructions instead of failing, and // keep the background flow alive so the user can still authorize. m.logger.Debug("authorization prompt undeliverable; falling back to manual instructions", "reason", err) - m.fallBackToUserAction(plan.fallback) + m.fallBackToUserAction(flowID, plan.fallback) default: // A user decline (ErrPromptDeclined) or any other prompt failure // ends the flow. @@ -267,17 +325,17 @@ func (m *Manager) runFlow(ctx context.Context, cancel context.CancelFunc, plan * } tok, err := plan.run(ctx) - m.complete(tok, err) + m.complete(flowID, tok, err) } // fallBackToUserAction promotes a running secure flow to the manual user-action // channel after its prompt could not be delivered. The background flow keeps // running, so the user can complete authorization out of band and retry. It is a // no-op if the flow has already resolved. -func (m *Manager) fallBackToUserAction(ua *UserAction) { +func (m *Manager) fallBackToUserAction(flowID string, ua *UserAction) { m.mu.Lock() defer m.mu.Unlock() - if m.status != statusInProgress { + if m.flowID != flowID || m.status != statusInProgress { return } m.status = statusAwaitingUser @@ -292,9 +350,12 @@ func (m *Manager) fallBackToUserAction(ua *UserAction) { // complete records the flow result, installing a refreshing token source on // success, and wakes any joined callers. -func (m *Manager) complete(tok *oauth2.Token, err error) { +func (m *Manager) complete(flowID string, tok *oauth2.Token, err error) { m.mu.Lock() defer m.mu.Unlock() + if m.flowID != flowID { + return + } m.status = statusIdle m.pending = nil @@ -310,6 +371,7 @@ func (m *Manager) complete(tok *oauth2.Token, err error) { // client so a stalled token endpoint can't block a tool call forever. refreshCtx := context.WithValue(context.Background(), oauth2.HTTPClient, &http.Client{Timeout: tokenRefreshTimeout}) m.source = m.refreshConfig.TokenSource(refreshCtx, tok) + m.tokenGeneration++ m.refreshErrLogged = false m.logger.Info("github authorization complete") } @@ -322,10 +384,10 @@ func (m *Manager) complete(tok *oauth2.Token, err error) { // joinWait blocks until the running flow finishes or ctx is cancelled. If the // flow was promoted to the manual channel while waiting (its prompt could not be // delivered), it returns that user action rather than an error. -func (m *Manager) joinWait(ctx context.Context, done chan struct{}) (*Outcome, error) { +func (m *Manager) joinWait(ctx context.Context, done chan struct{}, flowID string) (*Outcome, error) { select { case <-done: - return m.outcomeAfterFlow() + return m.outcomeAfterFlow(flowID) case <-ctx.Done(): return nil, ctx.Err() } @@ -334,16 +396,20 @@ func (m *Manager) joinWait(ctx context.Context, done chan struct{}) (*Outcome, e // outcomeAfterFlow reports the result once the flow's done channel has closed // (or when there is no flow in flight): a token to proceed (nil, nil), a pending // user action to surface, or the flow's error. -func (m *Manager) outcomeAfterFlow() (*Outcome, error) { - if m.AccessToken() != "" { - return nil, nil - } +func (m *Manager) outcomeAfterFlow(flowID string) (*Outcome, error) { m.mu.Lock() + if flowID == "" || flowID != m.flowID { + m.mu.Unlock() + return nil, ErrStaleAuthorizationFlow + } pending := m.pending err := m.lastErr m.mu.Unlock() + if m.AccessToken() != "" { + return nil, nil + } if pending != nil { - return &Outcome{UserAction: pending}, nil + return &Outcome{UserAction: pending, FlowID: flowID}, nil } if err != nil { return nil, err diff --git a/internal/oauth/manager_test.go b/internal/oauth/manager_test.go index 6f43c03ef9..52d5a54309 100644 --- a/internal/oauth/manager_test.go +++ b/internal/oauth/manager_test.go @@ -11,6 +11,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "golang.org/x/oauth2" ) // newManager wires a Manager to the fake GitHub server. By default the browser @@ -167,6 +168,7 @@ func TestAuthenticateLastDitchUserAction(t *testing.T) { require.NoError(t, err) require.NotNil(t, out) require.NotNil(t, out.UserAction) + require.NotEmpty(t, out.FlowID) assert.NotEmpty(t, out.UserAction.URL) assert.Contains(t, out.UserAction.Message, "open this URL") assert.Contains(t, out.UserAction.Message, securityAdvisory, @@ -178,12 +180,127 @@ func TestAuthenticateLastDitchUserAction(t *testing.T) { require.NoError(t, err) require.NotNil(t, out2.UserAction) assert.Equal(t, out.UserAction.URL, out2.UserAction.URL) + assert.Equal(t, out.FlowID, out2.FlowID) // The user opens the URL out of band; the background flow then completes. require.NoError(t, browserGet(out.UserAction.URL)) assert.Equal(t, "gho_access", waitForToken(t, m)) } +func TestAwaitTokenCompletesCurrentFlow(t *testing.T) { + f := newFakeGitHub(t) + m := newManager(t, f) + m.openURL = func(string) error { return errors.New("no browser") } + + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + + out, err := m.Authenticate(ctx, nil) + require.NoError(t, err) + require.NotNil(t, out) + require.NotNil(t, out.UserAction) + require.NotEmpty(t, out.FlowID) + + authDone := make(chan error, 1) + go func() { + authDone <- browserGet(out.UserAction.URL) + }() + + awaited, err := m.AwaitToken(ctx, out.FlowID) + require.NoError(t, err) + assert.Nil(t, awaited) + require.NoError(t, <-authDone) + assert.Equal(t, "gho_access", m.AccessToken()) +} + +func TestCancelAndAwaitTokenAreFlowScoped(t *testing.T) { + f := newFakeGitHub(t) + m := newManager(t, f) + m.openURL = func(string) error { return errors.New("no browser") } + + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + + first, err := m.Authenticate(ctx, nil) + require.NoError(t, err) + require.NotNil(t, first) + require.NotEmpty(t, first.FlowID) + + assert.True(t, m.Cancel(first.FlowID), "the current flow should be cancelled") + second, err := m.Authenticate(ctx, nil) + require.NoError(t, err) + require.NotNil(t, second) + require.NotNil(t, second.UserAction) + require.NotEmpty(t, second.FlowID) + require.NotEqual(t, first.FlowID, second.FlowID) + + assert.False(t, m.Cancel(first.FlowID), "a stale decline must not cancel the newer flow") + _, err = m.AwaitToken(ctx, first.FlowID) + assert.ErrorIs(t, err, ErrStaleAuthorizationFlow) + + authDone := make(chan error, 1) + go func() { + authDone <- browserGet(second.UserAction.URL) + }() + awaited, err := m.AwaitToken(ctx, second.FlowID) + require.NoError(t, err) + assert.Nil(t, awaited) + require.NoError(t, <-authDone) + assert.Equal(t, "gho_access", m.AccessToken()) +} + +type blockingTokenSource struct { + entered chan struct{} + release chan struct{} + once sync.Once +} + +func (s *blockingTokenSource) Token() (*oauth2.Token, error) { + s.once.Do(func() { close(s.entered) }) + <-s.release + return nil, errors.New("stale token source failed") +} + +func TestAuthenticateRechecksTokenBeforeStartingFlow(t *testing.T) { + f := newFakeGitHub(t) + m := newManager(t, f) + staleSource := &blockingTokenSource{ + entered: make(chan struct{}), + release: make(chan struct{}), + } + + m.mu.Lock() + m.source = staleSource + m.status = statusInProgress + m.flowID = "existing-flow" + m.done = make(chan struct{}) + m.mu.Unlock() + + type result struct { + out *Outcome + err error + } + authResult := make(chan result, 1) + go func() { + out, err := m.Authenticate(context.Background(), nil) + authResult <- result{out: out, err: err} + }() + + <-staleSource.entered + m.complete("existing-flow", &oauth2.Token{ + AccessToken: "installed-token", + TokenType: "bearer", + Expiry: time.Now().Add(time.Hour), + }, nil) + close(staleSource.release) + + got := <-authResult + require.NoError(t, got.err) + assert.Nil(t, got.out) + assert.Equal(t, "installed-token", m.AccessToken()) + assert.Empty(t, f.recordedGrants(), "a completed concurrent flow must not trigger a redundant authorization") +} + func TestAuthenticateDeviceFlow(t *testing.T) { f := newFakeGitHub(t) f.deviceToken = "gho_device_token" diff --git a/pkg/github/server.go b/pkg/github/server.go index 627cc678b2..67db83a77a 100644 --- a/pkg/github/server.go +++ b/pkg/github/server.go @@ -73,6 +73,11 @@ type MCPServerConfig struct { // token is obtained lazily on first use and refreshed thereafter. TokenProvider func() string + // ToolHandlerMiddleware wraps every registered tool handler. Unlike MCP + // receiving middleware, these wrappers execute inside Server.callTool, so + // SDK result finalization still runs on results they return. + ToolHandlerMiddleware []inventory.ToolHandlerMiddleware + // Additional server options to apply ServerOptions []MCPServerOption } @@ -105,7 +110,7 @@ func NewMCPServer(ctx context.Context, cfg *MCPServerConfig, deps ToolDependenci } // Register GitHub tools/resources/prompts from the inventory. - inv.RegisterAll(ctx, ghServer, deps) + inv.RegisterAll(ctx, ghServer, deps, cfg.ToolHandlerMiddleware...) // Register MCP App UI resources whenever the embedded UI assets are // available. The resources are static HTML and are only referenced by diff --git a/pkg/inventory/registry.go b/pkg/inventory/registry.go index 6505e6b5ef..915ed0aa1c 100644 --- a/pkg/inventory/registry.go +++ b/pkg/inventory/registry.go @@ -219,9 +219,9 @@ func shouldStripMCPAppsMetadata(ctx context.Context, featureFlagEnabled bool) bo // user identity from ctx would otherwise see context.Background() and // falsely report the flag off, even when the actual request arrived on the // /insiders route. -func (r *Inventory) RegisterTools(ctx context.Context, s *mcp.Server, deps any) { +func (r *Inventory) RegisterTools(ctx context.Context, s *mcp.Server, deps any, middleware ...ToolHandlerMiddleware) { for _, tool := range r.ToolsForRegistration(ctx) { - tool.RegisterFunc(s, deps) + tool.RegisterFunc(s, deps, middleware...) } } @@ -257,8 +257,8 @@ func (r *Inventory) RegisterPrompts(ctx context.Context, s *mcp.Server) { // RegisterAll registers all available tools, resources, and prompts with the server. // The context is used for feature flag evaluation. -func (r *Inventory) RegisterAll(ctx context.Context, s *mcp.Server, deps any) { - r.RegisterTools(ctx, s, deps) +func (r *Inventory) RegisterAll(ctx context.Context, s *mcp.Server, deps any, middleware ...ToolHandlerMiddleware) { + r.RegisterTools(ctx, s, deps, middleware...) r.RegisterResourceTemplates(ctx, s, deps) r.RegisterPrompts(ctx, s) } diff --git a/pkg/inventory/server_tool.go b/pkg/inventory/server_tool.go index beb70138eb..44a062ba2e 100644 --- a/pkg/inventory/server_tool.go +++ b/pkg/inventory/server_tool.go @@ -18,6 +18,10 @@ import ( // should define their own typed dependencies struct and type-assert as needed. type HandlerFunc func(deps any) mcp.ToolHandler +// ToolHandlerMiddleware wraps an MCP tool handler. Middleware is applied from +// right to left, so the first middleware passed to RegisterFunc executes first. +type ToolHandlerMiddleware func(next mcp.ToolHandler) mcp.ToolHandler + // ToolsetID is a unique identifier for a toolset. // Using a distinct type provides compile-time type safety. type ToolsetID string @@ -110,8 +114,11 @@ func (st *ServerTool) Handler(deps any) mcp.ToolHandler { // Icons are automatically applied from the toolset metadata if not already set. // A shallow copy of the tool is made to avoid mutating the original ServerTool. // Panics if the tool has no handler - all tools should have handlers. -func (st *ServerTool) RegisterFunc(s *mcp.Server, deps any) { +func (st *ServerTool) RegisterFunc(s *mcp.Server, deps any, middleware ...ToolHandlerMiddleware) { handler := st.Handler(deps) // This will panic if HandlerFunc is nil + for i := len(middleware) - 1; i >= 0; i-- { + handler = middleware[i](handler) + } // Make a shallow copy of the tool to avoid mutating the original toolCopy := st.Tool // Apply icons from toolset metadata if tool doesn't have icons set diff --git a/pkg/inventory/server_tool_test.go b/pkg/inventory/server_tool_test.go index adf012b1f5..c6d2a6fdd8 100644 --- a/pkg/inventory/server_tool_test.go +++ b/pkg/inventory/server_tool_test.go @@ -81,6 +81,51 @@ func TestNewServerToolWithContextHandler_ValidArguments_Succeeds(t *testing.T) { assert.Equal(t, "success: octocat/hello-world", textContent.Text) } +func TestServerToolRegisterFuncAppliesMiddleware(t *testing.T) { + tool := NewServerTool( + mcp.Tool{ + Name: "wrapped_tool", + InputSchema: &jsonschema.Schema{Type: "object"}, + }, + testToolsetMetadata("test"), + func(_ context.Context, _ *mcp.CallToolRequest) (*mcp.CallToolResult, error) { + return &mcp.CallToolResult{ + Content: []mcp.Content{&mcp.TextContent{Text: "handler"}}, + }, nil + }, + ) + + middlewareCalled := make(chan struct{}, 1) + middleware := func(next mcp.ToolHandler) mcp.ToolHandler { + return func(ctx context.Context, req *mcp.CallToolRequest) (*mcp.CallToolResult, error) { + middlewareCalled <- struct{}{} + return next(ctx, req) + } + } + + server := mcp.NewServer(&mcp.Implementation{Name: "test-server", Version: "v0.0.1"}, nil) + tool.RegisterFunc(server, nil, middleware) + st, ct := mcp.NewInMemoryTransports() + ss, err := server.Connect(context.Background(), st, nil) + require.NoError(t, err) + t.Cleanup(func() { _ = ss.Close() }) + + client := mcp.NewClient(&mcp.Implementation{Name: "test-client", Version: "v0.0.1"}, nil) + cs, err := client.Connect(context.Background(), ct, nil) + require.NoError(t, err) + t.Cleanup(func() { _ = cs.Close() }) + + result, err := cs.CallTool(context.Background(), &mcp.CallToolParams{Name: "wrapped_tool"}) + require.NoError(t, err) + select { + case <-middlewareCalled: + default: + t.Fatal("tool middleware was not called") + } + require.Len(t, result.Content, 1) + assert.Equal(t, "handler", result.Content[0].(*mcp.TextContent).Text) +} + func TestAnnotateHeaderParams(t *testing.T) { tool := &mcp.Tool{InputSchema: &jsonschema.Schema{ Type: "object", From 021c071117099e2129a7202dd7df1c087f0b0df7 Mon Sep 17 00:00:00 2001 From: Sam Morrow Date: Mon, 29 Jun 2026 23:59:40 +0200 Subject: [PATCH 3/3] feat(auth): add GitHub App server-to-server authentication for stdio MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add non-interactive GitHub App installation authentication to the stdio server, so headless deployments (CI, Kubernetes, background agents) can authenticate without a browser, device code, or elicitation. This is the outstanding follow-up tracked in #1333: OAuth login shipped the interactive user-to-server flows, but PEM-based server-to-server auth was still needed to remove the interactive requirement. The new internal/githubapp package signs a short-lived RS256 JWT with the app's private key, exchanges it for an installation access token, and refreshes it transparently before expiry. It exposes a Provider whose AccessToken method mirrors oauth.Manager so it plugs into the existing BearerAuthTransport token provider. Only the standard library and golang.org/x/oauth2 are used. The private key is injected safely: a file path (GITHUB_APP_PRIVATE_KEY_PATH, preferred — mountable as a secret and kept off argv and out of the environment) or an inline GITHUB_APP_PRIVATE_KEY env var. There is intentionally no flag for the key contents, which would otherwise leak via the process command line. App auth is mutually exclusive with a PAT and with OAuth login. A loud startup warning and a dedicated docs page (docs/github-app-auth.md, with Docker and Kubernetes examples) cover the security considerations: this injects a high-privilege credential alongside the agent and is not recommended without an independent security review. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- README.md | 2 + cmd/github-mcp-server/main.go | 109 ++++++++++- docs/github-app-auth.md | 261 +++++++++++++++++++++++++ docs/oauth-login.md | 7 + internal/ghmcp/oauth_test.go | 43 +++-- internal/ghmcp/server.go | 51 ++++- internal/githubapp/githubapp.go | 275 +++++++++++++++++++++++++++ internal/githubapp/githubapp_test.go | 271 ++++++++++++++++++++++++++ 8 files changed, 995 insertions(+), 24 deletions(-) create mode 100644 docs/github-app-auth.md create mode 100644 internal/githubapp/githubapp.go create mode 100644 internal/githubapp/githubapp_test.go diff --git a/README.md b/README.md index 10d987a262..3c45bfca30 100644 --- a/README.md +++ b/README.md @@ -311,6 +311,8 @@ Add one of the following JSON blocks to your IDE's MCP settings. See **[Local Server OAuth Login](docs/oauth-login.md)** for the native-binary flow (no fixed port needed), the headless/device-code fallback, GitHub Enterprise Server / `ghe.com`, and bringing your own OAuth or GitHub App. +**Running headless (CI, Kubernetes, background agents)?** The stdio server can authenticate as a **GitHub App installation** with no browser, device code, or elicitation — see **[GitHub App Server-to-Server Authentication](docs/github-app-auth.md)**. This injects a high-privilege credential alongside the agent, so read the security guidance there first; it is not recommended without an independent security review. + **Or authenticate with a Personal Access Token.** Set `GITHUB_PERSONAL_ACCESS_TOKEN` instead (it takes precedence over OAuth): ```json diff --git a/cmd/github-mcp-server/main.go b/cmd/github-mcp-server/main.go index 231b0cf2c3..7629889293 100644 --- a/cmd/github-mcp-server/main.go +++ b/cmd/github-mcp-server/main.go @@ -1,6 +1,7 @@ package main import ( + "context" "errors" "fmt" "os" @@ -9,10 +10,12 @@ import ( "github.com/github/github-mcp-server/internal/buildinfo" "github.com/github/github-mcp-server/internal/ghmcp" + "github.com/github/github-mcp-server/internal/githubapp" "github.com/github/github-mcp-server/internal/oauth" "github.com/github/github-mcp-server/pkg/github" ghhttp "github.com/github/github-mcp-server/pkg/http" ghoauth "github.com/github/github-mcp-server/pkg/http/oauth" + "github.com/github/github-mcp-server/pkg/utils" "github.com/spf13/cobra" "github.com/spf13/pflag" "github.com/spf13/viper" @@ -37,6 +40,17 @@ var ( Long: `Start a server that communicates via standard input/output streams using JSON-RPC messages.`, RunE: func(_ *cobra.Command, _ []string) error { token := viper.GetString("personal_access_token") + + // GitHub App server-to-server auth (non-interactive). It is detected + // when any app-* setting is present; a partial configuration yields a + // clear error from the loader/validator below rather than silently + // falling back to another mode. + appID := viper.GetString("app-id") + appInstallationID := viper.GetString("app-installation-id") + appPrivateKeyPath := viper.GetString("app-private-key-path") + appPrivateKeyInline := viper.GetString("app-private-key") + appAuthRequested := appID != "" || appInstallationID != "" || appPrivateKeyPath != "" || appPrivateKeyInline != "" + oauthClientID := viper.GetString("oauth-client-id") oauthClientSecret := viper.GetString("oauth-client-secret") // Fall back to the build-time baked-in client (official releases) when none is @@ -45,13 +59,20 @@ var ( // --oauth-client-id. Recognizing the host via NormalizeHost means an explicit // GITHUB_HOST=github.com (or api.github.com) still counts as the default and keeps // zero-config login working. The secret tracks the id, so an explicitly provided - // id with no secret never picks up the baked-in secret. - if oauthClientID == "" && oauth.NormalizeHost(viper.GetString("host")) == "https://github.com" { + // id with no secret never picks up the baked-in secret. App auth opts out of this + // default so configuring an app never accidentally enables OAuth login too. + if oauthClientID == "" && !appAuthRequested && oauth.NormalizeHost(viper.GetString("host")) == "https://github.com" { oauthClientID = buildinfo.OAuthClientID oauthClientSecret = buildinfo.OAuthClientSecret } - if token == "" && oauthClientID == "" { - return errors.New("authentication required: set GITHUB_PERSONAL_ACCESS_TOKEN, or pass --oauth-client-id to log in via OAuth") + if token == "" && !appAuthRequested && oauthClientID == "" { + return errors.New("authentication required: set GITHUB_PERSONAL_ACCESS_TOKEN, configure GitHub App auth (GITHUB_APP_ID, GITHUB_APP_INSTALLATION_ID and GITHUB_APP_PRIVATE_KEY_PATH), or pass --oauth-client-id to log in via OAuth") + } + if appAuthRequested && token != "" { + return errors.New("GitHub App authentication and GITHUB_PERSONAL_ACCESS_TOKEN are mutually exclusive: set only one") + } + if appAuthRequested && oauthClientID != "" { + return errors.New("GitHub App authentication and OAuth login (--oauth-client-id) are mutually exclusive: set only one") } // If you're wondering why we're not using viper.GetStringSlice("toolsets"), @@ -116,7 +137,8 @@ var ( // client. The requested scopes default to the full supported set // (which filters out no tools); an explicit, narrower --oauth-scopes // both narrows the grant and hides tools needing other scopes. - if token == "" { + // Skipped for GitHub App auth, which sources tokens non-interactively. + if token == "" && !appAuthRequested { scopes := ghoauth.SupportedScopes if viper.IsSet("oauth-scopes") { if err := viper.UnmarshalKey("oauth-scopes", &scopes); err != nil { @@ -134,6 +156,17 @@ var ( stdioServerConfig.OAuthScopes = scopes } + // GitHub App server-to-server auth: load and parse the private key, + // then resolve the REST base URL so the server can mint installation + // tokens for the configured host (github.com, GHES, or ghe.com). + if appAuthRequested { + appConfig, err := buildAppAuthConfig(appID, appInstallationID, appPrivateKeyPath, appPrivateKeyInline, viper.GetString("host")) + if err != nil { + return err + } + stdioServerConfig.AppAuth = appConfig + } + return ghmcp.RunStdioServer(stdioServerConfig) }, } @@ -230,6 +263,15 @@ func init() { stdioCmd.Flags().StringSlice("oauth-scopes", nil, "Comma-separated OAuth scopes to request; also filters tools to those scopes. Defaults to the full supported set") stdioCmd.Flags().Int("oauth-callback-port", 0, "Fixed local port for the OAuth callback server. Defaults to a random port; set a fixed port when mapping it through Docker") + // stdio-specific GitHub App (server-to-server) flags. Provide an app ID, + // installation ID, and private key to authenticate non-interactively — no + // browser, device code, or elicitation. Intended for headless deployments. + // The private key itself has no flag (only GITHUB_APP_PRIVATE_KEY): a flag + // would place the key in the process arguments. Prefer the key file path. + stdioCmd.Flags().String("app-id", "", "GitHub App ID or client ID, enabling non-interactive server-to-server authentication") + stdioCmd.Flags().String("app-installation-id", "", "GitHub App installation ID to mint installation access tokens for") + stdioCmd.Flags().String("app-private-key-path", "", "Path to the GitHub App private key (PEM). Preferred over GITHUB_APP_PRIVATE_KEY: keeps the key off the command line and out of the environment") + // HTTP-specific flags httpCmd.Flags().Int("port", 8082, "HTTP server port") httpCmd.Flags().String("listen-host", "", "Host the HTTP server binds to (e.g. 127.0.0.1). Empty binds to all interfaces.") @@ -256,6 +298,9 @@ func init() { _ = viper.BindPFlag("oauth-client-secret", stdioCmd.Flags().Lookup("oauth-client-secret")) _ = viper.BindPFlag("oauth-scopes", stdioCmd.Flags().Lookup("oauth-scopes")) _ = viper.BindPFlag("oauth-callback-port", stdioCmd.Flags().Lookup("oauth-callback-port")) + _ = viper.BindPFlag("app-id", stdioCmd.Flags().Lookup("app-id")) + _ = viper.BindPFlag("app-installation-id", stdioCmd.Flags().Lookup("app-installation-id")) + _ = viper.BindPFlag("app-private-key-path", stdioCmd.Flags().Lookup("app-private-key-path")) _ = viper.BindPFlag("port", httpCmd.Flags().Lookup("port")) _ = viper.BindPFlag("listen-host", httpCmd.Flags().Lookup("listen-host")) _ = viper.BindPFlag("base-url", httpCmd.Flags().Lookup("base-url")) @@ -281,6 +326,60 @@ func main() { } } +// buildAppAuthConfig assembles the GitHub App server-to-server configuration: +// it loads and parses the private key and resolves the REST base URL for the +// configured host. The private key is read from a file (preferred) or an inline +// environment value; a missing or partial configuration yields a clear error. +func buildAppAuthConfig(appID, installationID, keyPath, keyInline, host string) (*githubapp.Config, error) { + keyBytes, err := loadAppPrivateKey(keyPath, keyInline) + if err != nil { + return nil, err + } + privateKey, err := githubapp.ParsePrivateKey(keyBytes) + if err != nil { + return nil, fmt.Errorf("invalid GitHub App private key: %w", err) + } + + apiHost, err := utils.NewAPIHost(host) + if err != nil { + return nil, fmt.Errorf("failed to parse host for GitHub App authentication: %w", err) + } + restURL, err := apiHost.BaseRESTURL(context.Background()) + if err != nil { + return nil, fmt.Errorf("failed to resolve REST URL for GitHub App authentication: %w", err) + } + + cfg := &githubapp.Config{ + AppID: appID, + InstallationID: installationID, + PrivateKey: privateKey, + BaseRESTURL: restURL.String(), + } + if err := cfg.Validate(); err != nil { + return nil, err + } + return cfg, nil +} + +// loadAppPrivateKey returns the GitHub App private key bytes from a file path +// (preferred — it keeps the key off argv and out of the environment) or from an +// inline value. The inline form tolerates literal "\n" escapes so a PEM survives +// being carried in a single-line environment variable. +func loadAppPrivateKey(path, inline string) ([]byte, error) { + switch { + case path != "": + data, err := os.ReadFile(path) //#nosec G304 -- operator-supplied path to their own key + if err != nil { + return nil, fmt.Errorf("reading GitHub App private key file: %w", err) + } + return data, nil + case inline != "": + return []byte(strings.ReplaceAll(inline, `\n`, "\n")), nil + default: + return nil, errors.New("GitHub App authentication requires a private key: set GITHUB_APP_PRIVATE_KEY_PATH (preferred) or GITHUB_APP_PRIVATE_KEY") + } +} + func wordSepNormalizeFunc(_ *pflag.FlagSet, name string) pflag.NormalizedName { from := []string{"_"} to := "-" diff --git a/docs/github-app-auth.md b/docs/github-app-auth.md new file mode 100644 index 0000000000..ed9a3db11f --- /dev/null +++ b/docs/github-app-auth.md @@ -0,0 +1,261 @@ +# GitHub App Server-to-Server Authentication (stdio) + +The local (stdio) GitHub MCP Server can authenticate as a **GitHub App +installation** instead of as a user. This is a **server-to-server** (s2s) flow: +the server signs a short-lived JSON Web Token (JWT) with your app's private key, +exchanges it for an installation access token, and refreshes that token +automatically. There is **no browser, no device code, and no elicitation**, so +it works in fully non-interactive environments — CI, Kubernetes, and background +agents such as Copilot's cloud agent. + +> [!WARNING] +> **Read this before you enable it.** This mode was added by popular demand, but +> it is **dangerous** and is **not recommended without an independent security +> review** of your deployment and of this implementation. +> +> - It places a **long-lived, high-privilege credential** (your app's private +> key) in the same environment as an AI agent. Anyone or anything that can read +> that environment can mint tokens that act as your app. +> - Installation access tokens minted here can act across **every repository the +> app is installed on**, with the app's full set of permissions. +> - Exposing credentials to agents — and **especially in the cloud** — is +> inherently risky. Treat this as a break-glass capability and proceed with +> **extreme caution**. +> +> If an interactive login is at all possible for your use case, prefer +> [OAuth login](oauth-login.md) instead, which keeps no long-lived secret next to +> the agent. + +## Contents + +- [When to use this](#when-to-use-this) +- [Why stdio only](#why-stdio-only) +- [How it works](#how-it-works) +- [Prerequisites](#prerequisites) +- [Configuration reference](#configuration-reference) +- [Injecting the private key safely](#injecting-the-private-key-safely) +- [Quick start](#quick-start) +- [Kubernetes](#kubernetes) +- [GitHub Enterprise Server and ghe.com](#github-enterprise-server-and-ghecom) +- [Reducing the blast radius](#reducing-the-blast-radius) +- [Troubleshooting](#troubleshooting) + +## When to use this + +Use GitHub App s2s auth only when **all** of the following hold: + +- The server runs **non-interactively** (no human to complete a browser or + device flow). +- The workload should act as an **organization-managed identity** (the app), + not a single user's Personal Access Token (PAT). +- You have reviewed the security implications above and accept them. + +For everything else, prefer [OAuth login](oauth-login.md) or a +[PAT](https://github.com/settings/personal-access-tokens/new). + +## Why stdio only + +This mode is deliberately limited to the **stdio** server, where the server runs +as a subprocess of a single trusted client and the minted token never crosses +that process boundary. + +It is intentionally **not** available for the `http` server. An HTTP server that +authenticated with a server-wide app identity would let **any** client that can +reach its endpoint act as the app, with the app's full permissions — turning a +network-reachable port into ambient, unauthenticated access to your whole +installation. The `http` server therefore keeps requiring a per-request +`Authorization` token, so every caller's identity and permissions stay explicit. + +If you need a hosted, networked deployment, authenticate callers at the +client/proxy layer and pass per-request tokens; don't give the server a standing +identity. + +## How it works + +1. The server builds a JWT and signs it with your app's private key (RS256). The + JWT is valid for under 10 minutes (GitHub's maximum) and identifies your app. +2. It calls `POST /app/installations/{installation_id}/access_tokens` with that + JWT to obtain an **installation access token** (prefixed `ghs_`), which is + valid for up to one hour. +3. Every GitHub API call uses that token. The server refreshes it about five + minutes before it expires, so long-running sessions keep working without any + intervention. + +The private key is held **in memory only**; the server never writes it or the +minted tokens to disk. + +## Prerequisites + +1. **Register a GitHub App** and generate a **private key** (Settings → your + app → *Private keys* → *Generate a private key*). GitHub downloads a `.pem` + file in PKCS#1 or PKCS#8 format — both are accepted. +2. **Install the app** on the account/organization and grant it the **minimum** + permissions and **only the repositories** it needs (see + [Reducing the blast radius](#reducing-the-blast-radius)). +3. Note three values: + - the **App ID** (or the app's **client ID** — either works as the JWT issuer), + - the **installation ID** (visible in the installation's settings URL, or via + the [installations API](https://docs.github.com/en/rest/apps/apps#list-installations-for-the-authenticated-app)), + - the path to the **private key** `.pem`. + +## Configuration reference + +App auth is enabled when **any** of these `app-*` settings is present; a +partial configuration produces a clear startup error. Settings apply only to the +`stdio` command. + +| Flag | Environment variable | Description | +|------|----------------------|-------------| +| `--app-id` | `GITHUB_APP_ID` | GitHub App ID or client ID. Becomes the JWT issuer. | +| `--app-installation-id` | `GITHUB_APP_INSTALLATION_ID` | Installation ID whose token is minted. | +| `--app-private-key-path` | `GITHUB_APP_PRIVATE_KEY_PATH` | Path to the private key PEM file. **Preferred** way to supply the key. | +| _(no flag)_ | `GITHUB_APP_PRIVATE_KEY` | The PEM contents inline. Use only where a file can't be mounted. Literal `\n` sequences are accepted so the key can live in a single-line variable. | + +There is intentionally **no flag** for the private key contents: a flag would +place the key in the process's command line (`ps`, `/proc//cmdline`), where +other processes could read it. + +App auth is **mutually exclusive** with a PAT (`GITHUB_PERSONAL_ACCESS_TOKEN`) +and with OAuth login (`--oauth-client-id`). Configure exactly one. + +## Injecting the private key safely + +The private key is the most sensitive value in this flow. In order of +preference: + +1. **A mounted secret file** (recommended). Point `GITHUB_APP_PRIVATE_KEY_PATH` + at a file your platform mounts from its secret store — a Kubernetes secret + volume, a Docker secret, or a tmpfs file written by your secret manager. The + key never touches the command line or the process environment. +2. **An inline environment variable** (`GITHUB_APP_PRIVATE_KEY`). Acceptable + where files can't be mounted, but the key is then readable by anything that + can inspect the process environment. Avoid this in shared or cloud + environments. + +Never pass the key on the command line, never bake it into an image, and never +commit it to source control. + +## Quick start + +Native binary, key on disk: + +```bash +github-mcp-server stdio \ + --app-id 123456 \ + --app-installation-id 7891011 \ + --app-private-key-path /secrets/github-app.pem +``` + +Equivalently, with environment variables: + +```bash +export GITHUB_APP_ID=123456 +export GITHUB_APP_INSTALLATION_ID=7891011 +export GITHUB_APP_PRIVATE_KEY_PATH=/secrets/github-app.pem +github-mcp-server stdio +``` + +Docker, mounting the key as a read-only file (preferred over passing it inline): + +```bash +docker run -i --rm \ + -v /secrets/github-app.pem:/secrets/github-app.pem:ro \ + -e GITHUB_APP_ID=123456 \ + -e GITHUB_APP_INSTALLATION_ID=7891011 \ + -e GITHUB_APP_PRIVATE_KEY_PATH=/secrets/github-app.pem \ + ghcr.io/github/github-mcp-server +``` + +## Kubernetes + +Store the key in a `Secret` and mount it as a file; pass the IDs as environment +variables. This keeps the key off the command line and out of the container's +environment. + +```yaml +apiVersion: v1 +kind: Secret +metadata: + name: github-app +type: Opaque +stringData: + private-key.pem: | + -----BEGIN RSA PRIVATE KEY----- + ... + -----END RSA PRIVATE KEY----- +--- +apiVersion: v1 +kind: Pod +metadata: + name: github-mcp-server +spec: + containers: + - name: github-mcp-server + image: ghcr.io/github/github-mcp-server + stdin: true + env: + - name: GITHUB_APP_ID + value: "123456" + - name: GITHUB_APP_INSTALLATION_ID + value: "7891011" + - name: GITHUB_APP_PRIVATE_KEY_PATH + value: /secrets/github-app/private-key.pem + volumeMounts: + - name: github-app + mountPath: /secrets/github-app + readOnly: true + volumes: + - name: github-app + secret: + secretName: github-app +``` + +## GitHub Enterprise Server and ghe.com + +Set the host with `--gh-host` / `GITHUB_HOST`; the server derives the correct +installation token endpoint from it, so tokens are minted against your instance +rather than github.com. Register the app and generate its key on that same host. + +```bash +github-mcp-server stdio \ + --gh-host https://github.example.com \ + --app-id 123456 \ + --app-installation-id 7891011 \ + --app-private-key-path /secrets/github-app.pem +``` + +- For GitHub Enterprise Server, prefix the host with `https://`. +- For `ghe.com`, use `https://YOURSUBDOMAIN.ghe.com`. + +## Reducing the blast radius + +Because the minted token can act across the whole installation, minimize what it +can do: + +- **Grant least privilege.** Enable only the app permissions the workload needs, + and prefer read-only where possible. +- **Scope the installation to specific repositories** rather than *All + repositories*. +- **Rotate the private key** periodically and immediately if it may have been + exposed (Settings → your app → *Private keys*). +- **Isolate the runtime.** Run the server where only trusted code shares its + process environment and mounted secrets. +- **Combine with `--read-only` and toolset/scoping flags** to further narrow + what the agent can invoke. See the + [Server Configuration Guide](server-configuration.md). + +## Troubleshooting + +- **`GitHub App authentication requires a private key`** — you set some `app-*` + values but no key. Set `GITHUB_APP_PRIVATE_KEY_PATH` (preferred) or + `GITHUB_APP_PRIVATE_KEY`. +- **`invalid GitHub App private key`** — the PEM could not be parsed. Ensure it + is the app's RSA private key in PKCS#1 or PKCS#8 form and was not truncated + (when inline, encode newlines as literal `\n`). +- **`installation token request failed: 401`** — usually a clock-skew problem or + the wrong App ID/key pairing. Check the host clock and that the key belongs to + the configured app. +- **`installation token request failed: 404`** — the installation ID is wrong, + or the app is not installed where you think. Re-check the installation ID. +- **`... and GITHUB_PERSONAL_ACCESS_TOKEN are mutually exclusive`** — a PAT is + also set in the environment. Unset it; choose exactly one auth mode. diff --git a/docs/oauth-login.md b/docs/oauth-login.md index 16c5dab67e..31a0c90dce 100644 --- a/docs/oauth-login.md +++ b/docs/oauth-login.md @@ -15,6 +15,13 @@ pass `--oauth-client-id` (see [Bring your own app](#bring-your-own-app)). > `http` command have their own authentication; see > [Remote Server](remote-server.md). +> **Running non-interactively?** OAuth still needs a human to complete the flow +> once. For fully headless deployments (CI, Kubernetes, background agents), +> authenticate as a GitHub App installation instead — see +> [GitHub App Server-to-Server Authentication](github-app-auth.md). Note the +> security warnings there: it keeps a high-privilege credential next to the +> agent and is not recommended without an independent security review. + ## Contents - [How it works](#how-it-works) diff --git a/internal/ghmcp/oauth_test.go b/internal/ghmcp/oauth_test.go index ca9b6177ac..46c62d1156 100644 --- a/internal/ghmcp/oauth_test.go +++ b/internal/ghmcp/oauth_test.go @@ -9,6 +9,7 @@ import ( "net/http/httptest" "testing" + "github.com/github/github-mcp-server/internal/githubapp" "github.com/github/github-mcp-server/internal/oauth" "github.com/github/github-mcp-server/pkg/github" "github.com/github/github-mcp-server/pkg/http/headers" @@ -538,20 +539,40 @@ func TestOAuthMultiRoundTripResultType(t *testing.T) { assert.False(t, toolRan) } -// TestRunStdioServerRejectsTokenAndOAuth verifies the mutually-exclusive guard: -// supplying both a static token and an OAuth manager is rejected before the -// server starts, rather than silently preferring one for auth and the other for -// scope filtering. -func TestRunStdioServerRejectsTokenAndOAuth(t *testing.T) { +// TestRunStdioServerRejectsMultipleAuthModes verifies the mutually-exclusive +// guard: supplying more than one of a static token, an OAuth manager, or GitHub +// App auth is rejected before the server starts, rather than silently preferring +// one for auth and another for scope filtering. +func TestRunStdioServerRejectsMultipleAuthModes(t *testing.T) { t.Parallel() mgr := oauth.NewManager(oauth.NewGitHubConfig("client-id", "", nil, "", 0), discardLogger()) - err := RunStdioServer(StdioServerConfig{ - Token: "ghp_static", - OAuthManager: mgr, - }) - require.Error(t, err) - assert.Contains(t, err.Error(), "mutually exclusive") + + tests := []struct { + name string + cfg StdioServerConfig + }{ + { + name: "token and oauth", + cfg: StdioServerConfig{Token: "ghp_static", OAuthManager: mgr}, + }, + { + name: "token and app", + cfg: StdioServerConfig{Token: "ghp_static", AppAuth: &githubapp.Config{}}, + }, + { + name: "oauth and app", + cfg: StdioServerConfig{OAuthManager: mgr, AppAuth: &githubapp.Config{}}, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + err := RunStdioServer(tt.cfg) + require.Error(t, err) + assert.Contains(t, err.Error(), "exactly one authentication mode") + }) + } } // TestCreateGitHubClientsTokenProvider proves the OAuth wiring: when a diff --git a/internal/ghmcp/server.go b/internal/ghmcp/server.go index 1e0611d648..67c3b067e7 100644 --- a/internal/ghmcp/server.go +++ b/internal/ghmcp/server.go @@ -12,6 +12,7 @@ import ( "syscall" "time" + "github.com/github/github-mcp-server/internal/githubapp" "github.com/github/github-mcp-server/internal/oauth" "github.com/github/github-mcp-server/pkg/errors" "github.com/github/github-mcp-server/pkg/github" @@ -257,15 +258,31 @@ type StdioServerConfig struct { // are hidden. The default set is the full supported list, which hides // nothing; an explicit, narrower list filters accordingly. OAuthScopes []string + + // AppAuth, when non-nil, enables non-interactive GitHub App server-to-server + // authentication: the server mints and transparently refreshes installation + // access tokens from the app's private key, with no browser, device code, or + // elicitation. It suits headless deployments (CI, Kubernetes, background + // agents). It is mutually exclusive with a static Token and with + // OAuthManager. See internal/githubapp and docs/github-app-auth.md — this + // injects a high-privilege credential alongside the agent and should not be + // used without an independent security review. + AppAuth *githubapp.Config } // RunStdioServer is not concurrent safe. func RunStdioServer(cfg StdioServerConfig) error { - // OAuth login and a static token are mutually exclusive: they would - // disagree on how the token is sourced (lazy provider vs. static) and on - // scope filtering, so reject the ambiguous combination up front. - if cfg.OAuthManager != nil && cfg.Token != "" { - return fmt.Errorf("OAuthManager and a static Token are mutually exclusive: provide one or the other") + // A static token, OAuth login, and GitHub App auth are mutually exclusive: + // they disagree on how the token is sourced (static vs. lazy provider) and + // on scope filtering, so reject any ambiguous combination up front. + authModes := 0 + for _, on := range []bool{cfg.Token != "", cfg.OAuthManager != nil, cfg.AppAuth != nil} { + if on { + authModes++ + } + } + if authModes > 1 { + return fmt.Errorf("choose exactly one authentication mode: a static Token, OAuthManager (OAuth login), or AppAuth (GitHub App)") } // Create app context @@ -290,6 +307,20 @@ func RunStdioServer(cfg StdioServerConfig) error { logger := slog.New(slogHandler) logger.Info("starting server", "version", cfg.Version, "host", cfg.Host, "readOnly", cfg.ReadOnly, "lockdownEnabled", cfg.LockdownMode) + // GitHub App server-to-server auth mints installation tokens with no human + // in the loop. Build the provider here so it can use the configured logger. + var appProvider *githubapp.Provider + if cfg.AppAuth != nil { + // Surfaced loudly because this injects a high-privilege credential next + // to the agent; the detailed guidance lives in docs/github-app-auth.md. + logger.Warn("GitHub App server-to-server authentication is enabled; installation tokens minted here can act across every repository the app is installed on — review docs/github-app-auth.md and prefer least-privilege, repository-scoped installations") + provider, err := githubapp.NewProvider(*cfg.AppAuth, logger) + if err != nil { + return fmt.Errorf("failed to configure GitHub App authentication: %w", err) + } + appProvider = provider + } + // Determine the scope set used to filter tools. Classic PATs expose their // granted scopes via the API; OAuth uses the requested scopes (the default // set hides nothing, a narrower explicit set filters accordingly). Other @@ -311,13 +342,17 @@ func RunStdioServer(cfg StdioServerConfig) error { logger.Debug("skipping scope filtering for non-PAT token") } - // For OAuth, the token is resolved lazily: empty until the user authorizes - // on the first tool call, then refreshed for the rest of the session. + // For OAuth or GitHub App auth, the token is resolved lazily by a provider: + // empty until the user authorizes (OAuth) or minted on demand and refreshed + // (App). A static PAT, by contrast, is passed through unchanged. var tokenProvider func() string var toolHandlerMiddleware []inventory.ToolHandlerMiddleware - if cfg.OAuthManager != nil { + switch { + case cfg.OAuthManager != nil: tokenProvider = cfg.OAuthManager.AccessToken toolHandlerMiddleware = append(toolHandlerMiddleware, createOAuthToolMiddleware(cfg.OAuthManager, logger)) + case appProvider != nil: + tokenProvider = appProvider.AccessToken } ghServer, err := NewStdioMCPServer(ctx, github.MCPServerConfig{ diff --git a/internal/githubapp/githubapp.go b/internal/githubapp/githubapp.go new file mode 100644 index 0000000000..49072b93f0 --- /dev/null +++ b/internal/githubapp/githubapp.go @@ -0,0 +1,275 @@ +// Package githubapp implements non-interactive GitHub App server-to-server +// (s2s) authentication for the stdio server. +// +// Unlike the user-to-server OAuth flows in internal/oauth, this requires no +// human: no browser, no device code, no elicitation. It signs a short-lived +// JWT with the app's private key, exchanges it for an installation access +// token, and transparently refreshes that token before it expires. That makes +// it suitable for headless deployments — CI, Kubernetes, background agents. +// +// It only depends on the standard library and golang.org/x/oauth2. +// +// # Security +// +// This mode injects a long-lived, high-privilege credential (the app private +// key) into an environment shared with an AI agent, and the installation +// tokens it mints can act across every repository the app is installed on. It +// was added by popular demand for non-interactive deployments, but exposing +// credentials to agents — especially in the cloud — is dangerous and is not +// recommended without an independent security review. See +// docs/github-app-auth.md for the full guidance and least-privilege advice. +package githubapp + +import ( + "context" + "crypto" + "crypto/rand" + "crypto/rsa" + "crypto/sha256" + "crypto/x509" + "encoding/base64" + "encoding/json" + "encoding/pem" + "errors" + "fmt" + "io" + "log/slog" + "net/http" + "net/url" + "os" + "strings" + "sync" + "time" + + "golang.org/x/oauth2" +) + +const ( + // jwtLifetime is how long minted app JWTs are valid. GitHub rejects app JWTs + // whose exp is more than 10 minutes in the future; 9 minutes leaves headroom. + jwtLifetime = 9 * time.Minute + + // clockSkew backdates the JWT iat to tolerate small clock differences + // between this host and GitHub, which would otherwise reject the JWT. + clockSkew = 60 * time.Second + + // refreshBuffer refreshes installation tokens this long before their real + // expiry so an in-flight request never races the expiry boundary. + refreshBuffer = 5 * time.Minute + + // httpTimeout bounds each call to the installation token endpoint so a + // stalled GitHub API cannot block a tool call indefinitely. + httpTimeout = 30 * time.Second +) + +// Config describes a GitHub App installation used for server-to-server auth. +type Config struct { + // AppID is the GitHub App's App ID or client ID; it becomes the JWT issuer + // (iss). Both forms are accepted by GitHub. + AppID string + + // InstallationID identifies the installation whose access token is minted. + InstallationID string + + // PrivateKey signs the app JWT (RS256). Parse one with ParsePrivateKey. + PrivateKey *rsa.PrivateKey + + // BaseRESTURL is the REST API base, e.g. https://api.github.com/ for + // github.com or https://HOST/api/v3/ for GitHub Enterprise Server. + BaseRESTURL string +} + +// Validate reports whether the configuration is complete enough to mint tokens. +func (c Config) Validate() error { + switch { + case c.AppID == "": + return errors.New("GitHub App ID is required (GITHUB_APP_ID)") + case c.InstallationID == "": + return errors.New("GitHub App installation ID is required (GITHUB_APP_INSTALLATION_ID)") + case c.PrivateKey == nil: + return errors.New("GitHub App private key is required (GITHUB_APP_PRIVATE_KEY_PATH or GITHUB_APP_PRIVATE_KEY)") + case c.BaseRESTURL == "": + return errors.New("GitHub App REST base URL is required") + } + return nil +} + +// ParsePrivateKey parses a PEM-encoded RSA private key in PKCS#1 ("RSA PRIVATE +// KEY") or PKCS#8 ("PRIVATE KEY") form — the two formats GitHub issues for app +// keys. +func ParsePrivateKey(pemBytes []byte) (*rsa.PrivateKey, error) { + block, _ := pem.Decode(pemBytes) + if block == nil { + return nil, errors.New("no PEM block found in private key") + } + if key, err := x509.ParsePKCS1PrivateKey(block.Bytes); err == nil { + return key, nil + } + parsed, err := x509.ParsePKCS8PrivateKey(block.Bytes) + if err != nil { + return nil, fmt.Errorf("parsing private key (want PKCS#1 or PKCS#8 RSA): %w", err) + } + key, ok := parsed.(*rsa.PrivateKey) + if !ok { + return nil, fmt.Errorf("private key is %T, want an RSA key", parsed) + } + return key, nil +} + +// mintJWT builds and signs a short-lived app JWT (RS256) for the configured +// app, as required by the installation token endpoint. +func (c Config) mintJWT(now time.Time) (string, error) { + header := map[string]string{"alg": "RS256", "typ": "JWT"} + claims := map[string]any{ + "iat": now.Add(-clockSkew).Unix(), + "exp": now.Add(jwtLifetime).Unix(), + "iss": c.AppID, + } + + headerJSON, err := json.Marshal(header) + if err != nil { + return "", fmt.Errorf("encoding JWT header: %w", err) + } + claimsJSON, err := json.Marshal(claims) + if err != nil { + return "", fmt.Errorf("encoding JWT claims: %w", err) + } + + signingInput := base64.RawURLEncoding.EncodeToString(headerJSON) + "." + + base64.RawURLEncoding.EncodeToString(claimsJSON) + + digest := sha256.Sum256([]byte(signingInput)) + signature, err := rsa.SignPKCS1v15(rand.Reader, c.PrivateKey, crypto.SHA256, digest[:]) + if err != nil { + return "", fmt.Errorf("signing JWT: %w", err) + } + + return signingInput + "." + base64.RawURLEncoding.EncodeToString(signature), nil +} + +// installationTokenSource is an oauth2.TokenSource that mints GitHub App +// installation access tokens. It performs no caching itself; wrap it in +// oauth2.ReuseTokenSource (see NewProvider) for that. +type installationTokenSource struct { + cfg Config + httpClient *http.Client +} + +func newInstallationTokenSource(cfg Config, httpClient *http.Client) *installationTokenSource { + if httpClient == nil { + httpClient = &http.Client{Timeout: httpTimeout} + } + return &installationTokenSource{cfg: cfg, httpClient: httpClient} +} + +// Token mints a fresh installation access token. The returned token's Expiry is +// set refreshBuffer before the real expiry so callers refresh early. +func (s *installationTokenSource) Token() (*oauth2.Token, error) { + jwt, err := s.cfg.mintJWT(time.Now()) + if err != nil { + return nil, err + } + + endpoint, err := url.JoinPath(s.cfg.BaseRESTURL, "app", "installations", s.cfg.InstallationID, "access_tokens") + if err != nil { + return nil, fmt.Errorf("building installation token URL: %w", err) + } + + ctx, cancel := context.WithTimeout(context.Background(), httpTimeout) + defer cancel() + + req, err := http.NewRequestWithContext(ctx, http.MethodPost, endpoint, nil) + if err != nil { + return nil, fmt.Errorf("creating installation token request: %w", err) + } + req.Header.Set("Authorization", "Bearer "+jwt) + req.Header.Set("Accept", "application/vnd.github+json") + req.Header.Set("X-GitHub-Api-Version", "2022-11-28") + + resp, err := s.httpClient.Do(req) + if err != nil { + return nil, fmt.Errorf("requesting installation token: %w", err) + } + defer func() { _ = resp.Body.Close() }() + + if resp.StatusCode != http.StatusCreated { + // The error body is GitHub's JSON message (never the token); include a + // bounded snippet to make misconfiguration diagnosable. + snippet, _ := io.ReadAll(io.LimitReader(resp.Body, 512)) + return nil, fmt.Errorf("installation token request failed: %s: %s", resp.Status, strings.TrimSpace(string(snippet))) + } + + var body struct { + Token string `json:"token"` + ExpiresAt time.Time `json:"expires_at"` + } + if err := json.NewDecoder(resp.Body).Decode(&body); err != nil { + return nil, fmt.Errorf("decoding installation token response: %w", err) + } + if body.Token == "" { + return nil, errors.New("installation token response did not contain a token") + } + + expiry := body.ExpiresAt + if !expiry.IsZero() { + expiry = expiry.Add(-refreshBuffer) + } + return &oauth2.Token{ + AccessToken: body.Token, + TokenType: "token", + Expiry: expiry, + }, nil +} + +// Provider supplies GitHub App installation access tokens, caching and +// refreshing them transparently. Its AccessToken method mirrors +// oauth.Manager.AccessToken so it can back BearerAuthTransport.TokenProvider. +type Provider struct { + source oauth2.TokenSource + logger *slog.Logger + + mu sync.Mutex + errLogged bool +} + +// NewProvider validates cfg and returns a Provider that mints and refreshes +// installation tokens. A nil logger logs to stderr. +func NewProvider(cfg Config, logger *slog.Logger) (*Provider, error) { + if err := cfg.Validate(); err != nil { + return nil, err + } + if logger == nil { + logger = slog.New(slog.NewTextHandler(os.Stderr, nil)) + } + // ReuseTokenSource caches the token and only calls the underlying source + // once the cached token is expired. Because Token() backdates Expiry by + // refreshBuffer, that refresh happens ~5 minutes before the real expiry. + source := oauth2.ReuseTokenSource(nil, newInstallationTokenSource(cfg, nil)) + return &Provider{source: source, logger: logger}, nil +} + +// AccessToken returns a currently valid installation access token, refreshing +// it if needed, or "" if a token could not be obtained. A fetch failure is +// logged once (until the next success) so a misconfiguration is visible without +// flooding the log on every tool call. +func (p *Provider) AccessToken() string { + tok, err := p.source.Token() + if err != nil { + p.mu.Lock() + if !p.errLogged { + p.errLogged = true + p.logger.Error("failed to obtain GitHub App installation token", "error", err) + } + p.mu.Unlock() + return "" + } + p.mu.Lock() + p.errLogged = false + p.mu.Unlock() + return tok.AccessToken +} + +// HasToken reports whether a valid token can currently be obtained. +func (p *Provider) HasToken() bool { + return p.AccessToken() != "" +} diff --git a/internal/githubapp/githubapp_test.go b/internal/githubapp/githubapp_test.go new file mode 100644 index 0000000000..c2d5eeff7f --- /dev/null +++ b/internal/githubapp/githubapp_test.go @@ -0,0 +1,271 @@ +package githubapp + +import ( + "bytes" + "crypto" + "crypto/ed25519" + "crypto/rand" + "crypto/rsa" + "crypto/sha256" + "crypto/x509" + "encoding/base64" + "encoding/json" + "encoding/pem" + "fmt" + "log/slog" + "net/http" + "net/http/httptest" + "strings" + "sync/atomic" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func newTestKey(t *testing.T) *rsa.PrivateKey { + t.Helper() + key, err := rsa.GenerateKey(rand.Reader, 2048) + require.NoError(t, err) + return key +} + +func pkcs1PEM(t *testing.T, key *rsa.PrivateKey) []byte { + t.Helper() + return pem.EncodeToMemory(&pem.Block{Type: "RSA PRIVATE KEY", Bytes: x509.MarshalPKCS1PrivateKey(key)}) +} + +func pkcs8PEM(t *testing.T, key *rsa.PrivateKey) []byte { + t.Helper() + der, err := x509.MarshalPKCS8PrivateKey(key) + require.NoError(t, err) + return pem.EncodeToMemory(&pem.Block{Type: "PRIVATE KEY", Bytes: der}) +} + +func TestParsePrivateKey(t *testing.T) { + key := newTestKey(t) + + t.Run("PKCS1", func(t *testing.T) { + got, err := ParsePrivateKey(pkcs1PEM(t, key)) + require.NoError(t, err) + assert.Equal(t, key.N, got.N) + }) + + t.Run("PKCS8", func(t *testing.T) { + got, err := ParsePrivateKey(pkcs8PEM(t, key)) + require.NoError(t, err) + assert.Equal(t, key.N, got.N) + }) + + t.Run("not PEM", func(t *testing.T) { + _, err := ParsePrivateKey([]byte("not a pem")) + require.Error(t, err) + assert.Contains(t, err.Error(), "no PEM block") + }) + + t.Run("non-RSA key", func(t *testing.T) { + _, priv, err := ed25519.GenerateKey(rand.Reader) + require.NoError(t, err) + der, err := x509.MarshalPKCS8PrivateKey(priv) + require.NoError(t, err) + keyPEM := pem.EncodeToMemory(&pem.Block{Type: "PRIVATE KEY", Bytes: der}) + + _, err = ParsePrivateKey(keyPEM) + require.Error(t, err) + assert.Contains(t, err.Error(), "want an RSA key") + }) +} + +func TestConfigValidate(t *testing.T) { + key := newTestKey(t) + base := Config{AppID: "123", InstallationID: "456", PrivateKey: key, BaseRESTURL: "https://api.github.com/"} + require.NoError(t, base.Validate()) + + tests := []struct { + name string + mutate func(c *Config) + want string + }{ + {"missing app id", func(c *Config) { c.AppID = "" }, "App ID is required"}, + {"missing installation id", func(c *Config) { c.InstallationID = "" }, "installation ID is required"}, + {"missing private key", func(c *Config) { c.PrivateKey = nil }, "private key is required"}, + {"missing base url", func(c *Config) { c.BaseRESTURL = "" }, "REST base URL is required"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + c := base + tt.mutate(&c) + err := c.Validate() + require.Error(t, err) + assert.Contains(t, err.Error(), tt.want) + }) + } +} + +// verifyJWT parses and verifies an app JWT against the public key and returns +// its claims, asserting the structural requirements GitHub enforces. +func verifyJWT(t *testing.T, token string, pub *rsa.PublicKey) map[string]any { + t.Helper() + parts := strings.Split(token, ".") + require.Len(t, parts, 3, "JWT must have three segments") + + headerJSON, err := base64.RawURLEncoding.DecodeString(parts[0]) + require.NoError(t, err) + var header map[string]string + require.NoError(t, json.Unmarshal(headerJSON, &header)) + assert.Equal(t, "RS256", header["alg"]) + assert.Equal(t, "JWT", header["typ"]) + + signingInput := parts[0] + "." + parts[1] + digest := sha256.Sum256([]byte(signingInput)) + signature, err := base64.RawURLEncoding.DecodeString(parts[2]) + require.NoError(t, err) + require.NoError(t, rsa.VerifyPKCS1v15(pub, crypto.SHA256, digest[:], signature), "signature must verify") + + claimsJSON, err := base64.RawURLEncoding.DecodeString(parts[1]) + require.NoError(t, err) + var claims map[string]any + require.NoError(t, json.Unmarshal(claimsJSON, &claims)) + return claims +} + +func TestMintJWT(t *testing.T) { + key := newTestKey(t) + cfg := Config{AppID: "my-app-id", PrivateKey: key} + + now := time.Now() + token, err := cfg.mintJWT(now) + require.NoError(t, err) + + claims := verifyJWT(t, token, &key.PublicKey) + assert.Equal(t, "my-app-id", claims["iss"]) + + iat := int64(claims["iat"].(float64)) + exp := int64(claims["exp"].(float64)) + assert.Equal(t, now.Add(-clockSkew).Unix(), iat, "iat should be backdated by the clock skew") + assert.Equal(t, now.Add(jwtLifetime).Unix(), exp) + assert.LessOrEqual(t, exp-iat, int64((10 * time.Minute).Seconds()), "JWT must live no longer than GitHub's 10 minute cap") +} + +// installationServer is a fake installation token endpoint that verifies the +// app JWT and returns a token expiring at expiresAt. It counts mint requests. +func installationServer(t *testing.T, pub *rsa.PublicKey, token string, expiresAt time.Time) (*httptest.Server, *atomic.Int32) { + t.Helper() + var calls atomic.Int32 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + calls.Add(1) + assert.Equal(t, http.MethodPost, r.Method) + assert.Equal(t, "/app/installations/456/access_tokens", r.URL.Path) + + authz := r.Header.Get("Authorization") + require.True(t, strings.HasPrefix(authz, "Bearer "), "must send the app JWT as a bearer token") + verifyJWT(t, strings.TrimPrefix(authz, "Bearer "), pub) + + w.WriteHeader(http.StatusCreated) + _ = json.NewEncoder(w).Encode(map[string]any{ + "token": token, + "expires_at": expiresAt.UTC().Format(time.RFC3339), + }) + })) + t.Cleanup(srv.Close) + return srv, &calls +} + +func newTestConfig(key *rsa.PrivateKey, baseURL string) Config { + return Config{AppID: "123", InstallationID: "456", PrivateKey: key, BaseRESTURL: baseURL + "/"} +} + +func TestProviderFetchesToken(t *testing.T) { + key := newTestKey(t) + srv, calls := installationServer(t, &key.PublicKey, "ghs_fresh", time.Now().Add(time.Hour)) + + provider, err := NewProvider(newTestConfig(key, srv.URL), slog.New(slog.NewTextHandler(&bytes.Buffer{}, nil))) + require.NoError(t, err) + + assert.Equal(t, "ghs_fresh", provider.AccessToken()) + assert.True(t, provider.HasToken()) + assert.Equal(t, int32(1), calls.Load()) +} + +func TestProviderCachesToken(t *testing.T) { + key := newTestKey(t) + srv, calls := installationServer(t, &key.PublicKey, "ghs_cached", time.Now().Add(time.Hour)) + + provider, err := NewProvider(newTestConfig(key, srv.URL), slog.New(slog.NewTextHandler(&bytes.Buffer{}, nil))) + require.NoError(t, err) + + for range 3 { + assert.Equal(t, "ghs_cached", provider.AccessToken()) + } + assert.Equal(t, int32(1), calls.Load(), "a token valid for an hour should be minted only once") +} + +func TestProviderRefreshesNearExpiry(t *testing.T) { + key := newTestKey(t) + // expires within the refresh buffer, so the stored expiry is already in the + // past and every call re-mints. + srv, calls := installationServer(t, &key.PublicKey, "ghs_short", time.Now().Add(refreshBuffer-time.Minute)) + + provider, err := NewProvider(newTestConfig(key, srv.URL), slog.New(slog.NewTextHandler(&bytes.Buffer{}, nil))) + require.NoError(t, err) + + assert.Equal(t, "ghs_short", provider.AccessToken()) + assert.Equal(t, "ghs_short", provider.AccessToken()) + assert.Equal(t, int32(2), calls.Load(), "a token expiring within the refresh buffer should re-mint each call") +} + +func TestProviderErrorLoggedOnce(t *testing.T) { + key := newTestKey(t) + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusUnauthorized) + _, _ = w.Write([]byte(`{"message":"A JSON web token could not be decoded"}`)) + })) + t.Cleanup(srv.Close) + + var logBuf bytes.Buffer + logger := slog.New(slog.NewTextHandler(&logBuf, nil)) + provider, err := NewProvider(newTestConfig(key, srv.URL), logger) + require.NoError(t, err) + + assert.Empty(t, provider.AccessToken()) + assert.Empty(t, provider.AccessToken()) + assert.Equal(t, 1, strings.Count(logBuf.String(), "failed to obtain GitHub App installation token"), + "a repeated fetch failure should only be logged once") +} + +func TestProviderErrorIncludesStatus(t *testing.T) { + key := newTestKey(t) + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusNotFound) + _, _ = w.Write([]byte(`{"message":"Not Found"}`)) + })) + t.Cleanup(srv.Close) + + source := newInstallationTokenSource(newTestConfig(key, srv.URL), srv.Client()) + _, err := source.Token() + require.Error(t, err) + assert.Contains(t, err.Error(), "404") + assert.Contains(t, err.Error(), "Not Found") +} + +func TestNewProviderValidates(t *testing.T) { + _, err := NewProvider(Config{}, nil) + require.Error(t, err) + assert.Contains(t, err.Error(), "App ID is required") +} + +// Ensure the source returns an error rather than panicking on a token-less 201. +func TestSourceRejectsEmptyToken(t *testing.T) { + key := newTestKey(t) + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusCreated) + _, _ = fmt.Fprint(w, `{"expires_at":"2099-01-01T00:00:00Z"}`) + })) + t.Cleanup(srv.Close) + + source := newInstallationTokenSource(newTestConfig(key, srv.URL), srv.Client()) + _, err := source.Token() + require.Error(t, err) + assert.Contains(t, err.Error(), "did not contain a token") +}