From ddc3db290f483156263fc782236ba2420ae23c9a Mon Sep 17 00:00:00 2001 From: Ankitsinghsisodya Date: Sat, 9 May 2026 14:22:33 +0530 Subject: [PATCH 1/5] Enhance config_envs_add tool with new test cases and documentation updates - Added unit tests for handling environment variables sourced from Secrets and ConfigMaps, covering both individual keys and all keys scenarios. - Updated the tool's description to clarify supported source types and precedence rules for the Value field. - Improved argument handling in the ConfigEnvsAddInput struct to ensure correct value templates are constructed based on provided inputs. - Enhanced test coverage for the config_envs_add functionality to validate expected behavior and error handling. --- pkg/mcp/tools_config_envs.go | 53 ++++++-- pkg/mcp/tools_config_envs_test.go | 202 ++++++++++++++++++++++++++++++ 2 files changed, 247 insertions(+), 8 deletions(-) diff --git a/pkg/mcp/tools_config_envs.go b/pkg/mcp/tools_config_envs.go index a2d1fceefa..85bcad2ad5 100644 --- a/pkg/mcp/tools_config_envs.go +++ b/pkg/mcp/tools_config_envs.go @@ -48,9 +48,21 @@ func (s *Server) configEnvsListHandler(ctx context.Context, r *mcp.CallToolReque // config_envs_add var configEnvsAddTool = &mcp.Tool{ - Name: "config_envs_add", - Title: "Config Environment Variables Add", - Description: "Adds an environment variable to a function's configuration.", + Name: "config_envs_add", + Title: "Config Environment Variables Add", + Description: `Adds an environment variable to a function's configuration. + +Supports four source types: + 1. Literal value: set Name and Value directly. + 2. Local env var: set Name and Value to "{{ env:LOCAL_VAR }}". + 3. Secret (all keys): set SecretName only — imports every key from the Secret as env vars. + 4. Secret (one key): set Name, SecretName, and SecretKey. + 5. ConfigMap (all): set ConfigMapName only — imports every key from the ConfigMap as env vars. + 6. ConfigMap (one key): set Name, ConfigMapName, and ConfigMapKey. + +When SecretName or ConfigMapName is provided, the tool constructs the +appropriate "{{ secret:… }}" or "{{ configMap:… }}" value template automatically. +The explicit Value field takes precedence if provided alongside source fields.`, Annotations: &mcp.ToolAnnotations{ Title: "Config Environment Variables Add", ReadOnlyHint: false, @@ -60,16 +72,41 @@ var configEnvsAddTool = &mcp.Tool{ } type ConfigEnvsAddInput struct { - Path string `json:"path" jsonschema:"required,Path to the function project directory"` - Name *string `json:"name,omitempty" jsonschema:"Name of the environment variable"` - Value *string `json:"value,omitempty" jsonschema:"Value of the environment variable"` - 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,omitempty" jsonschema:"Name of the environment variable"` + Value *string `json:"value,omitempty" jsonschema:"Literal value for the environment variable"` + SecretName *string `json:"secretName,omitempty" jsonschema:"Name of the Kubernetes Secret to source the value from"` + SecretKey *string `json:"secretKey,omitempty" jsonschema:"Key within the Secret; omit to import all keys as env vars"` + ConfigMapName *string `json:"configMapName,omitempty" jsonschema:"Name of the Kubernetes ConfigMap to source the value from"` + ConfigMapKey *string `json:"configMapKey,omitempty" jsonschema:"Key within the ConfigMap; omit to import all keys as env vars"` + Verbose *bool `json:"verbose,omitempty" jsonschema:"Enable verbose logging output"` } func (i ConfigEnvsAddInput) Args() []string { args := []string{"envs", "add", "--path", i.Path} args = appendStringFlag(args, "--name", i.Name) - args = appendStringFlag(args, "--value", i.Value) + + // Explicit Value takes precedence over structured secret/configmap fields. + if i.Value != nil { + args = appendStringFlag(args, "--value", i.Value) + } else if i.SecretName != nil { + var v string + if i.SecretKey != nil { + v = fmt.Sprintf("{{ secret:%s:%s }}", *i.SecretName, *i.SecretKey) + } else { + v = fmt.Sprintf("{{ secret:%s }}", *i.SecretName) + } + args = append(args, "--value", v) + } else if i.ConfigMapName != nil { + var v string + if i.ConfigMapKey != nil { + v = fmt.Sprintf("{{ configMap:%s:%s }}", *i.ConfigMapName, *i.ConfigMapKey) + } else { + v = fmt.Sprintf("{{ configMap:%s }}", *i.ConfigMapName) + } + args = append(args, "--value", v) + } + args = appendBoolFlag(args, "--verbose", i.Verbose) return args } diff --git a/pkg/mcp/tools_config_envs_test.go b/pkg/mcp/tools_config_envs_test.go index 16440f701e..5f9d866ef3 100644 --- a/pkg/mcp/tools_config_envs_test.go +++ b/pkg/mcp/tools_config_envs_test.go @@ -72,6 +72,208 @@ 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) { + 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) + } + + argsMap := argsToMap(args) + wantValue := "{{ secret:my-secret:MY_KEY }}" + if got, ok := argsMap["--value"]; !ok || got != wantValue { + t.Fatalf("expected --value=%q, got %q", wantValue, got) + } + if got, ok := argsMap["--name"]; !ok || got != "API_KEY" { + t.Fatalf("expected --name='API_KEY', got %q", got) + } + return []byte("Environment variable added 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": ".", + "name": "API_KEY", + "secretName": "my-secret", + "secretKey": "MY_KEY", + }, + }) + if err != nil { + t.Fatal(err) + } + if result.IsError { + t.Fatalf("unexpected error result: %v", result) + } + if !executor.ExecuteInvoked { + t.Fatal("executor was not invoked") + } +} + +// 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) { + executor := mock.NewExecutor() + executor.ExecuteFn = func(ctx context.Context, subcommand string, args ...string) ([]byte, error) { + argsMap := argsToMap(args) + wantValue := "{{ secret:my-secret }}" + if got, ok := argsMap["--value"]; !ok || got != wantValue { + t.Fatalf("expected --value=%q, got %q", wantValue, got) + } + if _, ok := argsMap["--name"]; ok { + t.Fatal("expected no --name flag when importing all secret keys") + } + return []byte("Environment variable added 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": ".", + "secretName": "my-secret", + }, + }) + if err != nil { + t.Fatal(err) + } + if result.IsError { + t.Fatalf("unexpected error result: %v", result) + } + if !executor.ExecuteInvoked { + t.Fatal("executor was not invoked") + } +} + +// TestTool_ConfigEnvsAdd_ConfigMapKey ensures configmap-key-sourced env vars produce +// the correct "{{ configMap:name:key }}" value template. +func TestTool_ConfigEnvsAdd_ConfigMapKey(t *testing.T) { + executor := mock.NewExecutor() + executor.ExecuteFn = func(ctx context.Context, subcommand string, args ...string) ([]byte, error) { + argsMap := argsToMap(args) + wantValue := "{{ configMap:my-config:DB_HOST }}" + if got, ok := argsMap["--value"]; !ok || got != wantValue { + t.Fatalf("expected --value=%q, got %q", wantValue, got) + } + if got, ok := argsMap["--name"]; !ok || got != "DATABASE_HOST" { + t.Fatalf("expected --name='DATABASE_HOST', got %q", got) + } + return []byte("Environment variable added 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": ".", + "name": "DATABASE_HOST", + "configMapName": "my-config", + "configMapKey": "DB_HOST", + }, + }) + if err != nil { + t.Fatal(err) + } + if result.IsError { + t.Fatalf("unexpected error result: %v", result) + } + if !executor.ExecuteInvoked { + t.Fatal("executor was not invoked") + } +} + +// 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) { + executor := mock.NewExecutor() + executor.ExecuteFn = func(ctx context.Context, subcommand string, args ...string) ([]byte, error) { + argsMap := argsToMap(args) + wantValue := "{{ configMap:my-config }}" + if got, ok := argsMap["--value"]; !ok || got != wantValue { + t.Fatalf("expected --value=%q, got %q", wantValue, got) + } + if _, ok := argsMap["--name"]; ok { + t.Fatal("expected no --name flag when importing all configmap keys") + } + return []byte("Environment variable added 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": ".", + "configMapName": "my-config", + }, + }) + if err != nil { + t.Fatal(err) + } + if result.IsError { + t.Fatalf("unexpected error result: %v", result) + } + if !executor.ExecuteInvoked { + t.Fatal("executor was not invoked") + } +} + +// TestTool_ConfigEnvsAdd_ValueTakesPrecedence ensures that when both Value and +// SecretName are provided, the explicit Value is used. +func TestTool_ConfigEnvsAdd_ValueTakesPrecedence(t *testing.T) { + executor := mock.NewExecutor() + executor.ExecuteFn = func(ctx context.Context, subcommand string, args ...string) ([]byte, error) { + argsMap := argsToMap(args) + wantValue := "explicit-value" + if got, ok := argsMap["--value"]; !ok || got != wantValue { + t.Fatalf("expected --value=%q, got %q", wantValue, got) + } + return []byte("Environment variable added 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": ".", + "name": "MY_VAR", + "value": "explicit-value", + "secretName": "ignored-secret", + }, + }) + if err != nil { + t.Fatal(err) + } + if result.IsError { + t.Fatalf("unexpected error result: %v", result) + } + if !executor.ExecuteInvoked { + t.Fatal("executor was not invoked") + } +} + // TestTool_ConfigEnvsList ensures the config_envs_list tool lists environment variables. func TestTool_ConfigEnvsList(t *testing.T) { executor := mock.NewExecutor() From 364f6beb64aa39dfcfd7c4ed85147340eb82119c Mon Sep 17 00:00:00 2001 From: Ankitsinghsisodya Date: Sat, 9 May 2026 14:25:49 +0530 Subject: [PATCH 2/5] Refactor ConfigEnvsAddInput struct for improved clarity and consistency - Standardized formatting of struct fields in ConfigEnvsAddInput for better readability. - Updated test case for config_envs_add to ensure proper argument handling and maintain functionality. --- pkg/mcp/tools_config_envs.go | 12 ++++++------ pkg/mcp/tools_config_envs_test.go | 4 ++-- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/pkg/mcp/tools_config_envs.go b/pkg/mcp/tools_config_envs.go index 85bcad2ad5..597b1fcc0d 100644 --- a/pkg/mcp/tools_config_envs.go +++ b/pkg/mcp/tools_config_envs.go @@ -72,14 +72,14 @@ The explicit Value field takes precedence if provided alongside source fields.`, } type ConfigEnvsAddInput struct { - Path string `json:"path" jsonschema:"required,Path to the function project directory"` - Name *string `json:"name,omitempty" jsonschema:"Name of the environment variable"` - Value *string `json:"value,omitempty" jsonschema:"Literal value for the environment variable"` - SecretName *string `json:"secretName,omitempty" jsonschema:"Name of the Kubernetes Secret to source the value from"` - SecretKey *string `json:"secretKey,omitempty" jsonschema:"Key within the Secret; omit to import all keys as env vars"` + Path string `json:"path" jsonschema:"required,Path to the function project directory"` + Name *string `json:"name,omitempty" jsonschema:"Name of the environment variable"` + Value *string `json:"value,omitempty" jsonschema:"Literal value for the environment variable"` + SecretName *string `json:"secretName,omitempty" jsonschema:"Name of the Kubernetes Secret to source the value from"` + SecretKey *string `json:"secretKey,omitempty" jsonschema:"Key within the Secret; omit to import all keys as env vars"` ConfigMapName *string `json:"configMapName,omitempty" jsonschema:"Name of the Kubernetes ConfigMap to source the value from"` ConfigMapKey *string `json:"configMapKey,omitempty" jsonschema:"Key within the ConfigMap; omit to import all keys as env vars"` - Verbose *bool `json:"verbose,omitempty" jsonschema:"Enable verbose logging output"` + Verbose *bool `json:"verbose,omitempty" jsonschema:"Enable verbose logging output"` } func (i ConfigEnvsAddInput) Args() []string { diff --git a/pkg/mcp/tools_config_envs_test.go b/pkg/mcp/tools_config_envs_test.go index 5f9d866ef3..56bce83fcb 100644 --- a/pkg/mcp/tools_config_envs_test.go +++ b/pkg/mcp/tools_config_envs_test.go @@ -180,8 +180,8 @@ func TestTool_ConfigEnvsAdd_ConfigMapKey(t *testing.T) { result, err := client.CallTool(t.Context(), &mcp.CallToolParams{ Name: "config_envs_add", Arguments: map[string]any{ - "path": ".", - "name": "DATABASE_HOST", + "path": ".", + "name": "DATABASE_HOST", "configMapName": "my-config", "configMapKey": "DB_HOST", }, From 67e657806779cc2da3149730cd3612bab7cb322b Mon Sep 17 00:00:00 2001 From: Ankitsinghsisodya Date: Sat, 9 May 2026 14:37:54 +0530 Subject: [PATCH 3/5] Add validation tests for config_envs_add tool - Introduced new test cases to validate error handling for secretKey and configMapKey when provided without their corresponding names. - Added tests to ensure proper validation of secretName, secretKey, configMapName, and configMapKey against allowed character sets. - Enhanced the ConfigEnvsAddInput struct with a validate method to enforce input constraints before processing. - Updated the config_envs_add tool's functionality to return appropriate errors for invalid inputs, improving robustness and user feedback. --- pkg/mcp/tools_config_envs.go | 37 ++++++- pkg/mcp/tools_config_envs_test.go | 166 ++++++++++++++++++++++++++++++ 2 files changed, 202 insertions(+), 1 deletion(-) diff --git a/pkg/mcp/tools_config_envs.go b/pkg/mcp/tools_config_envs.go index 597b1fcc0d..104f197cca 100644 --- a/pkg/mcp/tools_config_envs.go +++ b/pkg/mcp/tools_config_envs.go @@ -3,10 +3,18 @@ package mcp import ( "context" "fmt" + "regexp" "github.com/modelcontextprotocol/go-sdk/mcp" ) +// validResourceName matches Kubernetes resource names as accepted by the env template syntax. +// Mirrors the name capture group used in pkg/functions/function.go validation regexes. +var validResourceName = regexp.MustCompile(`^((?:\w|['-]\w)+)$`) + +// validKeyName matches Secret/ConfigMap key names as accepted by the env template syntax. +var validKeyName = regexp.MustCompile(`^[-._a-zA-Z0-9]+$`) + // config_envs_list var configEnvsListTool = &mcp.Tool{ @@ -52,7 +60,7 @@ var configEnvsAddTool = &mcp.Tool{ Title: "Config Environment Variables Add", Description: `Adds an environment variable to a function's configuration. -Supports four source types: +Supports six source types: 1. Literal value: set Name and Value directly. 2. Local env var: set Name and Value to "{{ env:LOCAL_VAR }}". 3. Secret (all keys): set SecretName only — imports every key from the Secret as env vars. @@ -82,6 +90,30 @@ type ConfigEnvsAddInput struct { Verbose *bool `json:"verbose,omitempty" jsonschema:"Enable verbose logging output"` } +// validate returns an error for any illegal input combination or character set +// violation before args are constructed and forwarded to the CLI. +func (i ConfigEnvsAddInput) validate() error { + if i.SecretKey != nil && i.SecretName == nil { + return fmt.Errorf("secretKey requires secretName to be set") + } + if i.ConfigMapKey != nil && i.ConfigMapName == nil { + return fmt.Errorf("configMapKey requires configMapName to be set") + } + if i.SecretName != nil && !validResourceName.MatchString(*i.SecretName) { + return fmt.Errorf("invalid secretName %q: only word characters, hyphens and apostrophes are allowed", *i.SecretName) + } + if i.SecretKey != nil && !validKeyName.MatchString(*i.SecretKey) { + return fmt.Errorf("invalid secretKey %q: only alphanumeric characters, hyphens, dots and underscores are allowed", *i.SecretKey) + } + if i.ConfigMapName != nil && !validResourceName.MatchString(*i.ConfigMapName) { + return fmt.Errorf("invalid configMapName %q: only word characters, hyphens and apostrophes are allowed", *i.ConfigMapName) + } + if i.ConfigMapKey != nil && !validKeyName.MatchString(*i.ConfigMapKey) { + return fmt.Errorf("invalid configMapKey %q: only alphanumeric characters, hyphens, dots and underscores are allowed", *i.ConfigMapKey) + } + return nil +} + func (i ConfigEnvsAddInput) Args() []string { args := []string{"envs", "add", "--path", i.Path} args = appendStringFlag(args, "--name", i.Name) @@ -116,6 +148,9 @@ type ConfigEnvsAddOutput struct { } func (s *Server) configEnvsAddHandler(ctx context.Context, r *mcp.CallToolRequest, input ConfigEnvsAddInput) (result *mcp.CallToolResult, output ConfigEnvsAddOutput, err error) { + if err = input.validate(); err != nil { + return + } out, err := s.executor.Execute(ctx, "config", input.Args()...) if err != nil { err = fmt.Errorf("%w\n%s", err, string(out)) diff --git a/pkg/mcp/tools_config_envs_test.go b/pkg/mcp/tools_config_envs_test.go index 56bce83fcb..a824778d20 100644 --- a/pkg/mcp/tools_config_envs_test.go +++ b/pkg/mcp/tools_config_envs_test.go @@ -274,6 +274,172 @@ func TestTool_ConfigEnvsAdd_ValueTakesPrecedence(t *testing.T) { } } +// TestTool_ConfigEnvsAdd_SecretKeyWithoutSecretName ensures a validation error is returned +// when secretKey is provided without secretName (SEVERE-2). +func TestTool_ConfigEnvsAdd_SecretKeyWithoutSecretName(t *testing.T) { + executor := mock.NewExecutor() + executor.ExecuteFn = func(ctx context.Context, subcommand string, args ...string) ([]byte, error) { + t.Fatal("executor must not be called when input is invalid") + return nil, 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": ".", + "name": "MY_VAR", + "secretKey": "MY_KEY", + }, + }) + if err != nil { + t.Fatal(err) + } + if !result.IsError { + t.Fatal("expected error result when secretKey is set without secretName") + } + if executor.ExecuteInvoked { + t.Fatal("executor must not be invoked when input fails validation") + } +} + +// TestTool_ConfigEnvsAdd_ConfigMapKeyWithoutConfigMapName ensures a validation error is +// returned when configMapKey is provided without configMapName (SEVERE-2). +func TestTool_ConfigEnvsAdd_ConfigMapKeyWithoutConfigMapName(t *testing.T) { + executor := mock.NewExecutor() + executor.ExecuteFn = func(ctx context.Context, subcommand string, args ...string) ([]byte, error) { + t.Fatal("executor must not be called when input is invalid") + return nil, 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": ".", + "name": "MY_VAR", + "configMapKey": "MY_KEY", + }, + }) + if err != nil { + t.Fatal(err) + } + if !result.IsError { + t.Fatal("expected error result when configMapKey is set without configMapName") + } + if executor.ExecuteInvoked { + t.Fatal("executor must not be invoked when input fails validation") + } +} + +// TestTool_ConfigEnvsAdd_InvalidSecretName ensures a validation error is returned when +// secretName contains characters outside the allowed set (SEVERE-3). +func TestTool_ConfigEnvsAdd_InvalidSecretName(t *testing.T) { + executor := mock.NewExecutor() + executor.ExecuteFn = func(ctx context.Context, subcommand string, args ...string) ([]byte, error) { + t.Fatal("executor must not be called when input is invalid") + return nil, 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": ".", + "name": "MY_VAR", + "secretName": "evil:inject}}", + }, + }) + if err != nil { + t.Fatal(err) + } + if !result.IsError { + t.Fatal("expected error result when secretName contains invalid characters") + } + if executor.ExecuteInvoked { + t.Fatal("executor must not be invoked when input fails validation") + } +} + +// TestTool_ConfigEnvsAdd_InvalidSecretKey ensures a validation error is returned when +// secretKey contains characters outside the allowed set (SEVERE-3). +func TestTool_ConfigEnvsAdd_InvalidSecretKey(t *testing.T) { + executor := mock.NewExecutor() + executor.ExecuteFn = func(ctx context.Context, subcommand string, args ...string) ([]byte, error) { + t.Fatal("executor must not be called when input is invalid") + return nil, 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": ".", + "name": "MY_VAR", + "secretName": "my-secret", + "secretKey": "bad key with spaces", + }, + }) + if err != nil { + t.Fatal(err) + } + if !result.IsError { + t.Fatal("expected error result when secretKey contains invalid characters") + } + if executor.ExecuteInvoked { + t.Fatal("executor must not be invoked when input fails validation") + } +} + +// TestTool_ConfigEnvsAdd_InvalidConfigMapName ensures a validation error is returned when +// configMapName contains characters outside the allowed set (SEVERE-3). +func TestTool_ConfigEnvsAdd_InvalidConfigMapName(t *testing.T) { + executor := mock.NewExecutor() + executor.ExecuteFn = func(ctx context.Context, subcommand string, args ...string) ([]byte, error) { + t.Fatal("executor must not be called when input is invalid") + return nil, 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": ".", + "name": "MY_VAR", + "configMapName": "bad}name", + }, + }) + if err != nil { + t.Fatal(err) + } + if !result.IsError { + t.Fatal("expected error result when configMapName contains invalid characters") + } + if executor.ExecuteInvoked { + t.Fatal("executor must not be invoked when input fails validation") + } +} + // TestTool_ConfigEnvsList ensures the config_envs_list tool lists environment variables. func TestTool_ConfigEnvsList(t *testing.T) { executor := mock.NewExecutor() From 6736f1b71dcf1365d4feffd3ee8ebaaba294a516 Mon Sep 17 00:00:00 2001 From: Ankitsinghsisodya Date: Sat, 9 May 2026 14:45:36 +0530 Subject: [PATCH 4/5] Enhance validation in config_envs_add tool - Added new validation checks to ensure that the `name` field is not set when importing all keys from a Secret or ConfigMap. - Implemented mutual exclusivity validation for `secretName` and `configMapName` to prevent simultaneous usage. - Updated the `ConfigEnvsAddInput` struct to reflect these validation rules, improving error handling and user feedback. - Introduced additional test cases to validate these new constraints and ensure proper error handling in various scenarios. --- pkg/mcp/tools_config_envs.go | 17 +++- pkg/mcp/tools_config_envs_test.go | 138 ++++++++++++++++++++++++++++-- 2 files changed, 148 insertions(+), 7 deletions(-) diff --git a/pkg/mcp/tools_config_envs.go b/pkg/mcp/tools_config_envs.go index 104f197cca..d345b703d2 100644 --- a/pkg/mcp/tools_config_envs.go +++ b/pkg/mcp/tools_config_envs.go @@ -82,7 +82,7 @@ The explicit Value field takes precedence if provided alongside source fields.`, type ConfigEnvsAddInput struct { Path string `json:"path" jsonschema:"required,Path to the function project directory"` Name *string `json:"name,omitempty" jsonschema:"Name of the environment variable"` - Value *string `json:"value,omitempty" jsonschema:"Literal value for the environment variable"` + Value *string `json:"value,omitempty" jsonschema:"Literal value or template expression (e.g. '{{ env:MY_VAR }}') for the environment variable"` SecretName *string `json:"secretName,omitempty" jsonschema:"Name of the Kubernetes Secret to source the value from"` SecretKey *string `json:"secretKey,omitempty" jsonschema:"Key within the Secret; omit to import all keys as env vars"` ConfigMapName *string `json:"configMapName,omitempty" jsonschema:"Name of the Kubernetes ConfigMap to source the value from"` @@ -99,6 +99,21 @@ func (i ConfigEnvsAddInput) validate() error { if i.ConfigMapKey != nil && i.ConfigMapName == nil { return fmt.Errorf("configMapKey requires configMapName to be set") } + if i.SecretName != nil && i.ConfigMapName != nil { + return fmt.Errorf("secretName and configMapName are mutually exclusive; provide only one source") + } + // All-keys import (no secretKey/configMapKey) is incompatible with --name: the + // underlying env validation only allows whole-secret/configMap templates when name is nil. + // The guard is skipped when an explicit Value is present because Value takes precedence + // and SecretName/ConfigMapName is effectively ignored in that path. + if i.Value == nil { + if i.SecretName != nil && i.SecretKey == nil && i.Name != nil { + return fmt.Errorf("name must not be set when importing all keys from a Secret (omit secretKey to import all keys)") + } + if i.ConfigMapName != nil && i.ConfigMapKey == nil && i.Name != nil { + return fmt.Errorf("name must not be set when importing all keys from a ConfigMap (omit configMapKey to import all keys)") + } + } if i.SecretName != nil && !validResourceName.MatchString(*i.SecretName) { return fmt.Errorf("invalid secretName %q: only word characters, hyphens and apostrophes are allowed", *i.SecretName) } diff --git a/pkg/mcp/tools_config_envs_test.go b/pkg/mcp/tools_config_envs_test.go index a824778d20..a293244fb5 100644 --- a/pkg/mcp/tools_config_envs_test.go +++ b/pkg/mcp/tools_config_envs_test.go @@ -80,8 +80,10 @@ func TestTool_ConfigEnvsAdd_SecretKey(t *testing.T) { if subcommand != "config" { t.Fatalf("expected subcommand 'config', got %q", subcommand) } - - argsMap := argsToMap(args) + if len(args) < 2 || args[0] != "envs" || args[1] != "add" { + t.Fatalf("expected positional args ['envs','add'], got %v", args[:min(2, len(args))]) + } + argsMap := argsToMap(args[2:]) wantValue := "{{ secret:my-secret:MY_KEY }}" if got, ok := argsMap["--value"]; !ok || got != wantValue { t.Fatalf("expected --value=%q, got %q", wantValue, got) @@ -122,7 +124,13 @@ func TestTool_ConfigEnvsAdd_SecretKey(t *testing.T) { func TestTool_ConfigEnvsAdd_SecretAllKeys(t *testing.T) { executor := mock.NewExecutor() executor.ExecuteFn = func(ctx context.Context, subcommand string, args ...string) ([]byte, error) { - argsMap := argsToMap(args) + if subcommand != "config" { + t.Fatalf("expected subcommand 'config', got %q", subcommand) + } + if len(args) < 2 || args[0] != "envs" || args[1] != "add" { + t.Fatalf("expected positional args ['envs','add'], got %v", args[:min(2, len(args))]) + } + argsMap := argsToMap(args[2:]) wantValue := "{{ secret:my-secret }}" if got, ok := argsMap["--value"]; !ok || got != wantValue { t.Fatalf("expected --value=%q, got %q", wantValue, got) @@ -161,7 +169,13 @@ func TestTool_ConfigEnvsAdd_SecretAllKeys(t *testing.T) { func TestTool_ConfigEnvsAdd_ConfigMapKey(t *testing.T) { executor := mock.NewExecutor() executor.ExecuteFn = func(ctx context.Context, subcommand string, args ...string) ([]byte, error) { - argsMap := argsToMap(args) + if subcommand != "config" { + t.Fatalf("expected subcommand 'config', got %q", subcommand) + } + if len(args) < 2 || args[0] != "envs" || args[1] != "add" { + t.Fatalf("expected positional args ['envs','add'], got %v", args[:min(2, len(args))]) + } + argsMap := argsToMap(args[2:]) wantValue := "{{ configMap:my-config:DB_HOST }}" if got, ok := argsMap["--value"]; !ok || got != wantValue { t.Fatalf("expected --value=%q, got %q", wantValue, got) @@ -202,7 +216,13 @@ func TestTool_ConfigEnvsAdd_ConfigMapKey(t *testing.T) { func TestTool_ConfigEnvsAdd_ConfigMapAllKeys(t *testing.T) { executor := mock.NewExecutor() executor.ExecuteFn = func(ctx context.Context, subcommand string, args ...string) ([]byte, error) { - argsMap := argsToMap(args) + if subcommand != "config" { + t.Fatalf("expected subcommand 'config', got %q", subcommand) + } + if len(args) < 2 || args[0] != "envs" || args[1] != "add" { + t.Fatalf("expected positional args ['envs','add'], got %v", args[:min(2, len(args))]) + } + argsMap := argsToMap(args[2:]) wantValue := "{{ configMap:my-config }}" if got, ok := argsMap["--value"]; !ok || got != wantValue { t.Fatalf("expected --value=%q, got %q", wantValue, got) @@ -241,7 +261,13 @@ func TestTool_ConfigEnvsAdd_ConfigMapAllKeys(t *testing.T) { func TestTool_ConfigEnvsAdd_ValueTakesPrecedence(t *testing.T) { executor := mock.NewExecutor() executor.ExecuteFn = func(ctx context.Context, subcommand string, args ...string) ([]byte, error) { - argsMap := argsToMap(args) + if subcommand != "config" { + t.Fatalf("expected subcommand 'config', got %q", subcommand) + } + if len(args) < 2 || args[0] != "envs" || args[1] != "add" { + t.Fatalf("expected positional args ['envs','add'], got %v", args[:min(2, len(args))]) + } + argsMap := argsToMap(args[2:]) wantValue := "explicit-value" if got, ok := argsMap["--value"]; !ok || got != wantValue { t.Fatalf("expected --value=%q, got %q", wantValue, got) @@ -274,6 +300,106 @@ func TestTool_ConfigEnvsAdd_ValueTakesPrecedence(t *testing.T) { } } +// TestTool_ConfigEnvsAdd_NameWithSecretAllKeys ensures a validation error is returned +// when name is provided alongside an all-keys Secret import (no secretKey). +func TestTool_ConfigEnvsAdd_NameWithSecretAllKeys(t *testing.T) { + executor := mock.NewExecutor() + executor.ExecuteFn = func(ctx context.Context, subcommand string, args ...string) ([]byte, error) { + t.Fatal("executor must not be called when input is invalid") + return nil, 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": ".", + "name": "MY_VAR", + "secretName": "my-secret", + }, + }) + if err != nil { + t.Fatal(err) + } + if !result.IsError { + t.Fatal("expected error result when name is set alongside an all-keys Secret import") + } + if executor.ExecuteInvoked { + t.Fatal("executor must not be invoked when input fails validation") + } +} + +// TestTool_ConfigEnvsAdd_NameWithConfigMapAllKeys ensures a validation error is returned +// when name is provided alongside an all-keys ConfigMap import (no configMapKey). +func TestTool_ConfigEnvsAdd_NameWithConfigMapAllKeys(t *testing.T) { + executor := mock.NewExecutor() + executor.ExecuteFn = func(ctx context.Context, subcommand string, args ...string) ([]byte, error) { + t.Fatal("executor must not be called when input is invalid") + return nil, 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": ".", + "name": "MY_VAR", + "configMapName": "my-config", + }, + }) + if err != nil { + t.Fatal(err) + } + if !result.IsError { + t.Fatal("expected error result when name is set alongside an all-keys ConfigMap import") + } + if executor.ExecuteInvoked { + t.Fatal("executor must not be invoked when input fails validation") + } +} + +// TestTool_ConfigEnvsAdd_BothSecretAndConfigMap ensures a validation error is returned +// when both secretName and configMapName are provided simultaneously. +func TestTool_ConfigEnvsAdd_BothSecretAndConfigMap(t *testing.T) { + executor := mock.NewExecutor() + executor.ExecuteFn = func(ctx context.Context, subcommand string, args ...string) ([]byte, error) { + t.Fatal("executor must not be called when input is invalid") + return nil, 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": ".", + "name": "MY_VAR", + "secretName": "my-secret", + "configMapName": "my-config", + }, + }) + if err != nil { + t.Fatal(err) + } + if !result.IsError { + t.Fatal("expected error result when both secretName and configMapName are provided") + } + if executor.ExecuteInvoked { + t.Fatal("executor must not be invoked when input fails validation") + } +} + // TestTool_ConfigEnvsAdd_SecretKeyWithoutSecretName ensures a validation error is returned // when secretKey is provided without secretName (SEVERE-2). func TestTool_ConfigEnvsAdd_SecretKeyWithoutSecretName(t *testing.T) { From 0324b74d02b99747dcb05ca09b4fa9a862ff2c6a Mon Sep 17 00:00:00 2001 From: Ankitsinghsisodya Date: Mon, 11 May 2026 17:56:06 +0530 Subject: [PATCH 5/5] refactor(mcp): drop regex validation, make value/secret exclusive MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit MCP layer was duplicating name validation with regexes that didn't match DNS-1123 (e.g. accepted apostrophes). Deferred to func CLI. Also replaced silent value/secretName precedence with an explicit error — silent drop is a footgun for LLM clients. --- pkg/mcp/tools_config_envs.go | 43 +++------- pkg/mcp/tools_config_envs_test.go | 130 +++--------------------------- 2 files changed, 21 insertions(+), 152 deletions(-) diff --git a/pkg/mcp/tools_config_envs.go b/pkg/mcp/tools_config_envs.go index d345b703d2..397b8c1eef 100644 --- a/pkg/mcp/tools_config_envs.go +++ b/pkg/mcp/tools_config_envs.go @@ -3,18 +3,10 @@ package mcp import ( "context" "fmt" - "regexp" "github.com/modelcontextprotocol/go-sdk/mcp" ) -// validResourceName matches Kubernetes resource names as accepted by the env template syntax. -// Mirrors the name capture group used in pkg/functions/function.go validation regexes. -var validResourceName = regexp.MustCompile(`^((?:\w|['-]\w)+)$`) - -// validKeyName matches Secret/ConfigMap key names as accepted by the env template syntax. -var validKeyName = regexp.MustCompile(`^[-._a-zA-Z0-9]+$`) - // config_envs_list var configEnvsListTool = &mcp.Tool{ @@ -70,7 +62,7 @@ Supports six source types: When SecretName or ConfigMapName is provided, the tool constructs the appropriate "{{ secret:… }}" or "{{ configMap:… }}" value template automatically. -The explicit Value field takes precedence if provided alongside source fields.`, +Value is mutually exclusive with SecretName and ConfigMapName.`, Annotations: &mcp.ToolAnnotations{ Title: "Config Environment Variables Add", ReadOnlyHint: false, @@ -90,9 +82,13 @@ type ConfigEnvsAddInput struct { Verbose *bool `json:"verbose,omitempty" jsonschema:"Enable verbose logging output"` } -// validate returns an error for any illegal input combination or character set -// violation before args are constructed and forwarded to the CLI. +// validate returns an error for any illegal input combination before args are +// 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("value is mutually exclusive with secretName/configMapName; provide one or the other") + } if i.SecretKey != nil && i.SecretName == nil { return fmt.Errorf("secretKey requires secretName to be set") } @@ -104,27 +100,11 @@ func (i ConfigEnvsAddInput) validate() error { } // All-keys import (no secretKey/configMapKey) is incompatible with --name: the // underlying env validation only allows whole-secret/configMap templates when name is nil. - // The guard is skipped when an explicit Value is present because Value takes precedence - // and SecretName/ConfigMapName is effectively ignored in that path. - if i.Value == nil { - if i.SecretName != nil && i.SecretKey == nil && i.Name != nil { - return fmt.Errorf("name must not be set when importing all keys from a Secret (omit secretKey to import all keys)") - } - if i.ConfigMapName != nil && i.ConfigMapKey == nil && i.Name != nil { - return fmt.Errorf("name must not be set when importing all keys from a ConfigMap (omit configMapKey to import all keys)") - } - } - if i.SecretName != nil && !validResourceName.MatchString(*i.SecretName) { - return fmt.Errorf("invalid secretName %q: only word characters, hyphens and apostrophes are allowed", *i.SecretName) - } - if i.SecretKey != nil && !validKeyName.MatchString(*i.SecretKey) { - return fmt.Errorf("invalid secretKey %q: only alphanumeric characters, hyphens, dots and underscores are allowed", *i.SecretKey) - } - if i.ConfigMapName != nil && !validResourceName.MatchString(*i.ConfigMapName) { - return fmt.Errorf("invalid configMapName %q: only word characters, hyphens and apostrophes are allowed", *i.ConfigMapName) + if i.SecretName != nil && i.SecretKey == nil && i.Name != nil { + return fmt.Errorf("name must not be set when importing all keys from a Secret (omit secretKey to import all keys)") } - if i.ConfigMapKey != nil && !validKeyName.MatchString(*i.ConfigMapKey) { - return fmt.Errorf("invalid configMapKey %q: only alphanumeric characters, hyphens, dots and underscores are allowed", *i.ConfigMapKey) + if i.ConfigMapName != nil && i.ConfigMapKey == nil && i.Name != nil { + return fmt.Errorf("name must not be set when importing all keys from a ConfigMap (omit configMapKey to import all keys)") } return nil } @@ -133,7 +113,6 @@ func (i ConfigEnvsAddInput) Args() []string { args := []string{"envs", "add", "--path", i.Path} args = appendStringFlag(args, "--name", i.Name) - // Explicit Value takes precedence over structured secret/configmap fields. if i.Value != nil { args = appendStringFlag(args, "--value", i.Value) } else if i.SecretName != nil { diff --git a/pkg/mcp/tools_config_envs_test.go b/pkg/mcp/tools_config_envs_test.go index a293244fb5..327ab06f9a 100644 --- a/pkg/mcp/tools_config_envs_test.go +++ b/pkg/mcp/tools_config_envs_test.go @@ -256,23 +256,13 @@ func TestTool_ConfigEnvsAdd_ConfigMapAllKeys(t *testing.T) { } } -// TestTool_ConfigEnvsAdd_ValueTakesPrecedence ensures that when both Value and -// SecretName are provided, the explicit Value is used. -func TestTool_ConfigEnvsAdd_ValueTakesPrecedence(t *testing.T) { +// TestTool_ConfigEnvsAdd_ValueAndSecretMutuallyExclusive ensures a validation +// error is returned when both Value and SecretName are provided together. +func TestTool_ConfigEnvsAdd_ValueAndSecretMutuallyExclusive(t *testing.T) { 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) - } - if len(args) < 2 || args[0] != "envs" || args[1] != "add" { - t.Fatalf("expected positional args ['envs','add'], got %v", args[:min(2, len(args))]) - } - argsMap := argsToMap(args[2:]) - wantValue := "explicit-value" - if got, ok := argsMap["--value"]; !ok || got != wantValue { - t.Fatalf("expected --value=%q, got %q", wantValue, got) - } - return []byte("Environment variable added successfully\n"), nil + t.Fatal("executor must not be called when input is invalid") + return nil, nil } client, _, err := newTestPair(t, WithExecutor(executor)) @@ -286,17 +276,17 @@ func TestTool_ConfigEnvsAdd_ValueTakesPrecedence(t *testing.T) { "path": ".", "name": "MY_VAR", "value": "explicit-value", - "secretName": "ignored-secret", + "secretName": "my-secret", }, }) if err != nil { t.Fatal(err) } - if result.IsError { - t.Fatalf("unexpected error result: %v", result) + if !result.IsError { + t.Fatal("expected error result when value and secretName are both provided") } - if !executor.ExecuteInvoked { - t.Fatal("executor was not invoked") + if executor.ExecuteInvoked { + t.Fatal("executor must not be invoked when input fails validation") } } @@ -466,106 +456,6 @@ func TestTool_ConfigEnvsAdd_ConfigMapKeyWithoutConfigMapName(t *testing.T) { } } -// TestTool_ConfigEnvsAdd_InvalidSecretName ensures a validation error is returned when -// secretName contains characters outside the allowed set (SEVERE-3). -func TestTool_ConfigEnvsAdd_InvalidSecretName(t *testing.T) { - executor := mock.NewExecutor() - executor.ExecuteFn = func(ctx context.Context, subcommand string, args ...string) ([]byte, error) { - t.Fatal("executor must not be called when input is invalid") - return nil, 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": ".", - "name": "MY_VAR", - "secretName": "evil:inject}}", - }, - }) - if err != nil { - t.Fatal(err) - } - if !result.IsError { - t.Fatal("expected error result when secretName contains invalid characters") - } - if executor.ExecuteInvoked { - t.Fatal("executor must not be invoked when input fails validation") - } -} - -// TestTool_ConfigEnvsAdd_InvalidSecretKey ensures a validation error is returned when -// secretKey contains characters outside the allowed set (SEVERE-3). -func TestTool_ConfigEnvsAdd_InvalidSecretKey(t *testing.T) { - executor := mock.NewExecutor() - executor.ExecuteFn = func(ctx context.Context, subcommand string, args ...string) ([]byte, error) { - t.Fatal("executor must not be called when input is invalid") - return nil, 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": ".", - "name": "MY_VAR", - "secretName": "my-secret", - "secretKey": "bad key with spaces", - }, - }) - if err != nil { - t.Fatal(err) - } - if !result.IsError { - t.Fatal("expected error result when secretKey contains invalid characters") - } - if executor.ExecuteInvoked { - t.Fatal("executor must not be invoked when input fails validation") - } -} - -// TestTool_ConfigEnvsAdd_InvalidConfigMapName ensures a validation error is returned when -// configMapName contains characters outside the allowed set (SEVERE-3). -func TestTool_ConfigEnvsAdd_InvalidConfigMapName(t *testing.T) { - executor := mock.NewExecutor() - executor.ExecuteFn = func(ctx context.Context, subcommand string, args ...string) ([]byte, error) { - t.Fatal("executor must not be called when input is invalid") - return nil, 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": ".", - "name": "MY_VAR", - "configMapName": "bad}name", - }, - }) - if err != nil { - t.Fatal(err) - } - if !result.IsError { - t.Fatal("expected error result when configMapName contains invalid characters") - } - if executor.ExecuteInvoked { - t.Fatal("executor must not be invoked when input fails validation") - } -} - // TestTool_ConfigEnvsList ensures the config_envs_list tool lists environment variables. func TestTool_ConfigEnvsList(t *testing.T) { executor := mock.NewExecutor()