From 76b959c0d8608746f9dee26a6e19ca39d149f9ab Mon Sep 17 00:00:00 2001 From: ChethanUK Date: Fri, 17 Jul 2026 22:00:52 +0200 Subject: [PATCH 1/6] feat(config): resolve api_key/auth_token from a command (#236) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add `api_key_cmd` (provider entries) and `auth_token_cmd` (legacy llm block) so the LLM credential can be fetched from a secret manager at review time instead of stored plaintext in config.json — same pattern as git credential.helper / AWS credential_process. Resolution precedence (single site, presets and custom providers alike): static api_key always wins (stderr warning if a command is also set) → api_key_cmd → preset env var → error. The legacy llm block gets a mirrored auth_token_cmd; an incomplete legacy block never executes the command, and a set-but-failing command on a complete block is a hard error (never a silent fallback). Command execution is a build-tag split (sh -c / cmd /C) with a 60s timeout; the child's stderr passes through so pinentry/1Password/op prompts stay visible. Stdout is trimmed and used in memory only — never written to config or logged. Empty, whitespace-only, multi-line, and timed-out output are all hard errors. No caching (resolution runs once per process). - config set: api_key_cmd/auth_token_cmd are settable and round-trip; not masked (they are command lines, not secrets). - TUI cloneProviderEntry preserves api_key_cmd. - docs: 'API key from a command' section in configuration.md (en/zh/ja). Tests: table-driven runner matrix (success/trim/non-zero/empty/ whitespace/multi-line/not-found/timeout) + resolver precedence and legacy-fallthrough rows. Coverage 81.3%; Windows arm compile-checked (CI is Linux-only). --- cmd/opencodereview/config_cmd.go | 10 +- cmd/opencodereview/provider_tui.go | 1 + cmd/opencodereview/provider_tui_funcs_test.go | 4 + internal/llm/keycmd.go | 50 ++++++ internal/llm/keycmd_test.go | 72 +++++++++ internal/llm/keycmd_unix.go | 12 ++ internal/llm/keycmd_windows.go | 12 ++ internal/llm/resolver.go | 49 ++++-- internal/llm/resolver_keycmd_test.go | 143 ++++++++++++++++++ pages/src/content/docs/en/configuration.md | 20 +++ pages/src/content/docs/ja/configuration.md | 20 +++ pages/src/content/docs/zh/configuration.md | 16 ++ 12 files changed, 398 insertions(+), 11 deletions(-) create mode 100644 internal/llm/keycmd.go create mode 100644 internal/llm/keycmd_test.go create mode 100644 internal/llm/keycmd_unix.go create mode 100644 internal/llm/keycmd_windows.go create mode 100644 internal/llm/resolver_keycmd_test.go diff --git a/cmd/opencodereview/config_cmd.go b/cmd/opencodereview/config_cmd.go index 64d19190..a0842bb4 100644 --- a/cmd/opencodereview/config_cmd.go +++ b/cmd/opencodereview/config_cmd.go @@ -190,6 +190,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 +229,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 +335,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 +411,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 +420,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 +457,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/provider_tui.go b/cmd/opencodereview/provider_tui.go index b2ba1d17..7d2113f3 100644 --- a/cmd/opencodereview/provider_tui.go +++ b/cmd/opencodereview/provider_tui.go @@ -1181,6 +1181,7 @@ func (m providerTUIModel) applyCreateCustomProvider() (tea.Model, tea.Cmd) { func cloneProviderEntry(v ProviderEntry) ProviderEntry { out := ProviderEntry{ APIKey: v.APIKey, + APIKeyCmd: v.APIKeyCmd, URL: v.URL, Protocol: v.Protocol, Model: v.Model, diff --git a/cmd/opencodereview/provider_tui_funcs_test.go b/cmd/opencodereview/provider_tui_funcs_test.go index 1462b3a7..b32c5663 100644 --- a/cmd/opencodereview/provider_tui_funcs_test.go +++ b/cmd/opencodereview/provider_tui_funcs_test.go @@ -198,6 +198,7 @@ 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", @@ -210,6 +211,9 @@ func TestCloneProviderEntry_WithExtraBody(t *testing.T) { 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) } diff --git a/internal/llm/keycmd.go b/internal/llm/keycmd.go new file mode 100644 index 00000000..cd6ee2ba --- /dev/null +++ b/internal/llm/keycmd.go @@ -0,0 +1,50 @@ +package llm + +import ( + "context" + "fmt" + "os" + "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 + +// 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. 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 + + out, err := c.Output() + if ctx.Err() == context.DeadlineExceeded { + return "", fmt.Errorf("%s timed out after %s", label, keyCmdTimeout) + } + if err != nil { + // Covers non-zero exit and command-not-found (the shell exits non-zero + // and prints its not-found message on the child's stderr). + return "", fmt.Errorf("%s failed: %w", label, err) + } + + // Trim a trailing line break; multi-line output past that is ambiguous and refused. + trimmed := strings.TrimRight(string(out), "\r\n") + if strings.Contains(trimmed, "\n") { + return "", fmt.Errorf("%s produced multi-line output; expected a single credential", label) + } + 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..1ba66d42 --- /dev/null +++ b/internal/llm/keycmd_test.go @@ -0,0 +1,72 @@ +//go:build !windows + +package llm + +import ( + "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: "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"}, + {name: "command not found", cmd: "this-cmd-does-not-exist-xyz", wantErr: "failed:"}, + } + for _, tt := range tests { + tt := tt + 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) { + orig := keyCmdTimeout + keyCmdTimeout = 50 * time.Millisecond + t.Cleanup(func() { keyCmdTimeout = orig }) + + _, err := resolveKeyCmd("sleep 5", "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()) + } +} + +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..81d3ebb3 --- /dev/null +++ b/internal/llm/keycmd_unix.go @@ -0,0 +1,12 @@ +//go:build !windows + +package llm + +import ( + "context" + "os/exec" +) + +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..6253c330 --- /dev/null +++ b/internal/llm/keycmd_windows.go @@ -0,0 +1,12 @@ +//go:build windows + +package llm + +import ( + "context" + "os/exec" +) + +func newKeyCmd(ctx context.Context, cmd string) *exec.Cmd { + return exec.CommandContext(ctx, "cmd", "/C", cmd) +} diff --git a/internal/llm/resolver.go b/internal/llm/resolver.go index 853f42fd..6c7ffaa2 100644 --- a/internal/llm/resolver.go +++ b/internal/llm/resolver.go @@ -212,9 +212,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 +223,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"` @@ -282,13 +284,24 @@ func tryProviderConfig(cfg configFile, modelOverride string) (ResolvedEndpoint, } apiKey := entry.APIKey - if apiKey == "" { - if isPreset && preset.EnvVar != "" { - apiKey = os.Getenv(preset.EnvVar) + 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, "warning: provider %q has both api_key and api_key_cmd set; using the static api_key\n", cfg.Provider) + } + case entry.APIKeyCmd != "": + resolved, err := resolveKeyCmd(entry.APIKeyCmd, fmt.Sprintf("api_key_cmd for provider %q", cfg.Provider)) + if err != nil { + return ResolvedEndpoint{}, false, err } + apiKey = resolved + case isPreset && preset.EnvVar != "": + apiKey = os.Getenv(preset.EnvVar) } if apiKey == "" { - return ResolvedEndpoint{}, false, fmt.Errorf("provider %q has no api_key configured and no environment variable fallback found", cfg.Provider) + 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 @@ -408,9 +421,27 @@ 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. + token := cfg.Llm.AuthToken + if cfg.Llm.URL == "" || model == "" || (token == "" && cfg.Llm.AuthTokenCmd == "") { return ResolvedEndpoint{}, false, nil } + switch { + case token != "": + // Static auth_token always wins; warn if a command is also set. + if cfg.Llm.AuthTokenCmd != "" { + fmt.Fprintf(os.Stderr, "warning: llm config has both auth_token and auth_token_cmd set; using the static auth_token\n") + } + case cfg.Llm.AuthTokenCmd != "": + // Otherwise-complete legacy block with a set-but-failing command is a hard error. + resolved, err := resolveKeyCmd(cfg.Llm.AuthTokenCmd, "auth_token_cmd for llm config") + if err != nil { + return ResolvedEndpoint{}, false, err + } + token = resolved + } // llm.protocol (normalized) wins over use_anthropic when set. protocol := "" @@ -449,7 +480,7 @@ 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 + 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..f6b71582 --- /dev/null +++ b/internal/llm/resolver_keycmd_test.go @@ -0,0 +1,143 @@ +package llm + +import ( + "encoding/json" + "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") + } +} + +// (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") + } +} + +// (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()) + } +} + +// (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") + } +} + +// (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/pages/src/content/docs/en/configuration.md b/pages/src/content/docs/en/configuration.md index 1f3c19e0..2ffc2f70 100644 --- a/pages/src/content/docs/en/configuration.md +++ b/pages/src/content/docs/en/configuration.md @@ -124,6 +124,26 @@ 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, or multi-line output is a hard error (OCR never silently falls +back). It must complete within 60 seconds. The command's stderr is passed +through to your terminal, so interactive prompts (pinentry, Touch ID) still +work. ### Verify connectivity diff --git a/pages/src/content/docs/ja/configuration.md b/pages/src/content/docs/ja/configuration.md index 64172888..0102f0db 100644 --- a/pages/src/content/docs/ja/configuration.md +++ b/pages/src/content/docs/ja/configuration.md @@ -122,6 +122,26 @@ 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 回実行され、成功する必要があります。非ゼロ終了、 +空の出力、複数行の出力はいずれもハードエラーです（OCR が黙ってフォールバックする +ことはありません）。コマンドは 60 秒以内に完了する必要があります。コマンドの +stderr は端末へそのまま渡されるため、対話的なプロンプト（pinentry、Touch ID）も +引き続き動作します。 ### 接続性を検証する diff --git a/pages/src/content/docs/zh/configuration.md b/pages/src/content/docs/zh/configuration.md index ffb0914c..5e59f9e8 100644 --- a/pages/src/content/docs/zh/configuration.md +++ b/pages/src/content/docs/zh/configuration.md @@ -116,6 +116,22 @@ 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` 调用时运行一次，且必须成功：非零退出、空输出或多行输出都会 +被视为硬错误（OCR 绝不会静默回退）。命令须在 60 秒内完成。命令的 stderr 会透传 +到你的终端，因此交互式提示（pinentry、Touch ID）仍可正常工作。 ### 验证连通性 From a102fda13819ecbdd221b62919022150b3daf0b0 Mon Sep 17 00:00:00 2001 From: ChethanUK Date: Thu, 30 Jul 2026 09:49:47 +0200 Subject: [PATCH 2/6] fix(config): cloneProviderEntry drops timeout_sec and extra_headers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit cloneProviderEntry copied fields by hand and was never updated when TimeoutSec and ExtraHeaders were added to ProviderEntry, so editing any provider through `ocr config provider` silently erased both from the saved config: a per-provider timeout_sec of 900 reverted to the 300s default and custom extra_headers disappeared. Return the literal directly and use maps.Clone for both maps, which also preserves nil (matching each field's omitempty) where the old hand-rolled loop only special-cased ExtraBody. The regression test walks ProviderEntry with reflection and fails if the fixture leaves any field zero, so the next added field cannot be dropped without the DeepEqual catching it. Pre-existing bug, independent of the api_key_cmd work in this branch — separate commit so it can be bisected or cherry-picked on its own. --- cmd/opencodereview/provider_tui.go | 16 ++++------ cmd/opencodereview/provider_tui_funcs_test.go | 32 +++++++++++++++++++ 2 files changed, 39 insertions(+), 9 deletions(-) diff --git a/cmd/opencodereview/provider_tui.go b/cmd/opencodereview/provider_tui.go index 7d2113f3..17cd6f7b 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" @@ -1179,7 +1180,7 @@ 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, @@ -1187,15 +1188,12 @@ func cloneProviderEntry(v ProviderEntry) ProviderEntry { 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 { diff --git a/cmd/opencodereview/provider_tui_funcs_test.go b/cmd/opencodereview/provider_tui_funcs_test.go index b32c5663..779d9863 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" @@ -246,6 +247,37 @@ func TestCloneProviderEntry_NilExtraBody(t *testing.T) { } } +// TestCloneProviderEntry_CopiesEveryField fails when a field is added to +// ProviderEntry but not to cloneProviderEntry -- the way TimeoutSec and +// ExtraHeaders were silently dropped. DeepEqual catches a dropped field; the +// reflect sweep is what stops a zero-valued fixture from hiding one. +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) { cfg := &Config{ CustomProviders: map[string]ProviderEntry{ From ae88821ab0e5067dbb64aabd5f4989fcac8dc135 Mon Sep 17 00:00:00 2001 From: ChethanUK Date: Thu, 30 Jul 2026 10:12:52 +0200 Subject: [PATCH 3/6] fix(llm): harden the credential command and cover it on Windows MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up hardening on the api_key_cmd/auth_token_cmd path, plus the CI job that actually exercises its Windows arm. The 60s timeout was not a real bound. It killed the shell, but a helper that leaves a background process holding the inherited stdout pipe (gpg-agent, pinentry, a first-use `op` daemon) kept Cmd.Wait blocked on the read long after the context died — `api_key_cmd = "sleep 200 & printf tok"` hung for over 90s. Buffer stdout through a writer os/exec copies in its own goroutine and set WaitDelay, which is what lets Wait force the pipe closed; ErrWaitDelay on its own is not a failure, since the command exited and its output is already buffered. Three more ways a resolved value could not be used: - Stdin was /dev/null, so a helper needing a passphrase saw EOF or refused to prompt for lack of a tty. Wired to os.Stdin, which is safe because no path resolves an endpoint while the bubbletea TUI is reading stdin. - Output was unbounded; `cat /dev/urandom` grew the heap without limit. Capped at 64KiB, refusing the write so the child dies of SIGPIPE. - Control bytes reached the Authorization header, where net/http rejects them as an opaque `invalid header field value`. Rejected up front with the offending byte and offset, matching httpguts.ValidHeaderFieldValue. A lone interior CR survived both TrimRight and TrimSpace, so it is now caught as multi-line output. Ordering: the command ran before the rest of the config was known to be usable, so `ocr review --model nonexistent` fired a biometric prompt and only then failed on the model name. Execution is deferred past validation at both sites — the source selection in tryProviderConfig, and ResolveEndpointWithModelOverride, which parsed OCR_LLM_TIMEOUT and OCR_LLM_EXTRA_HEADERS after resolving the credential. A whitespace-only static api_key also used to win precedence over a working api_key_cmd and send `Authorization: Bearer `; it now normalizes to unset, and the Manual TUI tab trims its token like the other two tabs. `ocr config provider` rejected api_key_cmd-only providers in both directions: non-interactively applyOfficialProviderConfig demanded a static key or an env var, and interactively the API-key step could not be confirmed because the field renders blank for such a provider. Both now treat a configured command as satisfying the requirement, and the error messages name the option that would fix it. Windows: the command line goes to cmd.exe through SysProcAttr.CmdLine with /S rather than through Args, because os/exec quotes Args with syscall.EscapeArg, which targets CommandLineToArgvW; cmd.exe is a documented exception whose escaping mangles any command containing a double quote, so `op read "op://Private/My Vault/api-key"` arrived as a single literal filename. Args stays at its one-element default rather than nil (syscall.StartProcess ignores argv when CmdLine is set) so Cmd.String() cannot panic on Args[1:]. CI ran only self-hosted Linux, and the cross-compile job proves the windows arms compile but never runs them, so keycmd_windows.go had zero coverage on any platform. Adds a windows-latest job that vets, tests, builds and smoke-tests natively. It installs Go with setup-go instead of the shared golang:1.26.5 image because GitHub does not support `container:` on Windows runners (actions/runner#904); no -race, since the detector needs a C toolchain there and races are OS-independent; no coverage gate, since the //go:build !windows files legitimately put the total under the Linux job's 80%. Six existing tests needed a guard for that job, none a behavior change: three assert an unreadable path is skipped, but Chmod(0000) on Windows only sets the read-only bit (and their os.Getuid() == 0 guard cannot cover it, since Getuid returns -1 there); TestSaveConfig asserts the 0600 the config is written with, which Windows reports as 0666; the symlink-safety test needs a privilege an unelevated CI account lacks; and the "absolute unchanged" background-path case was passing a rooted but non-absolute path, so it had been exercising the relative branch. Docs (en/zh/ja) spell out the failure modes, the 60s budget including the time spent answering a prompt, the inherited stdin/stderr, the extra 5s a daemon holding the pipe costs, and that config.json is trusted input because the value is executed as a shell command. --- .github/workflows/ci.yml | 49 +++ cmd/opencodereview/background_file_test.go | 8 + cmd/opencodereview/config_cmd.go | 12 +- cmd/opencodereview/config_cmd_test.go | 50 ++++ cmd/opencodereview/flags.go | 4 +- cmd/opencodereview/flags_test.go | 54 ++++ cmd/opencodereview/provider_cmd.go | 12 +- cmd/opencodereview/provider_cmd_test.go | 72 ++++- cmd/opencodereview/provider_tui.go | 58 +++- cmd/opencodereview/provider_tui_funcs_test.go | 280 +++++++++++++----- cmd/opencodereview/provider_tui_test.go | 16 +- internal/config/rules/system_rules_test.go | 5 +- internal/llm/keycmd.go | 100 ++++++- internal/llm/keycmd_test.go | 96 +++++- internal/llm/keycmd_unix.go | 2 + internal/llm/keycmd_windows.go | 28 +- internal/llm/keycmd_windows_test.go | 139 +++++++++ internal/llm/resolver.go | 115 ++++--- internal/llm/resolver_keycmd_test.go | 247 +++++++++++++++ internal/llm/resolver_test.go | 144 +++++++++ internal/viewer/handler_test.go | 7 + internal/viewer/store_load_test.go | 11 + pages/src/content/docs/en/configuration.md | 17 +- pages/src/content/docs/ja/configuration.md | 17 +- pages/src/content/docs/zh/configuration.md | 14 +- 25 files changed, 1405 insertions(+), 152 deletions(-) create mode 100644 internal/llm/keycmd_windows_test.go 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 a0842bb4..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] == "" { 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 17cd6f7b..491e3c98 100644 --- a/cmd/opencodereview/provider_tui.go +++ b/cmd/opencodereview/provider_tui.go @@ -905,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) { @@ -919,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) { @@ -926,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" } @@ -1044,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 { @@ -1605,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 } @@ -1907,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 779d9863..61765044 100644 --- a/cmd/opencodereview/provider_tui_funcs_test.go +++ b/cmd/opencodereview/provider_tui_funcs_test.go @@ -205,7 +205,11 @@ func TestCloneProviderEntry_WithExtraBody(t *testing.T) { 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) @@ -234,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) { @@ -245,12 +265,17 @@ 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") + } } -// TestCloneProviderEntry_CopiesEveryField fails when a field is added to -// ProviderEntry but not to cloneProviderEntry -- the way TimeoutSec and -// ExtraHeaders were silently dropped. DeepEqual catches a dropped field; the -// reflect sweep is what stops a zero-valued fixture from hiding one. +// 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", @@ -1812,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 index cd6ee2ba..0060c88e 100644 --- a/internal/llm/keycmd.go +++ b/internal/llm/keycmd.go @@ -1,9 +1,12 @@ package llm import ( + "bytes" "context" + "errors" "fmt" "os" + "os/exec" "strings" "time" ) @@ -12,36 +15,113 @@ import ( // 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. 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. +// (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 - out, err := c.Output() + 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 { - return "", fmt.Errorf("%s timed out after %s", label, keyCmdTimeout) + // 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) } - if err != nil { + // 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). + // 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. - trimmed := strings.TrimRight(string(out), "\r\n") - if strings.Contains(trimmed, "\n") { - return "", fmt.Errorf("%s produced multi-line output; expected a single credential", label) + // 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) diff --git a/internal/llm/keycmd_test.go b/internal/llm/keycmd_test.go index 1ba66d42..ddf7263f 100644 --- a/internal/llm/keycmd_test.go +++ b/internal/llm/keycmd_test.go @@ -3,6 +3,7 @@ package llm import ( + "os" "strings" "testing" "time" @@ -18,16 +19,35 @@ func TestResolveKeyCmd(t *testing.T) { {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 { - tt := tt t.Run(tt.name, func(t *testing.T) { t.Parallel() got, err := resolveKeyCmd(tt.cmd, "api_key_cmd for provider \"x\"") @@ -51,11 +71,14 @@ func TestResolveKeyCmd(t *testing.T) { } func TestResolveKeyCmd_Timeout(t *testing.T) { - orig := keyCmdTimeout + origTimeout, origDelay := keyCmdTimeout, keyCmdWaitDelay keyCmdTimeout = 50 * time.Millisecond - t.Cleanup(func() { keyCmdTimeout = orig }) + // `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", "api_key_cmd for provider \"x\"") + _, err := resolveKeyCmd("sleep 5 2>/dev/null", "api_key_cmd for provider \"x\"") if err == nil { t.Fatal("expected timeout error, got nil") } @@ -64,6 +87,71 @@ func TestResolveKeyCmd_Timeout(t *testing.T) { } } +// 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") { diff --git a/internal/llm/keycmd_unix.go b/internal/llm/keycmd_unix.go index 81d3ebb3..03e91beb 100644 --- a/internal/llm/keycmd_unix.go +++ b/internal/llm/keycmd_unix.go @@ -7,6 +7,8 @@ import ( "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 index 6253c330..8f22cf89 100644 --- a/internal/llm/keycmd_windows.go +++ b/internal/llm/keycmd_windows.go @@ -5,8 +5,34 @@ 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. +// +// 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 { - return exec.CommandContext(ctx, "cmd", "/C", 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..14a80df5 --- /dev/null +++ b/internal/llm/keycmd_windows_test.go @@ -0,0 +1,139 @@ +//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"}, + {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 6c7ffaa2..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 { @@ -283,24 +293,38 @@ 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 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, "warning: provider %q has both api_key and api_key_cmd set; using the static api_key\n", cfg.Provider) + 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 != "": - resolved, err := resolveKeyCmd(entry.APIKeyCmd, fmt.Sprintf("api_key_cmd for provider %q", cfg.Provider)) - if err != nil { - return ResolvedEndpoint{}, false, err + 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 } - apiKey = resolved - case isPreset && preset.EnvVar != "": - apiKey = os.Getenv(preset.EnvVar) } - if apiKey == "" { + // 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) } @@ -402,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, @@ -424,23 +460,19 @@ func tryLegacyLlmConfig(cfg configFile, modelOverride string) (ResolvedEndpoint, // 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 } - switch { - case token != "": - // Static auth_token always wins; warn if a command is also set. - if cfg.Llm.AuthTokenCmd != "" { - fmt.Fprintf(os.Stderr, "warning: llm config has both auth_token and auth_token_cmd set; using the static auth_token\n") - } - case cfg.Llm.AuthTokenCmd != "": - // Otherwise-complete legacy block with a set-but-failing command is a hard error. - resolved, err := resolveKeyCmd(cfg.Llm.AuthTokenCmd, "auth_token_cmd for llm config") - if err != nil { - return ResolvedEndpoint{}, false, err - } - token = resolved + // 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. @@ -480,6 +512,17 @@ func tryLegacyLlmConfig(cfg configFile, modelOverride string) (ResolvedEndpoint, return ResolvedEndpoint{}, false, fmt.Errorf("OCR config file: %w", err) } + // 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 } diff --git a/internal/llm/resolver_keycmd_test.go b/internal/llm/resolver_keycmd_test.go index f6b71582..608a93b1 100644 --- a/internal/llm/resolver_keycmd_test.go +++ b/internal/llm/resolver_keycmd_test.go @@ -1,7 +1,13 @@ +//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" @@ -39,6 +45,37 @@ func TestResolveEndpoint_ProviderAPIKeyCmd(t *testing.T) { } } +// (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) @@ -57,6 +94,164 @@ func TestResolveEndpoint_ProviderStaticKeyWinsOverCmd(t *testing.T) { } } +// 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) @@ -98,6 +293,33 @@ func TestResolveEndpoint_ProviderAPIKeyCmdFailsHard(t *testing.T) { } } +// (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) @@ -117,6 +339,31 @@ func TestResolveEndpoint_LegacyAuthTokenCmd(t *testing.T) { } } +// (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) { 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 2ffc2f70..29878074 100644 --- a/pages/src/content/docs/en/configuration.md +++ b/pages/src/content/docs/en/configuration.md @@ -124,6 +124,7 @@ 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 @@ -140,10 +141,18 @@ 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, or multi-line output is a hard error (OCR never silently falls -back). It must complete within 60 seconds. The command's stderr is passed -through to your terminal, so interactive prompts (pinentry, Touch ID) still -work. +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 diff --git a/pages/src/content/docs/ja/configuration.md b/pages/src/content/docs/ja/configuration.md index 0102f0db..d9403111 100644 --- a/pages/src/content/docs/ja/configuration.md +++ b/pages/src/content/docs/ja/configuration.md @@ -122,6 +122,7 @@ Ollama は API key を無視しますが、カスタム provider は空でない } } ``` + ### API key をコマンドで取得する key を設定ファイルに保存する代わりに、`api_key_cmd` で実行時にシークレット @@ -138,10 +139,18 @@ ocr config set providers.anthropic.api_key_cmd "op read op://dev/anthropic/api-k 設定されていない場合のみ、OCR は provider の環境変数にフォールバックします。 コマンドは `ocr` 実行ごとに 1 回実行され、成功する必要があります。非ゼロ終了、 -空の出力、複数行の出力はいずれもハードエラーです（OCR が黙ってフォールバックする -ことはありません）。コマンドは 60 秒以内に完了する必要があります。コマンドの -stderr は端末へそのまま渡されるため、対話的なプロンプト（pinentry、Touch ID）も -引き続き動作します。 +空の出力、複数行の出力、64KiB を超える出力はいずれもハードエラーです（OCR が黙って +フォールバックすることはありません）。コマンドはプロンプトへの応答時間も含めて +60 秒以内に完了する必要があります。コマンドは端末の stdin と stderr を引き継ぐため、 +対話的なプロンプト（pinentry、Touch ID）は表示も応答も可能です。コマンドが stdout +パイプを保持したままバックグラウンドのデーモン（`gpg-agent`、初回起動時の `op` +デーモン）を残すと、認証情報は取得できるものの `ocr` の実行ごとにパイプが閉じるのを +5 秒余分に待つことになるため、デーモンの出力をリダイレクト（`>/dev/null 2>&1`） +してください。 + +この値は shell コマンドとして実行されるため、`config.json` は信頼された入力です。 +自分の所有のまま、他のユーザーが書き込めない状態に保ってください（OCR は `0600` +で書き込みます）。 ### 接続性を検証する diff --git a/pages/src/content/docs/zh/configuration.md b/pages/src/content/docs/zh/configuration.md index 5e59f9e8..a03d8af4 100644 --- a/pages/src/content/docs/zh/configuration.md +++ b/pages/src/content/docs/zh/configuration.md @@ -116,6 +116,7 @@ provider 没有环境变量回退），所以设任意占位值即可。模型 } } ``` + ### 通过命令获取 API key 除了把 key 直接写进配置文件，还可以用 `api_key_cmd` 在运行时从密钥管理器 @@ -129,9 +130,16 @@ ocr config set providers.anthropic.api_key_cmd "op read op://dev/anthropic/api-k 优先级：静态 `api_key` 始终优先（两者都设置时忽略命令并打印警告）；否则运行 `api_key_cmd`；只有两者都未设置时，OCR 才回退到 provider 对应的环境变量。 -命令在每次 `ocr` 调用时运行一次，且必须成功：非零退出、空输出或多行输出都会 -被视为硬错误（OCR 绝不会静默回退）。命令须在 60 秒内完成。命令的 stderr 会透传 -到你的终端，因此交互式提示（pinentry、Touch ID）仍可正常工作。 +命令在每次 `ocr` 调用时运行一次，且必须成功：非零退出、空输出、多行输出或超过 +64KiB 的输出都会被视为硬错误（OCR 绝不会静默回退）。命令须在 60 秒内完成，这也 +包括你回应提示所花的时间。命令会继承你终端的 stdin 和 stderr，因此交互式提示 +（pinentry、Touch ID）既能显示也能作答。如果命令留下了仍持有其 stdout 管道的后台 +守护进程（`gpg-agent`、首次使用时启动的 `op` 守护进程），凭据依然能取到，但每次 +`ocr` 调用都会额外等待 5 秒直到该管道关闭——把守护进程的输出重定向掉 +（`>/dev/null 2>&1`）即可消除这段等待。 + +由于这个值会作为 shell 命令执行，`config.json` 属于可信输入——请确保它归你所有、 +其他用户不可写（OCR 写入时使用 `0600` 权限）。 ### 验证连通性 From 7a5ee5478670c430a60f5d90f2978e2f31b3f802 Mon Sep 17 00:00:00 2001 From: ChethanUK Date: Thu, 30 Jul 2026 10:56:20 +0200 Subject: [PATCH 4/6] probe: record real cmd.exe /S /C quote semantics (do not merge) --- internal/llm/keycmd_windows_probe_test.go | 34 +++++++++++++++++++++++ 1 file changed, 34 insertions(+) create mode 100644 internal/llm/keycmd_windows_probe_test.go diff --git a/internal/llm/keycmd_windows_probe_test.go b/internal/llm/keycmd_windows_probe_test.go new file mode 100644 index 00000000..21a8476b --- /dev/null +++ b/internal/llm/keycmd_windows_probe_test.go @@ -0,0 +1,34 @@ +//go:build windows + +package llm + +import "testing" + +// TEMPORARY PROBE -- not for merge. Records what cmd.exe /S /C actually does +// with the quote shapes from the PR 605 review finding, so the answer is an +// observation rather than a reading of `cmd /?`. +// +// resolveKeyCmd reports "produced multi-line output" whenever the child printed +// two lines, which is exactly the signal for "cmd split my command line into two +// commands", so the returned error distinguishes the cases on its own. +func TestProbe_CmdExeQuoteSemantics(t *testing.T) { + probes := []struct { + name string + cmd string + }{ + // The reviewer's PoC shape: trailing `& "` after a bare closing quote. + {name: "poc_from_review", cmd: `echo A" & echo B & "`}, + // Doubled quote closes the region, so the following & should be unquoted. + {name: "doubled_quote_then_amp", cmd: `echo A"" & echo B`}, + // Unbalanced single quote, nothing after it. + {name: "unbalanced_trailing", cmd: `echo A" & echo B`}, + // Baseline: the documented working case. + {name: "baseline_quoted_arg", cmd: `echo sk-"a b"-token`}, + } + for _, p := range probes { + t.Run(p.name, func(t *testing.T) { + got, err := resolveKeyCmd(p.cmd, "probe") + t.Logf("PROBE %-24s cmd=%q\n -> out=%q\n -> err=%v", p.name, p.cmd, got, err) + }) + } +} From 9ef7531974eb3588b1af9c42ee451eac36f9e3b2 Mon Sep 17 00:00:00 2001 From: ChethanUK Date: Thu, 30 Jul 2026 11:01:15 +0200 Subject: [PATCH 5/6] probe: force output via Errorf (Logf is swallowed without -v) --- internal/llm/keycmd_windows_probe_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/llm/keycmd_windows_probe_test.go b/internal/llm/keycmd_windows_probe_test.go index 21a8476b..7348e0fc 100644 --- a/internal/llm/keycmd_windows_probe_test.go +++ b/internal/llm/keycmd_windows_probe_test.go @@ -28,7 +28,7 @@ func TestProbe_CmdExeQuoteSemantics(t *testing.T) { for _, p := range probes { t.Run(p.name, func(t *testing.T) { got, err := resolveKeyCmd(p.cmd, "probe") - t.Logf("PROBE %-24s cmd=%q\n -> out=%q\n -> err=%v", p.name, p.cmd, got, err) + t.Errorf("PROBE %-24s cmd=%q\n -> out=%q\n -> err=%v", p.name, p.cmd, got, err) }) } } From e04f3b07ac9e51563397244a74fb46153e983a86 Mon Sep 17 00:00:00 2001 From: ChethanUK Date: Thu, 30 Jul 2026 11:05:12 +0200 Subject: [PATCH 6/6] test(llm): pin cmd.exe /S quote semantics; document the trust model --- internal/llm/keycmd_windows.go | 9 ++++++ internal/llm/keycmd_windows_probe_test.go | 34 ----------------------- internal/llm/keycmd_windows_test.go | 13 +++++++++ 3 files changed, 22 insertions(+), 34 deletions(-) delete mode 100644 internal/llm/keycmd_windows_probe_test.go diff --git a/internal/llm/keycmd_windows.go b/internal/llm/keycmd_windows.go index 8f22cf89..5e71e7b1 100644 --- a/internal/llm/keycmd_windows.go +++ b/internal/llm/keycmd_windows.go @@ -20,6 +20,15 @@ import ( // 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. diff --git a/internal/llm/keycmd_windows_probe_test.go b/internal/llm/keycmd_windows_probe_test.go deleted file mode 100644 index 7348e0fc..00000000 --- a/internal/llm/keycmd_windows_probe_test.go +++ /dev/null @@ -1,34 +0,0 @@ -//go:build windows - -package llm - -import "testing" - -// TEMPORARY PROBE -- not for merge. Records what cmd.exe /S /C actually does -// with the quote shapes from the PR 605 review finding, so the answer is an -// observation rather than a reading of `cmd /?`. -// -// resolveKeyCmd reports "produced multi-line output" whenever the child printed -// two lines, which is exactly the signal for "cmd split my command line into two -// commands", so the returned error distinguishes the cases on its own. -func TestProbe_CmdExeQuoteSemantics(t *testing.T) { - probes := []struct { - name string - cmd string - }{ - // The reviewer's PoC shape: trailing `& "` after a bare closing quote. - {name: "poc_from_review", cmd: `echo A" & echo B & "`}, - // Doubled quote closes the region, so the following & should be unquoted. - {name: "doubled_quote_then_amp", cmd: `echo A"" & echo B`}, - // Unbalanced single quote, nothing after it. - {name: "unbalanced_trailing", cmd: `echo A" & echo B`}, - // Baseline: the documented working case. - {name: "baseline_quoted_arg", cmd: `echo sk-"a b"-token`}, - } - for _, p := range probes { - t.Run(p.name, func(t *testing.T) { - got, err := resolveKeyCmd(p.cmd, "probe") - t.Errorf("PROBE %-24s cmd=%q\n -> out=%q\n -> err=%v", p.name, p.cmd, got, err) - }) - } -} diff --git a/internal/llm/keycmd_windows_test.go b/internal/llm/keycmd_windows_test.go index 14a80df5..3cfa91d1 100644 --- a/internal/llm/keycmd_windows_test.go +++ b/internal/llm/keycmd_windows_test.go @@ -54,6 +54,19 @@ func TestResolveKeyCmd(t *testing.T) { {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 {