diff --git a/internal/http/providers.go b/internal/http/providers.go index 49040753..04d539fa 100644 --- a/internal/http/providers.go +++ b/internal/http/providers.go @@ -439,9 +439,10 @@ func normalizeOllamaAPIBase(p *store.LLMProviderData) { // localURLProviderTypes are provider types that legitimately run on localhost. // They are restricted to an explicit localhost allowlist // rather than skipping SSRF validation entirely. +// ACP is intentionally excluded: its api_base carries an executable command/path, +// not a URL (see issue #1481). var localURLProviderTypes = map[string]bool{ store.ProviderOllama: true, - store.ProviderACP: true, } // allowedLocalHosts are the only hosts permitted for local provider types. @@ -508,6 +509,9 @@ func validateProviderURL(rawURL string, providerType string) error { if providerType == store.ProviderClaudeCLI { return validateClaudeCLIExecutablePath(rawURL) } + if providerType == store.ProviderACP { + return validateACPExecutablePath(rawURL) + } u, err := url.Parse(rawURL) if err != nil { return fmt.Errorf("invalid URL: %w", err) @@ -595,6 +599,23 @@ func validateClaudeCLIExecutablePath(path string) error { return fmt.Errorf("Claude CLI api_base must be %q or an absolute executable path, got %q", "claude", path) } +// validateACPExecutablePath validates that api_base for ACP providers carries a +// command or absolute executable path, not a URL. This mirrors the runtime +// registration logic in cmd/gateway_providers.go. +func validateACPExecutablePath(path string) error { + if strings.Contains(path, "\x00") { + return fmt.Errorf("ACP binary path cannot contain NUL byte") + } + if _, err := url.ParseRequestURI(path); err == nil && strings.Contains(path, "://") { + return fmt.Errorf("ACP api_base must be an executable path or command, got URL %q", path) + } + // Keep parity with registerACPFromDB: built-in command names or absolute paths. + if path == "claude" || path == "codex" || path == "gemini" || filepath.IsAbs(path) { + return nil + } + return fmt.Errorf("ACP api_base must be %q, %q, %q, or an absolute executable path, got %q", "claude", "codex", "gemini", 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 e2481d1a..53637973 100644 --- a/internal/http/providers_test.go +++ b/internal/http/providers_test.go @@ -554,6 +554,179 @@ func TestProvidersHandlerUpdateAllowsClaudeCLIExecutablePath(t *testing.T) { } } +// TestProvidersHandlerCreateAllowsACPExecutablePath guards the fix for issue #1481: +// ACP provider api_base is an executable command/path, not a URL, and must not be +// rejected by the SSRF URL validator. +func TestProvidersHandlerCreateAllowsACPExecutablePath(t *testing.T) { + token := setupProvidersAdminToken(t) + providerStore := newMockProviderStore() + handler := NewProvidersHandler(providerStore, newMockSecretsStore(), nil, "") + mux := http.NewServeMux() + handler.RegisterRoutes(mux) + + body := map[string]any{ + "name": "acp-gemini", + "provider_type": store.ProviderACP, + "api_base": writeFakeClaudeBinary(t), // any absolute path is acceptable + "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["acp-gemini"].APIBase; got == "" || !filepath.IsAbs(got) { + t.Fatalf("stored ACP api_base = %q, want absolute executable path", got) + } +} + +// TestProvidersHandlerCreateAllowsEmptyACPBinary confirms that an empty api_base +// is accepted at create time; the runtime path may fall back to config/env. +func TestProvidersHandlerCreateAllowsEmptyACPBinary(t *testing.T) { + token := setupProvidersAdminToken(t) + providerStore := newMockProviderStore() + handler := NewProvidersHandler(providerStore, newMockSecretsStore(), nil, "") + mux := http.NewServeMux() + handler.RegisterRoutes(mux) + + body := map[string]any{ + "name": "acp-empty", + "provider_type": store.ProviderACP, + "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()) + } +} + +// TestProvidersHandlerCreateRejectsACPURL ensures ACP api_base cannot be a URL +// (it must be an executable path/command). +func TestProvidersHandlerCreateRejectsACPURL(t *testing.T) { + token := setupProvidersAdminToken(t) + providerStore := newMockProviderStore() + handler := NewProvidersHandler(providerStore, newMockSecretsStore(), nil, "") + mux := http.NewServeMux() + handler.RegisterRoutes(mux) + + body := map[string]any{ + "name": "acp-url", + "provider_type": store.ProviderACP, + "api_base": "http://127.0.0.1:9090", + "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.StatusBadRequest { + t.Fatalf("status code = %d, want %d, body=%s", w.Code, http.StatusBadRequest, w.Body.String()) + } +} + +// TestProvidersHandlerUpdateAllowsACPExecutablePath guards update-time validation +// for ACP providers. +func TestProvidersHandlerUpdateAllowsACPExecutablePath(t *testing.T) { + token := setupProvidersAdminToken(t) + providerStore := newMockProviderStore() + provider := &store.LLMProviderData{ + BaseModel: store.BaseModel{ID: uuid.New()}, + Name: "acp-local", + ProviderType: store.ProviderACP, + APIBase: "gemini", + 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) + } +} + +// TestProvidersHandlerUpdateRejectsACPURL ensures update-time validation rejects +// a URL-valued api_base for ACP providers. +func TestProvidersHandlerUpdateRejectsACPURL(t *testing.T) { + token := setupProvidersAdminToken(t) + providerStore := newMockProviderStore() + provider := &store.LLMProviderData{ + BaseModel: store.BaseModel{ID: uuid.New()}, + Name: "acp-local", + ProviderType: store.ProviderACP, + APIBase: "gemini", + 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) + + body := map[string]any{"api_base": "http://127.0.0.1:9090"} + 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.StatusBadRequest { + t.Fatalf("status code = %d, want %d, body=%s", w.Code, http.StatusBadRequest, w.Body.String()) + } +} + func TestProvidersHandlerUpdateMarshalsSettingsForStore(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 index e5fbe54a..7edaadaf 100644 --- a/internal/http/providers_url_validate_test.go +++ b/internal/http/providers_url_validate_test.go @@ -67,11 +67,10 @@ func TestValidateProviderURL(t *testing.T) { {"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 --- + // --- Scheme check: unconditional for URL-based types --- {"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 --- @@ -79,10 +78,18 @@ func TestValidateProviderURL(t *testing.T) { {"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}, + // --- ACP: executable path / command, not a URL (issue #1481) --- + {"acp empty", "", "acp", false}, + {"acp built-in gemini", "gemini", "acp", false}, + {"acp built-in claude", "claude", "acp", false}, + {"acp built-in codex", "codex", "acp", false}, + {"acp URL rejected", "http://127.0.0.1:9090", "acp", true}, + {"acp private URL rejected", "http://10.0.0.1:8080/v1", "acp", true}, + {"acp relative rejected", "relative/acp", "acp", true}, + // 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}, @@ -91,7 +98,6 @@ func TestValidateProviderURL(t *testing.T) { {"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}, @@ -189,7 +195,6 @@ func TestValidateProviderURL_LocalTypesIgnoreAllowPrivateFlag(t *testing.T) { {"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 { @@ -209,7 +214,6 @@ func TestValidateProviderURL_LocalTypeSchemeEnforced(t *testing.T) { }{ {"file:///etc/passwd", "ollama"}, {"gopher://localhost:25", "ollama"}, - {"file:///etc/passwd", "acp"}, } for _, c := range cases { err := validateProviderURL(c.url, c.providerType) @@ -275,8 +279,6 @@ func TestValidateProviderURL_LocalTypeAllowedHosts(t *testing.T) { {"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 { @@ -314,6 +316,38 @@ func TestValidateProviderURL_ClaudeCLIExecutablePath(t *testing.T) { } } +func TestValidateProviderURL_ACPExecutablePath(t *testing.T) { + saveAndRestoreGlobals(t) + absBinary := filepath.Join(t.TempDir(), "gemini") + + allowed := []string{ + "", + "claude", + "codex", + "gemini", + absBinary, + filepath.Join(t.TempDir(), "Gemini.app", "Contents", "MacOS", "gemini"), + } + for _, raw := range allowed { + if err := validateProviderURL(raw, "acp"); err != nil { + t.Errorf("expected ACP executable %q to be allowed, got: %v", raw, err) + } + } + + blocked := []string{ + "file:///usr/local/bin/gemini", + "https://api.anthropic.com/v1", + "relative/gemini", + "gemini --dangerous-flag", + "http://localhost:9090", + } + for _, raw := range blocked { + if err := validateProviderURL(raw, "acp"); err == nil { + t.Errorf("expected ACP executable %q to be rejected", raw) + } + } +} + // --- Public remote URL always OK --- func TestValidateProviderURL_PublicHostOK(t *testing.T) { @@ -346,9 +380,6 @@ func TestValidateProviderURL_OllamaAllowedHosts(t *testing.T) { if err := validateProviderURL("http://192.168.3.31:11434/v1", "ollama"); err != nil { t.Errorf("expected configured LAN IP to be allowed, got: %v", err) } - if err := validateProviderURL("http://ollama.lan:11434/v1", "acp"); err != nil { - t.Errorf("expected configured LAN hostname to be allowed for acp, got: %v", err) - } // Case-insensitive host match, matching the existing style in this file. if err := validateProviderURL("http://OLLAMA.LAN:11434/v1", "ollama"); err != nil { t.Errorf("expected case-insensitive host match to be allowed, got: %v", err)