fix: MCP server actor permission check fails closed on repo lookup error (#23285)

Author: Copilot

pkg/cli/mcp_server_helpers.go

@@ -196,15 +196,20 @@ func checkActorPermission(ctx context.Context, actor string, validateActor bool,
 	// Get repository using cached lookup
 	repo, err := getRepository()
 	if err != nil {
-		mcpLog.Printf("Tool %s: failed to get repository context, allowing access: %v", toolName, err)
-		// If we can't determine the repository, allow access (fail open)
-		return nil
+		mcpLog.Printf("Tool %s: failed to get repository context, denying access: %v", toolName, err)
+		return newMCPError(jsonrpc.CodeInternalError, "permission check failed", map[string]any{
+			"error":  err.Error(),
+			"tool":   toolName,
+			"reason": "Could not determine repository context to verify permissions. Ensure you are running from within a git repository with gh authenticated.",
+		})
 	}
 
 	if repo == "" {
-		mcpLog.Printf("Tool %s: no repository context, allowing access", toolName)
-		// No repository context, allow access
-		return nil
+		mcpLog.Printf("Tool %s: no repository context, denying access", toolName)
+		return newMCPError(jsonrpc.CodeInvalidRequest, "permission check failed", map[string]any{
+			"tool":   toolName,
+			"reason": "No repository context available. Run from within a git repository.",
+		})
 	}
 
 	// Query actor's role in the repository with caching

pkg/cli/mcp_server_helpers_test.go

@@ -3,6 +3,7 @@
 package cli
 
 import (
+	"context"
 	"encoding/json"
 	"testing"
 )
@@ -112,3 +113,39 @@ func TestValidateWorkflowName_Empty(t *testing.T) {
 		t.Errorf("validateWorkflowName(\"\") returned error: %v", err)
 	}
 }
+
+// TestCheckActorPermission_AllowsWhenValidationDisabled verifies that access is
+// always granted when validateActor is false, regardless of actor or repo context.
+func TestCheckActorPermission_AllowsWhenValidationDisabled(t *testing.T) {
+	err := checkActorPermission(context.Background(), "", false, "logs")
+	if err != nil {
+		t.Errorf("checkActorPermission with validateActor=false: expected nil, got %v", err)
+	}
+}
+
+// TestCheckActorPermission_DeniesWhenNoActorAndValidationEnabled verifies that access
+// is denied when actor is empty and validation is enabled.
+func TestCheckActorPermission_DeniesWhenNoActorAndValidationEnabled(t *testing.T) {
+	err := checkActorPermission(context.Background(), "", true, "logs")
+	if err == nil {
+		t.Error("checkActorPermission with empty actor and validateActor=true: expected error, got nil")
+	}
+}
+
+// TestCheckActorPermission_DeniesWhenNoRepoContext verifies the fail-closed behavior
+// when no repository context is available (GITHUB_REPOSITORY unset, no git context).
+// This tests the security fix: permission check must fail closed, not open.
+func TestCheckActorPermission_DeniesWhenNoRepoContext(t *testing.T) {
+	// Ensure no GITHUB_REPOSITORY is set (and cache is empty) so getRepository fails/returns "".
+	t.Setenv("GITHUB_REPOSITORY", "")
+
+	// Clear any cached repo value so getRepository does a fresh lookup.
+	mcpCache.SetRepo("")
+
+	// With GITHUB_REPOSITORY="" and no git context in the test environment,
+	// getRepository will either return "" or an error — both paths must deny access.
+	err := checkActorPermission(context.Background(), "someuser", true, "logs")
+	if err == nil {
+		t.Error("checkActorPermission with no repo context: expected error (fail closed), got nil (fail open)")
+	}
+}