diff --git a/internal/httpapi/server.go b/internal/httpapi/server.go index ed4af0692..a257cc551 100644 --- a/internal/httpapi/server.go +++ b/internal/httpapi/server.go @@ -602,6 +602,15 @@ func (s *Server) setupRoutes() { r.Post("/servers/enable_all", s.handleEnableAll) r.Post("/servers/disable_all", s.handleDisableAll) r.Route("/servers/{id}", func(r chi.Router) { + // chi routes on RawPath, so the {id} param arrives percent-encoded. + // Official modelcontextprotocol/registry v0.1 ids are namespace/name, + // so the slash reaches handlers as %2F. Decode it once here so every + // /servers/{id}/* sub-resource handler (tools, logs, restart, approve, + // scan, …) does its exact-match server lookup against the real name + // rather than 404ing on the encoded literal (MCP-1118, same class as + // MCP-1056). Centralising the decode also prevents new sub-resource + // routes from silently reintroducing the gap. + r.Use(decodeServerIDParam) r.Patch("/", s.handlePatchServer) // Partial update server config r.Delete("/", s.handleRemoveServer) // T002: Remove server r.Post("/config-to-secret", s.handleConvertConfigToSecret) // Move a header / env value into OS keyring @@ -2678,10 +2687,10 @@ func (s *Server) handleGetGlobalTools(w http.ResponseWriter, r *http.Request) { // @Failure 500 {object} contracts.ErrorResponse "Internal server error" // @Router /api/v1/servers/{id}/logs [get] func (s *Server) handleGetServerLogs(w http.ResponseWriter, r *http.Request) { - // chi routes on RawPath, so a namespace/name server id (e.g. - // io.github.evidai/polymarket-guard) arrives percent-encoded. Decode it - // before lookup, matching handleAddFromRegistry (MCP-1111 / #598). - serverID := decodePathParam(chi.URLParam(r, "id")) + // The {id} param is percent-decoded by decodeServerIDParam middleware on the + // /servers/{id} subtree (MCP-1118), so a namespace/name server id such as + // io.github.evidai/polymarket-guard reaches us already unescaped. + serverID := chi.URLParam(r, "id") if serverID == "" { s.writeError(w, r, http.StatusBadRequest, "Server ID required") return @@ -4168,6 +4177,26 @@ func decodePathParam(raw string) string { return raw } +// decodeServerIDParam percent-decodes the {id} path param of the /servers/{id} +// route subtree in place. chi matches the parent /servers/{id} segment (and thus +// populates "id") before mounting this subrouter, so the value is available to +// this middleware. Slash-name servers (io.github.owner/repo) arrive as +// io.github.owner%2Frepo; decoding here makes every sub-resource handler's +// exact-match server lookup work without each one having to call +// decodePathParam (MCP-1118). +func decodeServerIDParam(next http.Handler) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if rctx := chi.RouteContext(r.Context()); rctx != nil { + for i, k := range rctx.URLParams.Keys { + if k == "id" { + rctx.URLParams.Values[i] = decodePathParam(rctx.URLParams.Values[i]) + } + } + } + next.ServeHTTP(w, r) + }) +} + func (s *Server) handleAddFromRegistry(w http.ResponseWriter, r *http.Request) { // chi routes on RawPath, so path params arrive percent-encoded. Official // modelcontextprotocol/registry v0.1 ids are namespace/name, so the slash diff --git a/internal/httpapi/server_path_param_decode_test.go b/internal/httpapi/server_path_param_decode_test.go new file mode 100644 index 000000000..f3a2d4d20 --- /dev/null +++ b/internal/httpapi/server_path_param_decode_test.go @@ -0,0 +1,99 @@ +package httpapi + +import ( + "encoding/json" + "net/http" + "net/http/httptest" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.uber.org/zap/zaptest" + + "github.com/smart-mcp-proxy/mcpproxy-go/internal/contracts" +) + +// capturingToolCallsController records the serverID forwarded to the controller +// so the test can assert the {id} path param was percent-decoded before lookup. +type capturingToolCallsController struct { + *MockServerController + gotServerID string +} + +func (c *capturingToolCallsController) GetServerToolCalls(serverID string, _ int) ([]*contracts.ToolCallRecord, error) { + c.gotServerID = serverID + return nil, nil +} + +// TestServerSubresource_SlashServerIDUnescaped reproduces MCP-1118: official +// modelcontextprotocol/registry v0.1 server ids are namespace/name (e.g. +// io.github.owner/repo) and chi routes on RawPath, so the {id} param of every +// /api/v1/servers/{id}/* sub-resource handler arrives percent-encoded +// (io.github.owner%2Frepo). Without decoding, the exact-match server lookup +// targets the literal encoded name and 404s, so the Web UI server-detail Tools +// and Logs tabs (and CLI sub-resources) never load for slash-name servers. +// +// The fix decodes {id} once at the /servers/{id} route subtree, so this asserts +// the decoded name reaches both the management-service path (tools, restart) and +// the controller path (tool-calls). +func TestServerSubresource_SlashServerIDUnescaped(t *testing.T) { + const ( + encoded = "io.github.owner%2Frepo" + decoded = "io.github.owner/repo" + ) + + // dataField unwraps the APIResponse envelope and returns the requested + // string field from the data object. + dataString := func(t *testing.T, body []byte, key string) string { + t.Helper() + var resp struct { + Success bool `json:"success"` + Data map[string]interface{} `json:"data"` + } + require.NoError(t, json.Unmarshal(body, &resp), "body=%s", string(body)) + require.True(t, resp.Success, "request should succeed; body=%s", string(body)) + v, _ := resp.Data[key].(string) + return v + } + + t.Run("GET /tools echoes decoded server name", func(t *testing.T) { + logger := zaptest.NewLogger(t).Sugar() + server := NewServer(&MockServerController{}, logger, nil) + + req := httptest.NewRequest(http.MethodGet, "/api/v1/servers/"+encoded+"/tools", http.NoBody) + w := httptest.NewRecorder() + server.ServeHTTP(w, req) + + require.Equal(t, http.StatusOK, w.Code, "body=%s", w.Body.String()) + assert.Equal(t, decoded, dataString(t, w.Body.Bytes(), "server_name"), + "server id must be percent-decoded before lookup") + }) + + t.Run("POST /restart echoes decoded server name", func(t *testing.T) { + logger := zaptest.NewLogger(t).Sugar() + server := NewServer(&MockServerController{}, logger, nil) + + req := httptest.NewRequest(http.MethodPost, "/api/v1/servers/"+encoded+"/restart", http.NoBody) + w := httptest.NewRecorder() + server.ServeHTTP(w, req) + + require.Equal(t, http.StatusOK, w.Code, "body=%s", w.Body.String()) + assert.Equal(t, decoded, dataString(t, w.Body.Bytes(), "server"), + "server id must be percent-decoded before lookup") + }) + + t.Run("GET /tool-calls forwards decoded server name to controller", func(t *testing.T) { + logger := zaptest.NewLogger(t).Sugar() + controller := &capturingToolCallsController{MockServerController: &MockServerController{}} + server := NewServer(controller, logger, nil) + + req := httptest.NewRequest(http.MethodGet, "/api/v1/servers/"+encoded+"/tool-calls", http.NoBody) + w := httptest.NewRecorder() + server.ServeHTTP(w, req) + + require.Equal(t, http.StatusOK, w.Code, "body=%s", w.Body.String()) + assert.Equal(t, decoded, controller.gotServerID, + "server id must be percent-decoded before reaching the controller") + assert.Equal(t, decoded, dataString(t, w.Body.Bytes(), "server_name")) + }) +}