fix: harden install and credential cleanup (#14)

* feat: remove smoke test

* feat: fail intall if checksum cannot be verified

* fix: clean up stale keyring credentials

Store API keys under a stable keyring entry instead of deriving the entry name from base URL and account ID. The old scheme could leave credentials behind if config.yaml was edited manually before logout.

On first read, migrate an existing URL/account-scoped key to the stable api-key entry and delete the legacy entry. Keep a TODO to remove the migration in v1 after users have had a release cycle to move forward.

Logout now clears the whole chatwoot-cli keyring service even when config.yaml is missing, so stale entries are removed independently of the current config state.

* fix: sanitize server-controlled CLI output

Sanitize server-provided strings before writing auth login output, raw API responses, verbose non-JSON bodies, and API error bodies to the terminal.

Expand verbose JSON redaction to cover hmac_identifier and common secret-like field names such as *_token, *_secret, *_key, and hmac_* values.

Tests cover terminal escape stripping for the affected output paths and redaction of HMAC/secret-like response fields.

* fix: scope saved keyring credentials

Store stable keyring credentials as scoped JSON containing the normalized base URL, account ID, and API key instead of a raw token.

ResolveAPIKey now rejects saved credentials when the current config points at a different instance or account, preventing a token from being sent to the wrong host after config.yaml is edited or restored.

Legacy scoped entries are migrated into the new scoped JSON format and then deleted.

Author: scmmishra

install.sh

@@ -130,27 +130,27 @@ fi
 # ---------------------------------------------------------------------------
 # Verify checksum (sha256)
 # ---------------------------------------------------------------------------
-if curl -fsSL "$checksum_url" -o "$tmp/checksums.txt" 2>/dev/null; then
-  expected=$(grep " ${asset}\$" "$tmp/checksums.txt" | awk '{print $1}')
-  if [ -n "$expected" ]; then
-    if has sha256sum; then
-      actual=$(sha256sum "$tmp/$asset" | awk '{print $1}')
-    elif has shasum; then
-      actual=$(shasum -a 256 "$tmp/$asset" | awk '{print $1}')
-    else
-      warn "no sha256 tool found — skipping checksum verification"
-      actual=""
-    fi
-    if [ -n "$actual" ] && [ "$expected" != "$actual" ]; then
-      err "checksum mismatch (expected=${expected} got=${actual})"
-    fi
-    [ -n "$actual" ] && step "Verified checksum"
-  else
-    warn "${asset} not listed in checksums.txt — skipping verification"
-  fi
+if ! curl -fsSL "$checksum_url" -o "$tmp/checksums.txt" 2>/dev/null; then
+  err "failed to download checksums.txt; refusing to install without checksum verification"
+fi
+
+expected=$(grep " ${asset}\$" "$tmp/checksums.txt" | awk '{print $1}')
+if [ -z "$expected" ]; then
+  err "${asset} not listed in checksums.txt; refusing to install without checksum verification"
+fi
+
+if has sha256sum; then
+  actual=$(sha256sum "$tmp/$asset" | awk '{print $1}')
+elif has shasum; then
+  actual=$(shasum -a 256 "$tmp/$asset" | awk '{print $1}')
 else
-  warn "could not fetch checksums.txt — skipping verification"
+  err "no sha256 tool found; install sha256sum or shasum and retry"
+fi
+
+if [ "$expected" != "$actual" ]; then
+  err "checksum mismatch (expected=${expected} got=${actual})"
 fi
+step "Verified checksum"
 
 # ---------------------------------------------------------------------------
 # Extract and install

internal/cmd/api.go

@@ -10,6 +10,8 @@ import (
 	"net/url"
 	"os"
 	"strings"
+
+	"github.com/chatwoot/cli/internal/output"
 )
 
 type ApiCmd struct {
@@ -131,8 +133,9 @@ func printAPIResponse(w io.Writer, body []byte) {
 		return
 	}
 
-	_, _ = w.Write(body)
-	if body[len(body)-1] != '\n' {
+	safeBody := output.SanitizeText(string(body))
+	_, _ = io.WriteString(w, safeBody)
+	if !strings.HasSuffix(safeBody, "\n") {
 		_, _ = fmt.Fprintln(w)
 	}
 }

internal/cmd/api_test.go

@@ -123,6 +123,22 @@ func TestApiCmdSendsMethodBodyAndHeaders(t *testing.T) {
 	}
 }
 
+func TestPrintAPIResponseSanitizesNonJSONBody(t *testing.T) {
+	var out bytes.Buffer
+
+	printAPIResponse(&out, []byte("hello\x1b]52;c;Zm9v\aworld\x1b[31m"))
+
+	got := out.String()
+	for _, disallowed := range []string{"\x1b", "\a", "]52", "[31m"} {
+		if strings.Contains(got, disallowed) {
+			t.Fatalf("non-JSON API response contained terminal control %q: %q", disallowed, got)
+		}
+	}
+	if !strings.Contains(got, "helloworld") {
+		t.Fatalf("non-JSON API response stripped printable content: %q", got)
+	}
+}
+
 func TestNormalizeAPIPathRejectsAbsoluteURLs(t *testing.T) {
 	if _, _, err := normalizeAPIPath("https://example.com/api/v1/profile", false); err == nil {
 		t.Fatal("expected absolute URL error")

internal/cmd/auth.go

@@ -74,10 +74,14 @@ func (c *AuthLoginCmd) Run(app *App) error {
 		return fmt.Errorf("failed to save config: %w", err)
 	}
 
-	fmt.Printf("Logged in as %s (%s)\n", profile.Name, profile.Email)
+	fmt.Print(loginSuccessMessage(profile.Name, profile.Email))
 	return nil
 }
 
+func loginSuccessMessage(name, email string) string {
+	return fmt.Sprintf("Logged in as %s (%s)\n", output.SanitizeText(name), output.SanitizeText(email))
+}
+
 func readAPIKey(reader *bufio.Reader) (string, error) {
 	fmt.Print("API Key: ")
 
@@ -111,10 +115,8 @@ func (c *AuthLogoutCmd) Run(app *App) error {
 		return err
 	}
 
-	if cfg != nil {
-		if err := config.DeleteAPIKey(cfg); err != nil {
-			return err
-		}
+	if err := config.DeleteAPIKey(cfg); err != nil {
+		return err
 	}
 
 	if err := os.Remove(path); err != nil {

internal/cmd/auth_test.go

@@ -2,6 +2,7 @@ package cmd
 
 import (
 	"bytes"
+	"errors"
 	"net/http"
 	"net/http/httptest"
 	"strings"
@@ -112,6 +113,18 @@ func TestAuthStatusReportsIdentityAndCredentialSource(t *testing.T) {
 	}
 }
 
+func TestLoginSuccessMessageStripsTerminalControls(t *testing.T) {
+	got := loginSuccessMessage("Eve\x1b]52;c;Zm9v\a", "eve@example.com\x1b[31m")
+	for _, disallowed := range []string{"\x1b", "\a", "]52", "[31m"} {
+		if strings.Contains(got, disallowed) {
+			t.Fatalf("login success message contained terminal control %q: %q", disallowed, got)
+		}
+	}
+	if !strings.Contains(got, "Logged in as Eve (eve@example.com)") {
+		t.Fatalf("login success message stripped printable content: %q", got)
+	}
+}
+
 func TestMeAndWhoamiAliasAuthStatus(t *testing.T) {
 	profile := `{
 		"id": 7,
@@ -152,6 +165,27 @@ func TestAuthStatusNotLoggedIn(t *testing.T) {
 	}
 }
 
+func TestAuthLogoutRemovesKeyringTokenWithoutConfig(t *testing.T) {
+	keyring.MockInit()
+	if err := keyring.DeleteAll("chatwoot-cli"); err != nil {
+		t.Fatalf("keyring.DeleteAll: %v", err)
+	}
+	t.Setenv("HOME", t.TempDir())
+	t.Setenv(config.APIKeyEnv, "")
+
+	if err := keyring.Set("chatwoot-cli", "api-key", "stale-token"); err != nil {
+		t.Fatalf("keyring.Set: %v", err)
+	}
+
+	if err := (&AuthLogoutCmd{}).Run(&App{}); err != nil {
+		t.Fatalf("Run: %v", err)
+	}
+
+	if _, err := keyring.Get("chatwoot-cli", "api-key"); !errors.Is(err, keyring.ErrNotFound) {
+		t.Fatalf("expected logout to delete stale keyring token, err = %v", err)
+	}
+}
+
 func TestAuthStatusSelfHealsCachedUserID(t *testing.T) {
 	profile := `{
 		"id": 99,

internal/config/credentials.go

@@ -1,6 +1,7 @@
 package config
 
 import (
+	"encoding/json"
 	"errors"
 	"fmt"
 	"os"
@@ -10,8 +11,9 @@ import (
 )
 
 const (
-	APIKeyEnv      = "CHATWOOT_API_KEY"
-	keyringService = "chatwoot-cli"
+	APIKeyEnv          = "CHATWOOT_API_KEY"
+	keyringService     = "chatwoot-cli"
+	apiKeyKeyringEntry = "api-key"
 )
 
 type CredentialSource string
@@ -24,6 +26,12 @@ const (
 
 var ErrAPIKeyNotFound = errors.New("api key not found")
 
+type savedCredential struct {
+	BaseURL   string `json:"base_url"`
+	AccountID int    `json:"account_id"`
+	APIKey    string `json:"api_key"`
+}
+
 // ResolveAPIKey implements the auth flow for the CLI. YAML config intentionally
 // stores only non-secrets, and plaintext api_key values from older configs are
 // ignored. CHATWOOT_API_KEY wins for CI, coding agents, and temporary overrides;
@@ -37,14 +45,33 @@ func ResolveAPIKey(cfg *Config) (string, CredentialSource, error) {
 		return "", CredentialSourceMissing, missingAPIKeyError()
 	}
 
-	apiKey, err := keyring.Get(keyringService, credentialKey(cfg))
+	stored, err := keyring.Get(keyringService, apiKeyKeyringEntry)
 	if err == nil {
+		apiKey, err := parseSavedCredential(stored, cfg)
+		if err != nil {
+			return "", CredentialSourceMissing, err
+		}
 		return apiKey, CredentialSourceKeyring, nil
 	}
-	if errors.Is(err, keyring.ErrNotFound) {
-		return "", CredentialSourceMissing, missingAPIKeyError()
+	if !errors.Is(err, keyring.ErrNotFound) {
+		return "", CredentialSourceMissing, fmt.Errorf("failed to read API key from keyring: %w", err)
 	}
-	return "", CredentialSourceMissing, fmt.Errorf("failed to read API key from keyring: %w", err)
+
+	// TODO(v1): remove this legacy key migration after users have had a release
+	// cycle to move from URL/account-scoped keyring entries to api-key.
+	apiKey, err := keyring.Get(keyringService, legacyCredentialKey(cfg))
+	if err == nil {
+		if err := saveAPIKeyToKeyring(cfg, apiKey); err != nil {
+			return "", CredentialSourceMissing, fmt.Errorf("failed to migrate API key in keyring: %w", err)
+		}
+		_ = keyring.Delete(keyringService, legacyCredentialKey(cfg))
+		return apiKey, CredentialSourceKeyring, nil
+	}
+	if !errors.Is(err, keyring.ErrNotFound) {
+		return "", CredentialSourceMissing, fmt.Errorf("failed to read legacy API key from keyring: %w", err)
+	}
+
+	return "", CredentialSourceMissing, missingAPIKeyError()
 }
 
 func SaveAPIKey(cfg *Config, apiKey string) error {
@@ -55,30 +82,58 @@ func SaveAPIKey(cfg *Config, apiKey string) error {
 	if cfg == nil || !cfg.IsValid() {
 		return fmt.Errorf("valid config is required to save API key")
 	}
-	if err := keyring.Set(keyringService, credentialKey(cfg), apiKey); err != nil {
+	if err := saveAPIKeyToKeyring(cfg, apiKey); err != nil {
 		return fmt.Errorf("failed to save API key to keyring: %w", err)
 	}
 	return nil
 }
 
-func DeleteAPIKey(cfg *Config) error {
-	if cfg == nil || !cfg.IsValid() {
-		return nil
+func saveAPIKeyToKeyring(cfg *Config, apiKey string) error {
+	credential := savedCredential{
+		BaseURL:   normalizeBaseURL(cfg.BaseURL),
+		AccountID: cfg.AccountID,
+		APIKey:    apiKey,
 	}
-	if err := keyring.Delete(keyringService, credentialKey(cfg)); err != nil {
-		if errors.Is(err, keyring.ErrNotFound) {
-			return nil
-		}
-		return fmt.Errorf("failed to delete API key from keyring: %w", err)
+
+	data, err := json.Marshal(credential)
+	if err != nil {
+		return err
 	}
-	return nil
+	return keyring.Set(keyringService, apiKeyKeyringEntry, string(data))
+}
+
+func parseSavedCredential(stored string, cfg *Config) (string, error) {
+	var credential savedCredential
+	if err := json.Unmarshal([]byte(stored), &credential); err != nil {
+		return "", credentialScopeMismatchError()
+	}
+	if credential.APIKey == "" ||
+		credential.BaseURL != normalizeBaseURL(cfg.BaseURL) ||
+		credential.AccountID != cfg.AccountID {
+		return "", credentialScopeMismatchError()
+	}
+	return credential.APIKey, nil
+}
+
+// DeleteAPIKey removes every credential saved by this CLI service. This avoids
+// leaving stale keyring entries behind when config.yaml was edited or removed.
+func DeleteAPIKey(_ *Config) error {
+	err := keyring.DeleteAll(keyringService)
+	if err == nil || errors.Is(err, keyring.ErrNotFound) {
+		return nil
+	}
+	return fmt.Errorf("failed to delete API keys from keyring: %w", err)
 }
 
 func missingAPIKeyError() error {
 	return fmt.Errorf("%w; run 'chatwoot auth login' or set %s", ErrAPIKeyNotFound, APIKeyEnv)
 }
 
-func credentialKey(cfg *Config) string {
+func credentialScopeMismatchError() error {
+	return fmt.Errorf("%w; saved keyring credential does not match configured instance; run 'chatwoot auth login' for this base URL and account", ErrAPIKeyNotFound)
+}
+
+func legacyCredentialKey(cfg *Config) string {
 	return fmt.Sprintf("%s/accounts/%d", normalizeBaseURL(cfg.BaseURL), cfg.AccountID)
 }