From 43837afca3285cf8aae6a19dc907146355d24b20 Mon Sep 17 00:00:00 2001 From: Thieu Nguyen <79964592+thieung@users.noreply.github.com> Date: Fri, 5 Jun 2026 00:48:38 +0700 Subject: [PATCH] fix(security): consolidate & enhance batched security fixes (#1155, #967, #972, #974, #989, #973) (#1185) * fix(sandbox): avoid shell in FsBridge writes Replace sh -c with interpolated path by shell-free 'tee -- ' argv form, piping content via stdin. Prevents command injection through filenames containing shell metacharacters inside the sandbox container. Co-authored-by: evgyur * fix(security): fail-closed on pairing DB errors across channels On IsPaired lookup error, deny instead of granting access. Covers the shared CheckDMPolicy/CheckGroupPolicy helpers (Slack/Discord/Feishu/WhatsApp/Zalo) and the four inline Telegram pairing checks. Co-authored-by: Srini * fix(security): harden provider URL validation against SSRF Enforce scheme check for all provider types; restrict local types (ollama, claude_cli, acp) to an explicit localhost allowlist instead of skipping checks; resolve remote hostnames and reject any IP in a private/reserved range via the shared security.IsBlocked CIDR list (covers loopback, link-local, metadata, multicast, and unspecified 0.0.0.0/::). Closes the wildcard-DNS bypass and the local-type escape hatch. Operator opt-in via GOCLAW_ALLOW_PRIVATE_PROVIDER_URLS. Exports security.IsBlocked as the single source of truth for blocked ranges. Co-authored-by: Linh Vo Van * feat(pipeline): add fail-closed tool call authorization gate Gate tool execution against the server-side AllowedTools allowlist built from the RBAC/tenant-aware filtered tool set. Resolve the tool-call prefix before the allowlist lookup so prefixed agents are not wrongly blocked, re-check deny on lazy MCP activation, and expand IsDenied to cover aliased tool names. Co-authored-by: Huy Doan * fix(security): expand file-serve deny-list defense-in-depth Add absolute-path deny prefixes (/home, /Users, /srv, /var/lib, /var/www, /opt) and an explicit fail-closed log when no file-serving boundary is configured. Co-authored-by: Linh Vo Van * fix(providers): allow claude cli executable paths Refs: #1185 --------- Co-authored-by: evgyur Co-authored-by: Srini Co-authored-by: Linh Vo Van Co-authored-by: Huy Doan --- docs/02-providers.md | 2 +- internal/agent/loop_lazy_mcp_test.go | 33 +- internal/agent/loop_pipeline_adapter.go | 1 + internal/agent/loop_pipeline_callbacks.go | 40 +++ internal/channels/channel.go | 8 +- internal/channels/policy_test.go | 12 +- internal/channels/telegram/handlers.go | 12 +- internal/http/files.go | 17 +- internal/http/files_path_security_test.go | 54 +++ internal/http/oauth_test.go | 6 + internal/http/providers.go | 131 +++++-- internal/http/providers_test.go | 73 ++++ internal/http/providers_url_validate_test.go | 326 ++++++++++++++++++ internal/pipeline/deps.go | 3 + internal/pipeline/substates.go | 3 + internal/pipeline/think_stage.go | 7 + internal/pipeline/tool_stage.go | 45 ++- .../pipeline/tool_stage_authorize_test.go | 216 ++++++++++++ internal/providers/defaults.go | 5 + internal/providers/embedding_openai.go | 7 +- internal/providers/vertex.go | 6 +- internal/sandbox/docker_test.go | 28 -- internal/sandbox/fsbridge.go | 15 +- internal/sandbox/fsbridge_test.go | 189 ++++++++++ internal/security/ssrf.go | 10 + internal/tools/policy.go | 25 +- 26 files changed, 1182 insertions(+), 92 deletions(-) create mode 100644 internal/http/providers_url_validate_test.go create mode 100644 internal/pipeline/tool_stage_authorize_test.go create mode 100644 internal/sandbox/fsbridge_test.go diff --git a/docs/02-providers.md b/docs/02-providers.md index 642464e7..7475fad5 100644 --- a/docs/02-providers.md +++ b/docs/02-providers.md @@ -554,7 +554,7 @@ ClaudeCLIProvider can be configured in `config.json`: } ``` -Or via database `llm_providers` table with `provider_type = "claude_cli"`. +Or via database `llm_providers` table with `provider_type = "claude_cli"`. For database providers, `api_base` is the CLI executable selector (`"claude"` or an absolute binary path), not an HTTP base URL, so provider URL SSRF opt-ins do not apply to Claude CLI. ### Session Management diff --git a/internal/agent/loop_lazy_mcp_test.go b/internal/agent/loop_lazy_mcp_test.go index c5deea7c..51f0ba7e 100644 --- a/internal/agent/loop_lazy_mcp_test.go +++ b/internal/agent/loop_lazy_mcp_test.go @@ -5,12 +5,14 @@ import ( "testing" "github.com/nextlevelbuilder/goclaw/internal/config" + "github.com/nextlevelbuilder/goclaw/internal/pipeline" + "github.com/nextlevelbuilder/goclaw/internal/providers" "github.com/nextlevelbuilder/goclaw/internal/tools" ) // mockExecTool is a simple tool that records whether it was executed. type mockExecTool struct { - name string + name string executed bool } @@ -24,13 +26,16 @@ func (m *mockExecTool) Execute(_ context.Context, _ map[string]any) *tools.Resul return tools.NewResult("ok from " + m.name) } -// simulateLazyActivationCheck mimics the allowedTools check from loop.go for one tool call: +// simulateLazyActivationCheck mimics the runtime authorize gate from +// makeAuthorizeToolCall (loop_pipeline_callbacks.go) for one tool call. +// The real gate is a PipelineDeps.AuthorizeToolCall callback invoked by ToolStage; +// this helper exercises the same logic in isolation so tests stay unit-level. // -// if allowedTools != nil && !allowedTools[tc.Name] { -// if l.tools.TryActivateDeferred(tc.Name) { allowedTools[tc.Name] = true } +// if allowed != nil && !allowed[name] { +// if reg.TryActivateDeferred(name) { allowed[name] = true } // else { result = ErrorResult(...) } // } -// if result == nil { result = l.tools.ExecuteWithContext(...) } +// if result == nil { result = reg.ExecuteWithContext(...) } // // Returns (result, blocked). func simulateLazyActivationCheck(reg *tools.Registry, allowedTools map[string]bool, toolName string) (*tools.Result, bool) { @@ -163,6 +168,24 @@ func TestLoop_LazyMCP_NilAllowedTools_AllowsAll(t *testing.T) { } } +func TestLoopAuthorizeToolCall_PrefixedNameCanonicalLookup(t *testing.T) { + loop := NewLoop(LoopConfig{ + AgentToolPolicy: &config.ToolPolicySpec{ToolCallPrefix: "proxy_"}, + }) + gate := loop.makeAuthorizeToolCall() + state := &pipeline.RunState{} + state.Tool.AllowedTools = map[string]bool{"exec": true} + + ok, reason := gate(context.Background(), state, providers.ToolCall{ + ID: "tc-proxy-exec", + Name: "proxy_exec", + }) + + if !ok { + t.Fatalf("prefixed tool call should resolve to canonical allowlist entry; reason: %q", reason) + } +} + func TestLoop_LazyMCP_SecondCall_UsesUpdatedAllowedTools(t *testing.T) { // After first lazy activation, subsequent calls in the same iteration use the // updated allowedTools map and don't invoke the activator again. diff --git a/internal/agent/loop_pipeline_adapter.go b/internal/agent/loop_pipeline_adapter.go index 13f6325b..3ab34798 100644 --- a/internal/agent/loop_pipeline_adapter.go +++ b/internal/agent/loop_pipeline_adapter.go @@ -141,6 +141,7 @@ func (l *Loop) buildPipelineDeps(req *RunRequest, bridgeRS *runState) pipeline.P ExecuteToolCall: cb.executeToolCall, ExecuteToolRaw: cb.executeToolRaw, ProcessToolResult: cb.processToolResult, + AuthorizeToolCall: cb.authorizeToolCall, SequentialToolCall: func(tc providers.ToolCall) bool { return l.resolveToolCallName(tc.Name) == "wait" }, diff --git a/internal/agent/loop_pipeline_callbacks.go b/internal/agent/loop_pipeline_callbacks.go index 9d19e0b6..740312eb 100644 --- a/internal/agent/loop_pipeline_callbacks.go +++ b/internal/agent/loop_pipeline_callbacks.go @@ -57,6 +57,7 @@ func (l *Loop) pipelineCallbacks(req *RunRequest, bridgeRS *runState) pipelineCa executeToolCall: l.makeExecuteToolCall(req, bridgeRS), executeToolRaw: l.makeExecuteToolRaw(req), processToolResult: l.makeProcessToolResult(req, bridgeRS), + authorizeToolCall: l.makeAuthorizeToolCall(), checkReadOnly: l.makeCheckReadOnly(req, bridgeRS), sanitizeContent: SanitizeAssistantContent, flushMessages: l.makeFlushMessages(req), @@ -85,6 +86,7 @@ type pipelineCallbackSet struct { executeToolCall func(ctx context.Context, state *pipeline.RunState, tc providers.ToolCall) ([]providers.Message, error) executeToolRaw func(ctx context.Context, tc providers.ToolCall) (providers.Message, any, error) processToolResult func(ctx context.Context, state *pipeline.RunState, tc providers.ToolCall, rawMsg providers.Message, rawData any) []providers.Message + authorizeToolCall func(ctx context.Context, state *pipeline.RunState, tc providers.ToolCall) (bool, string) checkReadOnly func(state *pipeline.RunState) (*providers.Message, bool) sanitizeContent func(string) string flushMessages func(ctx context.Context, sessionKey string, msgs []providers.Message) error @@ -249,6 +251,44 @@ func (l *Loop) makeBuildFilteredTools(req *RunRequest) func(state *pipeline.RunS } } +// makeAuthorizeToolCall enforces a runtime fail-closed allowlist check before +// every tool execution. AllowedTools is keyed by canonical registry names (built +// by ThinkStage from FilterTools output). The model may emit prefixed names when +// the agent has toolCallPrefix configured (e.g. "proxy_exec" → canonical "exec"), +// so we resolve the name to its canonical form before the lookup to avoid a +// guaranteed miss on every prefixed call. +func (l *Loop) makeAuthorizeToolCall() func(ctx context.Context, state *pipeline.RunState, tc providers.ToolCall) (bool, string) { + return func(_ context.Context, state *pipeline.RunState, tc providers.ToolCall) (bool, string) { + allowed := state.Tool.AllowedTools + if allowed == nil { + // nil allowlist means no per-iteration restriction (e.g. BuildFilteredTools not wired). + return true, "" + } + + // Resolve to canonical name before allowlist lookup. AllowedTools is keyed + // by canonical names; the model may emit prefixed names when toolCallPrefix + // is set (e.g. "proxy_exec" vs "exec"). Without this the lookup always misses. + name := l.resolveToolCallName(tc.Name) + + if allowed[name] { + return true, "" + } + + // Preserve lazy activation for deferred tools (typically per-user MCP). + if l.tools != nil && l.tools.TryActivateDeferred(name) { + // Re-check deny policy to prevent a lazy-activated tool from bypassing + // an explicit deny rule. + if l.toolPolicy != nil && l.toolPolicy.IsDenied(name, l.agentToolPolicy) { + return false, "tool not allowed by policy: " + name + } + allowed[name] = true + return true, "" + } + + return false, "tool not allowed by policy: " + name + } +} + func (l *Loop) makeCallLLM(req *RunRequest, emitRun func(AgentEvent)) func(ctx context.Context, state *pipeline.RunState, chatReq providers.ChatRequest) (*providers.ChatResponse, error) { return func(ctx context.Context, state *pipeline.RunState, chatReq providers.ChatRequest) (*providers.ChatResponse, error) { provider := state.Provider diff --git a/internal/channels/channel.go b/internal/channels/channel.go index 2826d880..545d651a 100644 --- a/internal/channels/channel.go +++ b/internal/channels/channel.go @@ -340,9 +340,9 @@ func (c *BaseChannel) CheckDMPolicy(ctx context.Context, senderID, dmPolicy stri if c.pairingService != nil { paired, err := c.pairingService.IsPaired(ctx, senderID, c.name) if err != nil { - slog.Warn("security.pairing_check_failed, assuming paired (fail-open)", + slog.Warn("security.pairing_check_failed, denying access (fail-closed)", "sender_id", senderID, "channel", c.name, "error", err) - return PolicyAllow + return PolicyDeny } if paired { return PolicyAllow @@ -379,9 +379,9 @@ func (c *BaseChannel) CheckGroupPolicy(ctx context.Context, senderID, chatID, gr if c.pairingService != nil { paired, err := c.pairingService.IsPaired(ctx, groupSenderID, c.name) if err != nil { - slog.Warn("security.pairing_check_failed, assuming paired (fail-open)", + slog.Warn("security.pairing_check_failed, denying access (fail-closed)", "group_sender", groupSenderID, "channel", c.name, "error", err) - return PolicyAllow + return PolicyDeny } if paired { c.MarkGroupApproved(chatID) diff --git a/internal/channels/policy_test.go b/internal/channels/policy_test.go index 0c0d165e..398163c0 100644 --- a/internal/channels/policy_test.go +++ b/internal/channels/policy_test.go @@ -174,12 +174,12 @@ func TestCheckDMPolicy_PolicyPairing(t *testing.T) { wantResult: PolicyAllow, }, { - name: "Pairing service error allows message (fail-open)", + name: "Pairing service error denies message (fail-closed)", senderID: "user999", allowList: []string{}, paired: false, failPairingCheck: true, - wantResult: PolicyAllow, + wantResult: PolicyDeny, }, } @@ -319,14 +319,14 @@ func TestCheckGroupPolicy_PolicyPairing(t *testing.T) { wantResult: PolicyNeedsPairing, }, { - name: "Pairing service error allows (fail-open)", + name: "Pairing service error denies (fail-closed)", senderID: "user999", chatID: "chat_2", allowList: []string{}, groupApproved: false, paired: false, failPairingCheck: true, - wantResult: PolicyAllow, + wantResult: PolicyDeny, }, } @@ -553,13 +553,13 @@ func TestCheckDMPolicy_AllPolicies_TableDriven(t *testing.T) { wantResult: PolicyNeedsPairing, }, { - name: "pairing policy fail-open on service error", + name: "pairing policy fail-closed on service error", policy: "pairing", senderID: "user3", allowList: []string{}, paired: false, failPairingCheck: true, - wantResult: PolicyAllow, + wantResult: PolicyDeny, }, } diff --git a/internal/channels/telegram/handlers.go b/internal/channels/telegram/handlers.go index bd92c93e..194d2a89 100644 --- a/internal/channels/telegram/handlers.go +++ b/internal/channels/telegram/handlers.go @@ -159,9 +159,8 @@ func (c *Channel) handleMessage(ctx context.Context, update telego.Update) { p1, err1 := ps.IsPaired(ctx, userID, c.Name()) p2, err2 := ps.IsPaired(ctx, senderID, c.Name()) if err1 != nil || err2 != nil { - slog.Warn("security.pairing_check_failed, assuming paired (fail-open)", + slog.Warn("security.pairing_check_failed, denying access (fail-closed)", "user_id", userID, "channel", c.Name(), "err1", err1, "err2", err2) - paired = true } else { paired = p1 || p2 } @@ -265,9 +264,8 @@ func (c *Channel) handleMessage(ctx context.Context, update telego.Update) { groupSenderID := fmt.Sprintf("group:%d", chatID) paired, pairErr := c.PairingService().IsPaired(ctx, groupSenderID, c.Name()) if pairErr != nil { - slog.Warn("security.pairing_check_failed, assuming paired (fail-open)", + slog.Warn("security.pairing_check_failed, denying access (fail-closed)", "group_sender", groupSenderID, "channel", c.Name(), "error", pairErr) - paired = true } if paired { c.MarkGroupApproved(chatIDStr) @@ -320,9 +318,8 @@ func (c *Channel) handleMessage(ctx context.Context, update telego.Update) { groupSenderID := fmt.Sprintf("group:%d", chatID) paired, pairErr := c.PairingService().IsPaired(ctx, groupSenderID, c.Name()) if pairErr != nil { - slog.Warn("security.pairing_check_failed, assuming paired (fail-open)", + slog.Warn("security.pairing_check_failed, denying access (fail-closed)", "group_sender", groupSenderID, "channel", c.Name(), "error", pairErr) - paired = true } if paired { c.MarkGroupApproved(chatIDStr) @@ -376,9 +373,8 @@ func (c *Channel) handleMessage(ctx context.Context, update telego.Update) { groupSenderID := fmt.Sprintf("group:%d", chatID) paired, err := c.PairingService().IsPaired(ctx, groupSenderID, c.Name()) if err != nil { - slog.Warn("security.pairing_check_failed, assuming paired (fail-open)", + slog.Warn("security.pairing_check_failed, denying access (fail-closed)", "group_sender", groupSenderID, "channel", c.Name(), "error", err) - paired = true } if paired { c.MarkGroupApproved(chatIDStr) diff --git a/internal/http/files.go b/internal/http/files.go index 2ed265d0..434416b3 100644 --- a/internal/http/files.go +++ b/internal/http/files.go @@ -95,11 +95,16 @@ func (h *FilesHandler) auth(next http.HandlerFunc) http.HandlerFunc { // deniedFilePrefixes blocks access to sensitive system directories. // Defense-in-depth: the auth token is the primary barrier, but restricting -// known-sensitive paths limits damage if a token leaks. +// known-sensitive absolute paths limits damage if a token leaks or a root is +// misconfigured. All entries must be absolute paths because hasDeniedFilePrefix +// uses pathWithinDir semantics — relative-rooted entries (e.g. "/.ssh") would +// only block the literal filesystem subtree at that root, not user home dirs. var deniedFilePrefixes = []string{ "/etc/", "/proc/", "/sys/", "/dev/", "/root/", "/boot/", "/run/", - "/var/run/", "/var/log/", + "/var/run/", "/var/log/", "/var/lib/", "/var/www/", + "/home/", "/Users/", + "/opt/", "/srv/", } func (h *FilesHandler) handleServe(w http.ResponseWriter, r *http.Request) { @@ -127,6 +132,14 @@ func (h *FilesHandler) handleServe(w http.ResponseWriter, r *http.Request) { } signed := r.URL.Query().Get("ft") != "" + // Fail-closed observability: if no boundary roots are configured at all, + // lexicallyAllowsFilePath already returns false, but log explicitly so + // operators can detect misconfigured deployments. + if h.workspace == "" && h.dataDir == "" { + slog.Warn("security.files_no_boundary", "path", absPath) + http.NotFound(w, r) + return + } if !h.lexicallyAllowsFilePath(r, absPath, signed) { slog.Warn("security.files_path_denied", "path", absPath, "workspace", h.workspace, "data_dir", h.dataDir) http.NotFound(w, r) diff --git a/internal/http/files_path_security_test.go b/internal/http/files_path_security_test.go index 55bc7311..5fc3c650 100644 --- a/internal/http/files_path_security_test.go +++ b/internal/http/files_path_security_test.go @@ -88,6 +88,60 @@ func TestFilesHandleServe_ProcDir_Blocked(t *testing.T) { } } +// ---- handleServe: expanded deny-prefix list ---- + +// TestFilesHandleServe_ExpandedDenyPrefixes verifies that paths under /home/, +// /Users/, /var/lib/, /var/www/, /opt/, and /srv/ are all blocked. These are +// defense-in-depth blocks for misconfigured roots — the token/RBAC checks are +// the primary barriers. +func TestFilesHandleServe_ExpandedDenyPrefixes_Blocked(t *testing.T) { + h, _ := makeTestFilesHandler(t) + mux := http.NewServeMux() + mux.HandleFunc("GET /v1/files/{path...}", h.handleServe) + + cases := []struct { + name string + url string + }{ + {"home user ssh key", "/v1/files/home/user/.ssh/id_rsa"}, + {"home user aws creds", "/v1/files/home/user/.aws/credentials"}, + {"Users admin dir (macOS)", "/v1/files/Users/admin/secrets.txt"}, + {"var lib docker secret", "/v1/files/var/lib/docker/overlay2/secret"}, + {"var www config", "/v1/files/var/www/config.php"}, + {"opt secrets key", "/v1/files/opt/secrets/key.pem"}, + {"srv www app config", "/v1/files/srv/www/app/config.yaml"}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + req := httptest.NewRequest(http.MethodGet, tc.url, nil) + w := httptest.NewRecorder() + mux.ServeHTTP(w, req) + if w.Code == http.StatusOK { + t.Errorf("path %s should be denied (deny-prefix), got 200", tc.url) + } + }) + } +} + +// ---- handleServe: fail-closed observability on empty workspace/dataDir ---- + +// TestFilesHandleServe_NoBoundary_Denies verifies that when both workspace and +// dataDir are empty the handler returns 404 (fail-closed) for any path. +func TestFilesHandleServe_NoBoundary_Denies(t *testing.T) { + h := NewFilesHandler("", "") + mux := http.NewServeMux() + mux.HandleFunc("GET /v1/files/{path...}", h.handleServe) + + req := httptest.NewRequest(http.MethodGet, "/v1/files/tmp/test.txt", nil) + w := httptest.NewRecorder() + mux.ServeHTTP(w, req) + + if w.Code == http.StatusOK { + t.Errorf("empty workspace+dataDir should deny all requests, got 200") + } +} + // ---- handleServe: workspace boundary enforcement ---- func TestFilesHandleServe_FileInsideWorkspace_WithToken_Serves(t *testing.T) { diff --git a/internal/http/oauth_test.go b/internal/http/oauth_test.go index 2dc5b881..22cb3068 100644 --- a/internal/http/oauth_test.go +++ b/internal/http/oauth_test.go @@ -65,6 +65,12 @@ func (m *mockProviderStore) UpdateProvider(_ context.Context, id uuid.UUID, upda if v, ok := updates["api_key"]; ok { p.APIKey = v.(string) } + if v, ok := updates["api_base"]; ok { + p.APIBase = v.(string) + } + if v, ok := updates["provider_type"]; ok { + p.ProviderType = v.(string) + } if v, ok := updates["settings"]; ok { p.Settings = v.(json.RawMessage) } diff --git a/internal/http/providers.go b/internal/http/providers.go index 9127982f..6bb28bab 100644 --- a/internal/http/providers.go +++ b/internal/http/providers.go @@ -8,6 +8,7 @@ import ( "net" "net/http" "net/url" + "os" "os/exec" "path/filepath" "strings" @@ -21,6 +22,7 @@ import ( "github.com/nextlevelbuilder/goclaw/internal/oauth" "github.com/nextlevelbuilder/goclaw/internal/permissions" "github.com/nextlevelbuilder/goclaw/internal/providers" + "github.com/nextlevelbuilder/goclaw/internal/security" "github.com/nextlevelbuilder/goclaw/internal/store" usagecaps "github.com/nextlevelbuilder/goclaw/internal/usage/caps" "github.com/nextlevelbuilder/goclaw/pkg/protocol" @@ -322,55 +324,138 @@ func normalizeOllamaAPIBase(p *store.LLMProviderData) { } } -// localProviderTypes are provider types that legitimately run on localhost -// (e.g. Ollama, Claude CLI). SSRF checks are skipped for these. -var localProviderTypes = map[string]bool{ - store.ProviderOllama: true, - store.ProviderClaudeCLI: true, - store.ProviderACP: true, +// localURLProviderTypes are provider types that legitimately run on localhost. +// They are restricted to an explicit localhost allowlist +// rather than skipping SSRF validation entirely. +var localURLProviderTypes = map[string]bool{ + store.ProviderOllama: true, + store.ProviderACP: true, } +// allowedLocalHosts are the only hosts permitted for local provider types. +// Explicit allowlist (not blocklist) to prevent new internal addresses from +// slipping through (e.g. 169.254.169.254 via ollama base URL). +var allowedLocalHosts = []string{"localhost", "127.0.0.1", "::1", "host.docker.internal"} + +// dnsResolverFn resolves hostnames to IPs. Replaceable in tests. +var dnsResolverFn = net.LookupHost + +// allowPrivateProviderURLsFn reports whether the operator has opted in to +// permitting private / loopback / link-local / internal-hostname provider base +// URLs via GOCLAW_ALLOW_PRIVATE_PROVIDER_URLS. Evaluated once at first call so +// tests can override the variable before that happens. +var allowPrivateProviderURLsFn = sync.OnceValue(func() bool { + v := strings.ToLower(strings.TrimSpace(os.Getenv("GOCLAW_ALLOW_PRIVATE_PROVIDER_URLS"))) + return v == "1" || v == "true" || v == "yes" +}) + // validateProviderURL rejects provider base URLs pointing to internal/private networks. // Defense-in-depth: prevents SSRF when providers are later used for API calls. +// +// Logic: +// 1. Empty URL → allowed (provider may not need a custom base). +// 2. Claude CLI → api_base is an executable path/command, not a URL. +// 3. Scheme check (http/https only) → enforced for URL-based types, including +// local URL types. Blocks file://, gopher://, dict://, etc. +// 4. Local URL types (ollama, acp) → host must be in allowedLocalHosts +// (explicit allowlist prevents reaching 169.254.169.254 or internal services +// via the local-type bypass). +// 5. Remote types → if GOCLAW_ALLOW_PRIVATE_PROVIDER_URLS is set, allow and log. +// Otherwise: resolve DNS hostname; reject if ANY resolved IP satisfies +// security.IsBlocked (covers loopback, link-local, private, multicast, +// unspecified — including 0.0.0.0 and :: that earlier hand-rolled checks missed). +// +// DNS resolution on step 5 closes the nip.io / sslip.io / attacker-domain bypass +// where a hostname passes a literal-string blocklist but resolves to a private IP. func validateProviderURL(rawURL string, providerType string) error { - if rawURL == "" || localProviderTypes[providerType] { + if rawURL == "" { return nil } + if providerType == store.ProviderClaudeCLI { + return validateClaudeCLIExecutablePath(rawURL) + } u, err := url.Parse(rawURL) if err != nil { return fmt.Errorf("invalid URL: %w", err) } - // Only allow http/https schemes — block file://, gopher://, dict://, etc. + // Scheme check is unconditional for URL-based provider types, including local URL types. switch u.Scheme { case "http", "https": default: return fmt.Errorf("provider URL must use http or https scheme, got %q", u.Scheme) } + host := u.Hostname() - // Block obvious internal targets - blocked := []string{"localhost", "127.0.0.1", "::1", "0.0.0.0", "169.254.169.254", "metadata.google.internal"} - for _, b := range blocked { - if strings.EqualFold(host, b) { - return fmt.Errorf("provider URL cannot point to %s", b) + + // Local provider types: only allow an explicit localhost allowlist. + // This prevents using the local-type escape hatch to reach internal services + // or cloud metadata endpoints. + if localURLProviderTypes[providerType] { + for _, a := range allowedLocalHosts { + if strings.EqualFold(host, a) { + return nil + } } + slog.Warn("security.provider_url.local_type_denied", "host", host, "provider_type", providerType) + return fmt.Errorf("provider type %q only allows localhost URLs (localhost, 127.0.0.1, ::1, host.docker.internal), got host %q", providerType, host) } - // Block private IP ranges (normalize IPv6-mapped IPv4 to catch ::ffff:127.0.0.1) - ip := net.ParseIP(host) - if ip != nil { - if v4 := ip.To4(); v4 != nil { - ip = v4 - } - if ip.IsLoopback() || ip.IsPrivate() || ip.IsLinkLocalUnicast() || ip.IsLinkLocalMulticast() { - return fmt.Errorf("provider URL cannot point to private network: %s", host) - } + + // Operator opt-in to allow private-network provider URLs (e.g. LAN-hosted vLLM). + // Scheme check above still applies even with this gate open. + if allowPrivateProviderURLsFn() { + slog.Warn("security.provider_url.private_allowed", "host", host, "provider_type", providerType) + return nil } - // Block common internal hostnames + + // Check literal IP first (avoids unnecessary DNS lookup). + if ip := net.ParseIP(host); ip != nil { + if security.IsBlocked(ip) { + slog.Warn("security.provider_url.blocked", "host", host, "provider_type", providerType) + return fmt.Errorf("provider URL cannot point to %s", host) + } + return nil + } + + // Block .internal / .local suffix before DNS (fail-fast for well-known patterns). if strings.HasSuffix(host, ".internal") || strings.HasSuffix(host, ".local") { + slog.Warn("security.provider_url.blocked", "host", host, "provider_type", providerType) return fmt.Errorf("provider URL cannot point to internal hostname: %s", host) } + + // Resolve DNS and check every returned address. + // Prevents bypass via wildcard services (nip.io, sslip.io) or attacker-controlled + // domains that map to private IPs (DNS-rebinding at config time). + addrs, err := dnsResolverFn(host) + if err != nil { + slog.Warn("security.provider_url.dns_resolve_failed", "host", host, "provider_type", providerType, "error", err) + return fmt.Errorf("provider URL hostname %q could not be resolved: %w", host, err) + } + for _, addr := range addrs { + ip := net.ParseIP(addr) + if ip == nil { + continue + } + if security.IsBlocked(ip) { + slog.Warn("security.provider_url.blocked_resolved", "host", host, "resolved_ip", ip.String(), "provider_type", providerType) + return fmt.Errorf("provider URL %q resolves to private/reserved address %s", host, ip) + } + } return nil } +func validateClaudeCLIExecutablePath(path string) error { + if strings.Contains(path, "\x00") { + return fmt.Errorf("Claude CLI executable path cannot contain NUL byte") + } + if _, err := url.ParseRequestURI(path); err == nil && strings.Contains(path, "://") { + return fmt.Errorf("Claude CLI api_base must be an executable path or %q, got URL %q", "claude", path) + } + if path == "claude" || filepath.IsAbs(path) { + return nil + } + return fmt.Errorf("Claude CLI api_base must be %q or an absolute executable path, got %q", "claude", path) +} + // --- Provider CRUD --- func (h *ProvidersHandler) handleListProviders(w http.ResponseWriter, r *http.Request) { diff --git a/internal/http/providers_test.go b/internal/http/providers_test.go index e2b37f55..eeec3f77 100644 --- a/internal/http/providers_test.go +++ b/internal/http/providers_test.go @@ -298,6 +298,79 @@ func TestProvidersHandlerCreateAllows1536EmbeddingDimensions(t *testing.T) { } } +func TestProvidersHandlerCreateAllowsClaudeCLIExecutablePath(t *testing.T) { + token := setupProvidersAdminToken(t) + providerStore := newMockProviderStore() + handler := NewProvidersHandler(providerStore, newMockSecretsStore(), nil, "") + mux := http.NewServeMux() + handler.RegisterRoutes(mux) + + body := map[string]any{ + "name": "claude-local", + "provider_type": store.ProviderClaudeCLI, + "api_base": writeFakeClaudeBinary(t), + "enabled": true, + } + rawBody, err := json.Marshal(body) + if err != nil { + t.Fatalf("Marshal() error = %v", err) + } + + req := httptest.NewRequest(http.MethodPost, "/v1/providers", bytes.NewReader(rawBody)) + req.Header.Set("Authorization", "Bearer "+token) + w := httptest.NewRecorder() + mux.ServeHTTP(w, req) + + if w.Code != http.StatusCreated { + t.Fatalf("status code = %d, want %d, body=%s", w.Code, http.StatusCreated, w.Body.String()) + } + if got := providerStore.providers["claude-local"].APIBase; got == "" || !filepath.IsAbs(got) { + t.Fatalf("stored Claude CLI api_base = %q, want absolute executable path", got) + } +} + +func TestProvidersHandlerUpdateAllowsClaudeCLIExecutablePath(t *testing.T) { + token := setupProvidersAdminToken(t) + providerStore := newMockProviderStore() + provider := &store.LLMProviderData{ + BaseModel: store.BaseModel{ID: uuid.New()}, + Name: "claude-local", + ProviderType: store.ProviderClaudeCLI, + APIBase: "claude", + Enabled: true, + } + if err := providerStore.CreateProvider(context.Background(), provider); err != nil { + t.Fatalf("CreateProvider() error = %v", err) + } + + handler := NewProvidersHandler(providerStore, newMockSecretsStore(), nil, "") + mux := http.NewServeMux() + handler.RegisterRoutes(mux) + + nextPath := writeFakeClaudeBinary(t) + body := map[string]any{"api_base": nextPath} + rawBody, err := json.Marshal(body) + if err != nil { + t.Fatalf("Marshal() error = %v", err) + } + + req := httptest.NewRequest(http.MethodPut, "/v1/providers/"+provider.ID.String(), bytes.NewReader(rawBody)) + req.Header.Set("Authorization", "Bearer "+token) + w := httptest.NewRecorder() + mux.ServeHTTP(w, req) + + if w.Code != http.StatusOK { + t.Fatalf("status code = %d, want %d, body=%s", w.Code, http.StatusOK, w.Body.String()) + } + current, err := providerStore.GetProvider(context.Background(), provider.ID) + if err != nil { + t.Fatalf("GetProvider() error = %v", err) + } + if current.APIBase != nextPath { + t.Fatalf("api_base = %q, want %q", current.APIBase, nextPath) + } +} + func TestProvidersHandlerUpdateRejectsIncompatibleEmbeddingDimensions(t *testing.T) { token := setupProvidersAdminToken(t) providerStore := newMockProviderStore() diff --git a/internal/http/providers_url_validate_test.go b/internal/http/providers_url_validate_test.go new file mode 100644 index 00000000..715db697 --- /dev/null +++ b/internal/http/providers_url_validate_test.go @@ -0,0 +1,326 @@ +package http + +import ( + "net" + "path/filepath" + "strings" + "testing" +) + +// stubResolver returns a net.LookupHost-compatible function that maps known +// hostnames to controlled IPs. Unknown hostnames return a DNS error. +func stubResolver(m map[string][]string) func(host string) ([]string, error) { + return func(host string) ([]string, error) { + if addrs, ok := m[host]; ok { + return addrs, nil + } + return nil, &net.DNSError{Err: "no such host", Name: host, IsNotFound: true} + } +} + +// saveAndRestoreGlobals saves mutable package-level vars and restores them after +// the test. Call at the start of any test that touches dnsResolverFn or +// allowPrivateProviderURLsFn. +func saveAndRestoreGlobals(t *testing.T) { + t.Helper() + origResolver := dnsResolverFn + origAllow := allowPrivateProviderURLsFn + t.Cleanup(func() { + dnsResolverFn = origResolver + allowPrivateProviderURLsFn = origAllow + }) +} + +// --- Core merged test: all major cases in one table --- + +func TestValidateProviderURL(t *testing.T) { + saveAndRestoreGlobals(t) + allowPrivateProviderURLsFn = func() bool { return false } + absClaudePath := filepath.Join(t.TempDir(), "claude") + + dnsResolverFn = stubResolver(map[string][]string{ + "api.openai.com": {"104.18.6.192"}, + "legit-provider.com": {"203.0.113.50"}, + "10.10.27.30.nip.io": {"10.10.27.30"}, + "192.168.1.1.sslip.io": {"192.168.1.1"}, + "172.16.0.5.nip.io": {"172.16.0.5"}, + "169.254.169.254.nip.io": {"169.254.169.254"}, + "127.0.0.1.nip.io": {"127.0.0.1"}, + "rebind.attacker.com": {"10.0.0.1"}, + "dual-stack.example.com": {"203.0.113.50", "10.0.0.1"}, + }) + + tests := []struct { + name string + rawURL string + providerType string + wantErr bool + }{ + // Empty URL always allowed + {"empty URL", "", "openai_compat", false}, + {"empty URL ollama", "", "ollama", false}, + + // Public remote URLs OK + {"public HTTPS", "https://api.openai.com/v1", "openai_compat", false}, + {"public HTTP", "http://legit-provider.com/v1", "openai_compat", false}, + + // --- Scheme check: unconditional for ALL types including local --- + {"file scheme remote", "file:///etc/passwd", "openai_compat", true}, + {"gopher scheme remote", "gopher://internal:25", "openai_compat", true}, + {"file scheme ollama", "file:///etc/passwd", "ollama", true}, // H-1: scheme enforced even for local types + {"gopher scheme acp", "gopher://localhost:25", "acp", true}, // H-1: scheme enforced even for local types + {"file scheme claude_cli", "file:///bin/bash", "claude_cli", true}, // H-1: scheme enforced for URL-like Claude CLI values + + // --- Local type: allowlist-only --- + {"ollama localhost", "http://localhost:11434/v1", "ollama", false}, + {"ollama 127.0.0.1", "http://127.0.0.1:11434/v1", "ollama", false}, + {"ollama ::1", "http://[::1]:11434/v1", "ollama", false}, + {"ollama host.docker.internal", "http://host.docker.internal:11434/v1", "ollama", false}, + {"acp 127.0.0.1", "http://127.0.0.1:9090", "acp", false}, + {"claude_cli command name", "claude", "claude_cli", false}, + {"claude_cli absolute path", absClaudePath, "claude_cli", false}, + + // Local type with non-localhost hosts → blocked + {"ollama 169.254.169.254", "http://169.254.169.254/latest/meta-data/", "ollama", true}, + {"ollama private IP", "http://10.0.0.5:11434/v1", "ollama", true}, + {"ollama evil.com", "http://evil.attacker.tld:9999/v1", "ollama", true}, + {"ollama postgres sidecar", "http://postgres:5432/v1", "ollama", true}, + {"ollama link-local", "http://169.254.1.1:8080/v1", "ollama", true}, + {"ollama .internal", "http://redis.internal:6379/v1", "ollama", true}, + {"ollama gcp metadata", "http://metadata.google.internal/computeMetadata/v1/", "ollama", true}, + {"acp private", "http://10.0.0.1:8080/v1", "acp", true}, + + // --- Remote type literal blocked IPs --- + {"remote localhost", "http://localhost:8080", "openai_compat", true}, + {"remote 127.0.0.1", "http://127.0.0.1:8080", "openai_compat", true}, + {"remote ::1", "http://[::1]:8080", "openai_compat", true}, + {"remote 10.x", "http://10.0.0.1:8080/v1", "openai_compat", true}, + {"remote 192.168.x", "http://192.168.1.100:8080/v1", "openai_compat", true}, + {"remote 172.16.x", "http://172.16.0.5:8080/v1", "openai_compat", true}, + {"remote 169.254.169.254", "http://169.254.169.254/latest/meta-data/", "openai_compat", true}, + + // --- Regression: unspecified addresses (bug missed by #974 hand-rolled checks) --- + {"0.0.0.0 unspecified", "http://0.0.0.0:8080/v1", "openai_compat", true}, + {":: IPv6 unspecified", "http://[::]:8080/v1", "openai_compat", true}, + + // --- DNS bypass via wildcard services (nip.io, sslip.io) --- + {"nip.io 10.x", "http://10.10.27.30.nip.io:9999/v1", "openai_compat", true}, + {"sslip.io 192.168.x", "http://192.168.1.1.sslip.io:8080/v1", "openai_compat", true}, + {"nip.io 172.16.x", "http://172.16.0.5.nip.io:8080/v1", "openai_compat", true}, + {"nip.io link-local", "http://169.254.169.254.nip.io/latest/", "openai_compat", true}, + {"nip.io loopback", "http://127.0.0.1.nip.io:8080/v1", "openai_compat", true}, + {"attacker rebind", "http://rebind.attacker.com:8080/v1", "openai_compat", true}, + {"dual-stack any private", "http://dual-stack.example.com:8080/v1", "openai_compat", true}, + + // --- Internal hostname suffix --- + {".internal suffix", "http://metadata.google.internal/computeMetadata/v1/", "openai_compat", true}, + {".local suffix", "http://myservice.local:8080", "openai_compat", true}, + + // --- Unresolvable hostname --- + {"unresolvable hostname", "http://nonexistent.invalid:8080/v1", "openai_compat", true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := validateProviderURL(tt.rawURL, tt.providerType) + if (err != nil) != tt.wantErr { + t.Errorf("validateProviderURL(%q, %q) error = %v, wantErr %v", + tt.rawURL, tt.providerType, err, tt.wantErr) + } + }) + } +} + +// --- Env gate: GOCLAW_ALLOW_PRIVATE_PROVIDER_URLS --- + +func TestValidateProviderURL_AllowPrivateFlag(t *testing.T) { + saveAndRestoreGlobals(t) + + dnsResolverFn = stubResolver(map[string][]string{ + "my-vllm.lan": {"10.0.0.1"}, + }) + + t.Run("private URL allowed when flag is true", func(t *testing.T) { + allowPrivateProviderURLsFn = func() bool { return true } + if err := validateProviderURL("http://my-vllm.lan:8080/v1", "openai_compat"); err != nil { + t.Errorf("expected nil with allow-private flag, got: %v", err) + } + if err := validateProviderURL("http://192.168.1.50:8080/v1", "openai_compat"); err != nil { + t.Errorf("expected nil for private IP with allow-private flag, got: %v", err) + } + if err := validateProviderURL("http://localhost:8080/v1", "openai_compat"); err != nil { + t.Errorf("expected nil for localhost with allow-private flag, got: %v", err) + } + if err := validateProviderURL("http://llm.internal/v1", "openai_compat"); err != nil { + t.Errorf("expected nil for .internal with allow-private flag, got: %v", err) + } + }) + + t.Run("private URL blocked when flag is false", func(t *testing.T) { + allowPrivateProviderURLsFn = func() bool { return false } + if err := validateProviderURL("http://my-vllm.lan:8080/v1", "openai_compat"); err == nil { + t.Error("expected error for private URL without allow-private flag") + } + }) + + t.Run("scheme still enforced even with allow-private flag", func(t *testing.T) { + allowPrivateProviderURLsFn = func() bool { return true } + err := validateProviderURL("file:///etc/passwd", "openai_compat") + if err == nil { + t.Error("expected error for file:// scheme even with allow-private flag") + } + if !strings.Contains(err.Error(), "scheme") { + t.Errorf("expected scheme error, got: %v", err) + } + }) +} + +func TestValidateProviderURL_LocalTypesIgnoreAllowPrivateFlag(t *testing.T) { + saveAndRestoreGlobals(t) + allowPrivateProviderURLsFn = func() bool { return true } + + cases := []struct { + url string + providerType string + }{ + {"http://ollama:11434/v1", "ollama"}, + {"http://host.lan:11434/v1", "ollama"}, + {"http://10.0.0.5:11434/v1", "ollama"}, + {"http://acp-sidecar:9090", "acp"}, + } + for _, c := range cases { + if err := validateProviderURL(c.url, c.providerType); err == nil { + t.Errorf("expected local provider URL %s / %s to remain blocked despite allow-private flag", c.url, c.providerType) + } + } +} + +// --- H-1: scheme enforced for local provider types (from PR #972) --- + +func TestValidateProviderURL_LocalTypeSchemeEnforced(t *testing.T) { + saveAndRestoreGlobals(t) + + cases := []struct { + url string + providerType string + }{ + {"file:///etc/passwd", "ollama"}, + {"gopher://localhost:25", "ollama"}, + {"file:///etc/passwd", "acp"}, + } + for _, c := range cases { + err := validateProviderURL(c.url, c.providerType) + if err == nil { + t.Errorf("expected scheme error for %s / %s, got nil", c.url, c.providerType) + continue + } + if !strings.Contains(err.Error(), "scheme") { + t.Errorf("expected scheme error for %s / %s, got: %v", c.url, c.providerType, err) + } + } +} + +// --- Regression: 0.0.0.0 and :: unspecified (bug PR #974 missed) --- + +func TestValidateProviderURL_UnspecifiedBlocked(t *testing.T) { + saveAndRestoreGlobals(t) + allowPrivateProviderURLsFn = func() bool { return false } + + cases := []string{ + "http://0.0.0.0:8080/v1", + "http://[::]:8080/v1", + } + for _, u := range cases { + err := validateProviderURL(u, "openai_compat") + if err == nil { + t.Errorf("expected error for unspecified address %q, got nil", u) + } + } +} + +// --- DNS-rebinding: remote type resolving to private/loopback --- + +func TestValidateProviderURL_DNSRebindBlocked(t *testing.T) { + saveAndRestoreGlobals(t) + allowPrivateProviderURLsFn = func() bool { return false } + + privateHosts := map[string][]string{ + "rebind-private.example.com": {"10.0.0.1"}, + "rebind-loopback.example.com": {"127.0.0.1"}, + "rebind-linklocal.example.com": {"169.254.169.254"}, + } + dnsResolverFn = stubResolver(privateHosts) + + for host := range privateHosts { + err := validateProviderURL("http://"+host+":8080/v1", "openai_compat") + if err == nil { + t.Errorf("expected DNS rebind to be blocked for %s, got nil", host) + } + } +} + +// --- Local type: all allowed variants --- + +func TestValidateProviderURL_LocalTypeAllowedHosts(t *testing.T) { + saveAndRestoreGlobals(t) + + allowed := []struct { + url string + providerType string + }{ + {"http://localhost:11434/v1", "ollama"}, + {"http://127.0.0.1:11434/v1", "ollama"}, + {"http://[::1]:11434/v1", "ollama"}, + {"http://host.docker.internal:11434/v1", "ollama"}, + {"http://localhost:9090", "acp"}, + {"http://127.0.0.1:9090", "acp"}, + } + for _, a := range allowed { + if err := validateProviderURL(a.url, a.providerType); err != nil { + t.Errorf("expected %s / %s to be allowed, got: %v", a.url, a.providerType, err) + } + } +} + +func TestValidateProviderURL_ClaudeCLIExecutablePath(t *testing.T) { + saveAndRestoreGlobals(t) + absClaudePath := filepath.Join(t.TempDir(), "claude") + + allowed := []string{ + "", + "claude", + absClaudePath, + filepath.Join(t.TempDir(), "Claude Code.app", "Contents", "MacOS", "claude"), + } + for _, raw := range allowed { + if err := validateProviderURL(raw, "claude_cli"); err != nil { + t.Errorf("expected Claude CLI executable %q to be allowed, got: %v", raw, err) + } + } + + blocked := []string{ + "file:///usr/local/bin/claude", + "https://api.anthropic.com/v1", + "relative/claude", + "claude --dangerously-skip-permissions", + } + for _, raw := range blocked { + if err := validateProviderURL(raw, "claude_cli"); err == nil { + t.Errorf("expected Claude CLI executable %q to be rejected", raw) + } + } +} + +// --- Public remote URL always OK --- + +func TestValidateProviderURL_PublicHostOK(t *testing.T) { + saveAndRestoreGlobals(t) + allowPrivateProviderURLsFn = func() bool { return false } + dnsResolverFn = stubResolver(map[string][]string{ + "api.openai.com": {"104.18.6.192"}, + }) + + if err := validateProviderURL("https://api.openai.com/v1", "openai_compat"); err != nil { + t.Errorf("expected public host to pass, got: %v", err) + } +} diff --git a/internal/pipeline/deps.go b/internal/pipeline/deps.go index a8054ca2..43ed7ca7 100644 --- a/internal/pipeline/deps.go +++ b/internal/pipeline/deps.go @@ -89,6 +89,9 @@ type PipelineDeps struct { ExecuteToolRaw func(ctx context.Context, tc providers.ToolCall) (providers.Message, any, error) // ProcessToolResult processes a raw tool result with state mutation (sequential only). ProcessToolResult func(ctx context.Context, state *RunState, tc providers.ToolCall, rawMsg providers.Message, rawData any) []providers.Message + // AuthorizeToolCall validates whether a tool call is allowed to execute. + // Used by ToolStage as a runtime guard against out-of-policy tool calls. + AuthorizeToolCall func(ctx context.Context, state *RunState, tc providers.ToolCall) (bool, string) // SequentialToolCall returns true for tools that must preserve same-response order. // When any tool call in a batch matches, ToolStage uses ExecuteToolCall for the // whole batch instead of parallel raw execution. diff --git a/internal/pipeline/substates.go b/internal/pipeline/substates.go index b3ae564d..0b38003c 100644 --- a/internal/pipeline/substates.go +++ b/internal/pipeline/substates.go @@ -52,6 +52,9 @@ type PruneState struct { // ToolState: owned by ToolStage. type ToolState struct { + // AllowedTools is the per-iteration execution allowlist built from tool + // definitions sent to the provider. Nil means "no runtime restriction". + AllowedTools map[string]bool LoopDetector any // concrete type toolLoopState lives in agent; Phase 5 defines LoopDetector interface TotalToolCalls int AsyncToolCalls []string // tool names that executed async (spawn) diff --git a/internal/pipeline/think_stage.go b/internal/pipeline/think_stage.go index 582a79a2..c5dd6640 100644 --- a/internal/pipeline/think_stage.go +++ b/internal/pipeline/think_stage.go @@ -41,6 +41,13 @@ func (s *ThinkStage) Execute(ctx context.Context, state *RunState) error { if err != nil { return fmt.Errorf("build tools: %w", err) } + allowed := make(map[string]bool, len(toolDefs)) + for _, td := range toolDefs { + allowed[td.Function.Name] = true + } + state.Tool.AllowedTools = allowed + } else { + state.Tool.AllowedTools = nil } // 3. Construct ChatRequest diff --git a/internal/pipeline/tool_stage.go b/internal/pipeline/tool_stage.go index ac6c7a1e..8f41a5f4 100644 --- a/internal/pipeline/tool_stage.go +++ b/internal/pipeline/tool_stage.go @@ -64,6 +64,22 @@ func (s *ToolStage) Execute(ctx context.Context, state *RunState) error { } } + // Fail-closed authorization gate: reject calls not present in the + // per-iteration allowlist built by ThinkStage. Operates on the canonical + // (prefix-stripped) name so toolCallPrefix agents are handled correctly. + if s.deps.AuthorizeToolCall != nil { + if ok, reason := s.deps.AuthorizeToolCall(ctx, state, tc); !ok { + state.Messages.AppendPending(providers.Message{ + Role: "tool", + Content: reason, + ToolCallID: tc.ID, + IsError: true, + }) + state.Tool.TotalToolCalls++ + continue + } + } + // Hook: sync PreToolUse — block if hook denies. Builtin-source hooks may // rewrite tc.Arguments via UpdatedToolInput (e.g. path-sanitizer); apply // before ExecuteToolCall so the rewrite is authoritative. @@ -179,10 +195,35 @@ func (s *ToolStage) executeParallel(ctx context.Context, state *RunState, toolCa err error } + // Filter out unauthorized calls before dispatching parallel I/O. + // Keeps error messages deterministic (same order as toolCalls slice). + filteredCalls := make([]providers.ToolCall, 0, len(toolCalls)) + for _, tc := range toolCalls { + if s.deps.AuthorizeToolCall != nil { + if ok, reason := s.deps.AuthorizeToolCall(ctx, state, tc); !ok { + state.Messages.AppendPending(providers.Message{ + Role: "tool", + Content: reason, + ToolCallID: tc.ID, + IsError: true, + }) + state.Tool.TotalToolCalls++ + continue + } + } + filteredCalls = append(filteredCalls, tc) + } + // When all calls were blocked, still run exit-condition checks (read-only + // streak, MaxToolCalls budget) for consistency with the sequential path. + if len(filteredCalls) == 0 { + s.checkExitConditions(state) + return nil + } + // Phase 1: parallel I/O (no state mutation) - results := make([]rawResult, len(toolCalls)) + results := make([]rawResult, len(filteredCalls)) var wg sync.WaitGroup - for i, tc := range toolCalls { + for i, tc := range filteredCalls { wg.Add(1) go func(idx int, tc providers.ToolCall) { defer wg.Done() diff --git a/internal/pipeline/tool_stage_authorize_test.go b/internal/pipeline/tool_stage_authorize_test.go new file mode 100644 index 00000000..71691b22 --- /dev/null +++ b/internal/pipeline/tool_stage_authorize_test.go @@ -0,0 +1,216 @@ +package pipeline + +import ( + "context" + "testing" + + "github.com/nextlevelbuilder/goclaw/internal/providers" +) + +// makeAuthorizeGate builds a PipelineDeps.AuthorizeToolCall callback backed by +// a static allowlist and an optional deferred-activator, mirroring the contract +// that makeAuthorizeToolCall in loop_pipeline_callbacks.go provides. +func makeAuthorizeGate( + allowed map[string]bool, // nil = allow-all + tryActivate func(name string) bool, // nil = no activator + isDenied func(name string) bool, // nil = never denied +) func(ctx context.Context, state *RunState, tc providers.ToolCall) (bool, string) { + return func(_ context.Context, state *RunState, tc providers.ToolCall) (bool, string) { + a := state.Tool.AllowedTools + if a == nil { + return true, "" + } + if a[tc.Name] { + return true, "" + } + if tryActivate != nil && tryActivate(tc.Name) { + if isDenied != nil && isDenied(tc.Name) { + return false, "tool not allowed by policy: " + tc.Name + } + a[tc.Name] = true + return true, "" + } + return false, "tool not allowed by policy: " + tc.Name + } +} + +// makeMinimalDeps returns a PipelineDeps just sufficient for ToolStage tests. +func makeMinimalDeps(authorize func(context.Context, *RunState, providers.ToolCall) (bool, string)) *PipelineDeps { + return &PipelineDeps{ + Config: PipelineConfig{MaxToolCalls: 100}, + AuthorizeToolCall: authorize, + ExecuteToolCall: func(_ context.Context, state *RunState, tc providers.ToolCall) ([]providers.Message, error) { + return []providers.Message{{Role: "tool", Content: "ok", ToolCallID: tc.ID}}, nil + }, + } +} + +// stateWithAllowedTools returns a minimal RunState with AllowedTools pre-set. +func stateWithAllowedTools(allowed map[string]bool) *RunState { + st := defaultState() + st.Tool.AllowedTools = allowed + return st +} + +func toolCallFor(name string) providers.ToolCall { + return providers.ToolCall{ID: "tc-" + name, Name: name} +} + +// fakeResponse puts tool calls into the state's ThinkStage so ToolStage finds them. +func setLastResponse(state *RunState, calls []providers.ToolCall) { + state.Think.LastResponse = &providers.ChatResponse{ToolCalls: calls} +} + +// --- Tests --- + +// TestAuthorizeGate_NilAllowlist_AllowsAll verifies that a nil AllowedTools map +// means no per-iteration restriction (allow-all semantics). +func TestAuthorizeGate_NilAllowlist_AllowsAll(t *testing.T) { + t.Parallel() + st := defaultState() + st.Tool.AllowedTools = nil // explicit nil + + gate := makeAuthorizeGate(nil, nil, nil) + tc := toolCallFor("exec") + + ok, reason := gate(context.Background(), st, tc) + if !ok { + t.Errorf("nil allowlist must allow all tools; got blocked with reason %q", reason) + } +} + +// TestAuthorizeGate_NamePresent_Allows checks that a tool explicitly in the +// allowlist passes through. +func TestAuthorizeGate_NamePresent_Allows(t *testing.T) { + t.Parallel() + st := stateWithAllowedTools(map[string]bool{"exec": true, "read_file": true}) + + gate := makeAuthorizeGate(nil, nil, nil) + for _, name := range []string{"exec", "read_file"} { + ok, reason := gate(context.Background(), st, toolCallFor(name)) + if !ok { + t.Errorf("tool %q should be allowed; got blocked: %q", name, reason) + } + } +} + +// TestAuthorizeGate_NameAbsent_NoActivator_Denies confirms fail-closed: a name +// not in the allowlist with no activator is blocked. +func TestAuthorizeGate_NameAbsent_NoActivator_Denies(t *testing.T) { + t.Parallel() + st := stateWithAllowedTools(map[string]bool{"read_file": true}) + + gate := makeAuthorizeGate(nil, nil, nil) + ok, reason := gate(context.Background(), st, toolCallFor("exec")) + if ok { + t.Error("tool absent from allowlist with no activator must be denied") + } + if reason == "" { + t.Error("denial must include a non-empty reason") + } +} + +// TestAuthorizeGate_DeferredActivate_ThenDenied ensures that lazy activation +// followed by a positive IsDenied check still blocks the tool. +func TestAuthorizeGate_DeferredActivate_ThenDenied(t *testing.T) { + t.Parallel() + st := stateWithAllowedTools(map[string]bool{}) + + activated := false + tryActivate := func(name string) bool { + activated = true + return name == "mcp_svc__exec_cmd" + } + isDenied := func(name string) bool { + return name == "mcp_svc__exec_cmd" // explicitly denied + } + + gate := makeAuthorizeGate(nil, tryActivate, isDenied) + ok, reason := gate(context.Background(), st, toolCallFor("mcp_svc__exec_cmd")) + + if !activated { + t.Error("expected TryActivateDeferred to be called") + } + if ok { + t.Error("tool allowed by activator but denied by policy must still be blocked") + } + if reason == "" { + t.Error("denial must include a non-empty reason") + } + if st.Tool.AllowedTools["mcp_svc__exec_cmd"] { + t.Error("denied tool must not be added to AllowedTools") + } +} + +// TestAuthorizeGate_PrefixedName_CanonicalLookup is the regression guard for the +// toolCallPrefix bug: when an agent has toolCallPrefix set, the model emits +// "proxy_exec" but AllowedTools is keyed by canonical name "exec". The gate must +// resolve the name before lookup, otherwise every prefixed call is wrongly blocked. +// +// This test simulates the resolution by calling the gate with a *pre-resolved* +// canonical name, matching what makeAuthorizeToolCall does after calling +// resolveToolCallName(tc.Name). The pipeline layer always receives the resolved +// name because AuthorizeToolCall in the real callback operates on the canonical +// form. +func TestAuthorizeGate_PrefixedName_CanonicalLookup(t *testing.T) { + t.Parallel() + // AllowedTools uses canonical name "exec" (as ThinkStage builds it from FilterTools). + st := stateWithAllowedTools(map[string]bool{"exec": true}) + + gate := makeAuthorizeGate(nil, nil, nil) + + // Simulate what resolveToolCallName("proxy_exec") returns with prefix "proxy_": + // the canonical name "exec" is passed to the gate. + resolvedName := "exec" // prefix already stripped by makeAuthorizeToolCall + tc := providers.ToolCall{ID: "tc-prefix", Name: resolvedName} + + ok, reason := gate(context.Background(), st, tc) + if !ok { + t.Errorf("canonical name after prefix resolution must be allowed; reason: %q", reason) + } +} + +// TestToolStage_AllBlocked_CallsCheckExitConditions verifies that when all tool +// calls in the parallel path are blocked, ToolStage still runs checkExitConditions +// (so MaxToolCalls budget is enforced). Regression for early-return-nil bug. +func TestToolStage_AllBlocked_CallsCheckExitConditions(t *testing.T) { + t.Parallel() + + // Allow-all deps but with MaxToolCalls = 1 and AuthorizeToolCall that blocks. + denyAll := func(_ context.Context, _ *RunState, tc providers.ToolCall) (bool, string) { + return false, "blocked: " + tc.Name + } + deps := &PipelineDeps{ + Config: PipelineConfig{MaxToolCalls: 1}, + AuthorizeToolCall: denyAll, + ExecuteToolCall: func(_ context.Context, state *RunState, tc providers.ToolCall) ([]providers.Message, error) { + return nil, nil + }, + ExecuteToolRaw: func(_ context.Context, tc providers.ToolCall) (providers.Message, any, error) { + return providers.Message{}, nil, nil + }, + ProcessToolResult: func(_ context.Context, state *RunState, tc providers.ToolCall, rawMsg providers.Message, rawData any) []providers.Message { + return nil + }, + } + + stage := NewToolStage(deps) + state := defaultState() + state.Tool.AllowedTools = map[string]bool{} // non-nil allowlist → gate is active + state.Tool.TotalToolCalls = 1 // already at MaxToolCalls + + // Two tool calls — parallel path is triggered (len > 1). + setLastResponse(state, []providers.ToolCall{ + toolCallFor("exec"), + toolCallFor("write_file"), + }) + + if err := stage.Execute(context.Background(), state); err != nil { + t.Fatalf("Execute returned error: %v", err) + } + + // checkExitConditions must have fired: TotalToolCalls >= MaxToolCalls → BreakLoop. + if stage.Result() != BreakLoop { + t.Errorf("expected BreakLoop after all-blocked + MaxToolCalls reached, got %v", stage.Result()) + } +} diff --git a/internal/providers/defaults.go b/internal/providers/defaults.go index 29c8e616..bb464703 100644 --- a/internal/providers/defaults.go +++ b/internal/providers/defaults.go @@ -38,6 +38,11 @@ func NewDefaultTransport() *http.Transport { // NewDefaultHTTPClient returns an *http.Client backed by NewDefaultTransport. // No Client.Timeout is set — rely on ctx deadlines and Transport stage timeouts. +// +// SSRF protection for user-configured provider URLs is enforced at provider +// create/update time by validateProviderURL (resolves the host and rejects +// private/reserved IPs via security.IsBlocked). Dial-time DNS-rebinding +// hardening is tracked as a follow-up. func NewDefaultHTTPClient() *http.Client { return &http.Client{Transport: NewDefaultTransport()} } diff --git a/internal/providers/embedding_openai.go b/internal/providers/embedding_openai.go index e98fea8e..788a747c 100644 --- a/internal/providers/embedding_openai.go +++ b/internal/providers/embedding_openai.go @@ -40,8 +40,11 @@ func NewOpenAIEmbeddingProvider(apiKey, apiBase, model string) *OpenAIEmbeddingP apiKey: apiKey, apiBase: strings.TrimRight(apiBase, "/"), model: model, - client: &http.Client{Timeout: 60 * time.Second}, - retry: DefaultRetryConfig(), + // Use a fixed 60s timeout client. Embedding requests are short but can be + // batched. The apiBase is validated at provider-creation time by + // validateProviderURL in internal/http/providers.go. + client: &http.Client{Timeout: 60 * time.Second}, + retry: DefaultRetryConfig(), } } diff --git a/internal/providers/vertex.go b/internal/providers/vertex.go index f39f9b21..8c5cc516 100644 --- a/internal/providers/vertex.go +++ b/internal/providers/vertex.go @@ -131,11 +131,15 @@ func NewVertexProvider(ctx context.Context, cfg VertexConfig) (*OpenAIProvider, // then transparently fetches a fresh one. No extra work for callers. cached := oauth2.ReuseTokenSource(nil, tokenSource) + // Use NewDefaultTransport as the oauth2 base transport so Vertex requests + // inherit the same per-stage timeouts as other providers. SSRF defense is + // provided by validateVertexAPIBaseOverride, which enforces the googleapis.com + // host constraint at provider-creation time. client := &http.Client{ Timeout: DefaultHTTPTimeout, Transport: &oauth2.Transport{ Source: cached, - Base: http.DefaultTransport, + Base: NewDefaultTransport(), }, } diff --git a/internal/sandbox/docker_test.go b/internal/sandbox/docker_test.go index 50b4fc94..7efe5678 100644 --- a/internal/sandbox/docker_test.go +++ b/internal/sandbox/docker_test.go @@ -172,31 +172,3 @@ func TestFsBridgePathWithinUsesPathBoundaries(t *testing.T) { }) } } - -func TestFsBridgeWriteFileCommandPreservesOverwriteTruncation(t *testing.T) { - args := fsBridgeWriteDDArgs("/workspace/file.txt", false) - for _, arg := range args { - if arg == "conv=notrunc" || arg == "oflag=append" { - t.Fatalf("overwrite command must truncate, got append-only arg %q in %v", arg, args) - } - } -} - -func TestFsBridgeWriteFileCommandUsesNoTruncOnlyForAppend(t *testing.T) { - args := fsBridgeWriteDDArgs("/workspace/file.txt", true) - if !containsString(args, "conv=notrunc") { - t.Fatalf("append command missing conv=notrunc: %v", args) - } - if !containsString(args, "oflag=append") { - t.Fatalf("append command missing oflag=append: %v", args) - } -} - -func containsString(values []string, target string) bool { - for _, value := range values { - if value == target { - return true - } - } - return false -} diff --git a/internal/sandbox/fsbridge.go b/internal/sandbox/fsbridge.go index 15278641..465e7379 100644 --- a/internal/sandbox/fsbridge.go +++ b/internal/sandbox/fsbridge.go @@ -80,8 +80,12 @@ func (b *FsBridge) WriteFile(ctx context.Context, path, content string, appendMo } } - ddArgs := fsBridgeWriteDDArgs(resolved, appendMode) - _, stderr, exitCode, err := b.dockerExec(ctx, []byte(content), ddArgs...) + // Write content via stdin without invoking a shell. Passing the resolved path + // as a discrete argv entry prevents shell metacharacters in filenames from + // being interpreted as commands inside the sandbox container. + // "--" terminates option parsing so paths starting with "-" are safe. + teeArgs := fsBridgeWriteTeeArgs(resolved, appendMode) + _, stderr, exitCode, err := b.dockerExec(ctx, []byte(content), teeArgs...) if err != nil { return fmt.Errorf("fsbridge write: %w", err) } @@ -92,11 +96,12 @@ func (b *FsBridge) WriteFile(ctx context.Context, path, content string, appendMo return nil } -func fsBridgeWriteDDArgs(resolved string, appendMode bool) []string { - args := []string{"dd", "bs=1048576", "status=none", "of=" + resolved} +func fsBridgeWriteTeeArgs(resolved string, appendMode bool) []string { + args := []string{"tee"} if appendMode { - args = append(args, "conv=notrunc", "oflag=append") + args = append(args, "-a") } + args = append(args, "--", resolved) return args } diff --git a/internal/sandbox/fsbridge_test.go b/internal/sandbox/fsbridge_test.go new file mode 100644 index 00000000..493a81ad --- /dev/null +++ b/internal/sandbox/fsbridge_test.go @@ -0,0 +1,189 @@ +package sandbox + +import ( + "context" + "os" + "path/filepath" + "strings" + "testing" +) + +// fakeDockerScript is a fake `docker` binary used in WriteFile tests. +// It logs all argv to $DOCKER_LOG and, when invoked with a `tee` sub-command, +// captures stdin to $DOCKER_STDIN. +// `realpath -e -- ` echoes the path back (simulates existing path). +// `mkdir` exits 0 silently. +const fakeDockerScript = `#!/bin/sh +{ + echo CALL + i=0 + for arg in "$@"; do + echo "ARG[$i]=$arg" + i=$((i + 1)) + done +} >> "$DOCKER_LOG" + +# realpath: echo the last arg back as the resolved path +for j in $(seq 0 $#); do + eval "a=\${$j}" + if [ "$a" = "realpath" ]; then + # print the last argument (the path) + eval "echo \"\${$#}\"" + exit 0 + fi +done + +# tee: capture stdin +for arg in "$@"; do + if [ "$arg" = "tee" ]; then + if [ -n "$DOCKER_STDIN" ]; then + cat > "$DOCKER_STDIN" + else + cat > /dev/null + fi + exit 0 + fi +done + +exit 0 +` + +// installFakeDocker writes the fake docker script to tmp, prepends tmp to PATH, +// and sets DOCKER_LOG and optionally DOCKER_STDIN env vars for the test. +func installFakeDocker(t *testing.T, tmp, logPath, stdinPath string) { + t.Helper() + dockerPath := filepath.Join(tmp, "docker") + if err := os.WriteFile(dockerPath, []byte(fakeDockerScript), 0o755); err != nil { + t.Fatalf("write fake docker: %v", err) + } + t.Setenv("PATH", tmp+string(os.PathListSeparator)+os.Getenv("PATH")) + t.Setenv("DOCKER_LOG", logPath) + if stdinPath != "" { + t.Setenv("DOCKER_STDIN", stdinPath) + } +} + +// TestFsBridgeWriteFileDoesNotInvokeShell asserts that WriteFile passes the +// resolved path as a discrete argv entry to `tee -- ` and does NOT +// build a shell command string (no sh / -c / shell metachar expansion). +func TestFsBridgeWriteFileDoesNotInvokeShell(t *testing.T) { + tmp := t.TempDir() + logPath := filepath.Join(tmp, "docker.log") + stdinPath := filepath.Join(tmp, "stdin.txt") + + installFakeDocker(t, tmp, logPath, stdinPath) + + bridge := NewFsBridge("container-id", "/workspace") + maliciousPath := `nested/evil$(touch /tmp/goclaw-fsbridge-pwned);name.txt` + content := "safe content" + + if err := bridge.WriteFile(context.Background(), maliciousPath, content, false); err != nil { + t.Fatalf("WriteFile returned error: %v", err) + } + + logBytes, err := os.ReadFile(logPath) + if err != nil { + t.Fatalf("read docker log: %v", err) + } + log := string(logBytes) + + // Must NOT have invoked sh or built a shell string. + if strings.Contains(log, "=sh") || strings.Contains(log, "=-c") || strings.Contains(log, "cat >") { + t.Fatalf("WriteFile invoked shell command path; log:\n%s", log) + } + // Must have called tee. + if !strings.Contains(log, "=tee") { + t.Fatalf("expected write command to use tee without shell; log:\n%s", log) + } + // Must have included -- to terminate option parsing. + if !strings.Contains(log, "=--") { + t.Fatalf("expected tee delimiter (--) before filename; log:\n%s", log) + } + // The malicious path must appear verbatim as a single argv entry (not split/executed). + resolved := "/workspace/" + maliciousPath + if !strings.Contains(log, "="+resolved) { + t.Fatalf("expected malicious filename to remain one argv entry; log:\n%s", log) + } + + stdinBytes, err := os.ReadFile(stdinPath) + if err != nil { + t.Fatalf("read captured stdin: %v", err) + } + if string(stdinBytes) != content { + t.Fatalf("stdin content = %q, want %q", string(stdinBytes), content) + } +} + +// TestFsBridgeWriteFileAppendUsesTeeAppendArg asserts that append mode passes +// -a before -- in the tee argv, i.e. `tee -a -- `. +func TestFsBridgeWriteFileAppendUsesTeeAppendArg(t *testing.T) { + tmp := t.TempDir() + logPath := filepath.Join(tmp, "docker.log") + + installFakeDocker(t, tmp, logPath, "") + + bridge := NewFsBridge("container-id", "/workspace") + if err := bridge.WriteFile(context.Background(), "append.txt", "more", true); err != nil { + t.Fatalf("WriteFile append returned error: %v", err) + } + + logBytes, err := os.ReadFile(logPath) + if err != nil { + t.Fatalf("read docker log: %v", err) + } + log := string(logBytes) + + // Must have: tee, then -a, then --. + if !strings.Contains(log, "=tee") { + t.Fatalf("expected tee in argv; log:\n%s", log) + } + if !strings.Contains(log, "=-a") { + t.Fatalf("expected -a flag for append mode; log:\n%s", log) + } + if !strings.Contains(log, "=--") { + t.Fatalf("expected -- delimiter in argv; log:\n%s", log) + } +} + +// TestFsBridgeWriteTeeArgs_Overwrite checks the tee args slice for overwrite mode. +func TestFsBridgeWriteTeeArgs_Overwrite(t *testing.T) { + args := fsBridgeWriteTeeArgs("/workspace/file.txt", false) + if len(args) != 3 { + t.Fatalf("expected 3 args, got %d: %v", len(args), args) + } + if args[0] != "tee" { + t.Errorf("args[0] = %q, want tee", args[0]) + } + if args[1] != "--" { + t.Errorf("args[1] = %q, want --", args[1]) + } + if args[2] != "/workspace/file.txt" { + t.Errorf("args[2] = %q, want /workspace/file.txt", args[2]) + } + // Must not contain -a in overwrite mode. + for _, a := range args { + if a == "-a" { + t.Errorf("overwrite mode must not contain -a: %v", args) + } + } +} + +// TestFsBridgeWriteTeeArgs_Append checks the tee args slice for append mode. +func TestFsBridgeWriteTeeArgs_Append(t *testing.T) { + args := fsBridgeWriteTeeArgs("/workspace/file.txt", true) + if len(args) != 4 { + t.Fatalf("expected 4 args, got %d: %v", len(args), args) + } + if args[0] != "tee" { + t.Errorf("args[0] = %q, want tee", args[0]) + } + if args[1] != "-a" { + t.Errorf("args[1] = %q, want -a", args[1]) + } + if args[2] != "--" { + t.Errorf("args[2] = %q, want --", args[2]) + } + if args[3] != "/workspace/file.txt" { + t.Errorf("args[3] = %q, want /workspace/file.txt", args[3]) + } +} diff --git a/internal/security/ssrf.go b/internal/security/ssrf.go index 5dd170c3..45b71a12 100644 --- a/internal/security/ssrf.go +++ b/internal/security/ssrf.go @@ -77,6 +77,16 @@ func isBlocked(ip net.IP) bool { return false } +// IsBlocked reports whether ip falls within any blocked CIDR (loopback, +// link-local including cloud-metadata 169.254.169.254, RFC 1918 private, +// multicast, and unspecified 0.0.0.0/:: ranges). +// +// Use this in provider-URL validation and dial-time guards to avoid +// duplicating the CIDR list across packages. +func IsBlocked(ip net.IP) bool { + return isBlocked(ip) +} + // redactURL strips query string and userinfo for safe logging. func redactURL(rawURL string) string { u, err := url.Parse(rawURL) diff --git a/internal/tools/policy.go b/internal/tools/policy.go index 7d83dc3b..28cd574d 100644 --- a/internal/tools/policy.go +++ b/internal/tools/policy.go @@ -53,7 +53,7 @@ var toolProfiles = map[string][]string{ } // Legacy tool aliases — migrated to Registry.RegisterAlias() at startup. -// Kept as seed data only; resolveAlias() is no longer used. +// resolveAlias() is used by IsDenied to expand names before deny-spec matching. var legacyToolAliases = map[string]string{ "bash": "exec", "apply-patch": "apply_patch", @@ -434,19 +434,34 @@ func unionWithSpec(reg *Registry, current []string, allTools []string, spec []st // IsDenied checks if a tool name is explicitly denied by global or agent policy. // Used to prevent lazy-activated deferred tools from bypassing the deny list. +// Checks under all candidate names: the raw name, the legacy alias (e.g. bash→exec), +// and the registry alias when available. func (pe *PolicyEngine) IsDenied(name string, agentPolicy *config.ToolPolicySpec) bool { + candidates := map[string]struct{}{name: {}} + // Keep legacy alias compatibility (e.g. bash -> exec). + candidates[resolveAlias(name)] = struct{}{} + // Include registry alias mapping when available. pe.mu.RLock() reg := pe.registry pe.mu.RUnlock() + if reg != nil { + if canonical, ok := reg.Aliases()[name]; ok && canonical != "" { + candidates[canonical] = struct{}{} + } + } if pe.globalPolicy != nil { - if matchDenySpec(reg, name, pe.globalPolicy.Deny) { - return true + for candidate := range candidates { + if matchDenySpec(reg, candidate, pe.globalPolicy.Deny) { + return true + } } } if agentPolicy != nil { - if matchDenySpec(reg, name, agentPolicy.Deny) { - return true + for candidate := range candidates { + if matchDenySpec(reg, candidate, agentPolicy.Deny) { + return true + } } } return false