From 0799875744b55153c05b77b3d1847ff4641edea7 Mon Sep 17 00:00:00 2001 From: "maochengwei.1024" Date: Tue, 7 Apr 2026 19:57:40 +0800 Subject: [PATCH 1/2] fix(config): validate appId and appSecret keychain key consistency When config.json is hand-edited, the appId field can become out of sync with the appSecret keychain reference (e.g. appId changed but appSecret.id still points to the old app). This causes silent auth failures at API call time. Add a pre-flight check in ResolveConfigFromMulti that compares the two before any keychain lookup or OAPI request, failing fast with actionable guidance. Change-Id: I371b7da2fef402d9feec93ef9588136841674eff Co-Authored-By: Claude Opus 4.6 (1M context) --- internal/core/config.go | 5 ++ internal/core/config_test.go | 91 ++++++++++++++++++++++++++++ internal/core/secret_resolve.go | 19 ++++++ internal/core/secret_resolve_test.go | 59 ++++++++++++++++++ 4 files changed, 174 insertions(+) create mode 100644 internal/core/secret_resolve_test.go diff --git a/internal/core/config.go b/internal/core/config.go index 410c3e01..ba479b76 100644 --- a/internal/core/config.go +++ b/internal/core/config.go @@ -240,6 +240,11 @@ 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: err.Error(), + Hint: "the appId and appSecret in config.json are out of sync"} + } + 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..a9fabd52 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.Hint == "the appId and appSecret in config.json 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") + } +} From 6403f24d1ff56344ac43b045e5b061aeff1f7103 Mon Sep 17 00:00:00 2001 From: "maochengwei.1024" Date: Tue, 7 Apr 2026 20:09:21 +0800 Subject: [PATCH 2/2] fix: align ConfigError Message/Hint with codebase convention Per review feedback: Message should be a concise problem label, Hint should contain the detailed remediation. Swap the two fields so the mismatch error follows the same pattern as other ConfigErrors. Change-Id: Ifdbf8adc8be6952c18e4fb9dde760ccd728ef4de Co-Authored-By: Claude Opus 4.6 (1M context) --- internal/core/config.go | 5 +++-- internal/core/config_test.go | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/internal/core/config.go b/internal/core/config.go index ba479b76..ddd428aa 100644 --- a/internal/core/config.go +++ b/internal/core/config.go @@ -241,8 +241,9 @@ 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: err.Error(), - Hint: "the appId and appSecret in config.json are out of sync"} + 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) diff --git a/internal/core/config_test.go b/internal/core/config_test.go index a9fabd52..55fbcb83 100644 --- a/internal/core/config_test.go +++ b/internal/core/config_test.go @@ -158,7 +158,7 @@ func TestResolveConfigFromMulti_MatchingKeychainRefPassesValidation(t *testing.T // The error should come from keychain resolution, NOT from our mismatch check. var cfgErr *ConfigError if errors.As(err, &cfgErr) { - if cfgErr.Hint == "the appId and appSecret in config.json are out of sync" { + if cfgErr.Message == "appId and appSecret keychain key are out of sync" { t.Fatal("error came from mismatch check, but keys should match") } }