diff --git a/internal/mcp/dispatch_http.go b/internal/mcp/dispatch_http.go index a7d83abb..7afab0bc 100644 --- a/internal/mcp/dispatch_http.go +++ b/internal/mcp/dispatch_http.go @@ -11,6 +11,7 @@ import ( "net/url" "strings" + "github.com/go-chi/chi/v5" "github.com/mark3labs/mcp-go/mcp" "github.com/PerpetualSoftware/pad/internal/collections" @@ -442,6 +443,49 @@ func (d *HTTPHandlerDispatcher) buildAuthedRequest( // chain treats the call as authenticated. Pulled out so tests can // inspect / decorate it cheaply. func buildHTTPRequest(ctx context.Context, method, urlPath string, body []byte, user *models.User) (*http.Request, error) { + // Strip any inherited chi.RouteCtxKey from the inbound context + // before synthesizing the new request. Without this, every + // production /mcp tool call 404s on the synthesized /api/v1/... + // request because chi's Mux.ServeHTTP short-circuits when it + // detects an existing RouteCtxKey: + // + // // chi/v5/mux.go:71-75 + // rctx, _ := r.Context().Value(RouteCtxKey).(*Context) + // if rctx != nil { + // mx.handler.ServeHTTP(w, r) // bypass fresh routing + // return + // } + // + // chi assumes "if there's already a route context, I'm being + // invoked as a sub-router from a parent — don't reset state." + // That's correct for chi's own Sub() / Mount() patterns, but + // here we're synthesizing a brand-new request that needs to + // route from scratch against the ROOT mux. The stale RouteCtxKey + // from the inbound /mcp request causes chi to skip its + // rctx.Reset() + RoutePath = "/api/v1/..." setup; the route + // table lookup runs against contaminated routing state and + // falls through to chi's default NotFound handler — whose body + // is the literal "404 page not found\n" the production user + // reported on every dispatcher call. + // + // In tests this never fired because Dispatch was always called + // with context.Background() (no RouteCtxKey to inherit). In + // production every call enters via /mcp's chi-routed handler, + // so the contamination is universal. + // + // Setting the value to a typed nil shadows the parent's value: + // chi's `.(*Context)` type assertion on a context.Value of nil + // returns (nil, false), the `rctx != nil` check fails, and + // chi takes the fresh-routing branch as intended. + // + // Critically we DON'T strip pad's own context values + // (WithCurrentUser, WithAPITokenAuth, TokenScopes, + // TokenAllowedWorkspaces) — those are added below / preserved + // from the inbound request and are exactly what the synthesized + // request needs to authenticate as the same user. We only strip + // the chi-specific routing key. + ctx = context.WithValue(ctx, chi.RouteCtxKey, (*chi.Context)(nil)) + var bodyReader io.Reader if len(body) > 0 { bodyReader = bytes.NewReader(body) diff --git a/internal/mcp/dispatch_http_chi_route_ctx_test.go b/internal/mcp/dispatch_http_chi_route_ctx_test.go new file mode 100644 index 00000000..f571e07c --- /dev/null +++ b/internal/mcp/dispatch_http_chi_route_ctx_test.go @@ -0,0 +1,144 @@ +package mcp + +// Regression test for the bug Codex review surfaced (TASK-1075): +// every production /mcp tool call returned 404 from the dispatcher's +// synthesized /api/v1/... request because chi short-circuits routing +// when the context already carries a RouteCtxKey from the parent +// /mcp request. Pre-fix this only manifested in production (real +// chi-routed traffic); existing tests passed because they used +// context.Background(). +// +// This test pins the production-shaped path: a chi router with a +// /mcp route whose handler invokes the dispatcher, which in turn +// synthesizes a /api/v1/workspaces request that MUST reach the +// workspace-list handler and not get short-circuited to chi's +// default NotFound. + +import ( + "context" + "encoding/json" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/go-chi/chi/v5" + + "github.com/PerpetualSoftware/pad/internal/models" +) + +// TestHTTPHandlerDispatcher_StripsChiRouteCtx_ProductionPath wires a +// chi router with the same shape pad uses in production (a /mcp route +// at the top level + an /api/v1/workspaces handler the dispatcher +// would target), then drives traffic through /mcp's handler so the +// dispatcher inherits the chi route context. Pre-fix this test fails +// with a 404. Post-fix it succeeds. +func TestHTTPHandlerDispatcher_StripsChiRouteCtx_ProductionPath(t *testing.T) { + // What we're proving was reached. Set true ONLY by the + // /api/v1/workspaces handler — if the bug regresses, this stays + // false and the test reports the failure mode (the dispatcher + // returned an error envelope because chi 404'd). + apiHit := false + + // Build a chi router that mirrors the production shape: a /mcp + // endpoint at the top + an /api/v1/workspaces endpoint the + // dispatcher synthesizes a request for. We construct the + // dispatcher inside the /mcp handler so it sees the same chi- + // contaminated context production sees. + root := chi.NewRouter() + + root.Get("/api/v1/workspaces", func(w http.ResponseWriter, r *http.Request) { + apiHit = true + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`[{"slug":"test","name":"Test"}]`)) + }) + + root.Post("/mcp", func(w http.ResponseWriter, r *http.Request) { + // Sanity: the inbound request DOES carry a chi RouteCtxKey, + // matching the production scenario. If chi ever changes this + // behavior the test would silently start passing for the wrong + // reason; pin it. + if r.Context().Value(chi.RouteCtxKey) == nil { + t.Fatal("test setup: expected chi.RouteCtxKey in /mcp handler context — chi may have changed routing semantics") + } + + // Build the dispatcher here so it points at the same root + // router (mimicking production where srv is both the chi root + // AND the dispatcher's Handler). + d := &HTTPHandlerDispatcher{ + Handler: root, + UserResolver: fixedUserResolver(&models.User{ID: "u-1", Name: "Tester"}), + } + + // Drive Dispatch with the inbound request's context — this is + // exactly what mcp-go does when invoking a tool handler. + ctx := WithDispatchInput(r.Context(), map[string]any{}) + res, err := d.Dispatch(ctx, []string{"workspace", "list"}, nil) + if err != nil { + t.Errorf("dispatch error: %v", err) + } + if res == nil { + t.Fatal("nil result from dispatcher") + } + if res.IsError { + dumped, _ := json.Marshal(res) + t.Errorf("dispatch returned error envelope (chi 404'd?); full=%s", string(dumped)) + } + // Echo dispatcher result back so the outer test can sanity-check + // the body round-tripped (proves we hit the real handler, not + // a stub upstream). + w.Header().Set("Content-Type", "application/json") + dumped, _ := json.Marshal(res) + _, _ = w.Write(dumped) + }) + + // Drive the production-shaped path: external /mcp POST. + req := httptest.NewRequest("POST", "/mcp", strings.NewReader("{}")) + rec := httptest.NewRecorder() + root.ServeHTTP(rec, req) + + if !apiHit { + t.Fatalf("api/v1/workspaces handler was NOT reached — chi route ctx contamination likely. /mcp response body: %s", rec.Body.String()) + } + if rec.Code != http.StatusOK { + t.Errorf("/mcp returned %d; body=%s", rec.Code, rec.Body.String()) + } + if !strings.Contains(rec.Body.String(), "Test") { + t.Errorf("expected workspace name to round-trip through dispatcher; body=%s", rec.Body.String()) + } +} + +// TestBuildHTTPRequest_StripsChiRouteCtx is the unit-level pin for the +// strip itself: feed a context carrying a chi RouteCtxKey, confirm the +// resulting request's context returns nil for that key. Cheaper to run +// than the integration test above and pinpoints exactly where the +// strip happens if regression hits. +func TestBuildHTTPRequest_StripsChiRouteCtx(t *testing.T) { + // Seed a parent context with a chi RouteCtx (non-nil — matches + // what chi's Mux.ServeHTTP attaches before invoking handlers). + parentRctx := chi.NewRouteContext() + parentRctx.RoutePath = "/mcp" // mimic production state + parent := context.WithValue(context.Background(), chi.RouteCtxKey, parentRctx) + + req, err := buildHTTPRequest(parent, "GET", "/api/v1/workspaces", nil, &models.User{ID: "u"}) + if err != nil { + t.Fatalf("buildHTTPRequest: %v", err) + } + + // chi's check at mux.go:71 type-asserts to (*chi.Context). When the + // strip works correctly, that assertion against our typed-nil + // returns (nil, false) and chi falls through to fresh routing. + got, ok := req.Context().Value(chi.RouteCtxKey).(*chi.Context) + if got != nil { + t.Errorf("request context still carries non-nil chi RouteCtx after strip; got=%+v ok=%v", got, ok) + } + // And the value lookup itself should yield a nil interface (or + // typed-nil) — NOT the parent's non-nil RouteCtx. + if rawValue := req.Context().Value(chi.RouteCtxKey); rawValue != nil { + // typed-nil interface != nil; check via the assertion path + // that chi actually uses. + if rctx, _ := rawValue.(*chi.Context); rctx != nil { + t.Errorf("expected typed-nil after strip; got non-nil RouteCtx %+v", rctx) + } + } +}