From 97fd65d85da4b86410e4c19b9fbc656b1cf66f44 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Fri, 29 May 2026 22:55:52 -0700 Subject: [PATCH] Close config trust-boundary gaps and gate the completion loader - Trust-gate cache_dir, cache_enabled, llm_model, llm_max_concurrent and llm_token_budget so an untrusted local/repo config can't redirect cache writes or amplify paid-LLM usage; only honored from trusted sources. 'config set' now also warns when these keys are written to an untrusted local config so the value isn't silently ignored on the next load. - Scheme/host-validate llm_endpoint on accept (rejects file://, hostless and malformed forms) and re-validate at the root.go enforcement point so env/ profile-sourced endpoints can't slip a non-http(s) scheme past RequireSecureURL, which only blocks http:// for non-localhost; this prevents leaking llm_api_key in cleartext. - Gate the completion profile loader behind the TrustStore and strip control characters from completion descriptions. --- internal/cli/root.go | 57 +++++++- internal/cli/root_test.go | 45 ++++++ internal/commands/config.go | 27 +++- internal/commands/config_test.go | 31 +++- internal/completion/cache.go | 16 +- internal/completion/completer.go | 21 ++- internal/completion/completer_test.go | 22 +++ internal/config/config.go | 83 +++++++++-- internal/config/config_test.go | 202 +++++++++++++++++++++++++- 9 files changed, 475 insertions(+), 29 deletions(-) diff --git a/internal/cli/root.go b/internal/cli/root.go index e38a5680..12b28d9e 100644 --- a/internal/cli/root.go +++ b/internal/cli/root.go @@ -98,7 +98,13 @@ func NewRootCmd() *cobra.Command { return err } // Re-apply env and flag overrides (they take precedence over profile values) - config.LoadFromEnv(cfg) + if err := config.LoadFromEnv(cfg); err != nil { + if bareRoot { + initBareRootApp(cfg) + return nil + } + return err + } config.ApplyOverrides(cfg, config.FlagOverrides{ Account: flags.Account, Project: flags.Project, @@ -126,6 +132,24 @@ func NewRootCmd() *cobra.Command { } return fmt.Errorf("base_url (%s): %w\nFix with: basecamp config unset base_url", source, err) } + + // Validate the LLM endpoint. The config-file accept path already + // rejects non-http(s)/hostless values, but env- and profile-sourced + // endpoints reach here unchecked, so re-validate the scheme/host + // and enforce HTTPS (credential-gated) so an http:// non-localhost + // endpoint can't leak llm_api_key in cleartext. Empty endpoint is a + // no-op. Same config-subcommand skip. + if err := validateLLMEndpoint(cfg.LLMEndpoint, cfg.LLMProvider, cfg.LLMAPIKey); err != nil { + if bareRoot { + initBareRootApp(cfg) + return nil + } + source := cfg.Sources["llm_endpoint"] + if source == "" { + source = "unknown" + } + return fmt.Errorf("llm_endpoint (%s): %w\nFix with: basecamp config unset llm_endpoint", source, err) + } } // Resolve behavior preferences: explicit flag > config > version.IsDev() @@ -528,6 +552,37 @@ func promptForProfile(cfg *config.Config) (string, error) { return selected.ID, nil } +// validateLLMEndpoint rejects non-http(s)/hostless endpoints always, and requires +// HTTPS for a non-localhost endpoint only when a credential could be transmitted. +// +// RequireSecureURL only blocks http:// for non-localhost — it would let file://, +// ssh:// etc. through — so the IsHTTPURL scheme/host check stays unconditional. +// +// The HTTPS/credential gate is skipped for credential-less providers ("ollama", +// "apple", "none", "disabled"): they never transmit llm_api_key, so a remote http:// endpoint +// (e.g. a LAN Ollama) is allowed even when a global key exists for a different +// provider. For credentialed/ambiguous providers ("openai", "anthropic", "auto", +// or "") the gate applies — an http:// non-localhost endpoint is rejected when an +// llm_api_key is present, since the secret would otherwise leak in cleartext. +// Without a key there is no secret to leak. An empty endpoint is a no-op. +func validateLLMEndpoint(endpoint, provider, apiKey string) error { + if endpoint == "" { + return nil + } + if !config.IsHTTPURL(endpoint) { + return fmt.Errorf("must be an http:// or https:// URL with a host") + } + switch provider { + case "ollama", "apple", "none", "disabled": + // Credential-less providers never send the key; skip the HTTPS gate. + return nil + } + if apiKey != "" { + return hostutil.RequireSecureURL(endpoint) + } + return nil +} + // isConfigCmd returns true if cmd is "config" or any of its subcommands. // Used to skip HTTPS enforcement so users can repair a bad base_url. func isConfigCmd(cmd *cobra.Command) bool { diff --git a/internal/cli/root_test.go b/internal/cli/root_test.go index 5f3fd905..96f493f7 100644 --- a/internal/cli/root_test.go +++ b/internal/cli/root_test.go @@ -14,6 +14,51 @@ import ( "github.com/basecamp/basecamp-cli/internal/version" ) +// TestLLMEndpointValidation exercises the production validateLLMEndpoint helper +// in root.go: the scheme/host check is unconditional, while the HTTPS gate is +// only enforced for credentialed/ambiguous providers when a key is present. +// Credential-less providers (ollama, apple, none) never send the key, so a +// remote http endpoint is allowed even when a key exists for a different provider. +func TestLLMEndpointValidation(t *testing.T) { + tests := []struct { + name string + endpoint string + provider string + apiKey string + wantOK bool + }{ + {"file scheme rejected", "file:///etc/passwd", "", "", false}, + {"ssh scheme rejected", "ssh://host", "", "", false}, + {"hostless https rejected", "https:example.com", "", "", false}, + {"http remote with credential rejected", "http://remote-host:1234", "", "secret", false}, + {"http remote without credential accepted", "http://remote-host:1234", "", "", true}, + {"https remote accepted", "https://remote-host", "", "", true}, + {"https remote with credential accepted", "https://remote-host", "", "secret", true}, + {"localhost with credential accepted", "http://localhost:11434", "", "secret", true}, + {"empty endpoint no-op", "", "", "", true}, + {"empty endpoint with key no-op", "", "", "secret", true}, + // Credential-less provider: remote http allowed even with a stray key. + {"ollama remote http with key accepted", "http://192.168.1.10:11434", "ollama", "secret", true}, + // Credentialed/ambiguous providers: key still gates remote http. + {"openai remote http with key rejected", "http://remote:1234", "openai", "secret", false}, + {"auto remote http with key rejected", "http://remote:1234", "auto", "secret", false}, + {"anthropic remote http with key rejected", "http://remote:1234", "anthropic", "secret", false}, + // Unconditional scheme check fires before the credential-less exemption. + {"ollama file scheme rejected", "file:///etc/passwd", "ollama", "secret", false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := validateLLMEndpoint(tt.endpoint, tt.provider, tt.apiKey) + if tt.wantOK { + assert.NoError(t, err) + } else { + assert.Error(t, err) + } + }) + } +} + func TestResolvePreferences(t *testing.T) { boolPtr := func(b bool) *bool { return &b } intPtr := func(i int) *int { return &i } diff --git a/internal/commands/config.go b/internal/commands/config.go index d588f243..3411f460 100644 --- a/internal/commands/config.go +++ b/internal/commands/config.go @@ -297,6 +297,11 @@ Valid keys: account_id, project_id (or project), todolist_id, base_url, cache_di return output.ErrUsage(fmt.Sprintf("llm_provider must be one of: anthropic, openai, ollama, apple, none, auto (got %q)", value)) } configData[key] = value + case "llm_endpoint": + if !config.IsHTTPURL(value) { + return output.ErrUsage("llm_endpoint must be an http:// or https:// URL with a host") + } + configData[key] = value case "llm_max_concurrent": level, err := strconv.Atoi(value) if err != nil || level < 1 || level > 10 { @@ -343,12 +348,13 @@ Valid keys: account_id, project_id (or project), todolist_id, base_url, cache_di return fmt.Errorf("failed to write config: %w", err) } - // Warn when writing authority keys to local config without trust - if !global && isAuthorityKey(key) { + // Warn when writing trust-gated keys to local config without trust — + // otherwise the value is silently ignored on the next load. + if !global && isTrustGatedKey(key) { absPath, _ := filepath.Abs(configPath) ts := config.LoadTrustStore(config.GlobalConfigDir()) if ts == nil || !ts.IsTrusted(configPath) { - fmt.Fprintf(os.Stderr, "warning: authority key %q in local config requires trust to take effect; run:\n basecamp config trust %s\n", key, config.ShellQuote(absPath)) + fmt.Fprintf(os.Stderr, "warning: %q in local config requires trust to take effect; run:\n basecamp config trust %s\n", key, config.ShellQuote(absPath)) } } @@ -406,6 +412,21 @@ func isAuthorityKey(key string) bool { return false } +// isTrustGatedKey reports whether key is ignored when loaded from an untrusted +// local/repo config. It's the authority keys plus the cache/LLM keys gated for +// the same reason (cache redirection, paid-model substitution, cost +// amplification), so `config set` warns before a local write silently no-ops. +func isTrustGatedKey(key string) bool { + if isAuthorityKey(key) { + return true + } + switch key { + case "cache_dir", "cache_enabled", "llm_model", "llm_max_concurrent", "llm_token_budget": + return true + } + return false +} + // redactSecret returns "(set)" if the value is non-empty, empty string otherwise. func redactSecret(value string) string { if value != "" { diff --git a/internal/commands/config_test.go b/internal/commands/config_test.go index 8bcbdc7d..8d3328f6 100644 --- a/internal/commands/config_test.go +++ b/internal/commands/config_test.go @@ -337,12 +337,41 @@ func TestConfigSet_AuthorityKeyWarnsWithPath(t *testing.T) { stderr := string(buf[:n]) absPath := filepath.Join(tmpDir, ".basecamp", "config.json") - assert.Contains(t, stderr, "authority key") + assert.Contains(t, stderr, `"base_url"`) assert.Contains(t, stderr, "requires trust") assert.Contains(t, stderr, absPath, "warning must include the exact config path") assert.Contains(t, stderr, "'"+absPath+"'", "path must be single-quoted for shell safety") } +// TestConfigSet_GatedNonAuthorityKeyWarns verifies the trust warning also fires +// for the cache/LLM keys gated on load (cache_dir, llm_model, …), so a local +// `config set` doesn't silently produce a value that's ignored next run. +func TestConfigSet_GatedNonAuthorityKeyWarns(t *testing.T) { + app, _ := setupConfigTestApp(t) + + tmpDir, _ := filepath.EvalSymlinks(t.TempDir()) + origDir, _ := os.Getwd() + require.NoError(t, os.Chdir(tmpDir)) + defer os.Chdir(origDir) + require.NoError(t, os.MkdirAll(".basecamp", 0755)) + + origStderr := os.Stderr + r, w, _ := os.Pipe() + os.Stderr = w + + err := executeConfigCommand(app, "set", "cache_dir", "/tmp/somewhere") + require.NoError(t, err) + + w.Close() + var buf [4096]byte + n, _ := r.Read(buf[:]) + os.Stderr = origStderr + + stderr := string(buf[:n]) + assert.Contains(t, stderr, `"cache_dir"`) + assert.Contains(t, stderr, "requires trust") +} + // --- Config project tests --- // setupConfigProjectTestApp creates a test app wired to an httptest server diff --git a/internal/completion/cache.go b/internal/completion/cache.go index 7c3588b8..8de95281 100644 --- a/internal/completion/cache.go +++ b/internal/completion/cache.go @@ -10,6 +10,8 @@ import ( "path/filepath" "sync" "time" + + "github.com/basecamp/basecamp-cli/internal/config" ) // CachedProject holds project data for tab completion. @@ -366,13 +368,21 @@ func loadConfigForCompletion() *configForCompletion { loadProfilesFromFile(cfg, globalPath) } + // profiles is an authority key (it can redirect authenticated traffic), so + // repo/local configs must be explicitly trusted before their profiles feed + // completion — mirroring config.loadFromFile's trust gating. System/global + // configs above are trusted by location. + trust := config.LoadTrustStore(config.GlobalConfigDir()) + // Repo config (walk up to find .git, then .basecamp/config.json) if dir, err := os.Getwd(); err == nil { for { gitPath := filepath.Join(dir, ".git") if fi, err := os.Stat(gitPath); err == nil && fi.IsDir() { repoConfig := filepath.Join(dir, ".basecamp", "config.json") - loadProfilesFromFile(cfg, repoConfig) + if trust != nil && trust.IsTrusted(repoConfig) { + loadProfilesFromFile(cfg, repoConfig) + } break } parent := filepath.Dir(dir) @@ -385,7 +395,9 @@ func loadConfigForCompletion() *configForCompletion { // Local config localPath := filepath.Join(".basecamp", "config.json") - loadProfilesFromFile(cfg, localPath) + if trust != nil && trust.IsTrusted(localPath) { + loadProfilesFromFile(cfg, localPath) + } return cfg } diff --git a/internal/completion/completer.go b/internal/completion/completer.go index 74468eb2..2dfe72a3 100644 --- a/internal/completion/completer.go +++ b/internal/completion/completer.go @@ -91,7 +91,7 @@ func (c *Completer) ProjectCompletion() cobra.CompletionFunc { // Use ID as completion value with name as description completion := cobra.CompletionWithDesc( fmt.Sprintf("%d", p.ID), - p.Name, + sanitizeCompletionDesc(p.Name), ) completions = append(completions, completion) } @@ -164,7 +164,7 @@ func (c *Completer) PeopleCompletion() cobra.CompletionFunc { } completion := cobra.CompletionWithDesc( fmt.Sprintf("%d", p.ID), - desc, + sanitizeCompletionDesc(desc), ) completions = append(completions, completion) } @@ -237,7 +237,7 @@ func (c *Completer) AccountCompletion() cobra.CompletionFunc { strings.HasPrefix(nameLower, toCompleteLower) || strings.Contains(nameLower, toCompleteLower) { // Use ID as completion value with name as description - completions = append(completions, cobra.CompletionWithDesc(idStr, a.Name)) + completions = append(completions, cobra.CompletionWithDesc(idStr, sanitizeCompletionDesc(a.Name))) } } @@ -270,7 +270,7 @@ func (c *Completer) ProfileCompletion() cobra.CompletionFunc { if strings.HasPrefix(nameLower, toCompleteLower) || strings.Contains(nameLower, toCompleteLower) { // Use name as completion value with base URL as description - completions = append(completions, cobra.CompletionWithDesc(p.Name, p.BaseURL)) + completions = append(completions, cobra.CompletionWithDesc(p.Name, sanitizeCompletionDesc(p.BaseURL))) } } @@ -278,6 +278,19 @@ func (c *Completer) ProfileCompletion() cobra.CompletionFunc { } } +// sanitizeCompletionDesc drops control characters (including ESC) from a +// completion description. Descriptions can carry API- or config-controlled +// strings (project/person/account names, profile base_url) which the shell +// renders to the terminal; stripping control bytes prevents terminal injection. +func sanitizeCompletionDesc(s string) string { + return strings.Map(func(r rune) rune { + if r < 0x20 || r == 0x7f || (r >= 0x80 && r <= 0x9f) { + return -1 + } + return r + }, s) +} + // rankProjects returns projects sorted by priority: // 1. HQ (purpose="hq") // 2. Bookmarked diff --git a/internal/completion/completer_test.go b/internal/completion/completer_test.go index e21cd30e..5f9c31ca 100644 --- a/internal/completion/completer_test.go +++ b/internal/completion/completer_test.go @@ -24,6 +24,28 @@ func newTestCmd() *cobra.Command { return cmd } +func TestSanitizeCompletionDesc(t *testing.T) { + tests := []struct { + name string + in string + want string + }{ + {"C0 control SOH stripped", "a\x01b", "ab"}, + {"C0 control US stripped", "a\x1fb", "ab"}, + {"DEL stripped", "a\x7fb", "ab"}, + {"C1 control 0x80 stripped", "a\u0080b", "ab"}, + {"C1 control 0x9f stripped", "a\u009fb", "ab"}, + {"valid accented rune passes through", "café", "café"}, + {"valid emoji rune passes through", "party🎉", "party🎉"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, sanitizeCompletionDesc(tt.in)) + }) + } +} + func TestRankProjects(t *testing.T) { now := time.Now() projects := []CachedProject{ diff --git a/internal/config/config.go b/internal/config/config.go index ac649f20..2bfab63c 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -4,6 +4,7 @@ package config import ( "encoding/json" "fmt" + "net/url" "os" "path/filepath" "strings" @@ -144,7 +145,9 @@ func Load(overrides FlagOverrides) (*Config, error) { } // Load from environment - LoadFromEnv(cfg) + if err := LoadFromEnv(cfg); err != nil { + return nil, err + } // Apply flag overrides ApplyOverrides(cfg, overrides) @@ -196,12 +199,24 @@ func loadFromFile(cfg *Config, path string, source Source, trust *TrustStore) { cfg.Sources["scope"] = string(source) } if v, ok := fileCfg["cache_dir"].(string); ok && v != "" { - cfg.CacheDir = v - cfg.Sources["cache_dir"] = string(source) + // cache_dir redirects every cache write (completion, resilience, TUI + // workspace, recents, traces). An untrusted local/repo config could + // point it at any user-writable path, so gate it like other authority + // keys. filepath.Clean normalizes the accepted value. + if untrusted { + fmt.Fprintf(os.Stderr, "warning: ignoring cache_dir %q from %s config at %s\n (trust-gated key from local/repo config; run `basecamp config trust %s` to allow)\n", v, source, path, ShellQuote(path)) + } else { + cfg.CacheDir = filepath.Clean(v) + cfg.Sources["cache_dir"] = string(source) + } } if v, ok := fileCfg["cache_enabled"].(bool); ok { - cfg.CacheEnabled = v - cfg.Sources["cache_enabled"] = string(source) + if untrusted { + fmt.Fprintf(os.Stderr, "warning: ignoring cache_enabled from %s config at %s\n (trust-gated key from local/repo config; run `basecamp config trust %s` to allow)\n", source, path, ShellQuote(path)) + } else { + cfg.CacheEnabled = v + cfg.Sources["cache_enabled"] = string(source) + } } if v, ok := fileCfg["format"].(string); ok && v != "" { cfg.Format = v @@ -237,8 +252,14 @@ func loadFromFile(cfg *Config, path string, source Source, trust *TrustStore) { } } if v, ok := fileCfg["llm_model"].(string); ok && v != "" { - cfg.LLMModel = v - cfg.Sources["llm_model"] = string(source) + // Gate like other LLM authority keys: an untrusted config could + // silently substitute a costlier paid model. + if untrusted { + fmt.Fprintf(os.Stderr, "warning: ignoring llm_model %q from %s config at %s\n (trust-gated key from local/repo config; run `basecamp config trust %s` to allow)\n", v, source, path, ShellQuote(path)) + } else { + cfg.LLMModel = v + cfg.Sources["llm_model"] = string(source) + } } if v, ok := fileCfg["llm_api_key"].(string); ok && v != "" { // Secret: only from global/system config, never local/repo @@ -253,6 +274,11 @@ func loadFromFile(cfg *Config, path string, source Source, trust *TrustStore) { if untrusted { fmt.Fprintf(os.Stderr, "warning: ignoring llm_endpoint %q from %s config at %s\n (authority key from local/repo config; run `basecamp config trust %s` to allow)\n", v, source, path, ShellQuote(path)) } else { + // Keep the value even if malformed (non-http(s)/hostless): root.go's + // validateLLMEndpoint runs IsHTTPURL unconditionally for non-config + // commands and rejects it there, failing closed rather than silently + // dropping it and falling back to the default provider. config + // subcommands skip that check so `config unset llm_endpoint` can repair. cfg.LLMEndpoint = v cfg.Sources["llm_endpoint"] = string(source) } @@ -260,7 +286,11 @@ func loadFromFile(cfg *Config, path string, source Source, trust *TrustStore) { if v, ok := fileCfg["llm_max_concurrent"]; ok { if fv, ok := v.(float64); ok { iv := int(fv) - if iv >= 1 && iv <= 10 && fv == float64(iv) { + // Gate like other LLM authority keys: block a malicious repo from + // inflating paid-LLM concurrency (cost amplification). + if untrusted { + fmt.Fprintf(os.Stderr, "warning: ignoring llm_max_concurrent from %s config at %s\n (trust-gated key from local/repo config; run `basecamp config trust %s` to allow)\n", source, path, ShellQuote(path)) + } else if iv >= 1 && iv <= 10 && fv == float64(iv) { cfg.LLMMaxConcurrent = iv cfg.Sources["llm_max_concurrent"] = string(source) } @@ -269,7 +299,10 @@ func loadFromFile(cfg *Config, path string, source Source, trust *TrustStore) { if v, ok := fileCfg["llm_token_budget"]; ok { if fv, ok := v.(float64); ok { iv := int(fv) - if iv >= 100 && iv <= 100000 && fv == float64(iv) { + // Gate like other LLM authority keys (cost amplification). + if untrusted { + fmt.Fprintf(os.Stderr, "warning: ignoring llm_token_budget from %s config at %s\n (trust-gated key from local/repo config; run `basecamp config trust %s` to allow)\n", source, path, ShellQuote(path)) + } else if iv >= 100 && iv <= 100000 && fv == float64(iv) { cfg.LLMTokenBudget = iv cfg.Sources["llm_token_budget"] = string(source) } @@ -335,7 +368,14 @@ func loadFromFile(cfg *Config, path string, source Source, trust *TrustStore) { // LoadFromEnv loads configuration from environment variables. // Exported so root.go can re-apply after profile overlay. -func LoadFromEnv(cfg *Config) { +// +// Unlike loadFromFile (which warns and drops a malformed field so a +// multi-field file stays usable and repairable via `config unset`), a +// malformed BASECAMP_LLM_ENDPOINT env override fails closed: it returns an +// error rather than silently dropping the value. Dropping it would leave +// LLMEndpoint empty and route prompts to the default provider, so a typo +// must surface to the user instead of being swallowed. +func LoadFromEnv(cfg *Config) error { if v := os.Getenv("BASECAMP_BASE_URL"); v != "" { cfg.BaseURL = v cfg.Sources["base_url"] = string(SourceEnv) @@ -385,9 +425,19 @@ func LoadFromEnv(cfg *Config) { cfg.Sources["llm_api_key"] = string(SourceEnv) } if v := os.Getenv("BASECAMP_LLM_ENDPOINT"); v != "" { + if !IsHTTPURL(v) { + // Fail closed: reject non-http(s) schemes (file://, etc.) and + // hostless/malformed forms here rather than dropping them. Dropping + // would leave LLMEndpoint empty and silently fall back to the default + // provider, so a typo would route prompts to the wrong endpoint. + // https enforcement for non-localhost endpoints happens later in + // root.go via RequireSecureURL. + return fmt.Errorf("BASECAMP_LLM_ENDPOINT (%q): must be an http:// or https:// URL with a host", v) + } cfg.LLMEndpoint = v cfg.Sources["llm_endpoint"] = string(SourceEnv) } + return nil } // parseEnvBool parses a boolean environment variable strictly. @@ -660,6 +710,19 @@ func NormalizeBaseURL(url string) string { return strings.TrimSuffix(url, "/") } +// IsHTTPURL reports whether rawURL is an absolute http(s) URL with a host. +// Used to reject non-web schemes (file://, etc.) and hostless forms +// (e.g. "https:example.com", which url.Parse accepts with an empty Host) +// for llm_endpoint, since those would later be concatenated into a request +// URL and produce confusing/invalid requests. +func IsHTTPURL(rawURL string) bool { + u, err := url.Parse(rawURL) + if err != nil { + return false + } + return (u.Scheme == "http" || u.Scheme == "https") && u.Host != "" +} + // ShellQuote returns a POSIX single-quoted string safe for copy-paste into // a shell. Single quotes inside the value are escaped as '\” (end quote, // escaped literal quote, resume quote). diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 4ecf9895..d4d5ae90 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -117,7 +117,7 @@ func TestLoadFromEnv(t *testing.T) { os.Setenv("BASECAMP_CACHE_ENABLED", "false") cfg := Default() - LoadFromEnv(cfg) + require.NoError(t, LoadFromEnv(cfg)) // Verify values loaded assert.Equal(t, "http://env.example.com", cfg.BaseURL) @@ -149,7 +149,7 @@ func TestLoadFromEnvPrecedence(t *testing.T) { os.Setenv("BASECAMP_BASE_URL", "http://basecamp.example.com") cfg := Default() - LoadFromEnv(cfg) + require.NoError(t, LoadFromEnv(cfg)) assert.Equal(t, "http://basecamp.example.com", cfg.BaseURL) } @@ -286,7 +286,7 @@ func TestFullLayeringPrecedence(t *testing.T) { // Apply layers in order loadFromFile(cfg, globalConfig, SourceGlobal, nil) loadFromFile(cfg, localConfig, SourceLocal, nil) - LoadFromEnv(cfg) + require.NoError(t, LoadFromEnv(cfg)) ApplyOverrides(cfg, FlagOverrides{ // No flag overrides }) @@ -382,7 +382,7 @@ func TestCacheEnabledEnvParsing(t *testing.T) { cfg := Default() cfg.CacheEnabled = tt.startValue - LoadFromEnv(cfg) + require.NoError(t, LoadFromEnv(cfg)) assert.Equal(t, tt.expected, cfg.CacheEnabled) }) @@ -404,7 +404,7 @@ func TestCacheEnabledEnvEmpty(t *testing.T) { cfg := Default() cfg.CacheEnabled = true - LoadFromEnv(cfg) + require.NoError(t, LoadFromEnv(cfg)) // Should remain true (env var not set, so doesn't change) assert.True(t, cfg.CacheEnabled) @@ -781,6 +781,192 @@ func TestLoadFromFile_BaseURLRejectedFromLocal(t *testing.T) { assert.Equal(t, "https://3.basecampapi.com", cfg.BaseURL, "local config should not override base_url") } +// TestLoadFromFile_AuthorityKeysRejectedFromLocal verifies the sibling +// authority keys closed by the security audit (cache_dir, cache_enabled, +// llm_model, llm_max_concurrent, llm_token_budget) are ignored — with a +// warning — when they appear in an untrusted local/repo config, while their +// default values are preserved. +func TestLoadFromFile_AuthorityKeysRejectedFromLocal(t *testing.T) { + defaults := Default() + + cases := []struct { + name string + json string + warnFrag string + assertDef func(t *testing.T, cfg *Config) + }{ + { + name: "cache_dir", + json: `{"cache_dir":"/home/victim/.ssh"}`, + warnFrag: "ignoring cache_dir", + assertDef: func(t *testing.T, cfg *Config) { + assert.Equal(t, defaults.CacheDir, cfg.CacheDir) + assert.Empty(t, cfg.Sources["cache_dir"]) + }, + }, + { + name: "cache_enabled", + json: `{"cache_enabled":false}`, + warnFrag: "ignoring cache_enabled", + assertDef: func(t *testing.T, cfg *Config) { + assert.Equal(t, defaults.CacheEnabled, cfg.CacheEnabled) + assert.Empty(t, cfg.Sources["cache_enabled"]) + }, + }, + { + name: "llm_model", + json: `{"llm_model":"gpt-4-turbo-expensive"}`, + warnFrag: "ignoring llm_model", + assertDef: func(t *testing.T, cfg *Config) { + assert.Equal(t, defaults.LLMModel, cfg.LLMModel) + assert.Empty(t, cfg.Sources["llm_model"]) + }, + }, + { + name: "llm_max_concurrent", + json: `{"llm_max_concurrent":10}`, + warnFrag: "ignoring llm_max_concurrent", + assertDef: func(t *testing.T, cfg *Config) { + assert.Equal(t, defaults.LLMMaxConcurrent, cfg.LLMMaxConcurrent) + assert.Empty(t, cfg.Sources["llm_max_concurrent"]) + }, + }, + { + name: "llm_token_budget", + json: `{"llm_token_budget":100000}`, + warnFrag: "ignoring llm_token_budget", + assertDef: func(t *testing.T, cfg *Config) { + assert.Equal(t, defaults.LLMTokenBudget, cfg.LLMTokenBudget) + assert.Empty(t, cfg.Sources["llm_token_budget"]) + }, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + tmpDir := t.TempDir() + configPath := filepath.Join(tmpDir, "config.json") + require.NoError(t, os.WriteFile(configPath, []byte(tc.json), 0644)) + + origStderr := os.Stderr + r, w, _ := os.Pipe() + os.Stderr = w + + cfg := Default() + loadFromFile(cfg, configPath, SourceLocal, nil) // nil trust → untrusted + + w.Close() + var buf [1024]byte + n, _ := r.Read(buf[:]) + os.Stderr = origStderr + + assert.Contains(t, string(buf[:n]), tc.warnFrag) + tc.assertDef(t, cfg) + }) + } +} + +// TestLoadFromFile_AuthorityKeysAcceptedFromGlobal confirms the same keys are +// honored from a trusted (global) source, so the gating doesn't over-reach. +func TestLoadFromFile_AuthorityKeysAcceptedFromGlobal(t *testing.T) { + tmpDir := t.TempDir() + configPath := filepath.Join(tmpDir, "config.json") + require.NoError(t, os.WriteFile(configPath, []byte(`{ + "cache_dir":"/var/cache/basecamp", + "cache_enabled":false, + "llm_model":"gpt-4", + "llm_max_concurrent":7, + "llm_token_budget":5000 + }`), 0644)) + + cfg := Default() + loadFromFile(cfg, configPath, SourceGlobal, nil) + + assert.Equal(t, "/var/cache/basecamp", cfg.CacheDir) + assert.False(t, cfg.CacheEnabled) + assert.Equal(t, "gpt-4", cfg.LLMModel) + assert.Equal(t, 7, cfg.LLMMaxConcurrent) + assert.Equal(t, 5000, cfg.LLMTokenBudget) +} + +// TestLoadFromFile_LLMEndpointMalformedKept verifies a malformed (non-http(s) or +// hostless) llm_endpoint from a trusted source is KEPT rather than warned+dropped. +// Dropping it would silently fall back to the default provider; keeping it lets +// root.go's validateLLMEndpoint fail closed for non-config commands while config +// subcommands can still repair via `config unset llm_endpoint`. +func TestLoadFromFile_LLMEndpointMalformedKept(t *testing.T) { + for _, bad := range []string{"file:///etc/passwd", "https:example.com", "ssh://host/x"} { + t.Run(bad, func(t *testing.T) { + tmpDir := t.TempDir() + configPath := filepath.Join(tmpDir, "config.json") + body, _ := json.Marshal(map[string]any{"llm_endpoint": bad}) + require.NoError(t, os.WriteFile(configPath, body, 0644)) + + cfg := Default() + loadFromFile(cfg, configPath, SourceGlobal, nil) + + assert.Equal(t, bad, cfg.LLMEndpoint) + assert.Equal(t, "global", cfg.Sources["llm_endpoint"]) + }) + } +} + +// TestLoadFromEnv_LLMEndpointSchemeRejected verifies a non-http(s) or hostless +// BASECAMP_LLM_ENDPOINT fails closed: LoadFromEnv returns an error rather than +// warning and dropping the value, so a typo can't silently fall back to the +// default provider. +func TestLoadFromEnv_LLMEndpointSchemeRejected(t *testing.T) { + for _, bad := range []string{"file:///etc/passwd", "https:example.com", "ssh://host/x"} { + t.Run(bad, func(t *testing.T) { + t.Setenv("BASECAMP_LLM_ENDPOINT", bad) + + cfg := Default() + err := LoadFromEnv(cfg) + + require.Error(t, err) + assert.Contains(t, err.Error(), "must be an http:// or https:// URL with a host") + assert.Contains(t, err.Error(), "BASECAMP_LLM_ENDPOINT") + assert.Empty(t, cfg.LLMEndpoint) + assert.Empty(t, cfg.Sources["llm_endpoint"]) + }) + } +} + +// TestLoadFromEnv_LLMEndpointAccepted verifies a valid endpoint loads with env +// provenance and no error. +func TestLoadFromEnv_LLMEndpointAccepted(t *testing.T) { + t.Setenv("BASECAMP_LLM_ENDPOINT", "https://api.openai.com/v1") + + cfg := Default() + require.NoError(t, LoadFromEnv(cfg)) + + assert.Equal(t, "https://api.openai.com/v1", cfg.LLMEndpoint) + assert.Equal(t, "env", cfg.Sources["llm_endpoint"]) +} + +// TestLoadFromEnv_LLMEndpointEmpty verifies an unset BASECAMP_LLM_ENDPOINT is a +// no-op: no error, and no endpoint or provenance recorded. +func TestLoadFromEnv_LLMEndpointEmpty(t *testing.T) { + t.Setenv("BASECAMP_LLM_ENDPOINT", "") + + cfg := Default() + require.NoError(t, LoadFromEnv(cfg)) + + assert.Empty(t, cfg.LLMEndpoint) + assert.Empty(t, cfg.Sources["llm_endpoint"]) +} + +// TestIsHTTPURL covers the scheme+host validation reused by config-accept and +// the root.go llm_endpoint enforcement. +func TestIsHTTPURL(t *testing.T) { + for _, ok := range []string{"http://localhost:11434", "https://api.openai.com/v1", "http://127.0.0.1:8080/x"} { + assert.True(t, IsHTTPURL(ok), "%q should be accepted", ok) + } + for _, bad := range []string{"", "file:///etc/passwd", "ssh://h/x", "https:example.com", "https://", "not a url", "/relative"} { + assert.False(t, IsHTTPURL(bad), "%q should be rejected", bad) + } +} + func TestLoadFromFile_BaseURLNoWarningForGlobal(t *testing.T) { tmpDir := t.TempDir() configPath := filepath.Join(tmpDir, "config.json") @@ -961,7 +1147,7 @@ func TestPreferencesFromEnv(t *testing.T) { os.Setenv("BASECAMP_STATS", "0") cfg := Default() - LoadFromEnv(cfg) + require.NoError(t, LoadFromEnv(cfg)) require.NotNil(t, cfg.Hints) assert.True(t, *cfg.Hints) @@ -994,7 +1180,7 @@ func TestPreferencesEnvOverridesFile(t *testing.T) { cfg := Default() loadFromFile(cfg, configPath, SourceGlobal, nil) - LoadFromEnv(cfg) + require.NoError(t, LoadFromEnv(cfg)) require.NotNil(t, cfg.Hints) assert.False(t, *cfg.Hints, "env should override file") @@ -1042,7 +1228,7 @@ func TestEnvInvalidBoolIgnored(t *testing.T) { os.Setenv("BASECAMP_HINTS", "banana") cfg := Default() - LoadFromEnv(cfg) + require.NoError(t, LoadFromEnv(cfg)) assert.Nil(t, cfg.Hints, "invalid env bool should leave pointer nil") }