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

@@ -13,11 +13,14 @@ Encrypted keys are stored as `enc://<base64>` 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
+Run `picoclaw onboard` — it prompts for your passphrase and generates the SSH key,
-picoclaw encrypt sk-your-openai-key
+then automatically re-encrypts any plaintext `api_key` entries in your config on
-# outputs: enc://AAAA...base64...
+the next `SaveConfig` call. The resulting `enc://` value will look like:
+
+```
+enc://AAAA...base64...
 ```
 
 **3. Paste the output into your config**

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") {
+	if !strings.Contains(err.Error(), "passphrase required") {
-		t.Errorf("error should mention PICOCLAW_KEY_PASSPHRASE, got: %v", err)
+		t.Errorf("error should mention passphrase required, got: %v", err)
 	}
 }
 

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
+// no passphrase is available from PassphraseProvider. Callers can detect this
-// distinguish a missing-passphrase condition from other credential errors.
+// with errors.Is to distinguish a missing-passphrase condition from other errors.
-var ErrPassphraseRequired = errors.New("credential: enc:// key requires " + PassphraseEnvVar + " env var")
+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)
+		baseDir := r.resolvedConfigDir
-		// Prevent path traversal: "../../etc/passwd" or "/abs/path" must not escape configDir.
+		if baseDir == "" {
-		if !isWithinDir(keyPath, r.configDir) {
+			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

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;
+// The passphrase is never written to disk; callers decide how to
-// it is injected only into the gateway child-process environment via cmd.Env.
+// transport it outside this store (e.g., via cmd.Env or os.Environ).
 type SecureStore struct {
 	val atomic.Pointer[string]
 }

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
+	var wg sync.WaitGroup
-	// that a second Get() is not affected by the first).
+	wg.Add(goroutines)
-	got1 := s.Get()
+	for i := 0; i < goroutines; i++ {
-	got2 := s.Get()
+		go func(id int) {
-	if got1 != got2 {
+			defer wg.Done()
-		t.Errorf("successive Get() calls returned different values: %q vs %q", got1, got2)
+			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)
 	}
 }