diff --git a/coderd/coderd.go b/coderd/coderd.go index ab046242d9..07746322e6 100644 --- a/coderd/coderd.go +++ b/coderd/coderd.go @@ -74,6 +74,7 @@ import ( "github.com/coder/coder/v2/coderd/httpmw" "github.com/coder/coder/v2/coderd/httpmw/loggermw" "github.com/coder/coder/v2/coderd/idpsync" + "github.com/coder/coder/v2/coderd/mcp" "github.com/coder/coder/v2/coderd/metricscache" "github.com/coder/coder/v2/coderd/notifications" "github.com/coder/coder/v2/coderd/oauth2provider" @@ -1106,7 +1107,7 @@ func New(options *Options) *API { // and in the build info response and the AI bridge config, so a runtime // toggle would have to update all three. oauth2ProviderEnabled := api.DeploymentValues.OAuth2.Provider.Enable.Value() - apiKeyMiddleware := httpmw.ExtractAPIKeyMW(httpmw.ExtractAPIKeyConfig{ + apiKeyConfig := httpmw.ExtractAPIKeyConfig{ DB: options.Database, ActivateDormantUser: ActivateDormantUser(options.Logger, &api.Auditor, options.Database), OAuth2Configs: oauthConfigs, @@ -1117,7 +1118,8 @@ func New(options *Options) *API { PostAuthAdditionalHeadersFunc: options.PostAuthAdditionalHeadersFunc, Logger: options.Logger, AccessURL: options.AccessURL, - }) + } + apiKeyMiddleware := httpmw.ExtractAPIKeyMW(apiKeyConfig) // Same as above but it redirects to the login page. apiKeyMiddlewareRedirect := httpmw.ExtractAPIKeyMW(httpmw.ExtractAPIKeyConfig{ DB: options.Database, @@ -1405,13 +1407,32 @@ func New(options *Options) *API { }) r.Route("/mcp", func(r chi.Router) { - r.Use(apiKeyMiddleware) // Providers pin the redirect URI when a session is established, // so the callback URL cannot change for existing sessions without // breaking token refresh and forcing a re-auth. - r.Get("/servers/{mcpServer}/oauth2/callback", api.mcpServerOAuth2Callback) + r.With(apiKeyMiddleware).Get("/servers/{mcpServer}/oauth2/callback", api.mcpServerOAuth2Callback) // MCP HTTP transport endpoint with mandatory authentication. r.Route("/http", func(r chi.Router) { + r.Use(func(next http.Handler) http.Handler { + cfg := apiKeyConfig + cfg.ResourceURI = options.AccessURL.JoinPath(mcp.MCPEndpoint).String() + cfg.ResourceMetadataURL = options.AccessURL.JoinPath("/.well-known/oauth-protected-resource", mcp.MCPEndpoint).String() + canonical := httpmw.ExtractAPIKeyMW(cfg)(next) + cfg.ResourceMetadataURL = options.AccessURL.JoinPath("/.well-known/oauth-protected-resource", mcp.MCPEndpoint+"/").String() + trailingSlash := httpmw.ExtractAPIKeyMW(cfg)(next) + return http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { + // Routing normalizes slashes, but OAuth resource identity + // must not admit aliases or arbitrary descendant paths. + switch r.URL.EscapedPath() { + case mcp.MCPEndpoint: + canonical.ServeHTTP(rw, r) + case mcp.MCPEndpoint + "/": + trailingSlash.ServeHTTP(rw, r) + default: + httpapi.RouteNotFound(rw) + } + }) + }) r.Use( httpmw.RequireOAuth2Provider(oauth2ProviderEnabled), httpmw.RequireExperiment(api.Experiments, codersdk.ExperimentMCPServerHTTP), diff --git a/coderd/httpmw/apikey.go b/coderd/httpmw/apikey.go index 66cb038ce6..5bd731ca04 100644 --- a/coderd/httpmw/apikey.go +++ b/coderd/httpmw/apikey.go @@ -34,8 +34,25 @@ import ( type ( apiKeyContextKey struct{} apiKeyPrecheckedContextKey struct{} + apiKeyDelegationContextKey struct{} ) +type apiKeyDelegation struct { + resourceURI string + apiKeyID string +} + +// WithAPIKeyDelegation binds server-owned, in-process work to the originating +// resource URI and authenticated API key. Never derive these values from HTTP +// headers or query parameters. The original credential must still pass +// validation, including its audience, scopes, and expiry. +func WithAPIKeyDelegation(ctx context.Context, resourceURI, apiKeyID string) context.Context { + return context.WithValue(ctx, apiKeyDelegationContextKey{}, apiKeyDelegation{ + resourceURI: resourceURI, + apiKeyID: apiKeyID, + }) +} + // ValidateAPIKeyConfig holds the settings needed for API key // validation at the top of the request lifecycle. Unlike // ExtractAPIKeyConfig it omits route-specific fields @@ -175,9 +192,17 @@ type ExtractAPIKeyConfig struct { PostAuthAdditionalHeadersFunc func(a rbac.Subject, header http.Header) // AccessURL is the configured access URL for this Coder deployment. - // Used for generating OAuth2 resource metadata URLs in WWW-Authenticate headers. + // It supplies the default OAuth2 resource URI and metadata URL. AccessURL *url.URL + // ResourceURI overrides the OAuth2 audience expected by this route. + // Empty uses the deployment access URL, or the request origin if unset. + ResourceURI string + + // ResourceMetadataURL overrides the OAuth2 discovery URL advertised in + // WWW-Authenticate. Empty uses the deployment's root metadata endpoint. + ResourceMetadataURL string + // Logger is used for logging middleware operations. Logger slog.Logger } @@ -265,6 +290,13 @@ func ValidateAPIKey(ctx context.Context, cfg ValidateAPIKeyConfig, r *http.Reque return nil, valErr } + if delegation, ok := ctx.Value(apiKeyDelegationContextKey{}).(apiKeyDelegation); ok && delegation.apiKeyID != key.ID { + return nil, &ValidateAPIKeyError{ + Code: http.StatusUnauthorized, + Response: codersdk.Response{Message: "Delegated requests must use the credential that authenticated the originating request."}, + } + } + // Log the API key ID for all requests that have a valid key // format and secret, regardless of whether subsequent validation // (expiry, user status, etc.) succeeds. @@ -628,7 +660,7 @@ func ExtractAPIKey(rw http.ResponseWriter, r *http.Request, cfg ExtractAPIKeyCon // Add WWW-Authenticate header for 401/403 responses (RFC 6750 + RFC 9728) if code == http.StatusUnauthorized || code == http.StatusForbidden { - rw.Header().Set("WWW-Authenticate", buildWWWAuthenticateHeader(cfg.AccessURL, r, code, response)) + rw.Header().Set("WWW-Authenticate", buildWWWAuthenticateHeader(cfg.AccessURL, cfg.ResourceMetadataURL, r, code, response)) } httpapi.Write(ctx, rw, code, response) @@ -701,7 +733,11 @@ func ExtractAPIKey(rw http.ResponseWriter, r *http.Request, cfg ExtractAPIKeyCon // Validate OAuth2 provider app token audience (RFC 8707) if applicable. if key.LoginType == database.LoginTypeOAuth2ProviderApp { - if err := validateOAuth2ProviderAppTokenAudience(ctx, cfg.DB, *key, cfg.AccessURL, r); err != nil { + resourceURI := cfg.ResourceURI + if resourceURI == "" { + resourceURI = extractExpectedAudience(cfg.AccessURL, r) + } + if err := validateOAuth2ProviderAppTokenAudience(ctx, cfg.DB, *key, resourceURI); err != nil { // Log the detailed error for debugging but don't expose it to the client. cfg.Logger.Debug(ctx, "oauth2 token audience validation failed", slog.Error(err)) return optionalWrite(http.StatusForbidden, codersdk.Response{ @@ -742,7 +778,7 @@ func ExtractAPIKey(rw http.ResponseWriter, r *http.Request, cfg ExtractAPIKeyCon // validateOAuth2ProviderAppTokenAudience validates that an OAuth2 provider app token // is being used with the correct audience/resource server (RFC 8707). -func validateOAuth2ProviderAppTokenAudience(ctx context.Context, db database.Store, key database.APIKey, accessURL *url.URL, r *http.Request) error { +func validateOAuth2ProviderAppTokenAudience(ctx context.Context, db database.Store, key database.APIKey, expectedAudience string) error { // Get the OAuth2 provider app token to check its audience //nolint:gocritic // OAuth2 system context — audience validation for provider app tokens token, err := db.GetOAuth2ProviderAppTokenByAPIKeyID(dbauthz.AsSystemOAuth2(ctx), key.ID) @@ -755,8 +791,11 @@ func validateOAuth2ProviderAppTokenAudience(ctx context.Context, db database.Sto return nil } - // Extract the expected audience from the access URL - expectedAudience := extractExpectedAudience(accessURL, r) + if delegation, ok := ctx.Value(apiKeyDelegationContextKey{}).(apiKeyDelegation); ok && delegation.apiKeyID == key.ID { + // Server-owned delegation validates the originating resource rather + // than the resource implementing the internal request. + expectedAudience = delegation.resourceURI + } // Normalize both audience values for RFC 3986 compliant comparison normalizedTokenAudience := normalizeAudienceURI(token.Audience.String) @@ -878,7 +917,7 @@ func normalizePathSegments(path string) string { // Test export functions for testing package access // buildWWWAuthenticateHeader constructs RFC 6750 + RFC 9728 compliant WWW-Authenticate header -func buildWWWAuthenticateHeader(accessURL *url.URL, r *http.Request, code int, response codersdk.Response) string { +func buildWWWAuthenticateHeader(accessURL *url.URL, resourceMetadata string, r *http.Request, code int, response codersdk.Response) string { // Use the configured access URL for resource metadata if accessURL == nil { scheme := "https" @@ -893,7 +932,9 @@ func buildWWWAuthenticateHeader(accessURL *url.URL, r *http.Request, code int, r } } - resourceMetadata := accessURL.JoinPath("/.well-known/oauth-protected-resource").String() + if resourceMetadata == "" { + resourceMetadata = accessURL.JoinPath("/.well-known/oauth-protected-resource").String() + } switch code { case http.StatusUnauthorized: @@ -920,8 +961,6 @@ func buildWWWAuthenticateHeader(accessURL *url.URL, r *http.Request, code int, r // extractExpectedAudience determines the expected audience for the current request. // This should match the resource parameter used during authorization. func extractExpectedAudience(accessURL *url.URL, r *http.Request) string { - // For MCP compliance, the audience should be the canonical URI of the resource server - // This typically matches the access URL of the Coder deployment var audience string if accessURL != nil { diff --git a/coderd/httpmw/apikey_test.go b/coderd/httpmw/apikey_test.go index a56b8a825f..5929cb57e3 100644 --- a/coderd/httpmw/apikey_test.go +++ b/coderd/httpmw/apikey_test.go @@ -3,11 +3,13 @@ package httpmw_test import ( "context" "crypto/sha256" + "database/sql" "encoding/json" "fmt" "io" "net/http" "net/http/httptest" + "net/url" "strings" "sync/atomic" "testing" @@ -39,6 +41,131 @@ import ( "github.com/coder/coder/v2/testutil" ) +func TestAPIKeyResource(t *testing.T) { + t.Parallel() + + db, _ := dbtestutil.NewDB(t) + user := dbgen.User(t, db, database.User{}) + app := dbgen.OAuth2ProviderApp(t, db, database.OAuth2ProviderApp{}) + origin, err := url.Parse("https://coder.example.com") + require.NoError(t, err) + const ( + reports = "https://coder.example.com/reports" + jobs = "https://coder.example.com/jobs" + metadata = "https://discovery.example.com/reports" + ) + + for _, tc := range []struct { + name string + audience string + resourceURI string + delegatedResource string + wrongKey bool + invalidSecret bool + missingToken bool + queryToken bool + status int + }{ + {name: "DefaultRoot", audience: origin.String(), status: http.StatusNoContent}, + {name: "DefaultRejectsPathAudience", audience: reports, status: http.StatusForbidden}, + {name: "ReportsResource", audience: reports, resourceURI: reports, status: http.StatusNoContent}, + {name: "JobsResource", audience: jobs, resourceURI: jobs, status: http.StatusNoContent}, + {name: "NormalizedResource", audience: "https://CODER.example.com:443/reports/", resourceURI: reports, status: http.StatusNoContent}, + {name: "ResourceRejectsRoot", audience: origin.String(), resourceURI: reports, status: http.StatusForbidden}, + {name: "ResourceRejectsOtherResource", audience: jobs, resourceURI: reports, status: http.StatusForbidden}, + {name: "ResourceRejectsOtherDeployment", audience: "https://other.example.com/reports", resourceURI: reports, status: http.StatusForbidden}, + {name: "ReportsDelegation", audience: reports, delegatedResource: reports, status: http.StatusNoContent}, + {name: "JobsDelegation", audience: jobs, delegatedResource: jobs, status: http.StatusNoContent}, + {name: "DelegationOverridesDestinationResource", audience: reports, resourceURI: jobs, delegatedResource: reports, status: http.StatusNoContent}, + {name: "DelegationRejectsDestinationAudience", audience: jobs, resourceURI: jobs, delegatedResource: reports, status: http.StatusForbidden}, + {name: "DelegationRejectsRoot", audience: origin.String(), delegatedResource: reports, status: http.StatusForbidden}, + {name: "DelegationRejectsOtherResource", audience: jobs, delegatedResource: reports, status: http.StatusForbidden}, + {name: "DelegationRejectsOtherDeployment", audience: "https://other.example.com/reports", delegatedResource: reports, status: http.StatusForbidden}, + {name: "DelegationRejectsSubstitutedKey", audience: reports, delegatedResource: reports, wrongKey: true, status: http.StatusUnauthorized}, + {name: "DelegationRejectsInvalidSecret", audience: reports, delegatedResource: reports, invalidSecret: true, status: http.StatusUnauthorized}, + {name: "DelegationRequiresCredential", audience: reports, delegatedResource: reports, missingToken: true, status: http.StatusUnauthorized}, + {name: "DelegationRejectsQueryToken", audience: reports, delegatedResource: reports, queryToken: true, status: http.StatusUnauthorized}, + {name: "ResourceDiscoveryWithoutCredential", resourceURI: reports, missingToken: true, status: http.StatusUnauthorized}, + {name: "UnboundLegacyResource", resourceURI: reports, status: http.StatusNoContent}, + {name: "UnboundLegacyDelegation", delegatedResource: reports, status: http.StatusNoContent}, + {name: "UnboundLegacyRejectsSubstitutedKey", delegatedResource: reports, wrongKey: true, status: http.StatusUnauthorized}, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + key, token := dbgen.APIKey(t, db, database.APIKey{ + UserID: user.ID, LoginType: database.LoginTypeOAuth2ProviderApp, + Scopes: database.APIKeyScopes{database.ApiKeyScopeUserRead}, + }) + dbgen.OAuth2ProviderAppToken(t, db, database.OAuth2ProviderAppToken{ + AppID: app.ID, UserID: user.ID, APIKeyID: key.ID, HashPrefix: []byte(key.ID), + Audience: sql.NullString{String: tc.audience, Valid: tc.audience != ""}, + }) + delegatedID := key.ID + if tc.wrongKey { + otherKey, _ := dbgen.APIKey(t, db, database.APIKey{UserID: user.ID}) + delegatedID = otherKey.ID + } + if tc.invalidSecret { + token = key.ID + "-" + strings.Repeat("0", 22) + } + + for _, precheck := range []bool{false, true} { + t.Run(fmt.Sprintf("Precheck=%t", precheck), func(t *testing.T) { + t.Parallel() + // Request-controlled resource and delegation hints must have no + // effect, whether or not server-owned delegation is present. + req := httptest.NewRequest(http.MethodGet, "https://untrusted.example.com/api/v2/users/me?resource="+url.QueryEscape(reports), nil) + req.Header.Set("X-Coder-API-Key-Delegation", key.ID) + req.Header.Set("X-Coder-MCP-Delegation", "true") + req.Header.Set("X-Forwarded-Uri", "/reports") + req.Header.Set("X-Forwarded-Host", "coder.example.com") + req.Header.Set("Referer", reports) + if tc.queryToken { + q := req.URL.Query() + q.Set("access_token", token) + req.URL.RawQuery = q.Encode() + } else if !tc.missingToken { + req.Header.Set("Authorization", "Bearer "+token) + } + if tc.delegatedResource != "" { + req = req.WithContext(httpmw.WithAPIKeyDelegation(req.Context(), tc.delegatedResource, delegatedID)) + } + originalURL := req.URL.String() + handler := httpmw.ExtractAPIKeyMW(httpmw.ExtractAPIKeyConfig{ + DB: db, AccessURL: origin, Logger: testutil.Logger(t), + ResourceURI: tc.resourceURI, ResourceMetadataURL: metadata, + })(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + assert.Equal(t, originalURL, r.URL.String()) + assert.Equal(t, key.ID, httpmw.APIKey(r).ID) + actor := httpmw.UserAuthorization(r.Context()) + assert.Equal(t, user.ID.String(), actor.ID) + assert.Equal(t, key.ScopeSet(), actor.Scope) + w.WriteHeader(http.StatusNoContent) + })) + if precheck { + handler = httpmw.PrecheckAPIKey(httpmw.ValidateAPIKeyConfig{DB: db, Logger: testutil.Logger(t)})(handler) + } + rw := httptest.NewRecorder() + handler.ServeHTTP(rw, req) + require.Equal(t, tc.status, rw.Code) + if tc.wrongKey { + var response codersdk.Response + require.NoError(t, json.NewDecoder(rw.Body).Decode(&response)) + require.Equal(t, "Delegated requests must use the credential that authenticated the originating request.", response.Message) + require.NotContains(t, response.Message, key.ID) + require.NotContains(t, response.Message, delegatedID) + require.NotContains(t, rw.Header().Get("WWW-Authenticate"), "expired") + } + require.Equal(t, originalURL, req.URL.String()) + if tc.status != http.StatusNoContent { + require.Contains(t, rw.Header().Get("WWW-Authenticate"), `resource_metadata="`+metadata+`"`) + } + }) + } + }) + } +} + func randomAPIKeyParts() (id string, secret string, hashedSecret []byte) { id, _ = cryptorand.String(10) secret, hashedSecret, _ = apikey.GenerateSecret(22) diff --git a/coderd/httpmw/httpmw_internal_test.go b/coderd/httpmw/httpmw_internal_test.go index bf10f26551..7c99500e47 100644 --- a/coderd/httpmw/httpmw_internal_test.go +++ b/coderd/httpmw/httpmw_internal_test.go @@ -5,6 +5,7 @@ import ( "encoding/json" "net/http" "net/http/httptest" + "net/url" "testing" "github.com/go-chi/chi/v5" @@ -209,7 +210,51 @@ func TestNormalizePathSegments(t *testing.T) { } } -// TestExtractExpectedAudience tests audience extraction from HTTP requests +func TestBuildWWWAuthenticateHeaderResourceMetadata(t *testing.T) { + t.Parallel() + + accessURL, err := url.Parse("https://coder.example.com") + require.NoError(t, err) + for _, tc := range []struct { + path string + metadataURL string + expectedURL string + }{ + {path: "/api/experimental/mcp/http"}, + {path: "/api/experimental/mcp/http/"}, + {path: "/api/experimental/mcp/http/child"}, + {path: "/api/experimental/mcp/http?toolset=chatgpt"}, + {path: "/api/experimental/mcp/http-extra"}, + {path: "/api/v2/users/me"}, + { + path: "/reports", metadataURL: "https://discovery.example.com/reports", + expectedURL: "https://discovery.example.com/reports", + }, + { + path: "/reports/", metadataURL: "https://discovery.example.com/reports/", + expectedURL: "https://discovery.example.com/reports/", + }, + { + path: "/reports?resource_metadata=https://untrusted.example.com", metadataURL: "https://discovery.example.com/reports", + expectedURL: "https://discovery.example.com/reports", + }, + } { + t.Run(tc.path, func(t *testing.T) { + t.Parallel() + req := httptest.NewRequest(http.MethodPost, "https://untrusted.example.com"+tc.path, nil) + expectedURL := tc.expectedURL + if expectedURL == "" { + expectedURL = "https://coder.example.com/.well-known/oauth-protected-resource" + } + for _, code := range []int{http.StatusUnauthorized, http.StatusForbidden} { + header := buildWWWAuthenticateHeader(accessURL, tc.metadataURL, req, code, codersdk.Response{}) + require.Contains(t, header, `resource_metadata="`+expectedURL+`"`) + } + }) + } +} + +// TestExtractExpectedAudience tests audience extraction from HTTP requests. func TestExtractExpectedAudience(t *testing.T) { t.Parallel() @@ -234,6 +279,27 @@ func TestExtractExpectedAudience(t *testing.T) { path: "/v1/users", expected: "https://api.example.com/", }, + { + name: "MCPWithoutOverride", + scheme: "https", + host: "example.com", + path: "/api/experimental/mcp/http", + expected: "https://example.com/", + }, + { + name: "MCPWithTrailingSlashWithoutOverride", + scheme: "https", + host: "example.com", + path: "/api/experimental/mcp/http/", + expected: "https://example.com/", + }, + { + name: "MCPPrefixLookalike", + scheme: "https", + host: "example.com", + path: "/api/experimental/mcp/http-extra", + expected: "https://example.com/", + }, { name: "WithPort", scheme: "http", @@ -256,6 +322,11 @@ func TestExtractExpectedAudience(t *testing.T) { result := extractExpectedAudience(nil, req) assert.Equal(t, tc.expected, result) + + accessURL, err := url.Parse(tc.scheme + "://" + tc.host) + require.NoError(t, err) + req.Host = "untrusted.example.com" + assert.Equal(t, tc.expected, extractExpectedAudience(accessURL, req)) }) } } diff --git a/coderd/mcp/mcp.go b/coderd/mcp/mcp.go index d447003137..b909b6b779 100644 --- a/coderd/mcp/mcp.go +++ b/coderd/mcp/mcp.go @@ -22,8 +22,8 @@ const ( // MCPServerInstructions is the instructions text for the MCP server. MCPServerInstructions = "Coder MCP Server providing workspace and template management tools" - // Used in tests and aibridge. - MCPEndpoint = "/api/experimental/mcp/http" + // MCPEndpoint is the remote MCP HTTP endpoint path. + MCPEndpoint = codersdk.MCPEndpoint ) // Server represents an MCP HTTP server instance diff --git a/coderd/mcp/mcp_e2e_test.go b/coderd/mcp/mcp_e2e_test.go index c4b306e698..03bb9f303a 100644 --- a/coderd/mcp/mcp_e2e_test.go +++ b/coderd/mcp/mcp_e2e_test.go @@ -3,6 +3,7 @@ package mcp_test import ( "context" "crypto/sha256" + "database/sql" "encoding/base64" "encoding/json" "fmt" @@ -15,6 +16,7 @@ import ( "sync/atomic" "testing" + "github.com/go-chi/chi/v5" "github.com/google/uuid" "github.com/modelcontextprotocol/go-sdk/mcp" "github.com/spf13/afero" @@ -27,6 +29,8 @@ import ( "github.com/coder/coder/v2/coderd/coderdtest" "github.com/coder/coder/v2/coderd/database" "github.com/coder/coder/v2/coderd/database/dbfake" + "github.com/coder/coder/v2/coderd/database/dbgen" + "github.com/coder/coder/v2/coderd/database/dbtestutil" mcpserver "github.com/coder/coder/v2/coderd/mcp" "github.com/coder/coder/v2/coderd/oauth2provider/oauth2providertest" "github.com/coder/coder/v2/coderd/rbac" @@ -170,6 +174,118 @@ func TestMCPHTTP_E2E_ClientIntegration(t *testing.T) { } } +func TestMCPHTTP_E2E_CancelLogStream(t *testing.T) { + t.Parallel() + ctx := testutil.Context(t, testutil.WaitLong) + db, _ := dbtestutil.NewDB(t) + logsDB := &mcpLogStreamStore{Store: db, queried: make(chan struct{}, 1)} + outerDone := make(chan struct{}) + type outerContextKey struct{} + coderClient, closer, api := coderdtest.NewWithAPI(t, &coderdtest.Options{ + Database: logsDB, + DeploymentValues: mcpDeploymentValues(t), + APIMiddleware: func(next http.Handler) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == mcpserver.MCPEndpoint { + defer close(outerDone) + r = r.WithContext(context.WithValue(r.Context(), outerContextKey{}, "outer-request-state")) + } + next.ServeHTTP(w, r) + }) + }, + }) + t.Cleanup(func() { _ = closer.Close() }) + user := coderdtest.CreateFirstUser(t, coderClient) + build := dbfake.WorkspaceBuild(t, api.Database, database.WorkspaceTable{ + OrganizationID: user.OrganizationID, OwnerID: user.UserID, + }).Starting().Do().Build + key, token := dbgen.APIKey(t, api.Database, database.APIKey{ + UserID: user.UserID, LoginType: database.LoginTypeOAuth2ProviderApp, + }) + app := dbgen.OAuth2ProviderApp(t, api.Database, database.OAuth2ProviderApp{}) + dbgen.OAuth2ProviderAppToken(t, api.Database, database.OAuth2ProviderAppToken{ + AppID: app.ID, UserID: user.UserID, APIKeyID: key.ID, + Audience: sql.NullString{String: api.AccessURL.String() + mcpserver.MCPEndpoint, Valid: true}, + }) + + innerDone := make(chan struct{}) + cancelInner := make(chan context.CancelFunc, 1) + root := api.RootHandler + api.RootHandler = chi.NewRouter() + api.RootHandler.Use(func(next http.Handler) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == fmt.Sprintf("/api/v2/workspacebuilds/%s/logs", build.ID) { + defer close(innerDone) + assert.Nil(t, r.Context().Value(outerContextKey{}), "outer request values must not reach internal authentication") + // Allow cleanup to release the log follower even if the + // cancellation regression fails. + innerCtx, cancel := context.WithCancel(r.Context()) + defer cancel() + cancelInner <- cancel + r = r.WithContext(innerCtx) + } + next.ServeHTTP(w, r) + }) + }) + api.RootHandler.Mount("/", root) + t.Cleanup(func() { + select { + case cancel := <-cancelInner: + cancel() + default: + } + }) + + // Send a raw stateless MCP call so canceling it disconnects the HTTP + // request without an SDK cancellation notification masking the leak. + body, err := json.Marshal(map[string]any{ + "jsonrpc": "2.0", "id": 1, "method": "tools/call", + "params": map[string]any{ + "name": toolsdk.ToolNameGetWorkspaceBuildLogs, + "arguments": map[string]any{"workspace_build_id": build.ID.String()}, + }, + }) + require.NoError(t, err) + requestCtx, cancel := context.WithCancel(ctx) + defer cancel() + req, err := http.NewRequestWithContext(requestCtx, http.MethodPost, api.AccessURL.String()+mcpserver.MCPEndpoint, strings.NewReader(string(body))) + require.NoError(t, err) + req.Header.Set("Authorization", "Bearer "+token) + req.Header.Set("Content-Type", "application/json") + req.Header.Set("Accept", "application/json, text/event-stream") + req.Header.Set("MCP-Protocol-Version", "2025-03-26") + requestDone := make(chan error, 1) + go func() { + resp, err := coderClient.HTTPClient.Do(req) + if resp != nil { + _ = resp.Body.Close() + } + requestDone <- err + }() + + // The log query runs after WebSocket acceptance and pubsub subscription. + testutil.TryReceive(ctx, t, logsDB.queried) + cancel() + require.ErrorIs(t, testutil.TryReceive(ctx, t, requestDone), context.Canceled) + waitCtx := testutil.Context(t, testutil.WaitShort) + testutil.TryReceive(waitCtx, t, innerDone) + testutil.TryReceive(waitCtx, t, outerDone) +} + +type mcpLogStreamStore struct { + database.Store + queried chan struct{} +} + +func (s *mcpLogStreamStore) GetProvisionerLogsAfterID(ctx context.Context, params database.GetProvisionerLogsAfterIDParams) ([]database.ProvisionerJobLog, error) { + logs, err := s.Store.GetProvisionerLogsAfterID(ctx, params) + select { + case s.queried <- struct{}{}: + default: + } + return logs, err +} + func TestMCPHTTP_E2E_UnauthenticatedAccess(t *testing.T) { t.Parallel() @@ -198,12 +314,120 @@ func TestMCPHTTP_E2E_UnauthenticatedAccess(t *testing.T) { // Verify we get 401 Unauthorized require.Equal(t, http.StatusUnauthorized, resp.StatusCode, "Should get HTTP 401 for unauthenticated access") + require.Contains(t, resp.Header.Get("WWW-Authenticate"), "Bearer ") + require.Contains(t, resp.Header.Get("WWW-Authenticate"), fmt.Sprintf(`resource_metadata=%q`, + api.AccessURL.String()+"/.well-known/oauth-protected-resource"+mcpserver.MCPEndpoint)) + require.Empty(t, resp.Header.Get("Location")) // Also test with MCP client to ensure it handles the error gracefully _, err = newIsolatedMCPClient(ctx, mcpURL, "test-client-unauth", nil) require.Error(t, err, "Should fail during MCP initialization without authentication") } +func TestMCPHTTP_E2E_ResourcePaths(t *testing.T) { + t.Parallel() + client := coderdtest.New(t, &coderdtest.Options{DeploymentValues: mcpDeploymentValues(t)}) + coderdtest.CreateFirstUser(t, client) + type credential struct { + name string + token string + wantStatus int + } + tokens := []credential{ + {name: "Unauthenticated", wantStatus: http.StatusUnauthorized}, + {name: "SessionToken", token: client.SessionToken(), wantStatus: http.StatusOK}, + } + httpClient := &http.Client{CheckRedirect: func(*http.Request, []*http.Request) error { return http.ErrUseLastResponse }} + for _, resource := range []struct { + name string + audience string + wantStatus int + }{ + {name: "RootAudience", audience: client.URL.String(), wantStatus: http.StatusForbidden}, + {name: "MCPAudience", audience: client.URL.String() + mcpserver.MCPEndpoint, wantStatus: http.StatusOK}, + } { + app, secret := oauth2providertest.CreateTestOAuth2App(t, client) + verifier, challenge := oauth2providertest.GeneratePKCE(t) + code := oauth2providertest.AuthorizeOAuth2App(t, client, client.URL.String(), oauth2providertest.AuthorizeParams{ + ClientID: app.ID.String(), ResponseType: "code", RedirectURI: oauth2providertest.TestRedirectURI, + State: oauth2providertest.GenerateState(t), CodeChallenge: challenge, CodeChallengeMethod: "S256", + Resource: resource.audience, + }) + token := oauth2providertest.ExchangeCodeForToken(t, client.URL.String(), oauth2providertest.TokenExchangeParams{ + GrantType: "authorization_code", Code: code, ClientID: app.ID.String(), ClientSecret: secret, + RedirectURI: oauth2providertest.TestRedirectURI, CodeVerifier: verifier, Resource: resource.audience, + }) + tokens = append(tokens, credential{name: resource.name, token: token.AccessToken, wantStatus: resource.wantStatus}) + } + for _, path := range []string{ + "/api//experimental/mcp/http", "/api/experimental//mcp/http", + "/api/experimental/mcp//http", "/api/experimental/mcp/http//", + "/api/experimental/mcp/http/child", "/api/experimental/mcp/http/../http", + "/api/experimental/mcp/%68ttp", "/api/experimental/mcp/http%2f", + } { + t.Run(path, func(t *testing.T) { + t.Parallel() + for _, token := range tokens { + t.Run(token.name, func(t *testing.T) { + t.Parallel() + req, err := http.NewRequestWithContext(t.Context(), http.MethodPost, client.URL.String()+path, + strings.NewReader(`{"jsonrpc":"2.0","id":1,"method":"tools/list","params":{}}`)) + require.NoError(t, err) + req.Header.Set("Content-Type", "application/json") + req.Header.Set("Accept", "application/json, text/event-stream") + if token.token != "" { + req.Header.Set("Authorization", "Bearer "+token.token) + } + resp, err := httpClient.Do(req) + require.NoError(t, err) + _ = resp.Body.Close() + require.Equal(t, http.StatusNotFound, resp.StatusCode) + require.Empty(t, resp.Header.Get("WWW-Authenticate")) + }) + } + }) + } + t.Run("CanonicalPaths", func(t *testing.T) { + t.Parallel() + for _, path := range []string{mcpserver.MCPEndpoint, mcpserver.MCPEndpoint + "/"} { + for _, token := range tokens { + t.Run(path+"/"+token.name, func(t *testing.T) { + t.Parallel() + req, err := http.NewRequestWithContext(t.Context(), http.MethodPost, client.URL.String()+path, + strings.NewReader(`{"jsonrpc":"2.0","id":1,"method":"tools/list","params":{}}`)) + require.NoError(t, err) + req.Header.Set("Content-Type", "application/json") + req.Header.Set("Accept", "application/json, text/event-stream") + if token.token != "" { + req.Header.Set("Authorization", "Bearer "+token.token) + } + resp, err := httpClient.Do(req) + require.NoError(t, err) + _ = resp.Body.Close() + require.Equal(t, token.wantStatus, resp.StatusCode) + if token.wantStatus == http.StatusUnauthorized { + metadataURL := client.URL.String() + "/.well-known/oauth-protected-resource" + path + require.Contains(t, resp.Header.Get("WWW-Authenticate"), fmt.Sprintf(`resource_metadata=%q`, metadataURL)) + var metadata codersdk.OAuth2ProtectedResourceMetadata + testutil.RequireEventuallyResponseOK(testutil.Context(t, testutil.WaitLong), t, metadataURL, &metadata) + require.Equal(t, client.URL.String()+path, metadata.Resource) + } + }) + } + } + }) + t.Run("CallbackStillRequiresAuthentication", func(t *testing.T) { + t.Parallel() + req, err := http.NewRequestWithContext(t.Context(), http.MethodGet, + client.URL.String()+"/api/experimental/mcp/servers/"+uuid.NewString()+"/oauth2/callback", nil) + require.NoError(t, err) + resp, err := httpClient.Do(req) + require.NoError(t, err) + defer resp.Body.Close() + require.Equal(t, http.StatusUnauthorized, resp.StatusCode) + }) +} + func TestMCPHTTP_E2E_ToolWithWorkspace(t *testing.T) { t.Parallel() @@ -520,6 +744,205 @@ func TestMCPHTTP_E2E_OAuth2_EndToEnd(t *testing.T) { t.Logf("OAuth2 Bearer token MCP test successful: Found %d tools", len(tools.Tools)) }) + t.Run("PathQualifiedResource", func(t *testing.T) { + t.Parallel() + ctx := testutil.Context(t, testutil.WaitLong) + mcpURL := api.AccessURL.String() + mcpserver.MCPEndpoint + app, secret := oauth2providertest.CreateTestOAuth2App(t, coderClient) + verifier, challenge := oauth2providertest.GeneratePKCE(t) + code := oauth2providertest.AuthorizeOAuth2App(t, coderClient, api.AccessURL.String(), oauth2providertest.AuthorizeParams{ + ClientID: app.ID.String(), ResponseType: "code", + RedirectURI: oauth2providertest.TestRedirectURI, + State: oauth2providertest.GenerateState(t), + CodeChallenge: challenge, CodeChallengeMethod: "S256", + Resource: mcpURL, + }) + token := oauth2providertest.ExchangeCodeForToken(t, api.AccessURL.String(), oauth2providertest.TokenExchangeParams{ + GrantType: "authorization_code", Code: code, + ClientID: app.ID.String(), ClientSecret: secret, + RedirectURI: oauth2providertest.TestRedirectURI, + CodeVerifier: verifier, Resource: mcpURL, + }) + mcpClient, err := newIsolatedMCPClient(ctx, mcpURL, "path-resource-client", map[string]string{ + "Authorization": "Bearer " + token.AccessToken, + }) + require.NoError(t, err) + t.Cleanup(func() { _ = mcpClient.Close() }) + require.Equal(t, mcpserver.MCPServerName, mcpClient.InitializeResult().ServerInfo.Name) + tools, err := mcpClient.ListTools(ctx, nil) + require.NoError(t, err) + require.NotEmpty(t, tools.Tools) + + result, err := mcpClient.CallTool(ctx, &mcp.CallToolParams{ + Name: toolsdk.ToolNameGetAuthenticatedUser, + }) + require.NoError(t, err) + require.False(t, result.IsError, "REST-backed tool failed: %+v", result.Content) + require.Len(t, result.Content, 1) + text, ok := result.Content[0].(*mcp.TextContent) + require.True(t, ok) + var user codersdk.User + require.NoError(t, json.Unmarshal([]byte(text.Text), &user)) + me, err := coderClient.User(ctx, codersdk.Me) + require.NoError(t, err) + require.Equal(t, me.ID, user.ID) + + // Exercise SDK URL construction through the delegated transport. A + // workspace owner must not select the token-creation endpoint. + before, err := coderClient.Tokens(ctx, codersdk.Me, codersdk.TokensFilter{}) + require.NoError(t, err) + for _, injectedUser := range []string{ + "me/keys/tokens?", "me/keys/tokens#", "me/keys/tokens/../tokens?", + "me%2fkeys%2ftokens%3f", "../users/me/keys/tokens?", ".", "..", + } { + result, err := mcpClient.CallTool(ctx, &mcp.CallToolParams{ + Name: toolsdk.ToolNameCreateWorkspace, + Arguments: map[string]any{ + "user": injectedUser, "name": "unused", + "template_id": uuid.NewString(), "rich_parameters": map[string]string{}, + }, + }) + require.NoError(t, err) + assert.True(t, result.IsError, "injected owner %q must fail", injectedUser) + require.Len(t, result.Content, 1) + text, ok := result.Content[0].(*mcp.TextContent) + require.True(t, ok) + assert.Contains(t, text.Text, "invalid user") + after, err := coderClient.Tokens(ctx, codersdk.Me, codersdk.TokensFilter{}) + require.NoError(t, err) + require.ElementsMatch(t, before, after, "workspace tool must not create API keys") + } + + req, err := http.NewRequestWithContext(ctx, http.MethodGet, api.AccessURL.String()+"/api/v2/users/me", nil) + require.NoError(t, err) + req.Header.Set("Authorization", "Bearer "+token.AccessToken) + // Neither a claimed origin nor a delegation-looking header may + // turn an external REST request into trusted MCP work. + req.Header.Set("X-Coder-MCP-Delegation", "true") + req.Header.Set("X-Forwarded-Uri", mcpserver.MCPEndpoint) + req.Header.Set("Referer", mcpURL) + resp, err := http.DefaultClient.Do(req) + require.NoError(t, err) + defer resp.Body.Close() + require.Equal(t, http.StatusForbidden, resp.StatusCode) + var response codersdk.Response + require.NoError(t, json.NewDecoder(resp.Body).Decode(&response)) + require.Equal(t, "Token audience validation failed", response.Message) + + refreshed := oauth2providertest.ExchangeCodeForToken(t, api.AccessURL.String(), oauth2providertest.TokenExchangeParams{ + GrantType: "refresh_token", RefreshToken: token.RefreshToken, + ClientID: app.ID.String(), ClientSecret: secret, Resource: mcpURL, + }) + refreshedClient, err := newIsolatedMCPClient(ctx, mcpURL, "refreshed-path-resource", map[string]string{ + "Authorization": "Bearer " + refreshed.AccessToken, + }) + require.NoError(t, err) + t.Cleanup(func() { _ = refreshedClient.Close() }) + result, err = refreshedClient.CallTool(ctx, &mcp.CallToolParams{Name: toolsdk.ToolNameGetAuthenticatedUser}) + require.NoError(t, err) + require.False(t, result.IsError) + require.NoError(t, coderClient.RevokeOAuth2Token(ctx, app.ID, secret, refreshed.AccessToken)) + _, err = refreshedClient.CallTool(ctx, &mcp.CallToolParams{Name: toolsdk.ToolNameGetAuthenticatedUser}) + require.Error(t, err, "revoked token must not execute tools") + }) + + t.Run("PathQualifiedAuthorization", func(t *testing.T) { + t.Parallel() + ctx := testutil.Context(t, testutil.WaitLong) + me, err := coderClient.User(ctx, codersdk.Me) + require.NoError(t, err) + orgID := me.OrganizationIDs[0] + memberClient, member := coderdtest.CreateAnotherUser(t, coderClient, orgID) + orgAdminClient, orgAdmin := coderdtest.CreateAnotherUser(t, coderClient, orgID, rbac.ScopedRoleOrgAdmin(orgID)) + otherOrg := dbgen.Organization(t, api.Database, database.Organization{}) + ownWorkspace := dbfake.WorkspaceBuild(t, api.Database, database.WorkspaceTable{ + OrganizationID: orgID, OwnerID: member.ID, + }).Do().Workspace + peerWorkspace := dbfake.WorkspaceBuild(t, api.Database, database.WorkspaceTable{ + OrganizationID: orgID, OwnerID: me.ID, + }).Do().Workspace + otherOrgWorkspace := dbfake.WorkspaceBuild(t, api.Database, database.WorkspaceTable{ + OrganizationID: otherOrg.ID, OwnerID: me.ID, + }).Do().Workspace + + for _, tc := range []struct { + name string + client *codersdk.Client + userID uuid.UUID + scope string + ownAllowed bool + peerAllowed bool + }{ + {name: "Member", client: memberClient, userID: member.ID, scope: "coder:all", ownAllowed: true}, + {name: "OrganizationAdmin", client: orgAdminClient, userID: orgAdmin.ID, scope: "coder:all", ownAllowed: true, peerAllowed: true}, + {name: "UserReadOnlyScope", client: memberClient, userID: member.ID, scope: "user:read"}, + {name: "OwnerReadOnlyScope", client: coderClient, userID: me.ID, scope: "user:read"}, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + ctx := testutil.Context(t, testutil.WaitLong) + mcpURL := api.AccessURL.String() + mcpserver.MCPEndpoint + app, secret := oauth2providertest.CreateTestOAuth2App(t, coderClient) + verifier, challenge := oauth2providertest.GeneratePKCE(t) + code := oauth2providertest.AuthorizeOAuth2App(t, tc.client, api.AccessURL.String(), oauth2providertest.AuthorizeParams{ + ClientID: app.ID.String(), ResponseType: "code", + RedirectURI: oauth2providertest.TestRedirectURI, State: oauth2providertest.GenerateState(t), + CodeChallenge: challenge, CodeChallengeMethod: "S256", Resource: mcpURL, Scope: tc.scope, + }) + token := oauth2providertest.ExchangeCodeForToken(t, api.AccessURL.String(), oauth2providertest.TokenExchangeParams{ + GrantType: "authorization_code", Code: code, ClientID: app.ID.String(), ClientSecret: secret, + RedirectURI: oauth2providertest.TestRedirectURI, CodeVerifier: verifier, Resource: mcpURL, + }) + client, err := newIsolatedMCPClient(ctx, mcpURL, tc.name, map[string]string{ + "Authorization": "Bearer " + token.AccessToken, + }) + require.NoError(t, err) + t.Cleanup(func() { _ = client.Close() }) + result, err := client.CallTool(ctx, &mcp.CallToolParams{Name: toolsdk.ToolNameGetAuthenticatedUser}) + require.NoError(t, err) + require.False(t, result.IsError) + var user codersdk.User + require.NoError(t, json.Unmarshal([]byte(result.Content[0].(*mcp.TextContent).Text), &user)) + require.Equal(t, tc.userID, user.ID) + + if tc.scope == "user:read" { + visible, err := tc.client.Workspaces(ctx, codersdk.WorkspaceFilter{Owner: "me"}) + require.NoError(t, err) + require.NotEmpty(t, visible.Workspaces, "unrestricted credential must see fixture workspaces") + result, err := client.CallTool(ctx, &mcp.CallToolParams{Name: toolsdk.ToolNameListWorkspaces}) + require.NoError(t, err) + require.False(t, result.IsError) + var workspaces []toolsdk.MinimalWorkspace + require.NoError(t, json.Unmarshal([]byte(result.Content[0].(*mcp.TextContent).Text), &workspaces)) + require.Empty(t, workspaces, "OAuth scope must filter workspace visibility even for an Owner") + } + + for _, ws := range []struct { + id uuid.UUID + allowed bool + }{ + {id: ownWorkspace.ID, allowed: tc.ownAllowed}, + {id: peerWorkspace.ID, allowed: tc.peerAllowed}, + {id: otherOrgWorkspace.ID}, + } { + result, err := client.CallTool(ctx, &mcp.CallToolParams{ + Name: toolsdk.ToolNameGetWorkspace, Arguments: map[string]any{"workspace_id": ws.id.String()}, + }) + require.NoError(t, err) + require.Equal(t, !ws.allowed, result.IsError) + text := result.Content[0].(*mcp.TextContent).Text + if ws.allowed { + var workspace codersdk.Workspace + require.NoError(t, json.Unmarshal([]byte(text), &workspace)) + require.Equal(t, ws.id, workspace.ID) + } else { + require.Contains(t, text, "404", "must fail RBAC, not audience validation") + } + } + }) + } + }) + // Test 3: Full OAuth2 Authorization Code Flow with Token Refresh t.Run("OAuth2FullFlowWithTokenRefresh", func(t *testing.T) { t.Parallel() diff --git a/coderd/mcp_http.go b/coderd/mcp_http.go index 0c0fb75765..d3cf8b4003 100644 --- a/coderd/mcp_http.go +++ b/coderd/mcp_http.go @@ -39,9 +39,17 @@ func (api *API) mcpHTTPHandler() http.Handler { }) return } - // Extract the original session token from the request + // Keep tool requests in-process. The private transport carries the + // originating MCP audience without granting external REST access. + transport, closeTransport := newMCPDelegatedTransport(api.Logger, api.RootHandler, api.AccessURL, httpmw.APIKey(r).ID) + defer closeTransport() + // The MCP SDK detaches tool cancellation and waits for in-flight tools + // before returning, so close their transport independently on disconnect. + stopClose := context.AfterFunc(r.Context(), closeTransport) + defer stopClose() authenticatedClient := codersdk.New(api.AccessURL, - codersdk.WithSessionToken(httpmw.APITokenFromRequest(r))) + codersdk.WithSessionToken(httpmw.APITokenFromRequest(r)), + codersdk.WithHTTPClient(&http.Client{Transport: transport})) // Wrap the agent connection function to enforce ActionSSH // on the workspace. Without this check, a user who can read diff --git a/coderd/mcp_transport.go b/coderd/mcp_transport.go new file mode 100644 index 0000000000..edd2dcfb29 --- /dev/null +++ b/coderd/mcp_transport.go @@ -0,0 +1,138 @@ +package coderd + +import ( + "context" + "net" + "net/http" + "net/url" + "path" + "strings" + "sync" + "time" + + "golang.org/x/xerrors" + + "cdr.dev/slog/v3" + "github.com/coder/coder/v2/coderd/httpmw" + "github.com/coder/coder/v2/coderd/mcp" +) + +// mcpDelegatedReadHeaderTimeout bounds header reads on the in-process delegation +// server to satisfy gosec G112. The only client is the local net.Pipe transport, +// so this is not tuned for a slow network client. +const mcpDelegatedReadHeaderTimeout = 10 * time.Second + +// newMCPDelegatedTransport delegates same-origin REST requests over net.Pipe, +// preserving streaming and WebSockets without inheriting caller context values. +// Middleware revalidates the original credential, binds it to apiKeyID, and +// checks its MCP audience. Call cleanup to release the server and connections. +func newMCPDelegatedTransport(logger slog.Logger, handler http.Handler, accessURL *url.URL, apiKeyID string) (http.RoundTripper, func()) { + ctx, cancel := context.WithCancel(context.Background()) + listener := &mcpListener{ctx: ctx, cancel: cancel, connections: make(chan net.Conn)} + server := &http.Server{ + Handler: handler, + ReadHeaderTimeout: mcpDelegatedReadHeaderTimeout, + // Pipe connections carry no TLS, so net/http errors here are handler + // faults; route them to the Coder logger instead of stderr. + ErrorLog: slog.Stdlib(ctx, logger.Named("mcp_delegated_transport"), slog.LevelError), + BaseContext: func(net.Listener) context.Context { + return httpmw.WithAPIKeyDelegation(ctx, accessURL.JoinPath(mcp.MCPEndpoint).String(), apiKeyID) + }, + } + done := make(chan struct{}) + go func() { + defer close(done) + _ = server.Serve(listener) + }() + transport := &http.Transport{DialContext: listener.dialContext} + return &mcpDelegatedTransport{origin: *accessURL, transport: transport}, sync.OnceFunc(func() { + cancel() + transport.CloseIdleConnections() + _ = server.Close() + <-done + }) +} + +type mcpDelegatedTransport struct { + origin url.URL + transport *http.Transport +} + +func (t *mcpDelegatedTransport) RoundTrip(req *http.Request) (*http.Response, error) { + reject := func(message string) (*http.Response, error) { + if req.Body != nil { + _ = req.Body.Close() + } + return nil, xerrors.New(message) + } + // Reject other origins, including redirect targets, so the bearer token + // never leaves the process. + if req.URL.Scheme != t.origin.Scheme || req.URL.Host != t.origin.Host || req.Host != t.origin.Host || req.URL.User != nil { + return reject("MCP tool request must target the Coder deployment") + } + if err := validateMCPDelegatedPath(req.URL.Path); err != nil { + return reject(err.Error()) + } + clone := req.Clone(req.Context()) + // Even for HTTPS deployments, these bytes only travel over net.Pipe. + clone.URL.Scheme = "http" + return t.transport.RoundTrip(clone) +} + +func validateMCPDelegatedPath(requestPath string) error { + // Reject aliases before routing can normalize them into another endpoint. + if path.Clean(requestPath) != strings.TrimSuffix(requestPath, "/") { + return xerrors.New("MCP tool request must use a canonical REST path") + } + if (!strings.HasPrefix(requestPath, "/api/v2/") && !strings.HasPrefix(requestPath, "/api/experimental/")) || + strings.HasPrefix(requestPath, "/api/experimental/mcp/") { + return xerrors.New("MCP tool request must target an internal REST endpoint") + } + return nil +} + +type mcpListener struct { + ctx context.Context + cancel context.CancelFunc + connections chan net.Conn +} + +func (l *mcpListener) dialContext(ctx context.Context, _, _ string) (net.Conn, error) { + client, server := net.Pipe() + // Closing the MCP transport also closes hijacked WebSocket connections, + // which http.Server.Close does not own after the upgrade. + context.AfterFunc(l.ctx, func() { + _ = client.Close() + _ = server.Close() + }) + select { + case <-ctx.Done(): + _ = client.Close() + _ = server.Close() + return nil, ctx.Err() + case <-l.ctx.Done(): + _ = client.Close() + _ = server.Close() + return nil, net.ErrClosed + case l.connections <- server: + return client, nil + } +} + +func (l *mcpListener) Accept() (net.Conn, error) { + select { + case <-l.ctx.Done(): + return nil, net.ErrClosed + case conn := <-l.connections: + return conn, nil + } +} + +func (l *mcpListener) Close() error { + l.cancel() + return nil +} + +func (*mcpListener) Addr() net.Addr { + return &net.TCPAddr{IP: net.IPv4(127, 0, 0, 1)} +} diff --git a/coderd/mcp_transport_bench_internal_test.go b/coderd/mcp_transport_bench_internal_test.go new file mode 100644 index 0000000000..c0f64e9112 --- /dev/null +++ b/coderd/mcp_transport_bench_internal_test.go @@ -0,0 +1,149 @@ +package coderd + +import ( + "context" + "io" + "net/http" + "net/http/httptest" + "net/url" + "runtime" + "sync" + "testing" + + "github.com/stretchr/testify/require" + + "github.com/coder/coder/v2/testutil" +) + +// BenchmarkMCPDelegatedTransport isolates transport overhead with a trivial +// handler, excluding MCP, authentication, database work, and request creation. +func BenchmarkMCPDelegatedTransport(b *testing.B) { + origin := &url.URL{Scheme: "https", Host: "coder.example.com"} + req, err := http.NewRequestWithContext(b.Context(), http.MethodGet, origin.String()+"/api/v2/users/me", nil) + require.NoError(b, err) + logger := testutil.Logger(b) + handler := http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusNoContent) + }) + + b.Run("Reuse", func(b *testing.B) { + transport, closeTransport := newMCPDelegatedTransport(logger, handler, origin, "benchmark-key") + defer closeTransport() + benchmarkMCPNoContent(b, transport, req) + b.ReportAllocs() + for b.Loop() { + benchmarkMCPNoContent(b, transport, req) + } + }) + b.Run("SetupTeardown", func(b *testing.B) { + b.ReportAllocs() + for b.Loop() { + _, closeTransport := newMCPDelegatedTransport(logger, handler, origin, "benchmark-key") + closeTransport() + } + }) + b.Run("SetupFirstCallTeardown", func(b *testing.B) { + b.ReportAllocs() + for b.Loop() { + func() { + transport, closeTransport := newMCPDelegatedTransport(logger, handler, origin, "benchmark-key") + defer closeTransport() + benchmarkMCPNoContent(b, transport, req) + }() + } + }) + b.Run("BufferedRecorderBaseline", func(b *testing.B) { + // This direct buffered call is not production-equivalent: it has no + // streaming, hijacking, HTTP serialization, or delegation isolation. + b.ReportAllocs() + for b.Loop() { + recorder := httptest.NewRecorder() + handler.ServeHTTP(recorder, req) + resp := recorder.Result() + if err := resp.Body.Close(); err != nil { + b.Fatal(err) + } + if resp.StatusCode != http.StatusNoContent { + b.Fatalf("unexpected status: %d", resp.StatusCode) + } + } + }) +} + +func benchmarkMCPNoContent(b *testing.B, transport http.RoundTripper, req *http.Request) { + resp, err := transport.RoundTrip(req) + if err != nil { + b.Fatal(err) + } + if err := resp.Body.Close(); err != nil { + b.Fatal(err) + } + if resp.StatusCode != http.StatusNoContent { + b.Fatalf("unexpected status: %d", resp.StatusCode) + } +} + +// BenchmarkMCPDelegatedTransportActiveStreams measures one cohort per iteration, +// not per-request latency. Run standalone with -run '^$' -benchtime=1x so global +// goroutine counts are not obscured by other tests. Each stream owns a transport, +// modeling separate outer MCP requests rather than one shared connection pool. +func BenchmarkMCPDelegatedTransportActiveStreams(b *testing.B) { + for b.Loop() { + benchmarkMCPActiveStreamCohort(b) + } +} + +func benchmarkMCPActiveStreamCohort(b *testing.B) { + const streams = 1024 + ctx, cancel := context.WithTimeout(b.Context(), testutil.WaitLong) + defer cancel() + origin := &url.URL{Scheme: "https", Host: "coder.example.com"} + req, err := http.NewRequestWithContext(ctx, http.MethodGet, origin.String()+"/api/v2/logs", nil) + require.NoError(b, err) + logger := testutil.Logger(b) + baseline := runtime.NumGoroutine() + completed := make(chan struct{}, streams) + handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + defer func() { completed <- struct{}{} }() + w.Header().Set("Content-Type", "text/event-stream") + _, _ = io.WriteString(w, "data: ready\n\n") + w.(http.Flusher).Flush() + <-r.Context().Done() + }) + closers := make([]func(), 0, streams) + cleanup := sync.OnceFunc(func() { + for _, closeTransport := range closers { + closeTransport() + } + }) + defer cleanup() + for range streams { + transport, closeTransport := newMCPDelegatedTransport(logger, handler, origin, "benchmark-key") + closers = append(closers, closeTransport) + //nolint:bodyclose // Bodies are closed with their transports in cleanup. + resp, err := transport.RoundTrip(req) + require.NoError(b, err) + closers[len(closers)-1] = func() { + closeTransport() + _ = resp.Body.Close() + } + require.Equal(b, http.StatusOK, resp.StatusCode) + var first [len("data: ready\n\n")]byte + _, err = io.ReadFull(resp.Body, first[:]) + require.NoError(b, err) + } + active := runtime.NumGoroutine() + cleanup() + for range streams { + testutil.TryReceive(ctx, b, completed) + } + // Connection goroutines can exit after Server.Close returns. Report rather + // than assert the process-wide baseline, which unrelated work can change. + for runtime.NumGoroutine() > baseline && ctx.Err() == nil { + runtime.Gosched() + } + b.ReportMetric(float64(streams), "streams") + b.ReportMetric(float64(baseline), "goroutines-baseline") + b.ReportMetric(float64(active), "goroutines-active") + b.ReportMetric(float64(runtime.NumGoroutine()), "goroutines-cleaned") +} diff --git a/coderd/mcp_transport_internal_test.go b/coderd/mcp_transport_internal_test.go new file mode 100644 index 0000000000..dbdcd88c4b --- /dev/null +++ b/coderd/mcp_transport_internal_test.go @@ -0,0 +1,227 @@ +package coderd + +import ( + "context" + "database/sql" + "io" + "net/http" + "net/url" + "strings" + "sync/atomic" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/coder/coder/v2/coderd/database" + "github.com/coder/coder/v2/coderd/database/dbgen" + "github.com/coder/coder/v2/coderd/database/dbtestutil" + "github.com/coder/coder/v2/coderd/database/dbtime" + "github.com/coder/coder/v2/coderd/httpmw" + "github.com/coder/coder/v2/coderd/mcp" + "github.com/coder/coder/v2/codersdk" + "github.com/coder/coder/v2/testutil" + "github.com/coder/websocket" +) + +func TestMCPTransportAuthentication(t *testing.T) { + t.Parallel() + db, _ := dbtestutil.NewDB(t) + user := dbgen.User(t, db, database.User{}) + app := dbgen.OAuth2ProviderApp(t, db, database.OAuth2ProviderApp{}) + origin, err := url.Parse("https://coder.example.com") + require.NoError(t, err) + handler := httpmw.PrecheckAPIKey(httpmw.ValidateAPIKeyConfig{DB: db})( + httpmw.ExtractAPIKeyMW(httpmw.ExtractAPIKeyConfig{DB: db, AccessURL: origin, Logger: testutil.Logger(t)})( + http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + assert.Equal(t, user.ID.String(), httpmw.UserAuthorization(r.Context()).ID) + w.WriteHeader(http.StatusNoContent) + }))) + + for _, tc := range []struct { + name string + audience string + status int + expired bool + deleted bool + wrongKeyID bool + }{ + {name: "MCPAudience", audience: origin.String() + mcp.MCPEndpoint, status: http.StatusNoContent}, + {name: "TrailingSlash", audience: origin.String() + mcp.MCPEndpoint + "/", status: http.StatusNoContent}, + {name: "UnboundLegacyToken", status: http.StatusNoContent}, + {name: "RootAudience", audience: origin.String(), status: http.StatusForbidden}, + {name: "OtherDeployment", audience: "https://other.example.com" + mcp.MCPEndpoint, status: http.StatusForbidden}, + {name: "WrongKey", audience: origin.String() + mcp.MCPEndpoint, wrongKeyID: true, status: http.StatusUnauthorized}, + {name: "Expired", audience: origin.String() + mcp.MCPEndpoint, expired: true, status: http.StatusUnauthorized}, + {name: "Deleted", audience: origin.String() + mcp.MCPEndpoint, deleted: true, status: http.StatusUnauthorized}, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + ctx := testutil.Context(t, testutil.WaitLong) + key, token := dbgen.APIKey(t, db, database.APIKey{UserID: user.ID, LoginType: database.LoginTypeOAuth2ProviderApp}) + dbgen.OAuth2ProviderAppToken(t, db, database.OAuth2ProviderAppToken{ + AppID: app.ID, UserID: user.ID, APIKeyID: key.ID, HashPrefix: []byte(key.ID), + Audience: sql.NullString{String: tc.audience, Valid: tc.audience != ""}, + }) + delegatedID := key.ID + if tc.wrongKeyID { + delegatedID = "other-key" + } + transport, closeTransport := newMCPDelegatedTransport(testutil.Logger(t), handler, origin, delegatedID) + t.Cleanup(closeTransport) + client := codersdk.New(origin, codersdk.WithSessionToken(token), codersdk.WithHTTPClient(&http.Client{Transport: transport})) + + if tc.expired || tc.deleted { + // Validate once before mutation, then reuse the same private + // transport. Internal calls must not cache authentication. + resp, err := client.Request(ctx, http.MethodGet, "/api/v2/users/me", nil) + require.NoError(t, err) + require.NoError(t, resp.Body.Close()) + require.Equal(t, http.StatusNoContent, resp.StatusCode) + if tc.deleted { + require.NoError(t, db.DeleteAPIKeyByID(ctx, key.ID)) + } else { + require.NoError(t, db.UpdateAPIKeyByID(ctx, database.UpdateAPIKeyByIDParams{ + ID: key.ID, LastUsed: key.LastUsed, ExpiresAt: dbtime.Now().Add(-time.Hour), IPAddress: key.IPAddress, + })) + } + } + resp, err := client.Request(ctx, http.MethodGet, "/api/v2/users/me", nil) + require.NoError(t, err) + defer resp.Body.Close() + require.Equal(t, tc.status, resp.StatusCode) + }) + } +} + +func TestMCPTransportIsolation(t *testing.T) { + t.Parallel() + ctx := testutil.Context(t, testutil.WaitLong) + origin, err := url.Parse("https://coder.example.com") + require.NoError(t, err) + type callerContextKey struct{} + handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + assert.Nil(t, r.Context().Value(callerContextKey{}), "caller auth and routing state must not cross the connection") + if r.URL.Path == "/api/v2/redirect" { + http.Redirect(w, r, "https://other.example.com/api/v2/users/me", http.StatusFound) + return + } + w.WriteHeader(http.StatusNoContent) + }) + transport, closeTransport := newMCPDelegatedTransport(testutil.Logger(t), handler, origin, "test-key") + t.Cleanup(closeTransport) + client := &http.Client{Transport: transport} + + for _, target := range []string{ + "https://other.example.com/api/v2/users/me", + "http://coder.example.com/api/v2/users/me", + "https://coder.example.com/oauth2/tokens", + "https://coder.example.com/api/experimental/mcp/http", + "https://coder.example.com/api/experimental//mcp/http", + "https://coder.example.com/api/v2/../../oauth2/tokens", + "https://coder.example.com/api/v2/redirect", + } { + body := &mcpRequestBody{Reader: strings.NewReader("test")} + req, err := http.NewRequestWithContext(ctx, http.MethodPost, target, body) + require.NoError(t, err) + resp, err := client.Do(req) + if resp != nil { + _ = resp.Body.Close() + } + require.Error(t, err, "untrusted destination must not be reachable") + require.True(t, body.closed.Load(), "rejected request body must be closed") + } + req, err := http.NewRequestWithContext(context.WithValue(ctx, callerContextKey{}, "outer-auth"), http.MethodGet, origin.String()+"/api/v2/users/me", nil) + require.NoError(t, err) + resp, err := client.Do(req) + require.NoError(t, err) + require.NoError(t, resp.Body.Close()) + require.Equal(t, http.StatusNoContent, resp.StatusCode) + + closeTransport() + resp, err = client.Do(req) + if resp != nil { + _ = resp.Body.Close() + } + require.Error(t, err, "transport must not outlive its MCP request") +} + +type mcpRequestBody struct { + io.Reader + closed atomic.Bool +} + +func (b *mcpRequestBody) Close() error { + b.closed.Store(true) + return nil +} + +func TestMCPTransportStreaming(t *testing.T) { + t.Parallel() + ctx := testutil.Context(t, testutil.WaitLong) + origin, err := url.Parse("https://coder.example.com") + require.NoError(t, err) + handlerDone := make(chan struct{}) + handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + defer close(handlerDone) + _, _ = io.WriteString(w, "first\n") + w.(http.Flusher).Flush() + <-r.Context().Done() + }) + transport, closeTransport := newMCPDelegatedTransport(testutil.Logger(t), handler, origin, "test-key") + t.Cleanup(closeTransport) + client := &http.Client{Transport: transport} + requestCtx, cancel := context.WithCancel(ctx) + defer cancel() + req, err := http.NewRequestWithContext(requestCtx, http.MethodGet, origin.String()+"/api/v2/logs", nil) + require.NoError(t, err) + resp, err := client.Do(req) + require.NoError(t, err) + defer resp.Body.Close() + first := make([]byte, len("first\n")) + _, err = io.ReadFull(resp.Body, first) + require.NoError(t, err) + require.Equal(t, "first\n", string(first)) + cancel() + _, err = resp.Body.Read(first) + require.Error(t, err) + testutil.TryReceive(ctx, t, handlerDone) +} + +func TestMCPTransportWebSocket(t *testing.T) { + t.Parallel() + ctx := testutil.Context(t, testutil.WaitLong) + origin, err := url.Parse("https://coder.example.com") + require.NoError(t, err) + handlerDone := make(chan struct{}) + handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + defer close(handlerDone) + conn, err := websocket.Accept(w, r, nil) + if !assert.NoError(t, err) { + return + } + defer conn.CloseNow() + kind, data, err := conn.Read(ctx) + if !assert.NoError(t, err) { + return + } + assert.NoError(t, conn.Write(ctx, kind, data)) + _, _, err = conn.Read(ctx) + assert.Error(t, err, "transport cleanup must close hijacked connections") + }) + transport, closeTransport := newMCPDelegatedTransport(testutil.Logger(t), handler, origin, "test-key") + t.Cleanup(closeTransport) + conn, resp, err := websocket.Dial(ctx, "wss://coder.example.com/api/v2/chats/watch", &websocket.DialOptions{HTTPClient: &http.Client{Transport: transport}}) + if resp != nil && resp.Body != nil { + defer resp.Body.Close() + } + require.NoError(t, err) + defer conn.CloseNow() + require.NoError(t, conn.Write(ctx, websocket.MessageText, []byte("ping"))) + _, data, err := conn.Read(ctx) + require.NoError(t, err) + require.Equal(t, "ping", string(data)) + closeTransport() + testutil.TryReceive(ctx, t, handlerDone) +} diff --git a/coderd/oauth2provider/metadata.go b/coderd/oauth2provider/metadata.go index 5a98c4c76f..6f3a4bd43c 100644 --- a/coderd/oauth2provider/metadata.go +++ b/coderd/oauth2provider/metadata.go @@ -3,6 +3,7 @@ package oauth2provider import ( "net/http" "net/url" + "strings" "github.com/coder/coder/v2/coderd/database" "github.com/coder/coder/v2/coderd/database/dbauthz" @@ -51,8 +52,20 @@ func GetAuthorizationServerMetadata(db database.Store, accessURL *url.URL) http. func GetProtectedResourceMetadata(accessURL *url.URL) http.HandlerFunc { return func(rw http.ResponseWriter, r *http.Request) { ctx := r.Context() + resource := accessURL.String() + suffix := strings.TrimPrefix(r.URL.Path, "/.well-known/oauth-protected-resource") + switch suffix { + case "", "/": + case codersdk.MCPEndpoint, codersdk.MCPEndpoint + "/": + resource = accessURL.JoinPath(suffix).String() + default: + // Match before joining paths so malformed or unknown resources + // cannot be normalized into a supported resource (RFC 9728 §3.1). + httpapi.RouteNotFound(rw) + return + } metadata := codersdk.OAuth2ProtectedResourceMetadata{ - Resource: accessURL.String(), + Resource: resource, AuthorizationServers: []string{accessURL.String()}, ScopesSupported: rbac.ExternalScopeNames(), // RFC 6750 Bearer Token methods supported as fallback methods in api key middleware diff --git a/coderd/oauth2provider/metadata_test.go b/coderd/oauth2provider/metadata_test.go index 6ce480e335..db3152f5c0 100644 --- a/coderd/oauth2provider/metadata_test.go +++ b/coderd/oauth2provider/metadata_test.go @@ -119,6 +119,31 @@ func TestGetAuthorizationServerMetadata_DCREnabled(t *testing.T) { } } +func TestOAuth2ProtectedResourceMetadataUnknownPath(t *testing.T) { + t.Parallel() + + client := coderdtest.New(t, nil) + for _, suffix := range []string{ + "/unknown", "/api/v2/users/me", "/api/experimental/mcp/http/unknown", + "/api/experimental/mcp/http-extra", "/../api/experimental/mcp/http", + "/api/experimental/mcp/../mcp/http", "/api//experimental/mcp/http", + "//api/experimental/mcp/http", "/api/experimental/mcp/http//", + "/api/experimental/mcp/%2e%2e/mcp/http", "/%ff", + } { + t.Run(suffix, func(t *testing.T) { + t.Parallel() + ctx := testutil.Context(t, testutil.WaitLong) + req, err := http.NewRequestWithContext(ctx, http.MethodGet, + client.URL.String()+"/.well-known/oauth-protected-resource"+suffix, nil) + require.NoError(t, err) + resp, err := client.HTTPClient.Do(req) + require.NoError(t, err) + defer resp.Body.Close() + require.Equal(t, http.StatusNotFound, resp.StatusCode) + }) + } +} + func TestOAuth2ProtectedResourceMetadata(t *testing.T) { t.Parallel() @@ -135,10 +160,17 @@ func TestOAuth2ProtectedResourceMetadata(t *testing.T) { testutil.RequireEventuallyResponseOK(ctx, t, endpoint, &metadata) // Verify the metadata - require.NotEmpty(t, metadata.Resource) + require.Equal(t, serverURL.String(), metadata.Resource) require.NotEmpty(t, metadata.AuthorizationServers) require.Len(t, metadata.AuthorizationServers, 1) require.Equal(t, metadata.Resource, metadata.AuthorizationServers[0]) + + for _, suffix := range []string{"/api/experimental/mcp/http", "/api/experimental/mcp/http/"} { + var pathMetadata codersdk.OAuth2ProtectedResourceMetadata + testutil.RequireEventuallyResponseOK(ctx, t, endpoint+suffix, &pathMetadata) + require.Equal(t, serverURL.String()+suffix, pathMetadata.Resource) + require.Equal(t, []string{serverURL.String()}, pathMetadata.AuthorizationServers) + } // RFC 6750 bearer tokens are now supported as fallback methods require.Contains(t, metadata.BearerMethodsSupported, "header") require.Contains(t, metadata.BearerMethodsSupported, "query") diff --git a/coderd/oauth2provider/ratelimit_test.go b/coderd/oauth2provider/ratelimit_test.go index 17b17388b4..0db3499595 100644 --- a/coderd/oauth2provider/ratelimit_test.go +++ b/coderd/oauth2provider/ratelimit_test.go @@ -168,7 +168,7 @@ func TestOAuth2RateLimit(t *testing.T) { requireLimited(t, func() *http.Response { uri := baseURL + "/.well-known/oauth-protected-resource/" + uuid.NewString() return doRequest(ctx, t, http.MethodGet, uri, nil) - }, http.StatusOK) + }, http.StatusNotFound) }) t.Run("SeparateBucketPerEndpoint", func(t *testing.T) { diff --git a/codersdk/mcp.go b/codersdk/mcp.go index 9d222dfa1e..af3881cf6d 100644 --- a/codersdk/mcp.go +++ b/codersdk/mcp.go @@ -9,6 +9,9 @@ import ( "github.com/google/uuid" ) +// MCPEndpoint is the remote MCP HTTP endpoint path. +const MCPEndpoint = "/api/experimental/mcp/http" + // MCPServerOAuth2ConnectURL returns the URL the user should visit to // start the OAuth2 flow for an MCP server. The frontend opens this // in a new window/popup. diff --git a/codersdk/organizations.go b/codersdk/organizations.go index bb64af5ec9..e5edd794e3 100644 --- a/codersdk/organizations.go +++ b/codersdk/organizations.go @@ -674,6 +674,10 @@ func (c *Client) CreateWorkspace(ctx context.Context, _ uuid.UUID, user string, // CreateUserWorkspace creates a new workspace for the template specified. func (c *Client) CreateUserWorkspace(ctx context.Context, user string, request CreateWorkspaceRequest) (Workspace, error) { + // Validate before URL resolution can interpret user input as a path or query. + if err := validateWorkspacePathSegment(user); err != nil { + return Workspace{}, xerrors.Errorf("invalid user: %w", err) + } res, err := c.Request(ctx, http.MethodPost, fmt.Sprintf("/api/v2/users/%s/workspaces", user), request) if err != nil { return Workspace{}, err diff --git a/codersdk/toolsdk/toolsdk_test.go b/codersdk/toolsdk/toolsdk_test.go index f9e29c56d2..64d383f44a 100644 --- a/codersdk/toolsdk/toolsdk_test.go +++ b/codersdk/toolsdk/toolsdk_test.go @@ -1135,15 +1135,23 @@ func TestTools(t *testing.T) { tb, err := toolsdk.NewDeps(client) require.NoError(t, err) t.Run("WithoutPreset", func(t *testing.T) { - res, err := testTool(t, toolsdk.CreateWorkspace, tb, toolsdk.CreateWorkspaceArgs{ - User: "me", - TemplateVersionID: r.TemplateVersion.ID.String(), - Name: testutil.GetRandomNameHyphenated(t), - RichParameters: map[string]string{}, - }) - - require.NoError(t, err) - require.NotEmpty(t, res.ID, "expected a workspace ID") + for _, user := range []string{"", codersdk.Me, member.Username, member.ID.String()} { + t.Run(user, func(t *testing.T) { + res, err := testTool(t, toolsdk.CreateWorkspace, tb, toolsdk.CreateWorkspaceArgs{ + User: user, + TemplateVersionID: r.TemplateVersion.ID.String(), + Name: testutil.GetRandomNameHyphenated(t), + RichParameters: map[string]string{}, + }) + require.NoError(t, err) + require.NotEqual(t, uuid.Nil, res.ID) + if user == member.Username || user == member.ID.String() { + require.Equal(t, member.ID, res.OwnerID) + } else { + require.Equal(t, owner.UserID, res.OwnerID) + } + }) + } }) t.Run("WithPreset", func(t *testing.T) { diff --git a/codersdk/workspaces.go b/codersdk/workspaces.go index d928579b39..18fe0b8286 100644 --- a/codersdk/workspaces.go +++ b/codersdk/workspaces.go @@ -651,6 +651,13 @@ func (c *Client) Workspaces(ctx context.Context, filter WorkspaceFilter) (Worksp // WorkspaceByOwnerAndName returns a workspace by the owner's UUID and the workspace's name. func (c *Client) WorkspaceByOwnerAndName(ctx context.Context, owner string, name string, params WorkspaceOptions) (Workspace, error) { + // These identifiers must remain individual path segments during URL resolution. + if err := validateWorkspacePathSegment(owner); err != nil { + return Workspace{}, xerrors.Errorf("invalid workspace owner: %w", err) + } + if err := validateWorkspacePathSegment(name); err != nil { + return Workspace{}, xerrors.Errorf("invalid workspace name: %w", err) + } res, err := c.Request(ctx, http.MethodGet, fmt.Sprintf("/api/v2/users/%s/workspace/%s", owner, name), nil, func(r *http.Request) { q := r.URL.Query() q.Set("include_deleted", fmt.Sprintf("%t", params.IncludeDeleted)) @@ -669,6 +676,15 @@ func (c *Client) WorkspaceByOwnerAndName(ctx context.Context, owner string, name return workspace, ReadBodyAsJSON(res, &workspace) } +// validateWorkspacePathSegment prevents workspace and user identifiers from +// changing the endpoint during URL resolution, without restricting legacy names. +func validateWorkspacePathSegment(identifier string) error { + if identifier == "" || identifier == "." || identifier == ".." || strings.ContainsAny(identifier, `/\%?#`) { + return xerrors.New("must be a non-empty path segment without slashes, escapes, query, fragment, or dot segments") + } + return nil +} + // SplitWorkspaceIdentifier splits an identifier into owner and // workspace name. A bare name defaults the owner to Me ("me"). An // "owner/name" pair is accepted, and identifiers with more than one diff --git a/codersdk/workspaces_test.go b/codersdk/workspaces_test.go index 63cb99e062..6b62e76b4c 100644 --- a/codersdk/workspaces_test.go +++ b/codersdk/workspaces_test.go @@ -19,6 +19,53 @@ import ( "github.com/coder/coder/v2/testutil" ) +func TestWorkspacePathSegments(t *testing.T) { + t.Parallel() + + for _, identifier := range []string{ + codersdk.Me, "alice", "legacy_name", uuid.NewString(), + "me/keys/tokens?", "me/keys/tokens#", "../users/me", "me%2fkeys", + "me%252fkeys", `me\keys`, "", ".", "..", + } { + t.Run(identifier, func(t *testing.T) { + t.Parallel() + valid := identifier == codersdk.Me || identifier == "alice" || identifier == "legacy_name" + if _, err := uuid.Parse(identifier); err == nil { + valid = true + } + origin, err := url.Parse("https://coder.example.com") + require.NoError(t, err) + var calls int + client := codersdk.New(origin, codersdk.WithHTTPClient(&http.Client{ + Transport: testutil.RoundTripperFunc(func(req *http.Request) (*http.Response, error) { + calls++ + assert.Equal(t, origin.Host, req.URL.Host) + assert.Empty(t, req.URL.Fragment) + if req.Method == http.MethodPost { + assert.Equal(t, "/api/v2/users/"+identifier+"/workspaces", req.URL.Path) + assert.Empty(t, req.URL.RawQuery) + } else { + assert.Equal(t, "/api/v2/users/"+identifier+"/workspace/legacy_name", req.URL.Path) + assert.Equal(t, "include_deleted=false", req.URL.RawQuery) + } + return nil, xerrors.New("request reached transport") + }), + })) + _, createErr := client.CreateUserWorkspace(t.Context(), identifier, codersdk.CreateWorkspaceRequest{}) + _, lookupErr := client.WorkspaceByOwnerAndName(t.Context(), identifier, "legacy_name", codersdk.WorkspaceOptions{}) + if valid { + require.ErrorContains(t, createErr, "request reached transport") + require.ErrorContains(t, lookupErr, "request reached transport") + require.Equal(t, 2, calls) + } else { + require.ErrorContains(t, createErr, "invalid user") + require.ErrorContains(t, lookupErr, "invalid workspace owner") + require.Zero(t, calls) + } + }) + } +} + func TestResolveWorkspace(t *testing.T) { t.Parallel() @@ -293,20 +340,26 @@ func TestResolveWorkspace(t *testing.T) { t.Run("InvalidIdentifier", func(t *testing.T) { t.Parallel() - var hits atomic.Int64 - srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { - hits.Add(1) - t.Errorf("unexpected HTTP request for invalid identifier: %s", req.URL.Path) - })) - defer srv.Close() - - u, err := url.Parse(srv.URL) - require.NoError(t, err) - client := codersdk.New(u) - - _, err = client.ResolveWorkspace(t.Context(), "a/b/c") - require.Error(t, err) - require.ErrorContains(t, err, "invalid workspace identifier: \"a/b/c\"") - require.EqualValues(t, 0, hits.Load(), "invalid identifiers should fail before any HTTP request") + for _, identifier := range []string{ + "a/b/c", "me?/workspace", "me#/workspace", "me%3f/workspace", + "../workspace", "./workspace", "workspace?after=1", "workspace#fragment", + "%2e%2e", "me%252fkeys/workspace", `me\keys/workspace`, "..", ".", "", "/workspace", "me/", + } { + t.Run(identifier, func(t *testing.T) { + t.Parallel() + var hits atomic.Int64 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { + hits.Add(1) + t.Errorf("unexpected HTTP request for invalid identifier: %s", req.URL.Path) + })) + t.Cleanup(srv.Close) + u, err := url.Parse(srv.URL) + require.NoError(t, err) + client := codersdk.New(u) + _, err = client.ResolveWorkspace(t.Context(), identifier) + require.ErrorContains(t, err, "invalid workspace") + require.EqualValues(t, 0, hits.Load(), "invalid identifiers should fail before any HTTP request") + }) + } }) } diff --git a/docs/admin/integrations/oauth2-provider/integration-patterns.md b/docs/admin/integrations/oauth2-provider/integration-patterns.md index fe303f2e44..e97a48cfc3 100644 --- a/docs/admin/integrations/oauth2-provider/integration-patterns.md +++ b/docs/admin/integrations/oauth2-provider/integration-patterns.md @@ -121,9 +121,13 @@ Coder enforces PKCE in compliance with the OAuth 2.1 specification, for both pub Coder provides OAuth2 discovery endpoints for programmatic integration: - **Authorization Server Metadata**: `GET /.well-known/oauth-authorization-server` -- **Protected Resource Metadata**: `GET /.well-known/oauth-protected-resource` +- **REST API protected resource metadata**: `GET /.well-known/oauth-protected-resource` +- **MCP protected resource metadata**: `GET /.well-known/oauth-protected-resource/api/experimental/mcp/http` These endpoints return server capabilities and endpoint URLs according to [RFC 8414](https://datatracker.ietf.org/doc/html/rfc8414) and [RFC 9728](https://datatracker.ietf.org/doc/html/rfc9728). +The REST API metadata identifies the deployment URL as its `resource`, while the MCP metadata identifies the full MCP endpoint URL. +Both list the deployment URL in `authorization_servers` and use the same authorization server metadata endpoint. +OAuth2 tokens issued for the MCP resource work with MCP tools. `token_endpoint_auth_methods_supported` lists every method the token endpoint accepts, including `none`. It is not gated on [Dynamic Client Registration](./index.md#dynamic-client-registration), since existing public clients still exchange tokens when new registrations are disabled. diff --git a/docs/ai-coder/mcp-server.md b/docs/ai-coder/mcp-server.md index 353038575e..c5704ed6cc 100644 --- a/docs/ai-coder/mcp-server.md +++ b/docs/ai-coder/mcp-server.md @@ -178,15 +178,16 @@ The MCP server supports two authentication methods: ### OAuth2 (Recommended for Interactive Clients) -MCP clients that support [RFC 9728](https://datatracker.ietf.org/doc/html/rfc9728) -(Protected Resource Metadata) can authenticate automatically using OAuth2. The -server advertises its OAuth2 capabilities via the `WWW-Authenticate` header and -`/.well-known/oauth-protected-resource` endpoint. - +MCP clients that support [RFC 9728](https://datatracker.ietf.org/doc/html/rfc9728) (Protected Resource Metadata) can authenticate automatically using OAuth2. This enables a seamless connect-and-authenticate experience where users sign in through their browser without manually managing tokens. +The MCP endpoint's `WWW-Authenticate` header points to `/.well-known/oauth-protected-resource/api/experimental/mcp/http` through its `resource_metadata` parameter. +This metadata identifies the full MCP endpoint URL as the `resource`, for example, `https://coder.example.com/api/experimental/mcp/http`. + > [!NOTE] > OAuth2 requires `CODER_OAUTH2_PROVIDER_ENABLE=true` on your Coder deployment. +> +> [Dynamic Client Registration (DCR)](../admin/integrations/oauth2-provider/index.md#dynamic-client-registration) is off by default and must be enabled before clients can self-register. ### Session Token (For Programmatic Access) @@ -252,5 +253,6 @@ them for you to invoke, for example as slash commands: ### OAuth2 authentication not working - Ensure your Coder deployment has `CODER_OAUTH2_PROVIDER_ENABLE=true` set +- If your client registers itself automatically, ensure [Dynamic Client Registration](../admin/integrations/oauth2-provider/index.md#dynamic-client-registration) is enabled - Verify your MCP client supports RFC 9728 Protected Resource Metadata - Check that your browser can reach the Coder authorization endpoint diff --git a/site/src/api/typesGenerated.ts b/site/src/api/typesGenerated.ts index 64761a1527..521e720f5e 100644 --- a/site/src/api/typesGenerated.ts +++ b/site/src/api/typesGenerated.ts @@ -6701,6 +6701,12 @@ export interface LoginWithPasswordResponse { readonly session_token: string; } +// From codersdk/mcp.go +/** + * MCPEndpoint is the remote MCP HTTP endpoint path. + */ +export const MCPEndpoint = "/api/experimental/mcp/http"; + // From codersdk/mcp.go /** * MCPServerConfig represents an admin-configured MCP server.