Skip to content

Commit 3f44637

Browse files
committed
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. - Scheme-validate llm_endpoint on accept (reject file:// etc.) and enforce RequireSecureURL in root.go so an http:// endpoint can't leak the LLM API key in cleartext. - Gate the completion profile loader behind the TrustStore and strip control characters from completion descriptions.
1 parent 9e7bb58 commit 3f44637

5 files changed

Lines changed: 225 additions & 14 deletions

File tree

internal/cli/root.go

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -126,6 +126,21 @@ func NewRootCmd() *cobra.Command {
126126
}
127127
return fmt.Errorf("base_url (%s): %w\nFix with: basecamp config unset base_url", source, err)
128128
}
129+
130+
// Enforce HTTPS for the LLM endpoint too: an http:// non-localhost
131+
// endpoint would leak llm_api_key in cleartext. Empty endpoint is a
132+
// no-op. Same config-subcommand skip as base_url.
133+
if err := hostutil.RequireSecureURL(cfg.LLMEndpoint); err != nil {
134+
if bareRoot {
135+
initBareRootApp(cfg)
136+
return nil
137+
}
138+
source := cfg.Sources["llm_endpoint"]
139+
if source == "" {
140+
source = "unknown"
141+
}
142+
return fmt.Errorf("llm_endpoint (%s): %w\nFix with: basecamp config unset llm_endpoint", source, err)
143+
}
129144
}
130145

131146
// Resolve behavior preferences: explicit flag > config > version.IsDev()

internal/completion/cache.go

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,8 @@ import (
1010
"path/filepath"
1111
"sync"
1212
"time"
13+
14+
"github.com/basecamp/basecamp-cli/internal/config"
1315
)
1416

1517
// CachedProject holds project data for tab completion.
@@ -366,13 +368,21 @@ func loadConfigForCompletion() *configForCompletion {
366368
loadProfilesFromFile(cfg, globalPath)
367369
}
368370

371+
// profiles is an authority key (it can redirect authenticated traffic), so
372+
// repo/local configs must be explicitly trusted before their profiles feed
373+
// completion — mirroring config.loadFromFile's trust gating. System/global
374+
// configs above are trusted by location.
375+
trust := config.LoadTrustStore(config.GlobalConfigDir())
376+
369377
// Repo config (walk up to find .git, then .basecamp/config.json)
370378
if dir, err := os.Getwd(); err == nil {
371379
for {
372380
gitPath := filepath.Join(dir, ".git")
373381
if fi, err := os.Stat(gitPath); err == nil && fi.IsDir() {
374382
repoConfig := filepath.Join(dir, ".basecamp", "config.json")
375-
loadProfilesFromFile(cfg, repoConfig)
383+
if trust != nil && trust.IsTrusted(repoConfig) {
384+
loadProfilesFromFile(cfg, repoConfig)
385+
}
376386
break
377387
}
378388
parent := filepath.Dir(dir)
@@ -385,7 +395,9 @@ func loadConfigForCompletion() *configForCompletion {
385395

386396
// Local config
387397
localPath := filepath.Join(".basecamp", "config.json")
388-
loadProfilesFromFile(cfg, localPath)
398+
if trust != nil && trust.IsTrusted(localPath) {
399+
loadProfilesFromFile(cfg, localPath)
400+
}
389401

390402
return cfg
391403
}

internal/completion/completer.go

Lines changed: 17 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -91,7 +91,7 @@ func (c *Completer) ProjectCompletion() cobra.CompletionFunc {
9191
// Use ID as completion value with name as description
9292
completion := cobra.CompletionWithDesc(
9393
fmt.Sprintf("%d", p.ID),
94-
p.Name,
94+
sanitizeCompletionDesc(p.Name),
9595
)
9696
completions = append(completions, completion)
9797
}
@@ -164,7 +164,7 @@ func (c *Completer) PeopleCompletion() cobra.CompletionFunc {
164164
}
165165
completion := cobra.CompletionWithDesc(
166166
fmt.Sprintf("%d", p.ID),
167-
desc,
167+
sanitizeCompletionDesc(desc),
168168
)
169169
completions = append(completions, completion)
170170
}
@@ -237,7 +237,7 @@ func (c *Completer) AccountCompletion() cobra.CompletionFunc {
237237
strings.HasPrefix(nameLower, toCompleteLower) ||
238238
strings.Contains(nameLower, toCompleteLower) {
239239
// Use ID as completion value with name as description
240-
completions = append(completions, cobra.CompletionWithDesc(idStr, a.Name))
240+
completions = append(completions, cobra.CompletionWithDesc(idStr, sanitizeCompletionDesc(a.Name)))
241241
}
242242
}
243243

@@ -270,14 +270,27 @@ func (c *Completer) ProfileCompletion() cobra.CompletionFunc {
270270
if strings.HasPrefix(nameLower, toCompleteLower) ||
271271
strings.Contains(nameLower, toCompleteLower) {
272272
// Use name as completion value with base URL as description
273-
completions = append(completions, cobra.CompletionWithDesc(p.Name, p.BaseURL))
273+
completions = append(completions, cobra.CompletionWithDesc(p.Name, sanitizeCompletionDesc(p.BaseURL)))
274274
}
275275
}
276276

277277
return completions, cobra.ShellCompDirectiveNoFileComp
278278
}
279279
}
280280

