fix(mcp): canonical MCP URLs; PAT count mirrors /mcp's user check; one setting read per response (TASK-3302)

Codex round 1 on 7b000405 (gpt-6-luna):

- P2, URL parsing (fixed). The origin's no-path rule read the decoded
  path, so https://host/%2F passed. An empty port (https://host:) was kept,
  HTTPS:// was refused, and host case and default ports were preserved.
  parseMCPURL now judges the path on the escaped path, refuses an empty
  port, and lower-cases scheme and host. It drops a default port (443 for
  https, 80 for http), brackets IPv6 hosts, and trims a trailing slash.
  The result is one canonical spelling, which matters because the MCP URL
  is the OAuth audience, compared byte for byte. A new test pins that
  cloud's PAD_MCP_PUBLIC_URL (with and without a trailing slash) resolves
  to exactly the audience cmd_server.go has always built,
  TrimRight(value, "/"). If U2 switches the audience to the resolved URL,
  no issued Cloud token changes audience.
- P2, PAT count vs /mcp (fixed). /mcp also refuses a PAT whose user row is
  gone. The count now joins users. api_tokens.user_id has a foreign key
  (migration 068), so such a row cannot exist today; the join is
  defensive. A fixture for it was tried: the FK refused the insert, and its
  t.Skipf would have skipped the whole test. So it was removed rather than
  left skipping the assertions next to it.
- P2, orphan OAuth chain counted but not shown (REJECTED with evidence).
  ListUserOAuthConnections does not fail on a chain without an
  oauth_connections row. It takes the documented HasConnection == false
  fallback and lists it (connected_apps.go:190-199); only a DB error
  fails. A new test seeds an orphan chain and asserts the count and the
  page both see it.
- Minor (fixed). The readiness response read the setting twice.
  mcpAvailableWith(on) takes the value already read, so the reported state
  and the predicate cannot disagree across a concurrent toggle.
- Test gap (fixed). The PUT's audit event is now asserted.

Mutants, each BUILD OK and killed by the test aimed at it: default port
kept; host not lower-cased; path judged on the decoded path; empty port
accepted; PUT not audited.

Claude-Session: https://claude.ai/code/session_016PXRzXhokr61CLbbu3Txj9

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

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)
+		}
+	}
+}

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"

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) {

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)
 	}

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))
+	}
+}