From 4ffbe17a98fa074eeeaede98baa4c6e743a54e10 Mon Sep 17 00:00:00 2001 From: Kush Date: Wed, 6 May 2026 14:21:28 -0400 Subject: [PATCH 1/3] fix(config): apply BW_* env overlays at read time, not at load Load was applying BW_CLIENT_ID, BW_ACCOUNT_ID, and BW_ENVIRONMENT through the live *Profile pointer inside cfg.Profiles, so any subsequent Save would persist those session values onto the previously-active profile on disk. Move the overlay into ActiveProfileConfig, which now returns a fresh copy with env vars applied on top. Stored profiles are no longer mutated by Load. Existing env-overlay tests updated to match the read-time contract; new tests cover the persistence and copy-on-overlay invariants. --- internal/config/config.go | 52 +++++++------- internal/config/config_test.go | 126 +++++++++++++++++++++++++++++---- 2 files changed, 139 insertions(+), 39 deletions(-) diff --git a/internal/config/config.go b/internal/config/config.go index e84307b..19badf5 100644 --- a/internal/config/config.go +++ b/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 } diff --git a/internal/config/config_test.go b/internal/config/config_test.go index e98ac68..81da655 100644 --- a/internal/config/config_test.go +++ b/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,13 +332,13 @@ 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) @@ -340,16 +346,108 @@ func TestAllEnvVarOverrides(t *testing.T) { 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) } } From 0ad891b321c05147ec8ac2909839bf988d07c40a Mon Sep 17 00:00:00 2001 From: Kush Date: Wed, 6 May 2026 14:21:37 -0400 Subject: [PATCH 2/3] fix(auth): persist switched account into active profile MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit runSwitch was setting only the legacy top-level cfg.AccountID. Once a profile exists in cfg.Profiles, ActiveProfileConfig reads from the profile map, so the switch wasn't reflected in subsequent commands — they continued routing to the pre-switch account. Update the active profile's AccountID alongside the legacy field, the same way auth login (via SetProfile) and auth use already do. Adds a regression test that runs runSwitch end-to-end against a temp config and asserts the active profile is the source of truth on the next load. --- cmd/auth/auth_test.go | 52 +++++++++++++++++++++++++++++++++++++++++++ cmd/auth/switch.go | 13 +++++++++++ 2 files changed, 65 insertions(+) diff --git a/cmd/auth/auth_test.go b/cmd/auth/auth_test.go index 19a5a06..3275abc 100644 --- a/cmd/auth/auth_test.go +++ b/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") + } +} diff --git a/cmd/auth/switch.go b/cmd/auth/switch.go index fe07b26..af27037 100644 --- a/cmd/auth/switch.go +++ 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) } From 8b995918c03c49d8219e6f92ec89abfac595f405 Mon Sep 17 00:00:00 2001 From: Kush Date: Wed, 6 May 2026 14:21:42 -0400 Subject: [PATCH 3/3] fix(recording): write downloaded media with 0600 permissions Recordings can contain customer call audio. Match config.Save's permissions for sensitive blobs instead of 0644. --- cmd/recording/download.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/cmd/recording/download.go b/cmd/recording/download.go index 9680c5c..35483e9 100644 --- a/cmd/recording/download.go +++ b/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) }