fix(config): validate appId and appSecret keychain key consistency#295
Open
maochengwei1024-create wants to merge 2 commits intomainfrom
Open
fix(config): validate appId and appSecret keychain key consistency#295maochengwei1024-create wants to merge 2 commits intomainfrom
maochengwei1024-create wants to merge 2 commits intomainfrom
Conversation
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) <noreply@anthropic.com>
📝 WalkthroughWalkthroughResolveConfigFromMulti now pre-validates keychain secret refs by calling ValidateSecretKeyMatch(app.AppId, app.AppSecret). If validation fails it returns a ConfigError (Code: 2, Type: Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
github-actions
bot
added
the
size/M
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@6403f24d1ff56344ac43b045e5b061aeff1f7103🧩 Skill updatenpx skills add larksuite/cli#fix/validate-appid-secret-key-match -y -g |
Greptile SummaryThis PR adds Confidence Score: 5/5Safe to merge; the change is a pure validation guard with no mutations to existing data paths. No P0 or P1 issues remain after accounting for the already-flagged Message/Hint reversal in the previous review thread. All new logic is well-tested and the happy path is unaffected. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[ResolveConfigFromMulti] --> B{app == nil?}
B -- yes --> C[Return profile-not-found ConfigError]
B -- no --> D{ValidateSecretKeyMatch}
D -- Ref is nil or\nnon-keychain source --> E[Skip — return nil]
D -- Ref.ID != appsecret:appId --> F[Return out-of-sync ConfigError\nHint: run lark-cli config init]
D -- Ref.ID == appsecret:appId --> G[ResolveSecretInput]
E --> G
G -- keychain error ExitError --> H[Return ExitError]
G -- other error --> I[Return ConfigError]
G -- success --> J[Build and return CliConfig]
Greploops — Automatically fix all review issues by running Reviews (2): Last reviewed commit: "fix: align ConfigError Message/Hint with..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
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) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/core/config_test.go`:
- Around line 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.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f6f3d8e8-9bc0-4e37-9c0b-f5ec54614375
📒 Files selected for processing (2)
internal/core/config.gointernal/core/config_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/core/config.go
Comment on lines
+88
to
+165
| 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") | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
ValidateSecretKeyMatchto detect whenappIdin config.json is out of sync with theappSecretkeychain reference key (e.g. after hand-editing config)ResolveConfigFromMultibefore any keychain lookup or OAPI request, failing fast with actionable error messageResolveConfigFromMultiTest plan
go test ./internal/core/— all tests pass, no regressionsconfig init+auth loginflow is unaffected🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests