From ec97d05941231eba6f8d70888c800430ab9ee16f Mon Sep 17 00:00:00 2001 From: "jcool (Hermes agent)" Date: Thu, 24 Sep 2026 14:38:49 +0000 Subject: [PATCH] fix(auth): honor BW_CLIENT_SECRET past login, not just at login MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit band auth login accepts BW_CLIENT_ID/BW_CLIENT_SECRET and works fine with no OS keychain available — it verifies credentials and writes config.json, and keychain storage is a separate, best-effort step. But every command after login (including auth status) reads the secret exclusively from the keychain via loadConfigAndAuth, with no env var fallback. On a host with no D-Bus/keyring stack (any container, most headless CI runners, this box), that means login succeeds and then every single subsequent command fails with 'credentials not found in keychain' -- even though the caller still has the same BW_CLIENT_SECRET that worked for login. Found while running an internal PoC against a real Bandwidth Build account on a headless Linux host: auth login needed hand-rolling a local D-Bus session + gnome-keyring daemon from unpacked .deb files just to get past step one, and even then band auth status reported authenticated:false with BW_CLIENT_SECRET correctly set, right before every other command would have worked using that same env var. - loadConfigAndAuth (internal/cmdutil/helpers.go): check BW_CLIENT_SECRET before falling back to the keychain, mirroring how BW_CLIENT_ID is already overlaid by config.ActiveProfileConfig. - auth status (cmd/auth/status.go): authenticated now agrees with what every other command will actually do -- true if either the keychain has the secret or BW_CLIENT_SECRET is set, not keychain-only. - Error message on failure now mentions both remediation paths. No behavior change for the normal desktop keychain flow. --- cmd/auth/auth_test.go | 49 ++++++++++++++++++++++ cmd/auth/status.go | 11 +++-- internal/cmdutil/helpers.go | 15 ++++++- internal/cmdutil/helpers_test.go | 70 +++++++++++++++++++++++++++++++- 4 files changed, 140 insertions(+), 5 deletions(-) diff --git a/cmd/auth/auth_test.go b/cmd/auth/auth_test.go index cd995b0..0f69b95 100644 --- a/cmd/auth/auth_test.go +++ b/cmd/auth/auth_test.go @@ -390,6 +390,55 @@ func TestCustomerProfilesMatcherNotOverBroad(t *testing.T) { } } +// TestStatusPlainAuthenticatedViaEnvSecret guards against the headless/CI +// path being broken: `band auth login` accepts BW_CLIENT_ID/BW_CLIENT_SECRET +// with no keychain available (see cmdutil.loadConfigAndAuth), so `auth +// status` must report authenticated=true under the same env var rather than +// consulting the keychain alone — otherwise a caller with no keychain sees +// "not authenticated" immediately before every other command succeeds using +// that same env var. +func TestStatusPlainAuthenticatedViaEnvSecret(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + t.Setenv("XDG_CONFIG_HOME", filepath.Join(home, ".config")) + t.Setenv("BW_CLIENT_SECRET", "some-secret-not-in-any-keychain") + + cfgPath, err := config.DefaultPath() + if err != nil { + t.Fatal(err) + } + cfg := &config.Config{Format: "json"} + cfg.SetProfile("default", &config.Profile{ + ClientID: "id-with-no-keychain-entry", + AccountID: "ACCT_A", + }) + if err := config.Save(cfgPath, cfg); err != nil { + t.Fatal(err) + } + + wrap := &cobra.Command{Use: "status", RunE: runStatus} + root := testutil.NewTestRoot(wrap) + root.SetArgs([]string{"status", "--plain"}) + + out := testutil.CaptureStdout(t, func() { + if err := root.Execute(); err != nil { + t.Fatalf("Execute() error = %v", err) + } + }) + + var got statusJSON + if err := json.Unmarshal([]byte(out), &got); err != nil { + t.Fatalf("unmarshal output: %v\noutput: %s", err, out) + } + + if !got.Authenticated { + t.Errorf("Authenticated = false with BW_CLIENT_SECRET set and no keychain entry, want true. Error field: %q", got.Error) + } + if got.Error != "" { + t.Errorf("Error = %q, want empty when authenticated via env var", got.Error) + } +} + // 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 diff --git a/cmd/auth/status.go b/cmd/auth/status.go index 019c918..d584f57 100644 --- a/cmd/auth/status.go +++ b/cmd/auth/status.go @@ -82,12 +82,17 @@ func runStatus(cmd *cobra.Command, args []string) error { profileName = "default" } + // BW_CLIENT_SECRET satisfies auth the same way it does for every other + // command (see cmdutil.loadConfigAndAuth) — status must agree with what + // commands will actually do, or a headless caller with the env var set + // sees "not authenticated" right before every other command succeeds. _, keychainErr := intauth.GetPassword(p.ClientID) + authenticated := keychainErr == nil || os.Getenv("BW_CLIENT_SECRET") != "" if plain { caps := Capabilities(p.Roles) out := statusJSON{ - Authenticated: keychainErr == nil, + Authenticated: authenticated, Profile: profileName, ClientID: p.ClientID, AccountID: p.AccountID, @@ -99,13 +104,13 @@ func runStatus(cmd *cobra.Command, args []string) error { SIP: sipCapability(hasRole(p.Roles, "sip credentials")), TenDLC: tendlcCapability(caps["campaign_management"]), } - if keychainErr != nil { + if !authenticated { out.Error = "credentials not found in keychain" } return emitJSON(out) } - if keychainErr != nil { + if !authenticated { fmt.Printf("Client ID: %s\n", ui.ID(p.ClientID)) fmt.Printf("Account: %s\n", ui.ID(p.AccountID)) // Show environment only when it's informative. diff --git a/internal/cmdutil/helpers.go b/internal/cmdutil/helpers.go index 5af54ea..e0c2f51 100644 --- a/internal/cmdutil/helpers.go +++ b/internal/cmdutil/helpers.go @@ -132,9 +132,22 @@ func loadConfigAndAuth() (*config.Config, *config.Profile, string, error) { return nil, nil, "", fmt.Errorf("not logged in — run `band auth login` first") } + // BW_CLIENT_SECRET, like BW_CLIENT_ID (already overlaid into p by + // ActiveProfileConfig), lets headless/CI callers skip the OS keychain + // entirely. This matters in practice: the keychain backend (go-keyring) + // needs a running D-Bus session + keyring daemon, which `band auth + // login` can complete without (it only stores), but which many headless + // Linux hosts don't have at all. Without this fallback, `auth login` + // would succeed via the env var while every subsequent command failed + // looking up the keychain — the documented "headless and CI/CD" flow + // wouldn't actually hold together end to end. + if secret := os.Getenv("BW_CLIENT_SECRET"); secret != "" { + return cfg, p, secret, nil + } + clientSecret, err := auth.GetPassword(p.ClientID) if err != nil { - return nil, nil, "", fmt.Errorf("credentials not found in keychain for %s — run `band auth login`", p.ClientID) + return nil, nil, "", fmt.Errorf("credentials not found in keychain for %s — run `band auth login`, or set BW_CLIENT_ID/BW_CLIENT_SECRET env vars for headless use", p.ClientID) } return cfg, p, clientSecret, nil diff --git a/internal/cmdutil/helpers_test.go b/internal/cmdutil/helpers_test.go index 17cb1e5..b225048 100644 --- a/internal/cmdutil/helpers_test.go +++ b/internal/cmdutil/helpers_test.go @@ -1,6 +1,74 @@ package cmdutil -import "testing" +import ( + "path/filepath" + "testing" + + "github.com/Bandwidth/cli/internal/config" +) + +// TestLoadConfigAndAuth_BW_CLIENT_SECRET_SkipsKeychain guards the headless/CI +// path: `band auth login --client-id X --client-secret Y` succeeds with no +// keychain available (it only verifies + writes config.json; storing the +// secret in the OS keychain is a separate, best-effort step). Without this +// fallback, every command *after* login would fail with "credentials not +// found in keychain" on a host with no D-Bus/keyring stack, even though the +// same BW_CLIENT_SECRET the caller already has would work fine. +func TestLoadConfigAndAuth_BW_CLIENT_SECRET_SkipsKeychain(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + t.Setenv("XDG_CONFIG_HOME", filepath.Join(home, ".config")) + t.Setenv("BW_CLIENT_SECRET", "headless-secret-not-in-any-keychain") + + cfgPath, err := config.DefaultPath() + if err != nil { + t.Fatal(err) + } + cfg := &config.Config{Format: "json"} + cfg.SetProfile("default", &config.Profile{ + ClientID: "id-with-no-keychain-entry", + AccountID: "ACCT_A", + }) + if err := config.Save(cfgPath, cfg); err != nil { + t.Fatal(err) + } + + _, _, secret, err := loadConfigAndAuth() + if err != nil { + t.Fatalf("loadConfigAndAuth() error = %v, want nil (BW_CLIENT_SECRET should bypass the keychain lookup)", err) + } + if secret != "headless-secret-not-in-any-keychain" { + t.Errorf("secret = %q, want the BW_CLIENT_SECRET env var value", secret) + } +} + +// TestLoadConfigAndAuth_NoSecretNoKeychain_ReturnsActionableError guards the +// other side: without BW_CLIENT_SECRET and with no keychain entry, the error +// must still mention both remediation paths, not just `band auth login`. +func TestLoadConfigAndAuth_NoSecretNoKeychain_ReturnsActionableError(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + t.Setenv("XDG_CONFIG_HOME", filepath.Join(home, ".config")) + t.Setenv("BW_CLIENT_SECRET", "") + + cfgPath, err := config.DefaultPath() + if err != nil { + t.Fatal(err) + } + cfg := &config.Config{Format: "json"} + cfg.SetProfile("default", &config.Profile{ + ClientID: "id-with-no-keychain-entry", + AccountID: "ACCT_A", + }) + if err := config.Save(cfgPath, cfg); err != nil { + t.Fatal(err) + } + + _, _, _, err = loadConfigAndAuth() + if err == nil { + t.Fatal("loadConfigAndAuth() error = nil, want an error with no keychain entry and no BW_CLIENT_SECRET") + } +} func TestVoiceHostForEnvironment(t *testing.T) { tests := []struct {