Merge branch 'main' into feat/source-build-formula

Author: kshahbw

cmd/auth/auth_test.go

@@ -3,7 +3,10 @@ package auth
 import (
 	"encoding/base64"
 	"encoding/json"
+	"path/filepath"
 	"testing"
+
+	"github.com/Bandwidth/cli/internal/config"
 )
 
 func TestCmdStructure(t *testing.T) {
@@ -153,3 +156,52 @@ func TestParseJWTClaimsInvalidPayload(t *testing.T) {
 		t.Fatal("expected error for invalid base64 payload")
 	}
 }
+
+// TestRunSwitch_PersistsTargetIntoActiveProfile guards against the bug where
+// switch only updated the legacy top-level cfg.AccountID, leaving the active
+// profile's AccountID stale — so subsequent commands continued targeting the
+// pre-switch account.
+func TestRunSwitch_PersistsTargetIntoActiveProfile(t *testing.T) {
+	home := t.TempDir()
+	t.Setenv("HOME", home)
+	// On macOS, UserHomeDir checks HOME first, but ensure XDG_CONFIG_HOME isn't
+	// pointing somewhere else for this test.
+	t.Setenv("XDG_CONFIG_HOME", filepath.Join(home, ".config"))
+
+	cfgPath, err := config.DefaultPath()
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	cfg := &config.Config{Format: "json"}
+	cfg.SetProfile("default", &config.Profile{
+		ClientID:  "id1",
+		AccountID: "ACCT_A",
+		Accounts:  []string{"ACCT_A", "ACCT_B"},
+	})
+	if err := config.Save(cfgPath, cfg); err != nil {
+		t.Fatal(err)
+	}
+
+	if err := runSwitch(nil, []string{"ACCT_B"}); err != nil {
+		t.Fatalf("runSwitch returned error: %v", err)
+	}
+
+	loaded, err := config.Load(cfgPath)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	p := loaded.Profiles["default"]
+	if p == nil {
+		t.Fatal("default profile missing after switch")
+	}
+	if p.AccountID != "ACCT_B" {
+		t.Errorf("profile AccountID after switch = %q, want %q", p.AccountID, "ACCT_B")
+	}
+	// Active-profile lookup must agree (this is what subsequent commands consult).
+	active := loaded.ActiveProfileConfig()
+	if active.AccountID != "ACCT_B" {
+		t.Errorf("ActiveProfileConfig().AccountID after switch = %q, want %q", active.AccountID, "ACCT_B")
+	}
+}

cmd/auth/switch.go

@@ -105,7 +105,20 @@ func runSwitch(cmd *cobra.Command, args []string) error {
 		return nil
 	}
 
+	// Persist into both the active profile (the source of truth consulted by
+	// subsequent commands) and the legacy top-level field. Updating only one
+	// leaves them disagreeing, which silently routes commands to the old
+	// account.
 	cfg.AccountID = target
+	if len(cfg.Profiles) > 0 {
+		name := cfg.ActiveProfile
+		if name == "" {
+			name = "default"
+		}
+		if p, ok := cfg.Profiles[name]; ok {
+			p.AccountID = target
+		}
+	}
 	if err := config.Save(configPath, cfg); err != nil {
 		return fmt.Errorf("saving config: %w", err)
 	}

cmd/recording/download.go

@@ -42,7 +42,10 @@ func runDownload(cmd *cobra.Command, args []string) error {
 		return fmt.Errorf("downloading recording: %w", err)
 	}
 
