fix(coderd): add PKCE support to MCP server OAuth2 flow (#23503)

## 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
2026-09-22 05:05:20 +08:00 · 2026-03-24 11:55:14 -05:00
parent f71e897a83
commit e87ea1e0f5
2 changed files with 304 additions and 7 deletions
@@ -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.",
@@ -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()