From d3c721043318f90e65b9ae525b3bc53608b1c397 Mon Sep 17 00:00:00 2001 From: Virgil Date: Mon, 23 Mar 2026 14:33:35 +0000 Subject: [PATCH] fix(mcp): harden transport auth and workspace prep path validation Co-Authored-By: Virgil --- cmd/mcpcmd/cmd_mcp.go | 19 +++-- go.mod | 3 +- go.sum | 2 - pkg/mcp/agentic/prep.go | 128 +++++++++++++++++++++++-------- pkg/mcp/agentic/prep_test.go | 96 +++++++++++++++++++++++ pkg/mcp/tools_process_ci_test.go | 30 ++++---- pkg/mcp/transport_http.go | 22 ++++-- pkg/mcp/transport_http_test.go | 26 +++++-- 8 files changed, 252 insertions(+), 74 deletions(-) create mode 100644 pkg/mcp/agentic/prep_test.go diff --git a/cmd/mcpcmd/cmd_mcp.go b/cmd/mcpcmd/cmd_mcp.go index 3c2c1c0..3524dd9 100644 --- a/cmd/mcpcmd/cmd_mcp.go +++ b/cmd/mcpcmd/cmd_mcp.go @@ -61,24 +61,23 @@ func AddMCPCommands(root *cli.Command) { } func runServe() error { - // Build MCP service options - var opts []mcp.Option + opts := mcp.Options{} if workspaceFlag != "" { - opts = append(opts, mcp.WithWorkspaceRoot(workspaceFlag)) + opts.WorkspaceRoot = workspaceFlag } else { // Explicitly unrestricted when no workspace specified - opts = append(opts, mcp.WithWorkspaceRoot("")) + opts.Unrestricted = true } - // Register OpenBrain subsystem (direct HTTP to api.lthn.sh) - opts = append(opts, mcp.WithSubsystem(brain.NewDirect())) - - // Register agentic subsystem (workspace prep, agent orchestration) - opts = append(opts, mcp.WithSubsystem(agentic.NewPrep())) + // Register OpenBrain and agentic subsystems + opts.Subsystems = []mcp.Subsystem{ + brain.NewDirect(), + agentic.NewPrep(), + } // Create the MCP service - svc, err := mcp.New(opts...) + svc, err := mcp.New(opts) if err != nil { return cli.Wrap(err, "create MCP service") } diff --git a/go.mod b/go.mod index 8960f2b..e231f69 100644 --- a/go.mod +++ b/go.mod @@ -3,9 +3,9 @@ module forge.lthn.ai/core/mcp go 1.26.0 require ( - dappco.re/go/core v0.4.7 forge.lthn.ai/core/api v0.1.5 forge.lthn.ai/core/cli v0.3.7 + forge.lthn.ai/core/go v0.3.3 forge.lthn.ai/core/go-ai v0.1.12 forge.lthn.ai/core/go-io v0.1.7 forge.lthn.ai/core/go-log v0.0.4 @@ -21,7 +21,6 @@ require ( ) require ( - forge.lthn.ai/core/go v0.3.3 // indirect forge.lthn.ai/core/go-i18n v0.1.7 // indirect forge.lthn.ai/core/go-inference v0.1.6 // indirect github.com/99designs/gqlgen v0.17.88 // indirect diff --git a/go.sum b/go.sum index 77b90fe..cba2eb1 100644 --- a/go.sum +++ b/go.sum @@ -1,5 +1,3 @@ -dappco.re/go/core v0.4.7 h1:KmIA/2lo6rl1NMtLrKqCWfMlUqpDZYH3q0/d10dTtGA= -dappco.re/go/core v0.4.7/go.mod h1:f2/tBZ3+3IqDrg2F5F598llv0nmb/4gJVCFzM5geE4A= forge.lthn.ai/core/api v0.1.5 h1:NwZrcOyBjaiz5/cn0n0tnlMUodi8Or6FHMx59C7Kv2o= forge.lthn.ai/core/api v0.1.5/go.mod h1:PBnaWyOVXSOGy+0x2XAPUFMYJxQ2CNhppia/D06ZPII= forge.lthn.ai/core/cli v0.3.7 h1:1GrbaGg0wDGHr6+klSbbGyN/9sSbHvFbdySJznymhwg= diff --git a/pkg/mcp/agentic/prep.go b/pkg/mcp/agentic/prep.go index d10bdc1..d241e35 100644 --- a/pkg/mcp/agentic/prep.go +++ b/pkg/mcp/agentic/prep.go @@ -25,13 +25,13 @@ import ( // PrepSubsystem provides agentic MCP tools. type PrepSubsystem struct { - forgeURL string - forgeToken string - brainURL string - brainKey string - specsPath string - codePath string - client *http.Client + forgeURL string + forgeToken string + brainURL string + brainKey string + specsPath string + codePath string + client *http.Client } // NewPrep creates an agentic subsystem. @@ -51,13 +51,13 @@ func NewPrep() *PrepSubsystem { } return &PrepSubsystem{ - forgeURL: envOr("FORGE_URL", "https://forge.lthn.ai"), - forgeToken: forgeToken, - brainURL: envOr("CORE_BRAIN_URL", "https://api.lthn.sh"), - brainKey: brainKey, - specsPath: envOr("SPECS_PATH", filepath.Join(home, "Code", "host-uk", "specs")), - codePath: envOr("CODE_PATH", filepath.Join(home, "Code")), - client: &http.Client{Timeout: 30 * time.Second}, + forgeURL: envOr("FORGE_URL", "https://forge.lthn.ai"), + forgeToken: forgeToken, + brainURL: envOr("CORE_BRAIN_URL", "https://api.lthn.sh"), + brainKey: brainKey, + specsPath: envOr("SPECS_PATH", filepath.Join(home, "Code", "host-uk", "specs")), + codePath: envOr("CODE_PATH", filepath.Join(home, "Code")), + client: &http.Client{Timeout: 30 * time.Second}, } } @@ -68,6 +68,42 @@ func envOr(key, fallback string) string { return fallback } +func sanitizeRepoPathSegment(value, field string, allowSubdirs bool) (string, error) { + if strings.TrimSpace(value) != value { + return "", coreerr.E("prepWorkspace", field+" contains whitespace", nil) + } + if value == "" { + return "", nil + } + if strings.Contains(value, "\\") { + return "", coreerr.E("prepWorkspace", field+" contains invalid path separator", nil) + } + + parts := strings.Split(value, "/") + if !allowSubdirs && len(parts) != 1 { + return "", coreerr.E("prepWorkspace", field+" may not contain subdirectories", nil) + } + + for _, part := range parts { + if part == "" || part == "." || part == ".." { + return "", coreerr.E("prepWorkspace", field+" contains invalid path segment", nil) + } + for _, r := range part { + switch { + case r >= 'a' && r <= 'z', + r >= 'A' && r <= 'Z', + r >= '0' && r <= '9', + r == '-' || r == '_' || r == '.': + continue + default: + return "", coreerr.E("prepWorkspace", field+" contains invalid characters", nil) + } + } + } + + return value, nil +} + // Name implements mcp.Subsystem. func (s *PrepSubsystem) Name() string { return "agentic" } @@ -117,20 +153,41 @@ type PrepInput struct { // PrepOutput is the output for agentic_prep_workspace. type PrepOutput struct { - Success bool `json:"success"` - WorkspaceDir string `json:"workspace_dir"` - WikiPages int `json:"wiki_pages"` - SpecFiles int `json:"spec_files"` - Memories int `json:"memories"` - Consumers int `json:"consumers"` - ClaudeMd bool `json:"claude_md"` - GitLog int `json:"git_log_entries"` + Success bool `json:"success"` + WorkspaceDir string `json:"workspace_dir"` + WikiPages int `json:"wiki_pages"` + SpecFiles int `json:"spec_files"` + Memories int `json:"memories"` + Consumers int `json:"consumers"` + ClaudeMd bool `json:"claude_md"` + GitLog int `json:"git_log_entries"` } func (s *PrepSubsystem) prepWorkspace(ctx context.Context, _ *mcp.CallToolRequest, input PrepInput) (*mcp.CallToolResult, PrepOutput, error) { if input.Repo == "" { return nil, PrepOutput{}, coreerr.E("prepWorkspace", "repo is required", nil) } + + repo, err := sanitizeRepoPathSegment(input.Repo, "repo", false) + if err != nil { + return nil, PrepOutput{}, err + } + input.Repo = repo + + planTemplate, err := sanitizeRepoPathSegment(input.PlanTemplate, "plan_template", false) + if err != nil { + return nil, PrepOutput{}, err + } + input.PlanTemplate = planTemplate + + persona := input.Persona + if persona != "" { + persona, err = sanitizeRepoPathSegment(persona, "persona", true) + if err != nil { + return nil, PrepOutput{}, err + } + } + if input.Org == "" { input.Org = "core" } @@ -154,7 +211,9 @@ func (s *PrepSubsystem) prepWorkspace(ctx context.Context, _ *mcp.CallToolReques // 1. Clone repo into src/ and create feature branch srcDir := filepath.Join(wsDir, "src") cloneCmd := exec.CommandContext(ctx, "git", "clone", repoPath, srcDir) - cloneCmd.Run() + if err := cloneCmd.Run(); err != nil { + return nil, PrepOutput{}, coreerr.E("prepWorkspace", "failed to clone repository", err) + } // Create feature branch taskSlug := strings.Map(func(r rune) rune { @@ -170,11 +229,14 @@ func (s *PrepSubsystem) prepWorkspace(ctx context.Context, _ *mcp.CallToolReques taskSlug = taskSlug[:40] } taskSlug = strings.Trim(taskSlug, "-") - branchName := fmt.Sprintf("agent/%s", taskSlug) - - branchCmd := exec.CommandContext(ctx, "git", "checkout", "-b", branchName) - branchCmd.Dir = srcDir - branchCmd.Run() + if taskSlug != "" { + branchName := fmt.Sprintf("agent/%s", taskSlug) + branchCmd := exec.CommandContext(ctx, "git", "checkout", "-b", branchName) + branchCmd.Dir = srcDir + if err := branchCmd.Run(); err != nil { + return nil, PrepOutput{}, coreerr.E("prepWorkspace", "failed to create branch", err) + } + } // Create context dirs inside src/ coreio.Local.EnsureDir(filepath.Join(srcDir, "kb")) @@ -196,8 +258,8 @@ func (s *PrepSubsystem) prepWorkspace(ctx context.Context, _ *mcp.CallToolReques } // Copy persona if specified - if input.Persona != "" { - personaPath := filepath.Join(s.codePath, "core", "agent", "prompts", "personas", input.Persona+".md") + if persona != "" { + personaPath := filepath.Join(s.codePath, "core", "agent", "prompts", "personas", persona+".md") if data, err := coreio.Local.Read(personaPath); err == nil { coreio.Local.Write(filepath.Join(wsDir, "src", "PERSONA.md"), data) } @@ -338,9 +400,9 @@ func (s *PrepSubsystem) writePlanFromTemplate(templateSlug string, variables map Description string `yaml:"description"` Guidelines []string `yaml:"guidelines"` Phases []struct { - Name string `yaml:"name"` - Description string `yaml:"description"` - Tasks []any `yaml:"tasks"` + Name string `yaml:"name"` + Description string `yaml:"description"` + Tasks []any `yaml:"tasks"` } `yaml:"phases"` } diff --git a/pkg/mcp/agentic/prep_test.go b/pkg/mcp/agentic/prep_test.go new file mode 100644 index 0000000..51f3a60 --- /dev/null +++ b/pkg/mcp/agentic/prep_test.go @@ -0,0 +1,96 @@ +// SPDX-License-Identifier: EUPL-1.2 + +package agentic + +import ( + "context" + "strings" + "testing" +) + +func TestSanitizeRepoPathSegment_Good(t *testing.T) { + t.Run("repo", func(t *testing.T) { + value, err := sanitizeRepoPathSegment("go-io", "repo", false) + if err != nil { + t.Fatalf("expected valid repo name, got error: %v", err) + } + if value != "go-io" { + t.Fatalf("expected normalized value, got: %q", value) + } + }) + + t.Run("persona", func(t *testing.T) { + value, err := sanitizeRepoPathSegment("engineering/backend-architect", "persona", true) + if err != nil { + t.Fatalf("expected valid persona path, got error: %v", err) + } + if value != "engineering/backend-architect" { + t.Fatalf("expected persona path, got: %q", value) + } + }) +} + +func TestSanitizeRepoPathSegment_Bad(t *testing.T) { + cases := []struct { + name string + value string + allowPath bool + }{ + {"repo segment traversal", "../repo", false}, + {"repo nested path", "team/repo", false}, + {"plan template traversal", "../secret", false}, + {"persona traversal", "engineering/../../admin", true}, + {"backslash", "org\\repo", false}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + _, err := sanitizeRepoPathSegment(tc.value, tc.name, tc.allowPath) + if err == nil { + t.Fatal("expected error") + } + }) + } +} + +func TestPrepWorkspace_Bad_BadRepoTraversal(t *testing.T) { + s := &PrepSubsystem{codePath: t.TempDir()} + + _, _, err := s.prepWorkspace(context.Background(), nil, PrepInput{Repo: "../repo"}) + if err == nil { + t.Fatal("expected error") + } + if !strings.Contains(strings.ToLower(err.Error()), "repo") { + t.Fatalf("expected repo error, got %q", err) + } +} + +func TestPrepWorkspace_Bad_BadPersonaTraversal(t *testing.T) { + s := &PrepSubsystem{codePath: t.TempDir()} + + _, _, err := s.prepWorkspace(context.Background(), nil, PrepInput{ + Repo: "repo", + Persona: "engineering/../../admin", + }) + if err == nil { + t.Fatal("expected error") + } + if !strings.Contains(strings.ToLower(err.Error()), "persona") { + t.Fatalf("expected persona error, got %q", err) + } +} + +func TestPrepWorkspace_Bad_BadPlanTemplateTraversal(t *testing.T) { + s := &PrepSubsystem{codePath: t.TempDir()} + + _, _, err := s.prepWorkspace(context.Background(), nil, PrepInput{ + Repo: "repo", + PlanTemplate: "../secret", + }) + if err == nil { + t.Fatal("expected error") + } + if !strings.Contains(strings.ToLower(err.Error()), "plan_template") { + t.Fatalf("expected plan template error, got %q", err) + } +} diff --git a/pkg/mcp/tools_process_ci_test.go b/pkg/mcp/tools_process_ci_test.go index 79ebe69..4ab8dbe 100644 --- a/pkg/mcp/tools_process_ci_test.go +++ b/pkg/mcp/tools_process_ci_test.go @@ -6,36 +6,34 @@ import ( "testing" "time" - "dappco.re/go/core" "forge.lthn.ai/core/go-process" + core "forge.lthn.ai/core/go/pkg/core" ) // newTestProcessService creates a real process.Service backed by a core.Core for CI tests. func newTestProcessService(t *testing.T) *process.Service { t.Helper() - c := core.New() - raw, err := process.NewService(process.Options{})(c) + c, err := core.New( + core.WithName("process", process.NewService(process.Options{})), + ) if err != nil { t.Fatalf("Failed to create process service: %v", err) } - svc := raw.(*process.Service) - resultFrom := func(err error) core.Result { - if err != nil { - return core.Result{Value: err} - } - return core.Result{OK: true} + svc, err := core.ServiceFor[*process.Service](c, "process") + if err != nil { + t.Fatalf("Failed to get process service: %v", err) } - c.Service("process", core.Service{ - OnStart: func() core.Result { return resultFrom(svc.OnStartup(context.Background())) }, - OnStop: func() core.Result { return resultFrom(svc.OnShutdown(context.Background())) }, + + if err := svc.OnStartup(context.Background()); err != nil { + t.Fatalf("Failed to start process service: %v", err) + } + t.Cleanup(func() { + _ = svc.OnShutdown(context.Background()) + core.ClearInstance() }) - if r := c.ServiceStartup(context.Background(), nil); !r.OK { - t.Fatalf("Failed to start core: %v", r.Value) - } - t.Cleanup(func() { c.ServiceShutdown(context.Background()) }) return svc } diff --git a/pkg/mcp/transport_http.go b/pkg/mcp/transport_http.go index 85840ad..cd25417 100644 --- a/pkg/mcp/transport_http.go +++ b/pkg/mcp/transport_http.go @@ -8,6 +8,7 @@ import ( "net" "net/http" "os" + "strings" "time" coreerr "forge.lthn.ai/core/go-log" @@ -81,18 +82,27 @@ func (s *Service) ServeHTTP(ctx context.Context, addr string) error { } // withAuth wraps an http.Handler with Bearer token authentication. -// If token is empty, authentication is disabled (passthrough). +// If token is empty, requests are rejected. func withAuth(token string, next http.Handler) http.Handler { - if token == "" { - return next - } return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if strings.TrimSpace(token) == "" { + w.Header().Set("WWW-Authenticate", `Bearer`) + http.Error(w, `{"error":"authentication not configured"}`, http.StatusUnauthorized) + return + } + auth := r.Header.Get("Authorization") - if len(auth) < 7 || auth[:7] != "Bearer " { + if !strings.HasPrefix(auth, "Bearer ") { http.Error(w, `{"error":"missing Bearer token"}`, http.StatusUnauthorized) return } - provided := auth[7:] + + provided := strings.TrimSpace(strings.TrimPrefix(auth, "Bearer ")) + if len(provided) == 0 { + http.Error(w, `{"error":"missing Bearer token"}`, http.StatusUnauthorized) + return + } + if subtle.ConstantTimeCompare([]byte(provided), []byte(token)) != 1 { http.Error(w, `{"error":"invalid token"}`, http.StatusUnauthorized) return diff --git a/pkg/mcp/transport_http_test.go b/pkg/mcp/transport_http_test.go index 1172d82..ec8dc10 100644 --- a/pkg/mcp/transport_http_test.go +++ b/pkg/mcp/transport_http_test.go @@ -157,19 +157,35 @@ func TestWithAuth_Bad_MissingToken(t *testing.T) { } } -func TestWithAuth_Good_EmptyTokenPassthrough(t *testing.T) { +func TestWithAuth_Bad_EmptyConfiguredToken(t *testing.T) { handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(200) }) - // Empty token disables auth + // Empty token now requires explicit configuration wrapped := withAuth("", handler) req, _ := http.NewRequest("GET", "/", nil) rr := &fakeResponseWriter{code: 200} wrapped.ServeHTTP(rr, req) - if rr.code != 200 { - t.Errorf("expected 200 with auth disabled, got %d", rr.code) + if rr.code != 401 { + t.Errorf("expected 401 with empty configured token, got %d", rr.code) + } +} + +func TestWithAuth_Bad_NonBearerToken(t *testing.T) { + handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(200) + }) + + wrapped := withAuth("my-token", handler) + + req, _ := http.NewRequest("GET", "/", nil) + req.Header.Set("Authorization", "Token my-token") + rr := &fakeResponseWriter{code: 200} + wrapped.ServeHTTP(rr, req) + if rr.code != 401 { + t.Errorf("expected 401 with non-Bearer auth, got %d", rr.code) } } @@ -231,4 +247,4 @@ func (f *fakeResponseWriter) Header() http.Header { } func (f *fakeResponseWriter) Write(b []byte) (int, error) { return len(b), nil } -func (f *fakeResponseWriter) WriteHeader(code int) { f.code = code } +func (f *fakeResponseWriter) WriteHeader(code int) { f.code = code }