Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions internal/core/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
91 changes: 91 additions & 0 deletions internal/core/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"),
Expand Down Expand Up @@ -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")
}
}
}
Comment on lines +88 to +165
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Add config-dir isolation in each new test.

Please set LARKSUITE_CLI_CONFIG_DIR to a temp dir at the start of these tests to avoid accidental coupling with host/local config state.

🔧 Suggested patch
 func TestResolveConfigFromMulti_RejectsSecretKeyMismatch(t *testing.T) {
+	t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir())
 	raw := &MultiAppConfig{
 		Apps: []AppConfig{
 			{
@@
 func TestResolveConfigFromMulti_AcceptsPlainSecret(t *testing.T) {
+	t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir())
 	raw := &MultiAppConfig{
 		Apps: []AppConfig{
 			{
@@
 func TestResolveConfigFromMulti_MatchingKeychainRefPassesValidation(t *testing.T) {
+	t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir())
 	// Keychain ref matches appId, so validation passes.
 	// The subsequent ResolveSecretInput will fail (no real keychain),

As per coding guidelines **/*_test.go: "Isolate config state in Go tests by using t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir())."

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
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_RejectsSecretKeyMismatch(t *testing.T) {
t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir())
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) {
t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir())
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) {
t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir())
// 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")
}
}
}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@internal/core/config_test.go` around lines 88 - 165, These tests leak host
config state; add isolation by calling t.Setenv("LARKSUITE_CLI_CONFIG_DIR",
t.TempDir()) at the start of each impacted test
(TestResolveConfigFromMulti_RejectsSecretKeyMismatch,
TestResolveConfigFromMulti_AcceptsPlainSecret,
TestResolveConfigFromMulti_MatchingKeychainRefPassesValidation) so each test
uses a fresh temp config dir before invoking ResolveConfigFromMulti or
stubKeychain; place the call as the first statement in each test to ensure
config-dir isolation.


func TestResolveConfigFromMulti_DoesNotUseEnvProfileFallback(t *testing.T) {
t.Setenv("LARKSUITE_CLI_PROFILE", "missing")

Expand Down
19 changes: 19 additions & 0 deletions internal/core/secret_resolve.go
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
59 changes: 59 additions & 0 deletions internal/core/secret_resolve_test.go
Original file line number Diff line number Diff line change
@@ -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")
}
}
Loading