fix: harden auth check and config removal#125
fix: harden auth check and config removal#125zfaustk wants to merge 10 commits intolarksuite:mainfrom
Conversation
Fixes larksuite#116 by rejecting empty --scope values in auth check instead of returning a misleading ok response. Fixes larksuite#114 by saving an empty config before best-effort secret/token cleanup, and adds focused regression tests for both flows.
|
|
HanShaoshuai-k
left a comment
HanShaoshuai-k
left a comment
There was a problem hiding this comment.
Suggestions
1. Package-level mutable vars don't follow project conventions (remove.go:16-21) — Blocking
The project consistently uses Factory / TestFactory for dependency injection — HTTP via httpmock.Registry, keychain via noopKeychain, IO via buffer replacement. No other test uses package-level var swapping.
Suggest moving these deps into ConfigRemoveOptions or Factory to stay consistent. The current approach also risks data races if t.Parallel() is ever added.
2. Token cleanup errors silently discarded (remove.go:56) — Medium
_ = removeStoredToken(app.AppId, user.UserOpenId)If save succeeds but token cleanup fails, orphaned keychain entries remain with no CLI path to clean them. Suggest logging a warning on failure.
3. CLA not signed — Blocking
Please sign the Contributor License Agreement before we can merge.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR addresses two validation and order-of-operations bugs: Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
cmd/auth/check_test.go (1)
16-19: Consider adding a test case for literal empty string.The current test uses
" "(whitespace-only). Whilestrings.Fieldshandles both identically, testing""as well would document both edge cases explicitly and guard against future refactoring that might change the parsing logic.🧪 Optional: Add table-driven test for both edge cases
func TestAuthCheckRun_EmptyScopeReturnsValidationError(t *testing.T) { - f, stdout, _, _ := cmdutil.TestFactory(t, nil) - - err := authCheckRun(&CheckOptions{ - Factory: f, - Scope: " ", - }) - if err == nil { - t.Fatal("expected validation error") - } - if !strings.Contains(err.Error(), "--scope cannot be empty") { - t.Fatalf("unexpected error: %v", err) - } - if stdout.Len() != 0 { - t.Fatalf("expected no stdout, got %q", stdout.String()) + cases := []struct { + name string + scope string + }{ + {"empty string", ""}, + {"whitespace only", " "}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + f, stdout, _, _ := cmdutil.TestFactory(t, nil) + err := authCheckRun(&CheckOptions{ + Factory: f, + Scope: tc.scope, + }) + if err == nil { + t.Fatal("expected validation error") + } + if !strings.Contains(err.Error(), "--scope cannot be empty") { + t.Fatalf("unexpected error: %v", err) + } + if stdout.Len() != 0 { + t.Fatalf("expected no stdout, got %q", stdout.String()) + } + }) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/auth/check_test.go` around lines 16 - 19, Add an explicit test for the literal empty string alongside the whitespace-only test: call authCheckRun(&CheckOptions{Factory: f, Scope: ""}) and assert the same behavior as the existing `" "` case; you can also convert the test into a small table-driven loop over inputs `""` and `" "` to avoid duplication. Target the existing test that invokes authCheckRun and uses CheckOptions.Scope so the new case is exercised by the same assertions.cmd/config/remove_test.go (1)
83-122: Consider making the save-before-cleanup assertion order-explicit.Current checks prove save and cleanup both happened, but not strict sequencing. Recording call order would make the regression test stricter.
Example tightening
savedEmpty := false + callOrder := []string{} saveConfig := func(next *core.MultiAppConfig) error { + callOrder = append(callOrder, "save") if len(next.Apps) != 0 { t.Fatalf("expected empty config, got %+v", next.Apps) } savedEmpty = true return nil } @@ removeSecret := func(input core.SecretInput, kc keychain.KeychainAccess) { + callOrder = append(callOrder, "remove_secret") if input.Ref != nil { secretRemovals = append(secretRemovals, input.Ref.ID) } } @@ removeStoredToken := func(appID, userOpenID string) error { + callOrder = append(callOrder, "remove_token") tokenRemovals = append(tokenRemovals, appID+":"+userOpenID) return nil } @@ if !savedEmpty { t.Fatal("expected empty config to be saved before cleanup") } + if len(callOrder) == 0 || callOrder[0] != "save" { + t.Fatalf("expected save to happen first, order=%v", callOrder) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/config/remove_test.go` around lines 83 - 122, The test currently verifies that saveConfig ran and that removeSecret/removeStoredToken performed removals but doesn't assert the sequence; modify the test to record call order (e.g., append markers to a slice) inside saveConfig, removeSecret, and removeStoredToken and then after configRemoveRun assert the order contains the save marker before any cleanup markers; update the closures for saveConfig, removeSecret, and removeStoredToken to push distinct tokens (like "save", "secret:<id>", "token:<app:user>") into a shared slice and assert that the first occurrence of "save" precedes the first cleanup token to guarantee save-before-cleanup ordering for ConfigRemoveOptions/configRemoveRun.cmd/config/remove.go (1)
50-53: Add defensive guards for injected callbacks to avoid nil-function panics.With the new injection pattern, a partially initialized
ConfigRemoveOptionscan panic at runtime. A fast-fail internal error is safer.Suggested hardening
func configRemoveRun(opts *ConfigRemoveOptions) error { + if opts == nil || opts.Factory == nil || + opts.LoadConfig == nil || opts.SaveConfig == nil || + opts.RemoveSecret == nil || opts.RemoveStoredToken == nil { + return output.Errorf(output.ExitInternal, "internal", "config remove options not initialized") + } f := opts.Factory config, err := opts.LoadConfig()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/config/remove.go` around lines 50 - 53, The function configRemoveRun should defensively validate the incoming opts and any injected function fields on ConfigRemoveOptions before calling them: first check opts != nil, then ensure opts.LoadConfig and any other function-typed callback fields (the injected callbacks on ConfigRemoveOptions, e.g., any PreRemoveHook/PostRemoveHook/Validate callbacks present in the struct) are non-nil and return a clear internal error if any are nil; perform these guards at the top of configRemoveRun so callers fail fast instead of panicking when invoking opts.LoadConfig or other callbacks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@cmd/auth/check_test.go`:
- Around line 16-19: Add an explicit test for the literal empty string alongside
the whitespace-only test: call authCheckRun(&CheckOptions{Factory: f, Scope:
""}) and assert the same behavior as the existing `" "` case; you can also
convert the test into a small table-driven loop over inputs `""` and `" "` to
avoid duplication. Target the existing test that invokes authCheckRun and uses
CheckOptions.Scope so the new case is exercised by the same assertions.
In `@cmd/config/remove_test.go`:
- Around line 83-122: The test currently verifies that saveConfig ran and that
removeSecret/removeStoredToken performed removals but doesn't assert the
sequence; modify the test to record call order (e.g., append markers to a slice)
inside saveConfig, removeSecret, and removeStoredToken and then after
configRemoveRun assert the order contains the save marker before any cleanup
markers; update the closures for saveConfig, removeSecret, and removeStoredToken
to push distinct tokens (like "save", "secret:<id>", "token:<app:user>") into a
shared slice and assert that the first occurrence of "save" precedes the first
cleanup token to guarantee save-before-cleanup ordering for
ConfigRemoveOptions/configRemoveRun.
In `@cmd/config/remove.go`:
- Around line 50-53: The function configRemoveRun should defensively validate
the incoming opts and any injected function fields on ConfigRemoveOptions before
calling them: first check opts != nil, then ensure opts.LoadConfig and any other
function-typed callback fields (the injected callbacks on ConfigRemoveOptions,
e.g., any PreRemoveHook/PostRemoveHook/Validate callbacks present in the struct)
are non-nil and return a clear internal error if any are nil; perform these
guards at the top of configRemoveRun so callers fail fast instead of panicking
when invoking opts.LoadConfig or other callbacks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e843c71d-7dd9-48e8-81e8-2c9dee89fe34
📒 Files selected for processing (4)
cmd/auth/check.gocmd/auth/check_test.gocmd/config/remove.gocmd/config/remove_test.go
Greptile SummaryThis PR fixes two real bugs — the silent
Confidence Score: 2/5Not safe to merge — two P0 build-breaking file placement errors must be resolved first. The intended logic is sound, but the accidental rename of cmd/config/remove.go to a file literally named "undefined" at the repo root, plus remove_test.go landing at root instead of cmd/config/, means the entire config package fails to compile. These are hard blockers before any of the correct bug-fix logic can ship. undefined (must be restored to cmd/config/remove.go) and remove_test.go (must be moved to cmd/config/remove_test.go) Important Files Changed
Sequence DiagramsequenceDiagram
participant CLI as config remove (CLI)
participant SaveConfig
participant Keychain as RemoveSecret / RemoveStoredToken
CLI->>SaveConfig: SaveConfig(empty config)
alt SaveConfig fails
SaveConfig-->>CLI: error
CLI-->>CLI: return error (secrets untouched)
else SaveConfig succeeds
SaveConfig-->>CLI: nil
loop for each app
CLI->>Keychain: RemoveSecret(app.AppSecret)
loop for each user
CLI->>Keychain: RemoveStoredToken(appId, userId)
alt token removal fails
Keychain-->>CLI: error
CLI-->>CLI: warn to stderr (continue)
end
end
end
CLI-->>CLI: print "Configuration removed"
end
|
|
Addressed the requested changes in the latest commit (
Could you please take another look when you have time? |
github-actions
bot
added
the
size/L
|
@zfaustk please resolve the conflict to merge |
|
Resolved the merge conflict and refreshed the branch on the latest head ( This keeps the scoped follow-up only: empty/whitespace |
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 `@cmd/config/remove_test.go`:
- Around line 16-80: Add a new test that covers the failure path of SaveConfig
to ensure RemoveSecret and RemoveStoredToken are not invoked when saving fails:
duplicate the pattern from
TestConfigRemoveRun_UsesInjectedCallbacksAndSavesBeforeCleanup but make
SaveConfig return a non-nil error (and record callOrder), provide stub
RemoveSecret and RemoveStoredToken that append to slices or callOrder if
invoked, call configRemoveRun with these injected callbacks, assert
configRemoveRun returns the SaveConfig error, verify callOrder contains "save"
as first entry and that no "secret" or "token" entries were appended, and check
stderr contains an appropriate failure message; reference the existing test name
and the configRemoveRun, SaveConfig, RemoveSecret, RemoveStoredToken symbols to
place this new test alongside the current one.
🪄 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: 749106fb-076b-48e7-8b30-939970f65973
📒 Files selected for processing (3)
cmd/auth/check_test.gocmd/config/remove.gocmd/config/remove_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- cmd/auth/check_test.go
- cmd/config/remove.go
| func TestConfigRemoveRun_UsesInjectedCallbacksAndSavesBeforeCleanup(t *testing.T) { | ||
| f, _, stderr, _ := cmdutil.TestFactory(t, nil) | ||
|
|
||
| cfg := &core.MultiAppConfig{ | ||
| Apps: []core.AppConfig{ | ||
| { | ||
| AppId: "cli_app_a", | ||
| AppSecret: core.SecretInput{Ref: &core.SecretRef{Source: "keychain", ID: "appsecret:cli_app_a"}}, | ||
| Users: []core.AppUser{{UserOpenId: "ou_a"}}, | ||
| }, | ||
| { | ||
| AppId: "cli_app_b", | ||
| AppSecret: core.SecretInput{Ref: &core.SecretRef{Source: "keychain", ID: "appsecret:cli_app_b"}}, | ||
| Users: []core.AppUser{{UserOpenId: "ou_b1"}, {UserOpenId: "ou_b2"}}, | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| callOrder := []string{} | ||
| saveConfig := func(next *core.MultiAppConfig) error { | ||
| callOrder = append(callOrder, "save") | ||
| if len(next.Apps) != 0 { | ||
| t.Fatalf("expected empty config, got %+v", next.Apps) | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| var secretRemovals []string | ||
| removeSecret := func(input core.SecretInput, kc keychain.KeychainAccess) { | ||
| callOrder = append(callOrder, "secret") | ||
| if input.Ref != nil { | ||
| secretRemovals = append(secretRemovals, input.Ref.ID) | ||
| } | ||
| } | ||
|
|
||
| var tokenRemovals []string | ||
| removeStoredToken := func(appID, userOpenID string) error { | ||
| callOrder = append(callOrder, "token") | ||
| tokenRemovals = append(tokenRemovals, appID+":"+userOpenID) | ||
| return nil | ||
| } | ||
|
|
||
| if err := configRemoveRun(&ConfigRemoveOptions{ | ||
| Factory: f, | ||
| LoadConfig: func() (*core.MultiAppConfig, error) { return cfg, nil }, | ||
| SaveConfig: saveConfig, | ||
| RemoveSecret: removeSecret, | ||
| RemoveStoredToken: removeStoredToken, | ||
| }); err != nil { | ||
| t.Fatalf("unexpected error: %v", err) | ||
| } | ||
|
|
||
| if len(callOrder) == 0 || callOrder[0] != "save" { | ||
| t.Fatalf("expected save to happen first, order=%v", callOrder) | ||
| } | ||
| if len(secretRemovals) != 2 { | ||
| t.Fatalf("secret removals = %v, want 2 entries", secretRemovals) | ||
| } | ||
| if len(tokenRemovals) != 3 { | ||
| t.Fatalf("token removals = %v, want 3 entries", tokenRemovals) | ||
| } | ||
| if got := stderr.String(); !strings.Contains(got, "Configuration removed") { | ||
| t.Fatalf("expected success message on stderr, got %q", got) | ||
| } | ||
| } |
There was a problem hiding this comment.
Missing regression coverage for failed SaveConfig cleanup guard
The success-path ordering check is good, but this suite still doesn’t assert the critical failure invariant: if SaveConfig fails, RemoveSecret and RemoveStoredToken must not run. That behavior is central to issue #114 and should be covered explicitly.
Suggested test to add
+func TestConfigRemoveRun_SaveFailureDoesNotClearSecretsOrTokens(t *testing.T) {
+ f, _, _, _ := cmdutil.TestFactory(t, nil)
+
+ cleanupSecretCalls := 0
+ cleanupTokenCalls := 0
+
+ err := configRemoveRun(&ConfigRemoveOptions{
+ Factory: f,
+ LoadConfig: func() (*core.MultiAppConfig, error) {
+ return &core.MultiAppConfig{
+ Apps: []core.AppConfig{{
+ AppId: "cli_app",
+ AppSecret: core.SecretInput{Ref: &core.SecretRef{Source: "keychain", ID: "appsecret:cli_app"}},
+ Users: []core.AppUser{{UserOpenId: "ou_1"}},
+ }},
+ }, nil
+ },
+ SaveConfig: func(*core.MultiAppConfig) error { return errors.New("disk write failed") },
+ RemoveSecret: func(core.SecretInput, keychain.KeychainAccess) {
+ cleanupSecretCalls++
+ },
+ RemoveStoredToken: func(string, string) error {
+ cleanupTokenCalls++
+ return nil
+ },
+ })
+
+ if err == nil {
+ t.Fatal("expected save failure")
+ }
+ if cleanupSecretCalls != 0 || cleanupTokenCalls != 0 {
+ t.Fatalf("cleanup must not run when save fails; secret=%d token=%d", cleanupSecretCalls, cleanupTokenCalls)
+ }
+}📝 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.
| func TestConfigRemoveRun_UsesInjectedCallbacksAndSavesBeforeCleanup(t *testing.T) { | |
| f, _, stderr, _ := cmdutil.TestFactory(t, nil) | |
| cfg := &core.MultiAppConfig{ | |
| Apps: []core.AppConfig{ | |
| { | |
| AppId: "cli_app_a", | |
| AppSecret: core.SecretInput{Ref: &core.SecretRef{Source: "keychain", ID: "appsecret:cli_app_a"}}, | |
| Users: []core.AppUser{{UserOpenId: "ou_a"}}, | |
| }, | |
| { | |
| AppId: "cli_app_b", | |
| AppSecret: core.SecretInput{Ref: &core.SecretRef{Source: "keychain", ID: "appsecret:cli_app_b"}}, | |
| Users: []core.AppUser{{UserOpenId: "ou_b1"}, {UserOpenId: "ou_b2"}}, | |
| }, | |
| }, | |
| } | |
| callOrder := []string{} | |
| saveConfig := func(next *core.MultiAppConfig) error { | |
| callOrder = append(callOrder, "save") | |
| if len(next.Apps) != 0 { | |
| t.Fatalf("expected empty config, got %+v", next.Apps) | |
| } | |
| return nil | |
| } | |
| var secretRemovals []string | |
| removeSecret := func(input core.SecretInput, kc keychain.KeychainAccess) { | |
| callOrder = append(callOrder, "secret") | |
| if input.Ref != nil { | |
| secretRemovals = append(secretRemovals, input.Ref.ID) | |
| } | |
| } | |
| var tokenRemovals []string | |
| removeStoredToken := func(appID, userOpenID string) error { | |
| callOrder = append(callOrder, "token") | |
| tokenRemovals = append(tokenRemovals, appID+":"+userOpenID) | |
| return nil | |
| } | |
| if err := configRemoveRun(&ConfigRemoveOptions{ | |
| Factory: f, | |
| LoadConfig: func() (*core.MultiAppConfig, error) { return cfg, nil }, | |
| SaveConfig: saveConfig, | |
| RemoveSecret: removeSecret, | |
| RemoveStoredToken: removeStoredToken, | |
| }); err != nil { | |
| t.Fatalf("unexpected error: %v", err) | |
| } | |
| if len(callOrder) == 0 || callOrder[0] != "save" { | |
| t.Fatalf("expected save to happen first, order=%v", callOrder) | |
| } | |
| if len(secretRemovals) != 2 { | |
| t.Fatalf("secret removals = %v, want 2 entries", secretRemovals) | |
| } | |
| if len(tokenRemovals) != 3 { | |
| t.Fatalf("token removals = %v, want 3 entries", tokenRemovals) | |
| } | |
| if got := stderr.String(); !strings.Contains(got, "Configuration removed") { | |
| t.Fatalf("expected success message on stderr, got %q", got) | |
| } | |
| } | |
| func TestConfigRemoveRun_UsesInjectedCallbacksAndSavesBeforeCleanup(t *testing.T) { | |
| f, _, stderr, _ := cmdutil.TestFactory(t, nil) | |
| cfg := &core.MultiAppConfig{ | |
| Apps: []core.AppConfig{ | |
| { | |
| AppId: "cli_app_a", | |
| AppSecret: core.SecretInput{Ref: &core.SecretRef{Source: "keychain", ID: "appsecret:cli_app_a"}}, | |
| Users: []core.AppUser{{UserOpenId: "ou_a"}}, | |
| }, | |
| { | |
| AppId: "cli_app_b", | |
| AppSecret: core.SecretInput{Ref: &core.SecretRef{Source: "keychain", ID: "appsecret:cli_app_b"}}, | |
| Users: []core.AppUser{{UserOpenId: "ou_b1"}, {UserOpenId: "ou_b2"}}, | |
| }, | |
| }, | |
| } | |
| callOrder := []string{} | |
| saveConfig := func(next *core.MultiAppConfig) error { | |
| callOrder = append(callOrder, "save") | |
| if len(next.Apps) != 0 { | |
| t.Fatalf("expected empty config, got %+v", next.Apps) | |
| } | |
| return nil | |
| } | |
| var secretRemovals []string | |
| removeSecret := func(input core.SecretInput, kc keychain.KeychainAccess) { | |
| callOrder = append(callOrder, "secret") | |
| if input.Ref != nil { | |
| secretRemovals = append(secretRemovals, input.Ref.ID) | |
| } | |
| } | |
| var tokenRemovals []string | |
| removeStoredToken := func(appID, userOpenID string) error { | |
| callOrder = append(callOrder, "token") | |
| tokenRemovals = append(tokenRemovals, appID+":"+userOpenID) | |
| return nil | |
| } | |
| if err := configRemoveRun(&ConfigRemoveOptions{ | |
| Factory: f, | |
| LoadConfig: func() (*core.MultiAppConfig, error) { return cfg, nil }, | |
| SaveConfig: saveConfig, | |
| RemoveSecret: removeSecret, | |
| RemoveStoredToken: removeStoredToken, | |
| }); err != nil { | |
| t.Fatalf("unexpected error: %v", err) | |
| } | |
| if len(callOrder) == 0 || callOrder[0] != "save" { | |
| t.Fatalf("expected save to happen first, order=%v", callOrder) | |
| } | |
| if len(secretRemovals) != 2 { | |
| t.Fatalf("secret removals = %v, want 2 entries", secretRemovals) | |
| } | |
| if len(tokenRemovals) != 3 { | |
| t.Fatalf("token removals = %v, want 3 entries", tokenRemovals) | |
| } | |
| if got := stderr.String(); !strings.Contains(got, "Configuration removed") { | |
| t.Fatalf("expected success message on stderr, got %q", got) | |
| } | |
| } | |
| func TestConfigRemoveRun_SaveFailureDoesNotClearSecretsOrTokens(t *testing.T) { | |
| f, _, _, _ := cmdutil.TestFactory(t, nil) | |
| cleanupSecretCalls := 0 | |
| cleanupTokenCalls := 0 | |
| err := configRemoveRun(&ConfigRemoveOptions{ | |
| Factory: f, | |
| LoadConfig: func() (*core.MultiAppConfig, error) { | |
| return &core.MultiAppConfig{ | |
| Apps: []core.AppConfig{{ | |
| AppId: "cli_app", | |
| AppSecret: core.SecretInput{Ref: &core.SecretRef{Source: "keychain", ID: "appsecret:cli_app"}}, | |
| Users: []core.AppUser{{UserOpenId: "ou_1"}}, | |
| }}, | |
| }, nil | |
| }, | |
| SaveConfig: func(*core.MultiAppConfig) error { return errors.New("disk write failed") }, | |
| RemoveSecret: func(core.SecretInput, keychain.KeychainAccess) { | |
| cleanupSecretCalls++ | |
| }, | |
| RemoveStoredToken: func(string, string) error { | |
| cleanupTokenCalls++ | |
| return nil | |
| }, | |
| }) | |
| if err == nil { | |
| t.Fatal("expected save failure") | |
| } | |
| if cleanupSecretCalls != 0 || cleanupTokenCalls != 0 { | |
| t.Fatalf("cleanup must not run when save fails; secret=%d token=%d", cleanupSecretCalls, cleanupTokenCalls) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/config/remove_test.go` around lines 16 - 80, Add a new test that covers
the failure path of SaveConfig to ensure RemoveSecret and RemoveStoredToken are
not invoked when saving fails: duplicate the pattern from
TestConfigRemoveRun_UsesInjectedCallbacksAndSavesBeforeCleanup but make
SaveConfig return a non-nil error (and record callOrder), provide stub
RemoveSecret and RemoveStoredToken that append to slices or callOrder if
invoked, call configRemoveRun with these injected callbacks, assert
configRemoveRun returns the SaveConfig error, verify callOrder contains "save"
as first entry and that no "secret" or "token" entries were appended, and check
stderr contains an appropriate failure message; reference the existing test name
and the configRemoveRun, SaveConfig, RemoveSecret, RemoveStoredToken symbols to
place this new test alongside the current one.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/config/remove.go`:
- Around line 67-73: Resolve the unresolved merge conflict markers by removing
the conflict lines and keeping the fix branch change that calls
opts.SaveConfig(empty) (i.e., use ConfigRemoveOptions' method SaveConfig)
instead of core.SaveMultiAppConfig; delete the <<<<<<<, =======, and >>>>>>>
markers and ensure the conditional error check remains using
opts.SaveConfig(empty) with the existing output.Errorf call so the code compiles
and uses the injected SaveConfig on ConfigRemoveOptions.
- Around line 79-86: Remove the git conflict markers and restore the warning-log
behavior from the fix branch: replace the discarded call to
auth.RemoveStoredToken with the opts.RemoveStoredToken call that logs failures
using fmt.Fprintf to f.IOStreams.ErrOut; ensure the block uses if err :=
opts.RemoveStoredToken(app.AppId, user.UserOpenId); err != nil {
fmt.Fprintf(f.IOStreams.ErrOut, "warning: failed to remove stored token for app
%q user %q: %v\n", app.AppId, user.UserOpenId, err) } and remove the <<<<<<<,
=======, >>>>>>> markers so the file compiles.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
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 `@cmd/config/remove.go`:
- Around line 75-77: The warning currently prints a persistent user identifier
(user.UserOpenId) to stderr via fmt.Fprintf in the RemoveStoredToken error path;
change the message to avoid exposing the raw identifier by either omitting
user.UserOpenId entirely or printing a redacted/truncated form (e.g., show only
a fixed prefix/suffix or replace with "<redacted>") while keeping app.AppId and
the error; update the call that writes to f.IOStreams.ErrOut (the fmt.Fprintf
invocation) so it no longer emits the full user.UserOpenId.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
|
Addressed the remaining review concern in the latest two commits.\n\n- removed the raw value from the token-cleanup warning\n- kept the warning actionable by retaining the app id and underlying error\n- updated so the warning assertion no longer expects or allows the user identifier to appear\n\nAt this point the code-side cleanup for this review thread should be complete; the remaining blocker should be maintainer re-review and workflow approval.\n |
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 `@undefined`:
- Around line 75-77: The warning path around opts.RemoveStoredToken(app.AppId,
user.UserOpenId) can leave the final footer "Cleared tokens for %d users"
inaccurate; update the logic to track removal outcomes (e.g., increment success
and failure counters when RemoveStoredToken returns nil vs error) and use those
counters in the final summary so it prints a partial-success message like
"Cleared tokens for X users; failed to clear Y users" (and keep writing warnings
to f.IOStreams.ErrOut as now) instead of always claiming all tokens were
cleared.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| if err := opts.RemoveStoredToken(app.AppId, user.UserOpenId); err != nil { | ||
| fmt.Fprintf(f.IOStreams.ErrOut, "warning: failed to remove a stored token for app %q: %v\n", app.AppId, err) | ||
| } |
There was a problem hiding this comment.
Don’t claim all tokens were cleared after warning paths.
After making RemoveStoredToken failures non-fatal here, the later Cleared tokens for %d users footer can be inaccurate whenever any warning is emitted. Track failed removals and make the summary reflect partial success.
✏️ Possible tweak
+ failedTokenRemovals := 0
+
// Clean up keychain entries for all apps after config is cleared.
for _, app := range config.Apps {
opts.RemoveSecret(app.AppSecret, f.Keychain)
for _, user := range app.Users {
if err := opts.RemoveStoredToken(app.AppId, user.UserOpenId); err != nil {
+ failedTokenRemovals++
fmt.Fprintf(f.IOStreams.ErrOut, "warning: failed to remove a stored token for app %q: %v\n", app.AppId, err)
}
}
}
@@
if userCount > 0 {
- fmt.Fprintf(f.IOStreams.ErrOut, "Cleared tokens for %d users\n", userCount)
+ if failedTokenRemovals == 0 {
+ fmt.Fprintf(f.IOStreams.ErrOut, "Cleared tokens for %d users\n", userCount)
+ } else {
+ fmt.Fprintf(f.IOStreams.ErrOut, "Attempted to clear tokens for %d users (%d failed)\n", userCount, failedTokenRemovals)
+ }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@undefined` around lines 75 - 77, The warning path around
opts.RemoveStoredToken(app.AppId, user.UserOpenId) can leave the final footer
"Cleared tokens for %d users" inaccurate; update the logic to track removal
outcomes (e.g., increment success and failure counters when RemoveStoredToken
returns nil vs error) and use those counters in the final summary so it prints a
partial-success message like "Cleared tokens for X users; failed to clear Y
users" (and keep writing warnings to f.IOStreams.ErrOut as now) instead of
always claiming all tokens were cleared.
| // Copyright (c) 2026 Lark Technologies Pte. Ltd. | ||
| // SPDX-License-Identifier: MIT | ||
|
|
||
| package config | ||
|
|
||
| import ( |
There was a problem hiding this comment.
This file declares package config but lives at the repository root alongside main.go (package main). Go does not allow two package names in the same directory even for _test.go files unless the test package suffix (_test) is used. Running any root-level go test or build will produce a "found packages main and config" error.
This file should be placed in cmd/config/remove_test.go (next to cmd/config/remove.go) so it belongs to the correct package directory.
|
Follow-up clarification: the latest commits update cmd/config/remove.go and cmd/config/remove_test.go. The warning now avoids printing the raw user identifier, and the focused test now checks that the identifier is not leaked. |
4 participants
Summary
auth checkconfig removeFixes #116
Fixes #114
Why
auth check --scope ""currently returns{"ok":true}even though nothing was actually checked. That can silently greenlight automation when a dynamically built scope string collapses to empty.config removecurrently clears keychain/token state before persisting the empty config. If the config write fails, the CLI is left in an inconsistent state:config.jsonstill points at the old app/user records, but the secret/token material is already gone.Testing
TestAuthCheckRun_EmptyScopeReturnsValidationErrorTestConfigRemoveRun_SaveFailureDoesNotClearSecretsOrTokensTestConfigRemoveRun_ClearsSecretsAndTokensAfterSuccessfulSaveCGO_ENABLED=0 /ROOM/.toolchains/go1.24.13/bin/go test ./cmd/auth ./cmd/configin this container, but the local Go build/test process did not complete within the timeout window, so I am not claiming a full localgo testpass from this environmentSummary by CodeRabbit
Bug Fixes
Tests