From a0ac94de1d3f5059352224f7c54bf486fcf80c45 Mon Sep 17 00:00:00 2001 From: Ankitsinghsisodya Date: Fri, 15 May 2026 05:06:25 +0530 Subject: [PATCH 1/2] fix: prevent image push in read-only mode Added a check in the buildHandler function to return an error if an attempt is made to push images while the server is in read-only mode. This ensures that users are informed of the correct server state and can adjust their configurations accordingly. Co-authored-by: Cursor --- pkg/mcp/tools_build.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/pkg/mcp/tools_build.go b/pkg/mcp/tools_build.go index e1f6a6758f..8dfa54936a 100644 --- a/pkg/mcp/tools_build.go +++ b/pkg/mcp/tools_build.go @@ -20,11 +20,10 @@ var buildTool = &mcp.Tool{ } func (s *Server) buildHandler(ctx context.Context, r *mcp.CallToolRequest, input BuildInput) (result *mcp.CallToolResult, output BuildOutput, err error) { - if s.readonly.Load() { - err = fmt.Errorf("the server is currently in read-only mode; to enable write operations, set FUNC_ENABLE_MCP_WRITE in the server environment and restart the server") + if s.readonly.Load() && input.Push != nil && *input.Push { + err = fmt.Errorf("the server is in read-only mode; set FUNC_ENABLE_MCP_WRITE=true to push images") return } - out, err := s.executor.Execute(ctx, "build", input.Args()...) if err != nil { err = fmt.Errorf("%w\n%s", err, string(out)) From c129d489b7b4aef3d5bc338ad168757a0d0acb3a Mon Sep 17 00:00:00 2001 From: Ankitsinghsisodya Date: Fri, 12 Jun 2026 17:36:24 +0530 Subject: [PATCH 2/2] fix(mcp): address review feedback - extract EnvMCPWrite constant - Define EnvMCPWrite constant in pkg/mcp to replace the magic string "FUNC_ENABLE_MCP_WRITE" scattered across error messages - Update tools_build.go, tools_deploy.go, and tools_delete.go to use the constant so the name can never drift out of sync with cmd/mcp.go - Update cmd/mcp.go to read the env var and format error messages via the same constant - Add TestTool_Build_ReadonlyPushRejected and TestTool_Build_ReadonlyWithoutPushAllowed to cover the push guard Addresses Copilot review comments on #3828. Co-authored-by: Cursor --- cmd/mcp.go | 4 +-- pkg/mcp/mcp.go | 5 ++++ pkg/mcp/tools_build.go | 2 +- pkg/mcp/tools_build_test.go | 49 +++++++++++++++++++++++++++++++++++++ pkg/mcp/tools_delete.go | 2 +- pkg/mcp/tools_deploy.go | 2 +- 6 files changed, 59 insertions(+), 5 deletions(-) diff --git a/cmd/mcp.go b/cmd/mcp.go index 391817001c..4c1f6d41b5 100644 --- a/cmd/mcp.go +++ b/cmd/mcp.go @@ -98,10 +98,10 @@ DESCRIPTION func runMCPStart(cmd *cobra.Command, args []string, newClient ClientFactory) error { // Configure write mode writeEnabled := false - if val := os.Getenv("FUNC_ENABLE_MCP_WRITE"); val != "" { + if val := os.Getenv(mcp.EnvMCPWrite); val != "" { parsed, err := strconv.ParseBool(val) if err != nil { - return fmt.Errorf("FUNC_ENABLE_MCP_WRITE should be a boolean (true/false, 1/0, etc). Received %q", val) + return fmt.Errorf("%s should be a boolean (true/false, 1/0, etc). Received %q", mcp.EnvMCPWrite, val) } writeEnabled = parsed } diff --git a/pkg/mcp/mcp.go b/pkg/mcp/mcp.go index 02ea0f5d44..06290c9239 100644 --- a/pkg/mcp/mcp.go +++ b/pkg/mcp/mcp.go @@ -14,6 +14,11 @@ const ( name = "func" title = "func" version = "0.1.0" + + // EnvMCPWrite is the environment variable that enables write operations + // (build, deploy, delete) on the MCP server. Set to "true" to allow + // mutations; the server runs in read-only mode by default. + EnvMCPWrite = "FUNC_ENABLE_MCP_WRITE" ) // NOTE: Invoking prompts in some interfaces (such as Claude Code) when all diff --git a/pkg/mcp/tools_build.go b/pkg/mcp/tools_build.go index 8dfa54936a..1332dbc497 100644 --- a/pkg/mcp/tools_build.go +++ b/pkg/mcp/tools_build.go @@ -21,7 +21,7 @@ var buildTool = &mcp.Tool{ func (s *Server) buildHandler(ctx context.Context, r *mcp.CallToolRequest, input BuildInput) (result *mcp.CallToolResult, output BuildOutput, err error) { if s.readonly.Load() && input.Push != nil && *input.Push { - err = fmt.Errorf("the server is in read-only mode; set FUNC_ENABLE_MCP_WRITE=true to push images") + err = fmt.Errorf("pushing images is not allowed in read-only mode; set %s=true to enable write operations", EnvMCPWrite) return } out, err := s.executor.Execute(ctx, "build", input.Args()...) diff --git a/pkg/mcp/tools_build_test.go b/pkg/mcp/tools_build_test.go index 310fce45e0..56453835ec 100644 --- a/pkg/mcp/tools_build_test.go +++ b/pkg/mcp/tools_build_test.go @@ -8,6 +8,55 @@ import ( "knative.dev/func/pkg/mcp/mock" ) +// TestTool_Build_ReadonlyPushRejected ensures build with push=true is rejected in readonly mode. +func TestTool_Build_ReadonlyPushRejected(t *testing.T) { + client, _, err := newTestPairWithReadonly(t, true) // readonly = true + if err != nil { + t.Fatal(err) + } + + result, err := client.CallTool(t.Context(), &mcp.CallToolParams{ + Name: "build", + Arguments: map[string]any{"path": ".", "push": true}, + }) + if err != nil { + t.Fatal(err) + } + if !result.IsError { + t.Fatal("expected build --push to be rejected in readonly mode") + } +} + +// TestTool_Build_ReadonlyWithoutPushAllowed ensures build without push is allowed in readonly mode. +func TestTool_Build_ReadonlyWithoutPushAllowed(t *testing.T) { + executor := mock.NewExecutor() + executor.ExecuteFn = func(_ context.Context, subcommand string, _ ...string) ([]byte, error) { + if subcommand != "build" { + t.Fatalf("expected subcommand 'build', got %q", subcommand) + } + return []byte("OK\n"), nil + } + + client, _, err := newTestPair(t, WithReadonly(true), WithExecutor(executor)) + if err != nil { + t.Fatal(err) + } + + result, err := client.CallTool(t.Context(), &mcp.CallToolParams{ + Name: "build", + Arguments: map[string]any{"path": "."}, + }) + if err != nil { + t.Fatal(err) + } + if result.IsError { + t.Fatalf("expected build without push to succeed in readonly mode, got error: %v", result) + } + if !executor.ExecuteInvoked { + t.Fatal("executor was not invoked") + } +} + // 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 diff --git a/pkg/mcp/tools_delete.go b/pkg/mcp/tools_delete.go index 27760f864d..41ad8009e5 100644 --- a/pkg/mcp/tools_delete.go +++ b/pkg/mcp/tools_delete.go @@ -21,7 +21,7 @@ var deleteTool = &mcp.Tool{ func (s *Server) deleteHandler(ctx context.Context, r *mcp.CallToolRequest, input DeleteInput) (result *mcp.CallToolResult, output DeleteOutput, err error) { if s.readonly.Load() { - err = fmt.Errorf("the server is currently in readonly mode. Please set FUNC_ENABLE_MCP_WRITE and restart the client") + err = fmt.Errorf("delete is not allowed in read-only mode; set %s=true to enable write operations", EnvMCPWrite) return } diff --git a/pkg/mcp/tools_deploy.go b/pkg/mcp/tools_deploy.go index 09c7231656..e252d9fd7d 100644 --- a/pkg/mcp/tools_deploy.go +++ b/pkg/mcp/tools_deploy.go @@ -21,7 +21,7 @@ var deployTool = &mcp.Tool{ func (s *Server) deployHandler(ctx context.Context, r *mcp.CallToolRequest, input DeployInput) (result *mcp.CallToolResult, output DeployOutput, err error) { if s.readonly.Load() { - err = fmt.Errorf("the server is currently in readonly mode. Please set FUNC_ENABLE_MCP_WRITE and restart the client") + err = fmt.Errorf("deploy is not allowed in read-only mode; set %s=true to enable write operations", EnvMCPWrite) return } out, err := s.executor.Execute(ctx, "deploy", input.Args()...)