diff --git a/internal/core/config.go b/internal/core/config.go index 410c3e01..ddd428aa 100644 --- a/internal/core/config.go +++ b/internal/core/config.go @@ -240,6 +240,12 @@ func ResolveConfigFromMulti(raw *MultiAppConfig, kc keychain.KeychainAccess, pro } } + if err := ValidateSecretKeyMatch(app.AppId, app.AppSecret); err != nil { + return nil, &ConfigError{Code: 2, Type: "config", + Message: "appId and appSecret keychain key are out of sync", + Hint: err.Error()} + } + secret, err := ResolveSecretInput(app.AppSecret, kc) if err != nil { // If the error comes from the keychain, it will already be wrapped as an ExitError. diff --git a/internal/core/config_test.go b/internal/core/config_test.go index 0ffb8ee1..55fbcb83 100644 --- a/internal/core/config_test.go +++ b/internal/core/config_test.go @@ -5,9 +5,21 @@ package core import ( "encoding/json" + "errors" "testing" + + "github.com/larksuite/cli/internal/keychain" ) +// stubKeychain is a minimal KeychainAccess that always returns ErrNotFound. +type stubKeychain struct{} + +func (stubKeychain) Get(service, account string) (string, error) { + return "", keychain.ErrNotFound +} +func (stubKeychain) Set(service, account, value string) error { return nil } +func (stubKeychain) Remove(service, account string) error { return nil } + func TestAppConfig_LangSerialization(t *testing.T) { app := AppConfig{ AppId: "cli_test", AppSecret: PlainSecret("secret"), @@ -73,6 +85,85 @@ func TestMultiAppConfig_RoundTrip(t *testing.T) { } } +func TestResolveConfigFromMulti_RejectsSecretKeyMismatch(t *testing.T) { + raw := &MultiAppConfig{ + Apps: []AppConfig{ + { + AppId: "cli_new_app", + AppSecret: SecretInput{Ref: &SecretRef{ + Source: "keychain", + ID: "appsecret:cli_old_app", + }}, + Brand: BrandFeishu, + }, + }, + } + + _, err := ResolveConfigFromMulti(raw, nil, "") + if err == nil { + t.Fatal("expected error for mismatched appId and appSecret keychain key") + } + var cfgErr *ConfigError + if !errors.As(err, &cfgErr) { + t.Fatalf("expected ConfigError, got %T: %v", err, err) + } + if cfgErr.Hint == "" { + t.Error("expected non-empty hint in ConfigError") + } +} + +func TestResolveConfigFromMulti_AcceptsPlainSecret(t *testing.T) { + raw := &MultiAppConfig{ + Apps: []AppConfig{ + { + AppId: "cli_abc", + AppSecret: PlainSecret("my-secret"), + Brand: BrandFeishu, + }, + }, + } + + cfg, err := ResolveConfigFromMulti(raw, nil, "") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if cfg.AppID != "cli_abc" { + t.Errorf("AppID = %q, want %q", cfg.AppID, "cli_abc") + } +} + +func TestResolveConfigFromMulti_MatchingKeychainRefPassesValidation(t *testing.T) { + // Keychain ref matches appId, so validation passes. + // The subsequent ResolveSecretInput will fail (no real keychain), + // but that proves the mismatch check itself passed. + raw := &MultiAppConfig{ + Apps: []AppConfig{ + { + AppId: "cli_abc", + AppSecret: SecretInput{Ref: &SecretRef{ + Source: "keychain", + ID: "appsecret:cli_abc", + }}, + Brand: BrandFeishu, + }, + }, + } + + _, err := ResolveConfigFromMulti(raw, stubKeychain{}, "") + if err == nil { + // stubKeychain returns ErrNotFound, so we expect a keychain error, + // but NOT a mismatch error — that's the point of this test. + t.Fatal("expected error (keychain entry not found), got nil") + } + // The error should come from keychain resolution, NOT from our mismatch check. + var cfgErr *ConfigError + if errors.As(err, &cfgErr) { + if cfgErr.Message == "appId and appSecret keychain key are out of sync" { + t.Fatal("error came from mismatch check, but keys should match") + } + } +} + func TestResolveConfigFromMulti_DoesNotUseEnvProfileFallback(t *testing.T) { t.Setenv("LARKSUITE_CLI_PROFILE", "missing") diff --git a/internal/core/secret_resolve.go b/internal/core/secret_resolve.go index f5f4ebd9..337360bd 100644 --- a/internal/core/secret_resolve.go +++ b/internal/core/secret_resolve.go @@ -52,6 +52,25 @@ func ForStorage(appId string, input SecretInput, kc keychain.KeychainAccess) (Se return SecretInput{Ref: &SecretRef{Source: "keychain", ID: key}}, nil } +// ValidateSecretKeyMatch checks that the appSecret keychain key references the +// expected appId. This prevents silent mismatches when config.json is edited by +// hand (e.g. appId changed but appSecret.id still points to the old app). +// Only applicable when appSecret is a keychain SecretRef; other forms are skipped. +func ValidateSecretKeyMatch(appId string, secret SecretInput) error { + if secret.Ref == nil || secret.Ref.Source != "keychain" { + return nil + } + expected := secretAccountKey(appId) + if secret.Ref.ID != expected { + return fmt.Errorf( + "appSecret keychain key %q does not match appId %q (expected %q); "+ + "please run `lark-cli config init` to reconfigure", + secret.Ref.ID, appId, expected, + ) + } + return nil +} + // RemoveSecretStore cleans up keychain entries when an app is removed. // Errors are intentionally ignored — cleanup is best-effort. func RemoveSecretStore(input SecretInput, kc keychain.KeychainAccess) { diff --git a/internal/core/secret_resolve_test.go b/internal/core/secret_resolve_test.go new file mode 100644 index 00000000..79ec6b7f --- /dev/null +++ b/internal/core/secret_resolve_test.go @@ -0,0 +1,59 @@ +// Copyright (c) 2026 Lark Technologies Pte. Ltd. +// SPDX-License-Identifier: MIT + +package core + +import ( + "strings" + "testing" +) + +func TestValidateSecretKeyMatch_KeychainMatches(t *testing.T) { + secret := SecretInput{Ref: &SecretRef{Source: "keychain", ID: "appsecret:cli_abc123"}} + if err := ValidateSecretKeyMatch("cli_abc123", secret); err != nil { + t.Errorf("expected no error, got: %v", err) + } +} + +func TestValidateSecretKeyMatch_KeychainMismatch(t *testing.T) { + secret := SecretInput{Ref: &SecretRef{Source: "keychain", ID: "appsecret:cli_old_app"}} + err := ValidateSecretKeyMatch("cli_new_app", secret) + if err == nil { + t.Fatal("expected error for mismatched appId and keychain key") + } + // Verify the error message contains useful context + msg := err.Error() + for _, want := range []string{"cli_old_app", "cli_new_app", "appsecret:cli_new_app", "config init"} { + if !strings.Contains(msg, want) { + t.Errorf("error message missing %q: %s", want, msg) + } + } +} + +func TestValidateSecretKeyMatch_PlainSecret_Skipped(t *testing.T) { + secret := PlainSecret("some-secret") + if err := ValidateSecretKeyMatch("cli_abc123", secret); err != nil { + t.Errorf("plain secret should be skipped, got: %v", err) + } +} + +func TestValidateSecretKeyMatch_FileRef_Skipped(t *testing.T) { + secret := SecretInput{Ref: &SecretRef{Source: "file", ID: "/tmp/secret.txt"}} + if err := ValidateSecretKeyMatch("cli_abc123", secret); err != nil { + t.Errorf("file ref should be skipped, got: %v", err) + } +} + +func TestValidateSecretKeyMatch_ZeroValue_Skipped(t *testing.T) { + if err := ValidateSecretKeyMatch("cli_abc123", SecretInput{}); err != nil { + t.Errorf("zero SecretInput should be skipped, got: %v", err) + } +} + +func TestValidateSecretKeyMatch_EmptyAppId_Mismatch(t *testing.T) { + secret := SecretInput{Ref: &SecretRef{Source: "keychain", ID: "appsecret:cli_abc123"}} + err := ValidateSecretKeyMatch("", secret) + if err == nil { + t.Fatal("expected error when appId is empty but keychain key references a real app") + } +}