fix(auth): prevent destructive refresh token races - #2147
Closed
buxibuxi wants to merge 3 commits into
Closed
Conversation
|
|
📝 WalkthroughWalkthroughUAT credential storage now uses injectable operations and platform-specific shared locks. Refresh failures preserve stored credentials, reuse concurrent refresh results, retry persistence, and return structured errors. Logout and identity diagnosis now handle storage failures explicitly. ChangesUAT refresh handling
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant UATClient
participant TokenStore
participant CredentialLock
participant RefreshEndpoint
UATClient->>TokenStore: load stored credentials
UATClient->>CredentialLock: acquire credential lock
CredentialLock->>TokenStore: reload stored credentials
UATClient->>RefreshEndpoint: send refresh request
RefreshEndpoint-->>UATClient: return refreshed credentials or error
UATClient->>TokenStore: preserve or persist credentials
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
the
size/L
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
internal/auth/uat_client.go (1)
250-260: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAdd a short delay before the single retry.
The retry runs immediately after a retryable authentication failure. A transient server error is usually not resolved within microseconds, so the immediate retry mostly wastes one round trip. Add a small fixed delay before the second
callEndpoint()call.♻️ Proposed refactor
fmt.Fprintf(errOut, "[lark-cli] [WARN] uat-client: refresh transient error (code=%d) for %s, retrying once\n", code, opts.UserOpenId) + time.Sleep(500 * time.Millisecond) data, err = callEndpoint()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/auth/uat_client.go` around lines 250 - 260, In the retry branch of the refresh flow around concurrentRefreshWinner, add a small fixed delay immediately before the second callEndpoint() invocation. Keep the existing single-retry behavior and error handling unchanged.internal/auth/uat_client_test.go (1)
244-249: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winDo not pass
http.DefaultClientto a test that must not perform I/O.The test relies on the local expiry check returning before any request. If that guard regresses, the test performs a real network call to the Feishu token endpoint. Pass a transport that fails the test when it is called. The assertion then also proves that no request was sent.
♻️ Proposed test change
- _, err := GetValidAccessToken(http.DefaultClient, UATCallOptions{ + client := &http.Client{Transport: uatRoundTripFunc(func(*http.Request) (*http.Response, error) { + t.Error("AC4: locally expired refresh token must not reach the token endpoint") + return nil, errors.New("unexpected request") + })} + + _, err := GetValidAccessToken(client, UATCallOptions{🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/auth/uat_client_test.go` around lines 244 - 249, Update the GetValidAccessToken test to use an HTTP client with a transport that immediately fails if invoked instead of http.DefaultClient. Keep the existing expired-token setup and assertions so the test verifies the local expiry guard returns without performing any network request.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/auth/uat_client_process_test.go`:
- Around line 102-113: Update the “hold” branch in refreshLockHelperCommand to
remove the fixed 500 ms sleep and wait for an explicit probe-completion signal
instead. Pass the probe result-file path through an additional environment
variable, then have the holder poll or otherwise wait until that file exists
after writing the marker before allowing defer lock.Unlock() to run.
- Around line 91-95: Update the process-test setup around refreshLockPath and
its lock usage to register test cleanup that removes the lock path and any
test-created filesystem state. Replace direct os.MkdirAll, os.Stat, os.ReadFile,
and os.WriteFile calls in this test flow with the corresponding internal/vfs
operations, preserving the existing test behavior across Darwin, Linux, and
Windows.
In `@internal/auth/uat_client_test.go`:
- Around line 220-223: Update the error-path test around errs.ProblemOf to
assert the problem’s category, subtype, and param metadata, and verify the
original transport error remains reachable through the preserved cause. Declare
the connection-reset transport error once at the test’s start and have the round
tripper return that same error, then assert cause preservation alongside the
existing network subtype check.
In `@internal/auth/uat_client.go`:
- Around line 319-328: Update concurrentRefreshWinner to detect a concurrent
refresh when either the refresh token or access token differs from attempted,
while retaining the existing nil and valid-status checks. This must recognize
successful writes where the server reuses the refresh token but produces a new
access token.
In `@internal/auth/uat_lock_path_windows.go`:
- Around line 13-22: Update refreshLockDir to obtain cache, home, and temporary
directories exclusively through internal/vfs, adding the missing UserCacheDir
and TempDir accessors there. Preserve the cache-then-home lookup order, and when
both fail use an absolute temporary-directory fallback before appending the
lark-cli credential-locks path; remove the relative ".lark-cli" fallback.
---
Nitpick comments:
In `@internal/auth/uat_client_test.go`:
- Around line 244-249: Update the GetValidAccessToken test to use an HTTP client
with a transport that immediately fails if invoked instead of
http.DefaultClient. Keep the existing expired-token setup and assertions so the
test verifies the local expiry guard returns without performing any network
request.
In `@internal/auth/uat_client.go`:
- Around line 250-260: In the retry branch of the refresh flow around
concurrentRefreshWinner, add a small fixed delay immediately before the second
callEndpoint() invocation. Keep the existing single-retry behavior and error
handling unchanged.
🪄 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 Plus
Run ID: 955c4785-85e7-4e95-a451-4ff8d1900cf3
📒 Files selected for processing (5)
internal/auth/uat_client.gointernal/auth/uat_client_process_test.gointernal/auth/uat_client_test.gointernal/auth/uat_lock_path_filesystem.gointernal/auth/uat_lock_path_windows.go
Comment on lines
+91
to
+95
| lockPath := refreshLockPath("cli_process_test", "ou_process_test") | ||
| lock := flock.New(lockPath) | ||
| if err := os.MkdirAll(filepath.Dir(lockPath), 0700); err != nil { | ||
| t.Fatal(err) | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Determine whether keychain.StorageDir follows HOME or another env override.
ast-grep run --pattern 'func StorageDir($$$) $$$' --lang go internal/keychain
rg -n 'HOME|UserHomeDir|os.Getenv' internal/keychain -C2Repository: larksuite/cli
Length of output: 7402
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- test file outline ---'
ast-grep outline internal/auth/uat_client_process_test.go
printf '%s\n' '--- focused test ranges ---'
cat -n internal/auth/uat_client_process_test.go | sed -n '1,150p'
printf '%s\n' '--- refreshLockPath definitions and usages ---'
rg -n -C4 'refreshLockPath|flock\.New|RemoveAll|os\.Remove|vfs\.' internal/auth internal/keychain
printf '%s\n' '--- UserHomeDir implementation ---'
rg -n -C8 'func UserHomeDir|UserHomeDir\(' internal/vfs internal | head -160
printf '%s\n' '--- platform-specific cache directory references ---'
rg -n -C4 'UserCacheDir|CacheDir|LARKSUITE_CLI_DATA_DIR|LARKSUITE_CLI_CONFIG_DIR' internal/auth internal/keychain internal/vfsRepository: larksuite/cli
Length of output: 46936
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- all refresh lock path implementations ---'
for f in $(rg -l 'func refreshLockDir' internal/auth); do
echo "--- $f"
cat -n "$f"
done
printf '%s\n' '--- lock dependency and cleanup references ---'
rg -n -C3 'gofrs/flock|TryLock|Unlock|refresh_.*\.lock' go.mod go.sum internal/auth
printf '%s\n' '--- auth test main ---'
cat -n internal/auth/testmain_test.goRepository: larksuite/cli
Length of output: 8940
Keep the process-test lock and filesystem operations test-scoped.
On Darwin and Linux, HOME points to t.TempDir() and LARKSUITE_CLI_DATA_DIR is removed, so keychain.StorageDir resolves under the temporary directory. On Windows, refreshLockDir uses os.UserCacheDir, so the lock can remain in the user's cache. Register cleanup for the lock path, and use internal/vfs instead of os.MkdirAll, os.Stat, os.ReadFile, and os.WriteFile.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/auth/uat_client_process_test.go` around lines 91 - 95, Update the
process-test setup around refreshLockPath and its lock usage to register test
cleanup that removes the lock path and any test-created filesystem state.
Replace direct os.MkdirAll, os.Stat, os.ReadFile, and os.WriteFile calls in this
test flow with the corresponding internal/vfs operations, preserving the
existing test behavior across Darwin, Linux, and Windows.
Source: Learnings
Comment on lines
+220
to
+223
| problem, ok := errs.ProblemOf(err) | ||
| if !ok || problem.Subtype != errs.SubtypeNetworkTransport { | ||
| t.Fatalf("AC3: problem = %#v, want network_transport", problem) | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Assert the error category and cause preservation.
The test checks only problem.Subtype. The coding guidelines require error-path tests to assert typed metadata including category, and to verify cause preservation. wrapRefreshTransportError attaches the transport error with WithCause, so the test can assert that the original connection reset error is still reachable.
💚 Proposed test change
+ transportErr := errors.New("connection reset")
problem, ok := errs.ProblemOf(err)
if !ok || problem.Subtype != errs.SubtypeNetworkTransport {
t.Fatalf("AC3: problem = %#v, want network_transport", problem)
}
+ if problem.Category != errs.CategoryNetwork {
+ t.Fatalf("AC3: category = %q, want %q", problem.Category, errs.CategoryNetwork)
+ }
+ if !errors.Is(err, transportErr) {
+ t.Fatalf("AC3: cause was not preserved: %v", err)
+ }Declare transportErr once at the top of the test and return it from the round tripper.
As per coding guidelines: "Error-path tests must assert typed metadata through errs.ProblemOf (category, subtype, and param) and verify cause preservation rather than relying only on message substrings."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/auth/uat_client_test.go` around lines 220 - 223, Update the
error-path test around errs.ProblemOf to assert the problem’s category, subtype,
and param metadata, and verify the original transport error remains reachable
through the preserved cause. Declare the connection-reset transport error once
at the test’s start and have the round tripper return that same error, then
assert cause preservation alongside the existing network subtype check.
Source: Coding guidelines
Comment on lines
+13
to
+22
| func refreshLockDir() string { | ||
| baseDir, err := os.UserCacheDir() | ||
| if err != nil || baseDir == "" { | ||
| baseDir, err = os.UserHomeDir() | ||
| if err != nil || baseDir == "" { | ||
| baseDir = ".lark-cli" | ||
| } | ||
| } | ||
| return filepath.Join(baseDir, "lark-cli", "credential-locks") | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check which user-directory accessors internal/vfs exposes.
rg -n 'UserHomeDir|UserCacheDir|TempDir' internal/vfsRepository: larksuite/cli
Length of output: 2483
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- target file ---'
cat -n internal/auth/uat_lock_path_windows.go
printf '%s\n' '--- vfs API and implementations ---'
cat -n internal/vfs/fs.go
cat -n internal/vfs/default.go
cat -n internal/vfs/osfs.go
printf '%s\n' '--- lock helper and callers ---'
rg -n -C 3 'refreshLockDir|credential-locks|uat_lock_path|refresh.*lock|lock.*refresh' --glob '*.go' .
printf '%s\n' '--- filesystem accessor conventions ---'
rg -n -C 2 'os\.(UserHomeDir|UserCacheDir|TempDir)|vfs\.(UserHomeDir|UserCacheDir|TempDir)' --glob '*.go' .
printf '%s\n' '--- relevant diff summary ---'
git diff --stat -- internal/auth/uat_lock_path_windows.go internal/vfsRepository: larksuite/cli
Length of output: 21063
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- target file ---'
cat -n internal/auth/uat_lock_path_windows.go
printf '%s\n' '--- vfs API and implementations ---'
cat -n internal/vfs/fs.go
cat -n internal/vfs/default.go
cat -n internal/vfs/osfs.go
printf '%s\n' '--- lock helper and callers ---'
rg -n -C 3 'refreshLockDir|credential-locks|uat_lock_path|refresh.*lock|lock.*refresh' --glob '*.go' .
printf '%s\n' '--- filesystem accessor conventions ---'
rg -n -C 2 'os\.(UserHomeDir|UserCacheDir|TempDir)|vfs\.(UserHomeDir|UserCacheDir|UserTempDir)' --glob '*.go' .
printf '%s\n' '--- relevant diff summary ---'
git diff --stat -- internal/auth/uat_lock_path_windows.go internal/vfsRepository: larksuite/cli
Length of output: 21063
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- FS implementations and test doubles ---'
rg -n -C 3 'type [A-Za-z0-9_]+ struct|type [A-Za-z0-9_]+ FS|DefaultFS\s*=|implements.*FS' internal/vfs internal --glob '*.go' \
| rg -n 'FS|DefaultFS|type .*struct|type .*FS' || true
printf '%s\n' '--- direct vfs.DefaultFS assignments ---'
rg -n -C 2 'vfs\.DefaultFS\s*=' --glob '*.go' .
printf '%s\n' '--- direct FS assignments and constructors ---'
rg -n -C 2 '(DefaultFS\s*=|FS\s*\{|FS:|New.*FS|mock.*FS|fake.*FS)' --glob '*.go' internal
printf '%s\n' '--- standalone Windows path probe ---'
node - <<'JS'
const path = require('path');
const suffix = ['lark-cli', 'credential-locks'];
const relative = path.win32.join('.lark-cli', ...suffix);
const fromOne = path.win32.resolve('C:\\work\\one', relative);
const fromTwo = path.win32.resolve('C:\\work\\two', relative);
console.log(JSON.stringify({
relative,
fromOne,
fromTwo,
samePath: fromOne === fromTwo,
isAbsoluteRelative: path.win32.isAbsolute(relative),
}));
JSRepository: larksuite/cli
Length of output: 41693
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- FS implementations and test doubles ---'
rg -n -C 3 'type [A-Za-z0-9_]+ struct|type [A-Za-z0-9_]+ FS|DefaultFS\s*=|implements.*FS' internal/vfs internal --glob '*.go' \
| rg -n 'FS|DefaultFS|type .*struct|type .*FS' || true
printf '%s\n' '--- direct vfs.DefaultFS assignments ---'
rg -n -C 2 'vfs\.DefaultFS\s*=' --glob '*.go' .
printf '%s\n' '--- direct FS assignments and constructors ---'
rg -n -C 2 '(DefaultFS\s*=|FS\s*\{|FS:|New.*FS|mock.*FS|fake.*FS)' --glob '*.go' internal
printf '%s\n' '--- standalone Windows path probe ---'
node - <<'JS'
const path = require('path');
const relative = path.win32.join('.lark-cli', 'lark-cli', 'credential-locks');
const fromOne = path.win32.resolve('C:\\work\\one', relative);
const fromTwo = path.win32.resolve('C:\\work\\two', relative);
console.log(JSON.stringify({
relative,
fromOne,
fromTwo,
samePath: fromOne === fromTwo,
isAbsoluteRelative: path.win32.isAbsolute(relative),
}));
JSRepository: larksuite/cli
Length of output: 41686
Use internal/vfs for all directory lookups and keep the fallback absolute.
internal/vfs exposes UserHomeDir() but not UserCacheDir() or TempDir(). Add the required accessors and use them here. If both cache and home lookups fail, use an absolute temporary-directory fallback. The current ".lark-cli" fallback creates different lock files for processes started from different working directories.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/auth/uat_lock_path_windows.go` around lines 13 - 22, Update
refreshLockDir to obtain cache, home, and temporary directories exclusively
through internal/vfs, adding the missing UserCacheDir and TempDir accessors
there. Preserve the cache-then-home lookup order, and when both fail use an
absolute temporary-directory fallback before appending the lark-cli
credential-locks path; remove the relative ".lark-cli" fallback.
Source: Coding guidelines
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/auth/uat_client_test.go (2)
124-161: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winGive
winnera distinct refresh token so the assertion detects a real replacement.
winnerat Line 127 reuses"old-refresh", the same refresh-token string asattemptedat Line 126. The check at Lines 157-160 only fails ifstored.RefreshTokendiffers fromwinner.RefreshToken. Since both tokens share the same string, this check would still pass even ifstore.replace(winner)never actually applied and the originalattemptedtoken remained stored. Only the access-token check at Lines 154-156 currently proves the winner's data reached the caller; the refresh-token invariant is not verified.Use a distinct value for the winner's refresh token, for example
"winner-refresh", so Line 158 genuinely verifies that the concurrently-stored winner (not the originalattemptedtoken) survived the reuse-detection path.🐛 Proposed fix
- winner := refreshTestToken("winner-access", "old-refresh", now.Add(time.Hour), now.Add(7*24*time.Hour)) + winner := refreshTestToken("winner-access", "winner-refresh", now.Add(time.Hour), now.Add(7*24*time.Hour))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/auth/uat_client_test.go` around lines 124 - 161, Update the winner fixture in TestRefreshReusedReturnsWinnerStoredByConcurrentProcess to use a refresh-token value distinct from attempted, such as “winner-refresh”, so the stored.RefreshToken assertion verifies that store.replace(winner) actually persisted the concurrent winner.
189-192: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAssert
Category(and cause, where applicable) alongsideSubtypein these error-path tests. Each of these tests validates a typed error only throughproblem.Subtypefromerrs.ProblemOf. The coding guideline for*_test.gofiles requires asserting typed metadata includingcategory, and verifying cause preservation, rather than relying on a single field.
internal/auth/uat_client_test.go#L189-L192: InTestRefreshRevokedPreservesUnexpiredStoredState, add aproblem.Categorycheck next to the existingSubtypecheck forerrs.SubtypeRefreshTokenRevoked.internal/auth/uat_client_test.go#L248-L251: InTestLocallyExpiredRefreshTokenIsPreserved, add aproblem.Categorycheck next to the existingSubtypecheck forerrs.SubtypeRefreshTokenExpired.internal/auth/uat_client_test.go#L272-L275: InTestCredentialStoreReadFailureIsNotReportedAsMissing, add aproblem.Categorycheck next to the existingSubtypecheck forerrs.SubtypeStorage, and verify the injectedloadErrcause remains reachable viaerrors.Is/errors.Assince it is returned unwrapped.internal/auth/uat_client_test.go#L299-L302: InTestRefreshPreflightPreventsRotationWhenStorageIsUnwritable, add aproblem.Categorycheck next to the existingSubtypecheck forerrs.SubtypeStorage, and verify the underlying "credential store unavailable" cause is preserved.As per coding guidelines: "Error-path tests must assert typed metadata through
errs.ProblemOf(category,subtype, andparam) and verify cause preservation rather than relying only on message substrings."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/auth/uat_client_test.go` around lines 189 - 192, Update internal/auth/uat_client_test.go at lines 189-192, 248-251, 272-275, and 299-302: in TestRefreshRevokedPreservesUnexpiredStoredState and TestLocallyExpiredRefreshTokenIsPreserved, assert the expected problem.Category alongside Subtype; in TestCredentialStoreReadFailureIsNotReportedAsMissing, also verify loadErr remains reachable via errors.Is/errors.As; in TestRefreshPreflightPreventsRotationWhenStorageIsUnwritable, verify the underlying credential-store-unavailable cause is preserved. Keep each test’s existing errs.ProblemOf validation and assert the typed error metadata required by the guideline.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/auth/uat_client_test.go`:
- Around line 342-422: Isolate the credential lock storage from the host
filesystem in TestLoginWaitsForInFlightRefreshAndWins and
TestLogoutWaitsForInFlightRefreshAndWins by setting LARKSUITE_CLI_DATA_DIR to a
unique t.TempDir() before invoking SetStoredToken or RemoveStoredToken. Ensure
the environment override is cleaned up with the test lifecycle, while preserving
the existing refresh-lock assertions.
---
Outside diff comments:
In `@internal/auth/uat_client_test.go`:
- Around line 124-161: Update the winner fixture in
TestRefreshReusedReturnsWinnerStoredByConcurrentProcess to use a refresh-token
value distinct from attempted, such as “winner-refresh”, so the
stored.RefreshToken assertion verifies that store.replace(winner) actually
persisted the concurrent winner.
- Around line 189-192: Update internal/auth/uat_client_test.go at lines 189-192,
248-251, 272-275, and 299-302: in
TestRefreshRevokedPreservesUnexpiredStoredState and
TestLocallyExpiredRefreshTokenIsPreserved, assert the expected problem.Category
alongside Subtype; in TestCredentialStoreReadFailureIsNotReportedAsMissing, also
verify loadErr remains reachable via errors.Is/errors.As; in
TestRefreshPreflightPreventsRotationWhenStorageIsUnwritable, verify the
underlying credential-store-unavailable cause is preserved. Keep each test’s
existing errs.ProblemOf validation and assert the typed error metadata required
by the guideline.
🪄 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 Plus
Run ID: a5b02be6-b8de-4e68-ae1b-d78041378ee2
📒 Files selected for processing (6)
internal/auth/credential_lock_windows.gointernal/auth/token_store.gointernal/auth/uat_client.gointernal/auth/uat_client_process_test.gointernal/auth/uat_client_test.gointernal/auth/uat_lock_path_filesystem.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/auth/uat_client_process_test.go
- internal/auth/uat_client.go
Comment on lines
+342
to
+422
| func TestLoginWaitsForInFlightRefreshAndWins(t *testing.T) { | ||
| now := time.Now() | ||
| attempted := refreshTestToken("expired-access", "old-refresh", now.Add(-time.Minute), now.Add(24*time.Hour)) | ||
| store := installUATStoreStub(t, attempted) | ||
| requestStarted := make(chan struct{}) | ||
| releaseRefresh := make(chan struct{}) | ||
| registry := successfulRefreshRegistry("rotated-access", "rotated-refresh", func(*http.Request) { | ||
| close(requestStarted) | ||
| <-releaseRefresh | ||
| }) | ||
| t.Cleanup(func() { registry.Verify(t) }) | ||
| refreshDone := make(chan error, 1) | ||
| go func() { | ||
| _, err := GetValidAccessToken(httpmock.NewClient(registry), UATCallOptions{ | ||
| UserOpenId: "ou_user", AppId: "cli_test", AppSecret: "secret", Domain: core.BrandFeishu, ErrOut: &bytes.Buffer{}, | ||
| }) | ||
| refreshDone <- err | ||
| }() | ||
| <-requestStarted | ||
|
|
||
| login := refreshTestToken("login-access", "login-refresh", now.Add(time.Hour), now.Add(7*24*time.Hour)) | ||
| login.GrantedAt = now.UnixMilli() | ||
| loginDone := make(chan error, 1) | ||
| go func() { loginDone <- SetStoredToken(login) }() | ||
| select { | ||
| case err := <-loginDone: | ||
| t.Fatalf("AC2: login bypassed in-flight refresh lock: %v", err) | ||
| case <-time.After(50 * time.Millisecond): | ||
| } | ||
| close(releaseRefresh) | ||
| if err := <-refreshDone; err != nil { | ||
| t.Fatalf("AC2: refresh failed: %v", err) | ||
| } | ||
| if err := <-loginDone; err != nil { | ||
| t.Fatalf("AC2: login persistence failed: %v", err) | ||
| } | ||
| stored, _ := store.snapshot() | ||
| if stored == nil || stored.RefreshToken != login.RefreshToken || stored.GrantedAt != login.GrantedAt { | ||
| t.Fatalf("AC2: stale refresh overwrote newer login: %#v", stored) | ||
| } | ||
| } | ||
|
|
||
| func TestLogoutWaitsForInFlightRefreshAndWins(t *testing.T) { | ||
| now := time.Now() | ||
| attempted := refreshTestToken("expired-access", "old-refresh", now.Add(-time.Minute), now.Add(24*time.Hour)) | ||
| store := installUATStoreStub(t, attempted) | ||
| requestStarted := make(chan struct{}) | ||
| releaseRefresh := make(chan struct{}) | ||
| registry := successfulRefreshRegistry("rotated-access", "rotated-refresh", func(*http.Request) { | ||
| close(requestStarted) | ||
| <-releaseRefresh | ||
| }) | ||
| t.Cleanup(func() { registry.Verify(t) }) | ||
| refreshDone := make(chan error, 1) | ||
| go func() { | ||
| _, err := GetValidAccessToken(httpmock.NewClient(registry), UATCallOptions{ | ||
| UserOpenId: "ou_user", AppId: "cli_test", AppSecret: "secret", Domain: core.BrandFeishu, ErrOut: &bytes.Buffer{}, | ||
| }) | ||
| refreshDone <- err | ||
| }() | ||
| <-requestStarted | ||
|
|
||
| logoutDone := make(chan error, 1) | ||
| go func() { logoutDone <- RemoveStoredToken("cli_test", "ou_user") }() | ||
| select { | ||
| case err := <-logoutDone: | ||
| t.Fatalf("AC2: logout bypassed in-flight refresh lock: %v", err) | ||
| case <-time.After(50 * time.Millisecond): | ||
| } | ||
| close(releaseRefresh) | ||
| if err := <-refreshDone; err != nil { | ||
| t.Fatalf("AC2: refresh failed: %v", err) | ||
| } | ||
| if err := <-logoutDone; err != nil { | ||
| t.Fatalf("AC2: logout failed: %v", err) | ||
| } | ||
| stored, _ := store.snapshot() | ||
| if stored != nil { | ||
| t.Fatalf("AC2: refresh recreated token after logout: %#v", stored) | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check whether uat_client_test.go isolates LARKSUITE_CLI_CONFIG_DIR,
# and whether the credential lock path depends on it.
set -euo pipefail
echo "== t.Setenv / LARKSUITE_CLI_CONFIG_DIR usage in the test file =="
rg -n 'LARKSUITE_CLI_CONFIG_DIR|t\.Setenv|TestMain' internal/auth/uat_client_test.go
echo "== Lock path resolution source =="
fd -a 'uat_lock_path_filesystem.go|credential_lock_windows.go' internal/auth | while IFS= read -r f; do
echo "--- $f ---"
rg -n 'LARKSUITE_CLI_CONFIG_DIR|ConfigDir|appdir\.' "$f"
done
echo "== withCredentialLock definition =="
ast-grep run --pattern 'func withCredentialLock($$$) $$$ { $$$ }' --lang go internal/authRepository: larksuite/cli
Length of output: 216
🏁 Script executed:
#!/bin/bash
# Description: Inspect internal/auth test setup and credential lock path definitions across the package.
set -euo pipefail
echo "== internal/auth Go test files =="
git ls-files 'internal/auth/*_test.go'
echo "== uat_client_test.go outline and relevant setenv/lck lines =="
wc -l internal/auth/uat_client_test.go || true
rg -n 'LARKSUITE_CLI_CONFIG_DIR|t\.Setenv|TestMain|withCredentialLock|SetStoredToken|RemoveStoredToken|GetValidAccessToken|installUATStoreStub' internal/auth/uat_client_test.go || true
echo "== token_store lock-related code =="
rg -n 'func (withCredentialLock|loadStoredUAToken|persistStoredUAToken|removeStoredUAToken|SetStoredToken|RemoveStoredToken|GetValidAccessToken)|credential|appdir|ConfigDir|LARKSUITE_CLI_CONFIG_DIR' internal/auth -g '*.go' | head -n 300
echo "== candidate credential lock files =="
git ls-files internal/auth | rg '(lock|credential|uat|token|appdir)' || true
echo "== read credential lock candidates in chunks =="
for f in $(git ls-files internal/auth | rg '(lock|credential|uat|token|appdir)'); do
echo "--- $f ($(wc -l < "$f" | tr -d ' ') lines) ---"
sed -n '1,220p' "$f"
doneRepository: larksuite/cli
Length of output: 30891
🏁 Script executed:
#!/bin/bash
# Description: Inspect internal/auth test setup and credential lock path definitions across the package.
set -euo pipefail
echo "== internal/auth Go test files =="
git ls-files 'internal/auth/*_test.go'
echo "== uat_client_test.go outline and relevant setenv/lck lines =="
wc -l internal/auth/uat_client_test.go || true
rg -n 'LARKSUITE_CLI_CONFIG_DIR|t\.Setenv|TestMain|withCredentialLock|SetStoredToken|RemoveStoredUAToken|GetValidAccessToken|installUATStoreStub' internal/auth/uat_client_test.go || true
echo "== token_store lock-related code =="
rg -n 'func (withCredentialLock|loadStoredUAToken|persistStoredUAToken|removeStoredUAToken|SetStoredToken|RemoveStoredToken|GetValidAccessToken)|credential|appdir|ConfigDir|LARKSUITE_CLI_CONFIG_DIR' internal/auth -g '*.go' | head -n 300
echo "== candidate credential lock files =="
git ls-files internal/auth | rg '(lock|credential|uat|token|appdir)' || true
echo "== read credential lock candidates in chunks =="
for f in $(git ls-files internal/auth | rg '(lock|credential|uat|token|appdir)'); do
echo "--- $f ($(wc -l < "$f" | tr -d ' ') lines) ---"
sed -n '1,220p' "$f"
doneRepository: larksuite/cli
Length of output: 30815
🏁 Script executed:
#!/bin/bash
# Description: Check whether an auth package TestMain sets LARKSUITE_CLI_CONFIG_DIR before these tests run.
set -euo pipefail
echo "== internal/auth TestMain definitions/usages =="
rg -n 'func TestMain|LARKSUITE_CLI_CONFIG_DIR' internal/auth -g '*_test.go'
echo "== internal/auth/testmain_test.go =="
sed -n '1,160p' internal/auth/testmain_test.go || true
echo "== keychain StorageDir implementation =="
git ls-files | rg '(^|/)keychain/.*\.go$|keychain'
rg -n 'func StorageDir|LarkCliService|RundirDir|StorageDir' -g '*.go' .Repository: larksuite/cli
Length of output: 5139
🏁 Script executed:
#!/bin/bash
# Description: Inspect exact keychain StorageDir source for all builds to determine whether LARKSUITE_CLI_CONFIG_DIR is used.
set -euo pipefail
for f in internal/keychain/default.go internal/keychain/keychain_darwin.go internal/keychain/keychain_windows.go internal/keychain/keychain_other.go; do
echo "--- $f ($(wc -l < "$f" | tr -d ' ') lines) ---"
sed -n '/func StorageDir/,/^}/p' "$f"
done
echo "== Read-only model of Linux/default StorageDir behavior for absence/presence of LARKSUITE_CLI_CONFIG_DIR =="
python3 - <<'PY'
from pathlib import Path
text = Path('internal/keychain/keychain_other.go').read_text()
start = text.index('func StorageDir(service string) string')
end = text.index('\n}', start) + 2
print(text[start:end])
PYRepository: larksuite/cli
Length of output: 1939
Isolate the credential lock path from the host storage location.
Linux builds route the credential lock through keychain.StorageDir(keychain.LarkCliService), which resolves to ~/.local/share/lark-cli/locks unless LARKSUITE_CLI_DATA_DIR is set. internal/auth/testmain_test.go only sets LARKSUITE_CLI_LOG_DIR, not LARKSUITE_CLI_DATA_DIR, so these tests still create/temp lock real host credential files. Set LARKSUITE_CLI_DATA_DIR to t.TempDir() for SetStoredToken/RemoveStoredToken tests, or stub the storage directory under test.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/auth/uat_client_test.go` around lines 342 - 422, Isolate the
credential lock storage from the host filesystem in
TestLoginWaitsForInFlightRefreshAndWins and
TestLogoutWaitsForInFlightRefreshAndWins by setting LARKSUITE_CLI_DATA_DIR to a
unique t.TempDir() before invoking SetStoredToken or RemoveStoredToken. Ensure
the environment override is cleaned up with the test lifecycle, while preserving
the existing refresh-lock assertions.
Source: Path instructions
github-actions
Bot
added
size/XL
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/auth/revoke.go`:
- Around line 86-104: Update RevokeAndRemoveStoredToken to capture the error
returned by RevokeToken instead of discarding it, while still always attempting
removeStoredUAToken. Return the revoke error when local removal succeeds; if
removal fails, preserve and return the removal error as the required
local-cleanup outcome.
🪄 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 Plus
Run ID: 17096654-8edc-4871-bbb8-ee8a1d3dbf1c
📒 Files selected for processing (8)
cmd/auth/logout.gointernal/auth/credential_lock_windows.gointernal/auth/revoke.gointernal/auth/uat_client.gointernal/auth/uat_client_process_test.gointernal/auth/uat_client_test.gointernal/identitydiag/diagnostics.gointernal/identitydiag/diagnostics_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/auth/credential_lock_windows.go
- internal/auth/uat_client.go
- internal/auth/uat_client_test.go
Comment on lines
+86
to
+104
| // RevokeAndRemoveStoredToken serializes remote revocation with local credential removal. | ||
| func RevokeAndRemoveStoredToken(httpClient *http.Client, appID, appSecret string, brand core.LarkBrand, userOpenID string) error { | ||
| return withCredentialLock(appID, userOpenID, func() error { | ||
| stored, err := loadStoredUAToken(appID, userOpenID) | ||
| if err == nil && stored != nil { | ||
| revokeToken := stored.RefreshToken | ||
| tokenTypeHint := "refresh_token" | ||
| if revokeToken == "" { | ||
| revokeToken = stored.AccessToken | ||
| tokenTypeHint = "access_token" | ||
| } | ||
| if revokeToken != "" { | ||
| _ = RevokeToken(httpClient, appID, appSecret, brand, revokeToken, tokenTypeHint) | ||
| } | ||
| } | ||
| return removeStoredUAToken(appID, userOpenID) | ||
| }) | ||
| } | ||
|
|
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Surface the discarded RevokeToken error instead of dropping it.
Line 98 discards the result of RevokeToken with _ =. If remote revocation fails (network error, server rejects the token, revoked already), RevokeAndRemoveStoredToken still returns nil as long as removeStoredUAToken succeeds. The caller in cmd/auth/logout.go only prints its warning when this function returns a non-nil error, so a failed remote revocation produces no diagnostic output. The user believes the token is revoked, but the token can remain valid on the server with no local trace of the failure.
Preserve local removal as the required outcome, but do not discard the revoke error. Return it when removal succeeds so the existing warning path in cmd/auth/logout.go reports it.
🛠️ Proposed fix to surface the revoke error
func RevokeAndRemoveStoredToken(httpClient *http.Client, appID, appSecret string, brand core.LarkBrand, userOpenID string) error {
return withCredentialLock(appID, userOpenID, func() error {
+ var revokeErr error
stored, err := loadStoredUAToken(appID, userOpenID)
if err == nil && stored != nil {
revokeToken := stored.RefreshToken
tokenTypeHint := "refresh_token"
if revokeToken == "" {
revokeToken = stored.AccessToken
tokenTypeHint = "access_token"
}
if revokeToken != "" {
- _ = RevokeToken(httpClient, appID, appSecret, brand, revokeToken, tokenTypeHint)
+ revokeErr = RevokeToken(httpClient, appID, appSecret, brand, revokeToken, tokenTypeHint)
}
}
- return removeStoredUAToken(appID, userOpenID)
+ if removeErr := removeStoredUAToken(appID, userOpenID); removeErr != nil {
+ return removeErr
+ }
+ return revokeErr
})
}📝 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
| // RevokeAndRemoveStoredToken serializes remote revocation with local credential removal. | |
| func RevokeAndRemoveStoredToken(httpClient *http.Client, appID, appSecret string, brand core.LarkBrand, userOpenID string) error { | |
| return withCredentialLock(appID, userOpenID, func() error { | |
| stored, err := loadStoredUAToken(appID, userOpenID) | |
| if err == nil && stored != nil { | |
| revokeToken := stored.RefreshToken | |
| tokenTypeHint := "refresh_token" | |
| if revokeToken == "" { | |
| revokeToken = stored.AccessToken | |
| tokenTypeHint = "access_token" | |
| } | |
| if revokeToken != "" { | |
| _ = RevokeToken(httpClient, appID, appSecret, brand, revokeToken, tokenTypeHint) | |
| } | |
| } | |
| return removeStoredUAToken(appID, userOpenID) | |
| }) | |
| } | |
| // RevokeAndRemoveStoredToken serializes remote revocation with local credential removal. | |
| func RevokeAndRemoveStoredToken(httpClient *http.Client, appID, appSecret string, brand core.LarkBrand, userOpenID string) error { | |
| return withCredentialLock(appID, userOpenID, func() error { | |
| var revokeErr error | |
| stored, err := loadStoredUAToken(appID, userOpenID) | |
| if err == nil && stored != nil { | |
| revokeToken := stored.RefreshToken | |
| tokenTypeHint := "refresh_token" | |
| if revokeToken == "" { | |
| revokeToken = stored.AccessToken | |
| tokenTypeHint = "access_token" | |
| } | |
| if revokeToken != "" { | |
| revokeErr = RevokeToken(httpClient, appID, appSecret, brand, revokeToken, tokenTypeHint) | |
| } | |
| } | |
| if removeErr := removeStoredUAToken(appID, userOpenID); removeErr != nil { | |
| return removeErr | |
| } | |
| return revokeErr | |
| }) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/auth/revoke.go` around lines 86 - 104, Update
RevokeAndRemoveStoredToken to capture the error returned by RevokeToken instead
of discarding it, while still always attempting removeStoredUAToken. Return the
revoke error when local removal succeeds; if removal fails, preserve and return
the removal error as the required local-cleanup outcome.
2 tasks
Author
|
Closing this upstream contribution because the requested remediation is being kept local; no contributor agreement action is needed. |
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
2 participants
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
Prevent concurrent lark-cli processes from invalidating rotated Feishu refresh tokens, deleting newly stored credentials, or misreporting credential-store failures as missing authorization. All login, refresh, revoke, and logout transitions now serialize on one user-scoped credential lock.
Changes
Test Plan
make unit-testgo test -race ./internal/auth ./cmd/auth ./internal/identitydiag -count=1go test ./internal/auth -run TestRefreshRotatesOnceAcrossConfigIsolatedProcesses -count=5go vet ./internal/auth ./cmd/auth ./internal/identitydiagGOOS=windows GOARCH=amd64 go test -c ./internal/authlark-cli auth status --json --verifycalls returned user/ready/validRelated Issues
Compatibility