fix: use actual config ID in MCP OAuth2 redirect URI during auto-discovery (#23491)

## Problem During OAuth2 auto-discovery for MCP servers, the callback URL registered with the remote authorization server via Dynamic Client Registration (RFC 7591) contained the literal string `{id}` instead of the actual config UUID: ``` https://coder.example.com/api/experimental/mcp/servers/{id}/oauth2/callback ``` This happened because the discovery and registration occurred **before** the database insert that generates the ID. When the user later initiated the OAuth2 connect flow, the redirect URL used the real UUID, causing the authorization server to reject it with: > The provided redirect URIs are not approved for use by this authorization server ## Fix Restructure the auto-discovery flow in `createMCPServerConfig` to: 1. **Insert** the MCP server config first (with empty OAuth2 fields) to get the database-generated UUID 2. **Build** the callback URL with the actual UUID 3. **Perform** OAuth2 discovery and dynamic client registration with the correct URL 4. **Update** the record with the discovered OAuth2 credentials 5. **Clean up** the record if discovery fails ## Testing Added regression test `TestMCPServerConfigsOAuth2AutoDiscovery/RedirectURIContainsRealConfigID` that: - Stands up mock auth + MCP servers - Captures the `redirect_uris` sent during dynamic client registration - Asserts the URI contains the real config UUID, not `{id}` - Verifies the full callback path structure All existing MCP server config tests continue to pass.
2026-09-24 15:04:27 +08:00 · 2026-03-24 13:04:55 +00:00
parent 76eac82e5b
commit 631e4449bb
2 changed files with 252 additions and 7 deletions
@@ -118,9 +118,81 @@ func (api *API) createMCPServerConfig(rw http.ResponseWriter, r *http.Request) {
 		// Metadata (RFC 9728) and Authorization Server Metadata
 		// (RFC 8414), then register a client dynamically.
 		if req.OAuth2ClientID == "" && req.OAuth2AuthURL == "" && req.OAuth2TokenURL == "" {
-			callbackURL := fmt.Sprintf("%s/api/experimental/mcp/servers/{id}/oauth2/callback", api.AccessURL.String())
+			// Auto-discovery flow: we need the config ID first to
+			// build the correct callback URL.  Insert the record
+			// with empty OAuth2 fields, perform discovery, then
+			// update.
+			customHeadersJSON, err := marshalCustomHeaders(req.CustomHeaders)
+			if err != nil {
+				httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{
+					Message: "Invalid custom headers.",
+					Detail:  err.Error(),
+				})
+				return
+			}
+
+			inserted, err := api.Database.InsertMCPServerConfig(ctx, database.InsertMCPServerConfigParams{
+				DisplayName:             strings.TrimSpace(req.DisplayName),
+				Slug:                    strings.TrimSpace(req.Slug),
+				Description:             strings.TrimSpace(req.Description),
+				IconURL:                 strings.TrimSpace(req.IconURL),
+				Transport:               strings.TrimSpace(req.Transport),
+				Url:                     strings.TrimSpace(req.URL),
+				AuthType:                strings.TrimSpace(req.AuthType),
+				OAuth2ClientID:          "",
+				OAuth2ClientSecret:      "",
+				OAuth2ClientSecretKeyID: sql.NullString{},
+				OAuth2AuthURL:           "",
+				OAuth2TokenURL:          "",
+				OAuth2Scopes:            "",
+				APIKeyHeader:            strings.TrimSpace(req.APIKeyHeader),
+				APIKeyValue:             strings.TrimSpace(req.APIKeyValue),
+				APIKeyValueKeyID:        sql.NullString{},
+				CustomHeaders:           customHeadersJSON,
+				CustomHeadersKeyID:      sql.NullString{},
+				ToolAllowList:           coalesceStringSlice(trimStringSlice(req.ToolAllowList)),
+				ToolDenyList:            coalesceStringSlice(trimStringSlice(req.ToolDenyList)),
+				Availability:            strings.TrimSpace(req.Availability),
+				Enabled:                 req.Enabled,
+				CreatedBy:               apiKey.UserID,
+				UpdatedBy:               apiKey.UserID,
+			})
+			if err != nil {
+				switch {
+				case database.IsUniqueViolation(err):
+					httpapi.Write(ctx, rw, http.StatusConflict, codersdk.Response{
+						Message: "MCP server config already exists.",
+						Detail:  err.Error(),
+					})
+					return
+				case database.IsCheckViolation(err):
+					httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{
+						Message: "Invalid MCP server config.",
+						Detail:  err.Error(),
+					})
+					return
+				default:
+					httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
+						Message: "Failed to create MCP server config.",
+						Detail:  err.Error(),
+					})
+					return
+				}
+			}
+
+			// Now build the callback URL with the actual ID.
+			callbackURL := fmt.Sprintf("%s/api/experimental/mcp/servers/%s/oauth2/callback", api.AccessURL.String(), inserted.ID)
 			result, err := discoverAndRegisterMCPOAuth2(ctx, strings.TrimSpace(req.URL), callbackURL)
 			if err != nil {
+				// Clean up: delete the partially created config.
+				deleteErr := api.Database.DeleteMCPServerConfigByID(ctx, inserted.ID)
+				if deleteErr != nil {
+					api.Logger.Warn(ctx, "failed to clean up MCP server config after OAuth2 discovery failure",
+						slog.F("config_id", inserted.ID),
+						slog.Error(deleteErr),
+					)
+				}
+
 				api.Logger.Warn(ctx, "mcp oauth2 auto-discovery failed",
 					slog.F("url", req.URL),
 					slog.Error(err),
@@ -131,13 +203,51 @@ func (api *API) createMCPServerConfig(rw http.ResponseWriter, r *http.Request) {
 				})
 				return
 			}
