Skip to content

mcp: reject relative paths in tool handlers #3694

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
knative-prow merged 6 commits into from
Oct 29, 2026
Merged

mcp: reject relative paths in tool handlers #3694

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
cc7a1f3
feat: enforce absolute path validation in MCP tool handlers and updat…
Shrey327 May 10, 2026
e46160b
fix: added handler-level tests and updated the error message
Shrey327 May 11, 2026
858883e
use t.TempDir() in config_envs_add tests
Shrey327 May 17, 2026
a60c12f
mcp: cover every path-taking tool with validatePath
lkingland Oct 29, 2026
38c0b86
mcp: rename schema placeholder helper
lkingland Oct 29, 2026
7eb46f8
mcp: use platform absolute paths in MCP tool tests
lkingland Oct 29, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions pkg/mcp/tools.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package mcp

import (
"fmt"
"path/filepath"

"github.com/modelcontextprotocol/go-sdk/mcp"
)
Expand Down Expand Up @@ -44,3 +45,12 @@ func appendBoolFlag(args []string, flag string, value *bool) []string {
func ptr[T any](v T) *T {
return &v
}

// validatePath rejects relative paths. Since cmd.Dir is not set in the executor,
// relative paths resolve against the MCP server's CWD, not the user's project root.
func validatePath(path string) error {
if !filepath.IsAbs(path) {
return fmt.Errorf("path must be absolute (the MCP server's working directory is not the user's project root), got %q", path)
}
Comment thread
lkingland marked this conversation as resolved.
Resolved
return nil
}
3 changes: 3 additions & 0 deletions pkg/mcp/tools_build.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,9 @@ func (s *Server) buildHandler(ctx context.Context, r *mcp.CallToolRequest, input
err = fmt.Errorf("pushing images is not allowed in read-only mode; set %s=true to enable write operations", EnvMCPWrite)
return
}
if err = validatePath(input.Path); err != nil {
return
}
out, err := s.executor.Execute(ctx, "build", input.Args()...)
if err != nil {
err = fmt.Errorf("%w\n%s", err, string(out))
Expand Down
5 changes: 3 additions & 2 deletions pkg/mcp/tools_build_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ func TestTool_Build_ReadonlyWithoutPushAllowed(t *testing.T) {

result, err := client.CallTool(t.Context(), &mcp.CallToolParams{
Name: "build",
Arguments: map[string]any{"path": "."},
Arguments: map[string]any{"path": t.TempDir()},
})
if err != nil {
t.Fatal(err)
Expand All @@ -60,12 +60,13 @@ func TestTool_Build_ReadonlyWithoutPushAllowed(t *testing.T) {
// TestTool_Build_Args ensures the build tool executes with all arguments passed correctly.
func TestTool_Build_Args(t *testing.T) {
// Test data - defined once and used for both input and validation
path := t.TempDir()
stringFlags := map[string]struct {
jsonKey string
flag string
value string
}{
"path": {"path", "--path", "."},
"path": {"path", "--path", path},
"builder": {"builder", "--builder", "pack"},
"registry": {"registry", "--registry", "ghcr.io/user"},
"builderImage": {"builderImage", "--builder-image", "custom-builder:latest"},
Expand Down
3 changes: 3 additions & 0 deletions pkg/mcp/tools_config_ci.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,9 @@ type ConfigCIOutput struct {
}

func (s *Server) configCIHandler(ctx context.Context, r *mcp.CallToolRequest, input ConfigCIInput) (result *mcp.CallToolResult, output ConfigCIOutput, err error) {
if err = validatePath(input.Path); err != nil {
return
}
out, err := s.executor.Execute(ctx, "config", input.Args()...)
if err != nil {
err = fmt.Errorf("%w\n%s", err, string(out))
Expand Down
15 changes: 9 additions & 6 deletions pkg/mcp/tools_config_ci_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,12 +11,13 @@ import (

// TestTool_ConfigCI ensures the config_ci tool executes with all arguments forwarded.
func TestTool_ConfigCI(t *testing.T) {
path := t.TempDir()
stringFlags := map[string]struct {
jsonKey string
flag string
value string
}{
"path": {"path", "--path", "."},
"path": {"path", "--path", path},
"branch": {"branch", "--branch", "main"},
"workflowName": {"workflowName", "--workflow-name", "Func Deploy"},
"kubeconfigSecretName": {"kubeconfigSecretName", "--kubeconfig-secret-name", "KUBECONFIG"},
Expand Down Expand Up @@ -82,13 +83,14 @@ func TestTool_ConfigCI(t *testing.T) {
// TestTool_ConfigCI_MinimalArgs ensures the config_ci tool executes with only the
// required 'path' parameter, omitting all optional flags.
func TestTool_ConfigCI_MinimalArgs(t *testing.T) {
path := t.TempDir()
executor := mock.NewExecutor()
executor.ExecuteFn = func(ctx context.Context, subcommand string, args ...string) ([]byte, error) {
if subcommand != "config" {
t.Fatalf("expected subcommand 'config', got %q", subcommand)
}

// "ci" + "--path" + "." = 3 args
// "ci" + "--path" + <abs> = 3 args
if len(args) != 3 {
t.Fatalf("expected 3 args, got %d: %v", len(args), args)
}
Expand All @@ -97,8 +99,8 @@ func TestTool_ConfigCI_MinimalArgs(t *testing.T) {
}

argsMap := argsToMap(args[1:])
if val, ok := argsMap["--path"]; !ok || val != "." {
t.Fatalf("expected --path='.', got %q", val)
if val, ok := argsMap["--path"]; !ok || val != path {
t.Fatalf("expected --path=%q, got %q", path, val)
}

return []byte("GitHub workflow generated successfully\n"), nil
Expand All @@ -111,7 +113,7 @@ func TestTool_ConfigCI_MinimalArgs(t *testing.T) {

result, err := client.CallTool(t.Context(), &mcp.CallToolParams{
Name: "config_ci",
Arguments: map[string]any{"path": "."},
Arguments: map[string]any{"path": path},
})
if err != nil {
t.Fatal(err)
Expand All @@ -127,6 +129,7 @@ func TestTool_ConfigCI_MinimalArgs(t *testing.T) {
// TestTool_ConfigCI_Error ensures the config_ci tool propagates executor errors,
// such as the "unknown command" error returned when FUNC_ENABLE_CI_CONFIG is not set.
func TestTool_ConfigCI_Error(t *testing.T) {
path := t.TempDir()
executor := mock.NewExecutor()
executor.ExecuteFn = func(ctx context.Context, subcommand string, args ...string) ([]byte, error) {
return []byte(`unknown command "ci" for "config"`), errors.New("executor error")
Expand All @@ -139,7 +142,7 @@ func TestTool_ConfigCI_Error(t *testing.T) {

result, err := client.CallTool(t.Context(), &mcp.CallToolParams{
Name: "config_ci",
Arguments: map[string]any{"path": "."},
Arguments: map[string]any{"path": path},
})
if err != nil {
t.Fatal(err)
Expand Down
9 changes: 9 additions & 0 deletions pkg/mcp/tools_config_envs.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,9 @@ type ConfigEnvsListOutput struct {
}

func (s *Server) configEnvsListHandler(ctx context.Context, r *mcp.CallToolRequest, input ConfigEnvsListInput) (result *mcp.CallToolResult, output ConfigEnvsListOutput, err error) {
if err = validatePath(input.Path); err != nil {
return
}
out, err := s.executor.Execute(ctx, "config", input.Args()...)
if err != nil {
err = fmt.Errorf("%w\n%s", err, string(out))
Expand Down Expand Up @@ -146,6 +149,9 @@ func (s *Server) configEnvsAddHandler(ctx context.Context, r *mcp.CallToolReques
err = fmt.Errorf("the server is currently in readonly mode. Please set %s and restart the MCP server", EnvMCPWrite)
return
}
if err = validatePath(input.Path); err != nil {
return
}
if err = input.validate(); err != nil {
return
}
Expand Down Expand Up @@ -194,6 +200,9 @@ func (s *Server) configEnvsRemoveHandler(ctx context.Context, r *mcp.CallToolReq
err = fmt.Errorf("the server is currently in readonly mode. Please set %s and restart the MCP server", EnvMCPWrite)
return
}
if err = validatePath(input.Path); err != nil {
return
}
out, err := s.executor.Execute(ctx, "config", input.Args()...)
if err != nil {
err = fmt.Errorf("%w\n%s", err, string(out))
Expand Down
45 changes: 26 additions & 19 deletions pkg/mcp/tools_config_envs_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,12 +12,13 @@ import (

// TestTool_ConfigEnvsAdd ensures the config_envs_add tool executes with all arguments.
func TestTool_ConfigEnvsAdd(t *testing.T) {
path := t.TempDir()
stringFlags := map[string]struct {
jsonKey string
flag string
value string
}{
"path": {"path", "--path", "."},
"path": {"path", "--path", path},
"name": {"name", "--name", "API_KEY"},
"value": {"value", "--value", "secret123"},
}
Expand Down Expand Up @@ -76,6 +77,7 @@ func TestTool_ConfigEnvsAdd(t *testing.T) {
// TestTool_ConfigEnvsAdd_SecretKey ensures secret-key-sourced env vars produce
// the correct "{{ secret:name:key }}" value template.
func TestTool_ConfigEnvsAdd_SecretKey(t *testing.T) {
path := t.TempDir()
executor := mock.NewExecutor()
executor.ExecuteFn = func(ctx context.Context, subcommand string, args ...string) ([]byte, error) {
if subcommand != "config" {
Expand Down Expand Up @@ -103,7 +105,7 @@ func TestTool_ConfigEnvsAdd_SecretKey(t *testing.T) {
result, err := client.CallTool(t.Context(), &mcp.CallToolParams{
Name: "config_envs_add",
Arguments: map[string]any{
"path": ".",
"path": path,
"name": "API_KEY",
"secretName": "my-secret",
"secretKey": "MY_KEY",
Expand All @@ -123,6 +125,7 @@ func TestTool_ConfigEnvsAdd_SecretKey(t *testing.T) {
// TestTool_ConfigEnvsAdd_SecretAllKeys ensures importing all keys from a Secret
// produces the "{{ secret:name }}" value template without --name.
func TestTool_ConfigEnvsAdd_SecretAllKeys(t *testing.T) {
path := t.TempDir()
executor := mock.NewExecutor()
executor.ExecuteFn = func(ctx context.Context, subcommand string, args ...string) ([]byte, error) {
if subcommand != "config" {
Expand Down Expand Up @@ -150,7 +153,7 @@ func TestTool_ConfigEnvsAdd_SecretAllKeys(t *testing.T) {
result, err := client.CallTool(t.Context(), &mcp.CallToolParams{
Name: "config_envs_add",
Arguments: map[string]any{
"path": ".",
"path": path,
"secretName": "my-secret",
},
})
Expand All @@ -168,6 +171,7 @@ func TestTool_ConfigEnvsAdd_SecretAllKeys(t *testing.T) {
// TestTool_ConfigEnvsAdd_ConfigMapKey ensures configmap-key-sourced env vars produce
// the correct "{{ configMap:name:key }}" value template.
func TestTool_ConfigEnvsAdd_ConfigMapKey(t *testing.T) {
path := t.TempDir()
executor := mock.NewExecutor()
executor.ExecuteFn = func(ctx context.Context, subcommand string, args ...string) ([]byte, error) {
if subcommand != "config" {
Expand Down Expand Up @@ -195,7 +199,7 @@ func TestTool_ConfigEnvsAdd_ConfigMapKey(t *testing.T) {
result, err := client.CallTool(t.Context(), &mcp.CallToolParams{
Name: "config_envs_add",
Arguments: map[string]any{
"path": ".",
"path": path,
"name": "DATABASE_HOST",
"configMapName": "my-config",
"configMapKey": "DB_HOST",
Expand All @@ -215,6 +219,7 @@ func TestTool_ConfigEnvsAdd_ConfigMapKey(t *testing.T) {
// TestTool_ConfigEnvsAdd_ConfigMapAllKeys ensures importing all keys from a ConfigMap
// produces the "{{ configMap:name }}" value template without --name.
func TestTool_ConfigEnvsAdd_ConfigMapAllKeys(t *testing.T) {
path := t.TempDir()
executor := mock.NewExecutor()
executor.ExecuteFn = func(ctx context.Context, subcommand string, args ...string) ([]byte, error) {
if subcommand != "config" {
Expand Down Expand Up @@ -242,7 +247,7 @@ func TestTool_ConfigEnvsAdd_ConfigMapAllKeys(t *testing.T) {
result, err := client.CallTool(t.Context(), &mcp.CallToolParams{
Name: "config_envs_add",
Arguments: map[string]any{
"path": ".",
"path": path,
"configMapName": "my-config",
},
})
Expand Down Expand Up @@ -274,7 +279,7 @@ func TestTool_ConfigEnvsAdd_ValueAndSecretMutuallyExclusive(t *testing.T) {
result, err := client.CallTool(t.Context(), &mcp.CallToolParams{
Name: "config_envs_add",
Arguments: map[string]any{
"path": ".",
"path": t.TempDir(),
"name": "MY_VAR",
"value": "explicit-value",
"secretName": "my-secret",
Expand Down Expand Up @@ -308,7 +313,7 @@ func TestTool_ConfigEnvsAdd_NameWithSecretAllKeys(t *testing.T) {
result, err := client.CallTool(t.Context(), &mcp.CallToolParams{
Name: "config_envs_add",
Arguments: map[string]any{
"path": ".",
"path": t.TempDir(),
"name": "MY_VAR",
"secretName": "my-secret",
},
Expand Down Expand Up @@ -341,7 +346,7 @@ func TestTool_ConfigEnvsAdd_NameWithConfigMapAllKeys(t *testing.T) {
result, err := client.CallTool(t.Context(), &mcp.CallToolParams{
Name: "config_envs_add",
Arguments: map[string]any{
"path": ".",
"path": t.TempDir(),
"name": "MY_VAR",
"configMapName": "my-config",
},
Expand Down Expand Up @@ -374,7 +379,7 @@ func TestTool_ConfigEnvsAdd_BothSecretAndConfigMap(t *testing.T) {
result, err := client.CallTool(t.Context(), &mcp.CallToolParams{
Name: "config_envs_add",
Arguments: map[string]any{
"path": ".",
"path": t.TempDir(),
"name": "MY_VAR",
"secretName": "my-secret",
"configMapName": "my-config",
Expand Down Expand Up @@ -408,7 +413,7 @@ func TestTool_ConfigEnvsAdd_SecretKeyWithoutSecretName(t *testing.T) {
result, err := client.CallTool(t.Context(), &mcp.CallToolParams{
Name: "config_envs_add",
Arguments: map[string]any{
"path": ".",
"path": t.TempDir(),
"name": "MY_VAR",
"secretKey": "MY_KEY",
},
Expand Down Expand Up @@ -441,7 +446,7 @@ func TestTool_ConfigEnvsAdd_ConfigMapKeyWithoutConfigMapName(t *testing.T) {
result, err := client.CallTool(t.Context(), &mcp.CallToolParams{
Name: "config_envs_add",
Arguments: map[string]any{
"path": ".",
"path": t.TempDir(),
"name": "MY_VAR",
"configMapKey": "MY_KEY",
},
Expand All @@ -459,13 +464,14 @@ func TestTool_ConfigEnvsAdd_ConfigMapKeyWithoutConfigMapName(t *testing.T) {

// TestTool_ConfigEnvsList ensures the config_envs_list tool lists environment variables.
func TestTool_ConfigEnvsList(t *testing.T) {
path := t.TempDir()
executor := mock.NewExecutor()
executor.ExecuteFn = func(ctx context.Context, subcommand string, args ...string) ([]byte, error) {
if subcommand != "config" {
t.Fatalf("expected subcommand 'config', got %q", subcommand)
}

// "envs" + "--path" + "." = 3 args
// "envs" + "--path" + path = 3 args
if len(args) != 3 {
t.Fatalf("expected 3 args, got %d: %v", len(args), args)
}
Expand All @@ -474,8 +480,8 @@ func TestTool_ConfigEnvsList(t *testing.T) {
}

argsMap := argsToMap(args[1:])
if val, ok := argsMap["--path"]; !ok || val != "." {
t.Fatalf("expected --path='.', got %q", val)
if val, ok := argsMap["--path"]; !ok || val != path {
t.Fatalf("expected --path=%q, got %q", path, val)
}

return []byte("DATABASE_URL=postgres://localhost\nAPI_KEY=secret\n"), nil
Expand All @@ -488,7 +494,7 @@ func TestTool_ConfigEnvsList(t *testing.T) {

result, err := client.CallTool(t.Context(), &mcp.CallToolParams{
Name: "config_envs_list",
Arguments: map[string]any{"path": "."},
Arguments: map[string]any{"path": path},
})
if err != nil {
t.Fatal(err)
Expand All @@ -503,12 +509,13 @@ func TestTool_ConfigEnvsList(t *testing.T) {

// TestTool_ConfigEnvsRemove ensures the config_envs_remove tool removes an environment variable.
func TestTool_ConfigEnvsRemove(t *testing.T) {
path := t.TempDir()
stringFlags := map[string]struct {
jsonKey string
flag string
value string
}{
"path": {"path", "--path", "."},
"path": {"path", "--path", path},
"name": {"name", "--name", "API_KEY"},
}

Expand Down Expand Up @@ -571,7 +578,7 @@ func TestTool_ConfigEnvsList_Error(t *testing.T) {

result, err := client.CallTool(t.Context(), &mcp.CallToolParams{
Name: "config_envs_list",
Arguments: map[string]any{"path": "."},
Arguments: map[string]any{"path": t.TempDir()},
})
if err != nil {
t.Fatal(err)
Expand All @@ -595,7 +602,7 @@ func TestTool_ConfigEnvsAdd_Error(t *testing.T) {

result, err := client.CallTool(t.Context(), &mcp.CallToolParams{
Name: "config_envs_add",
Arguments: map[string]any{"path": "."},
Arguments: map[string]any{"path": t.TempDir()},
})
if err != nil {
t.Fatal(err)
Expand All @@ -619,7 +626,7 @@ func TestTool_ConfigEnvsRemove_Error(t *testing.T) {

result, err := client.CallTool(t.Context(), &mcp.CallToolParams{
Name: "config_envs_remove",
Arguments: map[string]any{"path": "."},
Arguments: map[string]any{"path": t.TempDir()},
})
if err != nil {
t.Fatal(err)
Expand Down
6 changes: 6 additions & 0 deletions pkg/mcp/tools_config_git.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,9 @@ func (s *Server) configGitSetHandler(ctx context.Context, r *mcp.CallToolRequest
err = fmt.Errorf("'path' is required")
return
}
if err = validatePath(input.Path); err != nil {
return
}
if input.GitURL == "" {
err = fmt.Errorf("'git_url' is required")
return
Expand Down Expand Up @@ -157,6 +160,9 @@ func (s *Server) configGitRemoveHandler(ctx context.Context, r *mcp.CallToolRequ
err = fmt.Errorf("'path' is required")
return
}
if err = validatePath(input.Path); err != nil {
return
}

out, err := s.executor.Execute(ctx, "config", input.Args()...)
if err != nil {
Expand Down
Loading
Loading