-	if err := os.WriteFile(downloadOutput, data, 0644); err != nil {
+	// Recordings can contain customer call audio (PII, financial discussions,
+	// voicemail). Write owner-only so other local users on shared hosts can't
+	// read the file.
+	if err := os.WriteFile(downloadOutput, data, 0600); err != nil {
 		return fmt.Errorf("writing recording to file: %w", err)
 	}
 

internal/config/config.go

@@ -42,9 +42,30 @@ type Config struct {
 	Environment string   `json:"environment,omitempty"`
 }
 
-// ActiveProfileConfig returns the active profile, resolving legacy config if needed.
+// ActiveProfileConfig returns the active profile with BW_* env overlays applied.
+// The returned *Profile is always a fresh copy — mutating it does not affect
+// stored profiles, so writers (Load → Save flows) cannot accidentally leak
+// session env values onto disk.
 func (c *Config) ActiveProfileConfig() *Profile {
-	// If we have profiles, use the active one
+	base := c.activeStoredProfile()
+	overlay := *base
+	if v := os.Getenv("BW_CLIENT_ID"); v != "" {
+		overlay.ClientID = v
+	}
+	if v := os.Getenv("BW_ACCOUNT_ID"); v != "" {
+		overlay.AccountID = v
+	}
+	if v := os.Getenv("BW_ENVIRONMENT"); v != "" {
+		overlay.Environment = v
+	}
+	return &overlay
+}
+
+// activeStoredProfile returns the profile as persisted on disk, with no env
+// overlays. It returns a pointer into c.Profiles when one exists, or a synthetic
+// profile from the legacy top-level fields. Callers must not mutate the result
+// unless they are intentionally rewriting stored state.
+func (c *Config) activeStoredProfile() *Profile {
 	if len(c.Profiles) > 0 {
 		name := c.ActiveProfile
 		if name == "" {
@@ -53,13 +74,10 @@ func (c *Config) ActiveProfileConfig() *Profile {
 		if p, ok := c.Profiles[name]; ok {
 			return p
 		}
-		// Active profile doesn't exist — return first available
 		for _, p := range c.Profiles {
 			return p
 		}
 	}
-
-	// Legacy: no profiles map, use top-level fields
 	return &Profile{
 		ClientID:    c.ClientID,
 		AccountID:   c.AccountID,
@@ -133,8 +151,10 @@ func DefaultPath() (string, error) {
 	return newPath, nil
 }
 
-// Load reads config from path, overlays env vars, and returns defaults if the
-// file does not exist. The default format is "json".
+// Load reads config from path and returns defaults if the file does not exist.
+// The default format is "json". BW_* env overlays are applied at read time by
+// ActiveProfileConfig, never mutated into stored fields here — otherwise a
+// later Save would persist the session value onto disk.
 func Load(path string) (*Config, error) {
 	cfg := &Config{Format: "json"}
 
@@ -149,24 +169,6 @@ func Load(path string) (*Config, error) {
 		}
 	}
 
-	// Overlay environment variables on the active profile
-	p := cfg.ActiveProfileConfig()
-	if v := os.Getenv("BW_CLIENT_ID"); v != "" {
-		p.ClientID = v
-		cfg.ClientID = v
-	}
-	if v := os.Getenv("BW_ACCOUNT_ID"); v != "" {
-		p.AccountID = v
-		cfg.AccountID = v
-	}
-	if v := os.Getenv("BW_FORMAT"); v != "" {
-		cfg.Format = v
-	}
-	if v := os.Getenv("BW_ENVIRONMENT"); v != "" {
-		p.Environment = v
-		cfg.Environment = v
-	}
-
 	return cfg, nil
 }
 

internal/config/config_test.go

@@ -123,12 +123,18 @@ func TestEnvVarOverride(t *testing.T) {
 		t.Fatalf("Load() error: %v", err)
 	}
 
-	if cfg.AccountID != "FROM_ENV" {
-		t.Errorf("AccountID = %q, want %q (env override)", cfg.AccountID, "FROM_ENV")
+	// Env overlay is applied at read time via ActiveProfileConfig,
+	// not mutated into stored fields during Load.
+	p := cfg.ActiveProfileConfig()
+	if p.AccountID != "FROM_ENV" {
+		t.Errorf("ActiveProfileConfig().AccountID = %q, want %q (env override)", p.AccountID, "FROM_ENV")
+	}
+	if p.ClientID != "FROM_FILE" {
+		t.Errorf("ActiveProfileConfig().ClientID = %q, want %q", p.ClientID, "FROM_FILE")
 	}
-	// Other fields should still come from file
-	if cfg.ClientID != "FROM_FILE" {
-		t.Errorf("ClientID = %q, want %q", cfg.ClientID, "FROM_FILE")
+	// Stored fields must remain untouched so Save can't leak env values to disk.
+	if cfg.AccountID != "ACC_FROM_FILE" {
+		t.Errorf("stored cfg.AccountID = %q, want %q (Load must not mutate stored fields)", cfg.AccountID, "ACC_FROM_FILE")
 	}
 }
 
@@ -326,30 +332,122 @@ func TestAllEnvVarOverrides(t *testing.T) {
 	path := filepath.Join(dir, "config.json")
 
 	base := &Config{Format: "json"}
+	base.SetProfile("default", &Profile{ClientID: "fileclientid", AccountID: "fileaccount", Environment: "prod"})
 	if err := Save(path, base); err != nil {
 		t.Fatalf("Save() error: %v", err)
 	}
 
 	t.Setenv("BW_CLIENT_ID", "envclientid")
 	t.Setenv("BW_ACCOUNT_ID", "envaccount")
-	t.Setenv("BW_FORMAT", "table")
 	t.Setenv("BW_ENVIRONMENT", "custom")
 
 	cfg, err := Load(path)
 	if err != nil {
 		t.Fatalf("Load() error: %v", err)
 	}
 
-	if cfg.ClientID != "envclientid" {
-		t.Errorf("ClientID = %q, want %q", cfg.ClientID, "envclientid")
+	p := cfg.ActiveProfileConfig()
+	if p.ClientID != "envclientid" {
+		t.Errorf("ActiveProfileConfig().ClientID = %q, want %q", p.ClientID, "envclientid")
 	}
-	if cfg.AccountID != "envaccount" {
-		t.Errorf("AccountID = %q, want %q", cfg.AccountID, "envaccount")
+	if p.AccountID != "envaccount" {
+		t.Errorf("ActiveProfileConfig().AccountID = %q, want %q", p.AccountID, "envaccount")
 	}
-	if cfg.Format != "table" {
-		t.Errorf("Format = %q, want %q", cfg.Format, "table")
+	if p.Environment != "custom" {
+		t.Errorf("ActiveProfileConfig().Environment = %q, want %q", p.Environment, "custom")
 	}
-	if cfg.Environment != "custom" {
-		t.Errorf("Environment = %q, want %q", cfg.Environment, "custom")
+
+	// Stored profile must remain untouched.
+	stored := cfg.Profiles["default"]
+	if stored.ClientID != "fileclientid" || stored.AccountID != "fileaccount" || stored.Environment != "prod" {
+		t.Errorf("stored profile mutated by Load: %+v", stored)
+	}
+}
+
+// TestLoad_EnvOverlayDoesNotPersistOntoStoredProfiles guards against the
+// regression where Load applied env vars to the live *Profile pointer in
+// cfg.Profiles, so that any subsequent Save (login, switch, etc.) would
+// silently rewrite the previously-active profile on disk with env values.
+func TestLoad_EnvOverlayDoesNotPersistOntoStoredProfiles(t *testing.T) {
+	dir := t.TempDir()
+	path := filepath.Join(dir, "config.json")
+
+	cfg := &Config{Format: "json"}
+	cfg.SetProfile("prod", &Profile{ClientID: "prod-id", AccountID: "ACCT_A", Environment: "prod"})
+	cfg.SetProfile("dev", &Profile{ClientID: "dev-id", AccountID: "ACCT_B", Environment: "test"})
+	cfg.ActiveProfile = "prod"
+	if err := Save(path, cfg); err != nil {
+		t.Fatal(err)
+	}
+
+	t.Setenv("BW_ACCOUNT_ID", "ENV_ACCT_Z")
+	t.Setenv("BW_CLIENT_ID", "ENV_CLIENT_Z")
+	t.Setenv("BW_ENVIRONMENT", "ENV_HOST_Z")
+
+	// Simulate a writer flow: Load → mutate something unrelated → Save.
+	loaded, err := Load(path)
+	if err != nil {
+		t.Fatal(err)
+	}
+	loaded.ActiveProfile = "dev"
+	if err := Save(path, loaded); err != nil {
+		t.Fatal(err)
+	}
+
+	// Re-read with env vars cleared to see only what was persisted.
+	t.Setenv("BW_ACCOUNT_ID", "")
+	t.Setenv("BW_CLIENT_ID", "")
+	t.Setenv("BW_ENVIRONMENT", "")
+
+	fresh, err := Load(path)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	prod := fresh.Profiles["prod"]
+	if prod.AccountID != "ACCT_A" || prod.ClientID != "prod-id" || prod.Environment != "prod" {
+		t.Errorf("prod profile leaked env values: %+v", prod)
+	}
+	dev := fresh.Profiles["dev"]
+	if dev.AccountID != "ACCT_B" || dev.ClientID != "dev-id" || dev.Environment != "test" {
+		t.Errorf("dev profile leaked env values: %+v", dev)
+	}
+}
+
+func TestActiveProfileConfig_AppliesEnvOverlay(t *testing.T) {
+	cfg := &Config{}
+	cfg.SetProfile("default", &Profile{ClientID: "id1", AccountID: "ACCT_A", Environment: "prod"})
+
+	t.Setenv("BW_ACCOUNT_ID", "ENV_ACCT_Z")
+	t.Setenv("BW_CLIENT_ID", "ENV_CLIENT_Z")
+	t.Setenv("BW_ENVIRONMENT", "test")
+
+	p := cfg.ActiveProfileConfig()
+	if p.AccountID != "ENV_ACCT_Z" {
+		t.Errorf("AccountID = %q, want %q", p.AccountID, "ENV_ACCT_Z")
+	}
+	if p.ClientID != "ENV_CLIENT_Z" {
+		t.Errorf("ClientID = %q, want %q", p.ClientID, "ENV_CLIENT_Z")
+	}
+	if p.Environment != "test" {
+		t.Errorf("Environment = %q, want %q", p.Environment, "test")
+	}
+
+	// Stored profile must not be mutated by ActiveProfileConfig.
+	stored := cfg.Profiles["default"]
+	if stored.AccountID != "ACCT_A" || stored.ClientID != "id1" || stored.Environment != "prod" {
+		t.Errorf("stored profile mutated by ActiveProfileConfig: %+v", stored)
+	}
+}
+
+func TestActiveProfileConfig_ReturnsCopySafeToMutate(t *testing.T) {
+	cfg := &Config{}
+	cfg.SetProfile("default", &Profile{ClientID: "id1", AccountID: "ACCT_A"})
+
+	p := cfg.ActiveProfileConfig()
+	p.AccountID = "MUTATED"
+
+	if cfg.Profiles["default"].AccountID != "ACCT_A" {
+		t.Errorf("mutating ActiveProfileConfig() result leaked into stored profile: %q", cfg.Profiles["default"].AccountID)
 	}
 }