-			req.OAuth2ClientID = result.clientID
-			req.OAuth2ClientSecret = result.clientSecret
-			req.OAuth2AuthURL = result.authURL
-			req.OAuth2TokenURL = result.tokenURL
-			if req.OAuth2Scopes == "" {
-				req.OAuth2Scopes = result.scopes
+
+			// Determine scopes: use the request value if provided,
+			// otherwise fall back to the discovered value.
+			oauth2Scopes := strings.TrimSpace(req.OAuth2Scopes)
+			if oauth2Scopes == "" {
+				oauth2Scopes = result.scopes
 			}
+
+			// Update the record with discovered OAuth2 credentials.
+			updated, err := api.Database.UpdateMCPServerConfig(ctx, database.UpdateMCPServerConfigParams{
+				ID:                      inserted.ID,
+				DisplayName:             inserted.DisplayName,
+				Slug:                    inserted.Slug,
+				Description:             inserted.Description,
+				IconURL:                 inserted.IconURL,
+				Transport:               inserted.Transport,
+				Url:                     inserted.Url,
+				AuthType:                inserted.AuthType,
+				OAuth2ClientID:          result.clientID,
+				OAuth2ClientSecret:      result.clientSecret,
+				OAuth2ClientSecretKeyID: sql.NullString{},
+				OAuth2AuthURL:           result.authURL,
+				OAuth2TokenURL:          result.tokenURL,
+				OAuth2Scopes:            oauth2Scopes,
+				APIKeyHeader:            inserted.APIKeyHeader,
+				APIKeyValue:             inserted.APIKeyValue,
+				APIKeyValueKeyID:        inserted.APIKeyValueKeyID,
+				CustomHeaders:           inserted.CustomHeaders,
+				CustomHeadersKeyID:      inserted.CustomHeadersKeyID,
+				ToolAllowList:           inserted.ToolAllowList,
+				ToolDenyList:            inserted.ToolDenyList,
+				Availability:            inserted.Availability,
+				Enabled:                 inserted.Enabled,
+				UpdatedBy:               apiKey.UserID,
+			})
+			if err != nil {
+				httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
+					Message: "Failed to update MCP server config with OAuth2 credentials.",
+					Detail:  err.Error(),
+				})
+				return
+			}
+
+			httpapi.Write(ctx, rw, http.StatusCreated, convertMCPServerConfig(updated))
+			return
 		} else if req.OAuth2ClientID == "" || req.OAuth2AuthURL == "" || req.OAuth2TokenURL == "" {
 			// Partial manual config: all three fields are required together.
 			httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{
@@ -509,6 +509,141 @@ func TestMCPServerConfigsOAuth2AutoDiscovery(t *testing.T) {
 		require.Equal(t, "read write", created.OAuth2Scopes)
 	})

+	// Regression test: verify that during dynamic client registration
+	// the redirect_uris sent to the authorization server contain the
+	// real config UUID, NOT the literal string "{id}".  Before the
+	// fix, the callback URL was built before the config row existed,
+	// so it contained "{id}" literally, which caused "redirect URIs
+	// not approved" errors when the user later tried to connect.
+	t.Run("RedirectURIContainsRealConfigID", func(t *testing.T) {
+		t.Parallel()
+
+		ctx := testutil.Context(t, testutil.WaitLong)
+
+		// Buffered channel so the handler never blocks.
+		registeredRedirectURI := make(chan string, 1)
+
+		// Stand up a mock auth server that captures the redirect_uris
+		// from the RFC 7591 Dynamic Client Registration request.
+		authServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+			switch r.URL.Path {
+			case "/.well-known/oauth-authorization-server":
+				w.Header().Set("Content-Type", "application/json")
+				_, _ = w.Write([]byte(`{
+					"issuer": "` + "http://" + r.Host + `",
+					"authorization_endpoint": "` + "http://" + r.Host + `/authorize",
+					"token_endpoint": "` + "http://" + r.Host + `/token",
+					"registration_endpoint": "` + "http://" + r.Host + `/register",
+					"response_types_supported": ["code"],
+					"scopes_supported": ["read", "write"]
+				}`))
+			case "/register":
+				if r.Method != http.MethodPost {
+					http.Error(w, "method not allowed", http.StatusMethodNotAllowed)
+					return
+				}
+
+				// Decode the registration body and capture redirect_uris.
+				var body map[string]interface{}
+				if err := json.NewDecoder(r.Body).Decode(&body); err != nil {
+					http.Error(w, "bad json", http.StatusBadRequest)
+					return
+				}
+				if uris, ok := body["redirect_uris"].([]interface{}); ok && len(uris) > 0 {
+					if uri, ok := uris[0].(string); ok {
+						registeredRedirectURI <- uri
+					}
+				}
+
+				w.Header().Set("Content-Type", "application/json")
+				w.WriteHeader(http.StatusCreated)
+				_, _ = w.Write([]byte(`{
+					"client_id": "test-client-id",
+					"client_secret": "test-client-secret"
+				}`))
+			default:
+				http.NotFound(w, r)
+			}
+		}))
+		t.Cleanup(authServer.Close)
+
+		// Stand up a mock MCP server that returns RFC 9728 Protected
+		// Resource Metadata pointing to the auth server.
+		mcpServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+			if r.URL.Path == "/.well-known/oauth-protected-resource" {
+				w.Header().Set("Content-Type", "application/json")
+				_, _ = w.Write([]byte(`{
+					"resource": "` + "http://" + r.Host + `",
+					"authorization_servers": ["` + authServer.URL + `"]
+				}`))
+				return
+			}
+			http.NotFound(w, r)
+		}))
+		t.Cleanup(mcpServer.Close)
+
+		client := newMCPClient(t)
+		_ = coderdtest.CreateFirstUser(t, client)
+
+		// Create config with auth_type=oauth2 but no OAuth2 fields to
+		// trigger auto-discovery and dynamic client registration.
+		created, err := client.CreateMCPServerConfig(ctx, codersdk.CreateMCPServerConfigRequest{
+			DisplayName:   "Redirect URI Test",
+			Slug:          "redirect-uri-test",
+			Transport:     "streamable_http",
+			URL:           mcpServer.URL + "/v1/mcp",
+			AuthType:      "oauth2",
+			Availability:  "default_on",
+			Enabled:       true,
+			ToolAllowList: []string{},
+			ToolDenyList:  []string{},
+		})
+		require.NoError(t, err)
+		require.Equal(t, "test-client-id", created.OAuth2ClientID)
+		require.True(t, created.HasOAuth2Secret)
+
+		// The registration request has already completed by the time
+		// CreateMCPServerConfig returns, so the URI is in the channel.
+		var redirectURI string
+		select {
+		case redirectURI = <-registeredRedirectURI:
+		case <-ctx.Done():
+			t.Fatal("timed out waiting for registration redirect URI")
+		}
+
+		// Core assertion: the redirect URI must NOT contain the
+		// literal placeholder "{id}".  Before the fix the callback
+		// URL was built before the database insert, so it had
+		// "{id}" where the UUID should be.
+		require.NotContains(t, redirectURI, "{id}",
+			"redirect URI sent during registration must not contain the literal \"{id}\" placeholder")
+
+		// Verify the redirect URI contains the real config UUID that
+		// was assigned by the database.
+		require.Contains(t, redirectURI, created.ID.String(),
+			"redirect URI should contain the actual config UUID")
+
+		// Sanity-check the full path structure.
+		require.Contains(t, redirectURI,
+			"/api/experimental/mcp/servers/"+created.ID.String()+"/oauth2/callback",
+			"redirect URI should have the expected callback path")
+
+		// Double-check that the ID segment is a valid UUID (not some
+		// other placeholder or malformed value).
+		pathParts := strings.Split(redirectURI, "/")
+		var foundUUID bool
+		for _, part := range pathParts {
+			if _, err := uuid.Parse(part); err == nil {
+				foundUUID = true
+				require.Equal(t, created.ID.String(), part,
+					"UUID in redirect URI path should match created config ID")
+				break
+			}
+		}
+		require.True(t, foundUUID,
+			"redirect URI path should contain a valid UUID segment")
+	})
+
 	t.Run("PartialOAuth2FieldsRejected", func(t *testing.T) {
 		t.Parallel()