Skip to content

fix(mcp): prevent hang in config add/remove handlers when required fields are omitted #3761

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

Open
Ankitsinghsisodya wants to merge 2 commits into
base: main
Choose a base branch
from
Open

fix(mcp): prevent hang in config add/remove handlers when required fields are omitted #3761

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f0f402c
test: add validation tests for config_envs and config_labels tools
Ankitsinghsisodya May 16, 2026
c7c4474
test: add validation for empty fields in config tools
Ankitsinghsisodya May 16, 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
16 changes: 11 additions & 5 deletions pkg/mcp/tools_config_envs.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,9 @@ type ConfigEnvsAddInput struct {
// constructed and forwarded to the CLI. Character-set validation is deferred to
// func's own parser, keeping this layer as a thin pass-through.
func (i ConfigEnvsAddInput) validate() error {
if i.Value == nil && i.SecretName == nil && i.ConfigMapName == nil {
return fmt.Errorf("at least one of value, secretName, or configMapName must be provided")
}
if i.Value != nil && (i.SecretName != nil || i.ConfigMapName != nil) {
return fmt.Errorf("value is mutually exclusive with secretName/configMapName; provide one or the other")
}
Expand Down Expand Up @@ -169,14 +172,13 @@ var configEnvsRemoveTool = &mcp.Tool{
}

type ConfigEnvsRemoveInput struct {
Path string `json:"path" jsonschema:"required,Path to the function project directory"`
Name *string `json:"name,omitempty" jsonschema:"Name of the environment variable to remove"`
Verbose *bool `json:"verbose,omitempty" jsonschema:"Enable verbose logging output"`
Path string `json:"path" jsonschema:"required,Path to the function project directory"`
Name string `json:"name" jsonschema:"required,Name of the environment variable to remove"`
Comment thread
Ankitsinghsisodya marked this conversation as resolved.
Verbose *bool `json:"verbose,omitempty" jsonschema:"Enable verbose logging output"`
}

func (i ConfigEnvsRemoveInput) Args() []string {
args := []string{"envs", "remove", "--path", i.Path}
args = appendStringFlag(args, "--name", i.Name)
args := []string{"envs", "remove", "--path", i.Path, "--name", i.Name}
Comment thread
Ankitsinghsisodya marked this conversation as resolved.
args = appendBoolFlag(args, "--verbose", i.Verbose)
return args
}
Expand All @@ -186,6 +188,10 @@ type ConfigEnvsRemoveOutput struct {
}

func (s *Server) configEnvsRemoveHandler(ctx context.Context, r *mcp.CallToolRequest, input ConfigEnvsRemoveInput) (result *mcp.CallToolResult, output ConfigEnvsRemoveOutput, err error) {
if input.Name == "" {
err = fmt.Errorf("'name' must not be empty")
return
}
out, err := s.executor.Execute(ctx, "config", input.Args()...)
if err != nil {
err = fmt.Errorf("%w\n%s", err, string(out))
Expand Down
103 changes: 101 additions & 2 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 @@ -556,6 +556,99 @@ func TestTool_ConfigEnvsRemove(t *testing.T) {
}
}

// TestTool_ConfigEnvsAdd_BulkImport ensures the config_envs_add tool accepts calls without name
// for bulk-import syntax (e.g. value='{{ secret:mySecret }}').
func TestTool_ConfigEnvsAdd_BulkImport(t *testing.T) {
executor := mock.NewExecutor()
executor.ExecuteFn = func(ctx context.Context, subcommand string, args ...string) ([]byte, error) {
return []byte("Environment variables imported successfully\n"), nil
}

client, _, err := newTestPair(t, WithExecutor(executor))
if err != nil {
t.Fatal(err)
}

result, err := client.CallTool(t.Context(), &mcp.CallToolParams{
Name: "config_envs_add",
Arguments: map[string]any{"path": ".", "value": "{{ secret:mySecret }}"},
})
if err != nil {
t.Fatal(err)
}
if result.IsError {
t.Fatalf("bulk-import without name must succeed: %v", result)
}
if !executor.ExecuteInvoked {
t.Fatal("executor was not invoked")
}
}

// TestTool_ConfigEnvsAdd_MissingValue ensures the config_envs_add tool rejects calls without value.
func TestTool_ConfigEnvsAdd_MissingValue(t *testing.T) {
executor := mock.NewExecutor()
client, _, err := newTestPair(t, WithExecutor(executor))
if err != nil {
t.Fatal(err)
}

result, err := client.CallTool(t.Context(), &mcp.CallToolParams{
Name: "config_envs_add",
Arguments: map[string]any{"path": ".", "name": "API_KEY"},
})
// Schema validation may produce a protocol-level error or a tool-level error result.
if err == nil && !result.IsError {
t.Fatal("expected error when value is missing")
}
if executor.ExecuteInvoked {
t.Fatal("executor must not be invoked when required fields are absent")
}
}

// TestTool_ConfigEnvsRemove_EmptyName ensures the config_envs_remove tool rejects an empty name.
func TestTool_ConfigEnvsRemove_EmptyName(t *testing.T) {
executor := mock.NewExecutor()
client, _, err := newTestPair(t, WithExecutor(executor))
if err != nil {
t.Fatal(err)
}

result, err := client.CallTool(t.Context(), &mcp.CallToolParams{
Name: "config_envs_remove",
Arguments: map[string]any{"path": ".", "name": ""},
})
if err != nil {
t.Fatal(err)
}
if !result.IsError {
t.Fatal("expected error result when name is empty")
}
if executor.ExecuteInvoked {
t.Fatal("executor must not be invoked when name is empty")
}
}

// TestTool_ConfigEnvsRemove_MissingName ensures the config_envs_remove tool rejects calls without name.
func TestTool_ConfigEnvsRemove_MissingName(t *testing.T) {
executor := mock.NewExecutor()
client, _, err := newTestPair(t, WithExecutor(executor))
if err != nil {
t.Fatal(err)
}

result, err := client.CallTool(t.Context(), &mcp.CallToolParams{
Name: "config_envs_remove",
Arguments: map[string]any{"path": "."},
})
// Schema validation may produce a protocol-level error or a tool-level error result.
if err == nil && !result.IsError {
t.Fatal("expected error when name is missing")
}
if executor.ExecuteInvoked {
t.Fatal("executor must not be invoked when required fields are absent")
}
}

// TestTool_ConfigEnvsList_Error ensures the config_envs_list tool propagates executor errors.
func TestTool_ConfigEnvsList_Error(t *testing.T) {
executor := mock.NewExecutor()
Expand Down Expand Up @@ -594,14 +687,17 @@ 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": ".", "name": "API_KEY", "value": "secret123"},
})
if err != nil {
t.Fatal(err)
}
if !result.IsError {
t.Fatal("expected error result, got success")
}
if !executor.ExecuteInvoked {
t.Fatal("executor was not invoked")
}
}

// TestTool_ConfigEnvsRemove_Error ensures the config_envs_remove tool propagates executor errors.
Expand All @@ -618,12 +714,15 @@ 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": ".", "name": "API_KEY"},
})
if err != nil {
t.Fatal(err)
}
if !result.IsError {
t.Fatal("expected error result, got success")
}
if !executor.ExecuteInvoked {
t.Fatal("executor was not invoked")
}
}
33 changes: 21 additions & 12 deletions pkg/mcp/tools_config_labels.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,16 +60,14 @@ var configLabelsAddTool = &mcp.Tool{
}

