From e87ea1e0f52d180b75978b32cea5ef7c6df17cd3 Mon Sep 17 00:00:00 2001 From: Kyle Carberry Date: Tue, 24 Mar 2026 12:55:14 -0400 Subject: [PATCH] fix(coderd): add PKCE support to MCP server OAuth2 flow (#23503) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Problem MCP servers like Linear (`mcp.linear.app`) require PKCE (RFC 7636) for their OAuth2 flow. Without it, the token exchange may succeed but the resulting access token is immediately rejected with a 401 `invalid_token` error when the chat daemon tries to connect to the MCP server. This means users can authenticate successfully in the UI (the OAuth popup completes, `auth_connected` shows `true`), but the model never receives the MCP tools — they silently fail to load. ### Root cause The `mcpServerOAuth2Connect` handler was calling `oauth2Config.AuthCodeURL(state)` without any PKCE parameters (`code_challenge`, `code_challenge_method`). The callback was calling `oauth2Config.Exchange(ctx, code)` without a `code_verifier`. Linear's MCP OAuth endpoint decoded state confirms it expected PKCE with `codeChallengeMethod: "plain"`. ### Investigation - The chat (`c2c04fc5-5622-4b71-a5a9-80508e86f78e`) had the Linear MCP server ID in `mcp_server_ids` - `auth_connected: true` (token row exists in DB) - No "expired" or "empty token" warnings in logs - Server log showed: `skipping MCP server due to connection failure ... error="initialize: transport error: request failed with status 401: {"error":"invalid_token","error_description":"Missing or invalid access token"}"` - Decoding Linear's OAuth state revealed PKCE was expected ## Changes - Generate a PKCE `code_verifier` during the OAuth2 connect step using `oauth2.GenerateVerifier()` and store it in a cookie scoped to the callback path - Include `code_challenge` (S256) in the authorization redirect URL via `oauth2.S256ChallengeOption()` - Pass the `code_verifier` during the token exchange in the callback via `oauth2.VerifierOption()` - Fix a nil-pointer guard on `api.HTTPClient` in the callback - Add tests verifying PKCE parameters are sent correctly and backwards compatibility when no verifier cookie is present --- coderd/mcp.go | 49 +++++++-- coderd/mcp_test.go | 262 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 304 insertions(+), 7 deletions(-) diff --git a/coderd/mcp.go b/coderd/mcp.go index 2644d49670..8d407360a2 100644 --- a/coderd/mcp.go +++ b/coderd/mcp.go @@ -743,10 +743,24 @@ func (api *API) mcpServerOAuth2Connect(rw http.ResponseWriter, r *http.Request) // The callback URL is on our server; after the exchange we store // the token and close the popup. state := uuid.New().String() + callbackPath := fmt.Sprintf("/api/experimental/mcp/servers/%s/oauth2/callback", config.ID) http.SetCookie(rw, api.DeploymentValues.HTTPCookies.Apply(&http.Cookie{ Name: "mcp_oauth2_state_" + config.ID.String(), Value: state, - Path: fmt.Sprintf("/api/experimental/mcp/servers/%s/oauth2/callback", config.ID), + Path: callbackPath, + MaxAge: 600, // 10 minutes + HttpOnly: true, + SameSite: http.SameSiteLaxMode, + })) + + // PKCE (RFC 7636) is required by many OAuth2 providers (e.g. + // Linear). We always send it because it is harmless when the + // server ignores it and essential when it does not. + verifier := oauth2.GenerateVerifier() + http.SetCookie(rw, api.DeploymentValues.HTTPCookies.Apply(&http.Cookie{ + Name: "mcp_oauth2_verifier_" + config.ID.String(), + Value: verifier, + Path: callbackPath, MaxAge: 600, // 10 minutes HttpOnly: true, SameSite: http.SameSiteLaxMode, @@ -759,14 +773,14 @@ func (api *API) mcpServerOAuth2Connect(rw http.ResponseWriter, r *http.Request) AuthURL: config.OAuth2AuthURL, TokenURL: config.OAuth2TokenURL, }, - RedirectURL: fmt.Sprintf("%s/api/experimental/mcp/servers/%s/oauth2/callback", api.AccessURL.String(), config.ID), + RedirectURL: fmt.Sprintf("%s%s", api.AccessURL.String(), callbackPath), } var scopes []string if config.OAuth2Scopes != "" { scopes = strings.Split(config.OAuth2Scopes, " ") } oauth2Config.Scopes = scopes - authURL := oauth2Config.AuthCodeURL(state) + authURL := oauth2Config.AuthCodeURL(state, oauth2.S256ChallengeOption(verifier)) http.Redirect(rw, r, authURL, http.StatusTemporaryRedirect) } @@ -848,10 +862,26 @@ func (api *API) mcpServerOAuth2Callback(rw http.ResponseWriter, r *http.Request) return } // Clear the state cookie. + callbackPath := fmt.Sprintf("/api/experimental/mcp/servers/%s/oauth2/callback", config.ID) http.SetCookie(rw, api.DeploymentValues.HTTPCookies.Apply(&http.Cookie{ Name: "mcp_oauth2_state_" + config.ID.String(), Value: "", - Path: fmt.Sprintf("/api/experimental/mcp/servers/%s/oauth2/callback", config.ID), + Path: callbackPath, + MaxAge: -1, + HttpOnly: true, + SameSite: http.SameSiteLaxMode, + })) + + // Recover the PKCE code_verifier set during the connect step. + var exchangeOpts []oauth2.AuthCodeOption + if verifierCookie, err := r.Cookie("mcp_oauth2_verifier_" + config.ID.String()); err == nil { + exchangeOpts = append(exchangeOpts, oauth2.VerifierOption(verifierCookie.Value)) + } + // Clear the verifier cookie regardless of whether it was present. + http.SetCookie(rw, api.DeploymentValues.HTTPCookies.Apply(&http.Cookie{ + Name: "mcp_oauth2_verifier_" + config.ID.String(), + Value: "", + Path: callbackPath, MaxAge: -1, HttpOnly: true, SameSite: http.SameSiteLaxMode, @@ -865,7 +895,7 @@ func (api *API) mcpServerOAuth2Callback(rw http.ResponseWriter, r *http.Request) AuthURL: config.OAuth2AuthURL, TokenURL: config.OAuth2TokenURL, }, - RedirectURL: fmt.Sprintf("%s/api/experimental/mcp/servers/%s/oauth2/callback", api.AccessURL.String(), config.ID), + RedirectURL: fmt.Sprintf("%s%s", api.AccessURL.String(), callbackPath), } var scopes []string if config.OAuth2Scopes != "" { @@ -875,8 +905,13 @@ func (api *API) mcpServerOAuth2Callback(rw http.ResponseWriter, r *http.Request) // Use the deployment's HTTP client for the token exchange to // respect proxy settings and avoid using http.DefaultClient. - exchangeCtx := context.WithValue(ctx, oauth2.HTTPClient, api.HTTPClient) - token, err := oauth2Config.Exchange(exchangeCtx, code) + // Guard against nil so the oauth2 library falls back to the + // default client instead of panicking. + exchangeCtx := ctx + if api.HTTPClient != nil { + exchangeCtx = context.WithValue(ctx, oauth2.HTTPClient, api.HTTPClient) + } + token, err := oauth2Config.Exchange(exchangeCtx, code, exchangeOpts...) if err != nil { httpapi.Write(ctx, rw, http.StatusBadGateway, codersdk.Response{ Message: "Failed to exchange authorization code for token.", diff --git a/coderd/mcp_test.go b/coderd/mcp_test.go index 98e59b12a0..b904a5eb91 100644 --- a/coderd/mcp_test.go +++ b/coderd/mcp_test.go @@ -1,6 +1,8 @@ package coderd_test import ( + "crypto/sha256" + "encoding/base64" "encoding/json" "net/http" "net/http/httptest" @@ -734,6 +736,266 @@ func TestMCPServerConfigsOAuth2AutoDiscovery(t *testing.T) { }) } +// nolint:bodyclose +func TestMCPServerOAuth2PKCE(t *testing.T) { + t.Parallel() + + t.Run("ConnectSetsPKCEParams", func(t *testing.T) { + t.Parallel() + + ctx := testutil.Context(t, testutil.WaitLong) + adminClient := newMCPClient(t) + firstUser := coderdtest.CreateFirstUser(t, adminClient) + memberClient, _ := coderdtest.CreateAnotherUser(t, adminClient, firstUser.OrganizationID) + + // Create an OAuth2 MCP server config. + created, err := adminClient.CreateMCPServerConfig(ctx, codersdk.CreateMCPServerConfigRequest{ + DisplayName: "PKCE Test", + Slug: "pkce-test", + Transport: "streamable_http", + URL: "https://mcp.example.com/pkce", + AuthType: "oauth2", + OAuth2ClientID: "test-client", + OAuth2AuthURL: "https://auth.example.com/authorize", + OAuth2TokenURL: "https://auth.example.com/token", + Availability: "default_on", + Enabled: true, + ToolAllowList: []string{}, + ToolDenyList: []string{}, + }) + require.NoError(t, err) + + // Prevent the HTTP client from following redirects so we + // can inspect the response headers and cookies directly. + memberClient.HTTPClient.CheckRedirect = func(_ *http.Request, _ []*http.Request) error { + return http.ErrUseLastResponse + } + + connectURL, err := memberClient.URL.Parse( + "/api/experimental/mcp/servers/" + created.ID.String() + "/oauth2/connect", + ) + require.NoError(t, err) + + req, err := http.NewRequestWithContext(ctx, "GET", connectURL.String(), nil) + require.NoError(t, err) + req.AddCookie(&http.Cookie{ + Name: codersdk.SessionTokenCookie, + Value: memberClient.SessionToken(), + }) + + res, err := memberClient.HTTPClient.Do(req) + require.NoError(t, err) + defer res.Body.Close() + + require.Equal(t, http.StatusTemporaryRedirect, res.StatusCode) + + // The redirect URL must contain PKCE query parameters. + location, err := res.Location() + require.NoError(t, err) + query := location.Query() + require.Equal(t, "S256", query.Get("code_challenge_method"), + "connect redirect must include code_challenge_method=S256") + require.NotEmpty(t, query.Get("code_challenge"), + "connect redirect must include a code_challenge") + + // A verifier cookie must be set. + var verifierCookie *http.Cookie + for _, c := range res.Cookies() { + if c.Name == "mcp_oauth2_verifier_"+created.ID.String() { + verifierCookie = c + break + } + } + require.NotNil(t, verifierCookie, "response must set a PKCE verifier cookie") + require.NotEmpty(t, verifierCookie.Value) + + // Verify the code_challenge matches SHA256(verifier). + h := sha256.Sum256([]byte(verifierCookie.Value)) + expectedChallenge := base64.RawURLEncoding.EncodeToString(h[:]) + require.Equal(t, expectedChallenge, query.Get("code_challenge"), + "code_challenge must equal base64url(SHA256(verifier))") + }) + + t.Run("CallbackSendsVerifier", func(t *testing.T) { + t.Parallel() + + ctx := testutil.Context(t, testutil.WaitLong) + + // Track the code_verifier received by the mock token endpoint. + receivedVerifier := make(chan string, 1) + + tokenServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == "/token" && r.Method == http.MethodPost { + if err := r.ParseForm(); err == nil { + receivedVerifier <- r.FormValue("code_verifier") + } + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{ + "access_token": "test-access-token", + "token_type": "Bearer", + "expires_in": 3600, + "refresh_token": "test-refresh-token" + }`)) + return + } + http.NotFound(w, r) + })) + t.Cleanup(tokenServer.Close) + + adminClient := newMCPClient(t) + firstUser := coderdtest.CreateFirstUser(t, adminClient) + memberClient, _ := coderdtest.CreateAnotherUser(t, adminClient, firstUser.OrganizationID) + + created, err := adminClient.CreateMCPServerConfig(ctx, codersdk.CreateMCPServerConfigRequest{ + DisplayName: "PKCE Callback Test", + Slug: "pkce-callback", + Transport: "streamable_http", + URL: "https://mcp.example.com/pkce-cb", + AuthType: "oauth2", + OAuth2ClientID: "test-client", + OAuth2AuthURL: "https://auth.example.com/authorize", + OAuth2TokenURL: tokenServer.URL + "/token", + Availability: "default_on", + Enabled: true, + ToolAllowList: []string{}, + ToolDenyList: []string{}, + }) + require.NoError(t, err) + + memberClient.HTTPClient.CheckRedirect = func(_ *http.Request, _ []*http.Request) error { + return http.ErrUseLastResponse + } + + // Simulate the callback with a known state and verifier. + state := "test-state-value" + verifier := "test-verifier-value-that-is-at-least-43-chars-long-for-pkce-spec" + + callbackURL, err := memberClient.URL.Parse( + "/api/experimental/mcp/servers/" + created.ID.String() + "/oauth2/callback", + ) + require.NoError(t, err) + q := callbackURL.Query() + q.Set("code", "test-auth-code") + q.Set("state", state) + callbackURL.RawQuery = q.Encode() + + req, err := http.NewRequestWithContext(ctx, "GET", callbackURL.String(), nil) + require.NoError(t, err) + req.AddCookie(&http.Cookie{ + Name: codersdk.SessionTokenCookie, + Value: memberClient.SessionToken(), + }) + req.AddCookie(&http.Cookie{ + Name: "mcp_oauth2_state_" + created.ID.String(), + Value: state, + }) + req.AddCookie(&http.Cookie{ + Name: "mcp_oauth2_verifier_" + created.ID.String(), + Value: verifier, + }) + + res, err := memberClient.HTTPClient.Do(req) + require.NoError(t, err) + defer res.Body.Close() + + require.Equal(t, http.StatusOK, res.StatusCode, + "callback should succeed when given valid state, verifier, and code") + + // Verify the mock token endpoint received the code_verifier. + var gotVerifier string + select { + case gotVerifier = <-receivedVerifier: + case <-ctx.Done(): + t.Fatal("timed out waiting for token exchange") + } + require.Equal(t, verifier, gotVerifier, + "token exchange must send the PKCE code_verifier") + + // Verify the verifier cookie is cleared in the response. + for _, c := range res.Cookies() { + if c.Name == "mcp_oauth2_verifier_"+created.ID.String() { + require.Equal(t, -1, c.MaxAge, + "verifier cookie must be cleared after callback") + } + } + }) + + t.Run("CallbackWithoutVerifierStillWorks", func(t *testing.T) { + t.Parallel() + + ctx := testutil.Context(t, testutil.WaitLong) + + // Token endpoint that does not require a code_verifier. + tokenServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == "/token" && r.Method == http.MethodPost { + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{ + "access_token": "no-pkce-token", + "token_type": "Bearer" + }`)) + return + } + http.NotFound(w, r) + })) + t.Cleanup(tokenServer.Close) + + adminClient := newMCPClient(t) + firstUser := coderdtest.CreateFirstUser(t, adminClient) + memberClient, _ := coderdtest.CreateAnotherUser(t, adminClient, firstUser.OrganizationID) + + created, err := adminClient.CreateMCPServerConfig(ctx, codersdk.CreateMCPServerConfigRequest{ + DisplayName: "No PKCE Callback", + Slug: "no-pkce-callback", + Transport: "streamable_http", + URL: "https://mcp.example.com/no-pkce", + AuthType: "oauth2", + OAuth2ClientID: "test-client", + OAuth2AuthURL: "https://auth.example.com/authorize", + OAuth2TokenURL: tokenServer.URL + "/token", + Availability: "default_on", + Enabled: true, + ToolAllowList: []string{}, + ToolDenyList: []string{}, + }) + require.NoError(t, err) + + memberClient.HTTPClient.CheckRedirect = func(_ *http.Request, _ []*http.Request) error { + return http.ErrUseLastResponse + } + + // Call the callback without a verifier cookie to verify + // backwards compatibility with providers that don't use PKCE. + state := "test-state-no-pkce" + callbackURL, err := memberClient.URL.Parse( + "/api/experimental/mcp/servers/" + created.ID.String() + "/oauth2/callback", + ) + require.NoError(t, err) + q := callbackURL.Query() + q.Set("code", "test-auth-code") + q.Set("state", state) + callbackURL.RawQuery = q.Encode() + + req, err := http.NewRequestWithContext(ctx, "GET", callbackURL.String(), nil) + require.NoError(t, err) + req.AddCookie(&http.Cookie{ + Name: codersdk.SessionTokenCookie, + Value: memberClient.SessionToken(), + }) + req.AddCookie(&http.Cookie{ + Name: "mcp_oauth2_state_" + created.ID.String(), + Value: state, + }) + // Deliberately omit the verifier cookie. + + res, err := memberClient.HTTPClient.Do(req) + require.NoError(t, err) + defer res.Body.Close() + + require.Equal(t, http.StatusOK, res.StatusCode, + "callback without verifier cookie should still succeed") + }) +} + func TestChatWithMCPServerIDs(t *testing.T) { t.Parallel()