From 23c0ca1c7738155f59bc2b64cced6e8381f253a8 Mon Sep 17 00:00:00 2001 From: sky5454 Date: Sat, 14 Mar 2026 05:37:11 +0800 Subject: [PATCH] fix(credential): address Copilot review comments on PR #1521 - credential.go: decouple ErrPassphraseRequired from env var name; message is now 'enc:// passphrase required' since PassphraseProvider may come from any source, not just os.Environ - credential.go: Resolver resolves symlinks via EvalSymlinks before the isWithinDir containment check, preventing symlink-based path traversal for file:// credential references - store.go: tighten comment to describe only what SecureStore guarantees (in-memory only); remove claims about how callers transport the value - store_test.go: replace the meaningless GetReturnsCopy test (Go strings are immutable, equality across two calls proves nothing) with TestSecureStore_ConcurrentSetGet that exercises atomic.Pointer under 10-goroutine concurrent Set/Get load - config_test.go: update error-message assertion to match new sentinel text - docs/credential_encryption.md: remove reference to non-existent 'picoclaw encrypt' subcommand; describe the onboard flow instead --- docs/credential_encryption.md | 11 +++++++---- pkg/config/config_test.go | 4 ++-- pkg/credential/credential.go | 37 ++++++++++++++++++++++++----------- pkg/credential/store.go | 4 ++-- pkg/credential/store_test.go | 32 ++++++++++++++++++++++-------- 5 files changed, 61 insertions(+), 27 deletions(-) diff --git a/docs/credential_encryption.md b/docs/credential_encryption.md index 4a8f12457..448eaaa10 100644 --- a/docs/credential_encryption.md +++ b/docs/credential_encryption.md @@ -13,11 +13,14 @@ Encrypted keys are stored as `enc://` strings and decrypted automaticall export PICOCLAW_KEY_PASSPHRASE="your-passphrase" ``` -**2. Encrypt an API key** (using the built-in CLI subcommand, or Go API) +**2. Encrypt an API key** -```bash -picoclaw encrypt sk-your-openai-key -# outputs: enc://AAAA...base64... +Run `picoclaw onboard` — it prompts for your passphrase and generates the SSH key, +then automatically re-encrypts any plaintext `api_key` entries in your config on +the next `SaveConfig` call. The resulting `enc://` value will look like: + +``` +enc://AAAA...base64... ``` **3. Paste the output into your config** diff --git a/pkg/config/config_test.go b/pkg/config/config_test.go index 20dfed891..0ce77ab08 100644 --- a/pkg/config/config_test.go +++ b/pkg/config/config_test.go @@ -886,8 +886,8 @@ func TestLoadConfig_MixedKeys_NoPassphrase(t *testing.T) { if err == nil { t.Fatal("LoadConfig should fail when enc:// key is present and no passphrase is set") } - if !strings.Contains(err.Error(), "PICOCLAW_KEY_PASSPHRASE") { - t.Errorf("error should mention PICOCLAW_KEY_PASSPHRASE, got: %v", err) + if !strings.Contains(err.Error(), "passphrase required") { + t.Errorf("error should mention passphrase required, got: %v", err) } } diff --git a/pkg/credential/credential.go b/pkg/credential/credential.go index 8372a59ad..e584d2415 100644 --- a/pkg/credential/credential.go +++ b/pkg/credential/credential.go @@ -58,9 +58,9 @@ var PassphraseProvider func() string = func() string { } // ErrPassphraseRequired is returned when an enc:// credential is encountered but -// PICOCLAW_KEY_PASSPHRASE is not set. Callers can detect this with errors.Is to -// distinguish a missing-passphrase condition from other credential errors. -var ErrPassphraseRequired = errors.New("credential: enc:// key requires " + PassphraseEnvVar + " env var") +// no passphrase is available from PassphraseProvider. Callers can detect this +// with errors.Is to distinguish a missing-passphrase condition from other errors. +var ErrPassphraseRequired = errors.New("credential: enc:// passphrase required") // ErrDecryptionFailed is returned when an enc:// credential cannot be decrypted, // indicating a wrong passphrase or SSH key. Callers can detect this with errors.Is. @@ -79,13 +79,20 @@ const ( // Resolver resolves raw credential strings for model_list api_key fields. // File references are resolved relative to the directory of the config file. type Resolver struct { - configDir string + configDir string + resolvedConfigDir string // symlink-resolved form of configDir } // NewResolver returns a Resolver that resolves file:// references relative to // configDir (typically filepath.Dir of the config file path). func NewResolver(configDir string) *Resolver { - return &Resolver{configDir: configDir} + resolved := configDir + if configDir != "" { + if real, err := filepath.EvalSymlinks(configDir); err == nil { + resolved = real + } + } + return &Resolver{configDir: configDir, resolvedConfigDir: resolved} } // Resolve returns the actual credential value for raw: @@ -104,19 +111,27 @@ func (r *Resolver) Resolve(raw string) (string, error) { return "", fmt.Errorf("credential: file:// reference has no filename") } - keyPath := filepath.Join(r.configDir, fileName) - // Prevent path traversal: "../../etc/passwd" or "/abs/path" must not escape configDir. - if !isWithinDir(keyPath, r.configDir) { + baseDir := r.resolvedConfigDir + if baseDir == "" { + baseDir = r.configDir + } + keyPath := filepath.Join(baseDir, fileName) + // Resolve symlinks before enforcing containment to prevent escaping via symlinks. + realKeyPath, err := filepath.EvalSymlinks(keyPath) + if err != nil { + return "", fmt.Errorf("credential: failed to resolve credential file path %q: %w", keyPath, err) + } + if !isWithinDir(realKeyPath, baseDir) { return "", fmt.Errorf("credential: file:// path escapes config directory") } - data, err := os.ReadFile(keyPath) + data, err := os.ReadFile(realKeyPath) if err != nil { - return "", fmt.Errorf("credential: failed to read credential file %q: %w", keyPath, err) + return "", fmt.Errorf("credential: failed to read credential file %q: %w", realKeyPath, err) } value := strings.TrimSpace(string(data)) if value == "" { - return "", fmt.Errorf("credential: credential file %q is empty", keyPath) + return "", fmt.Errorf("credential: credential file %q is empty", realKeyPath) } return value, nil diff --git a/pkg/credential/store.go b/pkg/credential/store.go index a9a5f988b..9c72974b0 100644 --- a/pkg/credential/store.go +++ b/pkg/credential/store.go @@ -5,8 +5,8 @@ import "sync/atomic" // SecureStore holds a passphrase in memory. // // Uses atomic.Pointer so reads and writes are lock-free. -// The passphrase is never written to disk or placed in os.Environ; -// it is injected only into the gateway child-process environment via cmd.Env. +// The passphrase is never written to disk; callers decide how to +// transport it outside this store (e.g., via cmd.Env or os.Environ). type SecureStore struct { val atomic.Pointer[string] } diff --git a/pkg/credential/store_test.go b/pkg/credential/store_test.go index 4b0d9988b..63299743a 100644 --- a/pkg/credential/store_test.go +++ b/pkg/credential/store_test.go @@ -1,6 +1,7 @@ package credential import ( + "sync" "testing" ) @@ -51,15 +52,30 @@ func TestSecureStore_EmptyPassphrase(t *testing.T) { } } -func TestSecureStore_GetReturnsCopy(t *testing.T) { +func TestSecureStore_ConcurrentSetGet(t *testing.T) { s := NewSecureStore() - s.SetString("abc") + const goroutines = 10 + const iterations = 1000 - // Mutating the returned string bytes (not possible in Go, but we verify - // that a second Get() is not affected by the first). - got1 := s.Get() - got2 := s.Get() - if got1 != got2 { - t.Errorf("successive Get() calls returned different values: %q vs %q", got1, got2) + var wg sync.WaitGroup + wg.Add(goroutines) + for i := 0; i < goroutines; i++ { + go func(id int) { + defer wg.Done() + for j := 0; j < iterations; j++ { + if id%2 == 0 { + s.SetString("even") + } else { + s.SetString("odd") + } + _ = s.Get() + } + }(i) + } + wg.Wait() + + final := s.Get() + if final != "" && final != "even" && final != "odd" { + t.Errorf("Get() returned unexpected value %q after concurrent Set/Get", final) } }