diff --git a/internal/config/mcp.go b/internal/config/mcp.go index 680aeb7fa..0da5c1b00 100644 --- a/internal/config/mcp.go +++ b/internal/config/mcp.go @@ -109,29 +109,45 @@ func (c *Config) ResolveMCPEndpoints() MCPEndpoints { } // parseMCPURL accepts an absolute http(s) URL with a host, no user info, no -// query and no fragment, and returns it without a trailing slash. An origin -// must also have no path; the MCP and auth-server URLs may have one. +// query and no fragment, and returns it in one canonical spelling: scheme +// and host lower-cased, a default port (443 for https, 80 for http) +// dropped, and no trailing slash. The canonical form matters because the +// MCP URL is the OAuth audience, compared byte-for-byte; a value that is +// already canonical (such as cloud's https://mcp.getpad.dev) comes back +// unchanged. An origin must also have no path, judged on the escaped path, +// so an encoded slash (%2F) cannot pass as empty. The MCP and auth-server +// URLs may have a path. func parseMCPURL(raw string, pathAllowed bool) (string, error) { u, err := url.Parse(strings.TrimSpace(raw)) if err != nil { return "", fmt.Errorf("does not parse as a URL") } + scheme := strings.ToLower(u.Scheme) + path := strings.TrimRight(u.EscapedPath(), "/") switch { - case u.Scheme != "http" && u.Scheme != "https": + case scheme != "http" && scheme != "https": return "", fmt.Errorf("the scheme must be http or https") - case u.Host == "" || u.Hostname() == "": + case u.Hostname() == "": return "", fmt.Errorf("it has no host") + case strings.HasSuffix(u.Host, ":"): + return "", fmt.Errorf("it has an empty port") case u.User != nil: return "", fmt.Errorf("it must not carry user info") case u.RawQuery != "" || u.ForceQuery: return "", fmt.Errorf("it must not have a query") case u.Fragment != "": return "", fmt.Errorf("it must not have a fragment") - case !pathAllowed && strings.Trim(u.Path, "/") != "": - return "", fmt.Errorf("an origin must not have a path (got %q)", u.Path) + case !pathAllowed && path != "": + return "", fmt.Errorf("an origin must not have a path (got %q)", u.EscapedPath()) } - out := u.Scheme + "://" + u.Host + strings.TrimRight(u.EscapedPath(), "/") - return out, nil + host := strings.ToLower(u.Hostname()) + if strings.Contains(host, ":") { + host = "[" + host + "]" // IPv6 + } + if port := u.Port(); port != "" && !(scheme == "https" && port == "443") && !(scheme == "http" && port == "80") { + host += ":" + port + } + return scheme + "://" + host + path, nil } // parseMCPEnabledEnv reads PAD_MCP_ENABLED: true/1/yes/on and diff --git a/internal/config/mcp_test.go b/internal/config/mcp_test.go index dc0d90e25..90c6d7c96 100644 --- a/internal/config/mcp_test.go +++ b/internal/config/mcp_test.go @@ -62,6 +62,29 @@ func TestResolveMCPEndpoints(t *testing.T) { name: "an override without an origin is still unavailable", cfg: Config{MCPPublicURL: "https://mcp.example.com"}, resource: "https://mcp.example.com", }, + { + name: "scheme and host are canonicalised, default port dropped", cfg: Config{URL: " HTTPS://Pad.Example.COM:443/ "}, + origin: "https://pad.example.com", resource: "https://pad.example.com/mcp", authServer: "https://pad.example.com", + usable: true, https: true, + }, + { + name: "a non-default port is kept", cfg: Config{URL: "http://pad.lan:8080"}, + origin: "http://pad.lan:8080", resource: "http://pad.lan:8080/mcp", authServer: "http://pad.lan:8080", + usable: true, + }, + { + name: "IPv6 host", cfg: Config{URL: "http://[FD00::1]:7777"}, + origin: "http://[fd00::1]:7777", resource: "http://[fd00::1]:7777/mcp", authServer: "http://[fd00::1]:7777", + usable: true, + }, + { + name: "an encoded slash is a path, so the origin is unusable", cfg: Config{URL: "https://pad.example.com/%2F"}, + problem: "path", + }, + { + name: "an empty port is unusable", cfg: Config{URL: "https://pad.example.com:"}, + problem: "empty port", + }, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { @@ -120,3 +143,17 @@ func TestLoadReadsPadMCPEnabled(t *testing.T) { t.Fatalf("unparseable value: env=%v raw=%q, want nil and the raw value", cfg.MCPEnabledEnv, cfg.MCPEnabledEnvRaw) } } + +// Cloud's OAuth tokens are bound to the audience cmd_server.go has always +// built: strings.TrimRight(PAD_MCP_PUBLIC_URL, "/"). The canonical form must +// be byte-identical for every value cloud has used, or PLAN-2310 U2's switch +// to the resolved URL would invalidate every issued token. +func TestResolveMCPEndpoints_CloudAudienceUnchanged(t *testing.T) { + for _, v := range []string{"https://mcp.getpad.dev", "https://mcp.getpad.dev/"} { + c := Config{PublicURL: "https://app.getpad.dev", MCPPublicURL: v, AuthServerURL: "https://app.getpad.dev"} + e := c.ResolveMCPEndpoints() + if want := strings.TrimRight(v, "/"); e.ResourceURL != want { + t.Errorf("PAD_MCP_PUBLIC_URL=%q resolves to %q, want the historical audience %q", v, e.ResourceURL, want) + } + } +} diff --git a/internal/server/handlers_admin_mcp.go b/internal/server/handlers_admin_mcp.go index 2de3b997c..b0b96022d 100644 --- a/internal/server/handlers_admin_mcp.go +++ b/internal/server/handlers_admin_mcp.go @@ -55,10 +55,17 @@ func (s *Server) mcpSetting() (bool, string) { // a usable public origin configured. A setting that is on without an origin // leaves MCP unavailable; the readiness panel says why. func (s *Server) mcpAvailable() bool { + on, _ := s.mcpSetting() + return s.mcpAvailableWith(on) +} + +// mcpAvailableWith is mcpAvailable for a setting value already read, so a +// caller that also reports the setting reads it once and the two cannot +// disagree across a concurrent toggle. +func (s *Server) mcpAvailableWith(on bool) bool { if s.cloudMode { return true } - on, _ := s.mcpSetting() return on && s.mcpEndpoints.Usable() } @@ -119,7 +126,7 @@ func (s *Server) buildMCPSettingsResponse() (mcpSettingsResponse, error) { switch { case !on: rd.State = "off" - case s.mcpAvailable(): + case s.mcpAvailableWith(on): rd.State = "on" default: rd.State = "blocked" diff --git a/internal/server/handlers_admin_mcp_test.go b/internal/server/handlers_admin_mcp_test.go index 1ca775c82..0f4bae206 100644 --- a/internal/server/handlers_admin_mcp_test.go +++ b/internal/server/handlers_admin_mcp_test.go @@ -4,6 +4,7 @@ import ( "encoding/json" "net/http" "net/http/httptest" + "strings" "testing" "github.com/PerpetualSoftware/pad/internal/config" @@ -64,6 +65,20 @@ func TestAdminMCP_EnableHTTPS(t *testing.T) { if v, _ := srv.store.GetPlatformSetting(settingMCPEnabled); v != "true" { t.Fatalf("stored mcp_enabled = %q, want true", v) } + // Audit-logged as a settings change naming the key. + events, err := srv.store.ListAuditLog(models.AuditLogParams{Action: models.ActionSettingsChanged, Limit: 10}) + if err != nil { + t.Fatal(err) + } + found := false + for _, e := range events { + if strings.Contains(e.Metadata, settingMCPEnabled) { + found = true + } + } + if !found { + t.Fatalf("no settings_changed audit event naming %s among %d events", settingMCPEnabled, len(events)) + } } func TestAdminMCP_HTTPIsPATOnly(t *testing.T) { diff --git a/internal/store/mcp_resume.go b/internal/store/mcp_resume.go index 914c92896..246f1112c 100644 --- a/internal/store/mcp_resume.go +++ b/internal/store/mcp_resume.go @@ -25,16 +25,20 @@ func (s *Store) CountLiveOAuthConnections() (int, error) { } // CountMCPUsablePATs counts personal access tokens that /mcp would accept. -// It applies the same two rules the request path does, in the same way: -// MCPBearerAuth refuses a token with no user_id (a legacy workspace token, -// middleware_mcp_auth.go), and ValidateToken refuses one whose parsed -// expires_at is before now (api_tokens.go). Expiry is compared as a time +// It applies the same three rules the request path does, in the same way: +// MCPBearerAuth refuses a token with no user_id (a legacy workspace token) +// and one whose user row no longer exists (middleware_mcp_auth.go), and +// ValidateToken refuses one whose parsed expires_at is before now +// (api_tokens.go). The user join is defensive: api_tokens.user_id carries a +// foreign key (migration 068), so no such row can exist today, but the +// count must not depend on that to agree with /mcp. Expiry is compared as a time // here too, not as a string in SQL, so the two cannot disagree about a // stored format. Revoking a token deletes its row. func (s *Store) CountMCPUsablePATs() (int, error) { rows, err := s.db.Query(s.q(` - SELECT expires_at FROM api_tokens - WHERE user_id IS NOT NULL AND user_id <> ''`)) + SELECT t.expires_at FROM api_tokens t + JOIN users u ON u.id = t.user_id + WHERE t.user_id IS NOT NULL AND t.user_id <> ''`)) if err != nil { return 0, fmt.Errorf("count mcp-usable personal access tokens: %w", err) } diff --git a/internal/store/mcp_resume_test.go b/internal/store/mcp_resume_test.go index ecda9430e..b6c897343 100644 --- a/internal/store/mcp_resume_test.go +++ b/internal/store/mcp_resume_test.go @@ -73,3 +73,27 @@ func TestCountMCPUsablePATs(t *testing.T) { t.Fatalf("usable PATs = %d, want 1 (live only)", n) } } + +// A live chain with no oauth_connections row is counted, and the Connected +// Apps page still lists it: ListUserOAuthConnections takes a defensive +// fallback for such a chain rather than failing (connected_apps.go). So the +// panel and the page agree about it. +func TestCountLiveOAuthConnections_OrphanChainMatchesPage(t *testing.T) { + t.Parallel() + s := testStore(t) + client := seedClient(t, s, "orphan-client") + user := seedUser(t, s, "orphan@example.com") + seedAccess(t, s, "orphan-chain", client, user, time.Now().UTC(), "{}", "pad:read", true) + + n, err := s.CountLiveOAuthConnections() + if err != nil { + t.Fatal(err) + } + conns, err := s.ListUserOAuthConnections(user) + if err != nil { + t.Fatalf("the page fails on an orphan chain: %v", err) + } + if n != 1 || len(conns) != 1 { + t.Fatalf("count = %d, page lists %d; want both 1", n, len(conns)) + } +}