fix: MCP server actor permission check fails open on repo lookup error

[lpcox]

Problem

When gh aw mcp server runs with --validate-actor, the checkActorPermission function gates logs and audit tools behind a write-access check. However, if the repository context cannot be determined (transient gh CLI error, missing git context, network failure), the function grants access instead of denying it.

This is a classic fail-open authorization anti-pattern. While the preconditions are narrow (MCP server mode + transient lookup failure + attacker timing), it violates the principle that authorization checks should fail closed.

Upstream tracking: https://github.com/github/agentic-workflows/issues/184
Severity: Low (read-only impact on logs/artifacts, opportunistic, not deterministic)

Affected Code

File: pkg/cli/mcp_server_helpers.go lines 196-208

// 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  // ← BUG: grants access when check cannot complete
}

if repo == "" {
    mcpLog.Printf("Tool %s: no repository context, allowing access", toolName)
    // No repository context, allow access
    return nil  // ← BUG: grants access when no repo context
}

Call sites (both in pkg/cli/mcp_tools_privileged.go):

• Line 75: checkActorPermission(ctx, actor, validateActor, "logs")
• Line 261: checkActorPermission(ctx, actor, validateActor, "audit")

Reproduction

# Run MCP server from a non-git directory with actor validation enabled
cd /tmp
GITHUB_ACTOR=someuser gh aw mcp server --validate-actor

# Send a logs tool request → access is granted despite no repo context
# Server logs: "Tool logs: no repository context, allowing access"

Solution

Part 1: Change both fail-open paths to fail-closed

Replace lines 196-208 in pkg/cli/mcp_server_helpers.go:

Before:

repo, err := getRepository()
if err != nil {
    mcpLog.Printf("Tool %s: failed to get repository context, allowing access: %v", toolName, err)
    return nil
}

if repo == "" {
    mcpLog.Printf("Tool %s: no repository context, allowing access", toolName)
    return nil
}

After:

repo, err := getRepository()
if err != 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, 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.",
    })
}

Key changes:

• return nil → return newMCPError(...) (fail closed)
• Log messages changed from "allowing access" → "denying access"
• Error messages include actionable guidance for the user
• Use CodeInternalError for transient failures, CodeInvalidRequest for missing context

Part 2: Add tests

Add tests to pkg/cli/mcp_server_helpers_test.go covering the fail-closed behavior. The existing test file already has TestHasWriteAccess — add companion tests:

func TestCheckActorPermission_DeniesWhenValidationEnabledAndNoActor(t *testing.T) {
    err := checkActorPermission(context.Background(), "", true, "logs")
    assert.Error(t, err, "Should deny when no actor and validation enabled")
    assert.Contains(t, err.Error(), "permission denied")
}

Note: checkActorPermission calls getRepository() which uses os.Getenv("GITHUB_REPOSITORY") and falls back to gh repo view. Tests should either:

• Set GITHUB_REPOSITORY env var for the repo-available case
• Ensure no env var and no git context for the fail-closed test
• Or refactor getRepository to be injectable (optional, more invasive)

The simplest approach for testing the fail-closed paths is to test from a context where GITHUB_REPOSITORY is unset and gh repo view fails (which is the default in test environments).

Part 3: Update log messages

Ensure consistency — the queryActorRole error path at line 215 already correctly fails closed:

if err != nil {
    mcpLog.Printf("Tool %s: failed to query actor role, denying access: %v", toolName, err)
    return newMCPError(...)
}

The fix makes the getRepository error paths match this existing pattern.

Validation

1. Run the specific tests: go test -v -run "TestCheckActorPermission" ./pkg/cli/
2. Run make test-unit to verify no regressions
3. Run make agent-finish before committing
4. Manual verification: run gh aw mcp server --validate-actor from a non-git directory and confirm the tool returns an error instead of allowing access

[lpcox]

@pelikhan straightforward fix to one of the lower priority outcomes of the security review