diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 5bebad38..0782308c 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -85,6 +85,55 @@ jobs: echo "$HELP" | grep -q "rules" rm -f ./opencodereview + # Runs the suite natively on Windows, which the cross-compile job below cannot + # do: it only proves the windows arms of the build-tag splits compile. GitHub + # does not support `container:` on Windows runners + # (actions/runner#904), so this job installs Go directly instead of reusing the + # golang:1.26.5 image the other jobs share. + windows: + runs-on: windows-latest + timeout-minutes: 20 + steps: + - uses: actions/checkout@v7 + + - uses: actions/setup-go@v7 + with: + go-version: '1.26.5' + cache: true + + - name: Vet + run: go vet ./... + + # No -race here: the race detector needs a working C toolchain on Windows, + # and races are OS-independent, so the Linux job above already covers them. + # This job is here for the OS-specific behavior instead. No coverage gate + # either -- the //go:build !windows test files legitimately drop the total + # below the 80% the Linux job enforces. + - name: Test + run: go test -count=1 ./... + + - name: Build + run: go build -o opencodereview.exe ./cmd/opencodereview + + # Same assertions as the Linux smoke test, under git-bash so the script is + # shared verbatim rather than reimplemented in PowerShell. + - name: Smoke test + shell: bash + run: | + ./opencodereview.exe --version + ./opencodereview.exe --version | grep -q "open-code-review" + HELP=$(./opencodereview.exe --help) + echo "$HELP" | grep -q "Commands:" + echo "$HELP" | grep -q "review" + echo "$HELP" | grep -q "scan" + echo "$HELP" | grep -q "delegate" + echo "$HELP" | grep -q "config" + echo "$HELP" | grep -q "llm" + echo "$HELP" | grep -q "viewer" + echo "$HELP" | grep -q "session" + echo "$HELP" | grep -q "rules" + rm -f ./opencodereview.exe + cross-compile: runs-on: self-hosted timeout-minutes: 10 diff --git a/cmd/opencodereview/background_file_test.go b/cmd/opencodereview/background_file_test.go index 827e52d7..39de56a8 100644 --- a/cmd/opencodereview/background_file_test.go +++ b/cmd/opencodereview/background_file_test.go @@ -4,6 +4,7 @@ import ( "os" "os/exec" "path/filepath" + "runtime" "strings" "testing" ) @@ -37,7 +38,14 @@ func TestResolveBackgroundFilePath(t *testing.T) { }) t.Run("absolute unchanged", func(t *testing.T) { + // FromSlash is not enough on its own: it only swaps separators, and + // `\etc\context.md` is rooted but not absolute on Windows, where + // filepath.IsAbs wants a volume. Without the drive letter this case + // exercised the relative branch instead of the one it names. abs := filepath.FromSlash("/etc/context.md") + if runtime.GOOS == "windows" { + abs = `C:\etc\context.md` + } if got := resolveBackgroundFilePath(repo, abs); got != abs { t.Errorf("resolveBackgroundFilePath = %q, want %q (absolute must be untouched)", got, abs) } diff --git a/cmd/opencodereview/config_cmd.go b/cmd/opencodereview/config_cmd.go index 64d19190..743ca94e 100644 --- a/cmd/opencodereview/config_cmd.go +++ b/cmd/opencodereview/config_cmd.go @@ -85,14 +85,22 @@ func runConfigSet(key, value string) error { } displayValue := value - normalizedKey := strings.ToLower(strings.ReplaceAll(key, "_", "")) - if strings.HasSuffix(normalizedKey, "apikey") || strings.HasSuffix(normalizedKey, "authtoken") { + if shouldMaskConfigValue(key) { displayValue = maskKey(value) } fmt.Printf("Set %s = %s\n", key, displayValue) return nil } +// shouldMaskConfigValue reports whether the echoed value of a config key holds a +// secret and must be masked. Matching on the normalized suffix covers both +// snake_case and Go field spellings of api_key/auth_token at any path depth, +// while the *_cmd variants stay unmasked: a command line is not a secret. +func shouldMaskConfigValue(key string) bool { + normalizedKey := strings.ToLower(strings.ReplaceAll(key, "_", "")) + return strings.HasSuffix(normalizedKey, "apikey") || strings.HasSuffix(normalizedKey, "authtoken") +} + func runConfigUnset(key string) error { parts := strings.SplitN(key, ".", 2) if len(parts) != 2 || parts[1] == "" { @@ -190,6 +198,7 @@ func deleteCustomProvider(cfg *Config, name string) (bool, error) { // ProviderEntry holds per-provider configuration in the providers map. type ProviderEntry struct { APIKey string `json:"api_key,omitempty"` + APIKeyCmd string `json:"api_key_cmd,omitempty"` // shell command whose stdout is the api key; used when api_key is empty URL string `json:"url,omitempty"` Protocol string `json:"protocol,omitempty"` Model string `json:"model,omitempty"` @@ -228,6 +237,7 @@ type Config struct { type LlmConfig struct { URL string `json:"url,omitempty"` AuthToken string `json:"auth_token,omitempty"` + AuthTokenCmd string `json:"auth_token_cmd,omitempty"` // shell command whose stdout is the auth token; used when auth_token is empty AuthHeader string `json:"auth_header,omitempty"` Model string `json:"model,omitempty"` Protocol string `json:"protocol,omitempty"` // canonical protocol name; takes priority over UseAnthropic @@ -333,6 +343,8 @@ func setConfigValue(cfg *Config, key, value string) error { cfg.Llm.URL = value case "llm.auth_token", "llm.AuthToken": cfg.Llm.AuthToken = value + case "llm.auth_token_cmd", "llm.AuthTokenCmd": + cfg.Llm.AuthTokenCmd = value case "llm.auth_header", "llm.AuthHeader": normalized, err := llm.NormalizeAuthHeader(value) if err != nil { @@ -407,7 +419,7 @@ func setConfigValue(cfg *Config, key, value string) error { } cfg.Llm.ExtraBody = m default: - return fmt.Errorf("unknown config key: %s\nSupported keys: provider, model, providers.., custom_providers.., mcp_servers.., llm.url, llm.auth_token, llm.auth_header, llm.model, llm.protocol, llm.use_anthropic, llm.extra_body, llm.extra_headers, language, telemetry.enabled, telemetry.exporter, telemetry.otlp_endpoint, telemetry.content_logging\nProvider fields: api_key, url, protocol, model, models, auth_header, extra_body, extra_headers\nProtocol values: anthropic, openai, openai-responses\nMCP server fields: type, command, args, env, url, headers, tools, setup", key) + return fmt.Errorf("unknown config key: %s\nSupported keys: provider, model, providers.., custom_providers.., mcp_servers.., llm.url, llm.auth_token, llm.auth_token_cmd, llm.auth_header, llm.model, llm.protocol, llm.use_anthropic, llm.extra_body, llm.extra_headers, language, telemetry.enabled, telemetry.exporter, telemetry.otlp_endpoint, telemetry.content_logging\nProvider fields: api_key, api_key_cmd, url, protocol, model, models, auth_header, extra_body, extra_headers\nProtocol values: anthropic, openai, openai-responses\nMCP server fields: type, command, args, env, url, headers, tools, setup", key) } return nil } @@ -416,6 +428,8 @@ func applyProviderField(entry *ProviderEntry, field, key, value string) error { switch field { case "api_key": entry.APIKey = value + case "api_key_cmd": + entry.APIKeyCmd = value case "url": entry.URL = value case "protocol": @@ -451,7 +465,7 @@ func applyProviderField(entry *ProviderEntry, field, key, value string) error { } entry.ExtraHeaders = parsed default: - return fmt.Errorf("unknown provider field %q: supported fields are api_key, url, protocol, model, models, auth_header, extra_body, extra_headers", field) + return fmt.Errorf("unknown provider field %q: supported fields are api_key, api_key_cmd, url, protocol, model, models, auth_header, extra_body, extra_headers", field) } return nil } diff --git a/cmd/opencodereview/config_cmd_test.go b/cmd/opencodereview/config_cmd_test.go index 1cba3f99..155ff6c2 100644 --- a/cmd/opencodereview/config_cmd_test.go +++ b/cmd/opencodereview/config_cmd_test.go @@ -87,6 +87,56 @@ func TestSetConfigValueProviderEntry(t *testing.T) { } } +func TestSetConfigValueKeyCmdFields(t *testing.T) { + // A typo in any of these case labels would silently degrade to "unknown + // provider field" / "unknown config key", so assert the field each key writes. + const value = "op read op://dev/anthropic/api-key" + tests := []struct { + name string + key string + got func(cfg *Config) string + }{ + {"preset provider api_key_cmd", "providers.anthropic.api_key_cmd", func(cfg *Config) string { return cfg.Providers["anthropic"].APIKeyCmd }}, + {"custom provider api_key_cmd", "custom_providers.my-gateway.api_key_cmd", func(cfg *Config) string { return cfg.CustomProviders["my-gateway"].APIKeyCmd }}, + {"llm auth_token_cmd", "llm.auth_token_cmd", func(cfg *Config) string { return cfg.Llm.AuthTokenCmd }}, + {"llm AuthTokenCmd alias", "llm.AuthTokenCmd", func(cfg *Config) string { return cfg.Llm.AuthTokenCmd }}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + cfg := &Config{} + if err := setConfigValue(cfg, tt.key, value); err != nil { + t.Fatalf("setConfigValue %s: %v", tt.key, err) + } + if got := tt.got(cfg); got != value { + t.Errorf("%s = %q, want %q", tt.key, got, value) + } + }) + } +} + +func TestShouldMaskConfigValue(t *testing.T) { + // api_key/auth_token values are secrets; the *_cmd variants are command + // lines, so they print unmasked. + tests := []struct { + key string + want bool + }{ + {"llm.auth_token", true}, + {"llm.auth_token_cmd", false}, + {"providers.x.api_key", true}, + {"providers.x.api_key_cmd", false}, + {"providers.x.APIKeyCmd", false}, + {"llm.AuthToken", true}, + } + for _, tt := range tests { + t.Run(tt.key, func(t *testing.T) { + if got := shouldMaskConfigValue(tt.key); got != tt.want { + t.Errorf("shouldMaskConfigValue(%q) = %v, want %v", tt.key, got, tt.want) + } + }) + } +} + func TestSetConfigValueProviderEntryNonPresetWritesCustomProvider(t *testing.T) { cfg := &Config{} diff --git a/cmd/opencodereview/flags.go b/cmd/opencodereview/flags.go index 18250ed7..f1fe56a5 100644 --- a/cmd/opencodereview/flags.go +++ b/cmd/opencodereview/flags.go @@ -351,8 +351,8 @@ Examples: ocr config set language English ocr config set telemetry.enabled true -Supported keys: provider, model, providers.., custom_providers.., mcp_servers.., llm.url, llm.auth_token, llm.auth_header, llm.model, llm.protocol, llm.use_anthropic, llm.extra_body, llm.extra_headers, language, telemetry.enabled, telemetry.exporter, telemetry.otlp_endpoint, telemetry.content_logging -Provider fields: api_key, url, protocol, model, models, auth_header, extra_body, extra_headers +Supported keys: provider, model, providers.., custom_providers.., mcp_servers.., llm.url, llm.auth_token, llm.auth_token_cmd, llm.auth_header, llm.model, llm.protocol, llm.use_anthropic, llm.extra_body, llm.extra_headers, language, telemetry.enabled, telemetry.exporter, telemetry.otlp_endpoint, telemetry.content_logging +Provider fields: api_key, api_key_cmd, url, protocol, model, models, auth_header, extra_body, extra_headers Protocol values: anthropic, openai, openai-responses MCP server fields: type, command, args, env, url, headers, tools, setup`) } diff --git a/cmd/opencodereview/flags_test.go b/cmd/opencodereview/flags_test.go index 55a8b9df..5bee8879 100644 --- a/cmd/opencodereview/flags_test.go +++ b/cmd/opencodereview/flags_test.go @@ -1,6 +1,8 @@ package main import ( + "slices" + "strings" "testing" "time" ) @@ -226,6 +228,58 @@ func TestPrintDefaults(t *testing.T) { fs.PrintDefaults() } +// configFieldList returns the comma-separated names that follow prefix on the +// one line of text starting with it. +func configFieldList(t *testing.T, text, prefix string) []string { + t.Helper() + for _, line := range strings.Split(text, "\n") { + if !strings.HasPrefix(line, prefix) { + continue + } + var out []string + for _, field := range strings.Split(strings.TrimPrefix(line, prefix), ",") { + if field = strings.TrimSpace(field); field != "" { + out = append(out, field) + } + } + return out + } + t.Fatalf("no line starting with %q in:\n%s", prefix, text) + return nil +} + +// These four lists are duplicated verbatim in printConfigUsage (what `ocr config` +// and `ocr config --help` print) and in setConfigValue's unknown-key error. +// api_key_cmd and llm.auth_token_cmd were added to the second copy and missed in +// the first, so the primary discovery surface silently disagreed with the code. +// Compared in order, since both copies are meant to be identical text. +func TestPrintConfigUsage_ListsMatchSetConfigValueError(t *testing.T) { + usage := captureStdout(t, printConfigUsage) + + err := setConfigValue(&Config{}, "definitely.not.a.key", "") + if err == nil { + t.Fatal("setConfigValue should reject an unknown key") + } + canonical := err.Error() + + prefixes := []string{ + "Supported keys: ", + "Provider fields: ", + "Protocol values: ", + "MCP server fields: ", + } + for _, prefix := range prefixes { + t.Run(strings.TrimSuffix(prefix, ": "), func(t *testing.T) { + want := configFieldList(t, canonical, prefix) + got := configFieldList(t, usage, prefix) + if !slices.Equal(got, want) { + t.Errorf("%q drifted between flags.go and config_cmd.go\n flags.go: %v\n config_cmd.go: %v", + prefix, got, want) + } + }) + } +} + func TestExpandShortFlags(t *testing.T) { m := map[string]string{"c": "commit", "f": "format"} tests := []struct { diff --git a/cmd/opencodereview/provider_cmd.go b/cmd/opencodereview/provider_cmd.go index f67da930..2a83abb3 100644 --- a/cmd/opencodereview/provider_cmd.go +++ b/cmd/opencodereview/provider_cmd.go @@ -235,13 +235,16 @@ func applyOfficialProviderConfig(configPath string, cfg *Config, result provider preset, isPreset := llm.LookupProvider(result.provider) - if result.apiKey == "" { + // Mirror the resolver's precedence (static api_key -> api_key_cmd -> env var): + // an already-configured api_key_cmd satisfies the requirement, so picking a + // model for such a provider must not fail and abandon the save. + if result.apiKey == "" && cfg.Providers[result.provider].APIKeyCmd == "" { if isPreset && preset.EnvVar != "" { if os.Getenv(preset.EnvVar) == "" { - return fmt.Errorf("API key is required for provider %s (configure it or set $%s)", result.provider, preset.EnvVar) + return fmt.Errorf("API key is required for provider %s (configure it, set providers.%s.api_key_cmd, or set $%s)", result.provider, result.provider, preset.EnvVar) } } else { - return fmt.Errorf("API key is required for provider %s", result.provider) + return fmt.Errorf("API key is required for provider %s (configure it or set providers.%s.api_key_cmd)", result.provider, result.provider) } } @@ -257,7 +260,8 @@ func applyOfficialProviderConfig(configPath string, cfg *Config, result provider if result.apiKey != "" { entry.APIKey = result.apiKey } else { - // Confirmed empty key: clear saved api_key so resolver falls back to $ENV_VAR. + // Confirmed empty key: clear saved api_key so the resolver falls back to + // api_key_cmd (when set) or $ENV_VAR. entry.APIKey = "" } cfg.Providers[result.provider] = entry diff --git a/cmd/opencodereview/provider_cmd_test.go b/cmd/opencodereview/provider_cmd_test.go index d1f03bba..a14a7d82 100644 --- a/cmd/opencodereview/provider_cmd_test.go +++ b/cmd/opencodereview/provider_cmd_test.go @@ -5,9 +5,37 @@ import ( "io" "os" "path/filepath" + "runtime" "testing" ) +// isolateLLMConnectionTest keeps the "Testing connection..." step that ends +// every apply*Config call away from the developer's own machine. Without it +// resolveConfigPath() falls back to ~/.opencodereview/config.json and `go test` +// resolves a real endpoint: with providers..api_key_cmd configured that +// runs the credential helper and blocks on a pinentry/Touch ID prompt for up to +// the 60s credential timeout, and with a static key it fires a real request. +// +// The path points at a file that does not exist, so resolution fails fast the +// way it already does on a machine with no config. HOME is redirected into an +// empty temp dir as well, so the shell-rc strategy has nothing to read either. +func isolateLLMConnectionTest(t *testing.T) { + t.Helper() + dir := t.TempDir() + t.Setenv("OCR_CONFIG_PATH", filepath.Join(dir, "no-such-config.json")) + // Both, because os.UserHomeDir reads USERPROFILE on Windows and never falls + // back to HOME -- setting HOME alone would leave the shell-rc strategy reading + // the real profile. + t.Setenv("HOME", dir) + t.Setenv("USERPROFILE", dir) + for _, k := range []string{ + "OCR_LLM_URL", "OCR_LLM_TOKEN", "OCR_LLM_MODEL", + "ANTHROPIC_BASE_URL", "ANTHROPIC_AUTH_TOKEN", "ANTHROPIC_MODEL", + } { + t.Setenv(k, "") + } +} + func TestMaskKey(t *testing.T) { tests := []struct { name string @@ -47,8 +75,12 @@ func TestSaveConfig(t *testing.T) { if err != nil { t.Fatalf("stat: %v", err) } - if perm := info.Mode().Perm(); perm != 0o600 { - t.Errorf("perm = %o, want 600", perm) + // Windows reports 0666 regardless of the mode passed to OpenFile, so only the + // unix arms can assert the 0600 the config file is written with. + if runtime.GOOS != "windows" { + if perm := info.Mode().Perm(); perm != 0o600 { + t.Errorf("perm = %o, want 600", perm) + } } data, err := os.ReadFile(path) @@ -206,6 +238,7 @@ func TestApplyOfficialProviderConfig_MissingFields(t *testing.T) { } func TestApplyOfficialProviderConfig_EmptyKeyClearsSavedAPIKey(t *testing.T) { + isolateLLMConnectionTest(t) t.Setenv("DEEPSEEK_API_KEY", "sk-from-env") dir := t.TempDir() configPath := filepath.Join(dir, "config.json") @@ -240,7 +273,41 @@ func TestApplyOfficialProviderConfig_EmptyKeyClearsSavedAPIKey(t *testing.T) { } } +// A provider configured with only api_key_cmd must survive a trip through the +// TUI: picking a model returns an empty apiKey, which must not be mistaken for +// "no credential" and abandon the save. +func TestApplyOfficialProviderConfig_APIKeyCmdSatisfiesRequirement(t *testing.T) { + isolateLLMConnectionTest(t) + t.Setenv("DEEPSEEK_API_KEY", "") + configPath := filepath.Join(t.TempDir(), "config.json") + cfg := &Config{ + Providers: map[string]ProviderEntry{ + "deepseek": {APIKeyCmd: "op read op://dev/deepseek/api-key"}, + }, + } + + err := applyOfficialProviderConfig(configPath, cfg, providerTUIResult{ + provider: "deepseek", + model: "deepseek-v4-flash", + apiKey: "", + }) + if err != nil { + t.Fatalf("api_key_cmd should satisfy the API key requirement: %v", err) + } + diskCfg, err := loadOrCreateConfig(configPath) + if err != nil { + t.Fatalf("load config: %v", err) + } + if diskCfg.Provider != "deepseek" || diskCfg.Model != "deepseek-v4-flash" { + t.Errorf("save was abandoned: provider=%q model=%q", diskCfg.Provider, diskCfg.Model) + } + if got := diskCfg.Providers["deepseek"].APIKeyCmd; got != "op read op://dev/deepseek/api-key" { + t.Errorf("persisted api_key_cmd = %q, want it preserved", got) + } +} + func TestApplyCustomProviderConfig_EmptyKeyClearsSavedAPIKey(t *testing.T) { + isolateLLMConnectionTest(t) dir := t.TempDir() configPath := filepath.Join(dir, "config.json") cfg := &Config{ @@ -300,6 +367,7 @@ func TestProviderTUIResult_ResolvedModel(t *testing.T) { } func TestApplyOfficialProviderConfig_UsesSessionModelPick(t *testing.T) { + isolateLLMConnectionTest(t) t.Setenv("QIANFAN_API_KEY", "sk-from-env") dir := t.TempDir() configPath := filepath.Join(dir, "config.json") diff --git a/cmd/opencodereview/provider_tui.go b/cmd/opencodereview/provider_tui.go index b2ba1d17..491e3c98 100644 --- a/cmd/opencodereview/provider_tui.go +++ b/cmd/opencodereview/provider_tui.go @@ -2,6 +2,7 @@ package main import ( "fmt" + "maps" "os" "sort" "strings" @@ -904,11 +905,36 @@ func officialProviderEnvKeySet(p llm.Provider) bool { return p.EnvVar != "" && os.Getenv(p.EnvVar) != "" } +// officialAPIKeyRequiredError mirrors the wording applyOfficialProviderConfig +// uses for the same failure, so the interactive and non-interactive paths name +// the same options in the same order (static key -> api_key_cmd -> env var). func officialAPIKeyRequiredError(p llm.Provider) string { + if p.Name == "" { + return "API key is required" + } if p.EnvVar != "" { - return fmt.Sprintf("API key is required (or set $%s)", p.EnvVar) + return fmt.Sprintf("API key is required (configure it, set providers.%s.api_key_cmd, or set $%s)", p.Name, p.EnvVar) } - return "API key is required" + return fmt.Sprintf("API key is required (configure it or set providers.%s.api_key_cmd)", p.Name) +} + +// apiKeyCmdForStep returns the api_key_cmd already configured for the provider +// the API-key step is editing, reading the same config entry loadExistingAPIKey +// reads the static key from. The step serves the Official and Custom tabs; the +// Manual tab has its own form and uses llm.auth_token_cmd instead. +func (m providerTUIModel) apiKeyCmdForStep() string { + switch m.activeTab { + case tabOfficial: + if m.existingCfg == nil { + return "" + } + return m.existingCfg.Providers[m.currentProvider().Name].APIKeyCmd + case tabCustom: + if cp, ok := m.selectedCustomProvider(); ok { + return m.customProviderEntry(cp.name, cp.entry).APIKeyCmd + } + } + return "" } func (m providerTUIModel) apiKeyStepCanConfirm() (ok bool, errMsg string) { @@ -918,6 +944,12 @@ func (m providerTUIModel) apiKeyStepCanConfirm() (ok bool, errMsg string) { if !m.apiKeyMasked && strings.TrimSpace(m.apiKeyInput.Value()) != "" { return true, "" } + // Resolver precedence is static key -> api_key_cmd -> env var, so an already + // configured command satisfies the requirement: the field renders blank for + // such a provider and must still be confirmable. + if m.apiKeyCmdForStep() != "" { + return true, "" + } if m.activeTab == tabOfficial { p := m.currentProvider() if officialProviderEnvKeySet(p) { @@ -925,6 +957,9 @@ func (m providerTUIModel) apiKeyStepCanConfirm() (ok bool, errMsg string) { } return false, officialAPIKeyRequiredError(p) } + if cp, ok := m.selectedCustomProvider(); ok && cp.name != "" { + return false, fmt.Sprintf("API key is required (configure it or set custom_providers.%s.api_key_cmd)", cp.name) + } return false, "API key is required" } @@ -1043,7 +1078,16 @@ func authHeaderFormError(raw string) string { ) } -const manualAuthTokenRequiredError = "Auth token is required (whitespace-only input is not accepted)" +const manualAuthTokenRequiredError = "Auth token is required (configure it or set llm.auth_token_cmd; whitespace-only input is not accepted)" + +// manualAuthTokenCmd returns the configured llm.auth_token_cmd, which the +// resolver runs when llm.auth_token is empty. +func (m providerTUIModel) manualAuthTokenCmd() string { + if m.existingCfg == nil { + return "" + } + return m.existingCfg.Llm.AuthTokenCmd +} func (m providerTUIModel) handleCustomFormEnter() (tea.Model, tea.Cmd) { switch m.cpStep { @@ -1179,22 +1223,20 @@ func (m providerTUIModel) applyCreateCustomProvider() (tea.Model, tea.Cmd) { // map cloning) can safely mutate the returned value without aliasing the // original's slice or map fields. func cloneProviderEntry(v ProviderEntry) ProviderEntry { - out := ProviderEntry{ + return ProviderEntry{ APIKey: v.APIKey, + APIKeyCmd: v.APIKeyCmd, URL: v.URL, Protocol: v.Protocol, Model: v.Model, Models: append([]string(nil), v.Models...), AuthHeader: v.AuthHeader, + TimeoutSec: v.TimeoutSec, + // Shallow copy only: nested maps/slices inside a value are not cloned. + // maps.Clone keeps a nil map nil, matching the field's omitempty. + ExtraBody: maps.Clone(v.ExtraBody), + ExtraHeaders: maps.Clone(v.ExtraHeaders), } - if v.ExtraBody != nil { - out.ExtraBody = make(map[string]any, len(v.ExtraBody)) - for k, val := range v.ExtraBody { - // Shallow copy only: nested maps/slices inside val are not cloned. - out.ExtraBody[k] = val - } - } - return out } func cloneCustomProvidersMap(src map[string]ProviderEntry) map[string]ProviderEntry { @@ -1606,7 +1648,9 @@ func (m providerTUIModel) handleManualFormEnter() (tea.Model, tea.Cmd) { m.manualStep = manualStepAuthToken return m, m.manualTokenInput.Focus() case manualStepAuthToken: - if strings.TrimSpace(m.manualTokenInput.Value()) == "" && m.manualTokenOriginal == "" { + // Same precedence as the provider tabs: an already configured + // llm.auth_token_cmd stands in for a typed or saved token. + if strings.TrimSpace(m.manualTokenInput.Value()) == "" && m.manualTokenOriginal == "" && m.manualAuthTokenCmd() == "" { m.formError = manualAuthTokenRequiredError return m, nil } @@ -1908,7 +1952,10 @@ func (m providerTUIModel) result() providerTUIResult { return providerTUIResult{} case tabManual: - apiKey := m.manualTokenInput.Value() + // Trim like the Official and Custom tabs: a whitespace-only token must + // never persist, or it wins precedence over a working auth_token_cmd + // and sends "Authorization: Bearer ". + apiKey := strings.TrimSpace(m.manualTokenInput.Value()) if m.manualTokenMasked || (apiKey == "" && m.manualTokenOriginal != "") { apiKey = m.manualTokenOriginal } diff --git a/cmd/opencodereview/provider_tui_funcs_test.go b/cmd/opencodereview/provider_tui_funcs_test.go index 1462b3a7..61765044 100644 --- a/cmd/opencodereview/provider_tui_funcs_test.go +++ b/cmd/opencodereview/provider_tui_funcs_test.go @@ -3,6 +3,7 @@ package main import ( "os" "path/filepath" + "reflect" "strings" "testing" @@ -198,18 +199,26 @@ func TestRenderListName_Inactive(t *testing.T) { func TestCloneProviderEntry_WithExtraBody(t *testing.T) { orig := ProviderEntry{ APIKey: "key", + APIKeyCmd: "op read op://dev/anthropic/api-key", URL: "http://localhost", Protocol: "openai", Model: "gpt-4", Models: []string{"gpt-4", "gpt-3.5"}, AuthHeader: "Authorization", + TimeoutSec: 45, ExtraBody: map[string]any{"temperature": 0.7, "stream": true}, + ExtraHeaders: map[string]string{ + "X-Trace": "on", + }, } clone := cloneProviderEntry(orig) if clone.APIKey != orig.APIKey || clone.URL != orig.URL || clone.Protocol != orig.Protocol { t.Error("basic fields not copied") } + if clone.APIKeyCmd != orig.APIKeyCmd { + t.Errorf("APIKeyCmd not copied: got %q, want %q", clone.APIKeyCmd, orig.APIKeyCmd) + } if len(clone.Models) != 2 || clone.Models[0] != "gpt-4" { t.Errorf("Models not cloned: %v", clone.Models) } @@ -229,6 +238,22 @@ func TestCloneProviderEntry_WithExtraBody(t *testing.T) { if len(orig.Models) != 2 { t.Error("modifying clone should not affect original Models") } + + if clone.TimeoutSec != orig.TimeoutSec { + t.Errorf("TimeoutSec not copied: got %d, want %d", clone.TimeoutSec, orig.TimeoutSec) + } + if clone.ExtraHeaders == nil { + // Fatal, not Error: writing to the nil map below would panic instead of + // reporting which field was dropped. + t.Fatal("ExtraHeaders should not be nil") + } + if clone.ExtraHeaders["X-Trace"] != "on" { + t.Errorf("ExtraHeaders not copied: %v", clone.ExtraHeaders) + } + clone.ExtraHeaders["X-New"] = "1" + if _, ok := orig.ExtraHeaders["X-New"]; ok { + t.Error("modifying clone should not affect original ExtraHeaders") + } } func TestCloneProviderEntry_NilExtraBody(t *testing.T) { @@ -240,6 +265,42 @@ func TestCloneProviderEntry_NilExtraBody(t *testing.T) { if clone.ExtraBody != nil { t.Error("ExtraBody should remain nil") } + if clone.ExtraHeaders != nil { + t.Error("ExtraHeaders should remain nil") + } +} + +// cloneProviderEntry lists fields by hand, which is how timeout_sec and +// extra_headers came to be silently dropped on the save-rollback paths. This +// fails when a field is added to ProviderEntry but not to the clone: the +// non-zero check forces the fixture to grow, and DeepEqual then catches the +// omission. It catches a dropped field, not an aliased one -- DeepEqual +// compares values, not identity; the sibling test above covers aliasing. +func TestCloneProviderEntry_CopiesEveryField(t *testing.T) { + orig := ProviderEntry{ + APIKey: "key", + APIKeyCmd: "op read op://dev/x/api-key", + URL: "http://localhost", + Protocol: "openai", + Model: "gpt-4", + Models: []string{"gpt-4"}, + AuthHeader: "Authorization", + TimeoutSec: 45, + ExtraBody: map[string]any{"temperature": 0.7}, + ExtraHeaders: map[string]string{"X-Trace": "on"}, + } + + rv := reflect.ValueOf(orig) + for i := range rv.NumField() { + if rv.Field(i).IsZero() { + t.Fatalf("fixture leaves %s zero-valued; set it so the clone is actually checked", + rv.Type().Field(i).Name) + } + } + + if clone := cloneProviderEntry(orig); !reflect.DeepEqual(clone, orig) { + t.Errorf("clone dropped a field:\n got %+v\nwant %+v", clone, orig) + } } func TestCustomListCount(t *testing.T) { @@ -1776,83 +1837,198 @@ func TestProviderTUI_ResultUsesSessionModelPickWhenSelectionEmpty(t *testing.T) } } -func TestApiKeyStepCanConfirm_OfficialEmptyWithoutEnv(t *testing.T) { - t.Setenv("DEEPSEEK_API_KEY", "") - cfg := &Config{ - Provider: "deepseek", - Model: "deepseek-v4-flash", - Providers: map[string]ProviderEntry{ - "deepseek": {Model: "deepseek-v4-flash"}, +// apiKeyStepCanConfirm gates the final Enter of `ocr config provider`. It has to +// mirror the resolver's precedence (static api_key -> api_key_cmd -> env var): +// a provider configured with only api_key_cmd renders a blank key field, and +// blocking it there made the feature unreachable from the documented wizard. +func TestApiKeyStepCanConfirm(t *testing.T) { + tests := []struct { + name string + env string + cfg *Config + customTab bool + typedKey string + wantOK bool + wantErrMsg string + }{ + { + name: "official saved api_key", + cfg: &Config{ + Provider: "deepseek", + Providers: map[string]ProviderEntry{"deepseek": {APIKey: "keep-me"}}, + }, + wantOK: true, }, - } - m := newProviderTUI(cfg, "") - m.activeTab = tabOfficial - m.step = stepAPIKey - - ok, errMsg := m.apiKeyStepCanConfirm() - if ok { - t.Fatal("expected confirmation to be blocked") - } - if errMsg != "API key is required (or set $DEEPSEEK_API_KEY)" { - t.Errorf("errMsg = %q", errMsg) - } -} - -func TestApiKeyStepCanConfirm_OfficialEmptyWithEnv(t *testing.T) { - t.Setenv("DEEPSEEK_API_KEY", "sk-from-env") - cfg := &Config{ - Provider: "deepseek", - Model: "deepseek-v4-flash", - Providers: map[string]ProviderEntry{ - "deepseek": {Model: "deepseek-v4-flash"}, + { + name: "official typed key", + cfg: &Config{Provider: "deepseek", Providers: map[string]ProviderEntry{"deepseek": {}}}, + typedKey: "sk-typed", + wantOK: true, }, - } - m := newProviderTUI(cfg, "") - m.activeTab = tabOfficial - m.step = stepAPIKey - - ok, errMsg := m.apiKeyStepCanConfirm() - if !ok { - t.Fatalf("expected confirmation allowed, errMsg = %q", errMsg) - } -} - -func TestApiKeyStepCanConfirm_CustomEmpty(t *testing.T) { - cfg := &Config{ - Provider: "stepfun", - CustomProviders: map[string]ProviderEntry{ - "stepfun": {APIKey: ""}, + { + name: "official api_key_cmd only", + cfg: &Config{ + Provider: "deepseek", + Providers: map[string]ProviderEntry{"deepseek": {APIKeyCmd: "op read op://dev/deepseek/api-key"}}, + }, + wantOK: true, + }, + { + name: "official nothing configured", + cfg: &Config{Provider: "deepseek", Providers: map[string]ProviderEntry{"deepseek": {}}}, + wantOK: false, + wantErrMsg: "API key is required (configure it, set providers.deepseek.api_key_cmd, or set $DEEPSEEK_API_KEY)", + }, + { + name: "official env var set", + env: "sk-from-env", + cfg: &Config{Provider: "deepseek", Providers: map[string]ProviderEntry{"deepseek": {}}}, + wantOK: true, + }, + { + name: "custom saved api_key", + customTab: true, + cfg: &Config{ + Provider: "stepfun", + CustomProviders: map[string]ProviderEntry{"stepfun": {APIKey: "sk-custom"}}, + }, + wantOK: true, + }, + { + name: "custom api_key_cmd only", + customTab: true, + cfg: &Config{ + Provider: "stepfun", + CustomProviders: map[string]ProviderEntry{"stepfun": {APIKeyCmd: "op read op://dev/stepfun/api-key"}}, + }, + wantOK: true, + }, + { + name: "custom nothing configured", + customTab: true, + cfg: &Config{Provider: "stepfun", CustomProviders: map[string]ProviderEntry{"stepfun": {}}}, + wantOK: false, + wantErrMsg: "API key is required (configure it or set custom_providers.stepfun.api_key_cmd)", }, } - m := newProviderTUI(cfg, "") - m.activeTab = tabCustom - m.customIdx = 0 - m.step = stepAPIKey - ok, errMsg := m.apiKeyStepCanConfirm() - if ok { - t.Fatal("expected confirmation to be blocked") - } - if errMsg != "API key is required" { - t.Errorf("errMsg = %q", errMsg) + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Setenv("DEEPSEEK_API_KEY", tc.env) + m := newProviderTUI(tc.cfg, "") + if tc.customTab { + m.activeTab = tabCustom + m.customIdx = 0 + } else { + m.activeTab = tabOfficial + } + m.step = stepAPIKey + // loadExistingAPIKey is what the wizard runs on entering the step, and + // is the only thing that populates apiKeyOriginal / the mask. + m.loadExistingAPIKey() + if tc.typedKey != "" { + m.apiKeyInput.SetValue(tc.typedKey) + } + + ok, errMsg := m.apiKeyStepCanConfirm() + if ok != tc.wantOK { + t.Fatalf("apiKeyStepCanConfirm() ok = %v, want %v (errMsg = %q)", ok, tc.wantOK, errMsg) + } + if errMsg != tc.wantErrMsg { + t.Errorf("errMsg = %q, want %q", errMsg, tc.wantErrMsg) + } + }) } } -func TestApiKeyStepCanConfirm_MaskedSavedKey(t *testing.T) { - cfg := &Config{ - Provider: "deepseek", - Providers: map[string]ProviderEntry{ - "deepseek": {APIKey: "keep-me"}, +// The Manual tab's auth-token gate is the legacy twin of apiKeyStepCanConfirm: +// llm.auth_token_cmd has to stand in for an empty field the same way. +func TestHandleManualFormEnter_AuthTokenGate(t *testing.T) { + tests := []struct { + name string + llmCfg LlmConfig + typedToken string + wantAdvance bool + // wantAPIKey is the token result() must persist once the step confirms. + wantAPIKey string + }{ + { + name: "saved auth_token", + llmCfg: LlmConfig{URL: "http://existing", Model: "m", AuthToken: "tok-saved"}, + wantAdvance: true, + wantAPIKey: "tok-saved", + }, + { + name: "typed token", + llmCfg: LlmConfig{URL: "http://existing", Model: "m"}, + typedToken: "tok-typed", + wantAdvance: true, + wantAPIKey: "tok-typed", + }, + { + name: "auth_token_cmd only", + llmCfg: LlmConfig{URL: "http://existing", Model: "m", AuthTokenCmd: "op read op://dev/gw/token"}, + wantAdvance: true, + }, + { + // auth_token_cmd opens the gate, so whitespace typed at this step + // confirms. It must not be saved as auth_token: a non-empty token + // wins precedence and would silently shadow the working command. + name: "auth_token_cmd with whitespace-only token", + llmCfg: LlmConfig{URL: "http://existing", Model: "m", AuthTokenCmd: "op read op://dev/gw/token"}, + typedToken: " ", + wantAdvance: true, + }, + { + name: "nothing configured", + llmCfg: LlmConfig{URL: "http://existing", Model: "m"}, + wantAdvance: false, + }, + { + name: "whitespace-only token", + llmCfg: LlmConfig{URL: "http://existing", Model: "m"}, + typedToken: " ", + wantAdvance: false, }, } - m := newProviderTUI(cfg, "") - m.activeTab = tabOfficial - m.step = stepAPIKey - m.loadExistingAPIKey() - ok, errMsg := m.apiKeyStepCanConfirm() - if !ok { - t.Fatalf("expected confirmation allowed, errMsg = %q", errMsg) + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + m := newProviderTUI(&Config{Llm: tc.llmCfg}, "") + m.activeTab = tabManual + m.inManualForm = true + m.manualStep = manualStepAuthToken + if tc.typedToken != "" { + m.manualTokenInput.SetValue(tc.typedToken) + } + + result, _ := m.handleManualFormEnter() + m2 := result.(providerTUIModel) + + if tc.wantAdvance { + if m2.manualStep != manualStepAuthHeader { + t.Fatalf("manualStep = %d, want manualStepAuthHeader (%d); formError = %q", + m2.manualStep, manualStepAuthHeader, m2.formError) + } + if m2.formError != "" { + t.Errorf("formError = %q, want empty", m2.formError) + } + if got := m2.result().apiKey; got != tc.wantAPIKey { + t.Errorf("result().apiKey = %q, want %q", got, tc.wantAPIKey) + } + return + } + if m2.manualStep != manualStepAuthToken { + t.Fatalf("manualStep = %d, want to stay on manualStepAuthToken (%d)", + m2.manualStep, manualStepAuthToken) + } + if m2.formError != manualAuthTokenRequiredError { + t.Errorf("formError = %q, want %q", m2.formError, manualAuthTokenRequiredError) + } + if !strings.Contains(m2.formError, "llm.auth_token_cmd") { + t.Errorf("formError should name llm.auth_token_cmd, got %q", m2.formError) + } + }) } } diff --git a/cmd/opencodereview/provider_tui_test.go b/cmd/opencodereview/provider_tui_test.go index 69d41821..ba52b0e9 100644 --- a/cmd/opencodereview/provider_tui_test.go +++ b/cmd/opencodereview/provider_tui_test.go @@ -2291,8 +2291,10 @@ func TestProviderTUI_OfficialApiKeyEmptyWithoutEnvBlocksEnter(t *testing.T) { if m2.step != stepAPIKey { t.Errorf("step = %d, want stepAPIKey", m2.step) } - if m2.formError != "API key is required (or set $DASHSCOPE_API_KEY)" { - t.Errorf("formError = %q", m2.formError) + // The exact prose is pinned by TestApiKeyStepCanConfirm; this test covers the + // Enter-key wiring, so compare against the helper and never drift again. + if want := officialAPIKeyRequiredError(m2.currentProvider()); m2.formError != want { + t.Errorf("formError = %q, want %q", m2.formError, want) } if cmd != nil { t.Error("Enter without key or env should not quit") @@ -2354,8 +2356,10 @@ func TestProviderTUI_CustomExistingApiKeyEmptyBlocksEnter(t *testing.T) { if m2.step != stepAPIKey { t.Errorf("step = %d, want stepAPIKey", m2.step) } - if m2.formError != "API key is required" { - t.Errorf("formError = %q, want %q", m2.formError, "API key is required") + // Prefix, not the full string: this test covers Enter-key gating, and the + // exact wording is pinned by TestApiKeyStepCanConfirm. + if !strings.HasPrefix(m2.formError, "API key is required") { + t.Errorf("formError = %q, want it to start with %q", m2.formError, "API key is required") } if cmd != nil { t.Error("Enter with cleared key should not quit") @@ -2600,6 +2604,7 @@ func TestProviderTUI_DeleteModelPreservesActiveModel(t *testing.T) { } func TestApplyCustomProviderConfigPreservesModelOrder(t *testing.T) { + isolateLLMConnectionTest(t) dir := t.TempDir() configPath := filepath.Join(dir, "config.json") models := []string{"test-model", "test-model-2", "bbb", "aaa", "test-model-3"} @@ -2643,6 +2648,7 @@ func TestApplyCustomProviderConfigPreservesModelOrder(t *testing.T) { } func TestApplyManualConfigNormalizesAuthHeader(t *testing.T) { + isolateLLMConnectionTest(t) dir := t.TempDir() configPath := filepath.Join(dir, "config.json") cfg := &Config{} @@ -2668,6 +2674,7 @@ func TestApplyManualConfigNormalizesAuthHeader(t *testing.T) { } func TestApplyCustomProviderConfigNormalizesAuthHeader(t *testing.T) { + isolateLLMConnectionTest(t) dir := t.TempDir() configPath := filepath.Join(dir, "config.json") cfg := &Config{ @@ -2816,6 +2823,7 @@ func TestEnterEditCustomProvider_ProtocolIndex(t *testing.T) { // mirrored for the two protocols that have a boolean equivalent so older // binaries can still read the config. func TestApplyManualConfig_DoubleWritesProtocolAndUseAnthropic(t *testing.T) { + isolateLLMConnectionTest(t) dir := t.TempDir() configPath := filepath.Join(dir, "config.json") diff --git a/internal/config/rules/system_rules_test.go b/internal/config/rules/system_rules_test.go index ed7bc926..edbec307 100644 --- a/internal/config/rules/system_rules_test.go +++ b/internal/config/rules/system_rules_test.go @@ -1170,7 +1170,10 @@ func TestResolveRuleEntries_SymlinkSafety(t *testing.T) { // The extension check on the resolved path should reject .json. symlinkPath := filepath.Join(dir, "evil.md") if err := os.Symlink(sensitiveFile, symlinkPath); err != nil { - t.Fatal(err) + // Creating a symlink on Windows needs SeCreateSymbolicLinkPrivilege, which + // an unelevated CI account does not have. Same skip the other symlink tests + // in this repo already use. + t.Skipf("cannot create symlink: %v", err) } entries := []ProjectRuleEntry{ diff --git a/internal/llm/keycmd.go b/internal/llm/keycmd.go new file mode 100644 index 00000000..0060c88e --- /dev/null +++ b/internal/llm/keycmd.go @@ -0,0 +1,130 @@ +package llm + +import ( + "bytes" + "context" + "errors" + "fmt" + "os" + "os/exec" + "strings" + "time" +) + +// keyCmdTimeout bounds how long an api_key_cmd / auth_token_cmd may run. +// It is a package var (not const) so tests can shrink it. +var keyCmdTimeout = 60 * time.Second + +// keyCmdWaitDelay bounds how long Wait keeps waiting on the child's stdout pipe +// after the command's own deadline has passed. Package var (not const) so tests +// can shrink it, same as keyCmdTimeout. +var keyCmdWaitDelay = 5 * time.Second + +// keyCmdMaxOutput caps how much of a credential command's stdout we buffer. +const keyCmdMaxOutput = 64 << 10 + +// errKeyCmdOutputTooLarge aborts the stdout copy once the cap is hit. It never +// reaches the caller: cappedBuffer.overflow is what produces the error message. +var errKeyCmdOutputTooLarge = errors.New("credential command output exceeds cap") + +// cappedBuffer collects at most max bytes and records whether more were offered. +// Refusing the write makes os/exec's copier close the pipe, so a runaway command +// (`cat /dev/urandom`) dies of SIGPIPE instead of growing our heap without bound. +type cappedBuffer struct { + max int + buf bytes.Buffer + overflow bool +} + +func (b *cappedBuffer) Write(p []byte) (int, error) { + if b.buf.Len()+len(p) > b.max { + b.overflow = true + return 0, errKeyCmdOutputTooLarge + } + return b.buf.Write(p) +} + +// resolveKeyCmd runs a credential-fetching shell command and returns its +// trimmed, single-line stdout. label names the source (e.g. +// `api_key_cmd for provider "x"`) and is used in error messages. +// +// The child's stderr is wired to the process stderr so interactive prompts +// (pinentry, 1Password, `op`) stay visible, and its stdin to the process stdin +// so those prompts can be answered. Any failure is a hard error, never a silent +// fallback. The resolved credential is used in memory only and is never written +// to config or logged. +func resolveKeyCmd(cmd, label string) (string, error) { + ctx, cancel := context.WithTimeout(context.Background(), keyCmdTimeout) + defer cancel() + + c := newKeyCmd(ctx, cmd) + c.Stderr = os.Stderr + // With Stdin nil, os/exec hands the child /dev/null, so a helper that needs + // to prompt for a passphrase gets EOF or refuses to prompt at all because it + // sees no tty. Safe to hand over os.Stdin because no code path resolves an + // endpoint while the bubbletea TUI (which also reads os.Stdin) is running: + // ResolveEndpoint's only callers are the non-TUI review/scan and `ocr llm + // test` paths. Adding an in-TUI connection test would break that. + c.Stdin = os.Stdin + // Buffer stdout through cappedBuffer rather than an *os.File so os/exec does + // the copying in its own goroutine: that is what lets WaitDelay force the + // pipe closed. exec.CommandContext SIGKILLs only the shell, so a grandchild + // (gpg-agent, pinentry, `op`) that inherited the stdout pipe keeps it open + // and Wait blocks on the read long past the timeout -- reproducible with + // api_key_cmd = "sleep 200 & printf tok". WaitDelay makes Wait give up + // shortly after the context dies. + out := &cappedBuffer{max: keyCmdMaxOutput} + c.Stdout = out + c.WaitDelay = keyCmdWaitDelay + + err := c.Run() + // Checked first so a timeout reports as such instead of as the SIGKILL exit + // status it produces. (Run has already joined every stdout copier, so the + // buffer below is safe to read on all paths.) + if ctx.Err() == context.DeadlineExceeded { + // Wrap ctx.Err() so callers can errors.Is(err, context.DeadlineExceeded). + return "", fmt.Errorf("%s timed out after %s: %w", label, keyCmdTimeout, ctx.Err()) + } + if out.overflow { + return "", fmt.Errorf("%s produced more than 64KiB of output", label) + } + // ErrWaitDelay only means an orphaned grandchild still holds the pipe; the + // command itself exited fine and its output is already buffered, so use it + // rather than surfacing an exec-internal error. + if err != nil && !errors.Is(err, exec.ErrWaitDelay) { + // Covers non-zero exit and command-not-found (the shell exits non-zero + // and prints its not-found message on the child's stderr). ExitError.Stderr + // stays nil because we assigned c.Stderr, so no output can leak here. + return "", fmt.Errorf("%s failed: %w", label, err) + } + + // Trim a trailing line break; multi-line output past that is ambiguous and refused. + // ContainsAny (not Contains "\n") so a lone interior CR is caught too: TrimRight + // leaves it, TrimSpace below only strips the edges, and a CR inside a credential + // makes net/http reject the Authorization header with an opaque error. + trimmed := strings.TrimRight(out.buf.String(), "\r\n") + if strings.ContainsAny(trimmed, "\n\r") { + return "", fmt.Errorf("%s produced multi-line output; expected a single credential (pipe through 'head -n1' if your command prints more)", label) + } + // Same reason as the line-break check, wider net: httpguts.ValidHeaderFieldValue + // (what net/http enforces) rejects every byte below 0x20 except SP and TAB, plus + // DEL. A NUL or VT smuggled in by e.g. `printf 'sk-a\0b'` would otherwise reach + // net/http as the opaque `invalid header field value for "Authorization"`. + // + // Deliberately before the TrimSpace below, so a trailing control byte is an + // error naming its offset rather than silently stripped: only TAB, SP and the + // line breaks already handled above are things a credential command can + // plausibly append by accident. Offsets are therefore into the pre-TrimSpace + // string, which is what the command actually produced. + for i := 0; i < len(trimmed); i++ { + if b := trimmed[i]; (b < 0x20 && b != '\t') || b == 0x7f { + return "", fmt.Errorf("%s produced a control byte 0x%02X at offset %d; a credential must not contain control characters", label, b, i) + } + } + + key := strings.TrimSpace(trimmed) + if key == "" { + return "", fmt.Errorf("%s produced empty output", label) + } + return key, nil +} diff --git a/internal/llm/keycmd_test.go b/internal/llm/keycmd_test.go new file mode 100644 index 00000000..ddf7263f --- /dev/null +++ b/internal/llm/keycmd_test.go @@ -0,0 +1,160 @@ +//go:build !windows + +package llm + +import ( + "os" + "strings" + "testing" + "time" +) + +func TestResolveKeyCmd(t *testing.T) { + tests := []struct { + name string + cmd string + want string + wantErr string // substring the error must contain; "" means success + }{ + {name: "success", cmd: "printf 'sk-test\\n'", want: "sk-test"}, + {name: "trailing whitespace trimmed", cmd: "printf ' sk-test \\n'", want: "sk-test"}, + {name: "no trailing newline", cmd: "printf 'sk-test'", want: "sk-test"}, + {name: "crlf line ending trimmed", cmd: "printf 'sk-crlf\\r\\n'", want: "sk-crlf"}, + {name: "non-zero exit", cmd: "exit 3", wantErr: "failed: exit status 3"}, + {name: "false", cmd: "false", wantErr: "failed:"}, + {name: "empty output", cmd: "true", wantErr: "produced empty output"}, + {name: "empty printf", cmd: "printf ''", wantErr: "produced empty output"}, + {name: "whitespace-only output", cmd: "printf ' \\n'", wantErr: "produced empty output"}, + {name: "multi-line output", cmd: "printf 'a\\nb\\n'", wantErr: "produced multi-line output"}, + // A lone interior CR is a line break too, and one that survives both + // TrimRight("\r\n") and TrimSpace. Refuse it here rather than let it reach + // net/http, which rejects the Authorization header with an opaque error. + {name: "interior carriage return", cmd: "printf 'a\\rb'", wantErr: "produced multi-line output"}, + {name: "multi-line error names the fix", cmd: "printf 'a\\nb\\n'", wantErr: "pipe through 'head -n1'"}, + // Every other control byte net/http rejects (httpguts.ValidHeaderFieldValue: + // anything < 0x20 except TAB, plus DEL) must be named here rather than reach + // the request as an opaque "invalid header field value" failure. + {name: "nul byte", cmd: "printf 'sk-a\\0b'", wantErr: "control byte 0x00 at offset 4"}, + {name: "vertical tab", cmd: "printf 'sk-a\\013b'", wantErr: "control byte 0x0B at offset 4"}, + {name: "form feed", cmd: "printf 'sk-a\\014b'", wantErr: "control byte 0x0C at offset 4"}, + {name: "delete byte", cmd: "printf 'sk-a\\177b'", wantErr: "control byte 0x7F at offset 4"}, + // TAB is legal in a header value, so it survives (interior only; TrimSpace + // takes the edges). + {name: "interior tab kept", cmd: "printf 'sk-a\\tb\\n'", want: "sk-a\tb"}, + {name: "command not found", cmd: "this-cmd-does-not-exist-xyz", wantErr: "failed:"}, + // Boundary: exactly the cap is fine, one byte more is refused. The child + // dies of SIGPIPE as soon as we stop accepting, so this stays fast. + {name: "output exactly at cap", cmd: "head -c 65536 /dev/zero | tr '\\0' a", want: strings.Repeat("a", keyCmdMaxOutput)}, + {name: "output over cap", cmd: "yes aaaaaaaaaa | head -c 200000 | tr -d '\\n'", wantErr: "produced more than 64KiB of output"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + got, err := resolveKeyCmd(tt.cmd, "api_key_cmd for provider \"x\"") + if tt.wantErr != "" { + if err == nil { + t.Fatalf("expected error containing %q, got nil (output %q)", tt.wantErr, got) + } + if !strings.Contains(err.Error(), tt.wantErr) { + t.Fatalf("error %q does not contain %q", err.Error(), tt.wantErr) + } + return + } + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if got != tt.want { + t.Fatalf("got %q, want %q", got, tt.want) + } + }) + } +} + +func TestResolveKeyCmd_Timeout(t *testing.T) { + origTimeout, origDelay := keyCmdTimeout, keyCmdWaitDelay + keyCmdTimeout = 50 * time.Millisecond + // `sleep 5` inherits the stdout pipe and outlives the SIGKILL'd shell, so + // without a shrunk WaitDelay this test waits the full default 5s. + keyCmdWaitDelay = 100 * time.Millisecond + t.Cleanup(func() { keyCmdTimeout, keyCmdWaitDelay = origTimeout, origDelay }) + + _, err := resolveKeyCmd("sleep 5 2>/dev/null", "api_key_cmd for provider \"x\"") + if err == nil { + t.Fatal("expected timeout error, got nil") + } + if !strings.Contains(err.Error(), "timed out after") { + t.Fatalf("error %q does not mention timeout", err.Error()) + } +} + +// A grandchild that inherited the stdout pipe keeps it open after the shell +// exits, which used to block Wait until the grandchild died. WaitDelay bounds +// that: this must finish in well under the 30s sleep. +func TestResolveKeyCmd_WaitDelayBoundsOrphanHoldingPipe(t *testing.T) { + origTimeout, origDelay := keyCmdTimeout, keyCmdWaitDelay + keyCmdTimeout = 50 * time.Millisecond + keyCmdWaitDelay = 100 * time.Millisecond + t.Cleanup(func() { keyCmdTimeout, keyCmdWaitDelay = origTimeout, origDelay }) + + // The grandchild must keep the inherited *stdout* pipe open (that is the case + // under test) but not our stderr: it outlives the test, and `go test` reads + // the test binary's stderr until EOF, so leaving it attached would stall the + // run for the full sleep even though resolveKeyCmd returned immediately. + start := time.Now() + _, err := resolveKeyCmd("sleep 30 2>/dev/null & printf tok", `api_key_cmd for provider "x"`) + elapsed := time.Since(start) + + if elapsed > 5*time.Second { + t.Fatalf("took %s; WaitDelay did not bound the orphaned grandchild", elapsed) + } + if err == nil { + t.Fatal("expected timeout error, got nil") + } + if !strings.Contains(err.Error(), "timed out after") { + t.Fatalf("error %q does not mention timeout", err.Error()) + } +} + +// TestResolveKeyCmd_StdinWired proves the child inherits our stdin: with Stdin +// left nil, os/exec hands the child /dev/null, `read` sees EOF and prints +// nothing, so this would fail with "produced empty output" instead. +// +// os.Stdin under `go test` is not a usable prompt source, so swap in a pipe. +// Mutating the global is safe here: this test is not parallel, and the only +// parallel tests in the package are subtests of TestResolveKeyCmd, which +// finishes before any later top-level test starts. +func TestResolveKeyCmd_StdinWired(t *testing.T) { + r, w, err := os.Pipe() + if err != nil { + t.Fatalf("os.Pipe: %v", err) + } + defer r.Close() + + orig := os.Stdin + os.Stdin = r + t.Cleanup(func() { os.Stdin = orig }) + + // Written and closed up front (well under the pipe buffer, so no blocking) + // so the child reads a full line and then EOF. + if _, err := w.WriteString("passphrase-from-stdin\n"); err != nil { + t.Fatalf("write to stdin pipe: %v", err) + } + if err := w.Close(); err != nil { + t.Fatalf("close stdin pipe writer: %v", err) + } + + got, err := resolveKeyCmd(`read -r x; printf %s "$x"`, `api_key_cmd for provider "x"`) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if got != "passphrase-from-stdin" { + t.Fatalf("got %q, want %q", got, "passphrase-from-stdin") + } +} + +func TestResolveKeyCmd_LabelInError(t *testing.T) { + _, err := resolveKeyCmd("false", `auth_token_cmd for llm config`) + if err == nil || !strings.HasPrefix(err.Error(), "auth_token_cmd for llm config") { + t.Fatalf("expected label prefix in error, got %v", err) + } +} diff --git a/internal/llm/keycmd_unix.go b/internal/llm/keycmd_unix.go new file mode 100644 index 00000000..03e91beb --- /dev/null +++ b/internal/llm/keycmd_unix.go @@ -0,0 +1,14 @@ +//go:build !windows + +package llm + +import ( + "context" + "os/exec" +) + +// newKeyCmd builds the OS-specific shell invocation (sh -c on Unix) that runs a +// credential command under ctx, so its timeout and cancellation are honored. +func newKeyCmd(ctx context.Context, cmd string) *exec.Cmd { + return exec.CommandContext(ctx, "sh", "-c", cmd) +} diff --git a/internal/llm/keycmd_windows.go b/internal/llm/keycmd_windows.go new file mode 100644 index 00000000..5e71e7b1 --- /dev/null +++ b/internal/llm/keycmd_windows.go @@ -0,0 +1,47 @@ +//go:build windows + +package llm + +import ( + "context" + "os/exec" + "syscall" +) + +// newKeyCmd builds the OS-specific shell invocation (cmd.exe /C on Windows) that runs a +// credential command under ctx, so its timeout and cancellation are honored. +// Spelled with the extension so a file named `cmd` on PATH cannot shadow the shell. +// +// The command line is handed over through SysProcAttr.CmdLine instead of Args +// because os/exec quotes Args with syscall.EscapeArg, which targets +// CommandLineToArgvW; cmd.exe is a documented exception with different unquoting +// rules (see the exec.Command doc comment), and its escaping mangles any command +// containing a double quote -- `op read "op://Private/My Vault/api-key"` would +// arrive as a single literal filename. /S makes cmd.exe strip exactly the outer +// pair of quotes we add and pass the rest through verbatim. +// +// Not escaping the interpolated cmd is deliberate rather than an injection hole: +// api_key_cmd is a command line its author asked us to run, so they already have +// arbitrary execution by design (`api_key_cmd = "whoami"` is a supported config, +// and the Unix arm hands the same string to `sh -c`), and it is read only from +// the user-level ~/.opencodereview/config.json -- never from the repository +// under review. Escaping the inner quotes would defeat the single case CmdLine +// exists for. See keycmd_windows_test.go for which quote shapes /S does and does +// not keep as one command. +// +// Note that a command string is not portable between the two arms: %VAR% and ^ +// are cmd.exe metacharacters and $VAR expansion / \ escaping do not apply, so an +// sh-authored api_key_cmd generally needs a Windows-specific rewrite. +func newKeyCmd(ctx context.Context, cmd string) *exec.Cmd { + // Still built by CommandContext so ctx cancellation and WaitDelay behave + // exactly as on Unix; only the command-line construction differs. + c := exec.CommandContext(ctx, "cmd.exe") + // CmdLine is the whole command line including argv[0]; the executable itself + // still comes from c.Path. Args stays at Command's default ([]string{"cmd.exe"}) + // rather than nil: syscall.StartProcess uses SysProcAttr.CmdLine verbatim when + // non-empty and never looks at argv, so the doc's "leaving Args empty" is not + // load-bearing here -- and a one-element Args keeps Cmd.String() from panicking + // on Args[1:]. + c.SysProcAttr = &syscall.SysProcAttr{CmdLine: `cmd.exe /S /C "` + cmd + `"`} + return c +} diff --git a/internal/llm/keycmd_windows_test.go b/internal/llm/keycmd_windows_test.go new file mode 100644 index 00000000..3cfa91d1 --- /dev/null +++ b/internal/llm/keycmd_windows_test.go @@ -0,0 +1,152 @@ +//go:build windows + +package llm + +import ( + "context" + "os" + "strings" + "testing" + "time" +) + +// TestNewKeyCmd_CmdLine locks in the two decisions in newKeyCmd that no runtime +// test can observe: the command reaches cmd.exe through SysProcAttr.CmdLine +// verbatim (not through Args, whose syscall.EscapeArg quoting mangles embedded +// double quotes), and Args keeps its one-element default so Cmd.String() cannot +// panic on Args[1:]. +func TestNewKeyCmd_CmdLine(t *testing.T) { + c := newKeyCmd(context.Background(), `op read "op://Private/My Vault/api-key"`) + + want := `cmd.exe /S /C "op read "op://Private/My Vault/api-key""` + if c.SysProcAttr == nil { + t.Fatal("SysProcAttr is nil; the command line would be built from Args instead") + } + if got := c.SysProcAttr.CmdLine; got != want { + t.Errorf("CmdLine = %q, want %q", got, want) + } + if len(c.Args) == 0 { + t.Error("Args is empty; Cmd.String() indexes Args[1:] and panics on a nil slice") + } + // Panics if Args were nilled out. + if s := c.String(); s == "" { + t.Error("Cmd.String() returned empty") + } +} + +func TestResolveKeyCmd(t *testing.T) { + tests := []struct { + name string + cmd string + want string + wantErr string // substring the error must contain; "" means success + }{ + {name: "success", cmd: "echo sk-test", want: "sk-test"}, + // ECHO eats exactly one delimiter after the command token, so stdout here is + // " sk-test \r\n" -- the trim is what produces the credential. + {name: "surrounding whitespace trimmed", cmd: "echo sk-test ", want: "sk-test"}, + // The case the CmdLine detour exists for: quotes and spaces must arrive at + // cmd.exe exactly as written. Routed through Args instead, EscapeArg would + // wrap and backslash-escape them and the output would carry the backslashes. + {name: "embedded quotes survive verbatim", cmd: `echo sk-"a b"-token`, want: `sk-"a b"-token`}, + {name: "non-zero exit", cmd: "exit 3", wantErr: "failed: exit status 3"}, + {name: "no output", cmd: "rem", wantErr: "produced empty output"}, + {name: "blank line only", cmd: "echo.", wantErr: "produced empty output"}, + // & is cmd.exe's command separator, so both echoes run and produce two lines. + {name: "multi-line output", cmd: "echo a& echo b", wantErr: "produced multi-line output"}, + // The two rows below pin down what the outer quote pair we add does and does + // not protect, because "the command line could split" reads like a hole until + // you know which shapes actually split. /S makes cmd.exe strip the first + // character and the last quote and run the remainder unchanged, so a bare + // interior quote leaves the following & inside a quoted region: it stays one + // command and echo prints the & literally. + {name: "interior quote keeps & quoted", cmd: `echo A" & echo B`, want: `A" & echo B`}, + // A doubled quote closes that region, so this & is a real separator and both + // echoes run. It is not a privilege boundary -- api_key_cmd is already a + // command line its author asked us to run -- but it is the one shape where the + // line splits, and the single-line guard is what stops the extra output from + // being mistaken for the credential. + {name: "doubled quote lets & split the line", cmd: `echo A"" & echo B`, wantErr: "produced multi-line output"}, + {name: "command not found", cmd: "this-cmd-does-not-exist-xyz", wantErr: "failed:"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + got, err := resolveKeyCmd(tt.cmd, `api_key_cmd for provider "x"`) + if tt.wantErr != "" { + if err == nil { + t.Fatalf("expected error containing %q, got nil (output %q)", tt.wantErr, got) + } + if !strings.Contains(err.Error(), tt.wantErr) { + t.Fatalf("error %q does not contain %q", err.Error(), tt.wantErr) + } + return + } + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if got != tt.want { + t.Fatalf("got %q, want %q", got, tt.want) + } + }) + } +} + +func TestResolveKeyCmd_Timeout(t *testing.T) { + origTimeout, origDelay := keyCmdTimeout, keyCmdWaitDelay + keyCmdTimeout = 50 * time.Millisecond + keyCmdWaitDelay = 100 * time.Millisecond + t.Cleanup(func() { keyCmdTimeout, keyCmdWaitDelay = origTimeout, origDelay }) + + // ping, not timeout.exe: timeout.exe refuses to run when stdin is redirected, + // and resolveKeyCmd hands the child the test binary's stdin. Its stderr is + // redirected for the same reason the unix twin redirects it: the killed + // command's orphan would otherwise hold the test binary's stderr, which + // cmd/go reads to EOF, stalling the run past the point resolveKeyCmd returned. + _, err := resolveKeyCmd("ping -n 6 127.0.0.1 2>nul", `api_key_cmd for provider "x"`) + if err == nil { + t.Fatal("expected timeout error, got nil") + } + if !strings.Contains(err.Error(), "timed out after") { + t.Fatalf("error %q does not mention timeout", err.Error()) + } +} + +// TestResolveKeyCmd_StdinWired proves the child inherits our stdin: with Stdin +// left nil, os/exec hands the child NUL, findstr reads EOF immediately and +// prints nothing, so this would fail with "produced empty output" instead. +func TestResolveKeyCmd_StdinWired(t *testing.T) { + r, w, err := os.Pipe() + if err != nil { + t.Fatalf("os.Pipe: %v", err) + } + defer r.Close() + + orig := os.Stdin + os.Stdin = r + t.Cleanup(func() { os.Stdin = orig }) + + if _, err := w.WriteString("passphrase-from-stdin\r\n"); err != nil { + t.Fatalf("write to stdin pipe: %v", err) + } + if err := w.Close(); err != nil { + t.Fatalf("close stdin pipe writer: %v", err) + } + + // findstr "^" copies every stdin line to stdout; ^ is passed through verbatim + // under /S rather than treated as cmd.exe's escape character. + got, err := resolveKeyCmd(`findstr "^"`, `api_key_cmd for provider "x"`) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if got != "passphrase-from-stdin" { + t.Fatalf("got %q, want %q", got, "passphrase-from-stdin") + } +} + +func TestResolveKeyCmd_LabelInError(t *testing.T) { + _, err := resolveKeyCmd("exit 1", `auth_token_cmd for llm config`) + if err == nil || !strings.HasPrefix(err.Error(), "auth_token_cmd for llm config") { + t.Fatalf("expected label prefix in error, got %v", err) + } +} diff --git a/internal/llm/resolver.go b/internal/llm/resolver.go index 853f42fd..c863d0db 100644 --- a/internal/llm/resolver.go +++ b/internal/llm/resolver.go @@ -40,10 +40,11 @@ const ( // openai | openai-responses). Takes priority // over OCR_USE_ANTHROPIC when set. envOCRLLMProtocol = "OCR_LLM_PROTOCOL" - // envOCRLLMTimeout is a global override applied in ResolveEndpointWithModelOverride - // after any strategy resolves, rather than inside tryOCREnv like other OCR_LLM_* vars. - // This lets it override timeout for all resolution paths (OCR env, config file, - // provider config, Claude Code env, shell RC). + // envOCRLLMTimeout is a global override parsed at the top of + // ResolveEndpointWithModelOverride and applied to whichever strategy resolves, + // rather than inside tryOCREnv like other OCR_LLM_* vars. This lets it override + // timeout for all resolution paths (OCR env, config file, provider config, + // Claude Code env, shell RC). envOCRLLMTimeout = "OCR_LLM_TIMEOUT" envOCRUseAnthropic = "OCR_USE_ANTHROPIC" ) @@ -68,6 +69,23 @@ func ResolveEndpoint(configPath string) (ResolvedEndpoint, error) { func ResolveEndpointWithModelOverride(configPath, modelOverride string) (ResolvedEndpoint, error) { modelOverride = strings.TrimSpace(modelOverride) + // Both global env overrides are parsed before any strategy runs, even though + // they are applied to the resolved endpoint below. Parsing them after the loop + // would let a typo'd OCR_LLM_TIMEOUT ("30s") or an unparseable + // OCR_LLM_EXTRA_HEADERS abort resolution *after* api_key_cmd already prompted + // 1Password/pinentry/Touch ID for a credential that then gets discarded. + envTimeout, hasEnvTimeout, err := parseTimeoutEnv() + if err != nil { + return ResolvedEndpoint{}, err + } + var envHeaders map[string]string + if raw := os.Getenv(envOCRLLMExtraHeaders); raw != "" { + envHeaders, err = ParseExtraHeaders(raw) + if err != nil { + return ResolvedEndpoint{}, fmt.Errorf("%s: %w", envOCRLLMExtraHeaders, err) + } + } + strategies := []struct { name string fn func() (ResolvedEndpoint, bool, error) @@ -91,21 +109,13 @@ func ResolveEndpointWithModelOverride(configPath, modelOverride string) (Resolve // OCR_LLM_TIMEOUT is a global override: applies regardless of // which strategy resolved the endpoint, and takes precedence // over config-file values when set. - envTimeout, ok, err := parseTimeoutEnv() - if err != nil { - return ResolvedEndpoint{}, fmt.Errorf("resolve %s: %w", s.name, err) - } - if ok { + if hasEnvTimeout { ep.Timeout = envTimeout } // OCR_LLM_EXTRA_HEADERS is a global override: merges into // extra headers regardless of which strategy resolved the // endpoint. Env values take precedence over config-file values. - if raw := os.Getenv(envOCRLLMExtraHeaders); raw != "" { - envHeaders, err := ParseExtraHeaders(raw) - if err != nil { - return ResolvedEndpoint{}, fmt.Errorf("resolve %s: %w", s.name, err) - } + if envHeaders != nil { if ep.ExtraHeaders == nil { ep.ExtraHeaders = envHeaders } else { @@ -212,9 +222,10 @@ type llmFileConfig struct { AuthToken string `json:"auth_token,omitempty"` AuthHeader string `json:"auth_header,omitempty"` Model string `json:"model,omitempty"` - Protocol string `json:"protocol,omitempty"` // anthropic|openai|openai-responses; takes priority over use_anthropic - UseAnthropic *bool `json:"use_anthropic,omitempty"` // pointer to distinguish unset from false; legacy fallback when protocol is empty - TimeoutSec int `json:"timeout_sec,omitempty"` // per-request HTTP timeout in seconds + AuthTokenCmd string `json:"auth_token_cmd,omitempty"` // shell command whose stdout is the auth token; used when auth_token is empty + Protocol string `json:"protocol,omitempty"` // anthropic|openai|openai-responses; takes priority over use_anthropic + UseAnthropic *bool `json:"use_anthropic,omitempty"` // pointer to distinguish unset from false; legacy fallback when protocol is empty + TimeoutSec int `json:"timeout_sec,omitempty"` // per-request HTTP timeout in seconds ExtraBody map[string]any `json:"extra_body,omitempty"` ExtraHeaders map[string]string `json:"extra_headers,omitempty"` } @@ -222,6 +233,7 @@ type llmFileConfig struct { // providerEntryConfig represents a single provider entry in config.json. type providerEntryConfig struct { APIKey string `json:"api_key,omitempty"` + APIKeyCmd string `json:"api_key_cmd,omitempty"` // shell command whose stdout is the api key; used when api_key is empty URL string `json:"url,omitempty"` Protocol string `json:"protocol,omitempty"` Model string `json:"model,omitempty"` @@ -281,14 +293,39 @@ func tryProviderConfig(cfg configFile, modelOverride string) (ResolvedEndpoint, return ResolvedEndpoint{}, false, fmt.Errorf("provider %q is set but not configured in %s section", cfg.Provider, section) } + // Pick the credential source here, but run api_key_cmd only just before + // returning (see below): a config typo must not trigger a secret-manager + // prompt before the cheap validation below has had a chance to fail. + // A whitespace-only api_key is a typo, not a credential: treat it as unset so + // it cannot silently shadow a working api_key_cmd (which otherwise resolves to + // a 401 with the command never running). A key with real content is used + // verbatim -- unlike command stdout, which has a mechanical trailing newline + // to strip, a static value has no artifact that trimming must undo. apiKey := entry.APIKey - if apiKey == "" { - if isPreset && preset.EnvVar != "" { - apiKey = os.Getenv(preset.EnvVar) + if strings.TrimSpace(apiKey) == "" { + apiKey = "" + } + switch { + case apiKey != "": + // Static api_key always wins. Warn (don't error) if a command is also set, + // so a config that keeps api_key_cmd as a deliberate fallback still works. + if entry.APIKeyCmd != "" { + fmt.Fprintf(os.Stderr, "[ocr] WARNING: provider %q has both api_key and api_key_cmd set; using the static api_key\n", cfg.Provider) + } + case entry.APIKeyCmd == "" && isPreset && preset.EnvVar != "": + // Env var is the last resort: only when neither api_key nor api_key_cmd + // is set, and only for preset providers (custom ones have no fallback). + // Same whitespace rule as the static key above, so `export + // ANTHROPIC_API_KEY=" "` reports "no api_key configured" instead of + // sending `Authorization: Bearer ` and getting an opaque 401. + if v := os.Getenv(preset.EnvVar); strings.TrimSpace(v) != "" { + apiKey = v } } - if apiKey == "" { - return ResolvedEndpoint{}, false, fmt.Errorf("provider %q has no api_key configured and no environment variable fallback found", cfg.Provider) + // No credential at all is still an error here, before any other validation: + // only the command's *execution* is deferred, not the emptiness check. + if apiKey == "" && entry.APIKeyCmd == "" { + return ResolvedEndpoint{}, false, fmt.Errorf("provider %q has no api_key or api_key_cmd configured and no environment variable fallback found", cfg.Provider) } var url, protocol, authHeader, model string @@ -389,6 +426,18 @@ func tryProviderConfig(cfg configFile, modelOverride string) (ResolvedEndpoint, url = ensureMessagesSuffix(url) } + // Single api_key_cmd resolution site for both preset and custom providers, + // as late as possible: everything above can fail without running the + // command. apiKey is empty here only when api_key_cmd is set (guaranteed by + // the emptiness check above), and a failing command is a hard error. + if apiKey == "" { + resolved, err := resolveKeyCmd(entry.APIKeyCmd, fmt.Sprintf("api_key_cmd for provider %q", cfg.Provider)) + if err != nil { + return ResolvedEndpoint{}, false, err + } + apiKey = resolved + } + return ResolvedEndpoint{ URL: url, Token: apiKey, @@ -408,9 +457,23 @@ func tryLegacyLlmConfig(cfg configFile, modelOverride string) (ResolvedEndpoint, if modelOverride != "" { model = modelOverride } - if cfg.Llm.URL == "" || cfg.Llm.AuthToken == "" || model == "" { + // Fall through to later strategies when the legacy block is incomplete. This + // includes the case where neither auth_token nor auth_token_cmd is set — and, + // critically, an incomplete block (e.g. missing url) never runs auth_token_cmd. + // Whitespace-only auth_token is treated as unset, same as api_key above, so it + // cannot shadow a working auth_token_cmd. + token := cfg.Llm.AuthToken + if strings.TrimSpace(token) == "" { + token = "" + } + if cfg.Llm.URL == "" || model == "" || (token == "" && cfg.Llm.AuthTokenCmd == "") { return ResolvedEndpoint{}, false, nil } + // Static auth_token always wins; warn if a command is also set. The command + // itself runs only just before returning, after the validation below. + if token != "" && cfg.Llm.AuthTokenCmd != "" { + fmt.Fprintln(os.Stderr, "[ocr] WARNING: llm config has both auth_token and auth_token_cmd set; using the static auth_token") + } // llm.protocol (normalized) wins over use_anthropic when set. protocol := "" @@ -449,7 +512,18 @@ func tryLegacyLlmConfig(cfg configFile, modelOverride string) (ResolvedEndpoint, return ResolvedEndpoint{}, false, fmt.Errorf("OCR config file: %w", err) } - return ResolvedEndpoint{URL: cfg.Llm.URL, Token: cfg.Llm.AuthToken, Model: model, Protocol: protocol, AuthHeader: authHeader, Source: "OCR config file", ExtraBody: cfg.Llm.ExtraBody, ExtraHeaders: cfg.Llm.ExtraHeaders, Timeout: timeout}, true, nil + // token is empty here only for an otherwise-complete block whose + // auth_token_cmd is set (guaranteed by the incompleteness check above), so a + // failing command is a hard error and an incomplete block never runs it. + if token == "" { + resolved, err := resolveKeyCmd(cfg.Llm.AuthTokenCmd, "auth_token_cmd for llm config") + if err != nil { + return ResolvedEndpoint{}, false, err + } + token = resolved + } + + return ResolvedEndpoint{URL: cfg.Llm.URL, Token: token, Model: model, Protocol: protocol, AuthHeader: authHeader, Source: "OCR config file", ExtraBody: cfg.Llm.ExtraBody, ExtraHeaders: cfg.Llm.ExtraHeaders, Timeout: timeout}, true, nil } // tryCCEnv reads Claude Code environment variables. diff --git a/internal/llm/resolver_keycmd_test.go b/internal/llm/resolver_keycmd_test.go new file mode 100644 index 00000000..608a93b1 --- /dev/null +++ b/internal/llm/resolver_keycmd_test.go @@ -0,0 +1,390 @@ +//go:build !windows + +// Every test in this file drives a credential command, and all of them are POSIX +// shell (`printf`, `exit N`), which would run through `cmd /C` on Windows. + +package llm + +import ( + "encoding/json" + "io" + "os" + "path/filepath" + "strings" + "testing" +) + +func writeConfigJSON(t *testing.T, cfg configFile) string { + t.Helper() + data, err := json.Marshal(cfg) + if err != nil { + t.Fatalf("marshal config: %v", err) + } + p := filepath.Join(t.TempDir(), "config.json") + if err := os.WriteFile(p, data, 0644); err != nil { + t.Fatalf("write config: %v", err) + } + return p +} + +// (a) api_key_cmd resolves when no static key is present. +func TestResolveEndpoint_ProviderAPIKeyCmd(t *testing.T) { + clearAllEnv(t) + cfgPath := writeConfigJSON(t, configFile{ + Provider: "anthropic", + Providers: map[string]providerEntryConfig{ + "anthropic": {APIKeyCmd: "printf 'sk-from-cmd\\n'", Model: "claude-sonnet-4-6"}, + }, + }) + ep, err := ResolveEndpoint(cfgPath) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if ep.Token != "sk-from-cmd" { + t.Errorf("Token = %q, want %q", ep.Token, "sk-from-cmd") + } +} + +// (a2) the command runs exactly once per resolution. "No caching" is correct +// today only because resolution happens once per process; a second call would +// mean a second pinentry prompt per review. +func TestResolveEndpoint_APIKeyCmdRunsExactlyOnce(t *testing.T) { + clearAllEnv(t) + counter := filepath.Join(t.TempDir(), "runs") + cfgPath := writeConfigJSON(t, configFile{ + Provider: "anthropic", + Providers: map[string]providerEntryConfig{ + "anthropic": { + APIKeyCmd: "echo run >> " + counter + "; printf 'sk-once\\n'", + Model: "claude-sonnet-4-6", + }, + }, + }) + ep, err := ResolveEndpoint(cfgPath) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if ep.Token != "sk-once" { + t.Fatalf("Token = %q, want %q", ep.Token, "sk-once") + } + data, err := os.ReadFile(counter) + if err != nil { + t.Fatalf("read counter file: %v", err) + } + if got := strings.Count(string(data), "\n"); got != 1 { + t.Errorf("api_key_cmd ran %d times, want exactly 1 (counter file %q)", got, data) + } +} + +// (b) static api_key wins even when api_key_cmd is also set. +func TestResolveEndpoint_ProviderStaticKeyWinsOverCmd(t *testing.T) { + clearAllEnv(t) + cfgPath := writeConfigJSON(t, configFile{ + Provider: "anthropic", + Providers: map[string]providerEntryConfig{ + "anthropic": {APIKey: "sk-static", APIKeyCmd: "printf 'sk-from-cmd\\n'", Model: "claude-sonnet-4-6"}, + }, + }) + ep, err := ResolveEndpoint(cfgPath) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if ep.Token != "sk-static" { + t.Errorf("Token = %q, want %q (static api_key must win)", ep.Token, "sk-static") + } +} + +// captureStderr swaps os.Stderr for a pipe around fn and returns what was written. +// Output here is tiny, so reading after the writer is closed avoids any pipe-buffer +// deadlock without a goroutine. +func captureStderr(t *testing.T, fn func()) string { + t.Helper() + r, w, err := os.Pipe() + if err != nil { + t.Fatalf("os.Pipe: %v", err) + } + orig := os.Stderr + os.Stderr = w + defer func() { os.Stderr = orig }() + + fn() + + if err := w.Close(); err != nil { + t.Fatalf("close pipe writer: %v", err) + } + out, err := io.ReadAll(r) + if err != nil { + t.Fatalf("read captured stderr: %v", err) + } + return string(out) +} + +// (b2) when both api_key and api_key_cmd are set, a warning is emitted on stderr +// and the resolved token is still the static api_key. +func TestResolveEndpoint_BothSetWarnsAndUsesStaticKey(t *testing.T) { + clearAllEnv(t) + cfgPath := writeConfigJSON(t, configFile{ + Provider: "anthropic", + Providers: map[string]providerEntryConfig{ + "anthropic": {APIKey: "sk-static", APIKeyCmd: "printf 'sk-from-cmd\\n'", Model: "claude-sonnet-4-6"}, + }, + }) + var ep ResolvedEndpoint + var err error + stderr := captureStderr(t, func() { + ep, err = ResolveEndpoint(cfgPath) + }) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if ep.Token != "sk-static" { + t.Errorf("Token = %q, want %q (static api_key must win)", ep.Token, "sk-static") + } + // Match the message, not the log prefix, so this does not break when the + // warning prefix is restyled. + want := `provider "anthropic" has both api_key and api_key_cmd set; using the static api_key` + if !strings.Contains(stderr, want) { + t.Errorf("stderr %q does not contain warning %q", stderr, want) + } +} + +// (e2) legacy path: both auth_token and auth_token_cmd set -> warning + static wins. +func TestResolveEndpoint_LegacyBothSetWarnsAndUsesStaticToken(t *testing.T) { + clearAllEnv(t) + cfgPath := writeConfigJSON(t, configFile{ + Llm: llmFileConfig{ + URL: "https://api.example.com/v1/messages", + AuthToken: "legacy-static", + AuthTokenCmd: "printf 'legacy-from-cmd\\n'", + Model: "claude-sonnet-4-6", + }, + }) + var ep ResolvedEndpoint + var err error + stderr := captureStderr(t, func() { + ep, err = ResolveEndpoint(cfgPath) + }) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if ep.Token != "legacy-static" { + t.Errorf("Token = %q, want %q (static auth_token must win)", ep.Token, "legacy-static") + } + want := "llm config has both auth_token and auth_token_cmd set; using the static auth_token" + if !strings.Contains(stderr, want) { + t.Errorf("stderr %q does not contain warning %q", stderr, want) + } +} + +// (b3) a whitespace-only api_key is a typo, not a credential: it must not shadow +// the command (which used to resolve Token=" " -> 401, command never run), and +// the both-set warning must stay quiet since nothing is really being shadowed. +func TestResolveEndpoint_WhitespaceOnlyStaticKeyUsesCmd(t *testing.T) { + clearAllEnv(t) + cfgPath := writeConfigJSON(t, configFile{ + Provider: "anthropic", + Providers: map[string]providerEntryConfig{ + "anthropic": {APIKey: " ", APIKeyCmd: "printf 'sk-from-cmd\\n'", Model: "claude-sonnet-4-6"}, + }, + }) + var ep ResolvedEndpoint + var err error + stderr := captureStderr(t, func() { + ep, err = ResolveEndpoint(cfgPath) + }) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if ep.Token != "sk-from-cmd" { + t.Errorf("Token = %q, want %q (whitespace-only api_key must not shadow api_key_cmd)", ep.Token, "sk-from-cmd") + } + if strings.Contains(stderr, "both api_key and api_key_cmd") { + t.Errorf("warned about a shadowed command that was actually used; stderr: %q", stderr) + } +} + +// (e3b) the same whitespace rule reaches the env-var fallback, which is the last +// source in the chain and had been exempt: a whitespace-only value there used to +// resolve successfully and send `Authorization: Bearer `, producing an opaque 401 +// instead of naming the missing credential. +func TestResolveEndpoint_WhitespaceOnlyEnvVarIsNotACredential(t *testing.T) { + clearAllEnv(t) + t.Setenv("ANTHROPIC_API_KEY", " ") + cfgPath := writeConfigJSON(t, configFile{ + Provider: "anthropic", + Providers: map[string]providerEntryConfig{ + "anthropic": {Model: "claude-sonnet-4-6"}, + }, + }) + _, err := ResolveEndpoint(cfgPath) + if err == nil { + t.Fatal("expected an error: a whitespace-only env var is not a credential") + } + if !strings.Contains(err.Error(), "no api_key or api_key_cmd configured") { + t.Errorf("error %q does not name the missing credential", err.Error()) + } +} + +// (e4) same on the legacy path. +func TestResolveEndpoint_LegacyWhitespaceOnlyStaticTokenUsesCmd(t *testing.T) { + clearAllEnv(t) + cfgPath := writeConfigJSON(t, configFile{ + Llm: llmFileConfig{ + URL: "https://api.example.com/v1/messages", + AuthToken: "\t\n ", + AuthTokenCmd: "printf 'legacy-from-cmd\\n'", + Model: "claude-sonnet-4-6", + }, + }) + var ep ResolvedEndpoint + var err error + stderr := captureStderr(t, func() { + ep, err = ResolveEndpoint(cfgPath) + }) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if ep.Token != "legacy-from-cmd" { + t.Errorf("Token = %q, want %q (whitespace-only auth_token must not shadow auth_token_cmd)", ep.Token, "legacy-from-cmd") + } + if strings.Contains(stderr, "both auth_token and auth_token_cmd") { + t.Errorf("warned about a shadowed command that was actually used; stderr: %q", stderr) + } +} + +// (c) custom provider with api_key_cmd resolves (custom providers have no env fallback). +func TestResolveEndpoint_CustomProviderAPIKeyCmd(t *testing.T) { + clearAllEnv(t) + cfgPath := writeConfigJSON(t, configFile{ + Provider: "my-gateway", + CustomProviders: map[string]providerEntryConfig{ + "my-gateway": { + APIKeyCmd: "printf 'gw-token\\n'", + URL: "https://gateway.internal.com/v1", + Protocol: "openai", + Model: "llama-3-8b", + }, + }, + }) + ep, err := ResolveEndpoint(cfgPath) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if ep.Token != "gw-token" { + t.Errorf("Token = %q, want %q", ep.Token, "gw-token") + } +} + +// (d) a failing api_key_cmd is a hard error, not a silent fallback. +func TestResolveEndpoint_ProviderAPIKeyCmdFailsHard(t *testing.T) { + clearAllEnv(t) + cfgPath := writeConfigJSON(t, configFile{ + Provider: "anthropic", + Providers: map[string]providerEntryConfig{ + "anthropic": {APIKeyCmd: "exit 7", Model: "claude-sonnet-4-6"}, + }, + }) + _, err := ResolveEndpoint(cfgPath) + if err == nil { + t.Fatal("expected hard error from failing api_key_cmd, got nil") + } + if !strings.Contains(err.Error(), "api_key_cmd") { + t.Errorf("error %q does not mention api_key_cmd", err.Error()) + } +} + +// (d2) the property the design calls non-negotiable: a misconfigured credential +// command must never silently downgrade to an env var. TestResolveEndpoint_ +// ProviderAPIKeyCmdFailsHard runs under clearAllEnv, so it would still pass if +// someone reintroduced an env-var fallback on command failure; this one sets the +// preset's env var so that regression cannot hide. +func TestResolveEndpoint_APIKeyCmdFailureDoesNotFallBackToEnv(t *testing.T) { + clearAllEnv(t) + t.Setenv("ANTHROPIC_API_KEY", "env-api-key") + cfgPath := writeConfigJSON(t, configFile{ + Provider: "anthropic", + Providers: map[string]providerEntryConfig{ + "anthropic": {APIKeyCmd: "exit 7", Model: "claude-sonnet-4-6"}, + }, + }) + ep, err := ResolveEndpoint(cfgPath) + if err == nil { + t.Fatalf("expected hard error from failing api_key_cmd, got nil (Token %q)", ep.Token) + } + if !strings.Contains(err.Error(), "api_key_cmd") { + t.Errorf("error %q does not mention api_key_cmd", err.Error()) + } + // Not an assertion on ep: every error path returns a zero ResolvedEndpoint, so + // ep.Token is "" by construction whenever err != nil. The witness that no + // fallback happened is err being non-nil at all -- with the env var set, a + // silent fallback would have returned success. +} + +// (e) legacy auth_token_cmd resolves on an otherwise-complete llm block. +func TestResolveEndpoint_LegacyAuthTokenCmd(t *testing.T) { + clearAllEnv(t) + cfgPath := writeConfigJSON(t, configFile{ + Llm: llmFileConfig{ + URL: "https://api.example.com/v1/messages", + AuthTokenCmd: "printf 'legacy-token\\n'", + Model: "claude-sonnet-4-6", + }, + }) + ep, err := ResolveEndpoint(cfgPath) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if ep.Token != "legacy-token" { + t.Errorf("Token = %q, want %q", ep.Token, "legacy-token") + } +} + +// (e3) legacy path: an otherwise-complete llm block whose auth_token_cmd fails is +// a hard error. The Claude Code env vars are set to prove it does not fall through +// to that strategy -- a failing credential command must not be papered over by a +// lower-priority source. +func TestResolveEndpoint_LegacyAuthTokenCmdFailsHard(t *testing.T) { + clearAllEnv(t) + t.Setenv("ANTHROPIC_BASE_URL", "https://cc.example.com") + t.Setenv("ANTHROPIC_AUTH_TOKEN", "cc-env-token") + t.Setenv("ANTHROPIC_MODEL", "claude-sonnet-4-6") + cfgPath := writeConfigJSON(t, configFile{ + Llm: llmFileConfig{ + URL: "https://api.example.com/v1/messages", + AuthTokenCmd: "exit 9", + Model: "claude-sonnet-4-6", + }, + }) + ep, err := ResolveEndpoint(cfgPath) + if err == nil { + t.Fatalf("expected hard error from failing auth_token_cmd, got nil (Source %q, Token %q)", ep.Source, ep.Token) + } + if !strings.Contains(err.Error(), "auth_token_cmd") { + t.Errorf("error %q does not mention auth_token_cmd", err.Error()) + } +} + +// (f) an incomplete legacy block (missing url) with auth_token_cmd set does NOT +// run the command and falls through to later strategies. +func TestResolveEndpoint_LegacyIncompleteDoesNotRunCmd(t *testing.T) { + clearAllEnv(t) + // Command would exit non-zero if ever executed; if it ran, we'd see that + // error instead of the generic "no valid endpoint" fall-through error. + cfgPath := writeConfigJSON(t, configFile{ + Llm: llmFileConfig{ + AuthTokenCmd: "exit 9", + Model: "claude-sonnet-4-6", + // URL intentionally omitted -> incomplete + }, + }) + _, err := ResolveEndpoint(cfgPath) + if err == nil { + t.Fatal("expected no-endpoint error, got nil") + } + if strings.Contains(err.Error(), "auth_token_cmd") { + t.Errorf("command should not have run for incomplete legacy config; error: %v", err) + } + if !strings.Contains(err.Error(), "no valid LLM endpoint") { + t.Errorf("expected fall-through no-endpoint error, got: %v", err) + } +} diff --git a/internal/llm/resolver_test.go b/internal/llm/resolver_test.go index ec329788..a59b9c4e 100644 --- a/internal/llm/resolver_test.go +++ b/internal/llm/resolver_test.go @@ -266,6 +266,14 @@ func clearAllEnv(t *testing.T) { } { t.Setenv(k, "") } + // Point os.UserHomeDir at an empty dir so the tryShellRC strategy cannot read + // the developer's (or a self-hosted CI runner's) real ~/.zshrc: one exporting + // the ANTHROPIC_* trio would resolve a live endpoint and break every test that + // asserts resolution fails. HOME covers Unix, USERPROFILE Windows; setting the + // one that does not apply is harmless. + home := t.TempDir() + t.Setenv("HOME", home) + t.Setenv("USERPROFILE", home) } func TestResolveEndpoint_ProviderAnthropic(t *testing.T) { @@ -569,6 +577,38 @@ func TestResolveEndpoint_CustomProviderMissingFields(t *testing.T) { } } +func TestResolveEndpoint_CustomProviderNoEnvFallback(t *testing.T) { + clearAllEnv(t) + // A preset provider would pick this up; a custom provider must not, since it + // has no associated env var. The api_key/api_key_cmd precedence relies on it. + t.Setenv("ANTHROPIC_API_KEY", "env-api-key") + + cfg := configFile{ + Provider: "my-gateway", + CustomProviders: map[string]providerEntryConfig{ + "my-gateway": { + URL: "https://gateway.internal.com/v1", + Protocol: "openai", + Model: "llama-3-70b", + // No api_key and no api_key_cmd. + }, + }, + } + data, _ := json.Marshal(cfg) + cfgPath := filepath.Join(t.TempDir(), "config.json") + if err := os.WriteFile(cfgPath, data, 0644); err != nil { + t.Fatalf("write config: %v", err) + } + + _, err := ResolveEndpoint(cfgPath) + if err == nil { + t.Fatal("expected error: custom providers have no environment variable fallback") + } + if !strings.Contains(err.Error(), "no api_key or api_key_cmd configured") { + t.Errorf("error = %v, want the missing-credential error", err) + } +} + func TestResolveEndpoint_CustomProviderModelFromTopLevel(t *testing.T) { clearAllEnv(t) @@ -732,6 +772,110 @@ func TestResolveEndpointWithModelOverride_InvalidModelInPresetList(t *testing.T) } } +func TestResolveEndpointWithModelOverride_InvalidModelDoesNotRunAPIKeyCmd(t *testing.T) { + clearAllEnv(t) + + // The command is guaranteed to fail, so the error it would produce doubles as + // a witness that it ran: a bad --model must fail on validation instead, with + // no secret-manager prompt. + cfg := configFile{ + Provider: "anthropic", + Providers: map[string]providerEntryConfig{ + "anthropic": {APIKeyCmd: "ocr-no-such-secret-command", Model: "claude-sonnet-4-6"}, + }, + } + data, _ := json.Marshal(cfg) + cfgPath := filepath.Join(t.TempDir(), "config.json") + if err := os.WriteFile(cfgPath, data, 0644); err != nil { + t.Fatalf("write config: %v", err) + } + + _, err := ResolveEndpointWithModelOverride(cfgPath, "claude-opsu-4-6") + if err == nil { + t.Fatal("expected error for invalid model override") + } + if !strings.Contains(err.Error(), "not available for provider") { + t.Errorf("error message should mention model unavailability, got: %v", err) + } + if strings.Contains(err.Error(), "api_key_cmd") { + t.Errorf("api_key_cmd ran before model validation, got: %v", err) + } +} + +// A bad global env override must be rejected before any strategy runs, for the +// same reason as the model check above: OCR_LLM_TIMEOUT="30s" (the field wants a +// bare integer) used to be parsed only after an endpoint resolved, so the user +// authenticated to 1Password/Touch ID and then got a config error. Same witness +// trick: the command cannot succeed, so its error proves it ran. +func TestResolveEndpointWithModelOverride_BadEnvOverrideDoesNotRunAPIKeyCmd(t *testing.T) { + tests := []struct { + name string + env string + value string + wantErr string + wantErr2 string + }{ + { + name: "non-integer timeout", + env: "OCR_LLM_TIMEOUT", + value: "30s", + wantErr: "OCR_LLM_TIMEOUT must be an integer (seconds)", + }, + { + name: "negative timeout", + env: "OCR_LLM_TIMEOUT", + value: "-30", + wantErr: "OCR_LLM_TIMEOUT", + }, + { + name: "reserved extra header", + env: "OCR_LLM_EXTRA_HEADERS", + value: "authorization=leak", + wantErr: "OCR_LLM_EXTRA_HEADERS", + wantErr2: "reserved header", + }, + { + name: "malformed extra header", + env: "OCR_LLM_EXTRA_HEADERS", + value: "no-equals-sign", + wantErr: "OCR_LLM_EXTRA_HEADERS", + wantErr2: "expected key=value", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + clearAllEnv(t) + t.Setenv(tt.env, tt.value) + + cfg := configFile{ + Provider: "anthropic", + Providers: map[string]providerEntryConfig{ + "anthropic": {APIKeyCmd: "ocr-no-such-secret-command", Model: "claude-sonnet-4-6"}, + }, + } + data, _ := json.Marshal(cfg) + cfgPath := filepath.Join(t.TempDir(), "config.json") + if err := os.WriteFile(cfgPath, data, 0644); err != nil { + t.Fatalf("write config: %v", err) + } + + _, err := ResolveEndpoint(cfgPath) + if err == nil { + t.Fatalf("expected error for %s=%q", tt.env, tt.value) + } + if !strings.Contains(err.Error(), tt.wantErr) { + t.Errorf("error %q does not contain %q", err.Error(), tt.wantErr) + } + if tt.wantErr2 != "" && !strings.Contains(err.Error(), tt.wantErr2) { + t.Errorf("error %q does not contain %q", err.Error(), tt.wantErr2) + } + if strings.Contains(err.Error(), "api_key_cmd") { + t.Errorf("api_key_cmd ran before %s was validated, got: %v", tt.env, err) + } + }) + } +} + func TestResolveEndpointWithModelOverride_ValidModelInCustomProviderList(t *testing.T) { clearAllEnv(t) diff --git a/internal/viewer/handler_test.go b/internal/viewer/handler_test.go index c24349ce..53186be7 100644 --- a/internal/viewer/handler_test.go +++ b/internal/viewer/handler_test.go @@ -5,6 +5,7 @@ import ( "net/http/httptest" "os" "path/filepath" + "runtime" "strings" "testing" ) @@ -67,6 +68,12 @@ func TestHandleRepos_UnreadableRoot(t *testing.T) { } func TestHandleRepos_PermissionDenied(t *testing.T) { + // Chmod(0000) on Windows only sets the read-only bit, so ReadDir still + // succeeds and the handler returns 200. (The Getuid guard below cannot cover + // this: Getuid returns -1 on Windows, never 0.) + if runtime.GOOS == "windows" { + t.Skip("unix permissions not enforced on Windows") + } if os.Getuid() == 0 { t.Skip("permission checks are bypassed for root") } diff --git a/internal/viewer/store_load_test.go b/internal/viewer/store_load_test.go index a4da1dbf..d3d0bd5c 100644 --- a/internal/viewer/store_load_test.go +++ b/internal/viewer/store_load_test.go @@ -3,6 +3,7 @@ package viewer import ( "os" "path/filepath" + "runtime" "testing" ) @@ -392,6 +393,11 @@ func TestLoadSession_ToolCallWithoutRequest(t *testing.T) { } func TestDiscoverRepos_SkipsUnreadableSubdir(t *testing.T) { + // Chmod(0000) is only the read-only bit on Windows, so ReadDir still succeeds + // and the repo is discovered rather than skipped. + if runtime.GOOS == "windows" { + t.Skip("unix permissions not enforced on Windows") + } if os.Getuid() == 0 { t.Skip("permission checks are bypassed for root") } @@ -418,6 +424,11 @@ func TestDiscoverRepos_SkipsUnreadableSubdir(t *testing.T) { } func TestListSessions_SkipsUnreadableFiles(t *testing.T) { + // Chmod(0000) is only the read-only bit on Windows, so the "bad" file is still + // readable and gets counted as a second session. + if runtime.GOOS == "windows" { + t.Skip("unix permissions not enforced on Windows") + } if os.Getuid() == 0 { t.Skip("permission checks are bypassed for root") } diff --git a/pages/src/content/docs/en/configuration.md b/pages/src/content/docs/en/configuration.md index 1f3c19e0..29878074 100644 --- a/pages/src/content/docs/en/configuration.md +++ b/pages/src/content/docs/en/configuration.md @@ -125,6 +125,35 @@ The `timeout_sec` keys are not supported by `ocr config set` — edit } ``` +### API key from a command + +Instead of storing a key in the config file, `api_key_cmd` fetches it at +runtime from a secret manager (1Password, `pass`, `gopass`, …). Its trimmed, +single-line stdout becomes the key. The same option is available for the +legacy `llm` block as `auth_token_cmd`. + +```bash +ocr config set providers.anthropic.api_key_cmd "op read op://dev/anthropic/api-key" +``` + +Precedence: a static `api_key` always wins (if both are set, the command is +ignored and a warning is printed); otherwise `api_key_cmd` runs; only if +neither is set does OCR fall back to the provider's environment variable. + +The command runs once per `ocr` invocation and must succeed: a non-zero exit, +empty output, multi-line output, or more than 64KiB of output is a hard error +(OCR never silently falls back). It must complete within 60 seconds, which +includes any time you spend answering a prompt. The command inherits your +terminal's stdin and stderr, so interactive prompts (pinentry, Touch ID) both +appear and can be answered. If the command leaves a background daemon holding +its stdout pipe (`gpg-agent`, a first-use `op` daemon), the credential still +arrives but every `ocr` run pauses an extra 5 seconds waiting for that pipe to +close — redirect the daemon's output (`>/dev/null 2>&1`) to get rid of the wait. + +Since the value is executed as a shell command, `config.json` is trusted +input — keep it owned by you and not writable by anyone else (OCR writes it +with `0600` permissions). + ### Verify connectivity ```bash diff --git a/pages/src/content/docs/ja/configuration.md b/pages/src/content/docs/ja/configuration.md index 64172888..d9403111 100644 --- a/pages/src/content/docs/ja/configuration.md +++ b/pages/src/content/docs/ja/configuration.md @@ -123,6 +123,35 @@ Ollama は API key を無視しますが、カスタム provider は空でない } ``` +### API key をコマンドで取得する + +key を設定ファイルに保存する代わりに、`api_key_cmd` で実行時にシークレット +マネージャー（1Password、`pass`、`gopass` など）から取得できます。前後の空白を +除いた 1 行の stdout が key になります。レガシーの `llm` ブロックにも同等の +`auth_token_cmd` があります。 + +```bash +ocr config set providers.anthropic.api_key_cmd "op read op://dev/anthropic/api-key" +``` + +優先順位：静的な `api_key` が常に優先されます（両方設定されている場合はコマンドを +無視し、警告を表示します）。それ以外の場合は `api_key_cmd` を実行します。どちらも +設定されていない場合のみ、OCR は provider の環境変数にフォールバックします。 + +コマンドは `ocr` 実行ごとに 1 回実行され、成功する必要があります。非ゼロ終了、 +空の出力、複数行の出力、64KiB を超える出力はいずれもハードエラーです（OCR が黙って +フォールバックすることはありません）。コマンドはプロンプトへの応答時間も含めて +60 秒以内に完了する必要があります。コマンドは端末の stdin と stderr を引き継ぐため、 +対話的なプロンプト（pinentry、Touch ID）は表示も応答も可能です。コマンドが stdout +パイプを保持したままバックグラウンドのデーモン（`gpg-agent`、初回起動時の `op` +デーモン）を残すと、認証情報は取得できるものの `ocr` の実行ごとにパイプが閉じるのを +5 秒余分に待つことになるため、デーモンの出力をリダイレクト（`>/dev/null 2>&1`） +してください。 + +この値は shell コマンドとして実行されるため、`config.json` は信頼された入力です。 +自分の所有のまま、他のユーザーが書き込めない状態に保ってください（OCR は `0600` +で書き込みます）。 + ### 接続性を検証する ```bash diff --git a/pages/src/content/docs/zh/configuration.md b/pages/src/content/docs/zh/configuration.md index ffb0914c..a03d8af4 100644 --- a/pages/src/content/docs/zh/configuration.md +++ b/pages/src/content/docs/zh/configuration.md @@ -117,6 +117,30 @@ provider 没有环境变量回退），所以设任意占位值即可。模型 } ``` +### 通过命令获取 API key + +除了把 key 直接写进配置文件，还可以用 `api_key_cmd` 在运行时从密钥管理器 +（1Password、`pass`、`gopass` 等）获取。命令去除首尾空白后的单行 stdout 即为 +key。旧版 `llm` 配置块也有对应的 `auth_token_cmd`。 + +```bash +ocr config set providers.anthropic.api_key_cmd "op read op://dev/anthropic/api-key" +``` + +优先级：静态 `api_key` 始终优先（两者都设置时忽略命令并打印警告）；否则运行 +`api_key_cmd`；只有两者都未设置时，OCR 才回退到 provider 对应的环境变量。 + +命令在每次 `ocr` 调用时运行一次，且必须成功：非零退出、空输出、多行输出或超过 +64KiB 的输出都会被视为硬错误（OCR 绝不会静默回退）。命令须在 60 秒内完成，这也 +包括你回应提示所花的时间。命令会继承你终端的 stdin 和 stderr，因此交互式提示 +（pinentry、Touch ID）既能显示也能作答。如果命令留下了仍持有其 stdout 管道的后台 +守护进程（`gpg-agent`、首次使用时启动的 `op` 守护进程），凭据依然能取到，但每次 +`ocr` 调用都会额外等待 5 秒直到该管道关闭——把守护进程的输出重定向掉 +（`>/dev/null 2>&1`）即可消除这段等待。 + +由于这个值会作为 shell 命令执行，`config.json` 属于可信输入——请确保它归你所有、 +其他用户不可写（OCR 写入时使用 `0600` 权限）。 + ### 验证连通性 ```bash