type ConfigLabelsAddInput struct {
Path string `json:"path" jsonschema:"required,Path to the function project directory"`
Name *string `json:"name,omitempty" jsonschema:"Name of the label"`
Value *string `json:"value,omitempty" jsonschema:"Value of the label"`
Verbose *bool `json:"verbose,omitempty" jsonschema:"Enable verbose logging output"`
Path string `json:"path" jsonschema:"required,Path to the function project directory"`
Name string `json:"name" jsonschema:"required,Name of the label"`
Value string `json:"value" jsonschema:"required,Value of the label"`
Comment thread
Ankitsinghsisodya marked this conversation as resolved.
Comment thread
Ankitsinghsisodya marked this conversation as resolved.
Verbose *bool `json:"verbose,omitempty" jsonschema:"Enable verbose logging output"`
}

func (i ConfigLabelsAddInput) Args() []string {
args := []string{"labels", "add", "--path", i.Path}
args = appendStringFlag(args, "--name", i.Name)
args = appendStringFlag(args, "--value", i.Value)
args := []string{"labels", "add", "--path", i.Path, "--name", i.Name, "--value", i.Value}
Comment thread
Ankitsinghsisodya marked this conversation as resolved.
args = appendBoolFlag(args, "--verbose", i.Verbose)
return args
}
Expand All @@ -79,6 +77,14 @@ type ConfigLabelsAddOutput struct {
}

func (s *Server) configLabelsAddHandler(ctx context.Context, r *mcp.CallToolRequest, input ConfigLabelsAddInput) (result *mcp.CallToolResult, output ConfigLabelsAddOutput, err error) {
if input.Name == "" {
err = fmt.Errorf("'name' must not be empty")
return
}
if input.Value == "" {
err = fmt.Errorf("'value' must not be empty")
return
}
out, err := s.executor.Execute(ctx, "config", input.Args()...)
if err != nil {
err = fmt.Errorf("%w\n%s", err, string(out))
Expand All @@ -103,14 +109,13 @@ var configLabelsRemoveTool = &mcp.Tool{
}

type ConfigLabelsRemoveInput struct {
Path string `json:"path" jsonschema:"required,Path to the function project directory"`
Name *string `json:"name,omitempty" jsonschema:"Name of the label to remove"`
Verbose *bool `json:"verbose,omitempty" jsonschema:"Enable verbose logging output"`
Path string `json:"path" jsonschema:"required,Path to the function project directory"`
Name string `json:"name" jsonschema:"required,Name of the label to remove"`
Comment thread
Ankitsinghsisodya marked this conversation as resolved.
Comment thread
Ankitsinghsisodya marked this conversation as resolved.
Verbose *bool `json:"verbose,omitempty" jsonschema:"Enable verbose logging output"`
}

func (i ConfigLabelsRemoveInput) Args() []string {
args := []string{"labels", "remove", "--path", i.Path}
args = appendStringFlag(args, "--name", i.Name)
args := []string{"labels", "remove", "--path", i.Path, "--name", i.Name}
Comment thread
Ankitsinghsisodya marked this conversation as resolved.
args = appendBoolFlag(args, "--verbose", i.Verbose)
return args
}
Expand All @@ -120,6 +125,10 @@ type ConfigLabelsRemoveOutput struct {
}

func (s *Server) configLabelsRemoveHandler(ctx context.Context, r *mcp.CallToolRequest, input ConfigLabelsRemoveInput) (result *mcp.CallToolResult, output ConfigLabelsRemoveOutput, err error) {
if input.Name == "" {
err = fmt.Errorf("'name' must not be empty")
return
}
out, err := s.executor.Execute(ctx, "config", input.Args()...)
if err != nil {
err = fmt.Errorf("%w\n%s", err, string(out))
Expand Down
Loading
Loading