Skip to content

Commit 0cd499d

Browse files
fix(mcp): allow workspace roots in analysis validation
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
1 parent eea7834 commit 0cd499d

4 files changed

Lines changed: 103 additions & 17 deletions

File tree

internal/mcp/handler_analysis.go

Lines changed: 63 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -3,9 +3,7 @@ package mcp
33
import (
44
"context"
55
"fmt"
6-
"os"
76
"path/filepath"
8-
"strings"
97

108
"github.com/mark3labs/mcp-go/mcp"
119
"github.com/tae2089/trace"
@@ -405,27 +403,83 @@ func validateRepoRootWithin(repoRoot, configuredRepoRoot, workspaceRoot string)
405403
if repoRoot == "" {
406404
return "", fmt.Errorf("repo_root is required")
407405
}
408-
allowed := configuredRepoRoot
409-
if allowed == "" {
410-
allowed = workspaceRoot
411-
}
412-
if allowed == "" {
406+
allowedRoots := configuredAnalysisRoots(configuredRepoRoot, workspaceRoot)
407+
if len(allowedRoots) == 0 {
413408
return "", fmt.Errorf("analysis repo root is not configured")
414409
}
415410
repo, err := canonicalPath(repoRoot)
416411
if err != nil {
417412
return "", fmt.Errorf("invalid repo_root: %w", err)
418413
}
419-
base, err := canonicalPath(allowed)
414+
allowed, err := validatePathWithinAllowedRoots(repo, allowedRoots)
420415
if err != nil {
421416
return "", fmt.Errorf("invalid configured repo root: %w", err)
422417
}
423-
if repo != base && !strings.HasPrefix(repo, base+string(os.PathSeparator)) {
418+
if !allowed {
424419
return "", fmt.Errorf("repo_root %q is outside configured analysis root", repoRoot)
425420
}
426421
return repo, nil
427422
}
428423

424+
func configuredAnalysisRoots(repoRoot, workspaceRoot string) []string {
425+
roots := make([]string, 0, 2)
426+
for _, root := range []string{repoRoot, workspaceRoot} {
427+
if root == "" {
428+
continue
429+
}
430+
if !sliceContainsString(roots, root) {
431+
roots = append(roots, root)
432+
}
433+
}
434+
return roots
435+
}
436+
437+
func sliceContainsString(values []string, target string) bool {
438+
for _, value := range values {
439+
if value == target {
440+
return true
441+
}
442+
}
443+
return false
444+
}
445+
446+
func validatePathWithinAllowedRoots(target string, allowedRoots []string) (bool, error) {
447+
for _, root := range allowedRoots {
448+
base, err := canonicalPath(root)
449+
if err != nil {
450+
return false, err
451+
}
452+
within, err := isWithinRoot(base, target)
453+
if err != nil {
454+
return false, err
455+
}
456+
if within {
457+
return true, nil
458+
}
459+
}
460+
return false, nil
461+
}
462+
463+
func isWithinRoot(root, target string) (bool, error) {
464+
rel, err := filepath.Rel(root, target)
465+
if err != nil {
466+
return false, err
467+
}
468+
if rel == "." {
469+
return true, nil
470+
}
471+
if rel == ".." {
472+
return false, nil
473+
}
474+
if len(rel) >= 3 && rel[:3] == ".."+string(filepath.Separator) {
475+
return false, nil
476+
}
477+
if filepath.IsAbs(rel) {
478+
return false, nil
479+
}
480+
return true, nil
481+
}
482+
429483
func canonicalPath(path string) (string, error) {
430484
abs, err := filepath.Abs(path)
431485
if err != nil {

internal/mcp/handler_parse.go

Lines changed: 4 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,6 @@ import (
77
"os"
88
"path/filepath"
99
"slices"
10-
"strings"
1110
"time"
1211

1312
"github.com/mark3labs/mcp-go/mcp"
@@ -488,22 +487,19 @@ func (h *handlers) validateAnalysisPath(path string) (string, error) {
488487
if path == "" {
489488
return "", fmt.Errorf("path is required")
490489
}
491-
allowed := h.deps.RepoRoot
492-
if allowed == "" {
493-
allowed = h.workspaceRoot()
494-
}
495-
if allowed == "" {
490+
allowedRoots := configuredAnalysisRoots(h.deps.RepoRoot, h.workspaceRoot())
491+
if len(allowedRoots) == 0 {
496492
return "", fmt.Errorf("analysis root is not configured")
497493
}
498494
target, err := canonicalExistingPath(path)
499495
if err != nil {
500496
return "", fmt.Errorf("invalid path: %w", err)
501497
}
502-
base, err := canonicalPath(allowed)
498+
allowed, err := validatePathWithinAllowedRoots(target, allowedRoots)
503499
if err != nil {
504500
return "", fmt.Errorf("invalid configured analysis root: %w", err)
505501
}
506-
if target != base && !strings.HasPrefix(target, base+string(os.PathSeparator)) {
502+
if !allowed {
507503
return "", fmt.Errorf("path %q is outside configured analysis root", path)
508504
}
509505
return target, nil

internal/mcp/handlers_test.go

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -94,6 +94,24 @@ func Hello() {}
9494
}
9595
}
9696

97+
func TestParseProject_AllowsWorkspacePathWhenRepoRootIsAlsoConfigured(t *testing.T) {
98+
deps := setupTestDeps(t)
99+
deps.RepoRoot = t.TempDir()
100+
deps.WorkspaceRoot = t.TempDir()
101+
workspaceDir := filepath.Join(deps.WorkspaceRoot, "sample-ws")
102+
if err := os.MkdirAll(workspaceDir, 0o755); err != nil {
103+
t.Fatal(err)
104+
}
105+
writeGoFile(t, workspaceDir, "main.go", `package main
106+
func Hello() {}
107+
`)
108+
109+
result := callTool(t, deps, "parse_project", map[string]any{"path": workspaceDir, "workspace": "sample-ws"})
110+
if result.IsError {
111+
t.Fatalf("expected workspace path to be allowed, got error: %s", getTextContent(result))
112+
}
113+
}
114+
97115
func TestParseProject_FailsClosedWithoutConfiguredRoot(t *testing.T) {
98116
deps := setupTestDeps(t)
99117
deps.RepoRoot = ""

internal/mcp/prompts_test.go

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -626,6 +626,24 @@ func TestPreMergeCheck_RejectsRepoRootOutsideConfiguredRoot(t *testing.T) {
626626
}
627627
}
628628

629+
func TestReviewChanges_AllowsWorkspaceRootWhenRepoRootAlsoConfigured(t *testing.T) {
630+
deps, _ := setupPromptTestDeps(t)
631+
deps.ChangesGitClient = &mockGitClient{changedFiles: []string{}}
632+
deps.RepoRoot = t.TempDir()
633+
deps.WorkspaceRoot = t.TempDir()
634+
workspaceRoot := t.TempDir()
635+
deps.WorkspaceRoot = workspaceRoot
636+
637+
resultJSON := callPrompt(t, deps, "review_changes", map[string]string{
638+
"repo_root": workspaceRoot,
639+
"base": "HEAD~1",
640+
})
641+
text := getPromptText(t, resultJSON)
642+
if !strings.Contains(text, "변경사항이 없습니다") {
643+
t.Fatalf("expected workspace-root repo_root to be accepted, got: %s", text)
644+
}
645+
}
646+
629647
// ============================================================
630648
// Mock types
631649
// ============================================================

0 commit comments

Comments
 (0)