281+
// sanitizeCompletionDesc drops control characters (including ESC) from a
282+
// completion description. Descriptions can carry API- or config-controlled
283+
// strings (project/person/account names, profile base_url) which the shell
284+
// renders to the terminal; stripping control bytes prevents terminal injection.
285+
func sanitizeCompletionDesc(s string) string {
286+
return strings.Map(func(r rune) rune {
287+
if r < 0x20 || r == 0x7f {
288+
return -1
289+
}
290+
return r
291+
}, s)
292+
}
293+
281294
// rankProjects returns projects sorted by priority:
282295
// 1. HQ (purpose="hq")
283296
// 2. Bookmarked

internal/config/config.go

Lines changed: 48 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ package config
44
import (
55
"encoding/json"
66
"fmt"
7+
"net/url"
78
"os"
89
"path/filepath"
910
"strings"
@@ -196,12 +197,24 @@ func loadFromFile(cfg *Config, path string, source Source, trust *TrustStore) {
196197
cfg.Sources["scope"] = string(source)
197198
}
198199
if v, ok := fileCfg["cache_dir"].(string); ok && v != "" {
199-
cfg.CacheDir = v
200-
cfg.Sources["cache_dir"] = string(source)
200+
// cache_dir redirects every cache write (completion, resilience, TUI
201+
// workspace, recents, traces). An untrusted local/repo config could
202+
// point it at any user-writable path, so gate it like other authority
203+
// keys. filepath.Clean normalizes the accepted value.
204+
if untrusted {
205+
fmt.Fprintf(os.Stderr, "warning: ignoring cache_dir %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))
206+
} else {
207+
cfg.CacheDir = filepath.Clean(v)
208+
cfg.Sources["cache_dir"] = string(source)
209+
}
201210
}
202211
if v, ok := fileCfg["cache_enabled"].(bool); ok {
203-
cfg.CacheEnabled = v
204-
cfg.Sources["cache_enabled"] = string(source)
212+
if untrusted {
213+
fmt.Fprintf(os.Stderr, "warning: ignoring cache_enabled from %s config at %s\n (authority key from local/repo config; run `basecamp config trust %s` to allow)\n", source, path, ShellQuote(path))
214+
} else {
215+
cfg.CacheEnabled = v
216+
cfg.Sources["cache_enabled"] = string(source)
217+
}
205218
}
206219
if v, ok := fileCfg["format"].(string); ok && v != "" {
207220
cfg.Format = v
@@ -237,8 +250,14 @@ func loadFromFile(cfg *Config, path string, source Source, trust *TrustStore) {
237250
}
238251
}
239252
if v, ok := fileCfg["llm_model"].(string); ok && v != "" {
240-
cfg.LLMModel = v
241-
cfg.Sources["llm_model"] = string(source)
253+
// Gate like other LLM authority keys: an untrusted config could
254+
// silently substitute a costlier paid model.
255+
if untrusted {
256+
fmt.Fprintf(os.Stderr, "warning: ignoring llm_model %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))
257+
} else {
258+
cfg.LLMModel = v
259+
cfg.Sources["llm_model"] = string(source)
260+
}
242261
}
243262
if v, ok := fileCfg["llm_api_key"].(string); ok && v != "" {
244263
// Secret: only from global/system config, never local/repo
@@ -252,6 +271,10 @@ func loadFromFile(cfg *Config, path string, source Source, trust *TrustStore) {
252271
if v, ok := fileCfg["llm_endpoint"].(string); ok && v != "" {
253272
if untrusted {
254273
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))
274+
} else if !isHTTPScheme(v) {
275+
// Reject non-http(s) schemes (file://, etc.). https enforcement for
276+
// non-localhost endpoints happens later in root.go via RequireSecureURL.
277+
fmt.Fprintf(os.Stderr, "warning: ignoring llm_endpoint %q from %s config at %s (scheme must be http or https)\n", v, source, path)
255278
} else {
256279
cfg.LLMEndpoint = v
257280
cfg.Sources["llm_endpoint"] = string(source)
@@ -260,7 +283,11 @@ func loadFromFile(cfg *Config, path string, source Source, trust *TrustStore) {
260283
if v, ok := fileCfg["llm_max_concurrent"]; ok {
261284
if fv, ok := v.(float64); ok {
262285
iv := int(fv)
263-
if iv >= 1 && iv <= 10 && fv == float64(iv) {
286+
// Gate like other LLM authority keys: block a malicious repo from
287+
// inflating paid-LLM concurrency (cost amplification).
288+
if untrusted {
289+
fmt.Fprintf(os.Stderr, "warning: ignoring llm_max_concurrent from %s config at %s\n (authority key from local/repo config; run `basecamp config trust %s` to allow)\n", source, path, ShellQuote(path))
290+
} else if iv >= 1 && iv <= 10 && fv == float64(iv) {
264291
cfg.LLMMaxConcurrent = iv
265292
cfg.Sources["llm_max_concurrent"] = string(source)
266293
}
@@ -269,7 +296,10 @@ func loadFromFile(cfg *Config, path string, source Source, trust *TrustStore) {
269296
if v, ok := fileCfg["llm_token_budget"]; ok {
270297
if fv, ok := v.(float64); ok {
271298
iv := int(fv)
272-
if iv >= 100 && iv <= 100000 && fv == float64(iv) {
299+
// Gate like other LLM authority keys (cost amplification).
300+
if untrusted {
301+
fmt.Fprintf(os.Stderr, "warning: ignoring llm_token_budget from %s config at %s\n (authority key from local/repo config; run `basecamp config trust %s` to allow)\n", source, path, ShellQuote(path))
302+
} else if iv >= 100 && iv <= 100000 && fv == float64(iv) {
273303
cfg.LLMTokenBudget = iv
274304
cfg.Sources["llm_token_budget"] = string(source)
275305
}
@@ -660,6 +690,16 @@ func NormalizeBaseURL(url string) string {
660690
return strings.TrimSuffix(url, "/")
661691
}
662692

693+
// isHTTPScheme reports whether rawURL parses with an http or https scheme.
694+
// Used to reject non-web schemes (file://, etc.) for llm_endpoint.
695+
func isHTTPScheme(rawURL string) bool {
696+
u, err := url.Parse(rawURL)
697+
if err != nil {
698+
return false
699+
}
700+
return u.Scheme == "http" || u.Scheme == "https"
701+
}
702+
663703
// ShellQuote returns a POSIX single-quoted string safe for copy-paste into
664704
// a shell. Single quotes inside the value are escaped as '\” (end quote,
665705
// escaped literal quote, resume quote).

internal/config/config_test.go

Lines changed: 131 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -781,6 +781,137 @@ func TestLoadFromFile_BaseURLRejectedFromLocal(t *testing.T) {
781781
assert.Equal(t, "https://3.basecampapi.com", cfg.BaseURL, "local config should not override base_url")
782782
}
783783

784+
// TestLoadFromFile_AuthorityKeysRejectedFromLocal verifies the sibling
785+
// authority keys closed by the security audit (cache_dir, cache_enabled,
786+
// llm_model, llm_max_concurrent, llm_token_budget) are ignored — with a
787+
// warning — when they appear in an untrusted local/repo config, while their
788+
// default values are preserved.
789+
func TestLoadFromFile_AuthorityKeysRejectedFromLocal(t *testing.T) {
790+
defaults := Default()
791+
792+
cases := []struct {
793+
name string
794+
json string
795+
warnFrag string
796+
assertDef func(t *testing.T, cfg *Config)
797+
}{
798+
{
799+
name: "cache_dir",
800+
json: `{"cache_dir":"/home/victim/.ssh"}`,
801+
warnFrag: "ignoring cache_dir",
802+
assertDef: func(t *testing.T, cfg *Config) {
803+
assert.Equal(t, defaults.CacheDir, cfg.CacheDir)
804+
assert.Empty(t, cfg.Sources["cache_dir"])
805+
},
806+
},
807+
{
808+
name: "cache_enabled",
809+
json: `{"cache_enabled":false}`,
810+
warnFrag: "ignoring cache_enabled",
811+
assertDef: func(t *testing.T, cfg *Config) {
812+
assert.Equal(t, defaults.CacheEnabled, cfg.CacheEnabled)
813+
assert.Empty(t, cfg.Sources["cache_enabled"])
814+
},
815+
},
816+
{
817+
name: "llm_model",
818+
json: `{"llm_model":"gpt-4-turbo-expensive"}`,
819+
warnFrag: "ignoring llm_model",
820+
assertDef: func(t *testing.T, cfg *Config) {
821+
assert.Equal(t, defaults.LLMModel, cfg.LLMModel)
822+
assert.Empty(t, cfg.Sources["llm_model"])
823+
},
824+
},
825+
{
826+
name: "llm_max_concurrent",
827+
json: `{"llm_max_concurrent":10}`,
828+
warnFrag: "ignoring llm_max_concurrent",
829+
assertDef: func(t *testing.T, cfg *Config) {
830+
assert.Equal(t, defaults.LLMMaxConcurrent, cfg.LLMMaxConcurrent)
831+
assert.Empty(t, cfg.Sources["llm_max_concurrent"])
832+
},
833+
},
834+
{
835+
name: "llm_token_budget",
836+
json: `{"llm_token_budget":100000}`,
837+
warnFrag: "ignoring llm_token_budget",
838+
assertDef: func(t *testing.T, cfg *Config) {
839+
assert.Equal(t, defaults.LLMTokenBudget, cfg.LLMTokenBudget)
840+
assert.Empty(t, cfg.Sources["llm_token_budget"])
841+
},
842+
},
843+
}
844+
845+
for _, tc := range cases {
846+
t.Run(tc.name, func(t *testing.T) {
847+
tmpDir := t.TempDir()
848+
configPath := filepath.Join(tmpDir, "config.json")
849+
require.NoError(t, os.WriteFile(configPath, []byte(tc.json), 0644))
850+
851+
origStderr := os.Stderr
852+
r, w, _ := os.Pipe()
853+
os.Stderr = w
854+
855+
cfg := Default()
856+
loadFromFile(cfg, configPath, SourceLocal, nil) // nil trust → untrusted
857+
858+
w.Close()
859+
var buf [1024]byte
860+
n, _ := r.Read(buf[:])
861+
os.Stderr = origStderr
862+
863+
assert.Contains(t, string(buf[:n]), tc.warnFrag)
864+
tc.assertDef(t, cfg)
865+
})
866+
}
867+
}
868+
869+
// TestLoadFromFile_AuthorityKeysAcceptedFromGlobal confirms the same keys are
870+
// honored from a trusted (global) source, so the gating doesn't over-reach.
871+
func TestLoadFromFile_AuthorityKeysAcceptedFromGlobal(t *testing.T) {
872+
tmpDir := t.TempDir()
873+
configPath := filepath.Join(tmpDir, "config.json")
874+
require.NoError(t, os.WriteFile(configPath, []byte(`{
875+
"cache_dir":"/var/cache/basecamp",
876+
"cache_enabled":false,
877+
"llm_model":"gpt-4",
878+
"llm_max_concurrent":7,
879+
"llm_token_budget":5000
880+
}`), 0644))
881+
882+
cfg := Default()
883+
loadFromFile(cfg, configPath, SourceGlobal, nil)
884+
885+
assert.Equal(t, "/var/cache/basecamp", cfg.CacheDir)
886+
assert.False(t, cfg.CacheEnabled)
887+
assert.Equal(t, "gpt-4", cfg.LLMModel)
888+
assert.Equal(t, 7, cfg.LLMMaxConcurrent)
889+
assert.Equal(t, 5000, cfg.LLMTokenBudget)
890+
}
891+
892+
// TestLoadFromFile_LLMEndpointSchemeRejected verifies a non-http(s) llm_endpoint
893+
// scheme (e.g. file://) is rejected even from a trusted source.
894+
func TestLoadFromFile_LLMEndpointSchemeRejected(t *testing.T) {
895+
tmpDir := t.TempDir()
896+
configPath := filepath.Join(tmpDir, "config.json")
897+
require.NoError(t, os.WriteFile(configPath, []byte(`{"llm_endpoint":"file:///etc/passwd"}`), 0644))
898+
899+
origStderr := os.Stderr
900+
r, w, _ := os.Pipe()
901+
os.Stderr = w
902+
903+
cfg := Default()
904+
loadFromFile(cfg, configPath, SourceGlobal, nil)
905+
906+
w.Close()
907+
var buf [1024]byte
908+
n, _ := r.Read(buf[:])
909+
os.Stderr = origStderr
910+
911+
assert.Contains(t, string(buf[:n]), "scheme must be http or https")
912+
assert.Empty(t, cfg.LLMEndpoint)
913+
}
914+
784915
func TestLoadFromFile_BaseURLNoWarningForGlobal(t *testing.T) {
785916
tmpDir := t.TempDir()
786917
configPath := filepath.Join(tmpDir, "config.json")

0 commit comments

Comments
 (0)